| Message ID | 20180202155826.15472-1-abrodkin@synopsys.com |
|---|---|
| State | Deferred |
| Delegated to: | Alexey Brodkin |
| Headers | show |
| Series | [U-Boot] usb: dwc2: Fix logic for setup of GUSBCFG->ULPI_UTMI_Sel bit | expand |
On 02/02/2018 04:58 PM, Alexey Brodkin wrote: > According to the databook "ULPI_UTMI_Sel" bit (#4) in GUSBCFG > register selects between ULPI and UTMI+ interfaces such that: > 0 - stands for UTMI+ interface > 1 - stands for ULPI interface > > Currently implemented logic in the driver is incorrect because > CONFIG_DWC2_PHY_TYPE is not a "bool" but instead this is an "enum" > which starts from 0 in case of full-speed and is either 1 for > UTMI or 2 for ULPI. > > In dwc2.h we have: > ---------------------->8---------------------- > #define DWC2_PHY_TYPE_FS 0 > #define DWC2_PHY_TYPE_UTMI 1 > #define DWC2_PHY_TYPE_ULPI 2 > #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI > ---------------------->8---------------------- > > And in dwc2.c we do "|= CONFIG_DWC2_PHY_TYPE << 4" > effectively setting "ULPI_UTMI_Sel" bit for UTMI+ and > what's even worse for ULMI we keep "ULPI_UTMI_Sel" zeroed > while setting the next "FSIntf" bit (#5) unexpectedly selecting > Full speed interface! > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Marek Vasut <marex@denx.de> > --- > drivers/usb/host/dwc2.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 784fcbdbd94f..29138a2579ab 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -366,7 +366,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) > * immediately after setting phyif. > */ > usbcfg &= ~(DWC2_GUSBCFG_ULPI_UTMI_SEL | DWC2_GUSBCFG_PHYIF); > - usbcfg |= CONFIG_DWC2_PHY_TYPE << DWC2_GUSBCFG_ULPI_UTMI_SEL_OFFSET; What happens if PHY type is set to FS or UTMI , won't that change the behavior of this code ? > +#if (CONFIG_DWC2_PHY_TYPE == DWC2_PHY_TYPE_ULPI) > + usbcfg |= DWC2_GUSBCFG_ULPI_UTMI_SEL; > +#endif > > if (usbcfg & DWC2_GUSBCFG_ULPI_UTMI_SEL) { /* ULPI interface */ This condition should be tweaked, I think. You set the ULPI_UTMI_SEL bit above and check for it here again, that's a bit odd. > #ifdef CONFIG_DWC2_PHY_ULPI_DDR >
Hi Marek, > -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Monday, February 5, 2018 4:23 PM > To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>; u-boot@lists.denx.de > Subject: Re: [PATCH] usb: dwc2: Fix logic for setup of GUSBCFG->ULPI_UTMI_Sel bit > > On 02/02/2018 04:58 PM, Alexey Brodkin wrote: > > According to the databook "ULPI_UTMI_Sel" bit (#4) in GUSBCFG > > register selects between ULPI and UTMI+ interfaces such that: > > 0 - stands for UTMI+ interface > > 1 - stands for ULPI interface > > > > Currently implemented logic in the driver is incorrect because > > CONFIG_DWC2_PHY_TYPE is not a "bool" but instead this is an "enum" > > which starts from 0 in case of full-speed and is either 1 for > > UTMI or 2 for ULPI. > > > > In dwc2.h we have: > > ---------------------->8---------------------- > > #define DWC2_PHY_TYPE_FS 0 > > #define DWC2_PHY_TYPE_UTMI 1 > > #define DWC2_PHY_TYPE_ULPI 2 > > #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI > > ---------------------->8---------------------- > > > > And in dwc2.c we do "|= CONFIG_DWC2_PHY_TYPE << 4" > > effectively setting "ULPI_UTMI_Sel" bit for UTMI+ and > > what's even worse for ULMI we keep "ULPI_UTMI_Sel" zeroed > > while setting the next "FSIntf" bit (#5) unexpectedly selecting > > Full speed interface! > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Marek Vasut <marex@denx.de> > > --- > > drivers/usb/host/dwc2.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > > index 784fcbdbd94f..29138a2579ab 100644 > > --- a/drivers/usb/host/dwc2.c > > +++ b/drivers/usb/host/dwc2.c > > @@ -366,7 +366,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) > > * immediately after setting phyif. > > */ > > usbcfg &= ~(DWC2_GUSBCFG_ULPI_UTMI_SEL | DWC2_GUSBCFG_PHYIF); > > - usbcfg |= CONFIG_DWC2_PHY_TYPE << DWC2_GUSBCFG_ULPI_UTMI_SEL_OFFSET; > > What happens if PHY type is set to FS or UTMI , won't that change the > behavior of this code ? Well if you look a bit more at that driver you'll notice that [there're way too many ifdefs] FS-part is in a separate ifdef section so we don’t need to worry about it in HS-part :) > > +#if (CONFIG_DWC2_PHY_TYPE == DWC2_PHY_TYPE_ULPI) > > + usbcfg |= DWC2_GUSBCFG_ULPI_UTMI_SEL; > > +#endif > > > > if (usbcfg & DWC2_GUSBCFG_ULPI_UTMI_SEL) { /* ULPI interface */ > > This condition should be tweaked, I think. You set the ULPI_UTMI_SEL bit > above and check for it here again, that's a bit odd. Completely agree but again my intention was to move with very tiny steps Fixing one issue at a time such that heavy refactoring could be escaped... But I would agree that this driver definitely needs refactoring because of those Configuration options, some incorrect checks etc. But I'd think it's still better To have expectedly working code today (i.e. fix existing code) instead of waiting for major rewrite sometime down the line. -Alexey
On 02/05/2018 07:18 PM, Alexey Brodkin wrote: > Hi Marek, > >> -----Original Message----- >> From: Marek Vasut [mailto:marex@denx.de] >> Sent: Monday, February 5, 2018 4:23 PM >> To: Alexey Brodkin <Alexey.Brodkin@synopsys.com>; u-boot@lists.denx.de >> Subject: Re: [PATCH] usb: dwc2: Fix logic for setup of GUSBCFG->ULPI_UTMI_Sel bit >> >> On 02/02/2018 04:58 PM, Alexey Brodkin wrote: >>> According to the databook "ULPI_UTMI_Sel" bit (#4) in GUSBCFG >>> register selects between ULPI and UTMI+ interfaces such that: >>> 0 - stands for UTMI+ interface >>> 1 - stands for ULPI interface >>> >>> Currently implemented logic in the driver is incorrect because >>> CONFIG_DWC2_PHY_TYPE is not a "bool" but instead this is an "enum" >>> which starts from 0 in case of full-speed and is either 1 for >>> UTMI or 2 for ULPI. >>> >>> In dwc2.h we have: >>> ---------------------->8---------------------- >>> #define DWC2_PHY_TYPE_FS 0 >>> #define DWC2_PHY_TYPE_UTMI 1 >>> #define DWC2_PHY_TYPE_ULPI 2 >>> #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI >>> ---------------------->8---------------------- >>> >>> And in dwc2.c we do "|= CONFIG_DWC2_PHY_TYPE << 4" >>> effectively setting "ULPI_UTMI_Sel" bit for UTMI+ and >>> what's even worse for ULMI we keep "ULPI_UTMI_Sel" zeroed >>> while setting the next "FSIntf" bit (#5) unexpectedly selecting >>> Full speed interface! >>> >>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> >>> Cc: Marek Vasut <marex@denx.de> >>> --- >>> drivers/usb/host/dwc2.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >>> index 784fcbdbd94f..29138a2579ab 100644 >>> --- a/drivers/usb/host/dwc2.c >>> +++ b/drivers/usb/host/dwc2.c >>> @@ -366,7 +366,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >>> * immediately after setting phyif. >>> */ >>> usbcfg &= ~(DWC2_GUSBCFG_ULPI_UTMI_SEL | DWC2_GUSBCFG_PHYIF); >>> - usbcfg |= CONFIG_DWC2_PHY_TYPE << DWC2_GUSBCFG_ULPI_UTMI_SEL_OFFSET; >> >> What happens if PHY type is set to FS or UTMI , won't that change the >> behavior of this code ? > > Well if you look a bit more at that driver you'll notice that > [there're way too many ifdefs] FS-part is in a separate ifdef section > so we don’t need to worry about it in HS-part :) Do you want to clean it up ? :) >>> +#if (CONFIG_DWC2_PHY_TYPE == DWC2_PHY_TYPE_ULPI) >>> + usbcfg |= DWC2_GUSBCFG_ULPI_UTMI_SEL; >>> +#endif >>> >>> if (usbcfg & DWC2_GUSBCFG_ULPI_UTMI_SEL) { /* ULPI interface */ >> >> This condition should be tweaked, I think. You set the ULPI_UTMI_SEL bit >> above and check for it here again, that's a bit odd. > > Completely agree but again my intention was to move with very tiny steps > Fixing one issue at a time such that heavy refactoring could be escaped... I'd like this cleaned, this is really braindead. Feel free to send two patches or squash it into one, I don't care, but this is just braindead. > But I would agree that this driver definitely needs refactoring because of those > Configuration options, some incorrect checks etc. But I'd think it's still better > To have expectedly working code today (i.e. fix existing code) instead of waiting > for major rewrite sometime down the line. I'm not debating that part, I'm just cranky about the weirdness above. > -Alexey >
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 784fcbdbd94f..29138a2579ab 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -366,7 +366,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) * immediately after setting phyif. */ usbcfg &= ~(DWC2_GUSBCFG_ULPI_UTMI_SEL | DWC2_GUSBCFG_PHYIF); - usbcfg |= CONFIG_DWC2_PHY_TYPE << DWC2_GUSBCFG_ULPI_UTMI_SEL_OFFSET; +#if (CONFIG_DWC2_PHY_TYPE == DWC2_PHY_TYPE_ULPI) + usbcfg |= DWC2_GUSBCFG_ULPI_UTMI_SEL; +#endif if (usbcfg & DWC2_GUSBCFG_ULPI_UTMI_SEL) { /* ULPI interface */ #ifdef CONFIG_DWC2_PHY_ULPI_DDR
According to the databook "ULPI_UTMI_Sel" bit (#4) in GUSBCFG register selects between ULPI and UTMI+ interfaces such that: 0 - stands for UTMI+ interface 1 - stands for ULPI interface Currently implemented logic in the driver is incorrect because CONFIG_DWC2_PHY_TYPE is not a "bool" but instead this is an "enum" which starts from 0 in case of full-speed and is either 1 for UTMI or 2 for ULPI. In dwc2.h we have: ---------------------->8---------------------- #define DWC2_PHY_TYPE_FS 0 #define DWC2_PHY_TYPE_UTMI 1 #define DWC2_PHY_TYPE_ULPI 2 #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI ---------------------->8---------------------- And in dwc2.c we do "|= CONFIG_DWC2_PHY_TYPE << 4" effectively setting "ULPI_UTMI_Sel" bit for UTMI+ and what's even worse for ULMI we keep "ULPI_UTMI_Sel" zeroed while setting the next "FSIntf" bit (#5) unexpectedly selecting Full speed interface! Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Marek Vasut <marex@denx.de> --- drivers/usb/host/dwc2.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)