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

From: Marco Felsch <m.felsch@pengutronix.de>
To: Peter Chen <peter.chen@nxp.com>
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: Thu, 27 Feb 2020 15:39:14 +0100	[thread overview]
Message-ID: <20200227143914.mi3vsltrtyo5sqed@pengutronix.de> (raw)
In-Reply-To: <VI1PR04MB53270541BB66CAB1EB8F00008BEB0@VI1PR04MB5327.eurprd04.prod.outlook.com>

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.

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

> 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.

> 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://lkml.org/lkml/2020/2/27/669

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.

Kind regards,
  Marco

> Peter
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |