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

[Marek Vasut]

On 02/05/2018 07:18 PM, Alexey Brodkin wrote:
> 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 :)

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
> 


-- 
Best regards,
Marek Vasut