On 07/06/2017 18:25, Alexander Stein wrote:
On Wednesday, June 7, 2017, 2:37:41 PM CEST Phil Elwell wrote:
On 07/06/2017 13:07, Alexander Stein wrote:
On Wednesday 07 June 2017 12:11:45, Phil Elwell wrote:
Devices in the AUX block share a common interrupt line, with a register
indicating which devices have active IRQs. Expose this as a nested
interrupt controller to avoid IRQ sharing problems (easily observed if
UART1 and SPI1/2 are enabled simultaneously).
Signed-off-by: Phil Elwell <phil@xxxxxxxxxxxxxxx>
---
drivers/clk/bcm/clk-bcm2835-aux.c | 120
++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+)
diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
b/drivers/clk/bcm/clk-bcm2835-aux.c index bd750cf..41e0702 100644
--- a/drivers/clk/bcm/clk-bcm2835-aux.c
+++ b/drivers/clk/bcm/clk-bcm2835-aux.c
[...]
+struct auxirq_state {
+ void __iomem *status;
+ u32 enables;
+ struct irq_domain *domain;
+ struct regmap *local_regmap;
+};
+
+static struct auxirq_state auxirq __read_mostly;
+
+static irqreturn_t bcm2835_auxirq_handler(int irq, void *dev_id)
+{
+ u32 stat = readl_relaxed(auxirq.status);
+ u32 masked = stat & auxirq.enables;
Doesn't this hide any spurious interrupts? Is this acceptable? I mean
getting informed about spurious interrupts seems nice to me, as it
indicates a hardware/configuration problem.
Thanks for the reply. This interrupt handler is capable of dispatching
multiple interrupts but must return a single value - IRQ_HANDLED or
IRQ_NONE. I've assumed that returning IRQ_NONE repeatedly will trigger the
spurious interrupt detection.
This implementation returns IRQ_HANDLED if any unmasked interrupts are
raised, otherwise it returns IRQ_NONE. Therefore provided any spurious
interrupt isn't always coincident with a real interrupt then it ought
eventually to be identified as spurious. The alternative - returning
IRQ_NONE if there are any spurious interrupts - seems prone to causing
collateral damage.
What did you have in mind?
I was wondering about "stat & auxirq.enables". With that you wouldn't forward
any spurious interrupts to e.g. SPI1. I don't know which way is better,
returning IRQ_NONE is a masked interrupt happens, or pass it further down. I
guess this also raises a warning if the SPI driver also returns IRQ_NONE.
That makes sense. Although my instinct was to ignore the spurious interrupt
as early as possible, it is better to let Linux handle in it. Without that
use of the enables field there is no need for the mask and unmask methods,
so the driver becomes even simpler.
I'll incorporate your suggestions into v2.
BTW: Is it even allowed to call generic_handle_irq on a masked irq?
Ah, I think that's just a naming confusion - the variable contains bits for
interrupts that are active and enabled, i.e. masked in, not masked out.
Phil
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
