Re: usb: dwc2: USB on Agilex platform broken after commit 50fb0c128b6e

[Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>]

Hi Dinh,

On 12/14/2021 9:31 PM, Dinh Nguyen wrote:
> On Tue, Dec 14, 2021 at 10:50 AM Dinh Nguyen <dinguyen@kernel.org> wrote:
>>
>>
>>
>> On 12/14/21 9:50 AM, Dinh Nguyen wrote:
>>>
>>>
>>> On 12/14/21 12:13 AM, Minas Harutyunyan wrote:
>>>> Hi Dinh,
>>>>
>>>> On 12/14/2021 12:40 AM, Dinh Nguyen wrote:
>>>>>
>>>>>
>>>>> On 12/13/21 10:13 AM, Dinh Nguyen wrote:
>>>>>> Hi Minas,
>>>>>>
>>>>>> Thanks for replying!
>>>>>>
>>>>>> On 12/13/21 3:47 AM, Minas Harutyunyan wrote:
>>>>>>> Hi Dinh,
>>>>>>>
>>>>>>> On 12/9/2021 1:44 AM, Dinh Nguyen wrote:
>>>>>>>> Hi Arthur,
>>>>>>>>
>>>>>>>> The usb driver is getting this error:
>>>>>>>>
>>>>>>>> [    1.539856] Hardware name: SoCFPGA Agilex SoCDK (DT)
>>>>>>>> [    1.544799] Call trace:
>>>>>>>> [    1.547233]  dump_backtrace+0x0/0x1c0
>>>>>>>> [    1.550897]  show_stack+0x18/0x68
>>>>>>>> [    1.554202]  dump_stack+0xd8/0x134
>>>>>>>> [    1.557597]  __report_bad_irq+0x38/0xf0
>>>>>>>> [    1.561423]  note_interrupt+0x31c/0x3e8
>>>>>>>> [    1.565244]  handle_irq_event_percpu+0x60/0x90
>>>>>>>> [    1.569671]  handle_irq_event+0x48/0xe0
>>>>>>>> [    1.573491]  handle_fasteoi_irq+0xb0/0x138
>>>>>>>> [    1.577574]  generic_handle_irq+0x30/0x48
>>>>>>>> [    1.581567]  __handle_domain_irq+0x60/0xb8
>>>>>>>> [    1.585646]  gic_handle_irq+0xb4/0xd0
>>>>>>>> [    1.589300]  el1_irq+0xb8/0x150
>>>>>>>> [    1.592428]  arch_cpu_idle+0x18/0x28
>>>>>>>> [    1.595992]  default_idle_call+0x24/0x5c
>>>>>>>> [    1.599899]  do_idle+0x1f4/0x290
>>>>>>>> [    1.603118]  cpu_startup_entry+0x28/0x68
>>>>>>>> [    1.607026]  rest_init+0xd8/0xe8
>>>>>>>> [    1.610242]  arch_call_rest_init+0x10/0x1c
>>>>>>>> [    1.614329]  start_kernel+0x50c/0x544
>>>>>>>> [    1.617980] handlers:
>>>>>>>> [    1.620241] [<00000000ae601aac>] dwc2_handle_common_intr
>>>>>>>> [    1.625541] [<00000000255d793a>] dwc2_hsotg_irq
>>>>>>>> [    1.630056] [<000000000bc68ab4>] usb_hcd_irq
>>>>>>>> [    1.634312] Disabling IRQ #33
>>>>>>>>
>>>>>>>> After a git-bisect, the result is this commit:
>>>>>>>>
>>>>>>>>
>>>>>>>> commit 50fb0c128b6e8df62200ba85c05ecd6b1396476d
>>>>>>>> Author: Artur Petrosyan<Arthur.Petrosyan@synopsys.com>
>>>>>>>> Date:   Tue Apr 13 11:37:15 2021 +0400
>>>>>>>>
>>>>>>>>         usb: dwc2: Add clock gating entering flow by system suspend
>>>>>>>>         If not hibernation nor partial power down are supported,
>>>>>>>>         clock gating is used to save power.
>>>>>>>>         Adds a new flow of entering clock gating when PC is
>>>>>>>>         suspended.
>>>>>>>>         Signed-off-by: Artur Petrosyan<Arthur.Petrosyan@synopsys.com>
>>>>>>>> Link:https://urldefense.com/v3/__https://lore.kernel.org/r/20210413073716.30C13A0094@mailhost.synopsys.com__;!!A4F2R9G_pg!IjGS70cMXB9miMnPdcoGriUpDvARm7ScwrlQXAiH9qN-ZnpcaiM_gD-KWp___MVyvEPm_djJ$
>>>>>>>>
>>>>>>>>
>>>>>>>>         Signed-off-by: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>>>>>
>>>>>>>> The DWC2 USB version on the Agilex platform is
>>>>>>>> 0x4F54330A
>>>>>>>>
>>>>>>> Could you please provide HW configuration registers values
>>>>>>> (GHWCFG1-4).
>>>>>>>
>>>>>>
>>>>>> [    0.987779] dwc2 ffb00000.usb: GHWCFG1    @0xFFFF800012940044 :
>>>>>> 0x00000000
>>>>>> [    0.994644] dwc2 ffb00000.usb: GHWCFG2    @0xFFFF800012940048 :
>>>>>> 0x238FFC90
>>>>>> [    1.001498] dwc2 ffb00000.usb: GHWCFG3    @0xFFFF80001294004C :
>>>>>> 0x1F8002E8
>>>>>> [    1.008350] dwc2 ffb00000.usb: GHWCFG4    @0xFFFF800012940050 :
>>>>>> 0xFE0F0020
>>>>>>
>>>>>>
>>>>>> I did a little debugging, and noticed that the GINTSTS_PRTINT is
>>>>>> firing, so the dwc2_handle_common_intr() is getting called
>>>>>> continuously. There's doesn't seem to be way to handle the
>>>>>> GINTSTS_PRTINT bit, because there's a check for device_mode().
>>>>>>
>>>>>
>>>>> It looks like the LPMMode bit is disabled in the GHWCFG3. Could that be
>>>>> the reason why?
>>>>>
>>>>> Dinh
>>>>
>>>> Could you please disable clock gating by adding to
>>>> dwc2_set_default_params() function follow line:
>>>> p->no_clock_gating = true;
>>>> like in dwc2_set_s3c6400_params() function.
>>>>
>>>
>>> That did not fix the issue. However, if I remove the line:
>>>
>>> +               /* After entering suspend, hardware is not accessible */
>>> +               clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>>>
>>> Then the issue gets fixed.
>>>
>>
>> I don't see _dwc2_hcd_resume() getting called.
>>
> 
> This fixes it:
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 13c779a28e94..f63a27d11fac 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4399,11 +4399,12 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>                   * If not hibernation nor partial power down are supported,
>                   * clock gating is used to save power.
>                   */
> -               if (!hsotg->params.no_clock_gating)
> +               if (!hsotg->params.no_clock_gating) {
>                          dwc2_host_enter_clock_gating(hsotg);
> 
> -               /* After entering suspend, hardware is not accessible */
> -               clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +                       /* After entering suspend, hardware is not accessible */
> +                       clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +               }
>                  break;
>          default:
>                  goto skip_power_saving;

Good, this patch acceptable.

> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index d300ae3d9274..9102fe8f6021 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -440,6 +440,7 @@ static void dwc2_set_default_params(struct
> dwc2_hsotg *hsotg)
>                  p->g_np_tx_fifo_size = 1024;
>                  dwc2_set_param_tx_fifo_sizes(hsotg);
>          }
> +       p->no_clock_gating = true;
Can be your platform Agilex included in list of capabilities device and 
"p->no_clock_gating = true;" peformed inside to platform specific 
settings function instead of dwc2_set_default_params()?

Thanks,
Minas


>   }