[U-Boot] [PATCH] usb: ehci-mx5: Fix bus enumeration for DM case

[Marek Vasut]

On 2/26/20 10:16 AM, Lukasz Majewski wrote:
[...]
>>>>> diff --git a/drivers/usb/host/ehci-mx5.c
>>>>> b/drivers/usb/host/ehci-mx5.c index 0b32728c57..4db513f4e5 100644
>>>>> --- a/drivers/usb/host/ehci-mx5.c
>>>>> +++ b/drivers/usb/host/ehci-mx5.c
>>>>> @@ -301,6 +301,22 @@ static int ehci_usb_ofdata_to_platdata(struct
>>>>> udevice *dev) return 0;
>>>>>  }
>>>>>  
>>>>> +static int ehci_usb_bind(struct udevice *dev)
>>>>> +{
>>>>> +	static int num_controllers;
>>>>> +
>>>>> +	/*
>>>>> +	 * Without this hack, if we return ENODEV for USB
>>>>> Controller 0, on
>>>>> +	 * probe for the next controller, USB Controller 1 will
>>>>> be given a
>>>>> +	 * sequence number of 0. This conflicts with our
>>>>> requirement of
>>>>> +	 * sequence numbers while initialising the peripherals.
>>>>> +	 */
>>>>> +	dev->req_seq = num_controllers;
>>>>> +	num_controllers++;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int ehci_usb_probe(struct udevice *dev)
>>>>>  {
>>>>>  	struct usb_platdata *plat = dev_get_platdata(dev);
>>>>> @@ -362,6 +378,7 @@ U_BOOT_DRIVER(usb_mx5) = {
>>>>>  	.id	= UCLASS_USB,
>>>>>  	.of_match = mx5_usb_ids,
>>>>>  	.ofdata_to_platdata = ehci_usb_ofdata_to_platdata,
>>>>> +	.bind	= ehci_usb_bind,
>>>>>  	.probe	= ehci_usb_probe,
>>>>>  	.remove = ehci_deregister,
>>>>>  	.ops	= &ehci_usb_ops,    
>>>>
>>>> Tested-by: Lukasz Majewski <lukma at denx.de>
>>>>
>>>> Tested on HSC|DDC i.MX53 board (usb start works as previously).
>>>>  
>>>
>>> Gentle ping on this patch :-)  
>>
>> I think this approach doesn't work, 
>> there was a discussion about this
>> under a patch like this for ehci-mx6.c . Take a look at the patches
>> for the ehci-mx6.c in mainline and replicate the approach
> 
> The code for ehci-mx5 DM/DTS conversion was based on ehci-mx6. There is
> a patch developed by you - SHA1:
> 501547cec1f7f0438cae388a104ff60f18576c01 which provides a partial DT
> conversion. 

Yes, and devfdt_get_addr_size_index() and "dev->req_seq = (addr -
USB_BASE_ADDR) / size" is used for a reason, read the TODO text in
501547cec1f7f0438cae388a104ff60f18576c01 .

> On top of that is another patch - SHA1:
> 1198a104d37b10064cd90f36d472787d549eda02 developed by Igor, which adds
> even more hacks for i.MX7.
> 
> That is what we do have now in-tree for iMX6/iMX7.

The TODO explains why it is done like that and how to finish the conversion.

> Since the last ping for this patch (6+ months) we do have a regression
> (just not usable USB) for some iMX53 boards. This patch fixes them.

This patch also breaks other MX53 boards for sure, again, read the TODO
in 501547cec1f7f0438cae388a104ff60f18576c01, it explains why this patch
is wrong, it's about the sequential ordering of PHY addresses.

Basically, consider that you only enable USB controller #1 and not USB
controller #0 . Then the controller ID would not match the PHY ID, but
the PHY address offset is derived from controller ID. So this setup
would then fail by accessing the wrong PHY for the controller.

> Why are you reluctant to accept the fix? It is similar to what was
> added for iMX7.

Because this is NOT a fix, this introduces another problem, see above.

>> , or even
>> better, solve the TODO there altogether.
> 
> Please correct me if I did not understood you correctly:
> 
> To fix regression on i.MX53 boards (with applying this patch) you do
> recommend to look for out of tree patches for i.MX6/i.MX7 and solve the
> TODO for i.MX6/i.MX7?

No. I recommend to fix this problem the same way as is done in
501547cec1f7f0438cae388a104ff60f18576c01 and ideally fix the TODO
outlined in that patch completely (which is more involved).

There are no out-of-tree patches involved here.