rpi-3.11.y - use of gpio_to_irq hangs kernel

[msperl]

Hi!

When modifying the SPI section of arch/arm/mach-bcm2708/bcm2708.c
and then adding an interrupt "translation" from GPIO PIN to interrupt like this:

diff --git a/arch/arm/mach-bcm2708/bcm2708.c b/arch/arm/mach-bcm2708/bcm2708.c
index 3fe7626..0c30461 100644
--- a/arch/arm/mach-bcm2708/bcm2708.c
+++ b/arch/arm/mach-bcm2708/bcm2708.c
@@ -54,6 +54,12 @@
 #include <mach/vcio.h>
 #include <mach/system.h>

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

 #include "bcm2708.h"
@@ -546,16 +552,26 @@ static struct platform_device bcm2708_spi_device = {
        .resource = bcm2708_spi_resources,
 };

+static struct mcp251x_platform_data mcp251x_info = {
+       .oscillator_frequency   = 16000000,
+       .board_specific_setup   = NULL,
+       .irq_flags              = IRQF_TRIGGER_FALLING|IRQF_ONESHOT,
+       .power_enable           = NULL,
+       .transceiver_enable     = NULL,
+};
+
 #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,
-       }, {
+       }
+#ifdef CONFIG_SPI_SPIDEV
+       ,{
                .modalias = "spidev",
                .max_speed_hz = 500000,
                .bus_num = 0,
@@ -690,6 +706,17 @@ static void bcm2708_power_off(void)
        }
 }

+static void bcm2708_mcp251x_init(void) {
+       bcm2708_spi_devices[0].irq =
+#if 1
+               gpio_to_irq(MCP2515_CAN_INT_GPIO_PIN);
+#else
+       195;
+#endif
+       printk(KERN_INFO " BCM2708 mcp251x_init:  got IRQ %d for MCP2515\n", bcm
+       return;
+};
+
 void __init bcm2708_init(void)
 {
        int i;
@@ -747,6 +774,7 @@ void __init bcm2708_init(void)
        system_serial_low = serial;

 #ifdef CONFIG_BCM2708_SPIDEV
+       bcm2708_mcp251x_init();
        spi_register_board_info(bcm2708_spi_devices,
                        ARRAY_SIZE(bcm2708_spi_devices));
 #endif 

then the Kernel does not boot (not a single byte shows up on the serial console) - I have to take the SD card and I have to copy the emergency kernel to the running kernel to make the RPI boot again.

On the other hand: the SAME piece of config/code works with the 3.6.y kernel branch and has been recommended in several "recipes" to configure SPI devices...

The work-around it to use the hardcoded-interrupt number instead, which is not an optimal solution(see the other branch of "#if 1") .

Is there any "better" option than hardcoding the irq line?

Thanks,
Martin

[popcornmix]

Looking at the code:

#define HARD_IRQS         (64 + 21)
#define FIQ_IRQS              (64 + 21)
#define GPIO_IRQ_START        (HARD_IRQS + FIQ_IRQS)
#define gpio_to_irq(x)  ((x) + GPIO_IRQ_START)

so gpio_to_irq(MCP2515_CAN_INT_GPIO_PIN) = 25 + 64+21 + 64+21 = 195
which should be the same as the your working code.

Does this work with 3.10.y kernel?
Can you add a printk of gpio_to_irq(MCP2515_CAN_INT_GPIO_PIN) to this function (in the working form) and report what it is?

[msperl]

The problem is that in the current code the kernel does not even PRINT anything on the serial console - I think it gets stuck prior to the usart being configured for output...

So any additional printk will not work - and there is already a printk after the assign...

I believe just adding a line: "gpio_to_irq(MCP2515_CAN_INT_GPIO_PIN)" in the function bcm2708_init will do the same...

I have been compiling it as is - and the objdump of the object file shows that:
a) the function in question (bcm2708_mcp251x_init) is inlined automatically
b) the gpio_to_irq is a function call - not a macro!!!

So there is no macro that handles that!

Ciao,
Martin

P.s: here the assembler code for the section showing that it is an actual call to the function __gpio_to_irq:

 254:   e3a00019        mov     r0, #25
 258:   e9930006        ldmib   r3, {r1, r2}
 25c:   e59f307c        ldr     r3, [pc, #124]  ; 2e0 <bcm2708_init+0x1c0>
 260:   e5831000        str     r1, [r3]
 264:   e59f3078        ldr     r3, [pc, #120]  ; 2e4 <bcm2708_init+0x1c4>
 268:   e5832000        str     r2, [r3]
 26c:   ebfffffe        bl      0 <__gpio_to_irq>
 270:   e1a03000        mov     r3, r0
 274:   e1a01000        mov     r1, r0
 278:   e50436d0        str     r3, [r4, #-1744]        ; 0x6d0
 27c:   e59f0064        ldr     r0, [pc, #100]  ; 2e8 <bcm2708_init+0x1c8>
 280:   ebfffffe        bl      0 <printk>

[msperl]

One observation:
the macros you mention are in <mach/gpio.h> and when I include this instead of <linux/gpio.h> the code works.
Strangely it was working fine with <linux/gpio.h> using the 3.6.y branch and earlier.

That is quite a confusing and dangerous change that got introduced somewhere after 3.6.y...

(and if you look here you find a very old recipe stating <linux/gpio.h> with kernel 3.2.27+: http://lnxpps.de/rpie/)

[popcornmix]

So if you include
#include <mach/gpio.h>
it works, and if you include something else you get the bad function call?

[msperl]

yes - the strange thing is that

#include <linux/gpio.h>

was working with 3.6.y and earlier - so suspecting a change there is not very obvious...
Maybe some structure is not initialized (yet) that is needed by this function due to some rearranging the code and change in sequence?

For me it is solved - but you may want to look at the "bigger picture" to find other issues that may be related...

[msperl]

So I went back to a backup with the 3.6.y branch and looked at the generated object file there (compiled on the 25th march) and found that the assembly code directly loads 195 into a register - so it is using <mach/gpio.h>!

 264:   e3a030c3        mov     r3, #195        ; 0xc3
 268:   e1a01003        mov     r1, r3
 26c:   e582c000        str     ip, [r2]
 270:   e50437a0        str     r3, [r4, #-1952]        ; 0x7a0
 274:   ebfffffe        bl      0 <printk>

so something must have changed on the include structure so that <linux/gpio.h> does no longer include <mach/gpio.h> directly or indirectly...

So there must be some other traps lurking as well in other places as that is probably not the only thing that has changed...

[notro]

There is a change in arch/arm/asm/gpio.h between 3.6 and 3.7 with regards to mach/gpio.h and gpio_to_irq.
3.6 includes mach/gpio.h, but 3.7 and later doesn't, because NEED_MACH_GPIO_H is not set.
This results in:
3.6: #define gpio_to_irq(x) ((x) + GPIO_IRQ_START)
3.11: #define gpio_to_irq __gpio_to_irq

This is 3.6: http://lxr.free-electrons.com/source/arch/arm/include/asm/gpio.h?v=3.6;a=arm

#include <mach/gpio.h>

#ifndef gpio_to_irq
#define gpio_to_irq     __gpio_to_irq
#endif

This is 3.7: http://lxr.free-electrons.com/source/arch/arm/include/asm/gpio.h?v=3.7;a=arm

#ifdef CONFIG_NEED_MACH_GPIO_H
#include <mach/gpio.h>
#endif

#ifndef gpio_to_irq
#define gpio_to_irq     __gpio_to_irq
#endif

Commit: 0146422

linux/gpio.h includes arch/arm/asm/gpio.h because ARCH_HAVE_CUSTOM_GPIO_H is set, which is set because ARM is set.

[notro]

To finish up in the 3.11 case: A call to gpio_to_irq will always return -ENXIO (-6).

Edit: I'm wrong in this use case. Since the driver is not loaded yet, it will return -EINVAL (-22).

http://lxr.free-electrons.com/source/drivers/gpio/gpiolib.c?a=arm#L1970

/**
 * __gpio_to_irq() - return the IRQ corresponding to a GPIO
 * @gpio: gpio whose IRQ will be returned (already requested)
 * Context: any
 *
 * This is used directly or indirectly to implement gpio_to_irq().
 * It returns the number of the IRQ signaled by this (input) GPIO,
 * or a negative errno.
 */
static int gpiod_to_irq(const struct gpio_desc *desc)
{
        struct gpio_chip        *chip;
        int                     offset;

        if (!desc)
                return -EINVAL;
        chip = desc->chip;
        offset = gpio_chip_hwgpio(desc);
        return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
}

int __gpio_to_irq(unsigned gpio)
{
        return gpiod_to_irq(gpio_to_desc(gpio));
}
EXPORT_SYMBOL_GPL(__gpio_to_irq);

This is because to_irq is not implemented in the BCM2708 gpio driver:

https://github.com/raspberrypi/linux/blob/rpi-3.11.y/arch/arm/mach-bcm2708/bcm2708_gpio.c#L281

    ucb->gc.label = "bcm2708_gpio";
    ucb->gc.base = 0;
    ucb->gc.ngpio = ARCH_NR_GPIOS;
    ucb->gc.owner = THIS_MODULE;

    ucb->gc.direction_input = bcm2708_gpio_dir_in;
    ucb->gc.direction_output = bcm2708_gpio_dir_out;
    ucb->gc.get = bcm2708_gpio_get;
    ucb->gc.set = bcm2708_gpio_set;
    ucb->gc.can_sleep = 0;

[msperl]

Notro: to thanks for investigating.

Still the question is: why does it only fail while running the board config - it works correctly after a boot when loading a module via modprobe - see spi-config as an example...

This means that at some point in time during the boot the value must be set...

So for now the workaround is to use the Mach include in bcm2708.c - or better still: use spi-config to define spi-settings on the fly...

Martin

[notro]

Still the question is: why does it only fail while running the board config - it works correctly after a boot when loading a module via modprobe - see spi-config as an example...

I don't have an answer to this. It's rather strange. Have you checked that gpio_to_irq actually compiles to a function call in spi-config.c?

So for now the workaround is to use the Mach include in bcm2708.c

I rather think the solution would be to change arch/arm/Kconfig as was done in the commit I mentioned.

 config ARCH_BCM2708
    bool "Broadcom BCM2708 family"
    select CPU_V6
    select ARM_AMBA
    select HAVE_CLK
    select HAVE_SCHED_CLOCK
+   select NEED_MACH_GPIO_H
    select NEED_MACH_MEMORY_H
    select CLKDEV_LOOKUP
    select ARCH_HAS_CPUFREQ
    select GENERIC_CLOCKEVENTS
    select ARM_ERRATA_411920
    select MACH_BCM2708
    select VC4
    select FIQ
    help
      This enables support for Broadcom BCM2708 boards.

This should give the right gpio_to_irq macro when including linux/gpio.h

[msperl]

Well, it does compile as a branch:

objdump -Dz spi-config.o |grep -A3 -B 3 gpio_to
160: e3500000 cmp r0, #0
164: 1a00001a bne 1d4 <register_device+0x1d4>
168: e59d0020 ldr r0, [sp, #32]
16c: ebfffffe bl 0 <__gpio_to_irq>
170: e5840028 str r0, [r4, #40] ; 0x28
174: ea0001b6 b 854 <register_device+0x854>
178: e1a00007 mov r0, r7

As said: it only seems to effect the system during board-initialization - maybe gpio has not been set up correctly at that time and then the call runs into an exception - did not check on the serial at that time, so I can not tell you the details...

Ciao, Martin

[notro]

Now I know why gpio_to_irq() fails in bcm2708.c, but works fine in a module.

The gpio driver hasn't been probed yet in bcm2708_init(), which makes gpio_to_irq() return -EINVAL

This include

#include <linux/gpio.h>

gives (because CONFIG_NEED_MACH_GPIO_H is not defined)

#define gpio_to_irq     __gpio_to_irq


int __gpio_to_irq(unsigned gpio)
{
        return gpiod_to_irq(gpio_to_desc(gpio));
}

static struct gpio_desc *gpio_to_desc(unsigned gpio)
{
        if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
                return NULL;
        else
                return &gpio_desc[gpio];
}

static int gpiod_to_irq(const struct gpio_desc *desc)
{
        struct gpio_chip        *chip;
        int                     offset;

        if (!desc)
                return -EINVAL;
        chip = desc->chip;
        offset = gpio_chip_hwgpio(desc);
        return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
}

gpio_to_desc() returns NULL, because there is no gpio_chip registered yet, which makes gpiod_to_irq() return -EINVAL

In a module when the gpio driver already has been probed, chip->to_irq will point to bcm2708_gpio_to_irq() in bcm2708_gpio.c:

static int bcm2708_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
{
        return gpio_to_irq(gpio);
}

Here the mach/gpio.h macro is used, because of the include ordering in that file:

#include <mach/gpio.h>
#include <linux/gpio.h>

So the solution for bcm2708_init() is:

#include <mach/gpio.h>

which gives

#define gpio_to_irq(x)        ((x) + GPIO_IRQ_START)

[popcornmix]

@msperl
So with @notro's patch:

+        select NEED_MACH_GPIO_H

does it all work as expected? That looks like right fix.

[msperl]

I did not want to recompile the whole kernel with NEED_MACH_GPIO_H as the "work-around" including:

#include mach/gpio.h

instead is good enough...

Anything that uses it during the post-boot phase (when all portions are configured) using the standard gpio include is probably the better choice...

The observation is really just in the board-config, so defining that overall might open up another can of worms...

Still - there must have been a bit of reordering in the kernel after 3.6, that resulted in this reordering of the init sequence - (GPIO getting initialized after the board-config runs.)

This also shows that there is some return value check missing somewhere, so that it "fails" with a lockup of the kernel (maybe a memory exception) prior to getting the UART up and running and thus we do not know where it is really stuck.

Finally to admit - I have even moved away from modifying the board-config and instead use the "spi-config" module to define the spi devices on the fly when loading this driver - see http://github.com/msperl/spi-config.
It makes life so much easier - you might want to think of including it for convenience to all users who want to start quickly (even if they just want to modify SPI Bus speed for SPIDEV...)

Martin

[notro]

I agree with Martin, I think it's better to leave this alone, since the "problem" only applies to the board-config, and there is a good solution to the problem.

But to be honest: my real reason for not wanting this, is that I'm going to try and use the BCM2835 irq driver with BCM2708, and NEED_MACH_GPIO_H will break that.

[msperl]

Well - there is a relatively simple patch to make the "upstream" spi-bcm2835 work.
@notro: I think I have even posted a patch as a pull request (404) that makes the upstream driver work, or Do you mean GPIO?

But in the end the spi-bcm2835 is still wasting lots of CPU-resources unnecessarily - the 2nd version of the SPI pipeline driver is much faster and does not require any CPU besides setup.

And I am fighting to get an SPI-API extension in place to make DMA work with less setup cost, so that there is even less latency...

Get in contact with me via the forum if you are interested, as you want to use it for your framebuffer drivers...

[notro]

I was referring to drivers/irqchip/irq-bcm2835.c

[msperl]

Oh, you mean the one that would allow level interrupts...

[notro]

No, it replaces arch/arm/mach-bcm2708/armctrl.c

Level interrupts is in drivers/pinctrl/pinctrl-bcm2835.c

I wrote this page to understand the difference between BCM2708 and BCM2835: https://github.com/notro/rpi-firmware/wiki/BCM2708vsBCM2835

Right now I'm trying to understand the kernel boot process: https://github.com/notro/rpi-firmware/wiki/start_kernel

[jfasch]

IMHO the problem with the gpio_to_irq() crash is that it is easy to confuse the BCM2708's gpio_to_irq() macro from <mach/gpio.h> with the public subsystem's gpio_to_irq() from <linux/gpio.h>. Which one applies depends on which header is included first. There's no point in having duplicate names. Here's a patch to fix this. Tested on 3.10.y, compiled on 3.11.y. Please have a look and comment; I can push and submit a pull request if you prefer.

Cheers,
Jörg

From 027fb1b2d9c12a3bf7838cae82c75c47add683df Mon Sep 17 00:00:00 2001
From: Joerg Faschingbauer <jf@faschingbauer.co.at>
Date: Mon, 6 Jan 2014 21:40:21 +0100
Subject: [PATCH] bcm2708: fix gpio_to_irq() name clash

<mach/gpio.h> has gpio_to_irq() defined as a macro. the macro is
obviously intended as the direct implementation of that
functionality. unfortunately the gpio subsystem offers a public
function of the same name through <linux/gpio.h>. one has to be very
careful to include <mach/gpio.h> before <linux/gpio.h> - otherwise the
code will compile but only work by chance. board code will certainly
not work - the gpio driver is simply not loaded at that time.

fix the clash by renaming the offending macros from <mach/gpio.h>,
together with their uses.

Signed-off-by: Joerg Faschingbauer <jf@faschingbauer.co.at>
---
 arch/arm/mach-bcm2708/bcm2708_gpio.c      | 16 ++++++++--------
 arch/arm/mach-bcm2708/include/mach/gpio.h |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-bcm2708/bcm2708_gpio.c b/arch/arm/mach-bcm2708/bcm2708_gpio.c
index d0339eb..14c6a26 100644
--- a/arch/arm/mach-bcm2708/bcm2708_gpio.c
+++ b/arch/arm/mach-bcm2708/bcm2708_gpio.c
@@ -137,7 +137,7 @@ static void bcm2708_gpio_set(struct gpio_chip *gc, unsigned offset, int value)

 static int bcm2708_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
 {
-   return gpio_to_irq(gpio);
+    return __bcm2708_gpio_to_irq(gpio);
 }

 static int bcm2708_gpio_irq_set_type(struct irq_data *d, unsigned type)
@@ -149,15 +149,15 @@ static int bcm2708_gpio_irq_set_type(struct irq_data *d, unsigned type)
        return -EINVAL;

    if (type & IRQ_TYPE_EDGE_RISING) {
-       gpio->rising |= (1 << irq_to_gpio(irq));
+       gpio->rising |= (1 << __bcm2708_irq_to_gpio(irq));
    } else {
-       gpio->rising &= ~(1 << irq_to_gpio(irq));
+       gpio->rising &= ~(1 << __bcm2708_irq_to_gpio(irq));
    }

    if (type & IRQ_TYPE_EDGE_FALLING) {
-       gpio->falling |= (1 << irq_to_gpio(irq));
+       gpio->falling |= (1 << __bcm2708_irq_to_gpio(irq));
    } else {
-       gpio->falling &= ~(1 << irq_to_gpio(irq));
+       gpio->falling &= ~(1 << __bcm2708_irq_to_gpio(irq));
    }
    return 0;
 }
@@ -166,7 +166,7 @@ static void bcm2708_gpio_irq_mask(struct irq_data *d)
 {
    unsigned irq = d->irq;
    struct bcm2708_gpio *gpio = irq_get_chip_data(irq);
-   unsigned gn = irq_to_gpio(irq);
+   unsigned gn = __bcm2708_irq_to_gpio(irq);
    unsigned gb = gn / 32;
    unsigned long rising = readl(gpio->base + GPIOREN(gb));
    unsigned long falling = readl(gpio->base + GPIOFEN(gb));
@@ -181,7 +181,7 @@ static void bcm2708_gpio_irq_unmask(struct irq_data *d)
 {
    unsigned irq = d->irq;
    struct bcm2708_gpio *gpio = irq_get_chip_data(irq);
-   unsigned gn = irq_to_gpio(irq);
+   unsigned gn = __bcm2708_irq_to_gpio(irq);
    unsigned gb = gn / 32;
    unsigned long rising = readl(gpio->base + GPIOREN(gb));
    unsigned long falling = readl(gpio->base + GPIOFEN(gb));
@@ -222,7 +222,7 @@ static irqreturn_t bcm2708_gpio_interrupt(int irq, void *dev_id)
        edsr = readl(__io_address(GPIO_BASE) + GPIOEDS(bank));
        for_each_set_bit(i, &edsr, 32) {
            gpio = i + bank * 32;
-           generic_handle_irq(gpio_to_irq(gpio));
+           generic_handle_irq(__bcm2708_gpio_to_irq(gpio));
        }
        writel(0xffffffff, __io_address(GPIO_BASE) + GPIOEDS(bank));
    }
diff --git a/arch/arm/mach-bcm2708/include/mach/gpio.h b/arch/arm/mach-bcm2708/include/mach/gpio.h
index f600bc7..bc01abb 100644
--- a/arch/arm/mach-bcm2708/include/mach/gpio.h
+++ b/arch/arm/mach-bcm2708/include/mach/gpio.h
@@ -11,8 +11,8 @@

 #define ARCH_NR_GPIOS 54 // number of gpio lines

-#define gpio_to_irq(x) ((x) + GPIO_IRQ_START)
-#define irq_to_gpio(x) ((x) - GPIO_IRQ_START)
+#define __bcm2708_gpio_to_irq(x)   ((x) + GPIO_IRQ_START)
+#define __bcm2708_irq_to_gpio(x)   ((x) - GPIO_IRQ_START)

 #endif

-- 
1.8.3.2

[popcornmix]

@jfasch I've applied your patch. Thanks.
@msperl is this still an issue?

[jfasch]

Cool, thank you.

Please note that my patch itself doesn't fix anything, it just resolves the gpio_to_irq() name conflict. This makes it explicit that gpio_to_irq() is a generic subsystem interface which has to be backed by a driver, whereas __bcm2708_gpio_to_irq() is board code.

Btw __bcm2708_gpio_to_irq() is a constant macro - so one can initialize an interrupt line at compile time, rather than jump through hoops and calculate it at runtime. (This was already the case before I renamed it, of course.)

For example,

static struct spi_board_info bcm2708_spi_devices[] = {
    {
        .modalias = "mcp2515",
        .max_speed_hz = 10000000,
        .bus_num = 0,
        .chip_select = 0,
        .mode = SPI_MODE_0,
        .irq = __bcm2708_gpio_to_irq(25),
        .platform_data = &mcp251x_data,
    },
};