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

[Marek Vasut]

On 6/24/19 12:26 PM, Lukasz Majewski wrote:
> On Mon, 24 Jun 2019 12:06:28 +0200
> Marek Vasut <marex at denx.de> wrote:
> 
>> On 6/24/19 8:33 AM, Peng Fan wrote:
>>> Hi Marek,
>>>   
>>>> Subject: Re: [PATCH] usb: ehci-mx6: Fix bus enumeration for DM case
>>>>
>>>> On 6/21/19 3:45 AM, Peng Fan wrote:  
>>>>> Hi Marek,
>>>>>  
>>>>>> -----Original Message-----
>>>>>> From: Marek Vasut [mailto:marex at denx.de]
>>>>>> Sent: 2019年6月21日 4:54
>>>>>> To: u-boot at lists.denx.de
>>>>>> Cc: Marek Vasut <marex at denx.de>; Abel Vesa <abel.vesa at nxp.com>;  
>>>> Adam  
>>>>>> Ford <aford173 at gmail.com>; Fabio Estevam <festevam at gmail.com>;  
>>>> Ludwig  
>>>>>> Zenz <lzenz at dh-electronics.com>; Peng Fan <peng.fan at nxp.com>;  
>>>> Stefano  
>>>>>> Babic <sbabic at denx.de>; Vagrant Cascadian <vagrant at debian.org>
>>>>>> Subject: [PATCH] usb: ehci-mx6: Fix bus enumeration for DM case
>>>>>>
>>>>>> It is likely that the DM conversion of EHCI iMX6 driver was a
>>>>>> derivative of EHCI VF, however the conversion is incomplete and
>>>>>> is missing the bind workaround, which updates dev->seq number.
>>>>>> Without this, all controllers have dev->seq number 0 . Add this
>>>>>> bind workaround  
>>>> into EHCI iMX6 driver as well.  
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>> Cc: Abel Vesa <abel.vesa at nxp.com>
>>>>>> Cc: Adam Ford <aford173 at gmail.com>
>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
>>>>>> Cc: Ludwig Zenz <lzenz at dh-electronics.com>
>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>> Cc: Stefano Babic <sbabic at denx.de>
>>>>>> Cc: Vagrant Cascadian <vagrant at debian.org>
>>>>>> ---
>>>>>>  drivers/usb/host/ehci-mx6.c | 17 +++++++++++++++++
>>>>>>  1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/host/ehci-mx6.c
>>>>>> b/drivers/usb/host/ehci-mx6.c index 33abfeada0..109ed7ed4a 100644
>>>>>> --- a/drivers/usb/host/ehci-mx6.c
>>>>>> +++ b/drivers/usb/host/ehci-mx6.c
>>>>>> @@ -503,6 +503,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;  
>>>>>
>>>>> With alias in dts, no need this, right?  
>>>>
>>>> There are no aliases in the DTs for the USB controllers, so that
>>>> doesn't help. I think for DM, the real solution would be to parse
>>>> the PHY phandle and pass around the PHY address instead of some
>>>> arbitrary index, but that's something to be done for next release.
>>>> For this release, it's only fixes.  
>>>
>>> I think the better method is use alias in dts, introducing
>>> xx-u-boot.dtsi for such case.  
>>
>> What good would that make if you can obtain the PHY address from the
>> DT, without the need for arbitrary error-prone indexes? The indexes
>> are remnants from when there was no DM or from the DM conversion, so
>> we should remove them altogether.
>>
>>>> Would you be interested in implementing the later for -next ? ;-)  
>>>
>>> In NXP vendor tree, we have ehci_usb_get_phy to parse phy from dtb,
>>> I could find some time send that to community.  
>> What about implementing a proper USB PHY driver, which binds to a DT
>> entry, and then hooking it up to the EHCI MX6 ? IIRC the
>> infrastructure for this is already in place and that would be the
>> right (TM) solution.
>>
> 
> Sorry, but I'm a bit confused.
> 
> The patch that you introduced above, in the commit message, states
> clearly that it is a hack.
> 
> Why shall we allow introducing hacks instead of implementing it in the
> "right way" from the outset ?

Because we're in RC4 right now and this is the simplest possible fix
which works and is consistent with the other NXP EHCI drivers. See the
commit message -- this fills in the missing bit which ehci-vf has, but
which was omitted from ehci-mx6 and ehci-mx5 during the conversion.

The PHY driver and the migration to that infrastructure can be done in
the next cycle, but right now there's no time to develop a driver and
thoroughly test either the PHY driver or the changes to the EHCI driver,
no way.

> As you mentioned the infrastructure is already in place, so why not do
> it in the correct way?
> 
> And yes, as Peng noted up, till now many boards use aliases to fix this
> situation.

Right, so this does not work with the USB stack, but feel free to try it
yourself. That's why the ehci-vf has this hack in place.

And I am in fact fine with this, because you don't end up patching DTs
with arbitrary aliases AND because this will code go away once the PHY
driver is in place anyway and so would the aliases.

-- 
Best regards,
Marek Vasut