diff mbox series

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

Message ID 20240603130219.25825-1-joswang1221@gmail.com
State New
Headers show
Series [v2,1/3] dt-bindings: usb: dwc3: Add snps,p2p3tranok quirk | expand

Commit Message

joswang June 3, 2024, 1:02 p.m. UTC 
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.

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(+)

Comments

joswang June 7, 2024, 2:07 p.m. UTC | #1 
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?

Thanks,
Jos Wang
Thinh Nguyen June 7, 2024, 10:36 p.m. UTC | #2 
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
Thinh Nguyen June 7, 2024, 10:49 p.m. UTC | #3 
On Fri, Jun 07, 2024, joswang wrote:

> My initial idea was similar to yours,Please help review the following changes.


> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 3a8fbc2d6b99..8c6a09718737 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -961,11 +961,15 @@ static bool dwc3_core_is_valid(struct dwc3 *dwc)
>  static void dwc3_core_setup_global_control(struct dwc3 *dwc)
>  {
>         u32 reg;
> +       unsigned int power_opt;
> +       unsigned int hw_mode;

Use reverse christmas tree declaration style:

	type1 abcdefg
	type2 abcde
	type3 abc

>  
>         reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>         reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> +       hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +       power_opt = DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1);
>  
> -       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> +       switch (power_opt) {
>         case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
>                 /**
>                  * WORKAROUND: DWC3 revisions between 2.10a and 2.50a have an
> @@ -998,6 +1002,18 @@ static void dwc3_core_setup_global_control(struct dwc3
> *dwc)
>                 break;
>         }
>  
> +       /*
> +        * WORKAROUND: DWC_usb31 version 2.00a have an issue that would
> +        * cause a CSR read timeout When CSR read coincides with RAM
> +        * Clock Gating Entry.

Note in the comment and commit message that this applies while operating
as host mode. Add the STAR number reference in the commit message.

> +        *
> +        * This workaround solution disable Clock Gating, sacrificing
> +        * power consumption for normal operation.
> +        */
> +       if (power_opt != DWC3_GHWPARAMS1_EN_PWROPT_NO &&
> +           hw_mode != DWC3_GHWPARAMS0_MODE_GADGET && DWC3_VER_IS(DWC31, 200A))
> +               reg |= DWC3_GCTL_DSBLCLKGTNG;
> +

Thanks,
Thinh
diff mbox series

Patch

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
+		 *
+		 * 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_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) &&
+				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 */