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

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut