Re: [PATCH 09/11] usb: dwc2: Skip clock gating on Broadcom SoCs

[Stefan Wahren <wahrenst@gmx.net>]

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/