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! _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
