RE: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

[Xu Yang]

Hi Luca,

> > > -----Original Message-----
> > >
> > > Hello Xu,
> > >
> > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > Xu Yang <xu.yang_2@xxxxxxx> wrote:
> > >
> > > > > -----Original Message-----
> > > > >
> > > > > Ciao Francesco,
> > > > >
> > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > Francesco Dolcini <francesco@xxxxxxxxxx> wrote:
> > > > >
> > > > > > Hello Luca,
> > > > > >
> > > > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > > > soon.
> > > > > >
> > > > > > A series was posted
> > > > > >
> > > > > > I had no time to try or look at it yet.
> > > > >
> > > > > Thanks for keeping me up to date on this topic, which is still totally
> > > > > relevant to me.
> > > > >
> > > > > I looked at the series, but it does not seem to be addressing the
> > > > > problem with USB host not detecting new devices when VBUS is not
> > > > > directly connected, e.g. in the Colibri imx6ull SoM.
> > > > >
> > > > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > > > being discussed here?
> > > >
> > > > Have you tried this patchset? The upstream driver couldn't get correct
> > > > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > > > what this patchset is trying to fix. With this patch, condition
> > > > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > > > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > > > will never be called. So I think it doesn't matter whether VBUS is connected
> > > > or not when act as host mode. If you still have issue after apply this patchset,
> > > > please let me know.
> > >
> > > I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> > > is still broken on the Colibri iMX6ULL. With or without the patches
> > > the behavior is the same: USB devices are detected only during boot,
> > > and anything connected after boot are never detected.
> >
> > Thanks for your feedback. As you said this issue will disappear with below change, right?
> >
> >       diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> >       index e1a2b2ea098b..ec5ee790455e 100644
> >       --- a/drivers/usb/phy/phy-mxs-usb.c
> >       +++ b/drivers/usb/phy/phy-mxs-usb.c
> >       @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> >        };
> >
> >        static const struct mxs_phy_data imx6ul_phy_data = {
> >       -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> >        };
> >
> >        static const struct mxs_phy_data imx7ulp_phy_data = {
> 
> Exactly.
> 
> > So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
> > Could you please help to find which line to comment to make this issue disappear?

To correct what I said:  __mxs_phy_disconnect_line(mxs_phy, false) should
be called.

I think the enable wakeup sequence should be follow for host-only port:
mxs_phy_set_wakeup(mxs_phy, true)
    mxs_phy_disconnect_line(mxs_phy, true);
        __mxs_phy_disconnect_line(mxs_phy, false);

And disable wakeup sequence:
mxs_phy_set_wakeup(mxs_phy, false)
    mxs_phy_disconnect_line(mxs_phy, false);
        __mxs_phy_disconnect_line(mxs_phy, false);

So "bool variable disconnect is false" all the time.

> 
> I did some tests and detection works by doing _any_ of the following
> two changes (or both of them).
> 
> Change 1:
> 
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -359,10 +359,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
>         void __iomem *base = mxs_phy->phy.io_priv;
>         u32 reg;
> 
> -       if (disconnect)
> -               writel_relaxed(BM_USBPHY_DEBUG_CLKGATE,
> -                       base + HW_USBPHY_DEBUG_CLR);
> -

Since disconnect = false, this code didn't get executed all the time.
Remove this will have no impact. But your test results didn't align
to this. Could you please help check the sequence? Is disconnect
true or false when __mxs_phy_disconnect_line is called?

Thanks,
Xu Yang

>         if (mxs_phy->port_id == 0) {
>                 reg = disconnect ? ANADIG_USB1_LOOPBACK_SET
>                         : ANADIG_USB1_LOOPBACK_CLR;
> 
> Change 2:
> 
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -372,9 +372,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
>         } else if (mxs_phy->port_id == 1) {
>                 reg = disconnect ? ANADIG_USB2_LOOPBACK_SET
>                         : ANADIG_USB2_LOOPBACK_CLR;
> -               regmap_write(mxs_phy->regmap_anatop, reg,
> -                       BM_ANADIG_USB2_LOOPBACK_UTMI_DIG_TST1 |
> -                       BM_ANADIG_USB2_LOOPBACK_TSTI_TX_EN);
>         }
> 
>         if (!disconnect)
> 
> I hope this clarifies something to you.
> 
> Luca
>