i.MX8MM USB autosuspend broken with power domain support
Frieder Schrempf
frieder.schrempf at kontron.de
Thu Apr 28 03:11:21 PDT 2022
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 at nxp.com>
>>> Sent: Friday, April 15, 2022 10:21 AM
>>> To: Jun Li <jun.li at nxp.com>; Lucas Stach <l.stach at pengutronix.de>; Fabio
>>> Estevam <festevam at gmail.com>; Frieder Schrempf
>>> <frieder.schrempf at kontron.de>
>>> Cc: Peter Chen <peter.chen at kernel.org>; Peng Fan <peng.fan at nxp.com>;
>>> linux-arm-kernel at lists.infradead.org; dl-linux-imx <linux-imx at nxp.com>;
>>> marek.vasut at gmail.com; tharvey at gateworks.com; Adam Ford
>>> <aford173 at gmail.com>; Breno Matheus Lima <breno.lima at nxp.com>; Xu Yang
>>> <xu.yang_2 at 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 at pengutronix.de>
>>>>> Sent: Wednesday, April 13, 2022 11:03 PM
>>>>> To: Fabio Estevam <festevam at gmail.com>; Frieder Schrempf
>>>>> <frieder.schrempf at kontron.de>; Jun Li <jun.li at nxp.com>
>>>>> Cc: Peter Chen <peter.chen at kernel.org>; Peng Fan <peng.fan at nxp.com>;
>>>>> linux-arm-kernel at lists.infradead.org; dl-linux-imx
>>>>> <linux-imx at nxp.com>; marek.vasut at gmail.com; tharvey at gateworks.com;
>>>>> Adam Ford <aford173 at gmail.com>; Breno Matheus Lima
>>>>> <breno.lima at 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 at 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&data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C09e76aed6ea746047b5808da1eb7ba52%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637856071702358709%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3STwzi1U9uAYHwa17k7cl2EfAEou6qi7vlHtCa%2FiG%2Bs%3D&reserved=0.
>>>>>
>>>> org%2Flkml%2F2021%2F5%2F19%2F883&data=04%7C01%7Cjun.li%40n
>>>> xp.com%7
>>>>> C
>>>>>
>>>> 95df1db516454246d2ef08da1d5eb444%7C686ea1d3bc2b4c6fa92cd99c5c30
>>>> 1635%7C
>>>>> 0
>>>>> %7C0%7C637854589844488598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
>>>> MC4wLjAwMDAi
>>>>> L
>>>>>
>>>> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Zz4
>>>> 47nG
>>>>> g
>>>>> 9Rr%2Bvn96v4KJelygmuqFWaScqPIFDWMBdWg%3D&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 at 1 {
>> pgc_otg1: power-domain at 2 {
>> #power-domain-cells = <0>;
>> reg = <IMX8MM_POWER_DOMAIN_OTG1>;
>> - power-domains = <&pgc_hsiomix>;
>> };
>>
>> pgc_otg2: power-domain at 3 {
>> #power-domain-cells = <0>;
>> reg = <IMX8MM_POWER_DOMAIN_OTG2>;
>> - power-domains = <&pgc_hsiomix>;
>> };
>>
>> pgc_gpumix: power-domain at 4 {
>> @@ -1180,7 +1180,7 @@ usbotg1: usb at 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 at 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?
More information about the linux-arm-kernel
mailing list