Re: [PATCH v2, 3/3] usb: dwc3: core: Workaround for CSR read timeout

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Fri, Jun 07, 2024, joswang wrote:
> On Thu, Jun 6, 2024 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Tue, Jun 04, 2024, joswang wrote:
> > > On Tue, Jun 4, 2024 at 8:07 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > > >
> > > > On Mon, Jun 03, 2024, joswang wrote:
> > > > > From: joswang <joswang@lenovo.com>
> > > > >
> > > > > DWC31 version 2.00a have an issue that would cause
> > > > > a CSR read timeout When CSR read coincides with RAM
> > > > > Clock Gating Entry.
> > > >
> > > > Do you have the STAR issue number?
> > > >
> > > Thanks for reviewing the code.
> > > The STAR number provided by Synopsys is 4846132.
> > > Please help review further.
> >
> > I've confirmed internally. As you have noted, this applies to DWC_usb31
> > v2.00a for host mode only and DRD mode operating as host.
> >
> > >
> > > > >
> > > > > This workaround solution disable Clock Gating, sacrificing
> > > > > power consumption for normal operation.
> > > > >
> > > > > Signed-off-by: joswang <joswang@lenovo.com>
> > > > > ---
> > > > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index 3a8fbc2d6b99..1df85c505c9e 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -978,11 +978,22 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > > >                *
> > > > >                * STAR#9000588375: Clock Gating, SOF Issues when ref_clk-Based
> > > > >                * SOF/ITP Mode Used
> >
> > Since there's another STAR, let's split the if-else case separately and
> > provide the comments separately.
> >
> OK
> > > > > +              *
> > > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> >
> > Can we use the full name DWC_usb31 instead of DWC31.
> >
> Subsequent V3 versions use DWC_usb31
> > > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > > +              * Clock Gating Entry.
> > > > > +              *
> > > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > > +              * power consumption for normal operation.
> > > > >                */
> > > > >               if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > >                               dwc->dr_mode == USB_DR_MODE_OTG) &&
> > > > >                               DWC3_VER_IS_WITHIN(DWC3, 210A, 250A))
> > > > >                       reg |= DWC3_GCTL_DSBLCLKGTNG | DWC3_GCTL_SOFITPSYNC;
> > > > > +             else if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > > +                             dwc->dr_mode == USB_DR_MODE_OTG) &&
> >
> > There's no OTG mode for DWC_usb31. Let's enable this workaround if the
> > HW mode is not DWC_GHWPARAMS0_MODE_GADGET.
> >
> > > > > +                             DWC3_VER_IS(DWC31, 200A))
> > > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > > >               else
> > > > >                       reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > > > >               break;
> > > > > @@ -992,6 +1003,18 @@ static void dwc3_core_setup_global_control(struct dwc3 *dwc)
> > > > >                * will work. Device-mode hibernation is not yet implemented.
> > > > >                */
> > > > >               reg |= DWC3_GCTL_GBLHIBERNATIONEN;
> > > > > +
> > > > > +             /*
> > > > > +              * WORKAROUND: DWC31 version 2.00a have an issue that would
> > > > > +              * cause a CSR read timeout When CSR read coincides with RAM
> > > > > +              * Clock Gating Entry.
> > > > > +              *
> > > > > +              * This workaround solution disable Clock Gating, sacrificing
> > > > > +              * power consumption for normal operation.
> > > > > +              */
> > > > > +             if ((dwc->dr_mode == USB_DR_MODE_HOST ||
> > > > > +                  dwc->dr_mode == USB_DR_MODE_OTG) && DWC3_VER_IS(DWC31, 200A))
> > > > > +                     reg |= DWC3_GCTL_DSBLCLKGTNG;
> > > > >               break;
> > > > >       default:
> > > > >               /* nothing */
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> >
> > We have the same checks and comments here. Can we refactor?
> > Perhaps something this?
> >
> > power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
> > switch (power_opt) {
> >     ...
> > }
> >
> > /*
> >  * <comment>
> >  */
> > if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO) {
> > }
> >
> >
> > Thanks,
> > Thinh
> 
> Thank you for your valuable suggestions.I can refactor according to
> your suggestion.
> Do I need to submit a V3 version patch separately, or should I submit
> a V3 version patch together with other cases?

I haven't reviewed the other case in detail yet. I'll get back on that.

It may be better if you can submit this separatedly so that the other
case won't hold this back (and it maybe easier for tracking too).

Thanks,
Thinh