[U-Boot] [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; + 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,
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
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 ? ;-)
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
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.
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 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;
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
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 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;
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.
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
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.
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
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
participants (4)
-
Ludwig Zenz -
Lukasz Majewski -
Marek Vasut -
Peng Fan