diff mbox series

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

Message ID 20240630153652.318882-10-wahrenst@gmx.net
State Superseded
Headers show
Series ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand

Commit Message

Stefan Wahren June 30, 2024, 3:36 p.m. UTC
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

Comments

Minas Harutyunyan July 1, 2024, 5:56 a.m. UTC | #1
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
>
Florian Fainelli July 4, 2024, 2:14 p.m. UTC | #2
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?
Stefan Wahren July 4, 2024, 3:33 p.m. UTC | #3
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/
Lukas Wunner July 5, 2024, 8:48 a.m. UTC | #4
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
Stefan Wahren July 5, 2024, 10:22 a.m. UTC | #5
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/
Lukas Wunner July 5, 2024, 3:03 p.m. UTC | #6
On Fri, Jul 05, 2024 at 12:22:33PM +0200, Stefan Wahren wrote:
> Am 05.07.24 um 10:48 schrieb Lukas Wunner:
> > 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.
> 
> 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?

I'd say mention both.  The Samsung one because it was the first
occurrence and the Agilex one because it specifically mentions
the interrupt storm which you've also witnessed on the Raspberry Pi.
Samsung's report mentions other symptoms than an interrupt storm.


> > 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).

Thanks,

Lukas
Stefan Wahren July 5, 2024, 3:21 p.m. UTC | #7
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
Jeremy Linton July 5, 2024, 5:16 p.m. UTC | #8
Hi,

On 7/5/24 10:21, Stefan Wahren wrote:
> 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?

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
Lukas Wunner July 5, 2024, 9:14 p.m. UTC | #9
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?
Stefan Wahren July 10, 2024, 1:33 p.m. UTC | #10
Am 05.07.24 um 23:14 schrieb Lukas Wunner:
> 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?
Using public available information like this [1], I wasn't able to find
any collision.

[1] - https://github.com/anholt/linux/wiki/Devices-with-Videocore-graphics
Jeremy Linton July 10, 2024, 3:50 p.m. UTC | #11
Hi,

On 7/5/24 16:14, Lukas Wunner wrote:
> 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!

You could say that, but there was some due diligence a couple years ago 
to track down the owner of the pnp id/information at broadcom, and it 
didn't yield anything helpful. Whether they are legitimate seems to be 
lost in time. At this point they are widely/publicly known Ids, without 
apparent conflict.


> 
> Has anyone checked whether they collide with actual Broadcom products?
diff mbox series

Patch

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)