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