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

[Marek Vasut]

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!