two gpio interrupts on RPi

[maddin1234]

Hello,
I tryed to connect two can-controller to a RaspberryPi and had problems with the interrupts.
I started in this post: http://www.raspberrypi.org/phpBB3/viewt ... =44&t=7027
but it looks like this topic is worth to make an own post.

I found out, that a second interrupt is not handled, when it occures during the low
time of the first interrupt.
I made some tests and found out some interresting things.

The gpio-interrupt handler is programmed in:
/usr/src/linux/arch/arm/mach-bcm2708/bcm2708_gpio.c

Code: Select all

static irqreturn_t bcm2708_gpio_interrupt(int irq, void *dev_id)
{
	unsigned long edsr;
	unsigned bank;
	int i;
	unsigned gpio;
	for (bank = 0; bank <= 1; bank++) {
		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));
		}
	writel(0xffffffff, __io_address(GPIO_BASE) + GPIOEDS(bank));
	}
	return IRQ_HANDLED;
}

static struct irqaction bcm2708_gpio_irq = {
	.name = "BCM2708 GPIO catchall handler",
	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
	.handler = bcm2708_gpio_interrupt,
};

static void bcm2708_gpio_irq_init(struct bcm2708_gpio *ucb)
{
	unsigned irq;

	ucb->gc.to_irq = bcm2708_gpio_to_irq;

	for (irq = GPIO_IRQ_START; irq < (GPIO_IRQ_START + GPIO_IRQS); irq++) {
		irq_set_chip_data(irq, ucb);
		irq_set_chip(irq, &bcm2708_irqchip);
		set_irq_flags(irq, IRQF_VALID);
	}
	setup_irq(IRQ_GPIO3, &bcm2708_gpio_irq);
}

That means there is a "BCM2708 GPIO catchall handler" that is executed, when
interrupt IRQ_GPIO3 is fired.
This handler reads the GPIOEDS registers and fires a generic interrupt for each set bit.

I found in the Broadcom manual, that there are 54 GPIOs organized in two banks and
3 interrupt lines, one for each bank and one in common.
Unfortunatly it is not said, how these banks are organized.
I assumed, that both banks have 27 GPIOs, but maybe it is different. In the schematic
for the RPi, there are two blocks, one with GPIO0 - GPIO27 (that is 28 GPIOs) and
the second with GPIO28 - GPIO53 (that is 26 GPIOs).

At another part of the manual, there is a table that shows 4 GPIO interrupts GPIO0 to GPIO3.

First thing I tested was, to change the interrupt source for the GPIO catchall handler and to test
it with an interrupt on GPIO17 and GPIO31 (from P5 header)

Code: Select all

...
setup_irq(IRQ_GPIO0, &bcm2708_gpio_irq);
//setup_irq(IRQ_GPIO1, &bcm2708_gpio_irq);
//setup_irq(IRQ_GPIO2, &bcm2708_gpio_irq);
//setup_irq(IRQ_GPIO3, &bcm2708_gpio_irq);
...

I found out, that
GPIO17 fires IRQ_GPIO0 or IRQ_GPIO3. If both are active, only IRQ_GPIO0 is fired.
GPIO31 fires IRQ_GPIO1 or IRQ_GPIO3. If both are active, only IRQ_GPIO1 is fired.

That means, IRQ_GPIO0 is the interrupt for bank0, IRQ_GPIO1 is the interrupt for bank1 and
IRQ_GPIO3 is the interrupt for both banks in common.

Then I found a bug in the handler. At the beginning of the handler, the GPIOEDS register is read.
Then there are generic interrupts fired for each set bit and after that, the whole register is cleared.
This causes a timing problem. All interrupts that appear in the short time between reading the
register and clearing the register will be cleared without beeing handled.
So I changed the code a little bit, now each bit is cleared seperatly.

Code: Select all

static irqreturn_t bcm2708_gpio_interrupt(int irq, void *dev_id)
{
	unsigned long edsr;
	unsigned bank;
	int i;
	unsigned gpio;
	for (bank = 0; bank <= 1; bank++) {
		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));
			writel(1<<i, __io_address(GPIO_BASE) + GPIOEDS(bank));
		}
	}
	return IRQ_HANDLED;
}

But I am not sure that this is enough. Maybe it is necessary to read GPIOEDS again
and check if all bits are cleared, before returning IRQ_HANDLED.

A second thing I tryed was to make two interrupt handlers, one for each bank.

Code: Select all

#define GPIO_BANK0_SIZE 27   //estimated size

static irqreturn_t bcm2708_gpio_interrupt_1(int irq, void *dev_id)
{
	unsigned long edsr;
	unsigned bank;
	int i;
	unsigned gpio;
	//might be nicer code when reading out 64bit at once if possible
	bank = 0;
	i = GPIO_BANK0_SIZE+1;
	edsr = readl(__io_address(GPIO_BASE) + GPIOEDS(bank));
	for_each_set_bit_from(i, &edsr, 32) {
		gpio = i + bank * 32;
		generic_handle_irq(gpio_to_irq(gpio));
		writel(1<<i,__io_address(GPIO_BASE) + GPIOEDS(bank));
	}
//	writel(GPIO_BANK1_MASK, __io_address(GPIO_BASE) + GPIOEDS(bank));
	
//	bank = 1;
//	i = 0;
//	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));
//	}
//	writel(0xffffffff, __io_address(GPIO_BASE) + GPIOEDS(bank));

	return IRQ_HANDLED;
}

static irqreturn_t bcm2708_gpio_interrupt_0(int irq, void *dev_id)
{
	unsigned long edsr;
	unsigned bank;
	int i;
	unsigned gpio;
	bank = 0; 
	edsr = readl(__io_address(GPIO_BASE) + GPIOEDS(bank));
	for_each_set_bit(i, &edsr, GPIO_BANK0_SIZE) {
		gpio = i + bank * 32;
		generic_handle_irq(gpio_to_irq(gpio));
		writel(1<<i,__io_address(GPIO_BASE) + GPIOEDS(bank));
	}
//	writel(GPIO_BANK0_MASK, __io_address(GPIO_BASE) + GPIOEDS(bank));
	return IRQ_HANDLED;
}

static struct irqaction bcm2708_gpio_irq_1 = {
	.name = "BCM2708 GPIO catch bank 1 handler",
	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
	.handler = bcm2708_gpio_interrupt_1,
};

static struct irqaction bcm2708_gpio_irq_0 = {
	.name = "BCM2708 GPIO catch bank 0 handler",
	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
	.handler = bcm2708_gpio_interrupt_0,
};

static void bcm2708_gpio_irq_init(struct bcm2708_gpio *ucb)
{
	unsigned irq;

	ucb->gc.to_irq = bcm2708_gpio_to_irq;

	for (irq = GPIO_IRQ_START; irq < (GPIO_IRQ_START + GPIO_IRQS); irq++) {
		irq_set_chip_data(irq, ucb);
		irq_set_chip(irq, &bcm2708_irqchip);
		set_irq_flags(irq, IRQF_VALID);
	}
	setup_irq(IRQ_GPIO1, &bcm2708_gpio_irq_1);
	setup_irq(IRQ_GPIO0, &bcm2708_gpio_irq_0);
}

This also works, but might have the same problem like described above.

A third thing I want to have a look at is the generic_handle_irq().
Perhaps it is possible to connect the my interrupt service routine (MCP2515)
directly to GPIO_IRQ0 and GPIO_IRQ1 and not through the generic_handle.
Maybe it is much faster.

I hope we can improve the interrupt behaviour a bit more.

Greetings maddin

[maddin1234]

Hello,
today I tested a new version.

Code: Select all

static irqreturn_t bcm2708_gpio_interrupt(int irq, void *dev_id)
{
	unsigned long edsr0;
	unsigned long edsr1;
	unsigned bank;
	int i;
	unsigned gpio;

	edsr0 = readl(__io_address(GPIO_BASE) + GPIOEDS(0));
	edsr1 = readl(__io_address(GPIO_BASE) + GPIOEDS(1));

	do {
		for_each_set_bit(i, &edsr0, 32) {
			gpio = i + 0 * 32;
			generic_handle_irq(gpio_to_irq(gpio));
			// only clear the bit that is handles with gerneric_handle_irq
			writel(1<<i, __io_address(GPIO_BASE) + GPIOEDS(0));
		}

		// this loop might be obsolete for RaspberryPi, because none of
		// these pins is available on headers
		for_each_set_bit(i, &edsr1, 32) {
			gpio = i + 1 * 32;
			generic_handle_irq(gpio_to_irq(gpio));
			writel(1<<i, __io_address(GPIO_BASE) + GPIOEDS(1));
		}

		// refresh EDS  state
		edsr0 = readl(__io_address(GPIO_BASE) + GPIOEDS(0));
		edsr1 = readl(__io_address(GPIO_BASE) + GPIOEDS(1));

	// do not exit as long as one bit is set
	while ((edsr0 != 0) || (edsr1 != 0));

	return IRQ_HANDLED;
}

It works much better than the kernel 3.6 version, but it still stops serving one interrupt.
The interrupt-line that falls down about 20us after the other stays low.

This is very puzzling. I would have expected, that it should be served latest with the next
interrupt from the other line, in case that the bit is still set.

The only explanation I have is, that the GPIOEDS bit is cleared though the interrupt
is not successfully served. The code doesn't check whether the generic_handle_irq was fired
successfully or not, before clearing the bit.

greeting maddin

[Zeta]

Hello maddin,

This is an interesting but complex topic...

I started to look at what these functions were doing, on my 3.6 kernel. I didn't find a lot of things new, but I think I spotted a bug in the "bcm2708_gpio_irq_mask" function.

Here is what I have seen:

Code: Select all

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 gb = gn / 32;
        unsigned long rising = readl(gpio->base + GPIOREN(gb));
        unsigned long falling = readl(gpio->base + GPIOFEN(gb));

        writel(rising & ~(1 << gn), gpio->base + GPIOREN(gb));
        writel(falling & ~(1 << gn), gpio->base + GPIOFEN(gb));
}

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 gb = gn / 32;
        unsigned long rising = readl(gpio->base + GPIOREN(gb));
        unsigned long falling = readl(gpio->base + GPIOFEN(gb));

        gn = gn % 32;

        writel(1 << gn, gpio->base + GPIOEDS(gb));

        if (gpio->rising & (1 << gn)) {
                writel(rising | (1 << gn), gpio->base + GPIOREN(gb));
        } else {
                writel(rising & ~(1 << gn), gpio->base + GPIOREN(gb));
        }

        if (gpio->falling & (1 << gn)) {
                writel(falling | (1 << gn), gpio->base + GPIOFEN(gb));
        } else {
                writel(falling & ~(1 << gn), gpio->base + GPIOFEN(gb));
        }
}

If you compare the code of the mask and unmask function, you can see two variables "gn" (gpio number?) and "gb" (gpio bank ?).
The principle is that each bank seems to contains only 32 bits, so bits higher are in the second bank. For example, bit 50 should be decomposed as : bank 1 (means 32) + bit 18.
However, the line

gn = gn % 32;

is missing in the "mask" function, but present is the "unmask" one.

I think using this function with a bit in the second bank (bank1) will fail, or a least do nothing (while something was expected). With the bit 50 example above, it would request bit 50 in bank 1, but bank 1 is only 32 bits wide. So 1<<50 would result in 0 => no effect.

This functions is given to the gpio definition like this:

Code: Select all

static struct irq_chip bcm2708_irqchip = {
        .name = "GPIO",
        .irq_enable = bcm2708_gpio_irq_unmask,
        .irq_disable = bcm2708_gpio_irq_mask,
        .irq_unmask = bcm2708_gpio_irq_unmask,
        .irq_mask = bcm2708_gpio_irq_mask,
        .irq_set_type = bcm2708_gpio_irq_set_type,
};

So it should be called for "irq disable" and "irq mask" operations. Don't when and how this happens...

This is probably not yet the solution of your problem, but its the only thing I spotted now. May be worth a trial. If you agree that this is a bug, I can open a ticket and create a pull request on github.

Zeta

[maddin1234]

Hello Zeta,
I agree with you that this is a bug and should be reported.
Also I am not sure that the calculation of the bank is correct.

Code: Select all

unsigned gb = gn / 32

gn / 32 is a floating point number, casted to an unsigned integer.
What was intended here is to get the remainder of the division.
Maybe the cast just cuts of the digits after the floating point, then it
would be correct.

For RaspberryPi it might have no effect, because these GPIOs are not
layouted to any header.

Greetings maddin

[Zeta]

gn / 32 is a floating point number, casted to an unsigned integer.
What was intended here is to get the remainder of the division.
Maybe the cast just cuts of the digits after the floating point, then it
would be correct.

"gn/32" uses the integer division. You should write "gn/32.0" of "(float(gn))/32" to explicitly require the floating point division. So this part is correct, there is no cast.
Using my previous example:
50/32 = 1 ("in GNU C the ‘/’ operator always rounds towards zero" : http://www.gnu.org/software/libc/manual ... ision.html)
50%32 = 18
and you can verify 1*32+18 = 50

For RaspberryPi it might have no effect, because these GPIOs are not
layouted to any header.

OK. I will report it anyway, but we still have your problem to spot then !

[maddin1234]

Hello,
I found one more thing, that might cause the problem, and maybe it is quite easy to solve.
The GPIOEDS bit is cleared after the generic_handle_irq has finished.
During generic_handle_irq some functions from mcp2515 driver are executed.

for example: mcp2515_interrupt(), mcp2515_read_flags() and mcp2515_read_flags_complete()
and in mcp2515_read_flags_complete() the function mcp2515_clear_canintf() can be executed.

This function clears the mcp2515 interrupt (if successfull) and the interrupt line goes to high.
The next event on can bus can cause a new interrupt, but this will not be recognized, until the
bcm2708_gpio clears the GPIOEDS bit.
And the time between clearing the mcp2515 register and clearing the bcm2708 register can
become "long", when a different interrupt comes between these two events.

I will try to clear the bcm2708 GPIOEDS bit before entering the mcp2515 interrupt service routine.
Then it is ready for receiving a new interrupt.

greetings maddin

[maddin1234]

Hello,
I did some more tests and it looks like the mcp2515 driver stops clearing the interrupt register in the mcp2515.
I will change back to the mcp2515 post.
http://www.raspberrypi.org/phpBB3/viewt ... &start=200

Greetings maddin

[maddin1234]

Hello,
I made some more test and now I don't have an idea anymore.

In the "log" are informations from the bcm2708_gpio module for both interrupt sources and
from the mcp2515 module only for can0 (to keep it readable).

Code: Select all

edsr at beginning of interrupt=0x00020000 
i=17 gpio=17 
CAN0_interrupt 
CAN0_read_flags 			CAN0_spi_async 
edsr after generic_interrupt=0x00000000
								                  CAN0_read_flags_complete Result: intf=04 eflg=00
CAN0_clear_intf 			CAN0_spi_async		CAN0_clear_intf_complete
CAN0_read_flags 			CAN0_spi_async 
CAN0_start_xmit 
								                CAN0_read_flags_complete Result: intf=00 eflg=00
CAN0_load_txb0 				CAN0_spi_async 		CAN0_load_txb0_complete 
edsr at beginning of interrupt=0x80000000 
i=31 gpio=31 
edsr after generic_interrupt=0x00000000
CAN0_send_rts_txb0 			CAN0_spi_async 		CAN0_send_rts_txb0_complete 
CAN0_read_flags 			CAN0_spi_async 		CAN0_read_flags_complete Result: intf=00 eflg=00
C0_clear_busy 
edsr at beginning of interrupt=0x00020000 
i=17 gpio=17 
CAN0_interrupt 
CAN0_read_flags 			CAN0_spi_async 
edsr after generic_interrupt=0x00000000
								                CAN0_read_flags_complete Result: intf=04 eflg=00
CAN0_clear_intf 			CAN0_spi_async 		CAN0_clear_intf_complete 
CAN0_read_flags 			CAN0_spi_async 
edsr at beginning of interrupt=0x80000000 
i=31 gpio=31 
edsr after generic_interrupt=0x00000000
CAN0_start_xmit 
								                CAN0_read_flags_complete Result: intf=00 eflg=00
CAN0_load_txb0 				CAN0_spi_async		CAN0_load_txb0_complete 
CAN0_send_rts_txb0 			CAN0_spi_async 		CAN0_send_rts_txb0_complete 
CAN0_read_flags 			CAN0_spi_async 		CAN0_read_flags_complete Result: intf=00 eflg=00
C0_clear_busy 
edsr at beginning of interrupt=0x80000000 
i=31 gpio=31 
edsr after generic_interrupt=0x00000000 
edsr at beginning of interrupt=0x80000000

For me, it looks like both modules work fine.
The mcp2515 module clears the intf flags and it is read out as 00 then.
The bcm2708_gpio module clears the gpio17 bit and it is read out as 0 then.

From this time it stays 0, I can see it, because the gpio 31 interrupt is still working.
When gpio17 interrupt would have been recognized, edsr should be 0x80020000.

So the only possibility I see is, that the falling edge has not been recognized.

Maybe it is possible to find out something more with a logic-analyser, but I don't have one.

greetings maddin

[maddin1234]

I found out, that the mcp2515 driver uses spin_lock_irqsave(&priv->lock, flags);

Does anyone know what this does exactly?
Does it disable the execution of a new interrupt in the operation system, or does it disable the recognition of an interrupt in the GPIO-registers?

[maddin1234]

Hello,
here they had the same ideas like we had.

http://lists.infradead.org/pipermail/li ... 00219.html

Greetings maddin

[bertr2d2]

Hmm,

Code: Select all

static irqreturn_t bcm2708_gpio_interrupt(int irq, void *dev_id)
{
   unsigned long edsr;
   unsigned bank;
   int i;
   unsigned gpio;
   for (bank = 0; bank <= 1; bank++) {
      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));
      }
   writel(0xffffffff, __io_address(GPIO_BASE) + GPIOEDS(bank));
   }
   return IRQ_HANDLED;
}

I had expected, that all between readl and writel shouldn't be interrupted. IMHO there could be a race condition. Did you tried to encapsulate the sequence with spinlock_irq_save and spinunlock_irq_restore ?