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

Mon Jun 24 11:57:49 UTC 2019

Hi Marek,

> 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.
> 

Apparently I was not causing the error which you have encountered.

> 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.

That is why I'm asking as I do use usb on imx53 and imx6q boards. The
imx53 uses usbh1 which has alias to usb1.

> 
> And I am in fact fine with this, because you don't end up patching DTs
> with arbitrary aliases

This patch causes regression (the board hang) when tested on TPC70
i.MX6Q board.

master branch
SHA1:  77f6e2dd0551d8a825bab391a1bd6b838874bcd4

Log from u-boot (broken):

Booting update from usb ...  
starting USB...      
Bus usb at 2184200:

Working one:

=> usb start
starting USB...
Bus usb at 2184200: USB EHCI 1.00
scanning bus usb at 2184200 for devices... 2 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found

> AND because this will code go away once the PHY
> driver is in place anyway and so would the aliases.
> 

For now it causes working board to hang. Please fix the patch.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190624/aca65b21/attachment.sig>