(EXT) 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 at ew.tq-group.com
Mon May 9 03:53:30 PDT 2022
Am Montag, 9. Mai 2022, 10:16:44 CEST schrieb Jun Li:
> > -----Original Message-----
> > From: Alexander Stein <alexander.stein at ew.tq-group.com>
> > Sent: Friday, May 6, 2022 3:38 PM
> > To: Peter Chen <peter.chen at kernel.org>; Jun Li <jun.li at nxp.com>
> > Cc: Peter Chen <hzpeterchen at gmail.com>; Greg Kroah-Hartman
> > <gregkh at linuxfoundation.org>; Shawn Guo <shawnguo at kernel.org>; Sascha
> > Hauer <s.hauer at pengutronix.de>; Fabio Estevam <festevam at gmail.com>;
> > Pengutronix Kernel Team <kernel at pengutronix.de>; dl-linux-imx
> > <linux-imx at nxp.com>; USB list <linux-usb at vger.kernel.org>;
> > linux-arm-kernel at lists.infradead.org
> > 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 <alexander.stein at ew.tq-group.com>
> > > > Sent: Wednesday, May 4, 2022 3:06 PM
> > > > To: Peter Chen <peter.chen at kernel.org>; Jun Li <jun.li at nxp.com>
> > > > Cc: Peter Chen <hzpeterchen at gmail.com>; Greg Kroah-Hartman
> > > > <gregkh at linuxfoundation.org>; Shawn Guo <shawnguo at kernel.org>;
> > > > Sascha Hauer <s.hauer at pengutronix.de>; Fabio Estevam
> > > > <festevam at gmail.com>; Pengutronix Kernel Team
> > > > <kernel at pengutronix.de>; dl-linux-imx <linux-imx at nxp.com>; USB list
> > > > <linux-usb at vger.kernel.org>; linux-arm-kernel at 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 at ew.tq-group.com>
> > > > > > Sent: Monday, April 11, 2022 9:52 PM
> > > > > > To: Peter Chen <peter.chen at kernel.org>; Jun Li <jun.li at nxp.com>
> > > > > > Cc: Peter Chen <hzpeterchen at gmail.com>; Greg Kroah-Hartman
> > > > > > <gregkh at linuxfoundation.org>; Shawn Guo <shawnguo at kernel.org>;
> > > > > > Sascha Hauer <s.hauer at pengutronix.de>; Fabio Estevam
> > > > > > <festevam at gmail.com>; Pengutronix Kernel Team
> > > > > > <kernel at pengutronix.de>; dl-linux-imx <linux-imx at nxp.com>; USB
> > > > > > list <linux-usb at vger.kernel.org>;
> > > > > > linux-arm-kernel at 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 at kernel.org>
> > > > > > > > Sent: Saturday, April 9, 2022 10:20 AM
> > > > > > > > To: Alexander Stein <alexander.stein at ew.tq-group.com>
> > > > > > > > Cc: Peter Chen <hzpeterchen at gmail.com>; Greg Kroah-Hartman
> > > > > > > > <gregkh at linuxfoundation.org>; Shawn Guo
> > > > > > > > <shawnguo at kernel.org>; Sascha Hauer
> > > > > > > > <s.hauer at pengutronix.de>; Fabio Estevam
> > > > > > > > <festevam at gmail.com>; Pengutronix Kernel Team
> > > > > > > > <kernel at pengutronix.de>; dl-linux-imx <linux-imx at nxp.com>;
> > > > > > > > USB list <linux-usb at vger.kernel.org>;
> > > > > > > > linux-arm-kernel at 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 at 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 at 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.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 at 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
More information about the linux-arm-kernel
mailing list