diff mbox series

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

Message ID 20190620205358.11534-1-marex@denx.de
State Deferred
Delegated to: Marek Vasut
Headers show
Series [U-Boot] usb: ehci-mx5: 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 iMX5 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 iMX5 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-mx5.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Lukasz Majewski June 24, 2019, 12:09 p.m. UTC | #1
On Thu, 20 Jun 2019 22:53:58 +0200
Marek Vasut <marex@denx.de> wrote:

> It is likely that the DM conversion of EHCI iMX5 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 iMX5 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-mx5.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> 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@denx.de>

Tested on HSC|DDC i.MX53 board (usb start works as previously).

master branch
SHA1:  77f6e2dd0551d8a825bab391a1bd6b838874bcd4

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
Lukasz Majewski Feb. 24, 2020, 3:28 p.m. UTC | #2
Hi Marek,

> On Thu, 20 Jun 2019 22:53:58 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
> > It is likely that the DM conversion of EHCI iMX5 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 iMX5 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-mx5.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > 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@denx.de>
> 
> Tested on HSC|DDC i.MX53 board (usb start works as previously).
> 

Gentle ping on this patch :-)

It must have been overlooked in some way ...

Marek, could you please apply this fix?

Otherwise kp_imx53 boards (and probably some other ones) are broken -
and cannot be easily debricked.

> master branch
> SHA1:  77f6e2dd0551d8a825bab391a1bd6b838874bcd4

It applies also on top of newest -master:

SHA1: 8e51bf746a11d7f67416859da73a83109af4e0a3

Thanks in advance.

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


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 Feb. 25, 2020, 6:01 p.m. UTC | #3
On 2/24/20 4:28 PM, Lukasz Majewski wrote:
> Hi Marek,

Hi,

>> On Thu, 20 Jun 2019 22:53:58 +0200
>> Marek Vasut <marex@denx.de> wrote:
>>
>>> It is likely that the DM conversion of EHCI iMX5 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 iMX5 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-mx5.c | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> 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@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, or even better,
solve the TODO there altogether.

[...]
Lukasz Majewski Feb. 26, 2020, 9:16 a.m. UTC | #4
Dear Marek,

> On 2/24/20 4:28 PM, Lukasz Majewski wrote:
> > Hi Marek,  
> 
> Hi,
> 
> >> On Thu, 20 Jun 2019 22:53:58 +0200
> >> Marek Vasut <marex@denx.de> wrote:
> >>  
> >>> It is likely that the DM conversion of EHCI iMX5 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 iMX5 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-mx5.c | 17 +++++++++++++++++
> >>>  1 file changed, 17 insertions(+)
> >>>
> >>> 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@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. 

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.



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.

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

> , 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?


> 
> [...]




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 Feb. 28, 2020, 11:43 p.m. UTC | #5
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@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.
diff mbox series

Patch

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,