[U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs

[Igor Opaniuk]

On Thu, Oct 10, 2019 at 3:43 PM Marek Vasut <marex at 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

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk