bcm2708: remove NEED_MACH_GPIO_H #495

jfasch · 2014-01-14T03:42:07Z

A recent commit of mine, f04ff75, removes the macro gpio_to_irq() from mach/gpio.h. Digging through the whole mess of the various gpio.h incarnations, I found that it's probably best to rename that file to gpio_irq.h, and to remove NEED_MACH_GPIO_H from the architecture.

See http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/118375.html which led me to the point that this has to go.

Conflicts: arch/arm/mach-bcm2708/bcm2708.c

people who upgrade from earlier version may still use the *ex* gpio_to_irq() of *ex* <mach/gpio.h>. in order to alert them to the bogusness of the situation, re-add <mach/gpio.> - for the sole purpose of placing a #error in it which clarifies what's to be done.

notro · 2014-01-14T14:15:51Z

What kind of problem are trying to solve here?

jfasch · 2014-01-14T14:55:18Z

See #389 for the origin of all the trouble.

User msperl had the problem that gpio_to_irq() did not work in board setup code. Including <mach/gpio.h> before <linux/gpio.h> solved the problem for him and the issue was quiet since then. I used similar code in my board setup (also having a mcp2515, curiously). using the wrong gpio_to_irq() (that from <linux/gpio.h> and not from <mach/gpio.h>) leaves me with an early kernel crash, seeing only the 4-color test screen that the bootloader brings. After hours of trying I figured out what the problem was.

I suspect me and msperl are not the only ones trying to wire SPI devices on the Raspberry, so I first cleaned up that gpio_to_irq() misnomer in <mach/gpio.h>. Naively, I have to admit. When bringing my code forward to rpi-3.12.y, I came across that NEED_MACH_GPIO_H thing which is defined there but not in rpi-3.11.y nor rpi-3.10.y. Things broke again because <linux/gpio.h>, err <asm/gpio.h>, err <asm-generic/gpio.h>, include <mach/gpio.h> in turn - apparently expecting a gpio_to_irq() macro to be defined there which I had removed. Headache.

Digging, I found arch/arm/Kconfig say

config NEED_MACH_GPIO_H
    bool
    help
      Select this when mach/gpio.h is required to provide special
      definitions for this platform. The need for mach/gpio.h should
      be avoided when possible.

Took the "should be avoided when possible" literally, and that's what this pull request is about. I tested /sys/class/gpio functionality, as well as the "framework" gpio_to_irq() by inserting a call and debug output in a random (well, mcp251x.c) driver.

notro · 2014-01-14T19:30:36Z

I really don't understand why you needed to change the gpio_to_irq macro in your previous pull request.

So I did a test, building a kernel from rpi-3.12.y going back before your commit:

git checkout 7b658f544fc60a2b3549cfce57a12343b005ce5f

Verify that we have the right version

$ grep gpio_to_irq arch/arm/mach-bcm2708/include/mach/gpio.h
#define gpio_to_irq(x)  ((x) + GPIO_IRQ_START)

Then I added the code for MCP2515 (from #389)

diff --git a/arch/arm/mach-bcm2708/bcm2708.c b/arch/arm/mach-bcm2708/bcm2708.c
index 4ab2609..dacb705 100644
--- a/arch/arm/mach-bcm2708/bcm2708.c
+++ b/arch/arm/mach-bcm2708/bcm2708.c
@@ -64,6 +64,11 @@
 #include <linux/broadcom/vc_cma.h>
 #endif

+#include <linux/can/platform/mcp251x.h>
+#include <linux/gpio.h>
+#include <linux/irq.h>
+
+#define MCP2515_CAN_INT_GPIO_PIN 25

 /* Effectively we have an IOMMU (ARM<->VideoCore map) that is set up to
  * give us IO access only to 64Mbytes of physical memory (26 bits).  We could
@@ -550,16 +555,23 @@ static struct platform_device bcm2708_spi_device = {
                .coherent_dma_mask = DMA_BIT_MASK(DMA_MASK_BITS_COMMON)},
 };

+static struct mcp251x_platform_data mcp251x_info = {
+       .oscillator_frequency   = 16000000,
+};
+
 #ifdef CONFIG_BCM2708_SPIDEV
 static struct spi_board_info bcm2708_spi_devices[] = {
-#ifdef CONFIG_SPI_SPIDEV
        {
-               .modalias = "spidev",
-               .max_speed_hz = 500000,
+               .modalias = "mcp2515",
+               .max_speed_hz = 1000000,
+               .platform_data = &mcp251x_info,
                .bus_num = 0,
                .chip_select = 0,
                .mode = SPI_MODE_0,
-       }, {
+               .irq= gpio_to_irq(MCP2515_CAN_INT_GPIO_PIN),
+       }
+#ifdef CONFIG_SPI_SPIDEV
+       ,{
                .modalias = "spidev",
                .max_speed_hz = 500000,
                .bus_num = 0,

config and build

make bcmrpi_defconfig
make

The kernel booted fine.

$ cat /sys/bus/spi/devices/spi0.0/modalias
spi:mcp2515

Then I wanted to see what was done with the gpio_to_irq macro:

make arch/arm/mach-bcm2708/bcm2708.i

bcm2708.i

static struct spi_board_info bcm2708_spi_devices[] = {
 {
  .modalias = "mcp2515",
  .max_speed_hz = 1000000,
  .platform_data = &mcp251x_info,
  .bus_num = 0,
  .chip_select = 0,
  .mode = (0|0),
  .irq= ((25) + ((64 + 21) + (64 + 21))),
 }

What did you do that gave you problems?

Edit:
Just for the record:

$ uname -a
Linux raspberrypi 3.12.7+ #1 PREEMPT Tue Jan 14 20:03:32 CET 2014 armv6l GNU/Linux

jfasch · 2014-01-15T08:39:25Z

On rpi-3.10.y, where I the commit is from, there is gpio_to_irq() in mach/gpio.h, as a macro suitable for use in, for example, spi_board_info. linux/gpio.h brings its own definition of gpio_to_irq() (as a function) because NEED_MACH_GPIO_H is not set for the bcm2708 platform. To fix this, I renamed gpio_to_irq() in mach/gpio.h to __bcm2708_gpio_to_irq(). Now that I have more understanding, I'd say the proper fix would have simply been to define NEED_MACH_GPIO_H for the platform, so linux/gpio.h took the definition of gpio_to_irq() right from mach/gpio.h.

On rpi-3.12.y, NEED_MACH_GPIO_H is defined. linux/gpio.h sees mach/gpio.h and you get gpio_to_irq() from there. Provided that it is there. My commit had removed it, so things work before that commit, and don't afterwards.

So: I'd propose to revert commit f04ff75 and ignore my pull request, and pull #497 (reversal) instead.

popcornmix · 2014-01-15T15:50:21Z

@jfasch would you suggest adding NEED_MACH_GPIO_H to 3.10.y?

jfasch · 2014-01-15T16:23:24Z

Yes, this would be the least confusing fix for the original issue.

There are rumours though that NEED_MACH_GPIO_H prevents devicetree from happening. I don't understand the details, but I sure understand that per-platform macro definitions don't make the kernel platform independent.

So, given that NEED_MACH_GPIO_H is the way to go:

On 3.10.y, revert dee3db1, define NEED_MACH_GPIO_H, and all is fine.
On 3.11.y, revert e598700 (same patch), NEED_MACH_GPIO_H is already defined there.
Likewise on 3.12.y, revert f04ff75 (same patch; pull request Reverting mach/gpio.h:gpio_to_irq() removal #497).

I can do that, together with a bit of testing (hints appreciated), over the weekend if you want.
Btw, how did NEED_MACH_GPIO_H make it into 3.11.y and 3.12.y?

popcornmix · 2014-01-15T16:26:25Z

Btw, how did NEED_MACH_GPIO_H make it into 3.11.y and 3.12.y?

I assume there was a build error without it, but can't remember the exact details.
Testing of the suggested changes would be appreciated. (check everything builds and boots is the first step).

jfasch · 2014-01-15T16:33:01Z

Ok.

notro · 2014-01-15T19:18:28Z

This is the commit that introduced NEED_MACH_GPIO_H: 0146422

It is apparently needed to make gpio_to_irq work in the board setup code. Drivers work fine without it.
So I agree that adding it to rpi-3.10.y would be the proper thing to do.

jfasch · 2014-01-19T19:00:07Z

@notro, @popcornmix: here's the pull requests I promised to create.

rpi-3.10.y: Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #505
rpi-3.11.y: Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #506
rpi-3.12.y: Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #507

Each of them

boots
boots with board code using gpio_to_irq() from linux/gpio.h
driver using gpio_to_irq() from linux/gpio.h works

jfasch added 2 commits January 14, 2014 04:26

bcm2708: remove NEED_MACH_GPIO_H

573ba30

Conflicts: arch/arm/mach-bcm2708/bcm2708.c

jfasch mentioned this pull request Jan 15, 2014

Reverting mach/gpio.h:gpio_to_irq() removal #497

Closed

jfasch closed this Jan 19, 2014

jfasch deleted the rpi-rm-mach-gpio branch January 19, 2014 18:08

This was referenced Jan 19, 2014

Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #505

Merged

Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #506

Merged

Please pull: NEED_MACH_GPIO_H vs. gpio_to_irq() woes #507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bcm2708: remove NEED_MACH_GPIO_H #495

bcm2708: remove NEED_MACH_GPIO_H #495

Uh oh!

jfasch commented Jan 14, 2014

Uh oh!

notro commented Jan 14, 2014

Uh oh!

jfasch commented Jan 14, 2014

Uh oh!

notro commented Jan 14, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

popcornmix commented Jan 15, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

popcornmix commented Jan 15, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

notro commented Jan 15, 2014

Uh oh!

jfasch commented Jan 19, 2014

Uh oh!

Uh oh!

bcm2708: remove NEED_MACH_GPIO_H #495

bcm2708: remove NEED_MACH_GPIO_H #495

Uh oh!

Conversation

jfasch commented Jan 14, 2014

Uh oh!

notro commented Jan 14, 2014

Uh oh!

jfasch commented Jan 14, 2014

Uh oh!

notro commented Jan 14, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

popcornmix commented Jan 15, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

popcornmix commented Jan 15, 2014

Uh oh!

jfasch commented Jan 15, 2014

Uh oh!

notro commented Jan 15, 2014

Uh oh!

jfasch commented Jan 19, 2014

Uh oh!

Uh oh!