(EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable runtime pm for HSIC interface
Jun Li
jun.li at nxp.com
Mon May 9 02:53:45 PDT 2022
> -----Original Message-----
> From: Frieder Schrempf <frieder.schrempf at kontron.de>
> Sent: Monday, May 9, 2022 5:28 PM
> To: Jun Li <jun.li at nxp.com>; Alexander Stein
> <alexander.stein at ew.tq-group.com>; Peter Chen <peter.chen at kernel.org>
> 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 09.05.22 um 10:16 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?
> >
> > 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
> &data=05%7C01%7Cjun.li%40nxp.com%7C7c3395a1a7dc4faf1a6108da319e2894
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637876852607377524%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=v693uldsFnm4a0wLyt5%2Bq5thp2jKV
> V%2FIGvk2u458vGY%3D&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
More information about the linux-arm-kernel
mailing list