RE: i.MX8MM USB autosuspend broken with power domain support

[Jun Li <jun.li@nxp.com>]

> -----Original Message-----
> From: Jacky Bai <ping.bai@nxp.com>
> Sent: Friday, April 15, 2022 10:21 AM
> To: Jun Li <jun.li@nxp.com>; Lucas Stach <l.stach@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>; Frieder Schrempf
> <frieder.schrempf@kontron.de>
> Cc: Peter Chen <peter.chen@kernel.org>; Peng Fan <peng.fan@nxp.com>;
> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> marek.vasut@gmail.com; tharvey@gateworks.com; Adam Ford
> <aford173@gmail.com>; Breno Matheus Lima <breno.lima@nxp.com>; Xu Yang
> <xu.yang_2@nxp.com>
> Subject: RE: i.MX8MM USB autosuspend broken with power domain support
> 
> > Subject: RE: i.MX8MM USB autosuspend broken with power domain support
> >
> >
> >
> > > -----Original Message-----
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > > Sent: Wednesday, April 13, 2022 11:03 PM
> > > To: Fabio Estevam <festevam@gmail.com>; Frieder Schrempf
> > > <frieder.schrempf@kontron.de>; Jun Li <jun.li@nxp.com>
> > > Cc: Peter Chen <peter.chen@kernel.org>; Peng Fan <peng.fan@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org; dl-linux-imx
> > > <linux-imx@nxp.com>; marek.vasut@gmail.com; tharvey@gateworks.com;
> > > Adam Ford <aford173@gmail.com>; Breno Matheus Lima
> > > <breno.lima@nxp.com>
> > > Subject: Re: i.MX8MM USB autosuspend broken with power domain support
> > >
> > > Am Mittwoch, dem 13.04.2022 um 11:40 -0300 schrieb Fabio Estevam:
> > > > [Adding Jun Li]
> > > >
> > > > On Wed, Apr 13, 2022 at 11:35 AM Frieder Schrempf
> > > > <frieder.schrempf@kontron.de> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > when power domain support was added for i.MX8MM, it seems like
> > > > > this broke the USB autosuspend feature.
> > > > >
> > > > > I reported this previously when testing the gpcv2 patches before
> > > > > they were merged [1] and the issue can also be reproduced on v5.18-rc2.
> > > > >
> > > > > Did anyone else encounter such a problem? Can anyone help with
> > > > > debugging or proposing a fix?
> > > > >
> > > > > Do the USB power domains need to stay enabled for autosuspend to
> > work?
> > > > > If yes how can this be achieved?
> > > > >
> > > > > Below is some more information on how to reproduce the issue
> > > > > including some debug output.
> > > > >
> > > > > Thanks a lot and best regards
> > > > > Frieder
> > > > >
> > > > > 1. Plug in USB device on host port, device is not enumerated, no
> > > > > debug output
> > > > >
> > > > > 2. Disable autosuspend, device gets enumerated
> > > > >
> > > > > ~# echo on > /sys/bus/usb/devices/usb1/power/control
> > > > > [ 2986.582786] imx_usb 32e40000.usb: genpd_runtime_resume() [
> > > > > 2986.588155] imx-pgc imx-pgc-domain.2: genpd_runtime_resume() [
> > > > > 2986.593876] imx-pgc imx-pgc-domain.2: resume latency exceeded,
> > > > > 1125
> > > ns
> > > > > [ 2986.600446] PM: usb-otg1: Power-on latency exceeded, new value
> > > > > 12295000 ns
> > > > > [ 2986.607342] imx_usb 32e40000.usb: at imx_controller_resume [
> > > > > 2986.612850] ci_hdrc ci_hdrc.0: genpd_runtime_resume() [
> > > > > 2986.617919] ci_hdrc ci_hdrc.0: at ci_controller_resume [
> > > > > 2986.858565] usb 1-1: new full-speed USB device number 10 using
> > > > > ci_hdrc
> > > > >
> > > > > [1]
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.
> > >
> > org%2Flkml%2F2021%2F5%2F19%2F883&amp;data=04%7C01%7Cjun.li%40n
> > xp.com%7
> > > C
> > >
> > 95df1db516454246d2ef08da1d5eb444%7C686ea1d3bc2b4c6fa92cd99c5c30
> > 1635%7C
> > > 0
> > > %7C0%7C637854589844488598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> > MC4wLjAwMDAi
> > > L
> > >
> > CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Zz4
> > 47nG
> > > g
> > > 9Rr%2Bvn96v4KJelygmuqFWaScqPIFDWMBdWg%3D&amp;reserved=0
> > >
> > > Now that I think about it again, it seems putting the USB controllers
> > > into the OTG1/2 power domains is wrong. I guess the controllers are
> > > actually located in the HSIOMIX domain, which probably needs to stay
> > > enabled even if there is no device connected, so that the wakeup logic
> > > works properly. It's the USB PHYs that should be placed in the OTG
> > > domains and which I expect can be powered down as long as no device is
> > > connected.
> >
> > Per my current understanding, USB remote wakeup(wakeup host via USB data
> > line) need PHY power on, but controller can be off, I will Check internally
> how
> > those power domains map to each physical part in SoC, and reproduce the
> issue
> > Frieder is reporting.
> >
> 
> On imx8mm, the USB OTG1/2 power domain are actually for PHY only, and it
> is just handling the PHY isolation.
> If these power domain are put into OFF mode in HW, PHY's output signal will
> be isolated, then
> leads to USB enumeration can NOT work. HSIOMIX PD only has ADB400 handshake.
> Just think again,
> the parent/child relationship between OTG1/2 and HSIOMIX is not really
> necessary. To simplify things,
> we can decouple the dependency between OTG1/2 and HSIOMIX, change them to
> sibling power domains,
> then controller just attaches the HSIOMIX pd, and USB PHY attaches OTG1/2
> correspondingly.

Yes, this can work.

Hi Frieder,

Could you please try below change?

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 1ee05677c2dd..3ff71ca122e4 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -275,6 +275,7 @@ usbphynop1: usbphynop1 {
                clocks = <&clk IMX8MM_CLK_USB_PHY_REF>;
                assigned-clocks = <&clk IMX8MM_CLK_USB_PHY_REF>;
                assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_100M>;
+               power-domains = <&pgc_otg1>;
                clock-names = "main_clk";
        };
 
@@ -284,6 +285,7 @@ usbphynop2: usbphynop2 {
                clocks = <&clk IMX8MM_CLK_USB_PHY_REF>;
                assigned-clocks = <&clk IMX8MM_CLK_USB_PHY_REF>;
                assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_100M>;
+               power-domains = <&pgc_otg2>;
                clock-names = "main_clk";
        };
 
@@ -669,13 +671,11 @@ pgc_pcie: power-domain@1 {
                                        pgc_otg1: power-domain@2 {
                                                #power-domain-cells = <0>;
                                                reg = <IMX8MM_POWER_DOMAIN_OTG1>;
-                                               power-domains = <&pgc_hsiomix>;
                                        };
 
                                        pgc_otg2: power-domain@3 {
                                                #power-domain-cells = <0>;
                                                reg = <IMX8MM_POWER_DOMAIN_OTG2>;
-                                               power-domains = <&pgc_hsiomix>;
                                        };
 
                                        pgc_gpumix: power-domain@4 {
@@ -1180,7 +1180,7 @@ usbotg1: usb@32e40000 {
                                assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>;
                                phys = <&usbphynop1>;
                                fsl,usbmisc = <&usbmisc1 0>;
-                               power-domains = <&pgc_otg1>;
+                               power-domains = <&pgc_hsiomix>;
                                status = "disabled";
                        };
 
@@ -1200,7 +1200,7 @@ usbotg2: usb@32e50000 {
                                assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>;
                                phys = <&usbphynop2>;
                                fsl,usbmisc = <&usbmisc2 0>;
-                               power-domains = <&pgc_otg2>;
+                               power-domains = <&pgc_hsiomix>;
                                status = "disabled";
                        };

> 
> BR
> Jacky Bai
> 
> > Li Jun
> > >
> > > The i.MX8MM support currently handles the PHYs via usb-nop-xceiv,
> > > which AFAICS doesn't properly handle runtime PM. :/

This matches the fact USB PHY power domain cannot be off, but we
May not rely on this.

Thanks
Li Jun

> > >
> > > Regards,
> > > Lucas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel