[U-Boot] [PATCH] usb: ehci-mx5: Fix bus enumeration for DM case
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,
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 Controller0, on
* probe for the next controller, USB Controller 1 will begiven a
* sequence number of 0. This conflicts with our requirementof
* 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
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 USBController 0, on
* probe for the next controller, USB Controller 1 will begiven a
* sequence number of 0. This conflicts with ourrequirement 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
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 USBController 0, on
* probe for the next controller, USB Controller 1 will begiven a
* sequence number of 0. This conflicts with ourrequirement 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.
[...]
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 USBController 0, on
* probe for the next controller, USB Controller 1 willbe given a
* sequence number of 0. This conflicts with ourrequirement 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
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 USBController 0, on
* probe for the next controller, USB Controller 1 willbe given a
* sequence number of 0. This conflicts with ourrequirement 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.
participants (2)
-
Lukasz Majewski -
Marek Vasut