| Message ID | 20200325035537.156911-7-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Modernize Loongson64 Machine v8 | expand |
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@flygoat.com> > Co-developed-by: Huacai Chen <chenhc@lemote.com> > Signed-off-by: Huacai Chen <chenhc@lemote.com> > Reviewed-by: Marc Zyngier <maz@kernel.org> > --- > 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 ? Thomas.
于 2020年3月25日 GMT+08:00 下午8:37:42, Thomas Bogendoerfer <tsbogend@alpha.franken.de> 写到: >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@flygoat.com> >> Co-developed-by: Huacai Chen <chenhc@lemote.com> >> Signed-off-by: Huacai Chen <chenhc@lemote.com> >> Reviewed-by: Marc Zyngier <maz@kernel.org> >> --- >> 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 ? I guess there is a inconsistent about whether IRQ 0 is a valid IRQ. In many places we consider IRQ 0 is invalid but here it should be valid. Thanks. > >Thomas.
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@flygoat.com> >> Co-developed-by: Huacai Chen <chenhc@lemote.com> >> Signed-off-by: Huacai Chen <chenhc@lemote.com> >> Reviewed-by: Marc Zyngier <maz@kernel.org> >> --- >> 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. Thanks, M.
于 2020年3月25日 GMT+08:00 下午9:00:01, Marc Zyngier <maz@kernel.org> 写到: >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@flygoat.com> >>> Co-developed-by: Huacai Chen <chenhc@lemote.com> >>> Signed-off-by: Huacai Chen <chenhc@lemote.com> >>> Reviewed-by: Marc Zyngier <maz@kernel.org> >>> --- >>> 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? Here in Loongson systems IRQ 0-15 is occupied by I8259 so I did this hack. Thanks. > >Thanks, > > M.
On 2020-03-25 13:07, Jiaxun Yang wrote: > 于 2020年3月25日 GMT+08:00 下午9:00:01, Marc Zyngier <maz@kernel.org> 写到: >> 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@flygoat.com> >>>> Co-developed-by: Huacai Chen <chenhc@lemote.com> >>>> Signed-off-by: Huacai Chen <chenhc@lemote.com> >>>> Reviewed-by: Marc Zyngier <maz@kernel.org> >>>> --- >>>> 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.
于 2020年3月25日 GMT+08:00 下午9:57:49, Marc Zyngier <maz@kernel.org> 写到: >On 2020-03-25 13:07, Jiaxun Yang wrote: >> 于 2020年3月25日 GMT+08:00 下午9:00:01, Marc Zyngier <maz@kernel.org> 写到: >>> 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@flygoat.com> >>>>> Co-developed-by: Huacai Chen <chenhc@lemote.com> >>>>> Signed-off-by: Huacai Chen <chenhc@lemote.com> >>>>> Reviewed-by: Marc Zyngier <maz@kernel.org> >>>>> --- >>>>> 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. Thanks. So I have to allocate irq_desc here in driver manually? > > M.
On 2020-03-25 13:59, Jiaxun Yang wrote: [...] >>> 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. > > Thanks. > > So I have to allocate irq_desc here in driver manually? No, you are probably better off just dropping this patch, as MIPS doesn't seem to be ready for a wholesale switch to virtual interrupts. M.
于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> 写到: >On 2020-03-25 13:59, Jiaxun Yang wrote: > >[...] > >>>> 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. >> >> Thanks. >> >> So I have to allocate irq_desc here in driver manually? > >No, you are probably better off just dropping this patch, as MIPS >doesn't seem to be ready for a wholesale switch to virtual interrupts. It can't work without this patch. Legacy domain require IRQ number within 0-15 however it's already occupied by i8259 or "HTPIC" driver. Previously Loongson even didn't enable IRQ domain so it's not a problem. > > M.
On 2020-03-25 14:31, Jiaxun Yang wrote: > 于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> 写到: >> On 2020-03-25 13:59, Jiaxun Yang wrote: >> >> [...] >> >>>>> 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. >>> >>> Thanks. >>> >>> So I have to allocate irq_desc here in driver manually? >> >> No, you are probably better off just dropping this patch, as MIPS >> doesn't seem to be ready for a wholesale switch to virtual interrupts. > > It can't work without this patch. > > Legacy domain require IRQ number within 0-15 > however it's already occupied by i8259 or "HTPIC" driver. > > Previously Loongson even didn't enable IRQ domain so it's not a > problem. Then your platform is breaking some fundamental assumption that the rest of the MIPS architecture seem to rely on. You could test for the base IRQ being 0 and create a legacy domain in this case, but that's really a horrible hack. I'm pretty worried about having to address this just 4 days away from the merge window TBH, as this code hasn't been in -next at all. That's really Thomas' call, but I'm not very enthusiastic. M.
On Wed, Mar 25, 2020 at 10:31:21PM +0800, Jiaxun Yang wrote: > > > 于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> 写到: > >On 2020-03-25 13:59, Jiaxun Yang wrote: > > > >[...] > > > >>>> 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. > >> > >> Thanks. > >> > >> So I have to allocate irq_desc here in driver manually? > > > >No, you are probably better off just dropping this patch, as MIPS > >doesn't seem to be ready for a wholesale switch to virtual interrupts. > > It can't work without this patch. > > Legacy domain require IRQ number within 0-15 > however it's already occupied by i8259 or "HTPIC" driver. what's the problem here ? AFAIK there could be more than one legacy domain, at least that's what at least IP22/SNI in MIPS world are doing. Thomas.
于 2020年3月25日 GMT+08:00 下午11:04:37, Thomas Bogendoerfer <tsbogend@alpha.franken.de> 写到: >On Wed, Mar 25, 2020 at 10:31:21PM +0800, Jiaxun Yang wrote: >> >> >> 于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> 写到: >> >On 2020-03-25 13:59, Jiaxun Yang wrote: >> > >> >[...] >> > >> >>>> 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. >> >> >> >> Thanks. >> >> >> >> So I have to allocate irq_desc here in driver manually? >> > >> >No, you are probably better off just dropping this patch, as MIPS >> >doesn't seem to be ready for a wholesale switch to virtual >interrupts. >> >> It can't work without this patch. >> >> Legacy domain require IRQ number within 0-15 >> however it's already occupied by i8259 or "HTPIC" driver. > >what's the problem here ? AFAIK there could be more than one >legacy domain, at least that's what at least IP22/SNI in MIPS world >are doing. MIPS_IRQ_BASE must be higher than 15, otherwise it will conflict with i8259. However we have only preallocated irq_desc for 0-15. And legacy domain require irq_desc being preallocated. > >Thomas.
On Wed, Mar 25, 2020 at 11:09:10PM +0800, Jiaxun Yang wrote: > > > 于 2020年3月25日 GMT+08:00 下午11:04:37, Thomas Bogendoerfer <tsbogend@alpha.franken.de> 写到: > >On Wed, Mar 25, 2020 at 10:31:21PM +0800, Jiaxun Yang wrote: > >> > >> > >> 于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> 写到: > >> >On 2020-03-25 13:59, Jiaxun Yang wrote: > >> > > >> >[...] > >> > > >> >>>> 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. > >> >> > >> >> Thanks. > >> >> > >> >> So I have to allocate irq_desc here in driver manually? > >> > > >> >No, you are probably better off just dropping this patch, as MIPS > >> >doesn't seem to be ready for a wholesale switch to virtual > >interrupts. > >> > >> It can't work without this patch. > >> > >> Legacy domain require IRQ number within 0-15 > >> however it's already occupied by i8259 or "HTPIC" driver. > > > >what's the problem here ? AFAIK there could be more than one > >legacy domain, at least that's what at least IP22/SNI in MIPS world > >are doing. > > MIPS_IRQ_BASE must be higher than 15, otherwise it will conflict with i8259. I still don't get it. We have following in arch/mips/include/asm/mach-generic/irq.h: #ifndef MIPS_CPU_IRQ_BASE #ifdef CONFIG_I8259 #define MIPS_CPU_IRQ_BASE 16 #else #define MIPS_CPU_IRQ_BASE 0 #endif /* CONFIG_I8259 */ #endif So every legacy platform with i8259 has MIPS_CPU_IRQ_BASE = 16. > However we have only preallocated irq_desc for 0-15. > And legacy domain require irq_desc being preallocated. maybe I'm too fast by judging the irq code, but without CONFIG_SPARSE_IRQ the whole irq_desc is pre-allocated. Thomas.
于 2020年3月25日 GMT+08:00 下午11:46:00, Thomas Bogendoerfer <tsbogend@alpha.franken.de> 写到: >On Wed, Mar 25, 2020 at 11:09:10PM +0800, Jiaxun Yang wrote: >> >> >> 于 2020年3月25日 GMT+08:00 下午11:04:37, Thomas Bogendoerfer ><tsbogend@alpha.franken.de> 写到: >> >On Wed, Mar 25, 2020 at 10:31:21PM +0800, Jiaxun Yang wrote: >> >> >> >> >> >> 于 2020年3月25日 GMT+08:00 下午10:15:16, Marc Zyngier <maz@kernel.org> >写到: >> >> >On 2020-03-25 13:59, Jiaxun Yang wrote: >> >> > >> >> >[...] >> >> > >> >> >>>> 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. >> >> >> >> >> >> Thanks. >> >> >> >> >> >> So I have to allocate irq_desc here in driver manually? >> >> > >> >> >No, you are probably better off just dropping this patch, as MIPS >> >> >doesn't seem to be ready for a wholesale switch to virtual >> >interrupts. >> >> >> >> It can't work without this patch. >> >> >> >> Legacy domain require IRQ number within 0-15 >> >> however it's already occupied by i8259 or "HTPIC" driver. >> > >> >what's the problem here ? AFAIK there could be more than one >> >legacy domain, at least that's what at least IP22/SNI in MIPS world >> >are doing. >> >> MIPS_IRQ_BASE must be higher than 15, otherwise it will conflict with >i8259. > >I still don't get it. > >We have following in arch/mips/include/asm/mach-generic/irq.h: > >#ifndef MIPS_CPU_IRQ_BASE >#ifdef CONFIG_I8259 >#define MIPS_CPU_IRQ_BASE 16 >#else >#define MIPS_CPU_IRQ_BASE 0 >#endif /* CONFIG_I8259 */ >#endif > >So every legacy platform with i8259 has MIPS_CPU_IRQ_BASE = 16. > >> However we have only preallocated irq_desc for 0-15. >> And legacy domain require irq_desc being preallocated. > >maybe I'm too fast by judging the irq code, but without >CONFIG_SPARSE_IRQ >the whole irq_desc is pre-allocated. Sorry. You're right. I found the problem is CONFIG_SPARSE_IRQ is accidentally enabled in my config due to another out-of-tree patch during my initial test and I always consider it as a problem. So we can drop this patch safely for now. But just need to consider how to deal with it when we want to enable SPARSE_IRQ. Thanks. > >Thomas.
On Thu, Mar 26, 2020 at 12:02:28AM +0800, Jiaxun Yang wrote: > >maybe I'm too fast by judging the irq code, but without > >CONFIG_SPARSE_IRQ > >the whole irq_desc is pre-allocated. > > Sorry. You're right. > I found the problem is CONFIG_SPARSE_IRQ is accidentally enabled in my config due to another out-of-tree patch ok, that explains it. > during my initial test and I always consider it as a problem. > > So we can drop this patch safely for now. already dropped in my test branch. If nothing shows up, I'll push it to mips-next. > But just need to consider how to deal with it when we want to enable SPARSE_IRQ. setting NR_IRQS_LEGACY so a sensible value should do the trick then. Thomas.
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); if (!irq_domain)