(EXT) RE: (EXT) RE: (EXT) Re: [RFC 1/1] usb: chipidea: ci_hdrc_imx: disable runtime pm for HSIC interface
Frieder Schrempf
frieder.schrempf at kontron.de
Mon May 9 02:27:34 PDT 2022
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:
http://lists.infradead.org/pipermail/linux-arm-kernel/2022-April/733370.html.
More information about the linux-arm-kernel
mailing list