[PATCH V2 24/24] ARM: imx8m: verdin-imx8mm: Enable USB Host support

[Marek Vasut]

On 4/23/21 5:04 PM, Tim Harvey wrote:
> On Thu, Apr 22, 2021 at 12:16 PM Marek Vasut <marex at denx.de> wrote:
>>
>> On 4/20/21 2:33 AM, Tim Harvey wrote:
>>
>> [...]
>>
>>>> +/* USB Configs */
>>>> +#define CONFIG_EHCI_HCD_INIT_AFTER_RESET
>>>> +#define CONFIG_MXC_USB_PORTSC  (PORT_PTS_UTMI | PORT_PTS_PTW)
>>>> +#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
>>>> +
>>>>    #endif /*_VERDIN_IMX8MM_H */
>>>>
>>>> --
>>>> 2.30.2
>>>>
>>>
>>> Marek,
>>>
>>> Thanks for your work on USB support for IMX8M!
>>>
>>> I'm attempting to add USB support to the venice board following this
>>> example but I think there are still some things missing from the dt to
>>> make this work. I find that mx6_parse_dt_adds failes; Looks like it is
>>> required to have an alias that points to the phy but then it fails
>>> because the phy doesn't have a reg. Also, it would see the
>>> CONFIG_MXC_USB_PORTSC is no longer needed as that is now the default.
>>
>> Have a look at this patch:
>>
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/8203ee09275c30766a1fd1cf19c60a904ee72f8c
>>
>> That should fix it for you.
>>
>> It would be good however if you could take the mx6 ventana and test the
>> driver there, and possibly submit fixes in case the USB is still broken
>> there. What would be even better is if you could implement the MXS PHY
>> driver, so all the !CONFIG_PHY stuff can be removed for MX6 too. That
>> shouldn't be much effort.
> 
> Marek,
> 
> That does resolve the issue with mx6_parse_dt_adds failing and works
> for IMX8MM host mode (and likely peripheral mode) but not for otg. For
> otg we also need something like:
> @@ -523,7 +568,7 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>                          plat->init_type = USB_INIT_DEVICE;
>                  else
>                          plat->init_type = USB_INIT_HOST;
> -       } else if (is_mx7()) {
> +       } else if (is_mx7() || is_imx8mm()) {
>                  phy_status = (void __iomem *)(addr +
>                                                USBNC_PHY_STATUS_OFFSET);
>                  val = readl(phy_status);
> 
> but then the issue is we hang due to the otg power domain not being
> enabled when ehci_usb_of_to_plat is called and I'm not quite clear why
> that is.

ehci_usb_phy_mode() is called from ehci_usb_of_to_plat(), which happens 
before probe(), so the power domain at that point is off. You would have 
to move that entire code to probe() to get OTG working properly, without 
the current workaround.

> Why would the power domain get probed/enabled for the usbotg2
> bus but not the usbotg1 bus? Here is some debugging:
> u-boot=> usb start
> starting USB...
> Bus usb at 32e40000: ehci_usb_phy_mode usb at 32e40000
> usb at 32e40000 probe ret=-22
> probe failed, error -22
> ^^^ probe fails here because ehci_usb_phy_mode returns EINVAL for
> dr_mode=otg but if we try to read the phy_status reg we will hang b/c
> power domain is not enabled yet
> Bus usb at 32e50000: imx8m_power_domain_probe gpc at 303a0000
> imx8m_power_domain_probe pgc
> ^^^ why did power domain get probed on the 2nd bus and not the first?

I don't know, can you have a look ?

> imx8m_power_domain_probe power-domain at 0
> imx8m_power_domain_probe power-domain at 3
> imx8m_power_domain_on power-domain at 3
> imx8m_power_domain_on power-domain at 0
> ehci_usb_probe usb at 32e50000
> USB EHCI 1.00
> usb at 32e50000 probe ret=0
> scanning bus usb at 32e50000 for devices... imx8m_power_domain_on power-domain at 3
> imx8m_power_domain_on power-domain at 0
> 3 USB Device(s) found
>         scanning usb for storage devices... 0 Storage Device(s) found
> u-boot=>
> 
> I also verified your patch does not break USB for IMX6Q/DL ventana as well.

That's good, thank you

> I am still a bit concerned that the power-domain patches you merged in
> this series are introducing bindings that are not approved upstream:
> d78f7d8199 ARM: dts: imx8mn: Add power domain nodes
> f0e10e33e5 ARM: dts: imx8mm: Add power domain nodes
> 
> Are you under the impression that is what will go upstream?

Pretty much, yes. And if there are some changes further down the line, 
we can pick those later when they hit upstream. I think this is better 
than having no working USB at all.

> I'm still
> waiting to see what this 'virtual power-domain' patch looks like that
> I believe Abel or Jacky was going to work on. Perhaps you, Adam, or
> Frieder took part in that discussion?

That is still going on, yes.