| Message ID | 20220702190705.5319-1-olek2@wp.pl (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | MIPS: smp-mt: enable all hardware interrupts on second VPE | expand |
Hi Aleksander, Since this is IRQ related: +CC Marc Zyngier On Sat, 2022-07-02 at 21:07 +0200, Aleksander Jan Bajkowski wrote: > This patch is needed to handle interrupts by the second VPE on > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > interrupts are connected to each hardware line. The SoC supports > a total of 160 interrupts. Currently changing smp_affinity to the > second VPE hangs interrupts. > > This problem affects multithreaded SoCs with a custom interrupt > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > on support for Realtek RTL930x chips with 34Kc core and Birger > has added a patch in OpenWRT that also enables all interrupt > lines. So it looks like this patch is useful for more SoCs. > > Tested on lantiq xRX200 and xRX330. > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> Thanks for bringing up this issue. Like you say OpenWrt carries a similar patch, and I also carry a patch on my tree to enable all CPU IRQ lines. Indiscriminately enabling all IRQ lines doesn't sit quite right with me though, since I would expect these to be enabled on-demand. I.e. when a peripheral requests an IRQ, or when an IRQ controller is cascaded into one of the CPU's interrupt lines. If I understand correctly, the IRQ mask/unmask functions in drivers/irqchip/irq-mips-cpu.c should do this. I haven't been able to achieve this (automatic) behaviour until now, so I think I must be doing something wrong when trying to cascade the SoC IRQ driver for the RTL839x/RTL930x chips into both VPEs. It is currently not clear to me how this should be made functional without a patch like this one, so I hope we'll be able to clear that up now. Best, Sander > --- > arch/mips/kernel/smp-mt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/mips/kernel/smp-mt.c b/arch/mips/kernel/smp-mt.c > index 5f04a0141068..f21cd0eb1fa7 100644 > --- a/arch/mips/kernel/smp-mt.c > +++ b/arch/mips/kernel/smp-mt.c > @@ -113,8 +113,7 @@ static void vsmp_init_secondary(void) > STATUSF_IP4 | STATUSF_IP5 | > STATUSF_IP6 | STATUSF_IP7); > else > - change_c0_status(ST0_IM, STATUSF_IP0 | STATUSF_IP1 | > - STATUSF_IP6 | STATUSF_IP7); > + set_c0_status(ST0_IM); > } > > static void vsmp_smp_finish(void)
On Sat, Jul 02, 2022 at 09:07:05PM +0200, Aleksander Jan Bajkowski wrote: > This patch is needed to handle interrupts by the second VPE on > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > interrupts are connected to each hardware line. The SoC supports > a total of 160 interrupts. Currently changing smp_affinity to the > second VPE hangs interrupts. > > This problem affects multithreaded SoCs with a custom interrupt > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > on support for Realtek RTL930x chips with 34Kc core and Birger > has added a patch in OpenWRT that also enables all interrupt > lines. So it looks like this patch is useful for more SoCs. > > Tested on lantiq xRX200 and xRX330. > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> > --- > arch/mips/kernel/smp-mt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/mips/kernel/smp-mt.c b/arch/mips/kernel/smp-mt.c > index 5f04a0141068..f21cd0eb1fa7 100644 > --- a/arch/mips/kernel/smp-mt.c > +++ b/arch/mips/kernel/smp-mt.c > @@ -113,8 +113,7 @@ static void vsmp_init_secondary(void) > STATUSF_IP4 | STATUSF_IP5 | > STATUSF_IP6 | STATUSF_IP7); > else > - change_c0_status(ST0_IM, STATUSF_IP0 | STATUSF_IP1 | > - STATUSF_IP6 | STATUSF_IP7); > + set_c0_status(ST0_IM); > } just blindly enabling all interrupts doesn't sound like a brilliant idea even when if works on some Lantiq platforms (probably because their interrupt controller prevents issuing unwanted interrupts). But not all smp-mt platforms are Lantiq. If some CPU interrupts need to be enabled a clean interrupt controller setup with hierarchy irq domains is IMHO the correct approach, Thomas.
On Sun, 03 Jul 2022 19:15:11 +0100, Sander Vanheule <sander@svanheule.net> wrote: > > Hi Aleksander, > > Since this is IRQ related: +CC Marc Zyngier > > On Sat, 2022-07-02 at 21:07 +0200, Aleksander Jan Bajkowski wrote: > > This patch is needed to handle interrupts by the second VPE on > > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > > interrupts are connected to each hardware line. The SoC supports > > a total of 160 interrupts. Currently changing smp_affinity to the > > second VPE hangs interrupts. > > > > This problem affects multithreaded SoCs with a custom interrupt > > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > > on support for Realtek RTL930x chips with 34Kc core and Birger > > has added a patch in OpenWRT that also enables all interrupt > > lines. So it looks like this patch is useful for more SoCs. > > > > Tested on lantiq xRX200 and xRX330. > > > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> > > Thanks for bringing up this issue. Like you say OpenWrt carries a > similar patch, and I also carry a patch on my tree to enable all CPU > IRQ lines. > > Indiscriminately enabling all IRQ lines doesn't sit quite right with > me though, since I would expect these to be enabled > on-demand. I.e. when a peripheral requests an IRQ, or when an IRQ > controller is cascaded into one of the CPU's interrupt lines. If I > understand correctly, the IRQ mask/unmask functions in > drivers/irqchip/irq-mips-cpu.c should do this. But this is only enabling interrupts at the CPU level, right? And the irqchip is still in control of the masking of the individual interrupts? If both assertions are true, then this patch seems OK. If it just let any interrupt through without any control, then this is wrong. So which one is it? M.
On Wed, Jul 06, 2022 at 08:05:30AM +0100, Marc Zyngier wrote: > On Sun, 03 Jul 2022 19:15:11 +0100, > Sander Vanheule <sander@svanheule.net> wrote: > > > > Hi Aleksander, > > > > Since this is IRQ related: +CC Marc Zyngier > > > > On Sat, 2022-07-02 at 21:07 +0200, Aleksander Jan Bajkowski wrote: > > > This patch is needed to handle interrupts by the second VPE on > > > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > > > interrupts are connected to each hardware line. The SoC supports > > > a total of 160 interrupts. Currently changing smp_affinity to the > > > second VPE hangs interrupts. > > > > > > This problem affects multithreaded SoCs with a custom interrupt > > > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > > > > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > > > on support for Realtek RTL930x chips with 34Kc core and Birger > > > has added a patch in OpenWRT that also enables all interrupt > > > lines. So it looks like this patch is useful for more SoCs. > > > > > > Tested on lantiq xRX200 and xRX330. > > > > > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> > > > > Thanks for bringing up this issue. Like you say OpenWrt carries a > > similar patch, and I also carry a patch on my tree to enable all CPU > > IRQ lines. > > > > Indiscriminately enabling all IRQ lines doesn't sit quite right with > > me though, since I would expect these to be enabled > > on-demand. I.e. when a peripheral requests an IRQ, or when an IRQ > > controller is cascaded into one of the CPU's interrupt lines. If I > > understand correctly, the IRQ mask/unmask functions in > > drivers/irqchip/irq-mips-cpu.c should do this. > > But this is only enabling interrupts at the CPU level, right? And the > irqchip is still in control of the masking of the individual > interrupts? in the Lantiq case yes > If both assertions are true, then this patch seems OK. If it just let > any interrupt through without any control, then this is wrong. > > So which one is it? if there isn't an additional irqchip connected to the cpu interrupt lines, this patch will cause problems. Thomas.
On Wed, 06 Jul 2022 09:19:01 +0100, Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > On Wed, Jul 06, 2022 at 08:05:30AM +0100, Marc Zyngier wrote: > > On Sun, 03 Jul 2022 19:15:11 +0100, > > Sander Vanheule <sander@svanheule.net> wrote: > > > > > > Hi Aleksander, > > > > > > Since this is IRQ related: +CC Marc Zyngier > > > > > > On Sat, 2022-07-02 at 21:07 +0200, Aleksander Jan Bajkowski wrote: > > > > This patch is needed to handle interrupts by the second VPE on > > > > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > > > > interrupts are connected to each hardware line. The SoC supports > > > > a total of 160 interrupts. Currently changing smp_affinity to the > > > > second VPE hangs interrupts. > > > > > > > > This problem affects multithreaded SoCs with a custom interrupt > > > > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > > > > > > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > > > > on support for Realtek RTL930x chips with 34Kc core and Birger > > > > has added a patch in OpenWRT that also enables all interrupt > > > > lines. So it looks like this patch is useful for more SoCs. > > > > > > > > Tested on lantiq xRX200 and xRX330. > > > > > > > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> > > > > > > Thanks for bringing up this issue. Like you say OpenWrt carries a > > > similar patch, and I also carry a patch on my tree to enable all CPU > > > IRQ lines. > > > > > > Indiscriminately enabling all IRQ lines doesn't sit quite right with > > > me though, since I would expect these to be enabled > > > on-demand. I.e. when a peripheral requests an IRQ, or when an IRQ > > > controller is cascaded into one of the CPU's interrupt lines. If I > > > understand correctly, the IRQ mask/unmask functions in > > > drivers/irqchip/irq-mips-cpu.c should do this. > > > > But this is only enabling interrupts at the CPU level, right? And the > > irqchip is still in control of the masking of the individual > > interrupts? > > in the Lantiq case yes > > > If both assertions are true, then this patch seems OK. If it just let > > any interrupt through without any control, then this is wrong. > > > > So which one is it? > > if there isn't an additional irqchip connected to the cpu interrupt lines, > this patch will cause problems. And that's what the irq-mips-cpu driver should solve, right? In this case, what's the problem with adopting this driver for the Lantiq platform (and all other ones using the same CPU)? M.
Hi Thomas, On Wed, Jul 6, 2022 at 11:42 AM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: [...] > > But this is only enabling interrupts at the CPU level, right? And the > > irqchip is still in control of the masking of the individual > > interrupts? > > in the Lantiq case yes > > > If both assertions are true, then this patch seems OK. If it just let > > any interrupt through without any control, then this is wrong. > > > > So which one is it? > > if there isn't an additional irqchip connected to the cpu interrupt lines, > this patch will cause problems. on Lantiq xRX200 SoCs (34Kc with two VPEs) we basically have: cpu_irqc: interrupt-controller { compatible = "mti,cpu-interrupt-controller"; interrupt-controller; #interrupt-cells = <1>; }; &icu { compatible = "lantiq,icu"; interrupt-parent = <&cpu_irqc>; interrupts = <2>, <3>, <4>, <5>, <6>; interrupt-controller; #interrupt-cells = <1>; } meaning: the Lantiq ICU interrupt controller provides 5*32 interrupt lines through 5 MIPS CPU interrupt lines Without this patch all interrupts are fine on VPE 0 and with SMP disabled. The ICU interrupt controller can route interrupts either to VPE 0 or VPE 1. Routing to VPE 1 is the problem: only the upper-most 32 interrupt lines (connected to MIPS CPU interrupt line 6) are working, all other interrupts never arrive on VPE 1. This is because MIPS CPU interrupt line is enabled, even before Aleksander's patch. With Aleksander's patch all 5*32 interrupts (at least all the ones I have tested) can be routed to VPE 1 as well. I understand that this doesn't mean that Aleksander's patch is automatically correct. My two main questions are: - why can MIPS CPU interrupt 6 and 7 be enabled unconditionally while 2-5 cannot be enabled unconditionally? - seeing that there's also a mips_gic_present() check in the opposite case of what Aleksander's patch modifies: does this indicate that unmasking CPU interrupt lines for VPE 1 is not handled by the MIPS CPU interrupt controller driver at all at this point (and if so: do you have any suggestions how to properly fix this)? Best regards, Martin
On Wed, Jul 06, 2022 at 10:53:36AM +0100, Marc Zyngier wrote: > On Wed, 06 Jul 2022 09:19:01 +0100, > Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > > > On Wed, Jul 06, 2022 at 08:05:30AM +0100, Marc Zyngier wrote: > > > On Sun, 03 Jul 2022 19:15:11 +0100, > > > Sander Vanheule <sander@svanheule.net> wrote: > > > > > > > > Hi Aleksander, > > > > > > > > Since this is IRQ related: +CC Marc Zyngier > > > > > > > > On Sat, 2022-07-02 at 21:07 +0200, Aleksander Jan Bajkowski wrote: > > > > > This patch is needed to handle interrupts by the second VPE on > > > > > the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU > > > > > interrupts are connected to each hardware line. The SoC supports > > > > > a total of 160 interrupts. Currently changing smp_affinity to the > > > > > second VPE hangs interrupts. > > > > > > > > > > This problem affects multithreaded SoCs with a custom interrupt > > > > > controller. Chips with 1004Kc core and newer use the MIPS GIC. > > > > > > > > > > Also CC'ed Birger Koblitz and Sander Vanheule. Both are working > > > > > on support for Realtek RTL930x chips with 34Kc core and Birger > > > > > has added a patch in OpenWRT that also enables all interrupt > > > > > lines. So it looks like this patch is useful for more SoCs. > > > > > > > > > > Tested on lantiq xRX200 and xRX330. > > > > > > > > > > Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> > > > > > > > > Thanks for bringing up this issue. Like you say OpenWrt carries a > > > > similar patch, and I also carry a patch on my tree to enable all CPU > > > > IRQ lines. > > > > > > > > Indiscriminately enabling all IRQ lines doesn't sit quite right with > > > > me though, since I would expect these to be enabled > > > > on-demand. I.e. when a peripheral requests an IRQ, or when an IRQ > > > > controller is cascaded into one of the CPU's interrupt lines. If I > > > > understand correctly, the IRQ mask/unmask functions in > > > > drivers/irqchip/irq-mips-cpu.c should do this. > > > > > > But this is only enabling interrupts at the CPU level, right? And the > > > irqchip is still in control of the masking of the individual > > > interrupts? > > > > in the Lantiq case yes > > > > > If both assertions are true, then this patch seems OK. If it just let > > > any interrupt through without any control, then this is wrong. > > > > > > So which one is it? > > > > if there isn't an additional irqchip connected to the cpu interrupt lines, > > this patch will cause problems. > > And that's what the irq-mips-cpu driver should solve, right? In this yes > case, what's the problem with adopting this driver for the Lantiq > platform (and all other ones using the same CPU)? I guess vendor code supplied more or less the current code base and nobody dared to change it. Thomas.
On Wed, Jul 06, 2022 at 11:56:47AM +0200, Martin Blumenstingl wrote: > Without this patch all interrupts are fine on VPE 0 and with SMP disabled. I fully understand the problem. But not everybody uses this interrupt setup, so changing generic code will have effects there too. > - why can MIPS CPU interrupt 6 and 7 be enabled unconditionally while > 2-5 cannot be enabled unconditionally? 7 is timer interrupt and is usually wired for 34K cpus and 6 is performance counter hopefully handled as well. And I agree that this still isn't the best approach here > - seeing that there's also a mips_gic_present() check in the opposite > case of what Aleksander's patch modifies: does this indicate that > unmasking CPU interrupt lines for VPE 1 is not handled by the MIPS CPU > interrupt controller driver at all at this point (and if so: do you > have any suggestions how to properly fix this)? I haven't checked how GIC is integrated. Iirc it does something similair to Lantiq's irq controller and hides all CPU internal interrupts behind it. So I see two solutions for your problem. 1. Add "mti,cpu-interrupt-controller" to the DT and wire it up 2. Create your own struct plat_smp_ops using vsmp_smp_ops as a template and overload .boot_secondary Thomas.
On Thu, Jul 7, 2022 at 12:11 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: [...] > > - why can MIPS CPU interrupt 6 and 7 be enabled unconditionally while > > 2-5 cannot be enabled unconditionally? > > 7 is timer interrupt and is usually wired for 34K cpus and 6 is > performance counter hopefully handled as well. And I agree that > this still isn't the best approach here Thanks for this explanation! > > - seeing that there's also a mips_gic_present() check in the opposite > > case of what Aleksander's patch modifies: does this indicate that > > unmasking CPU interrupt lines for VPE 1 is not handled by the MIPS CPU > > interrupt controller driver at all at this point (and if so: do you > > have any suggestions how to properly fix this)? > > I haven't checked how GIC is integrated. Iirc it does something similair > to Lantiq's irq controller and hides all CPU internal interrupts behind > it. > > So I see two solutions for your problem. > > 1. Add "mti,cpu-interrupt-controller" to the DT and wire it up I think this is the preferred way. I tried this before (if you are curious, see [0] and [1]) and it didn't work. Are you aware of any MIPS SoC with upstream drivers which do have working IRQs on VPE 1? Or can you point me to the code in drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the interrupts on VPE 1 (is it simply unmask_mips_irq)? > 2. Create your own struct plat_smp_ops using vsmp_smp_ops as > a template and overload .boot_secondary This would work, but: personally I would like to remove as much Lantiq platform specific code as possible so it's easier to maintain. Best regards, Martin [0] https://github.com/xdarklight/linux/commit/0e5a5dda0e999a3a2e5a81324fef15405d8c6b4a [1] https://github.com/xdarklight/linux/commit/97ec4689d2016606442577988d28fef6728c3bbf
On Thu, Jul 07, 2022 at 02:57:15PM +0200, Martin Blumenstingl wrote: > On Thu, Jul 7, 2022 at 12:11 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > [...] > > > - why can MIPS CPU interrupt 6 and 7 be enabled unconditionally while > > > 2-5 cannot be enabled unconditionally? > > > > 7 is timer interrupt and is usually wired for 34K cpus and 6 is > > performance counter hopefully handled as well. And I agree that > > this still isn't the best approach here > Thanks for this explanation! > > > > - seeing that there's also a mips_gic_present() check in the opposite > > > case of what Aleksander's patch modifies: does this indicate that > > > unmasking CPU interrupt lines for VPE 1 is not handled by the MIPS CPU > > > interrupt controller driver at all at this point (and if so: do you > > > have any suggestions how to properly fix this)? > > > > I haven't checked how GIC is integrated. Iirc it does something similair > > to Lantiq's irq controller and hides all CPU internal interrupts behind > > it. > > > > So I see two solutions for your problem. > > > > 1. Add "mti,cpu-interrupt-controller" to the DT and wire it up > I think this is the preferred way. I tried this before (if you are > curious, see [0] and [1]) and it didn't work. > Are you aware of any MIPS SoC with upstream drivers which do have > working IRQs on VPE 1? I don't know of such SoC. Looking at the comment in vsmp_init_secondary() /* This is Malta specific: IPI,performance and timer interrupts */ there is probably some Malta board using it. > Or can you point me to the code in > drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the > interrupts on VPE 1 (is it simply unmask_mips_irq)? IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations on the same CPU. I've checked MIPS MT specs and it's possible do modify CP0 registers between VPEs. Using that needs changes in irq-mips-cpu.c. But mabye that's not woth the effort as probably all SMP cabable platforms have some multi processort capable interrupt controller implemented. I thought about another way solve the issue. By introducing a new function in smp-mt.c which sets the value of the interrupt mask for the secondary CPU, which is then used in vsmp_init_secondary(). Not sure if this is worth the effort compared to a .boot_secondary override. Thomas.
On Thu, 2022-07-07 at 16:39 +0200, Thomas Bogendoerfer wrote: > On Thu, Jul 07, 2022 at 02:57:15PM +0200, Martin Blumenstingl wrote: > > On Thu, Jul 7, 2022 at 12:11 PM Thomas Bogendoerfer > > <tsbogend@alpha.franken.de> wrote: > > [...] > > > > - why can MIPS CPU interrupt 6 and 7 be enabled unconditionally while > > > > 2-5 cannot be enabled unconditionally? > > > > > > 7 is timer interrupt and is usually wired for 34K cpus and 6 is > > > performance counter hopefully handled as well. And I agree that > > > this still isn't the best approach here > > Thanks for this explanation! > > > > > > - seeing that there's also a mips_gic_present() check in the opposite > > > > case of what Aleksander's patch modifies: does this indicate that > > > > unmasking CPU interrupt lines for VPE 1 is not handled by the MIPS CPU > > > > interrupt controller driver at all at this point (and if so: do you > > > > have any suggestions how to properly fix this)? > > > > > > I haven't checked how GIC is integrated. Iirc it does something similair > > > to Lantiq's irq controller and hides all CPU internal interrupts behind > > > it. > > > > > > So I see two solutions for your problem. > > > > > > 1. Add "mti,cpu-interrupt-controller" to the DT and wire it up > > I think this is the preferred way. I tried this before (if you are > > curious, see [0] and [1]) and it didn't work. > > Are you aware of any MIPS SoC with upstream drivers which do have > > working IRQs on VPE 1? > > I don't know of such SoC. Looking at the comment in vsmp_init_secondary() > > /* This is Malta specific: IPI,performance and timer interrupts */ > > there is probably some Malta board using it. > > > Or can you point me to the code in > > drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the > > interrupts on VPE 1 (is it simply unmask_mips_irq)? > > IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations > on the same CPU. I've checked MIPS MT specs and it's possible do > modify CP0 registers between VPEs. Using that needs changes in > irq-mips-cpu.c. But mabye that's not woth the effort as probably > all SMP cabable platforms have some multi processort capable > interrupt controller implemented. Maybe I'm not getting this right, but as I understand vsmp_init_secondary() is run on VPE1, changing the local c0_status on that VPE. When unmask_mips_irq() is called from code running on VPE1, I would expect it does the same thing: enable the requested irq on VPE1 by modifying the local c0_status register. Since these IRQs can be configured VPE, aren't these then also per-cpu interrupts? I have been wondering if a cascaded IRQ controller needs to take special care with such per-cpu interrupts, but haven't been able to figure that out until now. Sorry if the above doesn't make much sense, but like Aleksander I've been struggling with this and would like to understand what the proper solution is. Best, Sander > I thought about another way solve the issue. By introducing a > new function in smp-mt.c which sets the value of the interrupt > mask for the secondary CPU, which is then used in vsmp_init_secondary(). > Not sure if this is worth the effort compared to a .boot_secondary > override. > > Thomas. >
Hi, On 7/7/22 17:12, Sander Vanheule wrote: > On Thu, 2022-07-07 at 16:39 +0200, Thomas Bogendoerfer wrote: >> On Thu, Jul 07, 2022 at 02:57:15PM +0200, Martin Blumenstingl wrote: >> IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations >> on the same CPU. I've checked MIPS MT specs and it's possible do >> modify CP0 registers between VPEs. Using that needs changes in >> irq-mips-cpu.c. But mabye that's not woth the effort as probably >> all SMP cabable platforms have some multi processort capable >> interrupt controller implemented. Not sure I can be of much help. That the patch works on the RTL SoCs is mostly empirical and was found in the vendor code. My understanding from the MIPS documentation is that it is not specified what happens when a multi VPE capable IRQ controller triggers CPU interrupts: if multiple VPEs are possible targets, then it is not defined whether one of them gets them (and which one), multiple, or all. So trying to control what happens between VPEs is probably SoC-dependent functionality. Cheers, Birger
Hi Thomas, On Thu, Jul 7, 2022 at 4:40 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: [...] > > Or can you point me to the code in > > drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the > > interrupts on VPE 1 (is it simply unmask_mips_irq)? > > IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations > on the same CPU. I've checked MIPS MT specs and it's possible do > modify CP0 registers between VPEs. Using that needs changes in > irq-mips-cpu.c. But mabye that's not woth the effort as probably > all SMP cabable platforms have some multi processort capable > interrupt controller implemented. On Lantiq the "multi processor capable interrupt controller" solution seems not very sophisticated: there's simply two identical copies of the IRQ controller IP, one connected to CPU0 and the other to CPU1. > I thought about another way solve the issue. By introducing a > new function in smp-mt.c which sets the value of the interrupt > mask for the secondary CPU, which is then used in vsmp_init_secondary(). > Not sure if this is worth the effort compared to a .boot_secondary > override. I think for the Realtek SoC's this would be problematic because it's using MIPS_GENERIC. My understanding is that in an ideal world all platforms would switch to MIPS_GENERIC. As an alternative to making irq-mips-cpu capable of changing another CPU's registers: would you also be happy with a change that implements the following idea (pseudocode) in vsmp_init_secondary(): struct device_node *root_node = of_find_node_by_path("/"); if (mips_gic_present() || of_device_is_compatible(root_node, "lantiq,xrx200") || of_device_is_compatible(root_node, "realtek,some-relevant-soc")) change_c0_status(ST0_IM, STATUSF_IP2 | STATUSF_IP3 | STATUSF_IP4 | STATUSF_IP5 | STATUSF_IP6 | STATUSF_IP7); else ... of_node_put(root_node); That way we don't risk enabling interrupt lines which shouldn't be enabled (on SoCs which we don't know). And also it would not cause any issues with MIPS_GENERIC support. Best regards, Martin
On Thu, Jul 28, 2022 at 05:50:10PM +0200, Martin Blumenstingl wrote: > I think for the Realtek SoC's this would be problematic because it's > using MIPS_GENERIC. My understanding is that in an ideal world all which SOC are these ? > platforms would switch to MIPS_GENERIC. > As an alternative to making irq-mips-cpu capable of changing another > CPU's registers: would you also be happy with a change that implements > the following idea (pseudocode) in vsmp_init_secondary(): > struct device_node *root_node = of_find_node_by_path("/"); > > if (mips_gic_present() || > of_device_is_compatible(root_node, "lantiq,xrx200") || > of_device_is_compatible(root_node, "realtek,some-relevant-soc")) > change_c0_status(ST0_IM, STATUSF_IP2 | STATUSF_IP3 | > STATUSF_IP4 | STATUSF_IP5 | > STATUSF_IP6 | STATUSF_IP7); > else > ... > > of_node_put(root_node); > > That way we don't risk enabling interrupt lines which shouldn't be > enabled (on SoCs which we don't know). > And also it would not cause any issues with MIPS_GENERIC support. well it's not exactly the abstraction I'm looking for, but it's ok for me as a short term way to move forward. Thomas.
Hi Thomas, On Mon, 2022-08-01 at 17:25 +0200, Thomas Bogendoerfer wrote: > On Thu, Jul 28, 2022 at 05:50:10PM +0200, Martin Blumenstingl wrote: > > I think for the Realtek SoC's this would be problematic because it's > > using MIPS_GENERIC. My understanding is that in an ideal world all > > which SOC are these ? That would be the SoCs supported by MACH_REALTEK_RTL. More specifically, the ones affected by this issue are the RTL8391M, RTL8392M, RTL8393M, and RTL8396M which have two VPEs. The SoC interrupt controller on these chips can route interrupts to all CPU HW interrupts. If only IP6 and IP7 are enabled on the second VPE, anything routed there to IP2-IP5 ends up in a black hole. Best, Sander > > > platforms would switch to MIPS_GENERIC. > > As an alternative to making irq-mips-cpu capable of changing another > > CPU's registers: would you also be happy with a change that implements > > the following idea (pseudocode) in vsmp_init_secondary(): > > struct device_node *root_node = of_find_node_by_path("/"); > > > > if (mips_gic_present() || > > of_device_is_compatible(root_node, "lantiq,xrx200") || > > of_device_is_compatible(root_node, "realtek,some-relevant-soc")) > > change_c0_status(ST0_IM, STATUSF_IP2 | STATUSF_IP3 | > > STATUSF_IP4 | STATUSF_IP5 | > > STATUSF_IP6 | STATUSF_IP7); > > else > > ... > > > > of_node_put(root_node); > > > > That way we don't risk enabling interrupt lines which shouldn't be > > enabled (on SoCs which we don't know). > > And also it would not cause any issues with MIPS_GENERIC support. > > well it's not exactly the abstraction I'm looking for, but it's ok for me > as a short term way to move forward. > > Thomas. >
Hi, On 8/1/22 18:02, Sander Vanheule wrote: > Hi Thomas, > > On Mon, 2022-08-01 at 17:25 +0200, Thomas Bogendoerfer wrote: >> On Thu, Jul 28, 2022 at 05:50:10PM +0200, Martin Blumenstingl wrote: >>> I think for the Realtek SoC's this would be problematic because it's >>> using MIPS_GENERIC. My understanding is that in an ideal world all >> >> which SOC are these ? > > That would be the SoCs supported by MACH_REALTEK_RTL. More specifically, the > ones affected by this issue are the RTL8391M, RTL8392M, RTL8393M, and RTL8396M > which have two VPEs. There is also a multitude of RTL930x SoCs, which have the same setup as the RTL839x SoCs regarding the VPEs and the same IRQ controller. But they also have a broken MIPS Timer IRQ and we are not able at the moment to get the second VPE up without running into immediate trouble. For the moment we only need to take care of the RTL839x, though. Cheers, Birger
Hi THomas, On 7/7/22 16:39, Thomas Bogendoerfer wrote: [...] >> Or can you point me to the code in >> drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the >> interrupts on VPE 1 (is it simply unmask_mips_irq)? > > IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations > on the same CPU. I've checked MIPS MT specs and it's possible do > modify CP0 registers between VPEs. Using that needs changes in > irq-mips-cpu.c. But mabye that's not woth the effort as probably > all SMP cabable platforms have some multi processort capable > interrupt controller implemented. > > I thought about another way solve the issue. By introducing a > new function in smp-mt.c which sets the value of the interrupt > mask for the secondary CPU, which is then used in vsmp_init_secondary(). > Not sure if this is worth the effort compared to a .boot_secondary > override. Enabling interrupts on the second VPE using hotplug will be accepted upstream? Below is a sample patch. Unfortunately, this is not a generic solution. If in the future there are more platforms that require a similar patch, this can be converted into some generic solution. --- a/arch/mips/lantiq/irq.c +++ b/arch/mips/lantiq/irq.c @@ -335,6 +336,18 @@ static const struct irq_domain_ops irq_domain_ops = { .map = icu_map, }; +static int lantiq_cpu_starting(unsigned int cpu) +{ + /* + * MIPS CPU startup function vsmp_init_secondary() will only enable some of + * the interrupts for the second CPU/VPE. Fix this during hotplug. + */ + if (cpu > 0) + set_c0_status(ST0_IM); + + return 0; +} + int __init icu_of_init(struct device_node *node, struct device_node *parent) { struct device_node *eiu_node; @@ -410,6 +423,10 @@ int __init icu_of_init(struct device_node *node, struct device_node *parent) } of_node_put(eiu_node); + cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_LANTIQ_STARTING, + "arch/mips/lantiq:starting", + lantiq_cpu_starting, NULL); + return 0; } --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -152,6 +152,7 @@ enum cpuhp_state { CPUHP_AP_IRQ_RISCV_STARTING, CPUHP_AP_IRQ_LOONGARCH_STARTING, CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING, + CPUHP_AP_IRQ_LANTIQ_STARTING, CPUHP_AP_ARM_MVEBU_COHERENCY, CPUHP_AP_MICROCODE_LOADER, CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING, Best regards, Aleksander
On Sat, Sep 10, 2022 at 12:53:40PM +0200, Aleksander Bajkowski wrote: > Hi THomas, > > On 7/7/22 16:39, Thomas Bogendoerfer wrote: > [...] > >> Or can you point me to the code in > >> drivers/irqchip/irq-mips-cpu.c that's responsible for enabling the > >> interrupts on VPE 1 (is it simply unmask_mips_irq)? > > > > IMHO there is the problem, irq-mips-cpu.c can only do CPU irq operations > > on the same CPU. I've checked MIPS MT specs and it's possible do > > modify CP0 registers between VPEs. Using that needs changes in > > irq-mips-cpu.c. But mabye that's not woth the effort as probably > > all SMP cabable platforms have some multi processort capable > > interrupt controller implemented. > > > > I thought about another way solve the issue. By introducing a > > new function in smp-mt.c which sets the value of the interrupt > > mask for the secondary CPU, which is then used in vsmp_init_secondary(). > > Not sure if this is worth the effort compared to a .boot_secondary > > override. > > > Enabling interrupts on the second VPE using hotplug will be accepted > upstream? Below is a sample patch. as this is just another hack, below is what I prefer. Thomas. commit 15853dc9e6d213558acbf961f98e9f77b4b61db2 Author: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Date: Mon Sep 12 15:59:44 2022 +0200 my lantiq approach diff --git a/arch/mips/lantiq/prom.c b/arch/mips/lantiq/prom.c index c731082a0c42..1cc4f56b57f6 100644 --- a/arch/mips/lantiq/prom.c +++ b/arch/mips/lantiq/prom.c @@ -84,6 +84,16 @@ void __init plat_mem_setup(void) __dt_setup_arch(dtb); } +#if defined(CONFIG_MIPS_MT_SMP) +extern const struct plat_smp_ops vsmp_smp_ops; +static struct plat_smp_ops lantiq_smp_ops; + +static void lantiq_init_secondary(void) +{ + set_c0_status(ST0_IM); +} +#endif + void __init prom_init(void) { /* call the soc specific detetcion code and get it to fill soc_info */ @@ -95,7 +105,13 @@ void __init prom_init(void) prom_init_cmdline(); #if defined(CONFIG_MIPS_MT_SMP) - if (register_vsmp_smp_ops()) + + if (cpu_has_mipsmt) { + lantiq_smp_ops = vsmp_smp_ops; + lantiq_smp_ops.init_secondary = lantiq_init_secondary; + register_smp_ops(&lantiq_smp_ops); + } else { panic("failed to register_vsmp_smp_ops()"); + } #endif }
diff --git a/arch/mips/kernel/smp-mt.c b/arch/mips/kernel/smp-mt.c index 5f04a0141068..f21cd0eb1fa7 100644 --- a/arch/mips/kernel/smp-mt.c +++ b/arch/mips/kernel/smp-mt.c @@ -113,8 +113,7 @@ static void vsmp_init_secondary(void) STATUSF_IP4 | STATUSF_IP5 | STATUSF_IP6 | STATUSF_IP7); else - change_c0_status(ST0_IM, STATUSF_IP0 | STATUSF_IP1 | - STATUSF_IP6 | STATUSF_IP7); + set_c0_status(ST0_IM); } static void vsmp_smp_finish(void)
This patch is needed to handle interrupts by the second VPE on the Lantiq xRX200, xRX300 and xRX330 SoCs. In these chips, 32 ICU interrupts are connected to each hardware line. The SoC supports a total of 160 interrupts. Currently changing smp_affinity to the second VPE hangs interrupts. This problem affects multithreaded SoCs with a custom interrupt controller. Chips with 1004Kc core and newer use the MIPS GIC. Also CC'ed Birger Koblitz and Sander Vanheule. Both are working on support for Realtek RTL930x chips with 34Kc core and Birger has added a patch in OpenWRT that also enables all interrupt lines. So it looks like this patch is useful for more SoCs. Tested on lantiq xRX200 and xRX330. Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl> --- arch/mips/kernel/smp-mt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)