All of lore.kernel.org
 help / color / mirror / Atom feed
* i.MX8MM USB autosuspend broken with power domain support
@ 2022-04-13 14:35 Frieder Schrempf
  2022-04-13 14:40 ` Fabio Estevam
  0 siblings, 1 reply; 9+ messages in thread
From: Frieder Schrempf @ 2022-04-13 14:35 UTC (permalink / raw)
  To: Peter Chen, Lucas Stach, Peng Fan
  Cc: linux-arm-kernel, NXP Linux Team, marek.vasut, Tim Harvey,
	Adam Ford, Fabio Estevam, breno.lima

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://lkml.org/lkml/2021/5/19/883

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: i.MX8MM USB autosuspend broken with power domain support
  2022-04-13 14:35 i.MX8MM USB autosuspend broken with power domain support Frieder Schrempf
@ 2022-04-13 14:40 ` Fabio Estevam
  2022-04-13 15:02   ` Lucas Stach
  0 siblings, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2022-04-13 14:40 UTC (permalink / raw)
  To: Frieder Schrempf, Li Jun
  Cc: Peter Chen, Lucas Stach, Peng Fan, linux-arm-kernel,
	NXP Linux Team, marek.vasut, Tim Harvey, Adam Ford, Breno Lima

[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://lkml.org/lkml/2021/5/19/883

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: i.MX8MM USB autosuspend broken with power domain support
  2022-04-13 14:40 ` Fabio Estevam
@ 2022-04-13 15:02   ` Lucas Stach
  2022-04-13 15:58     ` Jun Li
  0 siblings, 1 reply; 9+ messages in thread
From: Lucas Stach @ 2022-04-13 15:02 UTC (permalink / raw)
  To: Fabio Estevam, Frieder Schrempf, Li Jun
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, NXP Linux Team,
	marek.vasut, Tim Harvey, Adam Ford, Breno Lima

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://lkml.org/lkml/2021/5/19/883

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.

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

Regards,
Lucas


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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* RE: i.MX8MM USB autosuspend broken with power domain support
  2022-04-13 15:02   ` Lucas Stach
@ 2022-04-13 15:58     ` Jun Li
  2022-04-15  2:21       ` Jacky Bai
  0 siblings, 1 reply; 9+ messages in thread
From: Jun Li @ 2022-04-13 15:58 UTC (permalink / raw)
  To: Lucas Stach, Fabio Estevam, Frieder Schrempf
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang



> -----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%40nxp.com%7C
> 95df1db516454246d2ef08da1d5eb444%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C637854589844488598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Zz447nGg
> 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.

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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* RE: i.MX8MM USB autosuspend broken with power domain support
  2022-04-13 15:58     ` Jun Li
@ 2022-04-15  2:21       ` Jacky Bai
  2022-04-15  8:12         ` Jun Li
  0 siblings, 1 reply; 9+ messages in thread
From: Jacky Bai @ 2022-04-15  2:21 UTC (permalink / raw)
  To: Jun Li, Lucas Stach, Fabio Estevam, Frieder Schrempf
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang

> 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.

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. :/
> >
> > Regards,
> > Lucas

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* RE: i.MX8MM USB autosuspend broken with power domain support
  2022-04-15  2:21       ` Jacky Bai
@ 2022-04-15  8:12         ` Jun Li
  2022-04-28  9:57           ` Frieder Schrempf
  0 siblings, 1 reply; 9+ messages in thread
From: Jun Li @ 2022-04-15  8:12 UTC (permalink / raw)
  To: Jacky Bai, Lucas Stach, Fabio Estevam, Frieder Schrempf
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang



> -----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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: i.MX8MM USB autosuspend broken with power domain support
  2022-04-15  8:12         ` Jun Li
@ 2022-04-28  9:57           ` Frieder Schrempf
  2022-04-28 10:11             ` Frieder Schrempf
  2022-04-29  3:14             ` Jun Li
  0 siblings, 2 replies; 9+ messages in thread
From: Frieder Schrempf @ 2022-04-28  9:57 UTC (permalink / raw)
  To: Jun Li, Jacky Bai, Lucas Stach, Fabio Estevam
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang

Am 15.04.22 um 10:12 schrieb Jun Li:
> 
> 
>> -----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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flkml&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C09e76aed6ea746047b5808da1eb7ba52%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637856071702358709%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=3STwzi1U9uAYHwa17k7cl2EfAEou6qi7vlHtCa%2FiG%2Bs%3D&amp;reserved=0.
>>>>
>>> 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";
>                         };
> 

Thanks Jacky, Jun and Lucas for the comments and suggestions and sorry
for the delay!

If Jacky is correct, we can power down the HSIOMIX and keep the OTG1/2
domains enabled so the PHY is still in a state where it can detect
devices. So it's the other way round than what Lucas assumed first.

Jun's patch above does seem to fix the issue for me. The decoupling of
the OTG1/2 domains from the HSIO domain suggested by Jacky is still
missing, though. IIUC this should enable us to power down the HSIOMIX if
the controller is idle and save some power. Removing the "power-domains
= <&pgc_hsiomix>" from the pgc_otg1/2 nodes should do the trick and
seems to work fine for me, too.

Jun, do you want to send a formal patch for this, or would you like me
to do it?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: i.MX8MM USB autosuspend broken with power domain support
  2022-04-28  9:57           ` Frieder Schrempf
@ 2022-04-28 10:11             ` Frieder Schrempf
  2022-04-29  3:14             ` Jun Li
  1 sibling, 0 replies; 9+ messages in thread
From: Frieder Schrempf @ 2022-04-28 10:11 UTC (permalink / raw)
  To: Jun Li, Jacky Bai, Lucas Stach, Fabio Estevam
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang

Am 28.04.22 um 11:57 schrieb Frieder Schrempf:
> Am 15.04.22 um 10:12 schrieb Jun Li:
>>
>>
>>> -----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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flkml&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C09e76aed6ea746047b5808da1eb7ba52%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637856071702358709%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=3STwzi1U9uAYHwa17k7cl2EfAEou6qi7vlHtCa%2FiG%2Bs%3D&amp;reserved=0.
>>>>>
>>>> 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";
>>                         };
>>
> 
> Thanks Jacky, Jun and Lucas for the comments and suggestions and sorry
> for the delay!
> 
> If Jacky is correct, we can power down the HSIOMIX and keep the OTG1/2
> domains enabled so the PHY is still in a state where it can detect
> devices. So it's the other way round than what Lucas assumed first.
> 
> Jun's patch above does seem to fix the issue for me. The decoupling of
> the OTG1/2 domains from the HSIO domain suggested by Jacky is still
> missing, though. IIUC this should enable us to power down the HSIOMIX if
> the controller is idle and save some power. Removing the "power-domains
> = <&pgc_hsiomix>" from the pgc_otg1/2 nodes should do the trick and
> seems to work fine for me, too.

Forget about the last part above. Jun's patch above already does exactly
this. I just somehow missed that part.

> 
> Jun, do you want to send a formal patch for this, or would you like me
> to do it?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* RE: i.MX8MM USB autosuspend broken with power domain support
  2022-04-28  9:57           ` Frieder Schrempf
  2022-04-28 10:11             ` Frieder Schrempf
@ 2022-04-29  3:14             ` Jun Li
  1 sibling, 0 replies; 9+ messages in thread
From: Jun Li @ 2022-04-29  3:14 UTC (permalink / raw)
  To: Frieder Schrempf, Jacky Bai, Lucas Stach, Fabio Estevam
  Cc: Peter Chen, Peng Fan, linux-arm-kernel, dl-linux-imx,
	marek.vasut, tharvey, Adam Ford, Breno Matheus Lima, Xu Yang



> -----Original Message-----
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> Sent: Thursday, April 28, 2022 5:58 PM
> To: Jun Li <jun.li@nxp.com>; Jacky Bai <ping.bai@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Fabio Estevam <festevam@gmail.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>; Xu Yang
> <xu.yang_2@nxp.com>
> Subject: Re: i.MX8MM USB autosuspend broken with power domain support
> 
> Am 15.04.22 um 10:12 schrieb Jun Li:
> >
> >
> >> -----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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Feur01
> .safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flkml&a
> mp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C09e76aed6ea746047b580
> 8da1eb7ba52%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C63785607170235
> 8709%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=3STwzi1U9uAYHwa17k7cl2EfAEou6
> qi7vlHtCa%2FiG%2Bs%3D&amp;reserved=0.
> >>>>
> >>> 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";
> >                         };
> >
> 
> Thanks Jacky, Jun and Lucas for the comments and suggestions and sorry for
> the delay!
> 
> If Jacky is correct, we can power down the HSIOMIX and keep the OTG1/2 domains
> enabled so the PHY is still in a state where it can detect devices. So it's
> the other way round than what Lucas assumed first.
> 
> Jun's patch above does seem to fix the issue for me. The decoupling of the
> OTG1/2 domains from the HSIO domain suggested by Jacky is still missing,
> though. IIUC this should enable us to power down the HSIOMIX if the controller
> is idle and save some power. Removing the "power-domains = <&pgc_hsiomix>"
> from the pgc_otg1/2 nodes should do the trick and seems to work fine for
> me, too.
> 
> Jun, do you want to send a formal patch for this, or would you like me to
> do it?

OK, I will send a formal patch.

Li Jun

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-29  3:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 14:35 i.MX8MM USB autosuspend broken with power domain support Frieder Schrempf
2022-04-13 14:40 ` Fabio Estevam
2022-04-13 15:02   ` Lucas Stach
2022-04-13 15:58     ` Jun Li
2022-04-15  2:21       ` Jacky Bai
2022-04-15  8:12         ` Jun Li
2022-04-28  9:57           ` Frieder Schrempf
2022-04-28 10:11             ` Frieder Schrempf
2022-04-29  3:14             ` Jun Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.