| Message ID | 20210411162901.7238-24-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | d08cdc223db1023ebb8c03d6341fdf45b303700c |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [V2,01/24] phy: nop-phy: Add standard usb-nop-xceiv compat string | expand |
On Sun, Apr 11, 2021 at 9:35 AM Marek Vasut <marex@denx.de> wrote: > > Enable USB host support on MX8MM Verdin. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Cc: Max Krummenacher <max.krummenacher@toradex.com> > Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > --- > V2: No change > --- > configs/verdin-imx8mm_defconfig | 8 +++++++- > include/configs/verdin-imx8mm.h | 5 +++++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/configs/verdin-imx8mm_defconfig b/configs/verdin-imx8mm_defconfig > index ea0b5978f1..c8c3420b6a 100644 > --- a/configs/verdin-imx8mm_defconfig > +++ b/configs/verdin-imx8mm_defconfig > @@ -37,7 +37,6 @@ CONFIG_SPL_BOARD_INIT=y > CONFIG_SPL_SEPARATE_BSS=y > CONFIG_SPL_I2C_SUPPORT=y > CONFIG_SPL_POWER_SUPPORT=y > -CONFIG_SPL_USB_HOST_SUPPORT=y > CONFIG_SPL_WATCHDOG_SUPPORT=y > CONFIG_SYS_PROMPT="Verdin iMX8MM # " > # CONFIG_BOOTM_NETBSD is not set > @@ -50,6 +49,7 @@ CONFIG_CMD_FUSE=y > CONFIG_CMD_GPIO=y > CONFIG_CMD_I2C=y > CONFIG_CMD_MMC=y > +CONFIG_CMD_USB=y > CONFIG_CMD_CACHE=y > CONFIG_CMD_UUID=y > CONFIG_CMD_REGULATOR=y > @@ -89,6 +89,8 @@ CONFIG_MII=y > CONFIG_PINCTRL=y > CONFIG_SPL_PINCTRL=y > CONFIG_PINCTRL_IMX8M=y > +CONFIG_POWER_DOMAIN=y > +CONFIG_IMX8M_POWER_DOMAIN=y > CONFIG_DM_PMIC=y > CONFIG_SPL_DM_PMIC_PCA9450=y > CONFIG_DM_PMIC_PFUZE100=y > @@ -101,5 +103,9 @@ CONFIG_SPL_SYSRESET=y > CONFIG_SYSRESET_PSCI=y > CONFIG_SYSRESET_WATCHDOG=y > CONFIG_DM_THERMAL=y > +CONFIG_USB=y > +CONFIG_DM_USB=y > +# CONFIG_SPL_DM_USB is not set > +CONFIG_USB_EHCI_HCD=y > CONFIG_IMX_WATCHDOG=y > CONFIG_OF_LIBFDT_OVERLAY=y > diff --git a/include/configs/verdin-imx8mm.h b/include/configs/verdin-imx8mm.h > index 4751bf5a5a..e2a817891c 100644 > --- a/include/configs/verdin-imx8mm.h > +++ b/include/configs/verdin-imx8mm.h > @@ -117,5 +117,10 @@ > #define FEC_QUIRK_ENET_MAC > #define IMX_FEC_BASE 0x30BE0000 > > +/* 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. Best regards, Tim
Hi Marek, I have the same issue on my iMX8MM based board. Are you continue working on the patch for iMX8MM or should I dig into this problem? I'm asking this just for not duplicating efforts. So if you are not working on this then I'll see if I can properly retrieve the necessary patches to make this work. I think something is missing from previous patch set https://lists.denx.de/pipermail/u-boot/2020-September/426757.html Yours, Paul Tim Harvey 於 2021/4/20 上午8:33 寫道: > On Sun, Apr 11, 2021 at 9:35 AM Marek Vasut <marex@denx.de> wrote: >> Enable USB host support on MX8MM Verdin. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com> >> Cc: Max Krummenacher <max.krummenacher@toradex.com> >> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> >> --- >> V2: No change >> --- >> configs/verdin-imx8mm_defconfig | 8 +++++++- >> include/configs/verdin-imx8mm.h | 5 +++++ >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/configs/verdin-imx8mm_defconfig b/configs/verdin-imx8mm_defconfig >> index ea0b5978f1..c8c3420b6a 100644 >> --- a/configs/verdin-imx8mm_defconfig >> +++ b/configs/verdin-imx8mm_defconfig >> @@ -37,7 +37,6 @@ CONFIG_SPL_BOARD_INIT=y >> CONFIG_SPL_SEPARATE_BSS=y >> CONFIG_SPL_I2C_SUPPORT=y >> CONFIG_SPL_POWER_SUPPORT=y >> -CONFIG_SPL_USB_HOST_SUPPORT=y >> CONFIG_SPL_WATCHDOG_SUPPORT=y >> CONFIG_SYS_PROMPT="Verdin iMX8MM # " >> # CONFIG_BOOTM_NETBSD is not set >> @@ -50,6 +49,7 @@ CONFIG_CMD_FUSE=y >> CONFIG_CMD_GPIO=y >> CONFIG_CMD_I2C=y >> CONFIG_CMD_MMC=y >> +CONFIG_CMD_USB=y >> CONFIG_CMD_CACHE=y >> CONFIG_CMD_UUID=y >> CONFIG_CMD_REGULATOR=y >> @@ -89,6 +89,8 @@ CONFIG_MII=y >> CONFIG_PINCTRL=y >> CONFIG_SPL_PINCTRL=y >> CONFIG_PINCTRL_IMX8M=y >> +CONFIG_POWER_DOMAIN=y >> +CONFIG_IMX8M_POWER_DOMAIN=y >> CONFIG_DM_PMIC=y >> CONFIG_SPL_DM_PMIC_PCA9450=y >> CONFIG_DM_PMIC_PFUZE100=y >> @@ -101,5 +103,9 @@ CONFIG_SPL_SYSRESET=y >> CONFIG_SYSRESET_PSCI=y >> CONFIG_SYSRESET_WATCHDOG=y >> CONFIG_DM_THERMAL=y >> +CONFIG_USB=y >> +CONFIG_DM_USB=y >> +# CONFIG_SPL_DM_USB is not set >> +CONFIG_USB_EHCI_HCD=y >> CONFIG_IMX_WATCHDOG=y >> CONFIG_OF_LIBFDT_OVERLAY=y >> diff --git a/include/configs/verdin-imx8mm.h b/include/configs/verdin-imx8mm.h >> index 4751bf5a5a..e2a817891c 100644 >> --- a/include/configs/verdin-imx8mm.h >> +++ b/include/configs/verdin-imx8mm.h >> @@ -117,5 +117,10 @@ >> #define FEC_QUIRK_ENET_MAC >> #define IMX_FEC_BASE 0x30BE0000 >> >> +/* 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. > > Best regards, > > Tim
On Thu, Apr 22, 2021 at 3:34 AM Ying-Chun Liu (PaulLiu) <paulliu@debian.org> wrote: > > Hi Marek, > > I have the same issue on my iMX8MM based board. > Are you continue working on the patch for iMX8MM or should I dig into > this problem? > > I'm asking this just for not duplicating efforts. > So if you are not working on this then I'll see if I can properly > retrieve the necessary patches to make this work. > > > I think something is missing from previous patch set > > https://lists.denx.de/pipermail/u-boot/2020-September/426757.html > > Paul, It's not something missing from Peng's previous patchset since that didn't support DM. Marek added the dm bits but it requires some dt changes that are still not present to make mx6_parse_dt_addrs work (or some changes are needed to that func). The way mx6_parse_dt_addrs is written you need to have a 'usb0' alias for example that points to the phy node (compatible 'fsl,usbmisc') but that is not what has typically been done for USB as typically the alias points to the host controller instead. I'm still interested in what Marek tested with and also think there's a dt patch missing. The aliases should be added to imx8mm.dtsi, but I also think they should point to the controller and not the phy so there may be a code change needed as well. Also, I wasn't aware that the power-domain dt bindings were accepted upstream to Linux as there has been quite a bit of controversy there. cc'ing some others that are probably interested in mainline USB on IMX8M and likely know what's going on with IMX8M power domain. I regret missing this patchset when it was submitted or I would have asked these questions then... I just can't keep up with Linux and U-Boot mailing lists with my work-load in other areas. Best regards, Tim
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.
On Thu, Apr 22, 2021 at 12:16 PM Marek Vasut <marex@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. 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@32e40000: ehci_usb_phy_mode usb@32e40000 usb@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@32e50000: imx8m_power_domain_probe gpc@303a0000 imx8m_power_domain_probe pgc ^^^ why did power domain get probed on the 2nd bus and not the first? imx8m_power_domain_probe power-domain@0 imx8m_power_domain_probe power-domain@3 imx8m_power_domain_on power-domain@3 imx8m_power_domain_on power-domain@0 ehci_usb_probe usb@32e50000 USB EHCI 1.00 usb@32e50000 probe ret=0 scanning bus usb@32e50000 for devices... imx8m_power_domain_on power-domain@3 imx8m_power_domain_on power-domain@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. 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? 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? Best regards, Tim
On 4/23/21 5:04 PM, Tim Harvey wrote: > On Thu, Apr 22, 2021 at 12:16 PM Marek Vasut <marex@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@32e40000: ehci_usb_phy_mode usb@32e40000 > usb@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@32e50000: imx8m_power_domain_probe gpc@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@0 > imx8m_power_domain_probe power-domain@3 > imx8m_power_domain_on power-domain@3 > imx8m_power_domain_on power-domain@0 > ehci_usb_probe usb@32e50000 > USB EHCI 1.00 > usb@32e50000 probe ret=0 > scanning bus usb@32e50000 for devices... imx8m_power_domain_on power-domain@3 > imx8m_power_domain_on power-domain@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.
On Fri, Apr 23, 2021 at 8:21 AM Marek Vasut <marex@denx.de> wrote: > > On 4/23/21 5:04 PM, Tim Harvey wrote: > > On Thu, Apr 22, 2021 at 12:16 PM Marek Vasut <marex@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@32e40000: ehci_usb_phy_mode usb@32e40000 > > usb@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@32e50000: imx8m_power_domain_probe gpc@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 ? Marek, The reg domain does not get enabled for usbotg1 because device_of_to_plat gets called 'before' dev_power_domain_on in device_probe. The following will get imx8mm USB otg working: For OTG defer setting type until probe after clock and power have been brought up. index 06be9deaaa..2183ae4f9d 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -523,7 +523,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); @@ -555,7 +555,10 @@ static int ehci_usb_of_to_plat(struct udevice *dev) break; case USB_DR_MODE_OTG: case USB_DR_MODE_UNKNOWN: - return ehci_usb_phy_mode(dev); + if (is_imx8mm()) + plat->init_type = USB_INIT_HOST; + else + return ehci_usb_phy_mode(dev); }; return 0; @@ -657,6 +660,13 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(1); #endif + if (is_imx8mm() && (usb_get_dr_mode(dev_ofnode(dev)) == USB_DR_MODE_OTG)) { + ret = ehci_usb_phy_mode(dev); + if (ret) + return ret; + priv->init_type = plat->init_type; + }; + #if CONFIG_IS_ENABLED(DM_REGULATOR) ret = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); > > > imx8m_power_domain_probe power-domain@0 > > imx8m_power_domain_probe power-domain@3 > > imx8m_power_domain_on power-domain@3 > > imx8m_power_domain_on power-domain@0 > > ehci_usb_probe usb@32e50000 > > USB EHCI 1.00 > > usb@32e50000 probe ret=0 > > scanning bus usb@32e50000 for devices... imx8m_power_domain_on power-domain@3 > > imx8m_power_domain_on power-domain@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.
On 4/27/21 2:01 AM, Tim Harvey wrote: [...] >>> 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@32e40000: ehci_usb_phy_mode usb@32e40000 >>> usb@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@32e50000: imx8m_power_domain_probe gpc@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 ? > > Marek, > > The reg domain does not get enabled for usbotg1 because > device_of_to_plat gets called 'before' dev_power_domain_on in > device_probe. > > The following will get imx8mm USB otg working: > > For OTG defer setting type until probe after clock and power have been > brought up. > index 06be9deaaa..2183ae4f9d 100644 > --- a/drivers/usb/host/ehci-mx6.c > +++ b/drivers/usb/host/ehci-mx6.c > @@ -523,7 +523,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); > @@ -555,7 +555,10 @@ static int ehci_usb_of_to_plat(struct udevice *dev) > break; > case USB_DR_MODE_OTG: > case USB_DR_MODE_UNKNOWN: > - return ehci_usb_phy_mode(dev); > + if (is_imx8mm()) Does this mean OTG doesn't work on the 8MM then ? > + plat->init_type = USB_INIT_HOST; > + else > + return ehci_usb_phy_mode(dev); > }; > > return 0; > @@ -657,6 +660,13 @@ static int ehci_usb_probe(struct udevice *dev) > mdelay(1); > #endif > > + if (is_imx8mm() && (usb_get_dr_mode(dev_ofnode(dev)) == > USB_DR_MODE_OTG)) { > + ret = ehci_usb_phy_mode(dev); > + if (ret) > + return ret; > + priv->init_type = plat->init_type; > + }; I have to wonder, why not move the whole OTG/Host/Device detection to probe then ? Also, could you submit a regular patch ?
On Mon, Apr 26, 2021, 5:35 PM Marek Vasut <marex@denx.de> wrote: > > On 4/27/21 2:01 AM, Tim Harvey wrote: > [...] > >>> 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@32e40000: ehci_usb_phy_mode usb@32e40000 > >>> usb@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@32e50000: imx8m_power_domain_probe gpc@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 ? > > > > Marek, > > > > The reg domain does not get enabled for usbotg1 because > > device_of_to_plat gets called 'before' dev_power_domain_on in > > device_probe. > > > > The following will get imx8mm USB otg working: > > > > For OTG defer setting type until probe after clock and power have been > > brought up. > > index 06be9deaaa..2183ae4f9d 100644 > > --- a/drivers/usb/host/ehci-mx6.c > > +++ b/drivers/usb/host/ehci-mx6.c > > @@ -523,7 +523,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); > > @@ -555,7 +555,10 @@ static int ehci_usb_of_to_plat(struct udevice *dev) > > break; > > case USB_DR_MODE_OTG: > > case USB_DR_MODE_UNKNOWN: > > - return ehci_usb_phy_mode(dev); > > + if (is_imx8mm()) > > Does this mean OTG doesn't work on the 8MM then ? IMX8MM USB in general still doesn't work without your: usb: ehci-mx6: Limit PHY address parsing to !CONFIG_PHY With your patch, IMX8MM 'host' works but 'otg' will fail probe with -22 (due ehci_usb_phy_mode called from of_to_plat and it not having a case for imx8mm) > > > + plat->init_type = USB_INIT_HOST; > > + else > > + return ehci_usb_phy_mode(dev); > > }; > > > > return 0; > > @@ -657,6 +660,13 @@ static int ehci_usb_probe(struct udevice *dev) > > mdelay(1); > > #endif > > > > + if (is_imx8mm() && (usb_get_dr_mode(dev_ofnode(dev)) == > > USB_DR_MODE_OTG)) { > > + ret = ehci_usb_phy_mode(dev); > > + if (ret) > > + return ret; > > + priv->init_type = plat->init_type; > > + }; > > I have to wonder, why not move the whole OTG/Host/Device detection to > probe then ? Yes, I think that is the right thing to do. > > Also, could you submit a regular patch ? Yes, I will post patches to fix IMX8MM OTG. Can you submit your 'usb: ehci-mx6: Limit PHY address parsing to !CONFIG_PHY' patch so I can go on top of that or can I just pull that into my series? Best regards, Tim
On Tue, Apr 27, 2021 at 10:50 AM Tim Harvey <tharvey@gateworks.com> wrote: > > On Mon, Apr 26, 2021, 5:35 PM Marek Vasut <marex@denx.de> wrote: > > > > On 4/27/21 2:01 AM, Tim Harvey wrote: > > [...] > > >>> 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@32e40000: ehci_usb_phy_mode usb@32e40000 > > >>> usb@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@32e50000: imx8m_power_domain_probe gpc@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 ? > > > > > > Marek, > > > > > > The reg domain does not get enabled for usbotg1 because > > > device_of_to_plat gets called 'before' dev_power_domain_on in > > > device_probe. > > > > > > The following will get imx8mm USB otg working: > > > > > > For OTG defer setting type until probe after clock and power have been > > > brought up. > > > index 06be9deaaa..2183ae4f9d 100644 > > > --- a/drivers/usb/host/ehci-mx6.c > > > +++ b/drivers/usb/host/ehci-mx6.c > > > @@ -523,7 +523,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); > > > @@ -555,7 +555,10 @@ static int ehci_usb_of_to_plat(struct udevice *dev) > > > break; > > > case USB_DR_MODE_OTG: > > > case USB_DR_MODE_UNKNOWN: > > > - return ehci_usb_phy_mode(dev); > > > + if (is_imx8mm()) > > > > Does this mean OTG doesn't work on the 8MM then ? > > IMX8MM USB in general still doesn't work without your: > usb: ehci-mx6: Limit PHY address parsing to !CONFIG_PHY > > With your patch, IMX8MM 'host' works but 'otg' will fail probe with > -22 (due ehci_usb_phy_mode called from of_to_plat and it not having a > case for imx8mm) > > > > > > + plat->init_type = USB_INIT_HOST; > > > + else > > > + return ehci_usb_phy_mode(dev); > > > }; > > > > > > return 0; > > > @@ -657,6 +660,13 @@ static int ehci_usb_probe(struct udevice *dev) > > > mdelay(1); > > > #endif > > > > > > + if (is_imx8mm() && (usb_get_dr_mode(dev_ofnode(dev)) == > > > USB_DR_MODE_OTG)) { > > > + ret = ehci_usb_phy_mode(dev); > > > + if (ret) > > > + return ret; > > > + priv->init_type = plat->init_type; > > > + }; > > > > I have to wonder, why not move the whole OTG/Host/Device detection to > > probe then ? > > Yes, I think that is the right thing to do. > > > > > Also, could you submit a regular patch ? > > Yes, I will post patches to fix IMX8MM OTG. Can you submit your 'usb: > ehci-mx6: Limit PHY address parsing to !CONFIG_PHY' patch so I can go > on top of that or can I just pull that into my series? If you want me to test it on either a Mini or Nano, feel free to CC me as well. I was out of town the last week, so I wasn't in a place to do any work. I am a little behind, so I might need some pointers to prerequisite patches if they're necessary. thanks, adam > > Best regards, > > Tim
diff --git a/configs/verdin-imx8mm_defconfig b/configs/verdin-imx8mm_defconfig index ea0b5978f1..c8c3420b6a 100644 --- a/configs/verdin-imx8mm_defconfig +++ b/configs/verdin-imx8mm_defconfig @@ -37,7 +37,6 @@ CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_SEPARATE_BSS=y CONFIG_SPL_I2C_SUPPORT=y CONFIG_SPL_POWER_SUPPORT=y -CONFIG_SPL_USB_HOST_SUPPORT=y CONFIG_SPL_WATCHDOG_SUPPORT=y CONFIG_SYS_PROMPT="Verdin iMX8MM # " # CONFIG_BOOTM_NETBSD is not set @@ -50,6 +49,7 @@ CONFIG_CMD_FUSE=y CONFIG_CMD_GPIO=y CONFIG_CMD_I2C=y CONFIG_CMD_MMC=y +CONFIG_CMD_USB=y CONFIG_CMD_CACHE=y CONFIG_CMD_UUID=y CONFIG_CMD_REGULATOR=y @@ -89,6 +89,8 @@ CONFIG_MII=y CONFIG_PINCTRL=y CONFIG_SPL_PINCTRL=y CONFIG_PINCTRL_IMX8M=y +CONFIG_POWER_DOMAIN=y +CONFIG_IMX8M_POWER_DOMAIN=y CONFIG_DM_PMIC=y CONFIG_SPL_DM_PMIC_PCA9450=y CONFIG_DM_PMIC_PFUZE100=y @@ -101,5 +103,9 @@ CONFIG_SPL_SYSRESET=y CONFIG_SYSRESET_PSCI=y CONFIG_SYSRESET_WATCHDOG=y CONFIG_DM_THERMAL=y +CONFIG_USB=y +CONFIG_DM_USB=y +# CONFIG_SPL_DM_USB is not set +CONFIG_USB_EHCI_HCD=y CONFIG_IMX_WATCHDOG=y CONFIG_OF_LIBFDT_OVERLAY=y diff --git a/include/configs/verdin-imx8mm.h b/include/configs/verdin-imx8mm.h index 4751bf5a5a..e2a817891c 100644 --- a/include/configs/verdin-imx8mm.h +++ b/include/configs/verdin-imx8mm.h @@ -117,5 +117,10 @@ #define FEC_QUIRK_ENET_MAC #define IMX_FEC_BASE 0x30BE0000 +/* 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 */
Enable USB host support on MX8MM Verdin. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com> Cc: Max Krummenacher <max.krummenacher@toradex.com> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- V2: No change --- configs/verdin-imx8mm_defconfig | 8 +++++++- include/configs/verdin-imx8mm.h | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-)