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

[Peter Chen <hzpeterchen@gmail.com>]

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

Peter

> This will eventually also disable the 'pgc_hsic_phy' power
> domain. Results is that no more updates from USB hub is handled, neither HUB
> on HSIC link or HUB on that's downstream link. USB tree looks like this:
>
> /:  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 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
>             |__ Port 1: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>         |__ Port 2: Dev 5, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>
> I noticed once the power domain is disabled, no IRQs appear if I attach a new
> mass storage, essentially preventing any runtime resume.
> I do not know if this is specific to i.MX7 only or if this is a general USB
> HSIC problem, so this diff might be too much.
>
> BTW: An udev rule with the same effect is:
> ACTION=="add", SUBSYSTEMS=="platform", DRIVER=="ci_hdrc",
> KERNELS=="30b30000.usb", TEST=="power/control", ATTR{power/control}="on"
>
> But I would like to get this fixed on driver level.
>
> Regards
> Alexander
>
>  drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 097142ffb184..e5c22b70431c 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -344,6 +344,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>         if ((of_usb_get_phy_mode(dev->of_node) == USBPHY_INTERFACE_MODE_HSIC)
>                 && data->usbmisc_data) {
>                 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
> +               /* Runtime suspend is not supported for HSIC interface */
> +               pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
>                 data->usbmisc_data->hsic = 1;
>                 data->pinctrl = devm_pinctrl_get(dev);
>                 if (PTR_ERR(data->pinctrl) == -ENODEV)
> --
> 2.25.1
>