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&data=02%7C01%7Cpeter.chen%40nxp.com%7Cad9b3833ae2f433d93ef08d7bb92d4a0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637184111614326500&sdata=SPwwBEGBco6IdP8ufmAnJeeRxuAXGLa0xzYlzFA%2FAvg%3D&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
