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

[Marek Vasut]

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 ?

> +#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
> 


-- 
Best regards,
Marek Vasut