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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <DB7PR04MB4505F28957A32F09BB84B27A8C3FA@DB7PR04MB4505.eurprd04.prod.outlook.com>
Date:   Fri, 21 Jul 2023 02:06:18 +0000
From:   Xu Yang <xu.yang_2@....com>
To:     Luca Ceresoli <luca.ceresoli@...tlin.com>
CC:     Francesco Dolcini <francesco@...cini.it>, Jun Li <jun.li@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: RE: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling
 runtime-pm

Hi Luca,

> -----Original Message-----
> 
> Hi Xu,
> 
> On Thu, 20 Jul 2023 10:13:57 +0000
> Xu Yang <xu.yang_2@....com> wrote:
> 
> > Hi Luca,
> >
> > > > > -----Original Message-----
> > > > >
> > > > > Hello Xu,
> > > > >
> > > > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > > > Xu Yang <xu.yang_2@....com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > >
> > > > > > > Ciao Francesco,
> > > > > > >
> > > > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > > > Francesco Dolcini <francesco@...cini.it> 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?
> 
> What I observe is that __mxs_phy_disconnect_line(..., true) is called.
> This happens during boot and after unplugging a device in case one was
> detected during boot.
> 
> This is because in mxs_phy_disconnect_line() [0]:
>  - on = 1
>  - !vbus_is_on = 1
>  - !mxs_phy_is_otg_host(mxs_phy) = 1
> 
> Which one(s) of those three would you expect to be 0?
> 
> Some additional info about the !mxs_phy_is_otg_host(mxs_phy) value:
> with or without CONFIG_USB_OTG, it always has the same value because
> phyctrl always has the BM_USBPHY_CTRL_OTG_ID_VALUE bit set.
> 

I guess you didn't apply my whole patchset.

[1/3] usb: chipidea: add USB PHY event
[2/3] usb: phy: mxs: fix getting wrong state with mxs_phy_is_otg_host()
[3/3] usb: phy: mxs: disconnect line when USB charger is attached

The 2nd patch is going to fix the issue of mxs_phy_is_otg_host():
https://lore.kernel.org/all/20230627110353.1879477-2-xu.yang_2@nxp.com/

Our design is not to disconnect line when the controller is acting as host mode.

Thanks,
Xu Yang