Re: (EXT) 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>]

Am Freitag, 6. Mai 2022, 09:09:22 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Sent: Wednesday, May 4, 2022 3:06 PM
> > To: Peter Chen <peter.chen@kernel.org>; Jun Li <jun.li@nxp.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: (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 <alexander.stein@ew.tq-group.com>
> > > > Sent: Monday, April 11, 2022 9:52 PM
> > > > To: Peter Chen <peter.chen@kernel.org>; Jun Li <jun.li@nxp.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: (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 <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
> > > 
> > > 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