[0/3] USB IMX Chipidea fix gpio vbus control
mbox series

Message ID 20200227104212.12562-1-m.felsch@pengutronix.de
Headers show
Series
  • USB IMX Chipidea fix gpio vbus control
Related show

Message

Marco Felsch Feb. 27, 2020, 10:42 a.m. UTC
Hi,

the gpio-based port power is broken since commit [1,2]. I changed the
core behaviour to to cleanup the code and avoid local workaround fixes.

With this series and [3] I'am now able to disable the port completely
after I disbaled the pm_qos_no_power_off flag.

[1] 6adb9b7b5fb6 ("usb: chipidea: add a flag for turn on vbus early for host")
[2] 659459174188 ("usb: chipidea: host: turn on vbus before add hcd if early vbus on is required")
[3] https://lkml.org/lkml/2020/2/27/198

Regards,
  Marco

Marco Felsch (3):
  USB: ehci-hub: let port_power() override the ehci_port_power()
  Partially Revert "usb: chipidea: host: turn on vbus before add hcd if
    early vbus on is required"
  Revert "usb: chipidea: add a flag for turn on vbus early for host"

 drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++--------
 drivers/usb/chipidea/host.c        | 31 ++++++++++--------------------
 drivers/usb/host/ehci-hub.c        |  6 +++---
 include/linux/usb/chipidea.h       | 17 ++++++++--------
 4 files changed, 23 insertions(+), 41 deletions(-)

Comments

Peter Chen Feb. 27, 2020, 11:18 a.m. UTC | #1
On 20-02-27 11:42:09, Marco Felsch wrote:
> Hi,
> 
> the gpio-based port power is broken since commit [1,2]. I changed the
> core behaviour to to cleanup the code and avoid local workaround fixes.

Many i.mx series evk boards work well for gpio-based port power control,
what problem you have found, would you please list it detail?

Peter
> 
> With this series and [3] I'am now able to disable the port completely
> after I disbaled the pm_qos_no_power_off flag.
> 
> [1] 6adb9b7b5fb6 ("usb: chipidea: add a flag for turn on vbus early for host")
> [2] 659459174188 ("usb: chipidea: host: turn on vbus before add hcd if early vbus on is required")
> [3] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F2%2F27%2F198&data=02%7C01%7Cpeter.chen%40nxp.com%7Cc1b3094e826b49b572cb08d7bb71b6bc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637183969379126557&sdata=i7O7vBqiVCb2opVNva7ffxIdqFb%2BvGzV5Qsy5wwarbQ%3D&reserved=0
> 
> Regards,
>   Marco
> 
> Marco Felsch (3):
>   USB: ehci-hub: let port_power() override the ehci_port_power()
>   Partially Revert "usb: chipidea: host: turn on vbus before add hcd if
>     early vbus on is required"
>   Revert "usb: chipidea: add a flag for turn on vbus early for host"
> 
>  drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++--------
>  drivers/usb/chipidea/host.c        | 31 ++++++++++--------------------
>  drivers/usb/host/ehci-hub.c        |  6 +++---
>  include/linux/usb/chipidea.h       | 17 ++++++++--------
>  4 files changed, 23 insertions(+), 41 deletions(-)
> 
> -- 
> 2.20.1
>
Marco Felsch Feb. 27, 2020, 11:35 a.m. UTC | #2
Hi Peter,

thanks for your fast reply :)

On 20-02-27 11:18, Peter Chen wrote:
> On 20-02-27 11:42:09, Marco Felsch wrote:
> > Hi,
> > 
> > the gpio-based port power is broken since commit [1,2]. I changed the
> > core behaviour to to cleanup the code and avoid local workaround fixes.
> 
> Many i.mx series evk boards work well for gpio-based port power control,
> what problem you have found, would you please list it detail?

Hm.. who could that work? I picked the important parts:

static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
{
	...

	if (priv->reg_vbus && enable != priv->enabled) {

		...

		if (enable)
			ret = regulator_enable(priv->reg_vbus);
		else
			ret = regulator_disable(priv->reg_vbus);

		...
	}

	...
}

static int host_start(struct ci_hdrc *ci)
{
	...

	priv->reg_vbus = NULL;

	if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci)) {
		if (ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON) {
			ret = regulator_enable(ci->platdata->reg_vbus);
			if (ret) {
				dev_err(ci->dev,
				"Failed to enable vbus regulator, ret=%d\n",
									ret);
				goto put_hcd;
			}
		} else {
			priv->reg_vbus = ci->platdata->reg_vbus;
		}
	}

	...
}

Note, I'm using a imx6q which has the CI_HDRC_TURN_VBUS_EARLY_ON set.

Regards,
  Marco

> Peter
> > 
> > With this series and [3] I'am now able to disable the port completely
> > after I disbaled the pm_qos_no_power_off flag.
> > 
> > [1] 6adb9b7b5fb6 ("usb: chipidea: add a flag for turn on vbus early for host")
> > [2] 659459174188 ("usb: chipidea: host: turn on vbus before add hcd if early vbus on is required")
> > [3] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F2%2F27%2F198&data=02%7C01%7Cpeter.chen%40nxp.com%7Cc1b3094e826b49b572cb08d7bb71b6bc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637183969379126557&sdata=i7O7vBqiVCb2opVNva7ffxIdqFb%2BvGzV5Qsy5wwarbQ%3D&reserved=0
> > 
> > Regards,
> >   Marco
> > 
> > Marco Felsch (3):
> >   USB: ehci-hub: let port_power() override the ehci_port_power()
> >   Partially Revert "usb: chipidea: host: turn on vbus before add hcd if
> >     early vbus on is required"
> >   Revert "usb: chipidea: add a flag for turn on vbus early for host"
> > 
> >  drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++--------
> >  drivers/usb/chipidea/host.c        | 31 ++++++++++--------------------
> >  drivers/usb/host/ehci-hub.c        |  6 +++---
> >  include/linux/usb/chipidea.h       | 17 ++++++++--------
> >  4 files changed, 23 insertions(+), 41 deletions(-)
> > 
> > -- 
> > 2.20.1
> > 
> 
> -- 
> 
> Thanks,
> Peter Chen
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Peter Chen Feb. 27, 2020, 12:20 p.m. UTC | #3
On 20-02-27 12:35:39, Marco Felsch wrote:
> Hi Peter,
> 
> thanks for your fast reply :)
> 
> On 20-02-27 11:18, Peter Chen wrote:
> > On 20-02-27 11:42:09, Marco Felsch wrote:
> > > Hi,
> > > 
> > > the gpio-based port power is broken since commit [1,2]. I changed the
> > > core behaviour to to cleanup the code and avoid local workaround fixes.
> > 
> > Many i.mx series evk boards work well for gpio-based port power control,
> > what problem you have found, would you please list it detail?
> 
> Hm.. who could that work? I picked the important parts:
> 
> static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
> {
> 	...
> 
> 	if (priv->reg_vbus && enable != priv->enabled) {
> 
> 		...
> 
> 		if (enable)
> 			ret = regulator_enable(priv->reg_vbus);
> 		else
> 			ret = regulator_disable(priv->reg_vbus);
> 
> 		...
> 	}
> 
> 	...
> }
> 
> static int host_start(struct ci_hdrc *ci)
> {
> 	...
> 
> 	priv->reg_vbus = NULL;
> 
> 	if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci)) {
> 		if (ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON) {
> 			ret = regulator_enable(ci->platdata->reg_vbus);
> 			if (ret) {
> 				dev_err(ci->dev,
> 				"Failed to enable vbus regulator, ret=%d\n",
> 									ret);
> 				goto put_hcd;
> 			}
> 		} else {
> 			priv->reg_vbus = ci->platdata->reg_vbus;
> 		}
> 	}
> 
> 	...
> }
> 
> 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?

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).
Marco Felsch Feb. 27, 2020, 12:44 p.m. UTC | #4
On 20-02-27 12:20, Peter Chen wrote:
> On 20-02-27 12:35:39, Marco Felsch wrote:
> > Hi Peter,
> > 
> > thanks for your fast reply :)
> > 
> > On 20-02-27 11:18, Peter Chen wrote:
> > > On 20-02-27 11:42:09, Marco Felsch wrote:
> > > > Hi,
> > > > 
> > > > the gpio-based port power is broken since commit [1,2]. I changed the
> > > > core behaviour to to cleanup the code and avoid local workaround fixes.
> > > 
> > > Many i.mx series evk boards work well for gpio-based port power control,
> > > what problem you have found, would you please list it detail?
> > 
> > Hm.. who could that work? I picked the important parts:
> > 
> > static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
> > {
> > 	...
> > 
> > 	if (priv->reg_vbus && enable != priv->enabled) {
> > 
> > 		...
> > 
> > 		if (enable)
> > 			ret = regulator_enable(priv->reg_vbus);
> > 		else
> > 			ret = regulator_disable(priv->reg_vbus);
> > 
> > 		...
> > 	}
> > 
> > 	...
> > }
> > 
> > static int host_start(struct ci_hdrc *ci)
> > {
> > 	...
> > 
> > 	priv->reg_vbus = NULL;
> > 
> > 	if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci)) {
> > 		if (ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON) {
> > 			ret = regulator_enable(ci->platdata->reg_vbus);
> > 			if (ret) {
> > 				dev_err(ci->dev,
> > 				"Failed to enable vbus regulator, ret=%d\n",
> > 									ret);
> > 				goto put_hcd;
> > 			}
> > 		} else {
> > 			priv->reg_vbus = ci->platdata->reg_vbus;
> > 		}
> > 	}
> > 
> > 	...
> > }
> > 
> > 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.

Regards,
  Marco

> -- 
> 
> Thanks,
> Peter Chen
Peter Chen Feb. 27, 2020, 1:30 p.m. UTC | #5
> > >
> > > 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.

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.

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.

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.

Peter
Peter Chen Feb. 27, 2020, 1:59 p.m. UTC | #6
> > 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 

Or you keep refcount for VBUS regulator on/off, if VBUS is already on, don't re-turn on
again. Do not consider CI_HDRC_TURN_VBUS_EARLY_ON after probe, and let
the USB core handle pm_qos_no_power_off, and call .ehci_ci_portpower accordingly.

Peter
Marco Felsch Feb. 27, 2020, 2:39 p.m. UTC | #7
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
>
Peter Chen Feb. 28, 2020, 2:51 a.m. UTC | #8
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.
Marco Felsch Feb. 28, 2020, 7:48 a.m. UTC | #9
Hi Peter,

On 20-02-28 02:51, Peter Chen wrote:

...

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

Arg, sorry now I see what you mean. Thanks for your explanation :)
Since the 'struct ehci_ci_priv' contains now an enabled state we can
git rid of the flag. To get it right, the writing the ehci PORT_POWER
must be done before or after we enabled the VBUS? I'm asking because
we can drop the 1st patch of this series.

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

Many thanks for testing =)

Regards,
  Marco

> -- 
> 
> Thanks,
> Peter Chen
Peter Chen Feb. 28, 2020, 9:40 a.m. UTC | #10
On 20-02-28 08:48:56, Marco Felsch wrote:
> Hi Peter,
> 
> On 20-02-28 02:51, Peter Chen wrote:
> 
> ...
> 
> > > > 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.
> 
> Arg, sorry now I see what you mean. Thanks for your explanation :)
> Since the 'struct ehci_ci_priv' contains now an enabled state we can
> git rid of the flag. To get it right, the writing the ehci PORT_POWER
> must be done before or after we enabled the VBUS? I'm asking because
> we can drop the 1st patch of this series.

Both are OK.