| Message ID | 20240630153652.318882-10-wahrenst@gmx.net |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand |
On 6/30/24 19:36, Stefan Wahren wrote: > On resume of the Raspberry Pi the dwc2 driver fails to enable > HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. > This causes a situation where both handler ignore a incoming port > interrupt and force the upper layers to disable the dwc2 interrupt line. > This leaves the USB interface in a unusable state: > > irq 66: nobody cared (try booting with the "irqpoll" option) > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 > Hardware name: BCM2835 > Call trace: > unwind_backtrace from show_stack+0x10/0x14 > show_stack from dump_stack_lvl+0x50/0x64 > dump_stack_lvl from __report_bad_irq+0x38/0xc0 > __report_bad_irq from note_interrupt+0x2ac/0x2f4 > note_interrupt from handle_irq_event+0x88/0x8c > handle_irq_event from handle_level_irq+0xb4/0x1ac > handle_level_irq from generic_handle_domain_irq+0x24/0x34 > generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 > bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 > generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 > generic_handle_arch_irq from __irq_svc+0x88/0xb0 > Exception stack(0xc1b01f20 to 0xc1b01f68) > 1f20: 0005c0d4 00000001 00000000 00000000 c1b09780 c1d6b32c c1b04e54 c1a5eae8 > 1f40: c1b04e90 00000000 00000000 00000000 c1d6a8a0 c1b01f70 c11d2da8 c11d4160 > 1f60: 60000013 ffffffff > __irq_svc from default_idle_call+0x1c/0xb0 > default_idle_call from do_idle+0x21c/0x284 > do_idle from cpu_startup_entry+0x28/0x2c > cpu_startup_entry from kernel_init+0x0/0x12c > handlers: > [<f539e0f4>] dwc2_handle_common_intr > [<75cd278b>] usb_hcd_irq > Disabling IRQ #66 > > Disabling clock gatling workaround this issue. > > Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") > Link: https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/__;!!A4F2R9G_pg!ct8iWVOAvVd4m_4YnYx7c3W3MN-1-zNmESEntpanapAXTL3FHFP3YXzzyBZCEdOsDLfQh-a_d-mJT5A$ > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> Acked-by: Minas Harutyunyan <hminas@synopsys.com> > --- > drivers/usb/dwc2/params.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > index 5a1500d0bdd9..66580de52882 100644 > --- a/drivers/usb/dwc2/params.c > +++ b/drivers/usb/dwc2/params.c > @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) > p->max_transfer_size = 65535; > p->max_packet_count = 511; > p->ahbcfg = 0x10; > + p->no_clock_gating = true; > } > > static void dwc2_set_his_params(struct dwc2_hsotg *hsotg) > -- > 2.34.1 >
On 6/30/2024 4:36 PM, Stefan Wahren wrote: > On resume of the Raspberry Pi the dwc2 driver fails to enable > HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. > This causes a situation where both handler ignore a incoming port > interrupt and force the upper layers to disable the dwc2 interrupt line. > This leaves the USB interface in a unusable state: > > irq 66: nobody cared (try booting with the "irqpoll" option) > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 > Hardware name: BCM2835 > Call trace: > unwind_backtrace from show_stack+0x10/0x14 > show_stack from dump_stack_lvl+0x50/0x64 > dump_stack_lvl from __report_bad_irq+0x38/0xc0 > __report_bad_irq from note_interrupt+0x2ac/0x2f4 > note_interrupt from handle_irq_event+0x88/0x8c > handle_irq_event from handle_level_irq+0xb4/0x1ac > handle_level_irq from generic_handle_domain_irq+0x24/0x34 > generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 > bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 > generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 > generic_handle_arch_irq from __irq_svc+0x88/0xb0 > Exception stack(0xc1b01f20 to 0xc1b01f68) > 1f20: 0005c0d4 00000001 00000000 00000000 c1b09780 c1d6b32c c1b04e54 c1a5eae8 > 1f40: c1b04e90 00000000 00000000 00000000 c1d6a8a0 c1b01f70 c11d2da8 c11d4160 > 1f60: 60000013 ffffffff > __irq_svc from default_idle_call+0x1c/0xb0 > default_idle_call from do_idle+0x21c/0x284 > do_idle from cpu_startup_entry+0x28/0x2c > cpu_startup_entry from kernel_init+0x0/0x12c > handlers: > [<f539e0f4>] dwc2_handle_common_intr > [<75cd278b>] usb_hcd_irq > Disabling IRQ #66 > > Disabling clock gatling workaround this issue. Typo: gatling/gating. > > Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") > Link: https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/ > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/usb/dwc2/params.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > index 5a1500d0bdd9..66580de52882 100644 > --- a/drivers/usb/dwc2/params.c > +++ b/drivers/usb/dwc2/params.c > @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) > p->max_transfer_size = 65535; > p->max_packet_count = 511; > p->ahbcfg = 0x10; > + p->no_clock_gating = true; Could we set this depending upon whether the dwc2 host controller is a wake-up source for the system or not?
Hi Florian, Am 04.07.24 um 16:14 schrieb Florian Fainelli: > > > On 6/30/2024 4:36 PM, Stefan Wahren wrote: >> On resume of the Raspberry Pi the dwc2 driver fails to enable >> HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. >> This causes a situation where both handler ignore a incoming port >> interrupt and force the upper layers to disable the dwc2 interrupt line. >> This leaves the USB interface in a unusable state: >> >> irq 66: nobody cared (try booting with the "irqpoll" option) >> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 >> Hardware name: BCM2835 >> Call trace: >> unwind_backtrace from show_stack+0x10/0x14 >> show_stack from dump_stack_lvl+0x50/0x64 >> dump_stack_lvl from __report_bad_irq+0x38/0xc0 >> __report_bad_irq from note_interrupt+0x2ac/0x2f4 >> note_interrupt from handle_irq_event+0x88/0x8c >> handle_irq_event from handle_level_irq+0xb4/0x1ac >> handle_level_irq from generic_handle_domain_irq+0x24/0x34 >> generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 >> bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 >> generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 >> generic_handle_arch_irq from __irq_svc+0x88/0xb0 >> Exception stack(0xc1b01f20 to 0xc1b01f68) >> 1f20: 0005c0d4 00000001 00000000 00000000 c1b09780 c1d6b32c c1b04e54 >> c1a5eae8 >> 1f40: c1b04e90 00000000 00000000 00000000 c1d6a8a0 c1b01f70 c11d2da8 >> c11d4160 >> 1f60: 60000013 ffffffff >> __irq_svc from default_idle_call+0x1c/0xb0 >> default_idle_call from do_idle+0x21c/0x284 >> do_idle from cpu_startup_entry+0x28/0x2c >> cpu_startup_entry from kernel_init+0x0/0x12c >> handlers: >> [<f539e0f4>] dwc2_handle_common_intr >> [<75cd278b>] usb_hcd_irq >> Disabling IRQ #66 >> >> Disabling clock gatling workaround this issue. > > Typo: gatling/gating. > >> >> Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr >> function.") >> Link: >> https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/ >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/usb/dwc2/params.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c >> index 5a1500d0bdd9..66580de52882 100644 >> --- a/drivers/usb/dwc2/params.c >> +++ b/drivers/usb/dwc2/params.c >> @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg >> *hsotg) >> p->max_transfer_size = 65535; >> p->max_packet_count = 511; >> p->ahbcfg = 0x10; >> + p->no_clock_gating = true; > > Could we set this depending upon whether the dwc2 host controller is a > wake-up source for the system or not? I would prefer to fix the suspend/resume behavior reported here [1] instead of making tricky workarounds. But i don't have an idea how to achieve this. [1] - https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/
On Thu, Jul 04, 2024 at 03:14:50PM +0100, Florian Fainelli wrote: > On 6/30/2024 4:36 PM, Stefan Wahren wrote: > > On resume of the Raspberry Pi the dwc2 driver fails to enable > > HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. > > This causes a situation where both handler ignore a incoming port > > interrupt and force the upper layers to disable the dwc2 interrupt line. > > This leaves the USB interface in a unusable state: > > > > irq 66: nobody cared (try booting with the "irqpoll" option) > > CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 > > Hardware name: BCM2835 > > Call trace: > > unwind_backtrace from show_stack+0x10/0x14 > > show_stack from dump_stack_lvl+0x50/0x64 > > dump_stack_lvl from __report_bad_irq+0x38/0xc0 > > __report_bad_irq from note_interrupt+0x2ac/0x2f4 > > note_interrupt from handle_irq_event+0x88/0x8c > > handle_irq_event from handle_level_irq+0xb4/0x1ac > > handle_level_irq from generic_handle_domain_irq+0x24/0x34 > > generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 > > bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 > > generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 > > generic_handle_arch_irq from __irq_svc+0x88/0xb0 A similar issue was reported for Agilex platforms back in 2021: https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/ It was fixed by commit 3d8d3504d233 ("usb: dwc2: Add platform specific data for Intel's Agilex"), which sets the no_clock_gating flag on that platform. Looking at drivers/usb/dwc2/params.c, numerous other platforms need the same flag. Please amend the commit message to mention the Agilex issue and resulting commit. > > --- a/drivers/usb/dwc2/params.c > > +++ b/drivers/usb/dwc2/params.c > > @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) > > p->max_transfer_size = 65535; > > p->max_packet_count = 511; > > p->ahbcfg = 0x10; > > + p->no_clock_gating = true; > > Could we set this depending upon whether the dwc2 host controller is a > wake-up source for the system or not? The flag seems to mean whether the platform is actually capable of disabling the clock of the USB controller. BCM2835 seems to be incapable and as a result, even though dwc2_host_enter_clock_gating() is called, the chip signals interrupts. There doesn't seem to be a relation to using the controller as a wakeup source, so your comment doesn't seem to make sense. If the clock can't be gated, the chip can always serve as a wakeup source. The real question is whether BCM2848 platforms likewise cannot disable the clock of the dwc2 controller or whether this is specific to the BCM2835. Right now dwc2_set_bcm_params() is applied to both the BCM2848 and BCM2835. If the BCM2848 behaves differently in this regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835. Thanks, Lukas
Am 05.07.24 um 10:48 schrieb Lukas Wunner: > On Thu, Jul 04, 2024 at 03:14:50PM +0100, Florian Fainelli wrote: >> On 6/30/2024 4:36 PM, Stefan Wahren wrote: >>> On resume of the Raspberry Pi the dwc2 driver fails to enable >>> HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. >>> This causes a situation where both handler ignore a incoming port >>> interrupt and force the upper layers to disable the dwc2 interrupt line. >>> This leaves the USB interface in a unusable state: >>> >>> irq 66: nobody cared (try booting with the "irqpoll" option) >>> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 >>> Hardware name: BCM2835 >>> Call trace: >>> unwind_backtrace from show_stack+0x10/0x14 >>> show_stack from dump_stack_lvl+0x50/0x64 >>> dump_stack_lvl from __report_bad_irq+0x38/0xc0 >>> __report_bad_irq from note_interrupt+0x2ac/0x2f4 >>> note_interrupt from handle_irq_event+0x88/0x8c >>> handle_irq_event from handle_level_irq+0xb4/0x1ac >>> handle_level_irq from generic_handle_domain_irq+0x24/0x34 >>> generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 >>> bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 >>> generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 >>> generic_handle_arch_irq from __irq_svc+0x88/0xb0 > A similar issue was reported for Agilex platforms back in 2021: > > https://lore.kernel.org/all/5e8cbce0-3260-2971-484f-fc73a3b2bd28@synopsys.com/ > > It was fixed by commit 3d8d3504d233 ("usb: dwc2: Add platform specific > data for Intel's Agilex"), which sets the no_clock_gating flag on that > platform. > > Looking at drivers/usb/dwc2/params.c, numerous other platforms need > the same flag. > > Please amend the commit message to mention the Agilex issue and > resulting commit. From my understanding Samsung noticed this issue at first and introduced the no_clock_gating flag [1] and they referenced 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") as I did in this patch. Later some platforms like Rockchip and Agilex followed. Should i better refer to the Samsung bugfix instead of the Agilex bugfix? > > >>> --- a/drivers/usb/dwc2/params.c >>> +++ b/drivers/usb/dwc2/params.c >>> @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) >>> p->max_transfer_size = 65535; >>> p->max_packet_count = 511; >>> p->ahbcfg = 0x10; >>> + p->no_clock_gating = true; >> Could we set this depending upon whether the dwc2 host controller is a >> wake-up source for the system or not? > The flag seems to mean whether the platform is actually capable of > disabling the clock of the USB controller. BCM2835 seems to be > incapable and as a result, even though dwc2_host_enter_clock_gating() > is called, the chip signals interrupts. That's why I was asking about this in the initial bug report. Since I didn't get a reply, I submitted this workaround. > There doesn't seem to be a relation to using the controller as a > wakeup source, so your comment doesn't seem to make sense. > If the clock can't be gated, the chip can always serve as a > wakeup source. I wouldn't conclude that the chip can always serve as a wakeup source (e.g. power down the USB domain would also prevent this), but i agree creating a relation between wakeup source and clock gating is a bad idea. > The real question is whether BCM2848 platforms likewise cannot disable > the clock of the dwc2 controller or whether this is specific to the > BCM2835. Right now dwc2_set_bcm_params() is applied to both the > BCM2848 and BCM2835. If the BCM2848 behaves differently in this > regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835. From my understand BCM2848 refers to the same SoC, but the ACPI implementation uses a different ID [2]. So I think this is safe. > > Thanks, > > Lukas [1] - https://lore.kernel.org/linux-usb/20210716050127.4406-1-m.szyprowski@samsung.com/ [2] - https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/
Hi Jeremy, Am 05.07.24 um 17:03 schrieb Lukas Wunner: > On Fri, Jul 05, 2024 at 12:22:33PM +0200, Stefan Wahren wrote: >> Am 05.07.24 um 10:48 schrieb Lukas Wunner: >>> The real question is whether BCM2848 platforms likewise cannot disable >>> the clock of the dwc2 controller or whether this is specific to the >>> BCM2835. Right now dwc2_set_bcm_params() is applied to both the >>> BCM2848 and BCM2835. If the BCM2848 behaves differently in this >>> regard, we'd have to duplicate dwc2_set_bcm_params() for the BCM2835. >> From my understand BCM2848 refers to the same SoC, but the ACPI >> implementation uses a different ID [2]. So I think this is safe. >> [2] - >> https://patches.linaro.org/project/linux-usb/patch/20210413215834.3126447-2-jeremy.linton@arm.com/ > Careful there, the patch vaguely says... > > With that added and identified as "BCM2848", > an id in use by other OSs for this device, the dw2 > controller on the BCM2711 will work. > > ...which sounds like they copy-pasted the BCM2848 id from somewhere else. > I would assume that BCM2848 is really a different SoC and not just > a different name for the BCM2835, but hopefully BroadCom folks will > be able to confirm or deny this (and thus the necessity of the quirk > on BCM2848 and not just on BCM2835). could you please clarify this situation? Thanks > > Thanks, > > Lukas
On Fri, Jul 05, 2024 at 12:16:14PM -0500, Jeremy Linton wrote: > > Am 05.07.24 um 17:03 schrieb Lukas Wunner: > > > Careful there, the patch vaguely says... > > > > > > With that added and identified as "BCM2848", > > > an id in use by other OSs for this device, the dw2 > > > controller on the BCM2711 will work. > > > > > > ...which sounds like they copy-pasted the BCM2848 id from somewhere else. > > > I would assume that BCM2848 is really a different SoC and not just > > > a different name for the BCM2835, but hopefully BroadCom folks will > > > be able to confirm or deny this (and thus the necessity of the quirk > > > on BCM2848 and not just on BCM2835). > > This id comes from the edk2-platforms ACPI tables and is currently used by > both the rpi3 and rpi4, and AFAIK nothing else as the rpi5-dev work is > currently only exposing XHCI. > > The ID is strictly the USB controller not the SoC. Its a bit confusingly > named, but something we inherited from the much older windows/edk2 port, > where it appears that the peripheral HID's were just picked in numerical > order. > > [0] https://github.com/tianocore/edk2-platforms/blob/12f68d29abdc9d703f67bd743fdec23ebb1e966e/Platform/RaspberryPi/AcpiTables/GpuDevs.asl#L15 So BCM2848, BCM2849, BCM2850 and so on are just made-up IDs for a Windows/EDK2 port that got cargo-culted into the kernel? Yikes! Has anyone checked whether they collide with actual Broadcom products?
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index 5a1500d0bdd9..66580de52882 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -23,6 +23,7 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) p->max_transfer_size = 65535; p->max_packet_count = 511; p->ahbcfg = 0x10; + p->no_clock_gating = true; } static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
On resume of the Raspberry Pi the dwc2 driver fails to enable HCD_FLAG_HW_ACCESSIBLE before re-enabling the interrupts. This causes a situation where both handler ignore a incoming port interrupt and force the upper layers to disable the dwc2 interrupt line. This leaves the USB interface in a unusable state: irq 66: nobody cared (try booting with the "irqpoll" option) CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.10.0-rc3 Hardware name: BCM2835 Call trace: unwind_backtrace from show_stack+0x10/0x14 show_stack from dump_stack_lvl+0x50/0x64 dump_stack_lvl from __report_bad_irq+0x38/0xc0 __report_bad_irq from note_interrupt+0x2ac/0x2f4 note_interrupt from handle_irq_event+0x88/0x8c handle_irq_event from handle_level_irq+0xb4/0x1ac handle_level_irq from generic_handle_domain_irq+0x24/0x34 generic_handle_domain_irq from bcm2836_chained_handle_irq+0x24/0x28 bcm2836_chained_handle_irq from generic_handle_domain_irq+0x24/0x34 generic_handle_domain_irq from generic_handle_arch_irq+0x34/0x44 generic_handle_arch_irq from __irq_svc+0x88/0xb0 Exception stack(0xc1b01f20 to 0xc1b01f68) 1f20: 0005c0d4 00000001 00000000 00000000 c1b09780 c1d6b32c c1b04e54 c1a5eae8 1f40: c1b04e90 00000000 00000000 00000000 c1d6a8a0 c1b01f70 c11d2da8 c11d4160 1f60: 60000013 ffffffff __irq_svc from default_idle_call+0x1c/0xb0 default_idle_call from do_idle+0x21c/0x284 do_idle from cpu_startup_entry+0x28/0x2c cpu_startup_entry from kernel_init+0x0/0x12c handlers: [<f539e0f4>] dwc2_handle_common_intr [<75cd278b>] usb_hcd_irq Disabling IRQ #66 Disabling clock gatling workaround this issue. Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.") Link: https://lore.kernel.org/linux-usb/3fd0c2fb-4752-45b3-94eb-42352703e1fd@gmx.net/T/ Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/usb/dwc2/params.c | 1 + 1 file changed, 1 insertion(+) -- 2.34.1