Re: [PATCH v8 06/11] irqchip: mips-cpu: Convert to simple domain

[Marc Zyngier]

On 2020-03-25 13:07, Jiaxun Yang wrote:
> ä 2020å3æ25æ GMT+08:00 äå9:00:01, Marc Zyngier <maz@xxxxxxxxxx> åå:
> > On 2020-03-25 12:37, Thomas Bogendoerfer wrote:
> > > On Wed, Mar 25, 2020 at 11:54:59AM +0800, Jiaxun Yang wrote:
> > > > The old code is using legacy domain to setup irq_domain for CPU
> > > > interrupts
> > > > which requires irq_desc to be preallocated.
> > > >
> > > > However, when MIPS_CPU_IRQ_BASE >= 16, irq_desc for CPU IRQs may end
> >
> > > > up
> > > > unallocated and lead to incorrect behavior.
> > > >
> > > > Thus we convert the legacy domain to simple domain which can
> > allocate
> > > > irq_desc during initialization.
> > > >
> > > > Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > > > Co-developed-by: Huacai Chen <chenhc@xxxxxxxxxx>
> > > > Signed-off-by: Huacai Chen <chenhc@xxxxxxxxxx>
> > > > Reviewed-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > > ---
> > > >  drivers/irqchip/irq-mips-cpu.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-mips-cpu.c
> > > > b/drivers/irqchip/irq-mips-cpu.c
> > > > index 95d4fd8f7a96..c3cf7fa76424 100644
> > > > --- a/drivers/irqchip/irq-mips-cpu.c
> > > > +++ b/drivers/irqchip/irq-mips-cpu.c
> > > > @@ -251,7 +251,7 @@ static void __init __mips_cpu_irq_init(struct
> > > > device_node *of_node)
> > > >  	clear_c0_status(ST0_IM);
> > > >  	clear_c0_cause(CAUSEF_IP);
> > > >
> > > > -	irq_domain = irq_domain_add_legacy(of_node, 8, MIPS_CPU_IRQ_BASE,
> > 0,
> > > > +	irq_domain = irq_domain_add_simple(of_node, 8, MIPS_CPU_IRQ_BASE,
> > > >  					   &mips_cpu_intc_irq_domain_ops,
> > > >  					   NULL);
> > >
> > > this breaks at least IP30 and guess it will break every platform
> > where
> > > MIPS_CPU_IRQ_BASE == 0. add_legacy will always do
> > > irq_domain_associate_many(),
> > > while add_simple doesn't do it, if first_irq == 0.
> > >
> > > Marc, what is the reason not doing it all the time ? What's the
> > correct
> > > way here to work with irq_domain_add_simple() in this case ?
> >
> > On a fully DT-ified platform, using non-legacy irqdomains, virtual
> > interrupts
> > are allocated as a "random" number, depending on the order of
> > allocation,
> > and on demand.
> >
> > The first_irq hack in irq_domain_add_simple() is just a way to still
> > allocate
> > descriptors upfront (and I wish we could drop it...).
> >
> > If you have legacy code that "knows" about the relationship between
> > Linux's
> > virtual interrupt and the hwirq (that is only meaningful to the
> > interrupt
> > controller), you're screwed, and need to stick to the legacy 
> > irqdomain.
> >
> > It feels like the MIPS code is squarely in the latter case, so I guess
> > this
> > patch is probably the wrong thing to do for this architecture.
>
> So probably we can use legacy domain when  MIPS IRQ BASE is in the
> range of legacy IRQ
> and switch to simple domain when it's not in that range?

No, see below.

> Here in Loongson systems IRQ 0-15 is occupied by I8259 so I did this 
> hack.

Well, if you have to consider which Linux IRQ gets assigned,
then your platform is definitely not ready for non-legacy
irqdomains. Just stick to legacy for now until you have removed
all the code that knows the hwirq mapping.

        M.
--
Jazz is not dead. It just smells funny...