Re: [PATCH 0/3] USB IMX Chipidea fix gpio vbus control

From: Peter Chen <peter.chen@nxp.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: "jun.li@freescale.com" <jun.li@freescale.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 0/3] USB IMX Chipidea fix gpio vbus control
Date: Fri, 28 Feb 2020 02:51:27 +0000	[thread overview]
Message-ID: <20200228025129.GA31815@b29397-desktop> (raw)
In-Reply-To: <20200227143914.mi3vsltrtyo5sqed@pengutronix.de>

On 20-02-27 15:39:14, Marco Felsch wrote:
> Hi Peter,
> 
> On 20-02-27 13:30, Peter Chen wrote:
> >   
> > > > >
> > > > > Note, I'm using a imx6q which has the CI_HDRC_TURN_VBUS_EARLY_ON set.
> > > > >
> > > >
> > > > Do you have a VBUS regulator at your dts, and add it at controller's
> > > > node? See: arch/arm/boot/dts/imx6qdl-sabresd.dtsi as an example please?
> > > 
> > > Yes, that's my use case too.
> > > 
> > > > If you have set CI_HDRC_TURN_VBUS_EARLY_ON, the VBUS is controlled by
> > > > chipidea driver, not by USB core through PORTSC.PP (ehci_ci_portpower).
> > > 
> > > I know, pls have a look my the patches.
> > > 
> > > I had the problem that the vbus regulator wasn't turned off during a
> > > suspend/resume logic. The first issue within the usb-core should be fixed by [1] (v2
> > > RFC is on the way). You never run in that case if you don't fix this. After I fixed it
> > > the port-power is called during suspend but obviously the regulator didn't get turned
> > > off because we don't add it to the priv->reg_vbus.
> > > 
> > > This patchset should fix this and get rid of the CI_HDRC_TURN_VBUS_EARLY_ON
> > > flag.
> > > 
> >  
> > Hi Marco,
> > 
> > I may understand your case now. At old USB port design, the VBUS is never allowed to
> > turned off to response the USB wakeup event. But the expected behavior has changed
> > after pm_qos_no_power_off is introduced for USB port, it is allowed the port is powered off.
> 
> Luckily we have git :) and I my archeological findings are:
> 
>  0ero Patch 2012-07-07) 1530280084c3 usb: chipidea: add imx platform driver
>  1st  Patch 2012-10-23) ae0fb4b72c8d PM / QoS: Introduce PM QoS device flags support
>  2nd  Patch 2013-01-23) ad493e5e5805 usb: add usb port auto power off mechanism
>  3th  Patch 2014-10-13) 11a7e5940514 usb: ehci: add ehci_port_power interface
>  4th  Patch 2014-10-13) c8679a2fb8de usb: chipidea: host: add portpower override
>  5th  Patch 2015-02-11) 6adb9b7b5fb6 usb: chipidea: add a flag for turn on vbus early for host
>  6th  Patch 2015-02-11) 659459174188 usb: chipidea: host: turn on vbus before add hcd if early vbus on is required
> 
> A few more details:
> - Since day 0 the imx chipidea driver supports the regulator but it was
>   only used to turn it on (0ero Patch). Later the regulator support was
>   shifted to the core and optional.
> - 1-2 Patch added the pm_qos_no_power_off support
> - 3-4 Patch adds the support to disable the regulator
> - 5-6 Patch adding a workaround for patches 3-4 which breaks the
>   regulator power-off support.
> 

Thanks for tracking it so detail.

1-2: pm_qos_no_power_off is default on, and it is a standard USB working
way. In fact, no customer mentioned that before, and even for me, it is
the first time to try it.
3-4: It is not adding support for gpio-base VBUS control for chipidea,
it lets the USB core control the VBUS instead of controller driver.
5-6: With patch 3-4 introduced, we found issues at imx6 series SoCs
(see detail at the last reply), so we add quirk for imx6.

> So as you can see the pm_qos_no_power_off was introduced before the gpio
> regulator vbus power-off support was added.
> 

Like I said it is default off, we don't know there is such feature
at kernel. In my mind, the VBUS is never off for host.

> > PORTSC.PP could be controlled by USB core, but USB VBUS's power is not controlled
> > by this bit if the VBUS power enable pin is configured as GPIO function, that is your case.
> 
> Yep I know :)
> 
> > CI_HDRC_TURN_VBUS_EARLY_ON is introduced by fixing a bug that some i.mx USB
> > controllers PHY's power is sourced from VBUS, the PHY's power need to be on before
> > touch some ehci registers, otherwise, the USB signal will be wrong at some low speed
> > devices use case. So, this flag can't be deleted, it may cause regression.
> 
> Pls check my archeological findings and again pls check my patches. I
> deleted the flag because isn't required anymore afterwards.

I have already checked your patch, your patch deletes CI_HDRC_TURN_VBUS_EARLY_ON
quirk, and it may cause regression.

> 
> > The solution I see is your may need to implement chipidea VBUS control flow by considering
> > pm_qos_no_power_off at suspend situation. You may add .suspend API for ci_role_driver,
> > and called by ci_controller_suspend/ci_controller_resume, of cos, better solution is welcome.
> 
> I fixed it within the core [1] and here at the chipidea side.
> 
> [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F2%2F27%2F669&amp;data=02%7C01%7Cpeter.chen%40nxp.com%7Cad9b3833ae2f433d93ef08d7bb92d4a0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637184111614326500&amp;sdata=SPwwBEGBco6IdP8ufmAnJeeRxuAXGLa0xzYlzFA%2FAvg%3D&amp;reserved=0
> 
> You will never enter the ehci_ci_portpower() during suspend without [1]
> if you are using a vanilla kernel. So IMHO this case can't be tested,
> sorry.
> 

Through set pm_qos_no_power_off as 0, the VBUS will be off. You
never need to call .ehci_ci_portpower again. You may try my second
suggestion for fix chipidea issue. I will reply your RFC patch for
USB core.

-- 

Thanks,
Peter Chen