Re: [PATCH 2/2] irqchip/mips-gic: Fix Local compare interrupt

[Paul Burton <paul.burton@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

Hi Matt,

On Tuesday, 11 April 2017 01:20:35 PDT Matt Redfearn wrote:
> On 10/04/17 23:06, Paul Burton wrote:
> > On Friday, 31 March 2017 04:05:32 PDT Matt Redfearn wrote:
> >> Commit 4cfffcfa5106 ("irqchip/mips-gic: Fix local interrupts") added
> >> mapping of several local interrupts during initialisation of the gic
> >> driver. This associates virq numbers with these interrupts.
> >> Unfortunately, as not all of the interrupts are mapped in hardware
> >> order, when drivers subsequently request these interrupts they conflict
> >> with the mappings that have already been set up. For example, this
> >> manifests itself in the gic clocksource driver, which fails to probe
> >> with the message:
> >> 
> >> clocksource: GIC: mask: 0xffffffffffffffff max_cycles: 0x7350c9738,
> >> max_idle_ns: 440795203769 ns
> >> GIC timer IRQ 25 setup failed: -22
> >> 
> >> This is because virq 25 (the correct IRQ number specified via device
> >> tree) was allocated to the PERFCTR interrupt (and 24 to the timer, 26 to
> >> the FDC).
> > 
> > I'm confused by this - the DT doesn't specify VIRQs, it specifies hardware
> > IRQ numbers. Which VIRQ is used should be irrelevant. Is this on a system
> > using gic_clocksource_init() from platform code? (Malta?) and therefore
> > relying on MIPS_GIC_IRQ_BASE?
> 
> Yes, this is on Malta, which as you say, uses MIPS_GIC_IRQ_BASE. On
> Malta that ends up, through the definition of I8259A_IRQ_BASE and
> MIPS_CPU_IRQ_BASE, to be 24. Therefore hardware interrupt 1 of the GIC
> ends up expecting to be allocated at virq 25. But since 4cfffcfa5106,
> that virq number was allocated to the PERFCTR interrupt. Everything
> about the order-dependent and hardcoded bases of Maltas IRQs seems bad
> and needs looking at but this was the easiest fix for this cycle.
> 
> > If so I think this would be much more cleanly fixed by moving to probe the
> > clocksource using DT
> 
> Not sure that would help if Maltas expected virq for this source had
> already been allocated?

Well the problem is that Malta expects a particular VIRQ - if it instead 
probed the clock event driver via DT, which specifies a hardware interrupt 
number, then that problem goes away & it doesn't matter which VIRQ is used.

For example the generic platform (including the Malta board support for it in 
the downstream engineering kernels) ought to work fine without this because 
there isn't an expectation for a particular VIRQ. Hopefully I'll have time to 
submit some more of that code for the v4.13 cycle & we can eventually scrap 
all of these hardcoded VIRQs along with the mti-malta board code that commits 
the sins.

An alternative might be to have the Malta board code use irq_create_mapping() 
to map the hardware IRQ to a VIRQ rather than hardcoding the VIRQ, which would 
be an improvement but would then require that the board code have access to 
the GIC's struct irq_domain. Given that we want to move Malta towards DT 
anyway that doesn't seem worth much effort.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]