Re: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable runtime pm for HSIC interface

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi,

Am Samstag, 9. April 2022, 06:49:54 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Peter Chen <peter.chen@kernel.org>
> > Sent: Saturday, April 9, 2022 10:20 AM
> > To: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Cc: Peter Chen <hzpeterchen@gmail.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
> > Hauer <s.hauer@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx
> > <linux-imx@nxp.com>; USB list <linux-usb@vger.kernel.org>;
> > linux-arm-kernel@lists.infradead.org
> > 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
> > > > 
> > > > <alexander.stein@ew.tq-group.com> 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 <alexander.stein@ew.tq-group.com>
> > > > > ---
> > > > > 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