diff mbox series

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

Message ID 20190620205340.11015-1-marex@denx.de
State Deferred
Delegated to: Marek Vasut
Headers show
Series [U-Boot] usb: ehci-mx6: Fix bus enumeration for DM case | expand

Commit Message

Marek Vasut June 20, 2019, 8:53 p.m. UTC
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@denx.de>
Cc: Abel Vesa <abel.vesa@nxp.com>
Cc: Adam Ford <aford173@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Ludwig Zenz <lzenz@dh-electronics.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Vagrant Cascadian <vagrant@debian.org>
---
 drivers/usb/host/ehci-mx6.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Peng Fan June 21, 2019, 1:45 a.m. UTC | #1
Hi Marek,

> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: 2019年6月21日 4:54
> To: u-boot@lists.denx.de
> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>; Adam
> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;
> Ludwig Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;
> Stefano Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
> Cc: Abel Vesa <abel.vesa@nxp.com>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Vagrant Cascadian <vagrant@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?

Regards,
Peng.

> +	num_controllers++;
> +
> +	return 0;
> +}
> +
>  static int ehci_usb_probe(struct udevice *dev)  {
>  	struct usb_platdata *plat = dev_get_platdata(dev); @@ -564,6 +580,7
> @@ U_BOOT_DRIVER(usb_mx6) = {
>  	.id	= UCLASS_USB,
>  	.of_match = mx6_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,
> --
> 2.20.1
Marek Vasut June 21, 2019, 10:50 a.m. UTC | #2
On 6/21/19 3:45 AM, Peng Fan wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut [mailto:marex@denx.de]
>> Sent: 2019年6月21日 4:54
>> To: u-boot@lists.denx.de
>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>; Adam
>> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;
>> Ludwig Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;
>> Stefano Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
>> Cc: Abel Vesa <abel.vesa@nxp.com>
>> Cc: Adam Ford <aford173@gmail.com>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Vagrant Cascadian <vagrant@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.

Would you be interested in implementing the later for -next ? ;-)
Peng Fan June 24, 2019, 6:33 a.m. UTC | #3
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@denx.de]
> >> Sent: 2019年6月21日 4:54
> >> To: u-boot@lists.denx.de
> >> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>;
> Adam
> >> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;
> Ludwig
> >> Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;
> Stefano
> >> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
> >> Cc: Abel Vesa <abel.vesa@nxp.com>
> >> Cc: Adam Ford <aford173@gmail.com>
> >> Cc: Fabio Estevam <festevam@gmail.com>
> >> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> >> Cc: Peng Fan <peng.fan@nxp.com>
> >> Cc: Stefano Babic <sbabic@denx.de>
> >> Cc: Vagrant Cascadian <vagrant@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.

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

Regards,
Peng.

> 
> --
> Best regards,
> Marek Vasut
Marek Vasut June 24, 2019, 10:06 a.m. UTC | #4
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@denx.de]
>>>> Sent: 2019年6月21日 4:54
>>>> To: u-boot@lists.denx.de
>>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>;
>> Adam
>>>> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;
>> Ludwig
>>>> Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;
>> Stefano
>>>> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
>>>> Cc: Abel Vesa <abel.vesa@nxp.com>
>>>> Cc: Adam Ford <aford173@gmail.com>
>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>> Cc: Vagrant Cascadian <vagrant@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.
Ludwig Zenz June 24, 2019, 10:20 a.m. UTC | #5
Marek Vasut <marex@denx.de> Thursday 20th June 2019 22:55:
> 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@denx.de>
> Cc: Abel Vesa <abel.vesa@nxp.com>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Vagrant Cascadian <vagrant@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;
> +	num_controllers++;
> +
> +	return 0;
> +}
> +
>  static int ehci_usb_probe(struct udevice *dev)
>  {
>  	struct usb_platdata *plat = dev_get_platdata(dev);
> @@ -564,6 +580,7 @@ U_BOOT_DRIVER(usb_mx6) = {
>  	.id	= UCLASS_USB,
>  	.of_match = mx6_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,
> -- 
> 2.20.1
> 
> 

Tested-by: Ludwig Zenz <lzenz@dh-electronics.com>

Thanks,
Regards
Ludwig Zenz
Lukasz Majewski June 24, 2019, 10:26 a.m. UTC | #6
On Mon, 24 Jun 2019 12:06:28 +0200
Marek Vasut <marex@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@denx.de]
> >>>> Sent: 2019年6月21日 4:54
> >>>> To: u-boot@lists.denx.de
> >>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>;  
> >> Adam  
> >>>> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;  
> >> Ludwig  
> >>>> Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;  
> >> Stefano  
> >>>> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
> >>>> Cc: Abel Vesa <abel.vesa@nxp.com>
> >>>> Cc: Adam Ford <aford173@gmail.com>
> >>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> >>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>> Cc: Stefano Babic <sbabic@denx.de>
> >>>> Cc: Vagrant Cascadian <vagrant@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 ?

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.



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@denx.de
Marek Vasut June 24, 2019, 10:45 a.m. UTC | #7
On 6/24/19 12:26 PM, Lukasz Majewski wrote:
> On Mon, 24 Jun 2019 12:06:28 +0200
> Marek Vasut <marex@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@denx.de]
>>>>>> Sent: 2019年6月21日 4:54
>>>>>> To: u-boot@lists.denx.de
>>>>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa <abel.vesa@nxp.com>;  
>>>> Adam  
>>>>>> Ford <aford173@gmail.com>; Fabio Estevam <festevam@gmail.com>;  
>>>> Ludwig  
>>>>>> Zenz <lzenz@dh-electronics.com>; Peng Fan <peng.fan@nxp.com>;  
>>>> Stefano  
>>>>>> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
>>>>>> Cc: Abel Vesa <abel.vesa@nxp.com>
>>>>>> Cc: Adam Ford <aford173@gmail.com>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>>> Cc: Vagrant Cascadian <vagrant@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.
Lukasz Majewski June 24, 2019, 11:57 a.m. UTC | #8
Hi Marek,

> On 6/24/19 12:26 PM, Lukasz Majewski wrote:
> > On Mon, 24 Jun 2019 12:06:28 +0200
> > Marek Vasut <marex@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@denx.de]
> >>>>>> Sent: 2019年6月21日 4:54
> >>>>>> To: u-boot@lists.denx.de
> >>>>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa
> >>>>>> <abel.vesa@nxp.com>;    
> >>>> Adam    
> >>>>>> Ford <aford173@gmail.com>; Fabio Estevam
> >>>>>> <festevam@gmail.com>;    
> >>>> Ludwig    
> >>>>>> Zenz <lzenz@dh-electronics.com>; Peng Fan
> >>>>>> <peng.fan@nxp.com>;    
> >>>> Stefano    
> >>>>>> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
> >>>>>> Cc: Abel Vesa <abel.vesa@nxp.com>
> >>>>>> Cc: Adam Ford <aford173@gmail.com>
> >>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> >>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>> Cc: Stefano Babic <sbabic@denx.de>
> >>>>>> Cc: Vagrant Cascadian <vagrant@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@2184200:


Working one:

=> usb start
starting USB...
Bus usb@2184200: USB EHCI 1.00
scanning bus usb@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@denx.de
Marek Vasut June 24, 2019, 12:11 p.m. UTC | #9
On 6/24/19 1:57 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/24/19 12:26 PM, Lukasz Majewski wrote:
>>> On Mon, 24 Jun 2019 12:06:28 +0200
>>> Marek Vasut <marex@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@denx.de]
>>>>>>>> Sent: 2019年6月21日 4:54
>>>>>>>> To: u-boot@lists.denx.de
>>>>>>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa
>>>>>>>> <abel.vesa@nxp.com>;    
>>>>>> Adam    
>>>>>>>> Ford <aford173@gmail.com>; Fabio Estevam
>>>>>>>> <festevam@gmail.com>;    
>>>>>> Ludwig    
>>>>>>>> Zenz <lzenz@dh-electronics.com>; Peng Fan
>>>>>>>> <peng.fan@nxp.com>;    
>>>>>> Stefano    
>>>>>>>> Babic <sbabic@denx.de>; Vagrant Cascadian <vagrant@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@denx.de>
>>>>>>>> Cc: Abel Vesa <abel.vesa@nxp.com>
>>>>>>>> Cc: Adam Ford <aford173@gmail.com>
>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>>>>> Cc: Vagrant Cascadian <vagrant@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 the device sequence number is consistently 0, right ?

>> 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@2184200:
> 
> 
> Working one:
> 
> => usb start
> starting USB...
> Bus usb@2184200: USB EHCI 1.00
> scanning bus usb@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.

I wonder how you managed to trigger this on , presumably , kp_imx6q_tpc
. When I build this target on 77f6e2dd0551d8a825bab391a1bd6b838874bcd4 ,
I get:

===================== WARNING ======================
This board does not use CONFIG_DM_USB. Please update
the board to use CONFIG_DM_USB before the v2019.07 release.
Failure to update by the deadline may result in board removal.
See doc/driver-model/MIGRATION.txt for more info.
====================================================

So the code in this patch doesn't even get compiled.

I presume you have some additional extra patches on top, right ?

Anyway, I cannot debug a board I don't have. Take a look at
ehci_usb_probe() priv->portnr before and after this patch, does it
change ? That's the value from which the PHY index is derived ; if the
PHY index does not match the controller index, the probe hangs, I think
that's what you're hitting.
Lukasz Majewski June 24, 2019, 1:09 p.m. UTC | #10
Hi Marek,

> On 6/24/19 1:57 PM, Lukasz Majewski wrote:
> > Hi Marek,
> >   
> >> On 6/24/19 12:26 PM, Lukasz Majewski wrote:  
> >>> On Mon, 24 Jun 2019 12:06:28 +0200
> >>> Marek Vasut <marex@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@denx.de]
> >>>>>>>> Sent: 2019年6月21日 4:54
> >>>>>>>> To: u-boot@lists.denx.de
> >>>>>>>> Cc: Marek Vasut <marex@denx.de>; Abel Vesa
> >>>>>>>> <abel.vesa@nxp.com>;      
> >>>>>> Adam      
> >>>>>>>> Ford <aford173@gmail.com>; Fabio Estevam
> >>>>>>>> <festevam@gmail.com>;      
> >>>>>> Ludwig      
> >>>>>>>> Zenz <lzenz@dh-electronics.com>; Peng Fan
> >>>>>>>> <peng.fan@nxp.com>;      
> >>>>>> Stefano      
> >>>>>>>> Babic <sbabic@denx.de>; Vagrant Cascadian
> >>>>>>>> <vagrant@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@denx.de>
> >>>>>>>> Cc: Abel Vesa <abel.vesa@nxp.com>
> >>>>>>>> Cc: Adam Ford <aford173@gmail.com>
> >>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>>>> Cc: Ludwig Zenz <lzenz@dh-electronics.com>
> >>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>>>> Cc: Stefano Babic <sbabic@denx.de>
> >>>>>>>> Cc: Vagrant Cascadian <vagrant@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 the device sequence number is consistently 0, right ?

I've just tested it, without delving into details.

> 
> >> 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@2184200:
> > 
> > 
> > Working one:
> >   
> > => usb start  
> > starting USB...
> > Bus usb@2184200: USB EHCI 1.00
> > scanning bus usb@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.  
> 
> I wonder how you managed to trigger this on , presumably ,
> kp_imx6q_tpc . When I build this target on
> 77f6e2dd0551d8a825bab391a1bd6b838874bcd4 , I get:
> 
> ===================== WARNING ======================
> This board does not use CONFIG_DM_USB. Please update
> the board to use CONFIG_DM_USB before the v2019.07 release.
> Failure to update by the deadline may result in board removal.
> See doc/driver-model/MIGRATION.txt for more info.
> ====================================================
> 
> So the code in this patch doesn't even get compiled.

The board is being converted.

> 
> I presume you have some additional extra patches on top, right ?

Yes, the conversion patches. 

> 
> Anyway, I cannot debug a board I don't have. Take a look at
> ehci_usb_probe() priv->portnr before and after this patch, does it
> change ? That's the value from which the PHY index is derived ; if the
> PHY index does not match the controller index, the probe hangs, I
> think that's what you're hitting.

Thanks for the suggestion.

> 


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@denx.de
diff mbox series

Patch

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;
+	num_controllers++;
+
+	return 0;
+}
+
 static int ehci_usb_probe(struct udevice *dev)
 {
 	struct usb_platdata *plat = dev_get_platdata(dev);
@@ -564,6 +580,7 @@  U_BOOT_DRIVER(usb_mx6) = {
 	.id	= UCLASS_USB,
 	.of_match = mx6_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,