Re: [PATCH 1/3] arm: dts: imx8mm: Sync with Linux 6.3

[Tim Harvey <tharvey@gateworks.com>]

On Fri, Apr 28, 2023 at 9:44 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Fri, Apr 28, 2023 at 11:26 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Tim,
> >
> > On Fri, Apr 28, 2023 at 12:48 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > > Yes I think that is similar enough to test. In my experience simply
> > > enabling otg2 via dt on imx8mm-evk shows the issue I see here but
> > > Fabio says he sees a hang on 'usb start' even before this dt sync and
> > > I don't know why my results on an imx8mm-evk differ.
> >
> > I started from scratch today and now our results match.
> >
> > Applied the following change against U-Boot master:
> >
> > diff --git a/arch/arm/dts/imx8mm-evk.dtsi b/arch/arm/dts/imx8mm-evk.dtsi
> > index 7d6317d95b13..898639e33d5e 100644
> > --- a/arch/arm/dts/imx8mm-evk.dtsi
> > +++ b/arch/arm/dts/imx8mm-evk.dtsi
> > @@ -417,6 +417,10 @@
> >   };
> >  };
> >
> > +&usbotg2 {
> > +      status = "okay";
> > +};
> > +
> >  &usdhc2 {
> >   assigned-clocks = <&clk IMX8MM_CLK_USDHC2>;
> >   assigned-clock-rates = <200000000>;
> > diff --git a/configs/imx8mm_evk_defconfig b/configs/imx8mm_evk_defconfig
> > index ab9ad41b4548..70c7a21f2d9f 100644
> > --- a/configs/imx8mm_evk_defconfig
> > +++ b/configs/imx8mm_evk_defconfig
> > @@ -119,3 +119,4 @@ CONFIG_CI_UDC=y
> >  CONFIG_SDP_LOADADDR=0x40400000
> >  CONFIG_USB_GADGET_DOWNLOAD=y
> >  CONFIG_IMX_WATCHDOG=y
> > +CONFIG_CMD_USB=y
> > --
> > 2.34.1
> >
> > Running "usb start" does not hang.
> >
> > Running "ums 0 mmc 1", CTRL+C and then "ums 0 mmc 1" does not work (SD
> > card is not mounted on PC on the second time).
> >
> > After applying the imx8mm.dtsi sync with kernel 6.3:
> >
> > Running "ums 0 mmc 1", CTRL+C and then "ums 0 mmc 1" works fine.
> >
> > "usb start" hangs.
> >
> > So, yes, I agree we cannot do the imx8mm.dtsi sync with 6.3 right now
> > as we need to fix the USB hang first.
> >
> > If anyone has any ideas as to why syncing the commit below:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64/boot/dts/freescale/imx8mm.dtsi?h=v6.3&id=4585c79ff477f9517b7f384a4fce351417e8fa36
> >
> > causes issues in U-Boot, please let us know.
>
> I am not in a place to test this as I am traveling, but I thought I'd
> throw out an idea.  The power-domain looks like it moved to the
> usbphynop2  driver which has the compatible name of "usb-nop-xceiv"
> Is there a a driver for this?  Does it get enabled?
> If not, maybe we could  update the imx8mm-u-u-boot.dtsi to restore the
> power-domains to a driver that is present.
>

Adam,

Ya, I think you were on the right track here.

There is a driver (driver/phy/nop-phy.c) which does get enabled but
with the dt sync the phy's power domain gets enabled after EHCI
registers are accessed.

I believe the fix we need is the following which moves phy setup
before the register access (where it should have been along with the
case for !defined(CONFIG_PHY):
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 91633f013a55..fae20838c60a 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -703,6 +703,10 @@ static int ehci_usb_probe(struct udevice *dev)
        usb_internal_phy_clock_gate(priv->phy_addr, 1);
        usb_phy_enable(ehci, priv->phy_addr);
 #endif
+#else
+       ret = generic_setup_phy(dev, &priv->phy, 0);
+       if (ret)
+               goto err_regulator;
 #endif

 #if CONFIG_IS_ENABLED(DM_REGULATOR)
@@ -725,12 +729,6 @@ static int ehci_usb_probe(struct udevice *dev)

        mdelay(10);

-#if defined(CONFIG_PHY)
-       ret = generic_setup_phy(dev, &priv->phy, 0);
-       if (ret)
-               goto err_regulator;
-#endif
-
        hccr = (struct ehci_hccr *)((uintptr_t)&ehci->caplength);
        hcor = (struct ehci_hcor *)((uintptr_t)hccr +
                        HC_LENGTH(ehci_readl(&(hccr)->cr_capbase)));


If everyone agrees here I'll submit a formal patch which should get
applied through Marek via the usb tree before the dt sync.

Best Regards,

Tim