[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Stephen Warren]

On 09/21/2013 03:18 AM, Craig McGeachie wrote:
> The justification for doing this is:
>  * Simplify the decode and dispatch logic to make it easier to under-
>    stand and maintain.
>  * Re-arrange the hardware IRQ numbers to match the numbering scheme of
>    the FIQ register.

What's the benefit of that change? The bank numbers in the existing
driver match the register naming in the HW (base/pend1/pend2 for bank
0/1/2). Further on, one of your added comments says "This is the same
numbering scheme used in bits 0..6 of the FIQ register."; what about any
other bits in the FIQ register - do they still not match the renumbered
scheme, or are there just fewer bits in the FIQ register? Either way, I
don't understand why it's useful for the internal IRQ IDs to match the
FIQ register.

I guess the table of interrupt IDs in BCM2835_ARM_Peripherals.pdf

>  * Restore the flow of control that re-reads the base pending register
>    after handling any interrupt. The current version handles all
>    interrupts found in a GPU pending register before re-reading the
>    base pending register. In the original Broadcom assembly code, there
>    appear to be defect tracking numbers next to code inserted to create
>    this behaviour.

I assume you mean the assembly in BCM2835_ARM_Peripherals.pdf? I'm not
really sure that code implies that SW must work the way you've modified
it to work, although there's probably no harm making this change.

The "defect tracking numbers" in that code simply make sure that when
reading PEND1, any status bits there that also have shortcut bits in the
base pending register aren't handled. Presumably this is simply to avoid
having two IRQ numbers for the same physical IRQ, since any table
lookups for an IRQ handler would fail if using an IRQ ID derived from
PEND1 bank/bit ID rather than base bank/bit ID. IIRC, the existing Linux
driver already does this (ignores the bits in PEND1 that are
duplicates). This logic doesn't imply that one can't loop reading the
PEND1 register.

>  * DTS IRQ specifications can refer to either a shortcut bit in base or
>    the GPU bits.  E.g. UART can be either <0, 19> or <2, 25>.
>  * Add .irq_ack to the chip operations.

> diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c

>   * GNU General Public License for more details.
> - *
> - * Quirk 1: Shortcut interrupts don't set the bank 1/2 register pending bits
> - *
> - * If an interrupt fires on bank 1 that isn't in the shortcuts list, bit 8
...
> - * otherwise both handlers will fire at the same time!
>   */

That removes a bunch of useful description of the HW. I don't think it's
a good idea to remove it.

> -
> +#include <linux/bitops.h>

You should add a new line for the new header file, rather than replacing
the existing blank line.

>  #include <linux/io.h>
> -#include <linux/slab.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> -#include <linux/irqdomain.h>
> -
>  #include <asm/exception.h>
> -#include <asm/mach/irq.h>
> -
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include "irqchip.h"

Please keep the includes grouped by <linux/>, <asm/> then any local
includes.

> +#define BAD_IRQ_NUM	-1

That should be 0; Using -1 for invalid IRQ is deprecated.

> +static int bmc2835_domain_xlate(struct irq_domain *d, struct device_node *np,
>  	const u32 *intspec, unsigned int intsize,
>  	unsigned long *out_hwirq, unsigned int *out_type)
>  {
> -	if (WARN_ON(intsize != 2))
> +	unsigned int hwirq;
> +	if (WARN_ON(intsize != 2 || intspec[0] >= NR_BANKS || intspec[1] >= 32))
>  		return -EINVAL;

There should be a blank line between variable declarations and code.

> +	hwirq = ctrlr.hwirq_xlat[MAKE_HWIRQ((intspec[0] + 2) % 3, intspec[1])];
> +	if (WARN_ON(hwirq == BAD_IRQ_NUM))
>  		return -EINVAL;

I'd prefer to see "(intspec[0] + 2) % 3" be a macro or static inline
function so it can be given a name (e.g. dt_to_internal_bank()) so it's
obvious what that's doing. Otherwise, it seems rather confusing.

I'm not sure why a hwirq_xlat table lookup is needed; why not just
return the HW IRQ number directly like the existing code?

> +static void bmc2835_mask_irq(struct irq_data *d)
>  {
> +	void __iomem *ioreg = ctrlr.iobase + 0x1c + (HWIRQ_BANK(d->hwirq) << 2);
> +	u32 bit = HWIRQ_BIT(d->hwirq);
> +	writel_relaxed(bit, ioreg);
> +}

The old code seems a lot simpler here, since there aren't any magic
constants in the code, but rather simply a table of register addresses
that's indexed by bank number:

> static int reg_disable[] __initconst = { 0x24, 0x1c, 0x20 };
...
> static void armctrl_mask_irq(struct irq_data *d)
> {
>         writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]);
> }

What's the benefit of this change? A similar comment for
bmc2835_unmask_irq().

> +static struct irq_chip bmc2835_chip = {
> +	.name = "bmc2835-level",
> +	.irq_mask = bmc2835_mask_irq,
> +	.irq_unmask = bmc2835_unmask_irq,
> +	.irq_ack =  bmc2835_mask_irq,
> +};

Why should an IRQ be masked by the ack operation?

> +/**
> + * bmc2835_dispatch_irq() - translate bank/bit to kernel IRQ and dispatch
> + * @bank:	Register bank (BASE, GPU1, or GPU2).
> + * @bits:	Pending interrupt status bits.
> + * @regs:	register pointer passed on to the handler.
> + *
> + * The bits passed are the full set of bits read from register.  Only the lowest
> + * order bit is translated and dispatched. Dispatching the other IRQs requires
> + * re-reading the register and calling this function again.

I'm not actually sure that's a good change. If we read e.g. PEND1 and
saw 4 bits asserted, why wouldn't we optimize by handling each of those
4 bits. Re-reading the PEND1 register for each bit, and presumably
seeing the exact same set of remaining bits still set (or even more),
just seems like a waste. We know we're going to have to process all the
interrupts, so why not just process them?

> +static int __init bmc2835_of_init(struct device_node *node,

> +	for (i = 0; i != NR_CHIP_IRQS; i++) {
> +		int irq;
> +		BUG_ON((irq = irq_create_mapping(ctrlr.domain, i)) <= 0);

I rather dislike statements with assignment side-effects. That was a
separate assignment and test statement in the original code, which seems
much clearer.

Overall, I'm not sure what bug/issue in the original code this patch is
meant to solve, nor why the new IRQ ID scheme is better. Sorry.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

At the risk of being accused of top-posting, I'll just reply to this 
first in case you want to skip the rest.

On 09/24/2013 03:38 PM, Stephen Warren wrote:
 > Overall, I'm not sure what bug/issue in the original code this
 > patch is meant to solve, nor why the new IRQ ID scheme is better.
 > Sorry.

No apologies required.  I'm fairly thick skinned when it comes to 
criticism code I write.  I simply appreciate you taking to the time to 
review it.

Ultimately I was trying to achieve three things:
  * Restore the original order of interrupt dispatch, especially if new
    interrupts become pending.
  * Enable FIQ handling.
  * Clarify the processing and dispatch of interrupts, particularly in
    light of short cuts.

I wasn't too sure of the reasons for the ordering, and may have read too 
much into the comment "see SW-5809". But I was motivated by the Broadcom 
driver for the DMA controller that identified DMA 2 and 3 as "fast DMA" 
which correlates with the interrupts for those two channels being short 
cut.  Still, there is a strong element of cargo-cult programming here. 
Mea culpa.

The second point may be moot since I'm no longer sure if this is 
possible.  See below.

I'm in no position to judge the third point.  By the time I'd taken the 
trouble to understand the existing code, I understood both versions 
equally well.

All in all, I don't feel the need to be pushy about this and am happy to 
let it rest.  But if there are any ideas you'd like to salvage, I'd be 
happy to re-implement them from scatch and submit again.

On 09/24/2013 03:38 PM, Stephen Warren wrote:
> On 09/21/2013 03:18 AM, Craig McGeachie wrote:
>> The justification for doing this is:
>>   * Simplify the decode and dispatch logic to make it easier to under-
>>     stand and maintain.
>>   * Re-arrange the hardware IRQ numbers to match the numbering scheme of
>>     the FIQ register.
>
> What's the benefit of that change? The bank numbers in the existing
> driver match the register naming in the HW (base/pend1/pend2 for bank
> 0/1/2).

Not really. The different types registers from each bank have different 
bank orderings.  The pending registers go 
base=0x200/gpu0=0x204/gpu1=0x208.  The enable registers go 
gpu0=0x210/gpu1=0x214/base=0x218. The disable registers go 
gpu0=0x21C/gpu1=0x220/base=0x224.  The choice of bank numbering is 
arbitrary, but given that the pending registers require special handling 
anyway, I preferred that of the enable/disable registers. And it matches 
the implied order given the numbering in the FIQ control register.

As a side note, BCM2835_ARM_Peripherals.pdf doesn't number the base 
register but variously numbers the GPU registers as 0/1 or 1/2.  The 
document is inconsistent on this point.

> Further on, one of your added comments says "This is the same
> numbering scheme used in bits 0..6 of the FIQ register."; what about any
> other bits in the FIQ register - do they still not match the renumbered
> scheme, or are there just fewer bits in the FIQ register? Either way, I
> don't understand why it's useful for the internal IRQ IDs to match the
> FIQ register.

If you represent the UART interrupt as 57, then if you wish to treat the 
UART as a FIQ you simply write to the FIQ register like so:
   writel_relaxed((BIT(7) | 57), ctrlr.iobase + 0x0C)
and write the appropriate bit to the disable registers.  Any other 
numbering scheme for hardware IRQs requires translation to 57 first so 
that you can write the correct 7 bit unsigned integer to bits 6:0.

However, this might all be pointless. Only one interrupt can be flagged 
as fast at any given time.  After the first driver registers for a fast 
interrupt, the best that I would be able to do is effectively mark all 
fast interrupts as unavailable until the first one is released.

>
> I guess the table of interrupt IDs in BCM2835_ARM_Peripherals.pdf
>
>>   * Restore the flow of control that re-reads the base pending register
>>     after handling any interrupt. The current version handles all
>>     interrupts found in a GPU pending register before re-reading the
>>     base pending register. In the original Broadcom assembly code, there
>>     appear to be defect tracking numbers next to code inserted to create
>>     this behaviour.
>
> I assume you mean the assembly in BCM2835_ARM_Peripherals.pdf? I'm not
> really sure that code implies that SW must work the way you've modified
> it to work, although there's probably no harm making this change.
>
> The "defect tracking numbers" in that code simply make sure that when
> reading PEND1, any status bits there that also have shortcut bits in the
> base pending register aren't handled. Presumably this is simply to avoid
> having two IRQ numbers for the same physical IRQ, since any table
> lookups for an IRQ handler would fail if using an IRQ ID derived from
> PEND1 bank/bit ID rather than base bank/bit ID.

Now that you describe it like that, I think you're right.

> IIRC, the existing Linux
> driver already does this (ignores the bits in PEND1 that are
> duplicates).

I don't think it does. If it doesn't find any other interrupts in the 
base pending register it handles the first bank:
		} else if (stat & BANK1_HWIRQ) {
			armctrl_handle_bank(1, regs);

Then it reads the bits in tight loop handling all that it finds.
	while ((stat = readl_relaxed(intc.pending[bank]))) {
		irq = MAKE_HWIRQ(bank, ffs(stat) - 1);
		handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
	}

So long as new interrupts come in on this register, the base pending 
register will never be re-read.  Still, there could be memory access 
ordering and caching behaviour that I'm completely ignorant of, so I 
could be completely wrong. And it's probably unlikely that interrupts 
come in that fast.

> This logic doesn't imply that one can't loop reading the
> PEND1 register.

No. I just felt that always reading the base pending register and 
processing the short cut interrupts there lent itself to a natural 
interrupt prioritisation.

>
>
>> diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
>
>>    * GNU General Public License for more details.
>> - *
>> - * Quirk 1: Shortcut interrupts don't set the bank 1/2 register pending bits
>> - *
>> - * If an interrupt fires on bank 1 that isn't in the shortcuts list, bit 8
> ...
>> - * otherwise both handlers will fire at the same time!
>>    */
>
> That removes a bunch of useful description of the HW. I don't think it's
> a good idea to remove it.

I didn't find it any more helpful than the BCM2835_ARM_Peripherals.pdf. 
  Not that the PDF was very easy to follow either.  I also thought that 
comparing what was going on to a "proper cascaded interrupt controller" 
distracting. There is no cascade.  I don't think there's meant to be. 
Just a simple bit of combinational digital circuitry on top of the 
interrupt lines. But this is purely a personal opinion.

And now a few snips, simply because you're correct and I have nothing to 
add.

 >...cut...<
> I'm not sure why a hwirq_xlat table lookup is needed; why not just
> return the HW IRQ number directly like the existing code?

Have I managed to cover this elsewhere?

>...cut...<
>> +static struct irq_chip bmc2835_chip = {
>> +	.name = "bmc2835-level",
>> +	.irq_mask = bmc2835_mask_irq,
>> +	.irq_unmask = bmc2835_unmask_irq,
>> +	.irq_ack =  bmc2835_mask_irq,
>> +};
>
> Why should an IRQ be masked by the ack operation?

To be honest, I don't know for sure.  I took my cue from 
Documentation/arm/Interrupts.

ack    - required.  May be the same function as mask for IRQs
          handled by do_level_IRQ.

Some other irq_chips are set up the same way and I can't see any other 
way of implementing an acknowledgement function.  And my experience with 
a never ending flow of interrupts while writing the mailbox driver made 
me think it might be good idea for the BMC2835 as well.

>
>> +/**
>> + * bmc2835_dispatch_irq() - translate bank/bit to kernel IRQ and dispatch
>> + * @bank:	Register bank (BASE, GPU1, or GPU2).
>> + * @bits:	Pending interrupt status bits.
>> + * @regs:	register pointer passed on to the handler.
>> + *
>> + * The bits passed are the full set of bits read from register.  Only the lowest
>> + * order bit is translated and dispatched. Dispatching the other IRQs requires
>> + * re-reading the register and calling this function again.
>
> I'm not actually sure that's a good change. If we read e.g. PEND1 and
> saw 4 bits asserted, why wouldn't we optimize by handling each of those
> 4 bits. Re-reading the PEND1 register for each bit, and presumably
> seeing the exact same set of remaining bits still set (or even more),
> just seems like a waste. We know we're going to have to process all the
> interrupts, so why not just process them?

Again, simply from the point of prioritising interrupts.  It's not just 
re-reading the current pending register. As to whether a full review of 
pending interrupts was appropriate, I don't really know. Thats the flow 
that arch/arm/include/asm/entry-macro-multi.S implements.  Find the 
first known interrupt.  Handle it. Find the first known interrupt again 
without any context from previous search.  There might perhaps have been 
a touch of cargo-cult going on again.

>
>> +static int __init bmc2835_of_init(struct device_node *node,
>
>> +	for (i = 0; i != NR_CHIP_IRQS; i++) {
>> +		int irq;
>> +		BUG_ON((irq = irq_create_mapping(ctrlr.domain, i)) <= 0);
>
> I rather dislike statements with assignment side-effects. That was a
> separate assignment and test statement in the original code, which seems
> much clearer.

Um, I generally don't like assignment expressions either.  I have no 
idea why I did this.

Cheers,
Craig.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 09/24/2013 03:38 PM, Stephen Warren wrote:
>> +#define BAD_IRQ_NUM	-1
>
> That should be 0; Using -1 for invalid IRQ is deprecated.

Could you please point me to something that describes this? I can't find 
anything that limits the internal representations used by the driver for 
an interrupt controller.  That, and the design of 
include/linux/irqdomain.h and kernel/irq/irqdomain.c implies that a 
0-based hwirq numbering scheme is the natural order of things.

Cheers,
Craig.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 09/24/2013 03:38 PM, Stephen Warren wrote:
> I'm not actually sure that's a good change. If we read e.g. PEND1 and
> saw 4 bits asserted, why wouldn't we optimize by handling each of those
> 4 bits. Re-reading the PEND1 register for each bit, and presumably
> seeing the exact same set of remaining bits still set (or even more),
> just seems like a waste. We know we're going to have to process all the
> interrupts, so why not just process them?


On 09/25/2013 11:33 PM, Simon Arlott wrote:
> On Sat, September 21, 2013 09:14, Craig McGeachie wrote:
>>   * Restore the flow of control that re-reads the base pending register
>>     after handling any interrupt. The current version handles all
>>     interrupts found in a GPU pending register before re-reading the
>>     base pending register. In the original Broadcom assembly code, there
>>     appear to be defect tracking numbers next to code inserted to create
>>     this behaviour.
>
> This was by design so that continuous interrupts in a bank did not impact
> progress of interrupts in other bank. If there are defects with this
> strategy, then check that they do not exist in your interrupt handlers
> instead of the interrupt controller itself. Unless there is a real bug
> in the interrupt controller, you're decreasing the performance by
> re-reading the base pending register every time.

I don't understand the concern with re-reading two volatile registers 
between each dispatch.  Given the amount of processing that occurs 
between the call to handle_IRQ and the calls to the possibly multiple 
registered interrupt handlers, plus the processing that the handlers 
perform (even if they are implemented as top/bottom halves), I think the 
performance overhead of the two extra reads is vanishingly small.  In 
fact, I think that focusing on eliminating them is premature 
optimisation.  Developers are notoriously bad at identifying performance 
hotspots through visual inspection.

The point about the registers being volatile is important.  It's a C 
keyword for a very good reason. The state of those registers can change 
due to actions that the interrupt controller knows nothing about.  And 
as noted previously, there is in fact quite a lot of time for them to 
change. Especially with over a dozen bits of independent silicon logic 
beavering away.  I don't think the interrupt controller code can make 
any assumptions about which future interrupts it may, or may not, have 
to dispatch.

Let's start with the DMA controller as an example.  There are 15 
channels (maybe 16 but I think one is unusable by the ARM core) mapped 
onto 12 interrupt lines.  As well as 12 IRQs that the DMA controller 
module could register for, there is a 16 bit register that it can read 
to find the current interrupt status of all DMA channels.  The register 
is also documented as writeable, so my guess is that it is also the 
acknowledgement mechanism.  Given the overloading of the interrupt 
lines, I imagine the chip designers added this to resolve the issue of 
overloaded interrupts[1].  I'm going to be understandably cautious about 
saying this, but there doesn't seem to be any reason that the DMA 
controller couldn't read all the interrupt status bits in its register 
and optimise processing by handling them all at once.[2]

If this is done, and the interrupt controller code doesn't re-read the 
pending registers, then the result is not a minor performance gain, but 
a major performance loss as the execution path between handle_IRQ and 
the DMA handler is needlessly traversed multiple times.

That, and the ability to slave DMA channels to other devices, means 
making assumptions about the validity of the pending status registers is 
a risky proposition.

I don't think you can even assume that more important IRQs won't be 
raised.  I think the mailbox might be an example.  With the callback 
mechanism that Jassi Brar and Suman Anna implemented, it might even be 
possible to issue mailbox requests from interrupt handlers.  Not totally 
risk free, but avoiding deadlocks should be straight forward.  I'm 
really going out on a limb on this one, but given that device power 
management is a mailbox call to the properties channel, I can easily 
imagine good reasons to turn devices on and off in response to 
interrupts [3].  This really is a stretch.  I think this should be done 
in a tasklet, but I don't want to preclude the possibility that doing it 
from the handler directly is the better implementation option.

And if anyone is going to attempt this sort of trickery, then I'm fairly 
certain that having a well defined interrupt prioritisation scheme is 
going to be important.  Not being able to make a good prediction of 
which interrupt handler will be invoked next because you don't know what 
bit pattern the interrupt controller currently has cached doesn't seem 
good enough.

As I understand it, interrupt prioritisation isn't about some abstract 
notion of fairness.  Some interrupts are just more important than others 
and you handle them first - always.  The way to ensure all interrupts 
are handled is for the handlers to do the minimum amount of work 
required to acknowledge the interrupt, and to schedule future work.  If 
the interrupts are still coming too thick and fast, then you probably 
have some other problem intruding from the outside physical world. 
Masking may or may not help.  It would depend on the situation.

You aren't developing a framework for a bunch of cosseted application 
developers who can't even get basic resource management right.  Your 
users are device driver developers who should damn well know better than 
to hog the system.  And that is the sort of developer who would 
appreciate some more control and some slightly better guarantees from 
other code, even if the rest of the world isn't going to play ball.

Cheers,
Craig.


[1] I'm very sorry about that, but it was irresistible.

[2] Yes, I know that the ARM core isn't allowed to use all the channels 
and has to find out from the videocore which ones are permitted.  I'm 
skipping over that for simplicity's sake.

[3] I'd really like better power management.  There are a lot of people 
building battery powered projects (or solar powered) and I'd like to see 
Linux held up as the poster child for good power management.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Simon Arlott]

On Thu, September 26, 2013 09:19, Craig McGeachie wrote:
> And if anyone is going to attempt this sort of trickery, then I'm fairly
> certain that having a well defined interrupt prioritisation scheme is
> going to be important.  Not being able to make a good prediction of
> which interrupt handler will be invoked next because you don't know what
> bit pattern the interrupt controller currently has cached doesn't seem
> good enough.

Why do you need to predict which interrupt handler will be invoked next?

> You aren't developing a framework for a bunch of cosseted application
> developers who can't even get basic resource management right.  Your
> users are device driver developers who should damn well know better than
> to hog the system.  And that is the sort of developer who would
> appreciate some more control and some slightly better guarantees from
> other code, even if the rest of the world isn't going to play ball.

Device driver developers should know better than to write drivers that
depend on such specific behaviour of the interrupt controller driver.

-- 
Simon Arlott

[PATCH v3] irq: bmc2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

Force the re-reading of the pending status registers before dispatching
the next interrupt request.  There is no guarantee that the values will
remain valid.  Actions taken by interrupt handlers could handle and
acknowledge multiple interrupts.  For example, the DMA controller has
it's own view of the interrupt status of all channels rather than just
the one that the interrupt was dispatched for.  The cost of re-reading
is small.  The cost of unnecessary dispatches is large.

Reorder the internal numbering of interrupt banks so that hardware IRQ
numbers range from 0 to 71, and match the natural numbering implied by
the FIQ source register.  This is in anticipation of being to implement
a viable mechanism for registering one of the available interrupts as a
fast interrupt.  Preferably based on device tree configuration.

Enable device tree interrupt specifications to identify interrupts with
shortcut bits in the base register by either the shortcut bit, or the
canonical GPU register bit.   E.g. UART can be either <0, 19> or <2,
25>.

Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
Cc: linux-rpi-kernel at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org

---
v3 Fixes as per Stephen Warren's comments:
 * Restore table lookup for IRQ enable/disable.
 * Move magic numbers to defines.
 * Extract and document dt_to_internal_bank().
 * Correct #include line ordering.
 * remove .irq_ack from the chip definition.  It's unneccesary. The
   interrupt framework calls .irq_mask if it's not defined.
 * Various other layout fixes.
 * Correct the patch summary line.

v2:
 * Correct a stupid defect.  Should always test even minor changes.
 * Add forgotten bits to the commit message.
---
 drivers/irqchip/irq-bcm2835.c | 367 ++++++++++++++++++++++++------------------
 1 file changed, 206 insertions(+), 161 deletions(-)

diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
index 1693b8e7..f2241e84 100644
--- a/drivers/irqchip/irq-bcm2835.c
+++ b/drivers/irqchip/irq-bcm2835.c
@@ -1,6 +1,7 @@
 /*
  * Copyright 2010 Broadcom
  * Copyright 2012 Simon Arlott, Chris Boot, Stephen Warren
+ * Copyright 2013 Craig McGeachie
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -11,212 +12,256 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
- * Quirk 1: Shortcut interrupts don't set the bank 1/2 register pending bits
- *
- * If an interrupt fires on bank 1 that isn't in the shortcuts list, bit 8
- * on bank 0 is set to signify that an interrupt in bank 1 has fired, and
- * to look in the bank 1 status register for more information.
- *
- * If an interrupt fires on bank 1 that _is_ in the shortcuts list, its
- * shortcut bit in bank 0 is set as well as its interrupt bit in the bank 1
- * status register, but bank 0 bit 8 is _not_ set.
- *
- * Quirk 2: You can't mask the register 1/2 pending interrupts
- *
- * In a proper cascaded interrupt controller, the interrupt lines with
- * cascaded interrupt controllers on them are just normal interrupt lines.
- * You can mask the interrupts and get on with things. With this controller
- * you can't do that.
- *
- * Quirk 3: The shortcut interrupts can't be (un)masked in bank 0
- *
- * Those interrupts that have shortcuts can only be masked/unmasked in
- * their respective banks' enable/disable registers. Doing so in the bank 0
- * enable/disable registers has no effect.
- *
- * The FIQ control register:
- *  Bits 0-6: IRQ (index in order of interrupts from banks 1, 2, then 0)
- *  Bit    7: Enable FIQ generation
- *  Bits  8+: Unused
- *
- * An interrupt must be disabled before configuring it for FIQ generation
- * otherwise both handlers will fire at the same time!
  */
 
+#include <linux/bitops.h>
 #include <linux/io.h>
-#include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/irq.h>
 #include <linux/irqdomain.h>
 
 #include <asm/exception.h>
-#include <asm/mach/irq.h>
 
 #include "irqchip.h"
 
-/* Put the bank and irq (32 bits) into the hwirq */
-#define MAKE_HWIRQ(b, n)	((b << 5) | (n))
-#define HWIRQ_BANK(i)		(i >> 5)
-#define HWIRQ_BIT(i)		BIT(i & 0x1f)
-
-#define NR_IRQS_BANK0		8
-#define BANK0_HWIRQ_MASK	0xff
-/* Shortcuts can't be disabled so any unknown new ones need to be masked */
-#define SHORTCUT1_MASK		0x00007c00
-#define SHORTCUT2_MASK		0x001f8000
-#define SHORTCUT_SHIFT		10
-#define BANK1_HWIRQ		BIT(8)
-#define BANK2_HWIRQ		BIT(9)
-#define BANK0_VALID_MASK	(BANK0_HWIRQ_MASK | BANK1_HWIRQ | BANK2_HWIRQ \
-					| SHORTCUT1_MASK | SHORTCUT2_MASK)
-
-#define REG_FIQ_CONTROL		0x0c
-
-#define NR_BANKS		3
-#define IRQS_PER_BANK		32
-
-static int reg_pending[] __initconst = { 0x00, 0x04, 0x08 };
-static int reg_enable[] __initconst = { 0x18, 0x10, 0x14 };
-static int reg_disable[] __initconst = { 0x24, 0x1c, 0x20 };
-static int bank_irqs[] __initconst = { 8, 32, 32 };
-
-static const int shortcuts[] = {
-	7, 9, 10, 18, 19,		/* Bank 1 */
-	21, 22, 23, 24, 25, 30		/* Bank 2 */
-};
+/*
+ * Internal to this file, IRQs are numbered as per the enable and disable
+ * registers.  The register banks GPU1, GPU2, and BASE are numbered 0, 1, and 2
+ * respectively. IRQs 0..31 are bits 0..31 of the GPU1 register.  IRQs 32..63
+ * are bits 0..31 of the GPU2 register.  IRQs 64..71 are bits 0..7 of the BASE
+ * register.  This is the same numbering scheme used in bits 0..6 of the FIQ
+ * register.
+ *
+ * Translation into and out of this address space happens at two
+ * points:
+ * - external drivers registering handlers will result in a call to
+ *   bmc2835_domain_xlate()
+ * - interrupt pending status registers are read in bcm2835_handle_irq() which
+ *   uses bmc2835_dispatch_irq() to map through the internal hardware IRQ space
+ *   into the kernel's registered IRQ address space.
+ */
+
+#define MAKE_HWIRQ(b, n)	(((b) << 5) | ((n) & 0x1f))
+#define HWIRQ_BANK(i)		((i) >> 5)
+#define HWIRQ_BIT(i)		BIT((i) & 0x1f)
+
+#define GPU1_BANK	0
+#define GPU2_BANK	1
+#define BASE_BANK	2
+#define NR_BANKS	3
+
+#define BITS_PER_REG	32
+
+#define BASE_PEND_OFF	0x00
+#define GPU1_PEND_OFF	0x04
+#define GPU2_PEND_OFF	0x08
+
+#define BASE_READABLE	0x001fffff
+#define BASE_GPU1	BIT(8)
+#define BASE_GPU2	BIT(9)
+#define BASE_IRQS	(BASE_READABLE & ~(BASE_GPU1 | BASE_GPU2))
+#define GPU1_IRQS	~(BIT(7) | BIT(9) | BIT(10) | BIT(18) | BIT(19))
+#define GPU2_IRQS	~(BIT(21) | BIT(22) | BIT(23) | BIT(23) | BIT(25) \
+				| BIT(30))
+
+#define NR_CHIP_IRQS	(BITS_PER_REG * 2 + 8)
+#define BAD_IRQ_NUM	-1
+
+static int reg_enable[] __initconst = { 0x10, 0x14, 0x18 };
+static int reg_disable[] __initconst = { 0x1c, 0x20, 0x24 };
 
-struct armctrl_ic {
-	void __iomem *base;
-	void __iomem *pending[NR_BANKS];
+static struct ctrlr {
+	void __iomem *iobase;
+	void __iomem *base_pend;
+	void __iomem *gpu1_pend;
+	void __iomem *gpu2_pend;
 	void __iomem *enable[NR_BANKS];
 	void __iomem *disable[NR_BANKS];
 	struct irq_domain *domain;
-};
-
-static struct armctrl_ic intc __read_mostly;
-static asmlinkage void __exception_irq_entry bcm2835_handle_irq(
-	struct pt_regs *regs);
-
-static void armctrl_mask_irq(struct irq_data *d)
-{
-	writel_relaxed(HWIRQ_BIT(d->hwirq), intc.disable[HWIRQ_BANK(d->hwirq)]);
-}
+	unsigned int hwirq_xlat[BITS_PER_REG * NR_BANKS];
+} ctrlr __read_mostly;
 
-static void armctrl_unmask_irq(struct irq_data *d)
-{
-	writel_relaxed(HWIRQ_BIT(d->hwirq), intc.enable[HWIRQ_BANK(d->hwirq)]);
+/**
+ * dt_to_internal_bank() - translate DT bank to hardware bank.
+ * @dt_bank:	bank number as read from device tree.
+ *
+ * The device tree represents GPU1, GPU2, and BASE are numbered 1, 2, and 0
+ * respectively. Internally they are represented as 0, 1,and 2 respectively.
+ * This function performs the conversion.
+ */
+static inline unsigned int dt_to_internal_bank(const u32 dt_bank) {
+	return (dt_bank + 2) % 3;
 }
 
-static struct irq_chip armctrl_chip = {
-	.name = "ARMCTRL-level",
-	.irq_mask = armctrl_mask_irq,
-	.irq_unmask = armctrl_unmask_irq
-};
-
-static int armctrl_xlate(struct irq_domain *d, struct device_node *ctrlr,
+/**
+ * bmc2835_domain_xlate() - translate FTD IRQ specification to hardware IRQ
+ * @d:		controller domain
+ * @np:		device node
+ * @intspec:	interrupt specification values
+ * @intsize:	interrupt specification size
+ * @out_hwirq:	return the hardware IRQ
+ * @out_type:	return the IRQ type
+ *
+ * The interrupt is specified as two u32 values.  The first is the FDT bank
+ * number, the second is the bit number in bank.
+ *
+ * Interrupt specifiers may refer to an interrupt with a shortcut bit in the
+ * BASE register in two ways. Either by refering to the BASE register shortcut
+ * bit, or by referring to the GPU1/GPU2 register bit.  For example, UART may
+ * be either <0, 19> or <2, 25>.
+ */
+static int bmc2835_domain_xlate(struct irq_domain *d, struct device_node *np,
 	const u32 *intspec, unsigned int intsize,
 	unsigned long *out_hwirq, unsigned int *out_type)
 {
-	if (WARN_ON(intsize != 2))
-		return -EINVAL;
+	unsigned int bank, hwirq;
 
-	if (WARN_ON(intspec[0] >= NR_BANKS))
+	if (WARN_ON(intsize != 2 || intspec[0] >= NR_BANKS || intspec[1] >= 32))
 		return -EINVAL;
-
-	if (WARN_ON(intspec[1] >= IRQS_PER_BANK))
-		return -EINVAL;
-
-	if (WARN_ON(intspec[0] == 0 && intspec[1] >= NR_IRQS_BANK0))
+	bank = dt_to_internal_bank(intspec[0]);
+	hwirq = ctrlr.hwirq_xlat[MAKE_HWIRQ(bank, intspec[1])];
+	if (WARN_ON(hwirq == BAD_IRQ_NUM))
 		return -EINVAL;
-
-	*out_hwirq = MAKE_HWIRQ(intspec[0], intspec[1]);
+	*out_hwirq = hwirq;
 	*out_type = IRQ_TYPE_NONE;
 	return 0;
 }
 
-static struct irq_domain_ops armctrl_ops = {
-	.xlate = armctrl_xlate
+static struct irq_domain_ops bmc2835_ops = {
+	.xlate = bmc2835_domain_xlate
 };
 
-static int __init armctrl_of_init(struct device_node *node,
-	struct device_node *parent)
+static void bmc2835_mask_irq(struct irq_data *d)
 {
-	void __iomem *base;
-	int irq, b, i;
-
-	base = of_iomap(node, 0);
-	if (!base)
-		panic("%s: unable to map IC registers\n",
-			node->full_name);
+	void __iomem *ioreg = ctrlr.disable[HWIRQ_BANK(d->hwirq)];
+	u32 bit = HWIRQ_BIT(d->hwirq);
 
-	intc.domain = irq_domain_add_linear(node, MAKE_HWIRQ(NR_BANKS, 0),
-			&armctrl_ops, NULL);
-	if (!intc.domain)
-		panic("%s: unable to create IRQ domain\n", node->full_name);
+	writel_relaxed(bit, ioreg);
+}
 
-	for (b = 0; b < NR_BANKS; b++) {
-		intc.pending[b] = base + reg_pending[b];
-		intc.enable[b] = base + reg_enable[b];
-		intc.disable[b] = base + reg_disable[b];
-
-		for (i = 0; i < bank_irqs[b]; i++) {
-			irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i));
-			BUG_ON(irq <= 0);
-			irq_set_chip_and_handler(irq, &armctrl_chip,
-				handle_level_irq);
-			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
-		}
-	}
+static void bmc2835_unmask_irq(struct irq_data *d)
+{
+	void __iomem *ioreg = ctrlr.enable[HWIRQ_BANK(d->hwirq)];
+	u32 bit = HWIRQ_BIT(d->hwirq);
 
-	set_handle_irq(bcm2835_handle_irq);
-	return 0;
+	writel_relaxed(bit, ioreg);
 }
 
-/*
- * Handle each interrupt across the entire interrupt controller.  This reads the
- * status register before handling each interrupt, which is necessary given that
- * handle_IRQ may briefly re-enable interrupts for soft IRQ handling.
- */
+static struct irq_chip bmc2835_chip = {
+	.name = "bmc2835-level",
+	.irq_mask = bmc2835_mask_irq,
+	.irq_unmask = bmc2835_unmask_irq,
+};
 
-static void armctrl_handle_bank(int bank, struct pt_regs *regs)
-{
-	u32 stat, irq;
+/**
+ * bmc2835_dispatch_irq() - translate bank/bit to kernel IRQ and dispatch
+ * @bank:	Register bank (BASE, GPU1, or GPU2).
+ * @bits:	Pending interrupt status bits.
+ * @regs:	register pointer passed on to the handler.
+ *
+ * The bits passed are the full set of bits read from register.  Only the lowest
+ * order bit is translated and dispatched. Dispatching the other IRQs requires
+ * re-reading the register and calling this function again.
+ */
+static void __exception_irq_entry bmc2835_dispatch_irq(int bank, u32 bits,
+	struct pt_regs *regs) {
+	unsigned int irq = ctrlr.hwirq_xlat[MAKE_HWIRQ(bank, ffs(bits) - 1)];
 
-	while ((stat = readl_relaxed(intc.pending[bank]))) {
-		irq = MAKE_HWIRQ(bank, ffs(stat) - 1);
-		handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
+	if (irq == BAD_IRQ_NUM) {
+		BUG();
+		return;
 	}
-}
-
-static void armctrl_handle_shortcut(int bank, struct pt_regs *regs,
-	u32 stat)
-{
-	u32 irq = MAKE_HWIRQ(bank, shortcuts[ffs(stat >> SHORTCUT_SHIFT) - 1]);
-	handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
+	handle_IRQ(irq_linear_revmap(ctrlr.domain, irq), regs);
 }
 
 static asmlinkage void __exception_irq_entry bcm2835_handle_irq(
 	struct pt_regs *regs)
 {
-	u32 stat, irq;
-
-	while ((stat = readl_relaxed(intc.pending[0]) & BANK0_VALID_MASK)) {
-		if (stat & BANK0_HWIRQ_MASK) {
-			irq = MAKE_HWIRQ(0, ffs(stat & BANK0_HWIRQ_MASK) - 1);
-			handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
-		} else if (stat & SHORTCUT1_MASK) {
-			armctrl_handle_shortcut(1, regs, stat & SHORTCUT1_MASK);
-		} else if (stat & SHORTCUT2_MASK) {
-			armctrl_handle_shortcut(2, regs, stat & SHORTCUT2_MASK);
-		} else if (stat & BANK1_HWIRQ) {
-			armctrl_handle_bank(1, regs);
-		} else if (stat & BANK2_HWIRQ) {
-			armctrl_handle_bank(2, regs);
-		} else {
-			BUG();
+	u32 stat;
+
+	while ((stat = readl_relaxed(ctrlr.base_pend) & BASE_READABLE)) {
+		if (stat & BASE_IRQS) {
+			bmc2835_dispatch_irq(BASE_BANK, stat & BASE_IRQS, regs);
+			continue;
+		}
+		if (stat & BASE_GPU1) {
+			stat = readl_relaxed(ctrlr.gpu1_pend) & GPU1_IRQS;
+			if (stat)
+				bmc2835_dispatch_irq(GPU1_BANK, stat, regs);
+			continue;
 		}
+		if (stat & BASE_GPU2) {
+			stat = readl_relaxed(ctrlr.gpu2_pend) & GPU2_IRQS;
+			if (stat)
+				bmc2835_dispatch_irq(GPU2_BANK, stat, regs);
+			continue;
+		}
+		BUG();
 	}
 }
 
-IRQCHIP_DECLARE(bcm2835_armctrl_ic, "brcm,bcm2835-armctrl-ic", armctrl_of_init);
+/**
+ * int shortcut_xlat[] - normalisation of BASE pending register shortcut bits.
+ *
+ * Bits 10..20 of the BASE pending register are shortcuts.  For example, bit 19
+ * is the UART IRQ bit.  As an internal IRQ number, that would be 83.  Its
+ * canonical internal IRQ number is 57 (GPU1, bit 25).  When this array is
+ * copied into the appropriate subrange of ctrlr.hwirq_xlat, then
+ * ctrlr.hwirq_xlat[83] = 57.
+ */
+static int shortcut_xlat[] __initconst = {
+	MAKE_HWIRQ(GPU1_BANK,  7),
+	MAKE_HWIRQ(GPU1_BANK,  9),
+	MAKE_HWIRQ(GPU1_BANK, 10),
+	MAKE_HWIRQ(GPU1_BANK, 18),
+	MAKE_HWIRQ(GPU1_BANK, 19),
+	MAKE_HWIRQ(GPU2_BANK, 21),
+	MAKE_HWIRQ(GPU2_BANK, 22),
+	MAKE_HWIRQ(GPU2_BANK, 23),
+	MAKE_HWIRQ(GPU2_BANK, 24),
+	MAKE_HWIRQ(GPU2_BANK, 25),
+	MAKE_HWIRQ(GPU2_BANK, 30),
+};
+
+static int __init bmc2835_of_init(struct device_node *node,
+	struct device_node *parent)
+{
+	int i;
+
+	ctrlr.iobase = of_iomap(node, 0);
+	if (!ctrlr.iobase)
+		panic("%s: unable to map IC registers\n", node->full_name);
+	ctrlr.domain = irq_domain_add_linear(node, NR_CHIP_IRQS, &bmc2835_ops,
+		&ctrlr);
+	if (!ctrlr.domain)
+		panic("%s: unable to create IRQ domain\n", node->full_name);
+
+	ctrlr.base_pend = ctrlr.iobase + BASE_PEND_OFF;
+	ctrlr.gpu1_pend = ctrlr.iobase + GPU1_PEND_OFF;
+	ctrlr.gpu2_pend = ctrlr.iobase + GPU2_PEND_OFF;
+	for (i = 0; i != NR_BANKS; i++) {
+		ctrlr.enable[i] = ctrlr.iobase + reg_enable[i];
+		ctrlr.disable[i] = ctrlr.iobase + reg_disable[i];
+	}
+
+	for (i = 0; i != NR_CHIP_IRQS; i++)
+		ctrlr.hwirq_xlat[i] = i;
+	for (i = NR_CHIP_IRQS; i != BITS_PER_REG * NR_BANKS; i++)
+		ctrlr.hwirq_xlat[i] = BAD_IRQ_NUM;
+	for (i = 0; i != ARRAY_SIZE(shortcut_xlat); i++)
+		ctrlr.hwirq_xlat[i + NR_CHIP_IRQS + 2] = shortcut_xlat[i];
+
+	for (i = 0; i != NR_CHIP_IRQS; i++) {
+		int irq = irq_create_mapping(ctrlr.domain, i);
+
+		BUG_ON(irq <= 0);
+		irq_set_chip_and_handler(irq, &bmc2835_chip, handle_level_irq);
+		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+	}
+
+	set_handle_irq(bcm2835_handle_irq);
+	return 0;
+}
+
+IRQCHIP_DECLARE(bcm2835_armctrl_ic, "brcm,bcm2835-armctrl-ic", bmc2835_of_init);
-- 
1.8.3.1

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Stephen Warren]

On 09/24/2013 02:09 AM, Craig McGeachie wrote:
...
> On 09/24/2013 03:38 PM, Stephen Warren wrote:
>> On 09/21/2013 03:18 AM, Craig McGeachie wrote:
>>> The justification for doing this is:
>>>   * Simplify the decode and dispatch logic to make it easier to under-
>>>     stand and maintain.
>>>   * Re-arrange the hardware IRQ numbers to match the numbering scheme of
>>>     the FIQ register.
>>
>> What's the benefit of that change? The bank numbers in the existing
>> driver match the register naming in the HW (base/pend1/pend2 for bank
>> 0/1/2).
> 
> Not really. The different types registers from each bank have different
> bank orderings.  The pending registers go
> base=0x200/gpu0=0x204/gpu1=0x208.  The enable registers go
> gpu0=0x210/gpu1=0x214/base=0x218. The disable registers go
> gpu0=0x21C/gpu1=0x220/base=0x224.  The choice of bank numbering is
> arbitrary, but given that the pending registers require special handling
> anyway, I preferred that of the enable/disable registers. And it matches
> the implied order given the numbering in the FIQ control register.

Ah yes, I guess the enable/status registers are indeed in a different order.

I think this highlights part of why I preferred some aspects of the
original code; it used a set of lookup tables to map from the SW bank
number to the register addresses. There were no complicated calculations
here; just a simple table lookup. It would be possible to re-order the
banks without changing this.

>> Further on, one of your added comments says "This is the same
>> numbering scheme used in bits 0..6 of the FIQ register."; what about any
>> other bits in the FIQ register - do they still not match the renumbered
>> scheme, or are there just fewer bits in the FIQ register? Either way, I
>> don't understand why it's useful for the internal IRQ IDs to match the
>> FIQ register.
> 
> If you represent the UART interrupt as 57, then if you wish to treat the
> UART as a FIQ you simply write to the FIQ register like so:
>   writel_relaxed((BIT(7) | 57), ctrlr.iobase + 0x0C)
> and write the appropriate bit to the disable registers.  Any other
> numbering scheme for hardware IRQs requires translation to 57 first so
> that you can write the correct 7 bit unsigned integer to bits 6:0.

Ah right, I see. Renumbering the banks internally may make sense then.
You'd still end up needing to translate between the DT bank IDs and
internal bank IDs though, so I don't know how useful this would be.

>> I guess the table of interrupt IDs in BCM2835_ARM_Peripherals.pdf
>>
>>>   * Restore the flow of control that re-reads the base pending register
>>>     after handling any interrupt. The current version handles all
>>>     interrupts found in a GPU pending register before re-reading the
>>>     base pending register. In the original Broadcom assembly code, there
>>>     appear to be defect tracking numbers next to code inserted to create
>>>     this behaviour.

BTW, you mentioned "restore" above; while your patch did align the
upstream Linux driver more closely with the sample code in the PDF, I'm
not sure that any Linux port actually previously followed that example,
did it? I'm nit-picking about the semantics of the word "restore" here:-)

>> IIRC, the existing Linux
>> driver already does this (ignores the bits in PEND1 that are
>> duplicates).
> 
> I don't think it does. If it doesn't find any other interrupts in the
> base pending register it handles the first bank:
>
>         } else if (stat & BANK1_HWIRQ) {
>             armctrl_handle_bank(1, regs);
> 
> Then it reads the bits in tight loop handling all that it finds.
>     while ((stat = readl_relaxed(intc.pending[bank]))) {
>         irq = MAKE_HWIRQ(bank, ffs(stat) - 1);
>         handle_IRQ(irq_linear_revmap(intc.domain, irq), regs);
>     }

Oh yes, the bug is indeed present. That should indeed be fixed.

> So long as new interrupts come in on this register, the base pending
> register will never be re-read.  Still, there could be memory access
> ordering and caching behaviour that I'm completely ignorant of, so I
> could be completely wrong. And it's probably unlikely that interrupts
> come in that fast.
> 
>> This logic doesn't imply that one can't loop reading the
>> PEND1 register.
> 
> No. I just felt that always reading the base pending register and
> processing the short cut interrupts there lent itself to a natural
> interrupt prioritisation.

True. I suppose the shortcuts are indeed there because they're
theoretically higher priority .

>> I'm not sure why a hwirq_xlat table lookup is needed; why not just
>> return the HW IRQ number directly like the existing code?
> 
> Have I managed to cover this elsewhere?

Well, the existing code manages without any kind of lookup table, since
it numbers the banks in the same order as the enable/disable registers,
and hard-codes the bank ID parameter to MAKE_HWIRQ() when handling IRQs.
To my mind, this makes the code a lot simpler...

>>> +static struct irq_chip bmc2835_chip = {
>>> +    .name = "bmc2835-level",
>>> +    .irq_mask = bmc2835_mask_irq,
>>> +    .irq_unmask = bmc2835_unmask_irq,
>>> +    .irq_ack =  bmc2835_mask_irq,
>>> +};
>>
>> Why should an IRQ be masked by the ack operation?
> 
> To be honest, I don't know for sure.  I took my cue from
> Documentation/arm/Interrupts.
> 
> ack    - required.  May be the same function as mask for IRQs
>          handled by do_level_IRQ.
> 
> Some other irq_chips are set up the same way and I can't see any other
> way of implementing an acknowledgement function.  And my experience with
> a never ending flow of interrupts while writing the mailbox driver made
> me think it might be good idea for the BMC2835 as well.

OK, it makes sense to implement that.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Stephen Warren]

On 09/25/2013 12:00 AM, Craig McGeachie wrote:
> On 09/24/2013 03:38 PM, Stephen Warren wrote:
>>> +#define BAD_IRQ_NUM    -1
>>
>> That should be 0; Using -1 for invalid IRQ is deprecated.
> 
> Could you please point me to something that describes this? I can't find
> anything that limits the internal representations used by the driver for
> an interrupt controller.  That, and the design of
> include/linux/irqdomain.h and kernel/irq/irqdomain.c implies that a
> 0-based hwirq numbering scheme is the natural order of things.

I suppose there isn't any limitation on the driver-internal
representation, so this is OK from that perspective. I'd still prefer if
this constant simply weren't needed, just like it isn't in the current
code, simply because the entire range of hwirq numbers is valid, so
there's no need for a sparse lookup table, and hence no concept of an
invalid ID. The only exception would be if you're trying to align the
hwirq numbers with that FIQ register, and there truly are some invalid
values in the FIQ numbering scheme?

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Stephen Warren]

On 09/26/2013 02:19 AM, Craig McGeachie wrote:
> On 09/24/2013 03:38 PM, Stephen Warren wrote:
>> I'm not actually sure that's a good change. If we read e.g. PEND1 and
>> saw 4 bits asserted, why wouldn't we optimize by handling each of those
>> 4 bits. Re-reading the PEND1 register for each bit, and presumably
>> seeing the exact same set of remaining bits still set (or even more),
>> just seems like a waste. We know we're going to have to process all the
>> interrupts, so why not just process them?
> 
> 
> On 09/25/2013 11:33 PM, Simon Arlott wrote:
>> On Sat, September 21, 2013 09:14, Craig McGeachie wrote:
>>>   * Restore the flow of control that re-reads the base pending register
>>>     after handling any interrupt. The current version handles all
>>>     interrupts found in a GPU pending register before re-reading the
>>>     base pending register. In the original Broadcom assembly code, there
>>>     appear to be defect tracking numbers next to code inserted to create
>>>     this behaviour.
>>
>> This was by design so that continuous interrupts in a bank did not impact
>> progress of interrupts in other bank. If there are defects with this
>> strategy, then check that they do not exist in your interrupt handlers
>> instead of the interrupt controller itself. Unless there is a real bug
>> in the interrupt controller, you're decreasing the performance by
>> re-reading the base pending register every time.
> 
> I don't understand the concern with re-reading two volatile registers
> between each dispatch.  Given the amount of processing that occurs
> between the call to handle_IRQ and the calls to the possibly multiple
> registered interrupt handlers, plus the processing that the handlers
> perform (even if they are implemented as top/bottom halves), I think the
> performance overhead of the two extra reads is vanishingly small.  In
> fact, I think that focusing on eliminating them is premature
> optimisation.  Developers are notoriously bad at identifying performance
> hotspots through visual inspection.
> 
> The point about the registers being volatile is important.  It's a C
> keyword for a very good reason.

Volatile as a keyword isn't especially useful for registers though,
since register IO tends to need various barriers as well, but anyway...

I do agree that it's likely best if the driver processes interrupts in
the priority order that the HW designers came up with. So, I'm open to
that change. This might make a difference to some time-critical
shortcuts like the PCM (audio) interrupt.

[PATCH v3] irq: bmc2835: Re-implement the hardware IRQ handler.

[Stephen Warren]

On 09/27/2013 03:57 AM, Craig McGeachie wrote:
> Force the re-reading of the pending status registers before dispatching
> the next interrupt request.  There is no guarantee that the values will
> remain valid.

That last sentence isn't really the point. The point isn't that the
value previously read may become somehow invalid, but more that some
extra bits may become set. Perhaps this is quibling over words, but I
don't see that as the value becoming invalid, just out-of-date. Perhaps
rewrite that sentence as:

It is possible while processing one interrupt, new interrupts become
pending, and the new interrupts are higher priority and should be
handled first.

> Actions taken by interrupt handlers could handle and
> acknowledge multiple interrupts.  For example, the DMA controller has
> it's own view of the interrupt status of all channels rather than just
> the one that the interrupt was dispatched for.  The cost of re-reading
> is small.  The cost of unnecessary dispatches is large.

DMA is only one interrupt at the top-level, I believe; the multiple
interrupt sources are sub-interrupts within the DMA controller. Naively,
I'd expect it to be very unlikely that one top-level IRQ handler
directly caused an unrelated top-level IRQ status to clear, although I'm
sure someone can come up with some exceptions:-)

> Reorder the internal numbering of interrupt banks so that hardware IRQ
> numbers range from 0 to 71, and match the natural numbering implied by
> the FIQ source register.  This is in anticipation of being to implement
> a viable mechanism for registering one of the available interrupts as a
> fast interrupt.  Preferably based on device tree configuration.

DT shouldn't specify whether an interrupt should be handled as
FIQ-vs-non-FIQ, since that's a SW configuration issue, rather than a
pure HW description.

> Enable device tree interrupt specifications to identify interrupts with
> shortcut bits in the base register by either the shortcut bit, or the
> canonical GPU register bit.   E.g. UART can be either <0, 19> or <2,
> 25>.

Hmmm. Is there a benefit to allowing that? One defined way feels better
than multiple ways of specifying the same thing.

> diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c

I think I'll hold off reviewing the patch itself until you've had a
chance to look at the other responses I just sent, or I'll just be
saying the same things.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:01 PM, Stephen Warren wrote:
>>>> +static struct irq_chip bmc2835_chip = {
>>>> +    .name = "bmc2835-level",
>>>> +    .irq_mask = bmc2835_mask_irq,
>>>> +    .irq_unmask = bmc2835_unmask_irq,
>>>> +    .irq_ack =  bmc2835_mask_irq,
>>>> +};
>>>
>>> Why should an IRQ be masked by the ack operation?
>>
>> To be honest, I don't know for sure.  I took my cue from
>> Documentation/arm/Interrupts.
>>
>> ack    - required.  May be the same function as mask for IRQs
>>           handled by do_level_IRQ.
>>
>> Some other irq_chips are set up the same way and I can't see any other
>> way of implementing an acknowledgement function.  And my experience with
>> a never ending flow of interrupts while writing the mailbox driver made
>> me think it might be good idea for the BMC2835 as well.
>
> OK, it makes sense to implement that.
>

Possibly not.  I've dug a little deeper and followed the call chain 
through handle_level_irq() to mask_ack_irq().

static inline void mask_ack_irq(struct irq_desc *desc)
{
	if (desc->irq_data.chip->irq_mask_ack)
		desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
	else {
		desc->irq_data.chip->irq_mask(&desc->irq_data);
		if (desc->irq_data.chip->irq_ack)
			desc->irq_data.chip->irq_ack(&desc->irq_data);
	}
	irq_state_set_masked(desc);
}

I've not looked further to find out why code and documentation are 
different, but it looks like the .irq_ack is unnecessary, but harmless 
beyond an unneeded function call.  I plan to drop it.

Cheers,
Craig.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:04 PM, Stephen Warren wrote:
> On 09/25/2013 12:00 AM, Craig McGeachie wrote:
>> On 09/24/2013 03:38 PM, Stephen Warren wrote:
>>>> +#define BAD_IRQ_NUM    -1
>>>
>>> That should be 0; Using -1 for invalid IRQ is deprecated.
>>
>> Could you please point me to something that describes this? I can't find
>> anything that limits the internal representations used by the driver for
>> an interrupt controller.  That, and the design of
>> include/linux/irqdomain.h and kernel/irq/irqdomain.c implies that a
>> 0-based hwirq numbering scheme is the natural order of things.
>
> I suppose there isn't any limitation on the driver-internal
> representation, so this is OK from that perspective. I'd still prefer if
> this constant simply weren't needed, just like it isn't in the current
> code, simply because the entire range of hwirq numbers is valid, so
> there's no need for a sparse lookup table, and hence no concept of an
> invalid ID. The only exception would be if you're trying to align the
> hwirq numbers with that FIQ register, and there truly are some invalid
> values in the FIQ numbering scheme?

No, the FIQ numbering scheme has no invalid values in the range 0 to 71. 
  The field id 7 bits, but the invalid values that you must not write 
are 72 to 127.

The BAD_IRQ_NUM constant and the translation array really came about 
because I was looking for a single mechanism for all special case 
handling.  Together they replace:
  * A special check for the limited number of bits in the base pending
    register.
  * bank_irqs[], which is used only during initialisation.
  * The two different functions, armctrl_handle_shortcut() and
    armctrl_handle_bank() for handling interrupts from the GPU banks.
  * The separation of base pending into three different masks
    (BANK0_HWIRQ_MASK, SHORTCUT1_MASK, SHORTCUT2_MASK)
  * The SHORTCUT_SHIFT required to get a sensible number from the
    shortcut bits in base pending.

Bits 10 to 20 of base pending have no simple relationship to the 
corresponding bits in the GPU pending registers.  The mapping could be 
implemented a piecewise function with a sequence of if statements.  But 
since the domain is small and discrete, a straight forward one to one 
mapping seems easier.  Since the mapping had to be implemented anyway I 
decided to extend it to cover all 32 bits of the 3 registers.  That 
removed the need to have a different range check for the banks when 
reading a DT value.  It removed the need to treat different ranges of 
the base pending bits differently with an if-else statement. It gave the 
free benefit of allowing the DT value for a shortcut to be represented 
by either the GPU bit or the base shortcut bit.

The introduction of the constant just followed on from that. It marks 
which values, in a simple bank_nr * 32  + bit_nr, are invalid.  I chose 
used signed integers for the array, and chose -1, as being a marker 
value that could never be a valid hardware IRQ, since hardware IRQ 
numbers are unsigned int.

Annnd then ... I messed it up entirely.  Quite badly in fact.

In bmc2835_dispatch_irq()
   unsigned int irq = ctrlr.hwirq_xlat[MAKE_HWIRQ(bank, ffs(bits) - 1)];
   if (irq == BAD_IRQ_NUM) {

And in bmc2835_domain_xlate()
   unsigned int bank, hwirq;
   hwirq = ctrlr.hwirq_xlat[MAKE_HWIRQ(bank, intspec[1])];
   if (WARN_ON(hwirq == BAD_IRQ_NUM))

A rather bad mixing up of signed and unsigned.  I had it in my mind that 
I was being very careful about the conversion from signed to unsigned. 
Presumably my fingers didn't get the memo. Amazing what you find when 
checking that you really implemented what you're about to claim ... .

Cheers,
Craig.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:12 PM, Stephen Warren wrote:
> On 09/26/2013 02:19 AM, Craig McGeachie wrote:
>> I don't understand the concern with re-reading two volatile registers
>> between each dispatch.  Given the amount of processing that occurs
>> between the call to handle_IRQ and the calls to the possibly multiple
>> registered interrupt handlers, plus the processing that the handlers
>> perform (even if they are implemented as top/bottom halves), I think the
>> performance overhead of the two extra reads is vanishingly small.  In
>> fact, I think that focusing on eliminating them is premature
>> optimisation.  Developers are notoriously bad at identifying performance
>> hotspots through visual inspection.
>>
>> The point about the registers being volatile is important.  It's a C
>> keyword for a very good reason.
>
> Volatile as a keyword isn't especially useful for registers though,
> since register IO tends to need various barriers as well, but anyway...

I agree.  I hadn't really meant that the volatile would make register 
reading correct.  I really only meant it as a concept of values that 
might be changed by something else unknown.

I'm still a little uncertain about the correct reading of registers, 
especially here.  I tried to get a good understanding of readl versus 
readl_relaxed and what ordering guarantees were, or were not given. 
About the only thing I am sure of is the requirements for wmb() and 
rmb() given by section 1.3 of BCM2835_ARM_Peripherals.pdf.  And I don't 
know whether or not it should be applied to interrupt registers, or 
whether the two different readl functions remove the need for explicit 
barriers.

I do know that the barriers are required when you switch which 
peripheral IO memory you are reading/writing, and I think the interrupt 
controller counts as different to the various devices that the handlers 
will be interacting with.

Cheers,
Craig.

[PATCH v3] irq: bmc2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:23 PM, Stephen Warren wrote:
> On 09/27/2013 03:57 AM, Craig McGeachie wrote:

>> Actions taken by interrupt handlers could handle and
>> acknowledge multiple interrupts.  For example, the DMA controller has
>> it's own view of the interrupt status of all channels rather than just
>> the one that the interrupt was dispatched for.  The cost of re-reading
>> is small.  The cost of unnecessary dispatches is large.
>
> DMA is only one interrupt at the top-level, I believe; the multiple
> interrupt sources are sub-interrupts within the DMA controller. Naively,
> I'd expect it to be very unlikely that one top-level IRQ handler
> directly caused an unrelated top-level IRQ status to clear, although I'm
> sure someone can come up with some exceptions:-)

No.  It has 14 interrupts at the top level.  16..28,31 in the FIQ 
numbering scheme.  And <1 16> to <1 28>, <1 31> in the device tree 
numbering scheme.  Those 14 interrupts service 16 DMA channels.  Simon 
Arlott has identified a mapping of DMA channel to interrupt [1] which is 
surprising in that DMA channels 11 to 14 map onto interrupts <1 27> and 
<1 28> in a way which I can't see any obvious rationale for.

The iomem register 0x7E007fe0 is r/w for bits 15:0.  Each bit is the 
interrupt status for the corresponding DMA engine.  Bit 0 is DMA engine 
0, bit 15 is DMA engine, and so on. The way that I read this is that 
reading a set bit indicates a pending interrupt, and writing a clear bit 
acknowledges the interrupt, which should clear the appropriate pending 
status bit in interrupt controller.  The only example I know of that 
might provide some insight is the sdhci-bcm2708.c driver from
https://github.com/raspberrypi/linux.  I haven't read it yet.

Information on DMA channels and hardware interrupts is scarce.  Neither 
of two drivers at https://github.com/raspberrypi/linux offers any clues.

Given a DMA driver the works within the DMA engine framework, I can't 
see why the driver wouldn't read all the status bits from 0x7E007fe0 and 
simply handle all pending interrupts after being invoked to handle one.

>> Reorder the internal numbering of interrupt banks so that hardware IRQ
>> numbers range from 0 to 71, and match the natural numbering implied by
>> the FIQ source register.  This is in anticipation of being to implement
>> a viable mechanism for registering one of the available interrupts as a
>> fast interrupt.  Preferably based on device tree configuration.
>
> DT shouldn't specify whether an interrupt should be handled as
> FIQ-vs-non-FIQ, since that's a SW configuration issue, rather than a
> pure HW description.

Only one of the 72 interrupts could be FIQ.  I was about to write that I 
could not see a reasonable scheme for co-ordinating which one of these 
was mapped, but I might just have thought of something.  This is a rough 
idea, and hinges on more understanding of the flow of control for FIQ 
handling.

It would rely on using the translation table, even though I know you're 
not that taken with it.  Because the table runs up to 95, but has no 
valid mappings for 84 to 95, call init_FIQ() with 84 (I may be off by 
one or two here - I've not checked).  When configuring an FIQ, mask the 
normal IRQ bit, map 84 to the IRQ number, write the IRQ number to the 
FIQ selection register.  Reverse the steps to unconfigure.  The 
configuration could be dynamic and based on either module parameters or 
sysfs operations.

But this is a bit of a wild assed guess.  I don't know the processing 
flow for FIQ so I don't know how the hardware IRQ numbers are assigned, 
and if the flow of control passes a point where the mapping could be 
performed.

>> Enable device tree interrupt specifications to identify interrupts with
>> shortcut bits in the base register by either the shortcut bit, or the
>> canonical GPU register bit.   E.g. UART can be either <0, 19> or <2,
>> 25>.
>
> Hmmm. Is there a benefit to allowing that? One defined way feels better
> than multiple ways of specifying the same thing.

It's a reasonable question.  I thought so, but I couldn't defend that 
position.  My vague thought was that someone might find documentation 
about the interrupts from different sources. But given that anyone 
working in this area is going to need a minimum level of knowledge about 
interrupts, it's fair to expect them to work with a single canonical 
numbering. It was a natural fall out from using the translation table, 
which I liked and I don't think does any great harm.


Cheers,
Craig.

[1] 
https://github.com/lp0/linux/blob/rpi-dma-hack/arch/arm/boot/dts/bcm2835.dtsi

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:01 PM, Stephen Warren wrote:
>>> On 09/21/2013 03:18 AM, Craig McGeachie wrote:
>>>>    * Restore the flow of control that re-reads the base pending register
>>>>      after handling any interrupt. The current version handles all
>>>>      interrupts found in a GPU pending register before re-reading the
>>>>      base pending register. In the original Broadcom assembly code, there
>>>>      appear to be defect tracking numbers next to code inserted to create
>>>>      this behaviour.
>
> BTW, you mentioned "restore" above; while your patch did align the
> upstream Linux driver more closely with the sample code in the PDF, I'm
> not sure that any Linux port actually previously followed that example,
> did it? I'm nit-picking about the semantics of the word "restore" here:-)

When you refer to other ports, are you referring to the likes of 
omap2/tegra/u300?

Cheers,
Craig.

[PATCH] irq: bcm2835: Re-implement the hardware IRQ handler.

[Craig McGeachie]

On 10/02/2013 03:12 PM, Stephen Warren wrote:
> I do agree that it's likely best if the driver processes interrupts in
> the priority order that the HW designers came up with. So, I'm open to
> that change. This might make a difference to some time-critical
> shortcuts like the PCM (audio) interrupt.

FIQ would definitely make a difference.  I've tried playing audio out of 
the 3.5mm audio jack.  It sounds rubbish.  I think the issue might be 
the same as the one for getting decent PWM wave forms [1,2].  I found 
the links while researching how to implement FIQ support. 
Interestingly, a PWM driver is also a good candidate for FIQ.

Implementing FIQ was nothing like I expected, but none the less, I'm 
fairly certain that I've added FIQ support to the interrupt controller 
driver.  Other than normal interrupts seem to continue to behave well, I 
couldn't tell you how well FIQs behave.  That would require implementing 
an FIQ handler for some driver, which looks conceptually straight 
forward, but fiddly.

The implementation of an FIQ handler follow as per instruction from the 
introductory comment block in linux/arch/arm/kernel/fiq.c. The point 
about a relinquish function disabling FIQ before the next driver enables 
is important.  The interrupt controller driver only permits one FIQ 
enabled at any one time, and attempting to enable a second FIQ causes a 
kernel BUG().

I have not submitted it in the form of a patch, because I'm not sure of 
the current status of what you want changed or if you're even open to 
the possibility of changing the driver.  Still, if you're interested, 
you can always have a look at it here [3].

Cheers,
Craig.

[1] http://free-electrons.com/blog/fiq-handlers-in-the-arm-linux-kernel/
[2] http://elinux.org/images/2/27/0910-elce-fiq.pdf
[3] 
https://github.com/rickytarr/linux/blob/topic/interrupt/drivers/irqchip/irq-bcm2835.c