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

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

[Marco Felsch]

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(-)

-- 
2.20.1


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

[PATCH 1/3] USB: ehci-hub: let port_power() override the ehci_port_power()

[Marco Felsch]

Since commit 11a7e5940514 ("usb: ehci: add ehci_port_power interface")
drivers can add a port_power() override callback. Currently the
ChipIdea host driver is the only one using the callback to support gpio
controlled vbus enable/disable. Then commit 6adb9b7b5fb6 ("usb: chipidea:
add a flag for turn on vbus early for host") and commit 659459174188
("usb: chipidea: host: turn on vbus before add hcd if early vbus on is
required") adding a workaround to address the current core behaviour.
Since that the gpio based port-power is broken which was the only
use-case for this callback.

We should let the override callback override the core behaviour to avoid
adding local workarounds needed to address the core behaviour.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/host/ehci-hub.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index ce0eaf7d7c12..12b8c9055ab3 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -1338,13 +1338,13 @@ static int ehci_port_power(struct ehci_hcd *ehci, int portnum, bool enable)
 	u32 __iomem *status_reg = &ehci->regs->port_status[portnum];
 	u32 temp = ehci_readl(ehci, status_reg) & ~PORT_RWC_BITS;
 
+	if (hcd->driver->port_power)
+		return hcd->driver->port_power(hcd, portnum, enable);
+
 	if (enable)
 		ehci_writel(ehci, temp | PORT_POWER, status_reg);
 	else
 		ehci_writel(ehci, temp & ~PORT_POWER, status_reg);
 
-	if (hcd->driver->port_power)
-		hcd->driver->port_power(hcd, portnum, enable);
-
 	return 0;
 }
-- 
2.20.1


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

[PATCH 2/3] Partially Revert "usb: chipidea: host: turn on vbus before add hcd if early vbus on is required"

[Marco Felsch]

Commit 659459174188 ("usb: chipidea: host: turn on vbus before add hcd if
early vbus on is required") enabled the vbus regulator but didn't assign
the reg_vbus. So the vbus regulator can't be disabled anymore.

Since the port_power() callback is executed exclusive (without enabling
the port power (PP) bit) we can do the special handling within the
callback without the need of a special flag.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/chipidea/host.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 48e4a5ca1835..f1832847a023 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -37,6 +37,8 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 	struct ci_hdrc *ci = dev_get_drvdata(dev);
 	int ret = 0;
 	int port = HCS_N_PORTS(ehci->hcs_params);
+	u32 __iomem *status_reg = &ehci->regs->port_status[portnum];
+	u32 temp = ehci_readl(ehci, status_reg) & ~PORT_RWC_BITS;
 
 	if (priv->reg_vbus && enable != priv->enabled) {
 		if (port > 1) {
@@ -57,6 +59,11 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 		priv->enabled = enable;
 	}
 
+	if (enable)
+		ehci_writel(ehci, temp | PORT_POWER, status_reg);
+	else
+		ehci_writel(ehci, temp & ~PORT_POWER, status_reg);
+
 	if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
 		/*
 		 * Marvell 28nm HSIC PHY requires forcing the port to HS mode.
@@ -142,19 +149,8 @@ static int host_start(struct ci_hdrc *ci)
 	priv = (struct ehci_ci_priv *)ehci->priv;
 	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;
-		}
-	}
+	if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci))
+		priv->reg_vbus = ci->platdata->reg_vbus;
 
 	if (ci->platdata->pins_host)
 		pinctrl_select_state(ci->platdata->pctl,
@@ -162,7 +158,7 @@ static int host_start(struct ci_hdrc *ci)
 
 	ret = usb_add_hcd(hcd, 0, 0);
 	if (ret) {
-		goto disable_reg;
+		goto put_hcd;
 	} else {
 		struct usb_otg *otg = &ci->otg;
 
@@ -181,10 +177,6 @@ static int host_start(struct ci_hdrc *ci)
 
 	return ret;
 
-disable_reg:
-	if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci) &&
-			(ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON))
-		regulator_disable(ci->platdata->reg_vbus);
 put_hcd:
 	usb_put_hcd(hcd);
 
@@ -203,9 +195,6 @@ static void host_stop(struct ci_hdrc *ci)
 		ci->role = CI_ROLE_END;
 		synchronize_irq(ci->irq);
 		usb_put_hcd(hcd);
-		if (ci->platdata->reg_vbus && !ci_otg_is_fsm_mode(ci) &&
-			(ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON))
-				regulator_disable(ci->platdata->reg_vbus);
 	}
 	ci->hcd = NULL;
 	ci->otg.host = NULL;
-- 
2.20.1


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

[PATCH 3/3] Revert "usb: chipidea: add a flag for turn on vbus early for host"

[Marco Felsch]

The flag is no longer needed and can be removed since the quirk is now
handled within the port_power() callback.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++--------
 include/linux/usb/chipidea.h       | 17 ++++++++---------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index d8e7eb2f97b9..5c66921bbb9b 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -23,8 +23,7 @@ struct ci_hdrc_imx_platform_flag {
 };
 
 static const struct ci_hdrc_imx_platform_flag imx23_usb_data = {
-	.flags = CI_HDRC_TURN_VBUS_EARLY_ON |
-		CI_HDRC_DISABLE_STREAMING,
+	.flags = CI_HDRC_DISABLE_STREAMING,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx27_usb_data = {
@@ -33,31 +32,26 @@ static const struct ci_hdrc_imx_platform_flag imx27_usb_data = {
 
 static const struct ci_hdrc_imx_platform_flag imx28_usb_data = {
 	.flags = CI_HDRC_IMX28_WRITE_FIX |
-		CI_HDRC_TURN_VBUS_EARLY_ON |
 		CI_HDRC_DISABLE_STREAMING,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx6q_usb_data = {
 	.flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
-		CI_HDRC_TURN_VBUS_EARLY_ON |
 		CI_HDRC_DISABLE_STREAMING,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx6sl_usb_data = {
 	.flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
-		CI_HDRC_TURN_VBUS_EARLY_ON |
 		CI_HDRC_DISABLE_HOST_STREAMING,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx6sx_usb_data = {
 	.flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
-		CI_HDRC_TURN_VBUS_EARLY_ON |
 		CI_HDRC_DISABLE_HOST_STREAMING,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx6ul_usb_data = {
-	.flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
-		CI_HDRC_TURN_VBUS_EARLY_ON,
+	.flags = CI_HDRC_SUPPORTS_RUNTIME_PM,
 };
 
 static const struct ci_hdrc_imx_platform_flag imx7d_usb_data = {
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index edd89b7c8f18..fa373aafa80e 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -53,15 +53,14 @@ struct ci_hdrc_platform_data {
 #define CI_HDRC_DUAL_ROLE_NOT_OTG	BIT(4)
 #define CI_HDRC_IMX28_WRITE_FIX		BIT(5)
 #define CI_HDRC_FORCE_FULLSPEED		BIT(6)
-#define CI_HDRC_TURN_VBUS_EARLY_ON	BIT(7)
-#define CI_HDRC_SET_NON_ZERO_TTHA	BIT(8)
-#define CI_HDRC_OVERRIDE_AHB_BURST	BIT(9)
-#define CI_HDRC_OVERRIDE_TX_BURST	BIT(10)
-#define CI_HDRC_OVERRIDE_RX_BURST	BIT(11)
-#define CI_HDRC_OVERRIDE_PHY_CONTROL	BIT(12) /* Glue layer manages phy */
-#define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(13)
-#define CI_HDRC_IMX_IS_HSIC		BIT(14)
-#define CI_HDRC_PMQOS			BIT(15)
+#define CI_HDRC_SET_NON_ZERO_TTHA	BIT(7)
+#define CI_HDRC_OVERRIDE_AHB_BURST	BIT(8)
+#define CI_HDRC_OVERRIDE_TX_BURST	BIT(9)
+#define CI_HDRC_OVERRIDE_RX_BURST	BIT(10)
+#define CI_HDRC_OVERRIDE_PHY_CONTROL	BIT(11) /* Glue layer manages phy */
+#define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(12)
+#define CI_HDRC_IMX_IS_HSIC		BIT(13)
+#define CI_HDRC_PMQOS			BIT(14)
 	enum usb_dr_mode	dr_mode;
 #define CI_HDRC_CONTROLLER_RESET_EVENT		0
 #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
-- 
2.20.1


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

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

[Peter Chen]

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
> 

-- 

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

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

[Marco Felsch]

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
> 

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

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

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

[Peter Chen]

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

-- 

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

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

[Marco Felsch]

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

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

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

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

[Peter Chen]

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

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

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

[Peter Chen]

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

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

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

[Marco Felsch]

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 |

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

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

[Peter Chen]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

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

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

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

[Peter Chen]

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.

-- 

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