[RFC,1/1] usb: chipidea: ci_hdrc_imx: disable runtime pm for HSIC interface

Commit Message

Alexander Stein March 2, 2022, 9:42 a.m. UTC

With the add of power-domain support in commit 02f8eb40ef7b ("ARM: dts:
imx7s: Add power domain for imx7d HSIC") runtime suspend will disable
the power-domain. This prevents IRQs to occur when a new device is attached
on a downstream hub.

Signed-off-by: Alexander Stein <[e-mail]>
---
Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted USB HUB on
usbh device. Cold plugging an USB mass storage device is working fine. But
once the last non-HUB device is disconnected the ci_hdrc device goes into
runtime suspend. This will eventually also disable the 'pgc_hsic_phy' power
domain. Results is that no more updates from USB hub is handled, neither HUB
on HSIC link or HUB on that's downstream link. USB tree looks like this:

/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
            |__ Port 1: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 480M
        |__ Port 2: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M

I noticed once the power domain is disabled, no IRQs appear if I attach a new
mass storage, essentially preventing any runtime resume.
I do not know if this is specific to i.MX7 only or if this is a general USB
HSIC problem, so this diff might be too much.

BTW: An udev rule with the same effect is:
ACTION=="add", SUBSYSTEMS=="platform", DRIVER=="ci_hdrc",
KERNELS=="30b30000.usb", TEST=="power/control", ATTR{power/control}="on"

But I would like to get this fixed on driver level.

Regards
Alexander

 drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Peter Chen March 15, 2022, 1:23 a.m. UTC | #1

On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
<[e-mail]> wrote:
>
> With the add of power-domain support in commit 02f8eb40ef7b ("ARM: dts:
> imx7s: Add power domain for imx7d HSIC") runtime suspend will disable
> the power-domain. This prevents IRQs to occur when a new device is attached
> on a downstream hub.
>
> Signed-off-by: Alexander Stein <[e-mail]>
> ---
> Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted USB HUB on
> usbh device. Cold plugging an USB mass storage device is working fine. But
> once the last non-HUB device is disconnected the ci_hdrc device goes into
> runtime suspend.

Would you please show the difference between cold boot and runtime
suspend after disconnecting
the last USB device?

- Power domain on/off status for HUB device
- Runtime suspend status at /sys entry for HUB device
- "/sys/..power/wakeup" /sys entry  for HUB device

Peter

> This will eventually also disable the 'pgc_hsic_phy' power
> domain. Results is that no more updates from USB hub is handled, neither HUB
> on HSIC link or HUB on that's downstream link. USB tree looks like this:
>
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
>             |__ Port 1: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>         |__ Port 2: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>
> I noticed once the power domain is disabled, no IRQs appear if I attach a new
> mass storage, essentially preventing any runtime resume.
> I do not know if this is specific to i.MX7 only or if this is a general USB
> HSIC problem, so this diff might be too much.
>
> BTW: An udev rule with the same effect is:
> ACTION=="add", SUBSYSTEMS=="platform", DRIVER=="ci_hdrc",
> KERNELS=="30b30000.usb", TEST=="power/control", ATTR{power/control}="on"
>
> But I would like to get this fixed on driver level.
>
> Regards
> Alexander
>
>  drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 097142ffb184..e5c22b70431c 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -344,6 +344,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>         if ((of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC)
>                 && data->usbmisc_data) {
>                 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
> +               /* Runtime suspend is not supported for HSIC interface */
> +               pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
>                 data->usbmisc_data->hsic = 1;
>                 data->pinctrl = devm_pinctrl_get(dev);
>                 if (PTR_ERR(data->pinctrl) == -ENODEV)
> --
> 2.25.1
>

Alexander Stein March 29, 2022, 8:14 a.m. UTC | #2

Hello Peter,

Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> 
> <[e-mail]> wrote:
> > With the add of power-domain support in commit 02f8eb40ef7b ("ARM: dts:
> > imx7s: Add power domain for imx7d HSIC") runtime suspend will disable
> > the power-domain. This prevents IRQs to occur when a new device is
> > attached
> > on a downstream hub.
> > 
> > Signed-off-by: Alexander Stein <[e-mail]>
> > ---
> > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted USB HUB
> > on usbh device. Cold plugging an USB mass storage device is working fine.
> > But once the last non-HUB device is disconnected the ci_hdrc device goes
> > into runtime suspend.
> 
> Would you please show the difference between cold boot and runtime
> suspend after disconnecting
> the last USB device?
> 
> - Power domain on/off status for HUB device
> - Runtime suspend status at /sys entry for HUB device
> - "/sys/..power/wakeup" /sys entry  for HUB device

I hope I got all entries you requested.

For reference this is the bus topology:
lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M

Bus 2 is a different connector and doesn't matter here. I'm disconnecting 'Dev 
3' in this scenario.

After boot up with the bus as shown above:
$ cat /sys/bus/usb/devices/1-1/power/wakeup
disabled
$ cat /sys/bus/usb/devices/1-1/power/runtime_status
active
$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
on

After disconnecting Dev 3 from the bus ('usb 1-1.2: USB disconnect, device 
number 3' in dmesg) the status changes as follows (without the patch):
$ cat /sys/bus/usb/devices/1-1/power/wakeup
disabled
$ cat /sys/bus/usb/devices/1-1/power/runtime_status
suspended
$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
off-0

For the record, when applying the posted patch this changes into:
$ cat /sys/bus/usb/devices/1-1/power/wakeup
disabled
$ cat /sys/bus/usb/devices/1-1/power/runtime_status
suspended
$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
on

Regards,
Alexander

Peter Chen April 9, 2022, 2:19 a.m. UTC | #3

On 22-03-29 10:14:36, Alexander Stein wrote:
> Hello Peter,
> 
> Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > 
> > <[e-mail]> wrote:
> > > With the add of power-domain support in commit 02f8eb40ef7b ("ARM: dts:
> > > imx7s: Add power domain for imx7d HSIC") runtime suspend will disable
> > > the power-domain. This prevents IRQs to occur when a new device is
> > > attached
> > > on a downstream hub.
> > > 
> > > Signed-off-by: Alexander Stein <[e-mail]>
> > > ---
> > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted USB HUB
> > > on usbh device. Cold plugging an USB mass storage device is working fine.
> > > But once the last non-HUB device is disconnected the ci_hdrc device goes
> > > into runtime suspend.
> > 
> > Would you please show the difference between cold boot and runtime
> > suspend after disconnecting
> > the last USB device?
> > 
> > - Power domain on/off status for HUB device
> > - Runtime suspend status at /sys entry for HUB device
> > - "/sys/..power/wakeup" /sys entry  for HUB device
> 
> I hope I got all entries you requested.
> 
> For reference this is the bus topology:
> lsusb -t
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M
> 
> Bus 2 is a different connector and doesn't matter here. I'm disconnecting 'Dev 
> 3' in this scenario.
> 
> After boot up with the bus as shown above:
> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> disabled
> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> active
> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> on
> 
> After disconnecting Dev 3 from the bus ('usb 1-1.2: USB disconnect, device 
> number 3' in dmesg) the status changes as follows (without the patch):
> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> disabled
> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> suspended
> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> off-0
> 
> For the record, when applying the posted patch this changes into:
> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> disabled
> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> suspended
> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> on
> 

Okay, I think the problem here is the power domain for USB controller is
off at runtime, but USB controller/PHY needs to detect the USB wakeup
signal at runtime, so the USB controller/PHY's power domain should be
not off. The proper change may keep power domain on at runtime, and the
power domain could be off at system suspend.

The controller/PHY clk could be off at runtime, it does not affect the
wakeup detecting from controll/PHY side.

Jun Li April 9, 2022, 4:49 a.m. UTC | #4

> -----Original Message-----
> From: Peter Chen <[e-mail]>
> Sent: Saturday, April 9, 2022 10:20 AM
> To: Alexander Stein <[e-mail]>
> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> <[e-mail]>; USB list <[e-mail]>;
> [e-mail]
> Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable runtime
> pm for HSIC interface
> 
> On 22-03-29 10:14:36, Alexander Stein wrote:
> > Hello Peter,
> >
> > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > >
> > > <[e-mail]> wrote:
> > > > With the add of power-domain support in commit 02f8eb40ef7b ("ARM:
> dts:
> > > > imx7s: Add power domain for imx7d HSIC") runtime suspend will disable
> > > > the power-domain. This prevents IRQs to occur when a new device is
> > > > attached
> > > > on a downstream hub.
> > > >
> > > > Signed-off-by: Alexander Stein <[e-mail]>
> > > > ---
> > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted
> USB HUB
> > > > on usbh device. Cold plugging an USB mass storage device is working
> fine.
> > > > But once the last non-HUB device is disconnected the ci_hdrc device
> goes
> > > > into runtime suspend.
> > >
> > > Would you please show the difference between cold boot and runtime
> > > suspend after disconnecting
> > > the last USB device?
> > >
> > > - Power domain on/off status for HUB device
> > > - Runtime suspend status at /sys entry for HUB device
> > > - "/sys/..power/wakeup" /sys entry  for HUB device
> >
> > I hope I got all entries you requested.
> >
> > For reference this is the bus topology:
> > lsusb -t
> > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage,
> 480M
> >
> > Bus 2 is a different connector and doesn't matter here. I'm disconnecting
> 'Dev
> > 3' in this scenario.
> >
> > After boot up with the bus as shown above:
> > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > disabled
> > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > active
> > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > on
> >
> > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB disconnect, device
> > number 3' in dmesg) the status changes as follows (without the patch):
> > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > disabled
> > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > suspended
> > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > off-0
> >
> > For the record, when applying the posted patch this changes into:
> > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > disabled
> > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > suspended
> > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > on
> >
> 
> Okay, I think the problem here is the power domain for USB controller is
> off at runtime, but USB controller/PHY needs to detect the USB wakeup
> signal at runtime, so the USB controller/PHY's power domain should be
> not off. The proper change may keep power domain on at runtime, and the
> power domain could be off at system suspend.

Can this "hsic phy power domain off breaks wakeup" be confirmed?
Like with some hack to move hsic phy power domain on some other device
node:

non-usb-node {
        ...
        power-domains = <&pgc_hsic_phy>;
        status = "okay";
};

Just make sure this non-usb-node to be runtime active when
do hsic hub test.

Li Jun 

> 
> The controller/PHY clk could be off at runtime, it does not affect the
> wakeup detecting from controll/PHY side.
> 
> --
> 
> Thanks,
> Peter Chen

Alexander Stein April 11, 2022, 1:52 p.m. UTC | #5

Hi,

Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Peter Chen <[e-mail]>
> > Sent: Saturday, April 9, 2022 10:20 AM
> > To: Alexander Stein <[e-mail]>
> > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> > Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> > <[e-mail]>; USB list <[e-mail]>;
> > [e-mail]
> > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable
> > runtime pm for HSIC interface
> > 
> > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > Hello Peter,
> > > 
> > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > 
> > > > <[e-mail]> wrote:
> > > > > With the add of power-domain support in commit 02f8eb40ef7b ("ARM:
> > dts:
> > > > > imx7s: Add power domain for imx7d HSIC") runtime suspend will
> > > > > disable
> > > > > the power-domain. This prevents IRQs to occur when a new device is
> > > > > attached
> > > > > on a downstream hub.
> > > > > 
> > > > > Signed-off-by: Alexander Stein <[e-mail]>
> > > > > ---
> > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to mounted
> > 
> > USB HUB
> > 
> > > > > on usbh device. Cold plugging an USB mass storage device is working
> > 
> > fine.
> > 
> > > > > But once the last non-HUB device is disconnected the ci_hdrc device
> > 
> > goes
> > 
> > > > > into runtime suspend.
> > > > 
> > > > Would you please show the difference between cold boot and runtime
> > > > suspend after disconnecting
> > > > the last USB device?
> > > > 
> > > > - Power domain on/off status for HUB device
> > > > - Runtime suspend status at /sys entry for HUB device
> > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > 
> > > I hope I got all entries you requested.
> > > 
> > > For reference this is the bus topology:
> > > lsusb -t
> > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > 
> > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > >     |
> > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage,
> > 
> > 480M
> > 
> > > Bus 2 is a different connector and doesn't matter here. I'm
> > > disconnecting
> > 
> > 'Dev
> > 
> > > 3' in this scenario.
> > > 
> > > After boot up with the bus as shown above:
> > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > disabled
> > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > active
> > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > on
> > > 
> > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB disconnect,
> > > device
> > > number 3' in dmesg) the status changes as follows (without the patch):
> > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > disabled
> > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > suspended
> > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > off-0
> > > 
> > > For the record, when applying the posted patch this changes into:
> > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > disabled
> > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > suspended
> > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > on
> > 
> > Okay, I think the problem here is the power domain for USB controller is
> > off at runtime, but USB controller/PHY needs to detect the USB wakeup
> > signal at runtime, so the USB controller/PHY's power domain should be
> > not off. The proper change may keep power domain on at runtime, and the
> > power domain could be off at system suspend.
> 
> Can this "hsic phy power domain off breaks wakeup" be confirmed?
> Like with some hack to move hsic phy power domain on some other device
> node:
> 
> non-usb-node {
>         ...
>         power-domains = <&pgc_hsic_phy>;
>         status = "okay";
> };
> 
> Just make sure this non-usb-node to be runtime active when
> do hsic hub test.

Thanks for that suggestion. I apparently does work. Using the this small patch

--->8---
diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-
mba7.dtsi
index b05f662aa87b..cba2f9efa17e 100644
--- a/arch/arm/boot/dts/imx7-mba7.dtsi
+++ b/arch/arm/boot/dts/imx7-mba7.dtsi
@@ -580,6 +580,7 @@ &uart3 {
        assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
        assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
        status = "okay";
+       power-domains = <&pgc_hsic_phy>;
 };
 
 &uart4 {
--->8---

The HSIC power domain is also attached to to uart3.

$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
/devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.serial
/devices/platform/soc/30800000.bus/30b30000.usb
/devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
on
$ echo on > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
30880000.serial/power/control
$ lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=, 480M
[USB disconnect]
$ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
on
[USB reconnect]
$ lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=, 480M

Hot plug detecting still works as you can see the USB device number increased.

For the records, there is no difference to this patch in removing the power 
domain from USB HSIC device. I just wanted to keep the diff small.

Best regards,
Alexander

Jun Li April 12, 2022, 11:36 a.m. UTC | #6

> -----Original Message-----
> From: Alexander Stein <[e-mail]>
> Sent: Monday, April 11, 2022 9:52 PM
> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> <[e-mail]>; USB list <[e-mail]>;
> [e-mail]
> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable
> runtime pm for HSIC interface
> 
> Hi,
> 
> Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > -----Original Message-----
> > > From: Peter Chen <[e-mail]>
> > > Sent: Saturday, April 9, 2022 10:20 AM
> > > To: Alexander Stein <[e-mail]>
> > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Pengutronix Kernel Team
> > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable
> > > runtime pm for HSIC interface
> > >
> > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > Hello Peter,
> > > >
> > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > >
> > > > > <[e-mail]> wrote:
> > > > > > With the add of power-domain support in commit 02f8eb40ef7b ("ARM:
> > > dts:
> > > > > > imx7s: Add power domain for imx7d HSIC") runtime suspend will
> > > > > > disable the power-domain. This prevents IRQs to occur when a
> > > > > > new device is attached on a downstream hub.
> > > > > >
> > > > > > Signed-off-by: Alexander Stein
> > > > > > <[e-mail]>
> > > > > > ---
> > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to
> > > > > > mounted
> > >
> > > USB HUB
> > >
> > > > > > on usbh device. Cold plugging an USB mass storage device is
> > > > > > working
> > >
> > > fine.
> > >
> > > > > > But once the last non-HUB device is disconnected the ci_hdrc
> > > > > > device
> > >
> > > goes
> > >
> > > > > > into runtime suspend.
> > > > >
> > > > > Would you please show the difference between cold boot and
> > > > > runtime suspend after disconnecting the last USB device?
> > > > >
> > > > > - Power domain on/off status for HUB device
> > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > >
> > > > I hope I got all entries you requested.
> > > >
> > > > For reference this is the bus topology:
> > > > lsusb -t
> > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > >
> > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > >     |
> > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > Driver=usb-storage,
> > >
> > > 480M
> > >
> > > > Bus 2 is a different connector and doesn't matter here. I'm
> > > > disconnecting
> > >
> > > 'Dev
> > >
> > > > 3' in this scenario.
> > > >
> > > > After boot up with the bus as shown above:
> > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > disabled
> > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > active
> > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > on
> > > >
> > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > disconnect, device number 3' in dmesg) the status changes as
> > > > follows (without the patch):
> > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > disabled
> > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > suspended
> > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > off-0
> > > >
> > > > For the record, when applying the posted patch this changes into:
> > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > disabled
> > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > suspended
> > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > on
> > >
> > > Okay, I think the problem here is the power domain for USB
> > > controller is off at runtime, but USB controller/PHY needs to detect
> > > the USB wakeup signal at runtime, so the USB controller/PHY's power
> > > domain should be not off. The proper change may keep power domain on
> > > at runtime, and the power domain could be off at system suspend.
> >
> > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > Like with some hack to move hsic phy power domain on some other device
> > node:
> >
> > non-usb-node {
> >         ...
> >         power-domains = <&pgc_hsic_phy>;
> >         status = "okay";
> > };
> >
> > Just make sure this non-usb-node to be runtime active when do hsic hub
> > test.
> 
> Thanks for that suggestion. I apparently does work. Using the this small
> patch
> 
> --->8---
> diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-
> mba7.dtsi index b05f662aa87b..cba2f9efa17e 100644
> --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> @@ -580,6 +580,7 @@ &uart3 {
>         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
>         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
>         status = "okay";
> +       power-domains = <&pgc_hsic_phy>;
>  };
> 
>  &uart4 {
> --->8---
> 
> The HSIC power domain is also attached to to uart3.
> 
> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.serial
> /devices/platform/soc/30800000.bus/30b30000.usb
> /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> on
> $ echo on > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> 30880000.serial/power/control
> $ lsusb -t
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=, 480M [USB
> disconnect] $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> on

Just want to be sure this was done with hdrc imx runtime PM enabled.
 
> [USB reconnect]
> $ lsusb -t
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=, 480M
> 
> Hot plug detecting still works as you can see the USB device number increased.
> 
> For the records, there is no difference to this patch in removing the power
> domain from USB HSIC device. I just wanted to keep the diff small.

This is good enough to confirm this, thanks.

I don't have a HW with HSIC enabled for test, and I am not sure the initial
state of power domain is on, can something like below deserve a try?

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 3cb123016b3e..f5467ef18e33 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -416,6 +416,7 @@ static const struct imx_pgc_domain imx7_pgc_domains[] = {
        [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
                .genpd = {
                        .name      = "usb-hsic-phy",
+                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
                },
                .bits  = {
                        .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ,
@@ -930,7 +931,7 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
                regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
                                   domain->bits.map, domain->bits.map);
 
-       ret = pm_genpd_init(&domain->genpd, NULL, true);
+       ret = pm_genpd_init(&domain->genpd, NULL, !(domain->genpd.flags & GENPD_FLAG_RPM_ALWAYS_ON));
        if (ret) {
                dev_err(domain->dev, "Failed to init power domain\n");
                goto out_domain_unmap;

Li Jun
> 
> Best regards,
> Alexander
> 
>

Alexander Stein May 4, 2022, 7:06 a.m. UTC | #7

Helllo,

Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <[e-mail]>
> > Sent: Monday, April 11, 2022 9:52 PM
> > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> > Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> > <[e-mail]>; USB list <[e-mail]>;
> > [e-mail]
> > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > disable runtime pm for HSIC interface
> > 
> > Hi,
> > 
> > Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > > -----Original Message-----
> > > > From: Peter Chen <[e-mail]>
> > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > To: Alexander Stein <[e-mail]>
> > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > <[e-mail]>; Pengutronix Kernel Team
> > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable
> > > > runtime pm for HSIC interface
> > > > 
> > > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > > Hello Peter,
> > > > > 
> > > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > > > 
> > > > > > <[e-mail]> wrote:
> > > > > > > With the add of power-domain support in commit 02f8eb40ef7b 
("ARM:
> > > > dts:
> > > > > > > imx7s: Add power domain for imx7d HSIC") runtime suspend will
> > > > > > > disable the power-domain. This prevents IRQs to occur when a
> > > > > > > new device is attached on a downstream hub.
> > > > > > > 
> > > > > > > Signed-off-by: Alexander Stein
> > > > > > > <[e-mail]>
> > > > > > > ---
> > > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to
> > > > > > > mounted
> > > > 
> > > > USB HUB
> > > > 
> > > > > > > on usbh device. Cold plugging an USB mass storage device is
> > > > > > > working
> > > > 
> > > > fine.
> > > > 
> > > > > > > But once the last non-HUB device is disconnected the ci_hdrc
> > > > > > > device
> > > > 
> > > > goes
> > > > 
> > > > > > > into runtime suspend.
> > > > > > 
> > > > > > Would you please show the difference between cold boot and
> > > > > > runtime suspend after disconnecting the last USB device?
> > > > > > 
> > > > > > - Power domain on/off status for HUB device
> > > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > > > 
> > > > > I hope I got all entries you requested.
> > > > > 
> > > > > For reference this is the bus topology:
> > > > > lsusb -t
> > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > > 
> > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > >     |
> > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > > 
> > > > > Driver=usb-storage,
> > > > 
> > > > 480M
> > > > 
> > > > > Bus 2 is a different connector and doesn't matter here. I'm
> > > > > disconnecting
> > > > 
> > > > 'Dev
> > > > 
> > > > > 3' in this scenario.
> > > > > 
> > > > > After boot up with the bus as shown above:
> > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > disabled
> > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > active
> > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > on
> > > > > 
> > > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > > disconnect, device number 3' in dmesg) the status changes as
> > > > > follows (without the patch):
> > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > disabled
> > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > suspended
> > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > off-0
> > > > > 
> > > > > For the record, when applying the posted patch this changes into:
> > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > disabled
> > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > suspended
> > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > on
> > > > 
> > > > Okay, I think the problem here is the power domain for USB
> > > > controller is off at runtime, but USB controller/PHY needs to detect
> > > > the USB wakeup signal at runtime, so the USB controller/PHY's power
> > > > domain should be not off. The proper change may keep power domain on
> > > > at runtime, and the power domain could be off at system suspend.
> > > 
> > > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > > Like with some hack to move hsic phy power domain on some other device
> > > node:
> > > 
> > > non-usb-node {
> > > 
> > >         ...
> > >         power-domains = <&pgc_hsic_phy>;
> > >         status = "okay";
> > > 
> > > };
> > > 
> > > Just make sure this non-usb-node to be runtime active when do hsic hub
> > > test.
> > 
> > Thanks for that suggestion. I apparently does work. Using the this small
> > patch
> > 
> > --->8---
> > diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi b/arch/arm/boot/dts/imx7-
> > mba7.dtsi index b05f662aa87b..cba2f9efa17e 100644
> > --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> > +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> > @@ -580,6 +580,7 @@ &uart3 {
> > 
> >         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> >         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> >         status = "okay";
> > 
> > +       power-domains = <&pgc_hsic_phy>;
> > 
> >  };
> >  
> >  &uart4 {
> > 
> > --->8---
> > 
> > The HSIC power domain is also attached to to uart3.
> > 
> > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> > /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.serial
> > /devices/platform/soc/30800000.bus/30b30000.usb
> > /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > on
> > $ echo on > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> > 30880000.serial/power/control
> > $ lsusb -t
> > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > 
> >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >     |
> >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=, 480M [USB
> > 
> > disconnect] $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > on
> 
> Just want to be sure this was done with hdrc imx runtime PM enabled.
> 
> > [USB reconnect]
> > $ lsusb -t
> > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > 
> >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >     |
> >         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=, 480M
> > 
> > Hot plug detecting still works as you can see the USB device number
> > increased.
> > 
> > For the records, there is no difference to this patch in removing the
> > power
> > domain from USB HSIC device. I just wanted to keep the diff small.
> 
> This is good enough to confirm this, thanks.
> 
> I don't have a HW with HSIC enabled for test, and I am not sure the initial
> state of power domain is on, can something like below deserve a try?
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 3cb123016b3e..f5467ef18e33 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -416,6 +416,7 @@ static const struct imx_pgc_domain imx7_pgc_domains[] =
> { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
>                 .genpd = {
>                         .name      = "usb-hsic-phy",
> +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
>                 },
>                 .bits  = {
>                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ,
> @@ -930,7 +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> domain->bits.map, domain->bits.map);
> 
> -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> +       ret = pm_genpd_init(&domain->genpd, NULL, !(domain->genpd.flags &
> GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
>                 dev_err(domain->dev, "Failed to init power domain\n");
>                 goto out_domain_unmap;

This does indeed the trick. But AFAICS the downside is that the powerdomain is 
enabled, even if USB is not used. Not sure if this is acceptable though.

Alexander

Jun Li May 6, 2022, 7:09 a.m. UTC | #8

> -----Original Message-----
> From: Alexander Stein <[e-mail]>
> Sent: Wednesday, May 4, 2022 3:06 PM
> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> <[e-mail]>; USB list <[e-mail]>;
> [e-mail]
> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable
> runtime pm for HSIC interface
> 
> Helllo,
> 
> Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> > > -----Original Message-----
> > > From: Alexander Stein <[e-mail]>
> > > Sent: Monday, April 11, 2022 9:52 PM
> > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Pengutronix Kernel Team
> > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > disable runtime pm for HSIC interface
> > >
> > > Hi,
> > >
> > > Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > > > -----Original Message-----
> > > > > From: Peter Chen <[e-mail]>
> > > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > > To: Alexander Stein <[e-mail]>
> > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB
> > > > > list <[e-mail]>;
> > > > > [e-mail]
> > > > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > > disable runtime pm for HSIC interface
> > > > >
> > > > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > > > Hello Peter,
> > > > > >
> > > > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > > > >
> > > > > > > <[e-mail]> wrote:
> > > > > > > > With the add of power-domain support in commit
> > > > > > > > 02f8eb40ef7b
> ("ARM:
> > > > > dts:
> > > > > > > > imx7s: Add power domain for imx7d HSIC") runtime suspend
> > > > > > > > will disable the power-domain. This prevents IRQs to occur
> > > > > > > > when a new device is attached on a downstream hub.
> > > > > > > >
> > > > > > > > Signed-off-by: Alexander Stein
> > > > > > > > <[e-mail]>
> > > > > > > > ---
> > > > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to
> > > > > > > > mounted
> > > > >
> > > > > USB HUB
> > > > >
> > > > > > > > on usbh device. Cold plugging an USB mass storage device
> > > > > > > > is working
> > > > >
> > > > > fine.
> > > > >
> > > > > > > > But once the last non-HUB device is disconnected the
> > > > > > > > ci_hdrc device
> > > > >
> > > > > goes
> > > > >
> > > > > > > > into runtime suspend.
> > > > > > >
> > > > > > > Would you please show the difference between cold boot and
> > > > > > > runtime suspend after disconnecting the last USB device?
> > > > > > >
> > > > > > > - Power domain on/off status for HUB device
> > > > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > > > >
> > > > > > I hope I got all entries you requested.
> > > > > >
> > > > > > For reference this is the bus topology:
> > > > > > lsusb -t
> > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > >
> > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > >     |
> > > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > > >
> > > > > > Driver=usb-storage,
> > > > >
> > > > > 480M
> > > > >
> > > > > > Bus 2 is a different connector and doesn't matter here. I'm
> > > > > > disconnecting
> > > > >
> > > > > 'Dev
> > > > >
> > > > > > 3' in this scenario.
> > > > > >
> > > > > > After boot up with the bus as shown above:
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > disabled
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > active
> > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > on
> > > > > >
> > > > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > > > disconnect, device number 3' in dmesg) the status changes as
> > > > > > follows (without the patch):
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > disabled
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > suspended
> > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > off-0
> > > > > >
> > > > > > For the record, when applying the posted patch this changes into:
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > disabled
> > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > suspended
> > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > on
> > > > >
> > > > > Okay, I think the problem here is the power domain for USB
> > > > > controller is off at runtime, but USB controller/PHY needs to
> > > > > detect the USB wakeup signal at runtime, so the USB
> > > > > controller/PHY's power domain should be not off. The proper
> > > > > change may keep power domain on at runtime, and the power domain
> could be off at system suspend.
> > > >
> > > > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > > > Like with some hack to move hsic phy power domain on some other
> > > > device
> > > > node:
> > > >
> > > > non-usb-node {
> > > >
> > > >         ...
> > > >         power-domains = <&pgc_hsic_phy>;
> > > >         status = "okay";
> > > >
> > > > };
> > > >
> > > > Just make sure this non-usb-node to be runtime active when do hsic
> > > > hub test.
> > >
> > > Thanks for that suggestion. I apparently does work. Using the this
> > > small patch
> > >
> > > --->8---
> > > diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > b/arch/arm/boot/dts/imx7- mba7.dtsi index b05f662aa87b..cba2f9efa17e
> > > 100644
> > > --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> > > @@ -580,6 +580,7 @@ &uart3 {
> > >
> > >         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> > >         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> > >         status = "okay";
> > >
> > > +       power-domains = <&pgc_hsic_phy>;
> > >
> > >  };
> > >
> > >  &uart4 {
> > >
> > > --->8---
> > >
> > > The HSIC power domain is also attached to to uart3.
> > >
> > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> > > /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.serial
> > > /devices/platform/soc/30800000.bus/30b30000.usb
> > > /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > on
> > > $ echo on >
> > > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> > > 30880000.serial/power/control
> > > $ lsusb -t
> > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > >
> > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > >     |
> > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=, 480M
> > > [USB
> > >
> > > disconnect] $ cat
> > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > on
> >
> > Just want to be sure this was done with hdrc imx runtime PM enabled.
> >
> > > [USB reconnect]
> > > $ lsusb -t
> > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > >
> > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > >     |
> > >         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=, 480M
> > >
> > > Hot plug detecting still works as you can see the USB device number
> > > increased.
> > >
> > > For the records, there is no difference to this patch in removing
> > > the power domain from USB HSIC device. I just wanted to keep the
> > > diff small.
> >
> > This is good enough to confirm this, thanks.
> >
> > I don't have a HW with HSIC enabled for test, and I am not sure the
> > initial state of power domain is on, can something like below deserve a
> try?
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > 3cb123016b3e..f5467ef18e33 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
> > imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
> >                 .genpd = {
> >                         .name      = "usb-hsic-phy",
> > +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
> >                 },
> >                 .bits  = {
> >                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@ -930,7
> > +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> > *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > domain->bits.map, domain->bits.map);
> >
> > -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> > +       ret = pm_genpd_init(&domain->genpd, NULL,
> > + !(domain->genpd.flags &
> > GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
> >                 dev_err(domain->dev, "Failed to init power domain\n");
> >                 goto out_domain_unmap;
> 
> This does indeed the trick. But AFAICS the downside is that the powerdomain
> is enabled, even if USB is not used. Not sure if this is acceptable though.

I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC port
need the power domain on to detect remote wakeup, what's the exact meaning
of "USB is not used"?

Li Jun
> 
> Alexander
> 
>

Alexander Stein May 6, 2022, 7:38 a.m. UTC | #9

Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <[e-mail]>
> > Sent: Wednesday, May 4, 2022 3:06 PM
> > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> > Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> > <[e-mail]>; USB list <[e-mail]>;
> > [e-mail]
> > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > disable runtime pm for HSIC interface
> > 
> > Helllo,
> > 
> > Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> > > > -----Original Message-----
> > > > From: Alexander Stein <[e-mail]>
> > > > Sent: Monday, April 11, 2022 9:52 PM
> > > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > <[e-mail]>; Pengutronix Kernel Team
> > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > disable runtime pm for HSIC interface
> > > > 
> > > > Hi,
> > > > 
> > > > Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > > > > -----Original Message-----
> > > > > > From: Peter Chen <[e-mail]>
> > > > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > > > To: Alexander Stein <[e-mail]>
> > > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB
> > > > > > list <[e-mail]>;
> > > > > > [e-mail]
> > > > > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > > > disable runtime pm for HSIC interface
> > > > > > 
> > > > > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > > > > Hello Peter,
> > > > > > > 
> > > > > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > > > > > 
> > > > > > > > <[e-mail]> wrote:
> > > > > > > > > With the add of power-domain support in commit
> > > > > > > > > 02f8eb40ef7b
> > 
> > ("ARM:
> > > > > > dts:
> > > > > > > > > imx7s: Add power domain for imx7d HSIC") runtime suspend
> > > > > > > > > will disable the power-domain. This prevents IRQs to occur
> > > > > > > > > when a new device is attached on a downstream hub.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Alexander Stein
> > > > > > > > > <[e-mail]>
> > > > > > > > > ---
> > > > > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC link to
> > > > > > > > > mounted
> > > > > > 
> > > > > > USB HUB
> > > > > > 
> > > > > > > > > on usbh device. Cold plugging an USB mass storage device
> > > > > > > > > is working
> > > > > > 
> > > > > > fine.
> > > > > > 
> > > > > > > > > But once the last non-HUB device is disconnected the
> > > > > > > > > ci_hdrc device
> > > > > > 
> > > > > > goes
> > > > > > 
> > > > > > > > > into runtime suspend.
> > > > > > > > 
> > > > > > > > Would you please show the difference between cold boot and
> > > > > > > > runtime suspend after disconnecting the last USB device?
> > > > > > > > 
> > > > > > > > - Power domain on/off status for HUB device
> > > > > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > > > > > 
> > > > > > > I hope I got all entries you requested.
> > > > > > > 
> > > > > > > For reference this is the bus topology:
> > > > > > > lsusb -t
> > > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > > 480M
> > > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > > 480M
> > > > > > > 
> > > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > > >     |
> > > > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > > > > 
> > > > > > > Driver=usb-storage,
> > > > > > 
> > > > > > 480M
> > > > > > 
> > > > > > > Bus 2 is a different connector and doesn't matter here. I'm
> > > > > > > disconnecting
> > > > > > 
> > > > > > 'Dev
> > > > > > 
> > > > > > > 3' in this scenario.
> > > > > > > 
> > > > > > > After boot up with the bus as shown above:
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > disabled
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > active
> > > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > on
> > > > > > > 
> > > > > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > > > > disconnect, device number 3' in dmesg) the status changes as
> > > > > > > follows (without the patch):
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > disabled
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > suspended
> > > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > off-0
> > > > > > > 
> > > > > > > For the record, when applying the posted patch this changes
> > > > > > > into:
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > disabled
> > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > suspended
> > > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > on
> > > > > > 
> > > > > > Okay, I think the problem here is the power domain for USB
> > > > > > controller is off at runtime, but USB controller/PHY needs to
> > > > > > detect the USB wakeup signal at runtime, so the USB
> > > > > > controller/PHY's power domain should be not off. The proper
> > > > > > change may keep power domain on at runtime, and the power domain
> > 
> > could be off at system suspend.
> > 
> > > > > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > > > > Like with some hack to move hsic phy power domain on some other
> > > > > device
> > > > > node:
> > > > > 
> > > > > non-usb-node {
> > > > > 
> > > > >         ...
> > > > >         power-domains = <&pgc_hsic_phy>;
> > > > >         status = "okay";
> > > > > 
> > > > > };
> > > > > 
> > > > > Just make sure this non-usb-node to be runtime active when do hsic
> > > > > hub test.
> > > > 
> > > > Thanks for that suggestion. I apparently does work. Using the this
> > > > small patch
> > > > 
> > > > --->8---
> > > > diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > b/arch/arm/boot/dts/imx7- mba7.dtsi index b05f662aa87b..cba2f9efa17e
> > > > 100644
> > > > --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > @@ -580,6 +580,7 @@ &uart3 {
> > > > 
> > > >         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> > > >         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> > > >         status = "okay";
> > > > 
> > > > +       power-domains = <&pgc_hsic_phy>;
> > > > 
> > > >  };
> > > >  
> > > >  &uart4 {
> > > > 
> > > > --->8---
> > > > 
> > > > The HSIC power domain is also attached to to uart3.
> > > > 
> > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> > > > /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.serial
> > > > /devices/platform/soc/30800000.bus/30b30000.usb
> > > > /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > on
> > > > $ echo on >
> > > > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> > > > 30880000.serial/power/control
> > > > $ lsusb -t
> > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > 
> > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > >     |
> > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=, 480M
> > > > 
> > > > [USB
> > > > 
> > > > disconnect] $ cat
> > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > on
> > > 
> > > Just want to be sure this was done with hdrc imx runtime PM enabled.
> > > 
> > > > [USB reconnect]
> > > > $ lsusb -t
> > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M
> > > > 
> > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > >     |
> > > >         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=, 480M
> > > > 
> > > > Hot plug detecting still works as you can see the USB device number
> > > > increased.
> > > > 
> > > > For the records, there is no difference to this patch in removing
> > > > the power domain from USB HSIC device. I just wanted to keep the
> > > > diff small.
> > > 
> > > This is good enough to confirm this, thanks.
> > > 
> > > I don't have a HW with HSIC enabled for test, and I am not sure the
> > > initial state of power domain is on, can something like below deserve a
> > 
> > try?
> > 
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > > 3cb123016b3e..f5467ef18e33 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
> > > imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
> > > 
> > >                 .genpd = {
> > >                 
> > >                         .name      = "usb-hsic-phy",
> > > 
> > > +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
> > > 
> > >                 },
> > >                 .bits  = {
> > >                 
> > >                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@ -930,7
> > > 
> > > +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> > > *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > > domain->bits.map, domain->bits.map);
> > > 
> > > -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> > > +       ret = pm_genpd_init(&domain->genpd, NULL,
> > > + !(domain->genpd.flags &
> > > GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
> > > 
> > >                 dev_err(domain->dev, "Failed to init power domain\n");
> > >                 goto out_domain_unmap;
> > 
> > This does indeed the trick. But AFAICS the downside is that the
> > powerdomain
> > is enabled, even if USB is not used. Not sure if this is acceptable
> > though.
> 
> I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC port
> need the power domain on to detect remote wakeup, what's the exact meaning
> of "USB is not used"?

Exactly, GENPD_FLAG_RPM_ALWAYS_ON is the right thing to to iff the HSIC port 
needs the powerdomain. But what about the case when HSIC is not enabled at 
all? That's what I meant by "USB is not used".
AFAICS setting GENPD_FLAG_RPM_ALWAYS_ON enables the powerdomain 
unconditionally from any user.

Alexander

Jun Li May 9, 2022, 8:16 a.m. UTC | #10

> -----Original Message-----
> From: Alexander Stein <[e-mail]>
> Sent: Friday, May 6, 2022 3:38 PM
> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> <[e-mail]>; USB list <[e-mail]>;
> [e-mail]
> Subject: Re: (EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
> ci_hdrc_imx: disable runtime pm for HSIC interface
> 
> Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
> > > -----Original Message-----
> > > From: Alexander Stein <[e-mail]>
> > > Sent: Wednesday, May 4, 2022 3:06 PM
> > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Pengutronix Kernel Team
> > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > disable runtime pm for HSIC interface
> > >
> > > Helllo,
> > >
> > > Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> > > > > -----Original Message-----
> > > > > From: Alexander Stein <[e-mail]>
> > > > > Sent: Monday, April 11, 2022 9:52 PM
> > > > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB
> > > > > list <[e-mail]>;
> > > > > [e-mail]
> > > > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > > disable runtime pm for HSIC interface
> > > > >
> > > > > Hi,
> > > > >
> > > > > Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > > > > > -----Original Message-----
> > > > > > > From: Peter Chen <[e-mail]>
> > > > > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > > > > To: Alexander Stein <[e-mail]>
> > > > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > > > <[e-mail]>; Shawn Guo
> > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > > > <[e-mail]>; dl-linux-imx <[e-mail]>;
> > > > > > > USB list <[e-mail]>;
> > > > > > > [e-mail]
> > > > > > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > > > > disable runtime pm for HSIC interface
> > > > > > >
> > > > > > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > > > > > Hello Peter,
> > > > > > > >
> > > > > > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> > > > > > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > > > > > >
> > > > > > > > > <[e-mail]> wrote:
> > > > > > > > > > With the add of power-domain support in commit
> > > > > > > > > > 02f8eb40ef7b
> > >
> > > ("ARM:
> > > > > > > dts:
> > > > > > > > > > imx7s: Add power domain for imx7d HSIC") runtime
> > > > > > > > > > suspend will disable the power-domain. This prevents
> > > > > > > > > > IRQs to occur when a new device is attached on a downstream
> hub.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Alexander Stein
> > > > > > > > > > <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC
> > > > > > > > > > link to mounted
> > > > > > >
> > > > > > > USB HUB
> > > > > > >
> > > > > > > > > > on usbh device. Cold plugging an USB mass storage
> > > > > > > > > > device is working
> > > > > > >
> > > > > > > fine.
> > > > > > >
> > > > > > > > > > But once the last non-HUB device is disconnected the
> > > > > > > > > > ci_hdrc device
> > > > > > >
> > > > > > > goes
> > > > > > >
> > > > > > > > > > into runtime suspend.
> > > > > > > > >
> > > > > > > > > Would you please show the difference between cold boot
> > > > > > > > > and runtime suspend after disconnecting the last USB device?
> > > > > > > > >
> > > > > > > > > - Power domain on/off status for HUB device
> > > > > > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > > > > > >
> > > > > > > > I hope I got all entries you requested.
> > > > > > > >
> > > > > > > > For reference this is the bus topology:
> > > > > > > > lsusb -t
> > > > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub,
> > > > > > > > Driver=ci_hdrc/1p, 480M
> > > > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub,
> > > > > > > > Driver=ci_hdrc/1p, 480M
> > > > > > > >
> > > > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > > > >     |
> > > > > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > > > > >
> > > > > > > > Driver=usb-storage,
> > > > > > >
> > > > > > > 480M
> > > > > > >
> > > > > > > > Bus 2 is a different connector and doesn't matter here.
> > > > > > > > I'm disconnecting
> > > > > > >
> > > > > > > 'Dev
> > > > > > >
> > > > > > > > 3' in this scenario.
> > > > > > > >
> > > > > > > > After boot up with the bus as shown above:
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > disabled
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > active
> > > > > > > > $ cat
> > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > on
> > > > > > > >
> > > > > > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > > > > > disconnect, device number 3' in dmesg) the status changes
> > > > > > > > as follows (without the patch):
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > disabled
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > suspended
> > > > > > > > $ cat
> > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > off-0
> > > > > > > >
> > > > > > > > For the record, when applying the posted patch this
> > > > > > > > changes
> > > > > > > > into:
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > disabled
> > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > suspended
> > > > > > > > $ cat
> > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > on
> > > > > > >
> > > > > > > Okay, I think the problem here is the power domain for USB
> > > > > > > controller is off at runtime, but USB controller/PHY needs
> > > > > > > to detect the USB wakeup signal at runtime, so the USB
> > > > > > > controller/PHY's power domain should be not off. The proper
> > > > > > > change may keep power domain on at runtime, and the power
> > > > > > > domain
> > >
> > > could be off at system suspend.
> > >
> > > > > > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > > > > > Like with some hack to move hsic phy power domain on some
> > > > > > other device
> > > > > > node:
> > > > > >
> > > > > > non-usb-node {
> > > > > >
> > > > > >         ...
> > > > > >         power-domains = <&pgc_hsic_phy>;
> > > > > >         status = "okay";
> > > > > >
> > > > > > };
> > > > > >
> > > > > > Just make sure this non-usb-node to be runtime active when do
> > > > > > hsic hub test.
> > > > >
> > > > > Thanks for that suggestion. I apparently does work. Using the
> > > > > this small patch
> > > > >
> > > > > --->8---
> > > > > diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > b/arch/arm/boot/dts/imx7- mba7.dtsi index
> > > > > b05f662aa87b..cba2f9efa17e
> > > > > 100644
> > > > > --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > @@ -580,6 +580,7 @@ &uart3 {
> > > > >
> > > > >         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> > > > >         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> > > > >         status = "okay";
> > > > >
> > > > > +       power-domains = <&pgc_hsic_phy>;
> > > > >
> > > > >  };
> > > > >
> > > > >  &uart4 {
> > > > >
> > > > > --->8---
> > > > >
> > > > > The HSIC power domain is also attached to to uart3.
> > > > >
> > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> > > > > /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.se
> > > > > rial /devices/platform/soc/30800000.bus/30b30000.usb
> > > > > /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > on
> > > > > $ echo on >
> > > > > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> > > > > 30880000.serial/power/control
> > > > > $ lsusb -t
> > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > 480M
> > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > 480M
> > > > >
> > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > >     |
> > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=,
> > > > > 480M
> > > > >
> > > > > [USB
> > > > >
> > > > > disconnect] $ cat
> > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > on
> > > >
> > > > Just want to be sure this was done with hdrc imx runtime PM enabled.
> > > >
> > > > > [USB reconnect]
> > > > > $ lsusb -t
> > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > 480M
> > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > 480M
> > > > >
> > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > >     |
> > > > >         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=,
> > > > > 480M
> > > > >
> > > > > Hot plug detecting still works as you can see the USB device
> > > > > number increased.
> > > > >
> > > > > For the records, there is no difference to this patch in
> > > > > removing the power domain from USB HSIC device. I just wanted to
> > > > > keep the diff small.
> > > >
> > > > This is good enough to confirm this, thanks.
> > > >
> > > > I don't have a HW with HSIC enabled for test, and I am not sure
> > > > the initial state of power domain is on, can something like below
> > > > deserve a
> > >
> > > try?
> > >
> > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > index
> > > > 3cb123016b3e..f5467ef18e33 100644
> > > > --- a/drivers/soc/imx/gpcv2.c
> > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
> > > > imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
> > > >
> > > >                 .genpd = {
> > > >
> > > >                         .name      = "usb-hsic-phy",
> > > >
> > > > +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
> > > >
> > > >                 },
> > > >                 .bits  = {
> > > >
> > > >                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@
> > > > -930,7
> > > >
> > > > +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> > > > *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > > > domain->bits.map, domain->bits.map);
> > > >
> > > > -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> > > > +       ret = pm_genpd_init(&domain->genpd, NULL,
> > > > + !(domain->genpd.flags &
> > > > GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
> > > >
> > > >                 dev_err(domain->dev, "Failed to init power domain\n");
> > > >                 goto out_domain_unmap;
> > >
> > > This does indeed the trick. But AFAICS the downside is that the
> > > powerdomain is enabled, even if USB is not used. Not sure if this is
> > > acceptable though.
> >
> > I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC
> > port need the power domain on to detect remote wakeup, what's the
> > exact meaning of "USB is not used"?
> 
> Exactly, GENPD_FLAG_RPM_ALWAYS_ON is the right thing to to iff the HSIC port
> needs the powerdomain. But what about the case when HSIC is not enabled at
> all? That's what I meant by "USB is not used".
> AFAICS setting GENPD_FLAG_RPM_ALWAYS_ON enables the powerdomain
> unconditionally from any user.

If HSIC is not enabled at all, seems the power domain of it will not be touched
by kernel, so there maybe mismatch between the actual HW state and the SW state,
but this is another topic.

For this HSIC case, a second thought I think the better solution maybe
correct the power domain to its right user, since this power domain
is for phy, so:

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 008e3da460f1..039eed79d2e7 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -120,6 +120,7 @@ usbphynop3: usbphynop3 {
                compatible = "usb-nop-xceiv";
                clocks = <&clks IMX7D_USB_HSIC_ROOT_CLK>;
                clock-names = "main_clk";
+               power-domains = <&pgc_hsic_phy>;
                #phy-cells = <0>;
        };
 
@@ -1153,7 +1154,6 @@ usbh: usb@30b30000 {
                                compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
                                reg = <0x30b30000 0x200>;
                                interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
-                               power-domains = <&pgc_hsic_phy>;
                                clocks = <&clks IMX7D_USB_CTRL_CLK>;
                                fsl,usbphy = <&usbphynop3>;
                                fsl,usbmisc = <&usbmisc3 0>;

Could you please try if this can work for you as well?

Li Jun  
 
> 
> Alexander
> 
>

Frieder Schrempf May 9, 2022, 9:27 a.m. UTC | #11

Am 09.05.22 um 10:16 schrieb Jun Li:
> 
> 
>> -----Original Message-----
>> From: Alexander Stein <[e-mail]>
>> Sent: Friday, May 6, 2022 3:38 PM
>> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
>> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
>> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
>> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
>> <[e-mail]>; USB list <[e-mail]>;
>> [e-mail]
>> Subject: Re: (EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
>> ci_hdrc_imx: disable runtime pm for HSIC interface
>>
>> Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
>>>> -----Original Message-----
>>>> From: Alexander Stein <[e-mail]>
>>>> Sent: Wednesday, May 4, 2022 3:06 PM
>>>> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
>>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
>>>> <[e-mail]>; Shawn Guo <[e-mail]>;
>>>> Sascha Hauer <[e-mail]>; Fabio Estevam
>>>> <[e-mail]>; Pengutronix Kernel Team
>>>> <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
>>>> <[e-mail]>; [e-mail]
>>>> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
>>>> disable runtime pm for HSIC interface
>>>>
>>>> Helllo,
>>>>
>>>> Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
>>>>>> -----Original Message-----
>>>>>> From: Alexander Stein <[e-mail]>
>>>>>> Sent: Monday, April 11, 2022 9:52 PM
>>>>>> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
>>>>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
>>>>>> <[e-mail]>; Shawn Guo <[e-mail]>;
>>>>>> Sascha Hauer <[e-mail]>; Fabio Estevam
>>>>>> <[e-mail]>; Pengutronix Kernel Team
>>>>>> <[e-mail]>; dl-linux-imx <[e-mail]>; USB
>>>>>> list <[e-mail]>;
>>>>>> [e-mail]
>>>>>> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
>>>>>> disable runtime pm for HSIC interface
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Peter Chen <[e-mail]>
>>>>>>>> Sent: Saturday, April 9, 2022 10:20 AM
>>>>>>>> To: Alexander Stein <[e-mail]>
>>>>>>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
>>>>>>>> <[e-mail]>; Shawn Guo
>>>>>>>> <[e-mail]>; Sascha Hauer
>>>>>>>> <[e-mail]>; Fabio Estevam
>>>>>>>> <[e-mail]>; Pengutronix Kernel Team
>>>>>>>> <[e-mail]>; dl-linux-imx <[e-mail]>;
>>>>>>>> USB list <[e-mail]>;
>>>>>>>> [e-mail]
>>>>>>>> Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
>>>>>>>> disable runtime pm for HSIC interface
>>>>>>>>
>>>>>>>> On 22-03-29 10:14:36, Alexander Stein wrote:
>>>>>>>>> Hello Peter,
>>>>>>>>>
>>>>>>>>> Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
>>>>>>>>>> On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
>>>>>>>>>>
>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>> With the add of power-domain support in commit
>>>>>>>>>>> 02f8eb40ef7b
>>>>
>>>> ("ARM:
>>>>>>>> dts:
>>>>>>>>>>> imx7s: Add power domain for imx7d HSIC") runtime
>>>>>>>>>>> suspend will disable the power-domain. This prevents
>>>>>>>>>>> IRQs to occur when a new device is attached on a downstream
>> hub.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Alexander Stein
>>>>>>>>>>> <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>> Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC
>>>>>>>>>>> link to mounted
>>>>>>>>
>>>>>>>> USB HUB
>>>>>>>>
>>>>>>>>>>> on usbh device. Cold plugging an USB mass storage
>>>>>>>>>>> device is working
>>>>>>>>
>>>>>>>> fine.
>>>>>>>>
>>>>>>>>>>> But once the last non-HUB device is disconnected the
>>>>>>>>>>> ci_hdrc device
>>>>>>>>
>>>>>>>> goes
>>>>>>>>
>>>>>>>>>>> into runtime suspend.
>>>>>>>>>>
>>>>>>>>>> Would you please show the difference between cold boot
>>>>>>>>>> and runtime suspend after disconnecting the last USB device?
>>>>>>>>>>
>>>>>>>>>> - Power domain on/off status for HUB device
>>>>>>>>>> - Runtime suspend status at /sys entry for HUB device
>>>>>>>>>> - "/sys/..power/wakeup" /sys entry  for HUB device
>>>>>>>>>
>>>>>>>>> I hope I got all entries you requested.
>>>>>>>>>
>>>>>>>>> For reference this is the bus topology:
>>>>>>>>> lsusb -t
>>>>>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub,
>>>>>>>>> Driver=ci_hdrc/1p, 480M
>>>>>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub,
>>>>>>>>> Driver=ci_hdrc/1p, 480M
>>>>>>>>>
>>>>>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>>>>>>>>>     |
>>>>>>>>>         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
>>>>>>>>>
>>>>>>>>> Driver=usb-storage,
>>>>>>>>
>>>>>>>> 480M
>>>>>>>>
>>>>>>>>> Bus 2 is a different connector and doesn't matter here.
>>>>>>>>> I'm disconnecting
>>>>>>>>
>>>>>>>> 'Dev
>>>>>>>>
>>>>>>>>> 3' in this scenario.
>>>>>>>>>
>>>>>>>>> After boot up with the bus as shown above:
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
>>>>>>>>> disabled
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
>>>>>>>>> active
>>>>>>>>> $ cat
>>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
>>>>>>>>> on
>>>>>>>>>
>>>>>>>>> After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
>>>>>>>>> disconnect, device number 3' in dmesg) the status changes
>>>>>>>>> as follows (without the patch):
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
>>>>>>>>> disabled
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
>>>>>>>>> suspended
>>>>>>>>> $ cat
>>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
>>>>>>>>> off-0
>>>>>>>>>
>>>>>>>>> For the record, when applying the posted patch this
>>>>>>>>> changes
>>>>>>>>> into:
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
>>>>>>>>> disabled
>>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
>>>>>>>>> suspended
>>>>>>>>> $ cat
>>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
>>>>>>>>> on
>>>>>>>>
>>>>>>>> Okay, I think the problem here is the power domain for USB
>>>>>>>> controller is off at runtime, but USB controller/PHY needs
>>>>>>>> to detect the USB wakeup signal at runtime, so the USB
>>>>>>>> controller/PHY's power domain should be not off. The proper
>>>>>>>> change may keep power domain on at runtime, and the power
>>>>>>>> domain
>>>>
>>>> could be off at system suspend.
>>>>
>>>>>>> Can this "hsic phy power domain off breaks wakeup" be confirmed?
>>>>>>> Like with some hack to move hsic phy power domain on some
>>>>>>> other device
>>>>>>> node:
>>>>>>>
>>>>>>> non-usb-node {
>>>>>>>
>>>>>>>         ...
>>>>>>>         power-domains = <&pgc_hsic_phy>;
>>>>>>>         status = "okay";
>>>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> Just make sure this non-usb-node to be runtime active when do
>>>>>>> hsic hub test.
>>>>>>
>>>>>> Thanks for that suggestion. I apparently does work. Using the
>>>>>> this small patch
>>>>>>
>>>>>> --->8---
>>>>>> diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
>>>>>> b/arch/arm/boot/dts/imx7- mba7.dtsi index
>>>>>> b05f662aa87b..cba2f9efa17e
>>>>>> 100644
>>>>>> --- a/arch/arm/boot/dts/imx7-mba7.dtsi
>>>>>> +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
>>>>>> @@ -580,6 +580,7 @@ &uart3 {
>>>>>>
>>>>>>         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
>>>>>>         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
>>>>>>         status = "okay";
>>>>>>
>>>>>> +       power-domains = <&pgc_hsic_phy>;
>>>>>>
>>>>>>  };
>>>>>>
>>>>>>  &uart4 {
>>>>>>
>>>>>> --->8---
>>>>>>
>>>>>> The HSIC power domain is also attached to to uart3.
>>>>>>
>>>>>> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
>>>>>> /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.se
>>>>>> rial /devices/platform/soc/30800000.bus/30b30000.usb
>>>>>> /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
>>>>>> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
>>>>>> on
>>>>>> $ echo on >
>>>>>> /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
>>>>>> 30880000.serial/power/control
>>>>>> $ lsusb -t
>>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
>>>>>> 480M
>>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
>>>>>> 480M
>>>>>>
>>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>>>>>>     |
>>>>>>         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=,
>>>>>> 480M
>>>>>>
>>>>>> [USB
>>>>>>
>>>>>> disconnect] $ cat
>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
>>>>>> on
>>>>>
>>>>> Just want to be sure this was done with hdrc imx runtime PM enabled.
>>>>>
>>>>>> [USB reconnect]
>>>>>> $ lsusb -t
>>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
>>>>>> 480M
>>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
>>>>>> 480M
>>>>>>
>>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>>>>>>     |
>>>>>>         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=,
>>>>>> 480M
>>>>>>
>>>>>> Hot plug detecting still works as you can see the USB device
>>>>>> number increased.
>>>>>>
>>>>>> For the records, there is no difference to this patch in
>>>>>> removing the power domain from USB HSIC device. I just wanted to
>>>>>> keep the diff small.
>>>>>
>>>>> This is good enough to confirm this, thanks.
>>>>>
>>>>> I don't have a HW with HSIC enabled for test, and I am not sure
>>>>> the initial state of power domain is on, can something like below
>>>>> deserve a
>>>>
>>>> try?
>>>>
>>>>> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
>>>>> index
>>>>> 3cb123016b3e..f5467ef18e33 100644
>>>>> --- a/drivers/soc/imx/gpcv2.c
>>>>> +++ b/drivers/soc/imx/gpcv2.c
>>>>> @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
>>>>> imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
>>>>>
>>>>>                 .genpd = {
>>>>>
>>>>>                         .name      = "usb-hsic-phy",
>>>>>
>>>>> +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
>>>>>
>>>>>                 },
>>>>>                 .bits  = {
>>>>>
>>>>>                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@
>>>>> -930,7
>>>>>
>>>>> +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
>>>>> *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
>>>>> domain->bits.map, domain->bits.map);
>>>>>
>>>>> -       ret = pm_genpd_init(&domain->genpd, NULL, true);
>>>>> +       ret = pm_genpd_init(&domain->genpd, NULL,
>>>>> + !(domain->genpd.flags &
>>>>> GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
>>>>>
>>>>>                 dev_err(domain->dev, "Failed to init power domain\n");
>>>>>                 goto out_domain_unmap;
>>>>
>>>> This does indeed the trick. But AFAICS the downside is that the
>>>> powerdomain is enabled, even if USB is not used. Not sure if this is
>>>> acceptable though.
>>>
>>> I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC
>>> port need the power domain on to detect remote wakeup, what's the
>>> exact meaning of "USB is not used"?
>>
>> Exactly, GENPD_FLAG_RPM_ALWAYS_ON is the right thing to to iff the HSIC port
>> needs the powerdomain. But what about the case when HSIC is not enabled at
>> all? That's what I meant by "USB is not used".
>> AFAICS setting GENPD_FLAG_RPM_ALWAYS_ON enables the powerdomain
>> unconditionally from any user.
> 
> If HSIC is not enabled at all, seems the power domain of it will not be touched
> by kernel, so there maybe mismatch between the actual HW state and the SW state,
> but this is another topic.
> 
> For this HSIC case, a second thought I think the better solution maybe
> correct the power domain to its right user, since this power domain
> is for phy, so:
> 
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 008e3da460f1..039eed79d2e7 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -120,6 +120,7 @@ usbphynop3: usbphynop3 {
>                 compatible = "usb-nop-xceiv";
>                 clocks = <&clks IMX7D_USB_HSIC_ROOT_CLK>;
>                 clock-names = "main_clk";
> +               power-domains = <&pgc_hsic_phy>;
>                 #phy-cells = <0>;
>         };
>  
> @@ -1153,7 +1154,6 @@ usbh: usb@30b30000 {
>                                 compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
>                                 reg = <0x30b30000 0x200>;
>                                 interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> -                               power-domains = <&pgc_hsic_phy>;
>                                 clocks = <&clks IMX7D_USB_CTRL_CLK>;
>                                 fsl,usbphy = <&usbphynop3>;
>                                 fsl,usbmisc = <&usbmisc3 0>;
> 
> Could you please try if this can work for you as well?
> 
> Li Jun  

I just want to point out, that on i.MX8MM I'm seeing a similar or even
the same issue for non-HSIC ports. See this thread for more information:
http://lists.infradead.org/pipermail/linux-arm-kernel/2022-April/733370.html.

Jun Li May 9, 2022, 9:53 a.m. UTC | #12

> -----Original Message-----
> From: Frieder Schrempf <[e-mail]>
> Sent: Monday, May 9, 2022 5:28 PM
> To: Jun Li <[e-mail]>; Alexander Stein
> <[e-mail]>; Peter Chen <[e-mail]>
> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> <[e-mail]>; USB list <[e-mail]>;
> [e-mail]
> Subject: Re: (EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
> ci_hdrc_imx: disable runtime pm for HSIC interface
> 
> Am 09.05.22 um 10:16 schrieb Jun Li:
> >
> >
> >> -----Original Message-----
> >> From: Alexander Stein <[e-mail]>
> >> Sent: Friday, May 6, 2022 3:38 PM
> >> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> >> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> >> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> >> Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> >> Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> >> <[e-mail]>; USB list <[e-mail]>;
> >> [e-mail]
> >> Subject: Re: (EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
> >> ci_hdrc_imx: disable runtime pm for HSIC interface
> >>
> >> Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
> >>>> -----Original Message-----
> >>>> From: Alexander Stein <[e-mail]>
> >>>> Sent: Wednesday, May 4, 2022 3:06 PM
> >>>> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> >>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> >>>> <[e-mail]>; Shawn Guo <[e-mail]>;
> >>>> Sascha Hauer <[e-mail]>; Fabio Estevam
> >>>> <[e-mail]>; Pengutronix Kernel Team
> >>>> <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> >>>> <[e-mail]>; [e-mail]
> >>>> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> >>>> disable runtime pm for HSIC interface
> >>>>
> >>>> Helllo,
> >>>>
> >>>> Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> >>>>>> -----Original Message-----
> >>>>>> From: Alexander Stein <[e-mail]>
> >>>>>> Sent: Monday, April 11, 2022 9:52 PM
> >>>>>> To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> >>>>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> >>>>>> <[e-mail]>; Shawn Guo <[e-mail]>;
> >>>>>> Sascha Hauer <[e-mail]>; Fabio Estevam
> >>>>>> <[e-mail]>; Pengutronix Kernel Team
> >>>>>> <[e-mail]>; dl-linux-imx <[e-mail]>; USB
> >>>>>> list <[e-mail]>;
> >>>>>> [e-mail]
> >>>>>> Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> >>>>>> disable runtime pm for HSIC interface
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Peter Chen <[e-mail]>
> >>>>>>>> Sent: Saturday, April 9, 2022 10:20 AM
> >>>>>>>> To: Alexander Stein <[e-mail]>
> >>>>>>>> Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> >>>>>>>> <[e-mail]>; Shawn Guo
> >>>>>>>> <[e-mail]>; Sascha Hauer
> >>>>>>>> <[e-mail]>; Fabio Estevam
> >>>>>>>> <[e-mail]>; Pengutronix Kernel Team
> >>>>>>>> <[e-mail]>; dl-linux-imx <[e-mail]>;
> >>>>>>>> USB list <[e-mail]>;
> >>>>>>>> [e-mail]
> >>>>>>>> Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> >>>>>>>> disable runtime pm for HSIC interface
> >>>>>>>>
> >>>>>>>> On 22-03-29 10:14:36, Alexander Stein wrote:
> >>>>>>>>> Hello Peter,
> >>>>>>>>>
> >>>>>>>>> Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter Chen:
> >>>>>>>>>> On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> >>>>>>>>>>
> >>>>>>>>>> <[e-mail]> wrote:
> >>>>>>>>>>> With the add of power-domain support in commit
> >>>>>>>>>>> 02f8eb40ef7b
> >>>>
> >>>> ("ARM:
> >>>>>>>> dts:
> >>>>>>>>>>> imx7s: Add power domain for imx7d HSIC") runtime
> >>>>>>>>>>> suspend will disable the power-domain. This prevents
> >>>>>>>>>>> IRQs to occur when a new device is attached on a downstream
> >> hub.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Alexander Stein
> >>>>>>>>>>> <[e-mail]>
> >>>>>>>>>>> ---
> >>>>>>>>>>> Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC
> >>>>>>>>>>> link to mounted
> >>>>>>>>
> >>>>>>>> USB HUB
> >>>>>>>>
> >>>>>>>>>>> on usbh device. Cold plugging an USB mass storage
> >>>>>>>>>>> device is working
> >>>>>>>>
> >>>>>>>> fine.
> >>>>>>>>
> >>>>>>>>>>> But once the last non-HUB device is disconnected the
> >>>>>>>>>>> ci_hdrc device
> >>>>>>>>
> >>>>>>>> goes
> >>>>>>>>
> >>>>>>>>>>> into runtime suspend.
> >>>>>>>>>>
> >>>>>>>>>> Would you please show the difference between cold boot
> >>>>>>>>>> and runtime suspend after disconnecting the last USB device?
> >>>>>>>>>>
> >>>>>>>>>> - Power domain on/off status for HUB device
> >>>>>>>>>> - Runtime suspend status at /sys entry for HUB device
> >>>>>>>>>> - "/sys/..power/wakeup" /sys entry  for HUB device
> >>>>>>>>>
> >>>>>>>>> I hope I got all entries you requested.
> >>>>>>>>>
> >>>>>>>>> For reference this is the bus topology:
> >>>>>>>>> lsusb -t
> >>>>>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub,
> >>>>>>>>> Driver=ci_hdrc/1p, 480M
> >>>>>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub,
> >>>>>>>>> Driver=ci_hdrc/1p, 480M
> >>>>>>>>>
> >>>>>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >>>>>>>>>     |
> >>>>>>>>>         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> >>>>>>>>>
> >>>>>>>>> Driver=usb-storage,
> >>>>>>>>
> >>>>>>>> 480M
> >>>>>>>>
> >>>>>>>>> Bus 2 is a different connector and doesn't matter here.
> >>>>>>>>> I'm disconnecting
> >>>>>>>>
> >>>>>>>> 'Dev
> >>>>>>>>
> >>>>>>>>> 3' in this scenario.
> >>>>>>>>>
> >>>>>>>>> After boot up with the bus as shown above:
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> >>>>>>>>> disabled
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> >>>>>>>>> active
> >>>>>>>>> $ cat
> >>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> >>>>>>>>> on
> >>>>>>>>>
> >>>>>>>>> After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> >>>>>>>>> disconnect, device number 3' in dmesg) the status changes
> >>>>>>>>> as follows (without the patch):
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> >>>>>>>>> disabled
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> >>>>>>>>> suspended
> >>>>>>>>> $ cat
> >>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> >>>>>>>>> off-0
> >>>>>>>>>
> >>>>>>>>> For the record, when applying the posted patch this
> >>>>>>>>> changes
> >>>>>>>>> into:
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/wakeup
> >>>>>>>>> disabled
> >>>>>>>>> $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> >>>>>>>>> suspended
> >>>>>>>>> $ cat
> >>>>>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> >>>>>>>>> on
> >>>>>>>>
> >>>>>>>> Okay, I think the problem here is the power domain for USB
> >>>>>>>> controller is off at runtime, but USB controller/PHY needs
> >>>>>>>> to detect the USB wakeup signal at runtime, so the USB
> >>>>>>>> controller/PHY's power domain should be not off. The proper
> >>>>>>>> change may keep power domain on at runtime, and the power
> >>>>>>>> domain
> >>>>
> >>>> could be off at system suspend.
> >>>>
> >>>>>>> Can this "hsic phy power domain off breaks wakeup" be confirmed?
> >>>>>>> Like with some hack to move hsic phy power domain on some
> >>>>>>> other device
> >>>>>>> node:
> >>>>>>>
> >>>>>>> non-usb-node {
> >>>>>>>
> >>>>>>>         ...
> >>>>>>>         power-domains = <&pgc_hsic_phy>;
> >>>>>>>         status = "okay";
> >>>>>>>
> >>>>>>> };
> >>>>>>>
> >>>>>>> Just make sure this non-usb-node to be runtime active when do
> >>>>>>> hsic hub test.
> >>>>>>
> >>>>>> Thanks for that suggestion. I apparently does work. Using the
> >>>>>> this small patch
> >>>>>>
> >>>>>> --->8---
> >>>>>> diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
> >>>>>> b/arch/arm/boot/dts/imx7- mba7.dtsi index
> >>>>>> b05f662aa87b..cba2f9efa17e
> >>>>>> 100644
> >>>>>> --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> >>>>>> +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> >>>>>> @@ -580,6 +580,7 @@ &uart3 {
> >>>>>>
> >>>>>>         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> >>>>>>         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> >>>>>>         status = "okay";
> >>>>>>
> >>>>>> +       power-domains = <&pgc_hsic_phy>;
> >>>>>>
> >>>>>>  };
> >>>>>>
> >>>>>>  &uart4 {
> >>>>>>
> >>>>>> --->8---
> >>>>>>
> >>>>>> The HSIC power domain is also attached to to uart3.
> >>>>>>
> >>>>>> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> >>>>>> /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.se
> >>>>>> rial /devices/platform/soc/30800000.bus/30b30000.usb
> >>>>>> /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> >>>>>> $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> >>>>>> on
> >>>>>> $ echo on >
> >>>>>> /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> >>>>>> 30880000.serial/power/control
> >>>>>> $ lsusb -t
> >>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> >>>>>> 480M
> >>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> >>>>>> 480M
> >>>>>>
> >>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >>>>>>     |
> >>>>>>         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=,
> >>>>>> 480M
> >>>>>>
> >>>>>> [USB
> >>>>>>
> >>>>>> disconnect] $ cat
> >>>>>> /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> >>>>>> on
> >>>>>
> >>>>> Just want to be sure this was done with hdrc imx runtime PM enabled.
> >>>>>
> >>>>>> [USB reconnect]
> >>>>>> $ lsusb -t
> >>>>>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> >>>>>> 480M
> >>>>>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> >>>>>> 480M
> >>>>>>
> >>>>>>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >>>>>>     |
> >>>>>>         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=,
> >>>>>> 480M
> >>>>>>
> >>>>>> Hot plug detecting still works as you can see the USB device
> >>>>>> number increased.
> >>>>>>
> >>>>>> For the records, there is no difference to this patch in
> >>>>>> removing the power domain from USB HSIC device. I just wanted to
> >>>>>> keep the diff small.
> >>>>>
> >>>>> This is good enough to confirm this, thanks.
> >>>>>
> >>>>> I don't have a HW with HSIC enabled for test, and I am not sure
> >>>>> the initial state of power domain is on, can something like below
> >>>>> deserve a
> >>>>
> >>>> try?
> >>>>
> >>>>> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> >>>>> index
> >>>>> 3cb123016b3e..f5467ef18e33 100644
> >>>>> --- a/drivers/soc/imx/gpcv2.c
> >>>>> +++ b/drivers/soc/imx/gpcv2.c
> >>>>> @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
> >>>>> imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
> >>>>>
> >>>>>                 .genpd = {
> >>>>>
> >>>>>                         .name      = "usb-hsic-phy",
> >>>>>
> >>>>> +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
> >>>>>
> >>>>>                 },
> >>>>>                 .bits  = {
> >>>>>
> >>>>>                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@
> >>>>> -930,7
> >>>>>
> >>>>> +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> >>>>> *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> >>>>> domain->bits.map, domain->bits.map);
> >>>>>
> >>>>> -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> >>>>> +       ret = pm_genpd_init(&domain->genpd, NULL,
> >>>>> + !(domain->genpd.flags &
> >>>>> GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
> >>>>>
> >>>>>                 dev_err(domain->dev, "Failed to init power domain\n");
> >>>>>                 goto out_domain_unmap;
> >>>>
> >>>> This does indeed the trick. But AFAICS the downside is that the
> >>>> powerdomain is enabled, even if USB is not used. Not sure if this is
> >>>> acceptable though.
> >>>
> >>> I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC
> >>> port need the power domain on to detect remote wakeup, what's the
> >>> exact meaning of "USB is not used"?
> >>
> >> Exactly, GENPD_FLAG_RPM_ALWAYS_ON is the right thing to to iff the HSIC
> port
> >> needs the powerdomain. But what about the case when HSIC is not enabled
> at
> >> all? That's what I meant by "USB is not used".
> >> AFAICS setting GENPD_FLAG_RPM_ALWAYS_ON enables the powerdomain
> >> unconditionally from any user.
> >
> > If HSIC is not enabled at all, seems the power domain of it will not be
> touched
> > by kernel, so there maybe mismatch between the actual HW state and the
> SW state,
> > but this is another topic.
> >
> > For this HSIC case, a second thought I think the better solution maybe
> > correct the power domain to its right user, since this power domain
> > is for phy, so:
> >
> > diff --git a/arch/arm/boot/dts/imx7s.dtsi
> b/arch/arm/boot/dts/imx7s.dtsi
> > index 008e3da460f1..039eed79d2e7 100644
> > --- a/arch/arm/boot/dts/imx7s.dtsi
> > +++ b/arch/arm/boot/dts/imx7s.dtsi
> > @@ -120,6 +120,7 @@ usbphynop3: usbphynop3 {
> >                 compatible = "usb-nop-xceiv";
> >                 clocks = <&clks IMX7D_USB_HSIC_ROOT_CLK>;
> >                 clock-names = "main_clk";
> > +               power-domains = <&pgc_hsic_phy>;
> >                 #phy-cells = <0>;
> >         };
> >
> > @@ -1153,7 +1154,6 @@ usbh: usb@30b30000 {
> >                                 compatible = "fsl,imx7d-usb",
> "fsl,imx27-usb";
> >                                 reg = <0x30b30000 0x200>;
> >                                 interrupts = <GIC_SPI 40
> IRQ_TYPE_LEVEL_HIGH>;
> > -                               power-domains = <&pgc_hsic_phy>;
> >                                 clocks = <&clks IMX7D_USB_CTRL_CLK>;
> >                                 fsl,usbphy = <&usbphynop3>;
> >                                 fsl,usbmisc = <&usbmisc3 0>;
> >
> > Could you please try if this can work for you as well?
> >
> > Li Jun
> 
> I just want to point out, that on i.MX8MM I'm seeing a similar or even
> the same issue for non-HSIC ports. See this thread for more information:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.
> infradead.org%2Fpipermail%2Flinux-arm-kernel%2F2022-April%2F733370.html
> &amp;data=05%7C01%7Cjun.li%40nxp.com%7C7c3395a1a7dc4faf1a6108da319e2894
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637876852607377524%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=v693uldsFnm4a0wLyt5%2Bq5thp2jKV
> V%2FIGvk2u458vGY%3D&amp;reserved=0.

I am involving that issue discussion too, in both issues, we
need keep the PHY power domain on, I think we have a clear
result the phy power domain should be attached to phy node
to match the actual HW component, apart from this, I cannot
image how that weird test result of "one USB port impact another
port on his board" links to your case.

Li Jun

Alexander Stein May 9, 2022, 10:53 a.m. UTC | #13

Am Montag, 9. Mai 2022, 10:16:44 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <[e-mail]>
> > Sent: Friday, May 6, 2022 3:38 PM
> > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> > Hauer <[e-mail]>; Fabio Estevam <[e-mail]>;
> > Pengutronix Kernel Team <[e-mail]>; dl-linux-imx
> > <[e-mail]>; USB list <[e-mail]>;
> > [e-mail]
> > Subject: Re: (EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
> > ci_hdrc_imx: disable runtime pm for HSIC interface
> > 
> > Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
> > > > -----Original Message-----
> > > > From: Alexander Stein <[e-mail]>
> > > > Sent: Wednesday, May 4, 2022 3:06 PM
> > > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > <[e-mail]>; Pengutronix Kernel Team
> > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB list
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > disable runtime pm for HSIC interface
> > > > 
> > > > Helllo,
> > > > 
> > > > Am Dienstag, 12. April 2022, 13:36:55 CEST schrieb Jun Li:
> > > > > > -----Original Message-----
> > > > > > From: Alexander Stein <[e-mail]>
> > > > > > Sent: Monday, April 11, 2022 9:52 PM
> > > > > > To: Peter Chen <[e-mail]>; Jun Li <[e-mail]>
> > > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > > <[e-mail]>; Shawn Guo <[e-mail]>;
> > > > > > Sascha Hauer <[e-mail]>; Fabio Estevam
> > > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > > <[e-mail]>; dl-linux-imx <[e-mail]>; USB
> > > > > > list <[e-mail]>;
> > > > > > [e-mail]
> > > > > > Subject: Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea:
> > > > > > ci_hdrc_imx:
> > > > > > disable runtime pm for HSIC interface
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Peter Chen <[e-mail]>
> > > > > > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > > > > > To: Alexander Stein <[e-mail]>
> > > > > > > > Cc: Peter Chen <[e-mail]>; Greg Kroah-Hartman
> > > > > > > > <[e-mail]>; Shawn Guo
> > > > > > > > <[e-mail]>; Sascha Hauer
> > > > > > > > <[e-mail]>; Fabio Estevam
> > > > > > > > <[e-mail]>; Pengutronix Kernel Team
> > > > > > > > <[e-mail]>; dl-linux-imx <[e-mail]>;
> > > > > > > > USB list <[e-mail]>;
> > > > > > > > [e-mail]
> > > > > > > > Subject: Re: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx:
> > > > > > > > disable runtime pm for HSIC interface
> > > > > > > > 
> > > > > > > > On 22-03-29 10:14:36, Alexander Stein wrote:
> > > > > > > > > Hello Peter,
> > > > > > > > > 
> > > > > > > > > Am Dienstag, 15. März 2022, 02:23:23 CEST schrieb Peter 
Chen:
> > > > > > > > > > On Wed, Mar 2, 2022 at 5:42 PM Alexander Stein
> > > > > > > > > > 
> > > > > > > > > > <[e-mail]> wrote:
> > > > > > > > > > > With the add of power-domain support in commit
> > > > > > > > > > > 02f8eb40ef7b
> > > > 
> > > > ("ARM:
> > > > > > > > dts:
> > > > > > > > > > > imx7s: Add power domain for imx7d HSIC") runtime
> > > > > > > > > > > suspend will disable the power-domain. This prevents
> > > > > > > > > > > IRQs to occur when a new device is attached on a
> > > > > > > > > > > downstream
> > 
> > hub.
> > 
> > > > > > > > > > > Signed-off-by: Alexander Stein
> > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > > Our board TQMa7x + MBa7x (i.MX7 based) uses a HSIC
> > > > > > > > > > > link to mounted
> > > > > > > > 
> > > > > > > > USB HUB
> > > > > > > > 
> > > > > > > > > > > on usbh device. Cold plugging an USB mass storage
> > > > > > > > > > > device is working
> > > > > > > > 
> > > > > > > > fine.
> > > > > > > > 
> > > > > > > > > > > But once the last non-HUB device is disconnected the
> > > > > > > > > > > ci_hdrc device
> > > > > > > > 
> > > > > > > > goes
> > > > > > > > 
> > > > > > > > > > > into runtime suspend.
> > > > > > > > > > 
> > > > > > > > > > Would you please show the difference between cold boot
> > > > > > > > > > and runtime suspend after disconnecting the last USB
> > > > > > > > > > device?
> > > > > > > > > > 
> > > > > > > > > > - Power domain on/off status for HUB device
> > > > > > > > > > - Runtime suspend status at /sys entry for HUB device
> > > > > > > > > > - "/sys/..power/wakeup" /sys entry  for HUB device
> > > > > > > > > 
> > > > > > > > > I hope I got all entries you requested.
> > > > > > > > > 
> > > > > > > > > For reference this is the bus topology:
> > > > > > > > > lsusb -t
> > > > > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub,
> > > > > > > > > Driver=ci_hdrc/1p, 480M
> > > > > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub,
> > > > > > > > > Driver=ci_hdrc/1p, 480M
> > > > > > > > > 
> > > > > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > > > > >     |
> > > > > > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage,
> > > > > > > > > 
> > > > > > > > > Driver=usb-storage,
> > > > > > > > 
> > > > > > > > 480M
> > > > > > > > 
> > > > > > > > > Bus 2 is a different connector and doesn't matter here.
> > > > > > > > > I'm disconnecting
> > > > > > > > 
> > > > > > > > 'Dev
> > > > > > > > 
> > > > > > > > > 3' in this scenario.
> > > > > > > > > 
> > > > > > > > > After boot up with the bus as shown above:
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > > disabled
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > > active
> > > > > > > > > $ cat
> > > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > > on
> > > > > > > > > 
> > > > > > > > > After disconnecting Dev 3 from the bus ('usb 1-1.2: USB
> > > > > > > > > disconnect, device number 3' in dmesg) the status changes
> > > > > > > > > as follows (without the patch):
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > > disabled
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > > suspended
> > > > > > > > > $ cat
> > > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > > off-0
> > > > > > > > > 
> > > > > > > > > For the record, when applying the posted patch this
> > > > > > > > > changes
> > > > > > > > > into:
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/wakeup
> > > > > > > > > disabled
> > > > > > > > > $ cat /sys/bus/usb/devices/1-1/power/runtime_status
> > > > > > > > > suspended
> > > > > > > > > $ cat
> > > > > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > > > > on
> > > > > > > > 
> > > > > > > > Okay, I think the problem here is the power domain for USB
> > > > > > > > controller is off at runtime, but USB controller/PHY needs
> > > > > > > > to detect the USB wakeup signal at runtime, so the USB
> > > > > > > > controller/PHY's power domain should be not off. The proper
> > > > > > > > change may keep power domain on at runtime, and the power
> > > > > > > > domain
> > > > 
> > > > could be off at system suspend.
> > > > 
> > > > > > > Can this "hsic phy power domain off breaks wakeup" be confirmed?
> > > > > > > Like with some hack to move hsic phy power domain on some
> > > > > > > other device
> > > > > > > node:
> > > > > > > 
> > > > > > > non-usb-node {
> > > > > > > 
> > > > > > >         ...
> > > > > > >         power-domains = <&pgc_hsic_phy>;
> > > > > > >         status = "okay";
> > > > > > > 
> > > > > > > };
> > > > > > > 
> > > > > > > Just make sure this non-usb-node to be runtime active when do
> > > > > > > hsic hub test.
> > > > > > 
> > > > > > Thanks for that suggestion. I apparently does work. Using the
> > > > > > this small patch
> > > > > > 
> > > > > > --->8---
> > > > > > diff --git a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > > b/arch/arm/boot/dts/imx7- mba7.dtsi index
> > > > > > b05f662aa87b..cba2f9efa17e
> > > > > > 100644
> > > > > > --- a/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > > +++ b/arch/arm/boot/dts/imx7-mba7.dtsi
> > > > > > @@ -580,6 +580,7 @@ &uart3 {
> > > > > > 
> > > > > >         assigned-clocks = <&clks IMX7D_UART3_ROOT_SRC>;
> > > > > >         assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
> > > > > >         status = "okay";
> > > > > > 
> > > > > > +       power-domains = <&pgc_hsic_phy>;
> > > > > > 
> > > > > >  };
> > > > > >  
> > > > > >  &uart4 {
> > > > > > 
> > > > > > --->8---
> > > > > > 
> > > > > > The HSIC power domain is also attached to to uart3.
> > > > > > 
> > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/devices
> > > > > > /devices/platform/soc/30800000.bus/30800000.spba-bus/30880000.se
> > > > > > rial /devices/platform/soc/30800000.bus/30b30000.usb
> > > > > > /devices/platform/soc/30800000.bus/30b30000.usb/ci_hdrc.1
> > > > > > $ cat /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > on
> > > > > > $ echo on >
> > > > > > /sys/devices/platform/soc/30800000.bus/30800000.spba-bus/
> > > > > > 30880000.serial/power/control
> > > > > > $ lsusb -t
> > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > > 
> > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > >     |
> > > > > >         |__ Port 2: Dev 3, If 0, Class=Mass Storage, Driver=,
> > > > > > 
> > > > > > 480M
> > > > > > 
> > > > > > [USB
> > > > > > 
> > > > > > disconnect] $ cat
> > > > > > /sys/kernel/debug/pm_genpd/usb-hsic-phy/current_state
> > > > > > on
> > > > > 
> > > > > Just want to be sure this was done with hdrc imx runtime PM enabled.
> > > > > 
> > > > > > [USB reconnect]
> > > > > > $ lsusb -t
> > > > > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p,
> > > > > > 480M
> > > > > > 
> > > > > >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> > > > > >     |
> > > > > >         |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=,
> > > > > > 
> > > > > > 480M
> > > > > > 
> > > > > > Hot plug detecting still works as you can see the USB device
> > > > > > number increased.
> > > > > > 
> > > > > > For the records, there is no difference to this patch in
> > > > > > removing the power domain from USB HSIC device. I just wanted to
> > > > > > keep the diff small.
> > > > > 
> > > > > This is good enough to confirm this, thanks.
> > > > > 
> > > > > I don't have a HW with HSIC enabled for test, and I am not sure
> > > > > the initial state of power domain is on, can something like below
> > > > > deserve a
> > > > 
> > > > try?
> > > > 
> > > > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > > > index
> > > > > 3cb123016b3e..f5467ef18e33 100644
> > > > > --- a/drivers/soc/imx/gpcv2.c
> > > > > +++ b/drivers/soc/imx/gpcv2.c
> > > > > @@ -416,6 +416,7 @@ static const struct imx_pgc_domain
> > > > > imx7_pgc_domains[] = { [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = {
> > > > > 
> > > > >                 .genpd = {
> > > > >                 
> > > > >                         .name      = "usb-hsic-phy",
> > > > > 
> > > > > +                       .flags     = GENPD_FLAG_RPM_ALWAYS_ON,
> > > > > 
> > > > >                 },
> > > > >                 .bits  = {
> > > > >                 
> > > > >                         .pxx = IMX7_USB_HSIC_PHY_SW_Pxx_REQ, @@
> > > > > 
> > > > > -930,7
> > > > > 
> > > > > +931,7 @@ static int imx_pgc_domain_probe(struct platform_device
> > > > > *pdev) regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > > > > domain->bits.map, domain->bits.map);
> > > > > 
> > > > > -       ret = pm_genpd_init(&domain->genpd, NULL, true);
> > > > > +       ret = pm_genpd_init(&domain->genpd, NULL,
> > > > > + !(domain->genpd.flags &
> > > > > GENPD_FLAG_RPM_ALWAYS_ON)); if (ret) {
> > > > > 
> > > > >                 dev_err(domain->dev, "Failed to init power
> > > > >                 domain\n");
> > > > >                 goto out_domain_unmap;
> > > > 
> > > > This does indeed the trick. But AFAICS the downside is that the
> > > > powerdomain is enabled, even if USB is not used. Not sure if this is
> > > > acceptable though.
> > > 
> > > I think GENPD_FLAG_RPM_ALWAYS_ON is the right thing to do if the HSIC
> > > port need the power domain on to detect remote wakeup, what's the
> > > exact meaning of "USB is not used"?
> > 
> > Exactly, GENPD_FLAG_RPM_ALWAYS_ON is the right thing to to iff the HSIC
> > port needs the powerdomain. But what about the case when HSIC is not
> > enabled at all? That's what I meant by "USB is not used".
> > AFAICS setting GENPD_FLAG_RPM_ALWAYS_ON enables the powerdomain
> > unconditionally from any user.
> 
> If HSIC is not enabled at all, seems the power domain of it will not be
> touched by kernel, so there maybe mismatch between the actual HW state and
> the SW state, but this is another topic.
> 
> For this HSIC case, a second thought I think the better solution maybe
> correct the power domain to its right user, since this power domain
> is for phy, so:
> 
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 008e3da460f1..039eed79d2e7 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -120,6 +120,7 @@ usbphynop3: usbphynop3 {
>                 compatible = "usb-nop-xceiv";
>                 clocks = <&clks IMX7D_USB_HSIC_ROOT_CLK>;
>                 clock-names = "main_clk";
> +               power-domains = <&pgc_hsic_phy>;
>                 #phy-cells = <0>;
>         };
> 
> @@ -1153,7 +1154,6 @@ usbh: usb@30b30000 {
>                                 compatible = "fsl,imx7d-usb",
> "fsl,imx27-usb"; reg = <0x30b30000 0x200>;
>                                 interrupts = <GIC_SPI 40
> IRQ_TYPE_LEVEL_HIGH>; -                               power-domains =
> <&pgc_hsic_phy>;
>                                 clocks = <&clks IMX7D_USB_CTRL_CLK>;
>                                 fsl,usbphy = <&usbphynop3>;
>                                 fsl,usbmisc = <&usbmisc3 0>;
> 
> Could you please try if this can work for you as well?

This does indeed work as well. If you both disable 'usbh' and 'usbphynop3', 
the power domain is switched off as well.
I'll create a proper patch for this.

Alexander

Patch

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 097142ffb184..e5c22b70431c 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -344,6 +344,8 @@  static int ci_hdrc_imx_probe(struct platform_device *pdev)
 	if ((of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC)
 		&& data->usbmisc_data) {
 		pdata.flags |= CI_HDRC_IMX_IS_HSIC;
+		/* Runtime suspend is not supported for HSIC interface */
+		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
 		data->usbmisc_data->hsic = 1;
 		data->pinctrl = devm_pinctrl_get(dev);
 		if (PTR_ERR(data->pinctrl) == -ENODEV)