[U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
From: Igor Opaniuk igor.opaniuk@toradex.com
This fixes the issues with calculation of controller indexes in ehci_usb_bind() for iMX7, as USB controllers on iMX7 SoCs aren't placed next to each other, and their addresses incremented by 0x1000.
Example of USB nodes for iMX7S/D:
usbotg1: usb@30b10000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; reg = <0x30b10000 0x200>; ^^^^^^^^^^ .... usbotg2: usb@30b20000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; reg = <0x30b20000 0x200>; ^^^^^^^^^^ ....
usbh: usb@30b30000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; reg = <0x30b30000 0x200>; ^^^^^^^^^^ ....
Which was leading to usb enumeration issues: Colibri iMX7 # usb start starting USB... Bus usb@30b10000: USB EHCI 1.00 Bus usb@30b20000: probe failed, error -22 scanning bus usb@30b10000 for devices... 1 USB Device(s) found scanning usb for storage devices... 0 Storage Device(s) found
Fixes: 501547cec1("usb: ehci-mx6: Fix bus enumeration for DM case") Signed-off-by: Igor Opaniuk igor.opaniuk@toradex.com ---
drivers/usb/host/ehci-mx6.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index e9e6ed596d..d60b36d7b5 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -513,10 +513,11 @@ static int ehci_usb_bind(struct udevice *dev) * from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as - * well as we can. The USB controllers on all existing iMX6/iMX7 SoCs - * are placed next to each other, at addresses incremented by 0x200. - * Thus, the index is derived from the multiple of 0x200 offset from - * the first controller address. + * well as we can. The USB controllers on all existing iMX6 SoCs + * are placed next to each other, at addresses incremented by 0x200, + * and iMX7 their addresses are shifted by 0x1000. + * Thus, the index is derived from the multiple of 0x200 (0x1000 for + * iMX7) offset from the first controller address. * * However, to complete conversion of this driver to DT probing, the * following has to be done: @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */ - fdt_size_t size; - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size); +#if defined(CONFIG_MX6) + u32 controller_spacing = 0x200; +#elif defined(CONFIG_MX7) + u32 controller_spacing = 0x10000; +#endif + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);
- dev->req_seq = (addr - USB_BASE_ADDR) / size; + dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
return 0; }
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...]
* from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as
* well as we can. The USB controllers on all existing iMX6/iMX7 SoCs* are placed next to each other, at addresses incremented by 0x200.* Thus, the index is derived from the multiple of 0x200 offset from* the first controller address.
* well as we can. The USB controllers on all existing iMX6 SoCs* are placed next to each other, at addresses incremented by 0x200,* and iMX7 their addresses are shifted by 0x1000.* Thus, the index is derived from the multiple of 0x200 (0x1000 for* iMX7) offset from the first controller address.
- However, to complete conversion of this driver to DT probing, the
- following has to be done:
@@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */
- fdt_size_t size;
- fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size);
+#if defined(CONFIG_MX6)
- u32 controller_spacing = 0x200;
+#elif defined(CONFIG_MX7)
- u32 controller_spacing = 0x10000;
+#endif
- fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);
This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match.
Hi Marek
On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut marex@denx.de wrote:
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...]
* from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as
* well as we can. The USB controllers on all existing iMX6/iMX7 SoCs* are placed next to each other, at addresses incremented by 0x200.* Thus, the index is derived from the multiple of 0x200 offset from* the first controller address.
* well as we can. The USB controllers on all existing iMX6 SoCs* are placed next to each other, at addresses incremented by 0x200,* and iMX7 their addresses are shifted by 0x1000.* Thus, the index is derived from the multiple of 0x200 (0x1000 for* iMX7) offset from the first controller address. * * However, to complete conversion of this driver to DT probing, the * following has to be done:@@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */
fdt_size_t size;fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size);+#if defined(CONFIG_MX6)
u32 controller_spacing = 0x200;+#elif defined(CONFIG_MX7)
u32 controller_spacing = 0x10000;+#endif
fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match.
So in that case existing ehci_hcd_init() implementation won't work either, as it does the same. So if we want to address this case (anyway, is it the real case when both CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement other parts of the driver.
But taking into account that it's a temporary workaround and should be fully reimplemented,why not to proceed with a pretty simple solution for now as what is done in ehci-vf.c :
295 static int vf_usb_bind(struct udevice *dev) 296 { 297 static int num_controllers; 298 299 /* 300 * Without this hack, if we return ENODEV for USB Controller 0, on 301 * probe for the next controller, USB Controller 1 will be given a 302 * sequence number of 0. This conflicts with our requirement of 303 * sequence numbers while initialising the peripherals. 304 */ 305 dev->req_seq = num_controllers; 306 num_controllers++; 307 308 return 0; 309 }
Thanks
On 10/10/19 2:29 PM, Igor Opaniuk wrote:
Hi Marek
Hi Igor,
On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...]
* from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as
* well as we can. The USB controllers on all existing iMX6/iMX7 SoCs* are placed next to each other, at addresses incremented by 0x200.* Thus, the index is derived from the multiple of 0x200 offset from* the first controller address.
* well as we can. The USB controllers on all existing iMX6 SoCs* are placed next to each other, at addresses incremented by 0x200,* and iMX7 their addresses are shifted by 0x1000.* Thus, the index is derived from the multiple of 0x200 (0x1000 for* iMX7) offset from the first controller address. * * However, to complete conversion of this driver to DT probing, the * following has to be done:@@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */
fdt_size_t size;fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size);+#if defined(CONFIG_MX6)
u32 controller_spacing = 0x200;+#elif defined(CONFIG_MX7)
u32 controller_spacing = 0x10000;+#endif
fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match.
So in that case existing ehci_hcd_init() implementation won't work either, as it does the same.
Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM code and probes from information in DT (or platdata), hence no such ifdeffery is allowed.
So if we want to address this case (anyway, is it the real case when both CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement other parts of the driver.
But taking into account that it's a temporary workaround and should be fully reimplemented,why not to proceed with a pretty simple solution for now as what is done in ehci-vf.c :
Because this does not work if ehci0 is not your first controller, then the derivation of ANATOP bit offsets breaks down because the controller indexes are off. That's what the original patch was trying to fix and what is described in that long explanation text in the driver too. I was trying to make sure this problem is documented.
I think ehci-vf and ehci-mx5 have the same problem and need to be fixed too. But the real fix is to finish converting the driver to DM proper and fix that ANATOP bit offset derivation properly. Or even pull it out of DT, which would be the best.
Anyway, what about this:
u32 controller_spacing = is_mx7() ? 0x1000 : 0x200; fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
This gets rid of the ifdeffery and works on both mx6 and mx7, no ?
295 static int vf_usb_bind(struct udevice *dev) 296 { 297 static int num_controllers; 298 299 /* 300 * Without this hack, if we return ENODEV for USB Controller 0, on 301 * probe for the next controller, USB Controller 1 will be given a 302 * sequence number of 0. This conflicts with our requirement of 303 * sequence numbers while initialising the peripherals. 304 */ 305 dev->req_seq = num_controllers; 306 num_controllers++; 307 308 return 0; 309 }
Thanks
On Thu, Oct 10, 2019 at 3:43 PM Marek Vasut marex@denx.de wrote:
On 10/10/19 2:29 PM, Igor Opaniuk wrote:
Hi Marek
Hi Igor,
On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...]
* from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as
* well as we can. The USB controllers on all existing iMX6/iMX7 SoCs* are placed next to each other, at addresses incremented by 0x200.* Thus, the index is derived from the multiple of 0x200 offset from* the first controller address.
* well as we can. The USB controllers on all existing iMX6 SoCs* are placed next to each other, at addresses incremented by 0x200,* and iMX7 their addresses are shifted by 0x1000.* Thus, the index is derived from the multiple of 0x200 (0x1000 for* iMX7) offset from the first controller address. * * However, to complete conversion of this driver to DT probing, the * following has to be done:@@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */
fdt_size_t size;fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size);+#if defined(CONFIG_MX6)
u32 controller_spacing = 0x200;+#elif defined(CONFIG_MX7)
u32 controller_spacing = 0x10000;+#endif
fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match.
So in that case existing ehci_hcd_init() implementation won't work either, as it does the same.
Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM code and probes from information in DT (or platdata), hence no such ifdeffery is allowed.
So if we want to address this case (anyway, is it the real case when both CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement other parts of the driver.
But taking into account that it's a temporary workaround and should be fully reimplemented,why not to proceed with a pretty simple solution for now as what is done in ehci-vf.c :
Because this does not work if ehci0 is not your first controller, then the derivation of ANATOP bit offsets breaks down because the controller indexes are off. That's what the original patch was trying to fix and what is described in that long explanation text in the driver too. I was trying to make sure this problem is documented.
I think ehci-vf and ehci-mx5 have the same problem and need to be fixed too. But the real fix is to finish converting the driver to DM proper and fix that ANATOP bit offset derivation properly. Or even pull it out of DT, which would be the best.
Thanks for the explanation!
Anyway, what about this:
u32 controller_spacing = is_mx7() ? 0x1000 : 0x200; fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
This gets rid of the ifdeffery and works on both mx6 and mx7, no ?
Makes sense, will change it in v2.
295 static int vf_usb_bind(struct udevice *dev) 296 { 297 static int num_controllers; 298 299 /* 300 * Without this hack, if we return ENODEV for USB Controller 0, on 301 * probe for the next controller, USB Controller 1 will be given a 302 * sequence number of 0. This conflicts with our requirement of 303 * sequence numbers while initialising the peripherals. 304 */ 305 dev->req_seq = num_controllers; 306 num_controllers++; 307 308 return 0; 309 }
Thanks
-- Best regards, Marek Vasut
Thanks
On 10/10/19 2:55 PM, Igor Opaniuk wrote:
On Thu, Oct 10, 2019 at 3:43 PM Marek Vasut wrote:
On 10/10/19 2:29 PM, Igor Opaniuk wrote:
Hi Marek
Hi Igor,
On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...]
* from which it derives offsets in the PHY and ANATOP register sets. * * Here we attempt to calculate these indexes from DT information as
* well as we can. The USB controllers on all existing iMX6/iMX7 SoCs* are placed next to each other, at addresses incremented by 0x200.* Thus, the index is derived from the multiple of 0x200 offset from* the first controller address.
* well as we can. The USB controllers on all existing iMX6 SoCs* are placed next to each other, at addresses incremented by 0x200,* and iMX7 their addresses are shifted by 0x1000.* Thus, the index is derived from the multiple of 0x200 (0x1000 for* iMX7) offset from the first controller address. * * However, to complete conversion of this driver to DT probing, the * following has to be done:@@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) * With these changes in place, the ad-hoc indexing goes away and * the driver is fully converted to DT probing. */
fdt_size_t size;fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, &size);+#if defined(CONFIG_MX6)
u32 controller_spacing = 0x200;+#elif defined(CONFIG_MX7)
u32 controller_spacing = 0x10000;+#endif
fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match.
So in that case existing ehci_hcd_init() implementation won't work either, as it does the same.
Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM code and probes from information in DT (or platdata), hence no such ifdeffery is allowed.
So if we want to address this case (anyway, is it the real case when both CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement other parts of the driver.
But taking into account that it's a temporary workaround and should be fully reimplemented,why not to proceed with a pretty simple solution for now as what is done in ehci-vf.c :
Because this does not work if ehci0 is not your first controller, then the derivation of ANATOP bit offsets breaks down because the controller indexes are off. That's what the original patch was trying to fix and what is described in that long explanation text in the driver too. I was trying to make sure this problem is documented.
I think ehci-vf and ehci-mx5 have the same problem and need to be fixed too. But the real fix is to finish converting the driver to DM proper and fix that ANATOP bit offset derivation properly. Or even pull it out of DT, which would be the best.
Thanks for the explanation!
Anyway, what about this:
u32 controller_spacing = is_mx7() ? 0x1000 : 0x200; fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
This gets rid of the ifdeffery and works on both mx6 and mx7, no ?
Makes sense, will change it in v2.
Great, thanks!
participants (2)
-
Igor Opaniuk -
Marek Vasut