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

From: Peter Chen <peter.chen@kernel.org>
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>,
	NXP Linux Team <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
Date: Sat, 9 Apr 2022 10:19:48 +0800	[thread overview]
Message-ID: <20220409021948.GA3618@Peter> (raw)
In-Reply-To: <1891703.PYKUYFuaPT@steina-w>

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.

The controller/PHY clk could be off at runtime, it does not affect the
wakeup detecting from controll/PHY side.

-- 

Thanks,
Peter Chen