[U-Boot] [PATCH] usb: dwc2: Fix logic for setup of GUSBCFG->ULPI_UTMI_Sel bit

[Alexey Brodkin]

Hi Marek,

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Monday, February 5, 2018 4:23 PM
> To: Alexey Brodkin <Alexey.Brodkin at synopsys.com>; u-boot at 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 at synopsys.com>
> > Cc: Marek Vasut <marex at 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