[v2,2/3] usb: chipidea: imx: support disabling runtime-pm

Commit Message

Philippe Schenker July 14, 2020, 3:18 p.m. UTC

The Toradex Colibri iMX6ULL board has a special USB hardware design.
With runtime-pm enabled USB reset itself continuously. Furthermore the
OTG port is also not enumerating devices if the Chipidea IP is in
runtime sleep mode and a device or host gets plugged in.

This patch adds the opportunity to disable Runtime Power Management
from devicetree

Signed-off-by: Philippe Schenker <[e-mail]>

---

Changes in v2:
- Change commit message to tell the use case for Colibri iMX6ULL

 drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Peter Chen July 15, 2020, 12:51 a.m. UTC | #1

> The Toradex Colibri iMX6ULL board has a special USB hardware design.
> With runtime-pm enabled USB reset itself continuously. Furthermore the OTG port
> is also not enumerating devices if the Chipidea IP is in runtime sleep mode and a
> device or host gets plugged in.
> 

Hi Philippe,

You may describe the detail what's the special USB hardware design for your board,
and why it causes the problem, and why disable runtime pm could fix this issue, then,
the other users could know if it could apply to their platforms or not in future.

Peter

> This patch adds the opportunity to disable Runtime Power Management from
> devicetree
> 
> Signed-off-by: Philippe Schenker <[e-mail]>
> 
> ---
> 
> Changes in v2:
> - Change commit message to tell the use case for Colibri iMX6ULL
> 
>  drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 5ae16368a0c7..5078d0695eb7 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -434,6 +434,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  		usb_phy_init(pdata.usb_phy);
>  	}
> 
> +	if (of_property_read_bool(np, "disable-runtime-pm"))
> +		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
> +
>  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
>  		data->supports_runtime_pm = true;
> 
> --
> 2.27.0

Philippe Schenker July 15, 2020, 10:23 a.m. UTC | #2

On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
>  
> > The Toradex Colibri iMX6ULL board has a special USB hardware design.
> > With runtime-pm enabled USB reset itself continuously. Furthermore
> > the OTG port
> > is also not enumerating devices if the Chipidea IP is in runtime
> > sleep mode and a
> > device or host gets plugged in.
> > 
> 
> Hi Philippe,
> 
> You may describe the detail what's the special USB hardware design for
> your board,

If I only knew the root-cause of that problem - unfortunately I don't.
That's also why I have such a hard time to describe it.

> and why it causes the problem, and why disable runtime pm could fix
> this issue, then,

I cannot provide the 'why' part yet. I'll try something more and hope I
can provide you guys with the exact description.

> the other users could know if it could apply to their platforms or not
> in future.

I only found out about it because you were pointing me in that
direction. I debugged for hours now and didn't came to the root-cause of
the issue. I think to really understand it I would need to know much
more about the Chipidea IP.

I'll get back to you guys with a proposal for a new description.

Philippe

> 
> Peter
> 
> > This patch adds the opportunity to disable Runtime Power Management
> > from
> > devicetree
> > 
> > Signed-off-by: Philippe Schenker <[e-mail]>
> > 
> > ---
> > 
> > Changes in v2:
> > - Change commit message to tell the use case for Colibri iMX6ULL
> > 
> >  drivers/usb/chipidea/ci_hdrc_imx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 5ae16368a0c7..5078d0695eb7 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -434,6 +434,9 @@ static int ci_hdrc_imx_probe(struct
> > platform_device *pdev)
> >  		usb_phy_init(pdata.usb_phy);
> >  	}
> > 
> > +	if (of_property_read_bool(np, "disable-runtime-pm"))
> > +		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
> > +
> >  	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
> >  		data->supports_runtime_pm = true;
> > 
> > --
> > 2.27.0

Peter Chen July 20, 2020, 3:44 a.m. UTC | #3

> On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> >
> > > The Toradex Colibri iMX6ULL board has a special USB hardware design.
> > > With runtime-pm enabled USB reset itself continuously. Furthermore
> > > the OTG port is also not enumerating devices if the Chipidea IP is
> > > in runtime sleep mode and a device or host gets plugged in.
> > >
> >
> > Hi Philippe,
> >
> > You may describe the detail what's the special USB hardware design for
> > your board,
> 
> If I only knew the root-cause of that problem - unfortunately I don't.
> That's also why I have such a hard time to describe it.
> 
> > and why it causes the problem, and why disable runtime pm could fix
> > this issue, then,
> 
> I cannot provide the 'why' part yet. I'll try something more and hope I can provide
> you guys with the exact description.
> 
> > the other users could know if it could apply to their platforms or not
> > in future.
> 
> I only found out about it because you were pointing me in that direction. I debugged
> for hours now and didn't came to the root-cause of the issue. I think to really
> understand it I would need to know much more about the Chipidea IP.
> 
> I'll get back to you guys with a proposal for a new description.
> 

Philippe, is it possible to share your USB hardware design at 6ULL?
And how ci_hdrc_gadget_connect is called when the runtime pm is disabled?

Thanks,
Peter

Philippe Schenker July 20, 2020, 7:51 a.m. UTC | #4

On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
>  
> > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > design.
> > > > With runtime-pm enabled USB reset itself continuously.
> > > > Furthermore
> > > > the OTG port is also not enumerating devices if the Chipidea IP
> > > > is
> > > > in runtime sleep mode and a device or host gets plugged in.
> > > > 
> > > 
> > > Hi Philippe,
> > > 
> > > You may describe the detail what's the special USB hardware design
> > > for
> > > your board,
> > 
> > If I only knew the root-cause of that problem - unfortunately I
> > don't.
> > That's also why I have such a hard time to describe it.
> > 
> > > and why it causes the problem, and why disable runtime pm could
> > > fix
> > > this issue, then,
> > 
> > I cannot provide the 'why' part yet. I'll try something more and
> > hope I can provide
> > you guys with the exact description.
> > 
> > > the other users could know if it could apply to their platforms or
> > > not
> > > in future.
> > 
> > I only found out about it because you were pointing me in that
> > direction. I debugged
> > for hours now and didn't came to the root-cause of the issue. I
> > think to really
> > understand it I would need to know much more about the Chipidea IP.
> > 
> > I'll get back to you guys with a proposal for a new description.
> > 
> 
> Philippe, is it possible to share your USB hardware design at 6ULL?

It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF capacitor
and +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the
big problem is that the driver can not detect the 5V on VBUS signal.

I tried to 'inject' 5V to that pin last week and things got really
better with runtime-pm. But I still thinks disabling it for our board
would make sense.

I'll send a new description today where I try to point to VBUS signal
not connected.

Philippe

> And how ci_hdrc_gadget_connect is called when the runtime pm is
> disabled?
> 
> Thanks,
> Peter
>

Peter Chen July 20, 2020, 8:06 a.m. UTC | #5

> On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
> >
> > > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > > design.
> > > > > With runtime-pm enabled USB reset itself continuously.
> > > > > Furthermore
> > > > > the OTG port is also not enumerating devices if the Chipidea IP
> > > > > is in runtime sleep mode and a device or host gets plugged in.
> > > > >
> > > >
> > > > Hi Philippe,
> > > >
> > > > You may describe the detail what's the special USB hardware design
> > > > for your board,
> > >
> > > If I only knew the root-cause of that problem - unfortunately I
> > > don't.
> > > That's also why I have such a hard time to describe it.
> > >
> > > > and why it causes the problem, and why disable runtime pm could
> > > > fix this issue, then,
> > >
> > > I cannot provide the 'why' part yet. I'll try something more and
> > > hope I can provide you guys with the exact description.
> > >
> > > > the other users could know if it could apply to their platforms or
> > > > not in future.
> > >
> > > I only found out about it because you were pointing me in that
> > > direction. I debugged for hours now and didn't came to the
> > > root-cause of the issue. I think to really understand it I would
> > > need to know much more about the Chipidea IP.
> > >
> > > I'll get back to you guys with a proposal for a new description.
> > >
> >
> > Philippe, is it possible to share your USB hardware design at 6ULL?
> 
> It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF capacitor and
> +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the big
> problem is that the driver can not detect the 5V on VBUS signal.
> 

Could you confirm it does not see VBUS at register OTGSC? If it is, how can it work with runtime
disabled, the USBCMD.RS setting (ci_hdrc_gadget_connect is called) depends on VBUS.

Peter

> I tried to 'inject' 5V to that pin last week and things got really better with runtime-pm.
> But I still thinks disabling it for our board would make sense.
> 
> I'll send a new description today where I try to point to VBUS signal not connected.
> 
> Philippe
> 
> > And how ci_hdrc_gadget_connect is called when the runtime pm is
> > disabled?
> >
> > Thanks,
> > Peter
> >

Philippe Schenker July 20, 2020, 10:10 a.m. UTC | #6

On Mon, 2020-07-20 at 08:06 +0000, Peter Chen wrote:
>  
> > On Mon, 2020-07-20 at 03:44 +0000, Peter Chen wrote:
> > > > On Wed, 2020-07-15 at 00:51 +0000, Peter Chen wrote:
> > > > > > The Toradex Colibri iMX6ULL board has a special USB hardware
> > > > > > design.
> > > > > > With runtime-pm enabled USB reset itself continuously.
> > > > > > Furthermore
> > > > > > the OTG port is also not enumerating devices if the Chipidea
> > > > > > IP
> > > > > > is in runtime sleep mode and a device or host gets plugged
> > > > > > in.
> > > > > > 
> > > > > 
> > > > > Hi Philippe,
> > > > > 
> > > > > You may describe the detail what's the special USB hardware
> > > > > design
> > > > > for your board,
> > > > 
> > > > If I only knew the root-cause of that problem - unfortunately I
> > > > don't.
> > > > That's also why I have such a hard time to describe it.
> > > > 
> > > > > and why it causes the problem, and why disable runtime pm
> > > > > could
> > > > > fix this issue, then,
> > > > 
> > > > I cannot provide the 'why' part yet. I'll try something more and
> > > > hope I can provide you guys with the exact description.
> > > > 
> > > > > the other users could know if it could apply to their
> > > > > platforms or
> > > > > not in future.
> > > > 
> > > > I only found out about it because you were pointing me in that
> > > > direction. I debugged for hours now and didn't came to the
> > > > root-cause of the issue. I think to really understand it I would
> > > > need to know much more about the Chipidea IP.
> > > > 
> > > > I'll get back to you guys with a proposal for a new description.
> > > > 
> > > 
> > > Philippe, is it possible to share your USB hardware design at
> > > 6ULL?
> > 
> > It's actually pretty simple: We have on USB_OTG1_VBUS a 1uF
> > capacitor and
> > +3.0V on VDD_USB_CAP together with 100n and 10u bypass caps. Now the
> > big
> > problem is that the driver can not detect the 5V on VBUS signal.
> > 
> 
> Could you confirm it does not see VBUS at register OTGSC? If it is,
> how can it work with runtime
> disabled, the USBCMD.RS setting (ci_hdrc_gadget_connect is called)
> depends on VBUS.
> 
> Peter

For this reason I'm using a workaround in extcon like this:

extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;

I know that this is undocumented and wrong, but it works for our
hardware. With this and enabled runtime-pm devices do not get
enumerated.

But with runtime-pm disabled, devices get enumerated.

Further with this workaround the VBUS signal gets 'simulated'
in hw_read_otgsc.

Another problem with runtime-pm enabled is that with no devices plugged
into USB it resets itself every ~1 second.

Philippe.
> 
> > I tried to 'inject' 5V to that pin last week and things got really
> > better with runtime-pm.
> > But I still thinks disabling it for our board would make sense.
> > 
> > I'll send a new description today where I try to point to VBUS
> > signal not connected.
> > 
> > Philippe
> > 
> > > And how ci_hdrc_gadget_connect is called when the runtime pm is
> > > disabled?
> > > 
> > > Thanks,
> > > Peter
> > >

Luca Ceresoli May 4, 2023, 4:23 p.m. UTC | #7

Hello Philippe,

I found this thread after several hours spent in debugging why USB host is
not detecting new devices on a custom board using the iMX6ULL Colibri
SoM.

My best workaround at the moment is:

  --- a/drivers/usb/chipidea/ci_hdrc_imx.c
  +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
  @@ -56,7 +58,7 @@ static const struct ci_hdrc_imx_platform_flag imx6sx_usb_data = {
   };
 
   static const struct ci_hdrc_imx_platform_flag imx6ul_usb_data = {
  -       .flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
  +       .flags = //CI_HDRC_SUPPORTS_RUNTIME_PM |
                  CI_HDRC_TURN_VBUS_EARLY_ON |
                  CI_HDRC_DISABLE_DEVICE_STREAMING,
   };

so it's equivalent to yours, but hard-coded.

I haven't found any follow-up patches from you, so I'm wondering whether
you have made any progress on this issue.

As I see it, a quirk in the driver would make sense. What needs to be
sorted out is how to enable it from device tree.

I think a DT boolean as you propose would do, but with a name describing
the hardware aspects of it ("chipidea,vbus-detection-is-broken"?) and not
the driver behaviour.

Otherwise looking for the "toradex,colibri-imx6ul" prefix in the top-level
/compatible strings seems doable. I don't know how acceptable this is from
a driver maintainer's point of view though.

Best regards,
Luca

Francesco Dolcini May 4, 2023, 4:50 p.m. UTC | #8

Hello Luca,
I guess your mail to Philippe bounced, let me try to answer since I am aware
of the issue here.

On Thu, May 04, 2023 at 06:23:12PM +0200, Luca Ceresoli wrote:
> I found this thread after several hours spent in debugging why USB host is
> not detecting new devices on a custom board using the iMX6ULL Colibri
> SoM.
> 
> My best workaround at the moment is:
We have the same workaround in our BSP since quite some time, see
https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-kernel/linux/linux-toradex-mainline-git/0002-drivers-chipidea-disable-runtime-pm-for-imx6ul.patch

> I haven't found any follow-up patches from you, so I'm wondering whether
> you have made any progress on this issue.
You can find the latest discussion on that regard here
https://lore.kernel.org/all/[e-mail]/

> As I see it, a quirk in the driver would make sense.
I am not sure.

The reason this is not working is that the VBUS is not directly
connected to the SOC and the USB IP is powered in a different way (all
of that was reviewed/acked by NXP when the board was designed).

The issue is in drivers/usb/phy/phy-mxs-usb.c:mxs_phy_disconnect_line(),
however:
 1 - checking for the VBUS when in host mode is not correct.
 2 - checking for the VBUS when in OTG/dual-role mode should be done using
 extcon or equivalent. In this case on the colibri-imx6ull you would get
 the correct behavior.

The NXP downstream kernel has some improvements on that regard, for
instance 1 would work, 2 I am not 100% sure.

Here the related patches:

From 60422d8301fc354e69ec0184831468c3b65cc26a Mon Sep 17 00:00:00 2001
From: Li Jun <[e-mail]>
Date: Wed, 12 Apr 2017 05:31:17 +0800
Subject: [PATCH 1/3] MLK-14285-1 usb: phy: add usb mode for usb_phy

USB phy driver may need to know the current working mode of
the controller, and does some different settings according to
host mode or device mode.

Signed-off-by: Li Jun <[e-mail]>
(cherry picked from commit 2286cb30feedd6f4a5cb82a0f0af5aa3a04ab698)
Signed-off-by: Vipul Kumar <[e-mail]>
Signed-off-by: Srikanth Krishnakar <[e-mail]>
---
 include/linux/usb/phy.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index e4de6bc1f69b..93963a018a13 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -63,6 +63,13 @@ enum usb_otg_state {
 	OTG_STATE_A_VBUS_ERR,
 };
 
+/* The usb role of phy to be working with */
+enum usb_current_mode {
+	CUR_USB_MODE_NONE,
+	CUR_USB_MODE_HOST,
+	CUR_USB_MODE_DEVICE,
+};
+
 struct usb_phy;
 struct usb_otg;
 
@@ -155,6 +162,10 @@ struct usb_phy {
 	 * manually detect the charger type.
 	 */
 	enum usb_charger_type (*charger_detect)(struct usb_phy *x);
+
+	int	(*set_mode)(struct usb_phy *x,
+			enum usb_current_mode mode);
+
 };
 
 /* for board-specific init logic */
@@ -213,6 +224,15 @@ usb_phy_vbus_off(struct usb_phy *x)
 	return x->set_vbus(x, false);
 }
 
+static inline int
+usb_phy_set_mode(struct usb_phy *x, enum usb_current_mode mode)
+{
+	if (!x || !x->set_mode)
+		return 0;
+
+	return x->set_mode(x, mode);
+}
+
 /* for usb host and peripheral controller drivers */
 #if IS_ENABLED(CONFIG_USB_PHY)
 extern struct usb_phy *usb_get_phy(enum usb_phy_type type);


From cf3f741f4b7ab5d139938c4c0cd65d4547d386ff Mon Sep 17 00:00:00 2001
From: Li Jun <[e-mail]>
Date: Wed, 12 Apr 2017 05:41:21 +0800
Subject: [PATCH 2/3] MLK-14285-3 usb: phy: mxs: optimize disconnect line
 condition

We only have below cases to disconnect line when suspend:
1. Device mode without connection to any host/charger(no vbus).
2. Device mode connect to a charger(w/ vbus), usb suspend when
   system is entering suspend.
This patch can fix usb phy wrongly does disconnect line in case
some usb host enters suspend but vbus is off.

Signed-off-by: Li Jun <[e-mail]>
(cherry picked from commit 2af48913f77cec3658f5863b13f63619d8101279)
---
 drivers/usb/phy/phy-mxs-usb.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index 8a262c5a0408..31f40dbb71c0 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Copyright 2012-2014 Freescale Semiconductor, Inc.
+ * Copyright 2012-2016 Freescale Semiconductor, Inc.
+ * Copyright 2017 NXP
  * Copyright (C) 2012 Marek Vasut <[e-mail]>
  * on behalf of DENX Software Engineering GmbH
  */
@@ -204,6 +205,7 @@ struct mxs_phy {
 	int port_id;
 	u32 tx_reg_set;
 	u32 tx_reg_mask;
+	enum usb_current_mode mode;
 };
 
 static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy)
@@ -386,18 +388,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
 		usleep_range(500, 1000);
 }
 
-static bool mxs_phy_is_otg_host(struct mxs_phy *mxs_phy)
-{
-	void __iomem *base = mxs_phy->phy.io_priv;
-	u32 phyctrl = readl(base + HW_USBPHY_CTRL);
-
-	if (IS_ENABLED(CONFIG_USB_OTG) &&
-			!(phyctrl & BM_USBPHY_CTRL_OTG_ID_VALUE))
-		return true;
-
-	return false;
-}
-
 static void mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool on)
 {
 	bool vbus_is_on = false;
@@ -412,7 +402,7 @@ static void mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool on)
 
 	vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);
 
-	if (on && !vbus_is_on && !mxs_phy_is_otg_host(mxs_phy))
+	if (on && ((!vbus_is_on && mxs_phy->mode != CUR_USB_MODE_HOST)))
 		__mxs_phy_disconnect_line(mxs_phy, true);
 	else
 		__mxs_phy_disconnect_line(mxs_phy, false);
@@ -708,6 +698,19 @@ static enum usb_charger_type mxs_phy_charger_detect(struct usb_phy *phy)
 	return chgr_type;
 }
 
+/*
+ * Set the usb current role for phy.
+ */
+static int mxs_phy_set_mode(struct usb_phy *phy,
+		enum usb_current_mode mode)
+{
+	struct mxs_phy *mxs_phy = to_mxs_phy(phy);
+
+	mxs_phy->mode = mode;
+
+	return 0;
+}
+
 static int mxs_phy_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
@@ -793,6 +796,7 @@ static int mxs_phy_probe(struct platform_device *pdev)
 
 	mxs_phy->clk = clk;
 	mxs_phy->data = of_device_get_match_data(&pdev->dev);
+	mxs_phy->phy.set_mode		= mxs_phy_set_mode;
 
 	platform_set_drvdata(pdev, mxs_phy);
 

From d4c4385b997f32b081e859d491f25f5beb738ae9 Mon Sep 17 00:00:00 2001
From: Li Jun <[e-mail]>
Date: Wed, 12 Apr 2017 05:38:48 +0800
Subject: [PATCH 3/3] MLK-14285-2 usb: chipidea: set mode for usb phy driver

After enters one specific role, notify usb phy driver.

Signed-off-by: Li Jun <[e-mail]>
(cherry picked from commit d3aa2a13f4e47bc7fae7f2eee1e86291d7513312)
---
 drivers/usb/chipidea/ci.h | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 99440baa6458..9d7ba902246d 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -274,9 +274,21 @@ static inline int ci_role_start(struct ci_hdrc *ci, enum ci_role role)
 		return -ENXIO;
 
 	ret = ci->roles[role]->start(ci);
-	if (!ret)
-		ci->role = role;
-	return ret;
+	if (ret)
+		return ret;
+
+	ci->role = role;
+
+	if (ci->usb_phy) {
+		if (role == CI_ROLE_HOST)
+			usb_phy_set_mode(ci->usb_phy,
+					CUR_USB_MODE_HOST);
+		else
+			usb_phy_set_mode(ci->usb_phy,
+					CUR_USB_MODE_DEVICE);
+	}
+
+	return 0;
 }
 
 static inline void ci_role_stop(struct ci_hdrc *ci)
@@ -289,6 +301,9 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 	ci->role = CI_ROLE_END;
 
 	ci->roles[role]->stop(ci);
+
+	if (ci->usb_phy)
+		usb_phy_set_mode(ci->usb_phy, CUR_USB_MODE_NONE);
 }
 
 static inline enum usb_role ci_role_to_usb_role(struct ci_hdrc *ci)


Francesco

Luca Ceresoli May 5, 2023, 9:23 a.m. UTC | #9

Hello Francesco,

On Thu, 4 May 2023 18:50:14 +0200
Francesco Dolcini <[e-mail]> wrote:

> Hello Luca,
> I guess your mail to Philippe bounced, let me try to answer since I am aware
> of the issue here.
> 
> On Thu, May 04, 2023 at 06:23:12PM +0200, Luca Ceresoli wrote:
> > I found this thread after several hours spent in debugging why USB host is
> > not detecting new devices on a custom board using the iMX6ULL Colibri
> > SoM.
> > 
> > My best workaround at the moment is:  
> We have the same workaround in our BSP since quite some time, see
> https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-kernel/linux/linux-toradex-mainline-git/0002-drivers-chipidea-disable-runtime-pm-for-imx6ul.patch
> 
> > I haven't found any follow-up patches from you, so I'm wondering whether
> > you have made any progress on this issue.  
> You can find the latest discussion on that regard here
> https://lore.kernel.org/all/[e-mail]/

Thanks for this pointer! I have read the discussion and it was a bit
confusing, especially about whether the hardware can work at all.

Are you planning to continue on that work? I would be very glad to test
on product based on the i.MX6ULL Colibri module I am currently working
on.

Best regards,
Luca

Jun Li May 5, 2023, 9:49 a.m. UTC | #10

> -----Original Message-----
> From: Francesco Dolcini <[e-mail]>
> Sent: Friday, May 5, 2023 12:50 AM
> To: Luca Ceresoli <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Jun Li <[e-mail]>;
> [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Krzysztof Kozlowski
> <[e-mail]>; Francesco Dolcini
> <[e-mail]>
> Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
>
> Hello Luca,
> I guess your mail to Philippe bounced, let me try to answer since I am aware
> of the issue here.
>
> On Thu, May 04, 2023 at 06:23:12PM +0200, Luca Ceresoli wrote:
> > I found this thread after several hours spent in debugging why USB host
> is
> > not detecting new devices on a custom board using the iMX6ULL Colibri
> > SoM.
> >
> > My best workaround at the moment is:
> We have the same workaround in our BSP since quite some time, see
> https://git.t/
> oradex.com%2Fcgit%2Fmeta-toradex-bsp-common.git%2Ftree%2Frecipes-kernel
> %2Flinux%2Flinux-toradex-mainline-git%2F0002-drivers-chipidea-disable-r
> untime-pm-for-imx6ul.patch&data=05%7C01%7Cjun.li%40nxp.com%7C776dc1e71a
> 554ee20ed908db4cbfa5aa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 188158228795706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3cFsUurgKBfic
> LNfRxMOErukGiMczhJREFGvaFYTH34%3D&reserved=0
>
> > I haven't found any follow-up patches from you, so I'm wondering whether
> > you have made any progress on this issue.
> You can find the latest discussion on that regard here
> https://lore/.
> kernel.org%2Fall%2FY1vLpaxpc5WBCuGD%40francesco-nb.int.toradex.com%2F&d
> ata=05%7C01%7Cjun.li%40nxp.com%7C776dc1e71a554ee20ed908db4cbfa5aa%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638188158228795706%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=xgIAlY5Az9zQgOBaUTHeGJDXNIyNRmEBlcR49JOVB
> sI%3D&reserved=0
>
> > As I see it, a quirk in the driver would make sense.
> I am not sure.
>
> The reason this is not working is that the VBUS is not directly
> connected to the SOC and the USB IP is powered in a different way (all
> of that was reviewed/acked by NXP when the board was designed).

Hi Luca,

Is your board design similar like Francesco's as below?

"VDD_USB_CAP in our design is connected to a 3.0V external LDO,
 voltage on both VBUS pads is 0V"

I am going to fix this by update the PHY power supply check to cover
Francesco's case(PHY is powered by VDD_USB_CAP).

Thanks
Li Jun

>
> The issue is in drivers/usb/phy/phy-mxs-usb.c:mxs_phy_disconnect_line(),
> however:
>  1 - checking for the VBUS when in host mode is not correct.
>  2 - checking for the VBUS when in OTG/dual-role mode should be done using
>  extcon or equivalent. In this case on the colibri-imx6ull you would get
>  the correct behavior.
>
> The NXP downstream kernel has some improvements on that regard, for
> instance 1 would work, 2 I am not 100% sure.
>
> Here the related patches:
>
> From 60422d8301fc354e69ec0184831468c3b65cc26a Mon Sep 17 00:00:00 2001
> From: Li Jun <[e-mail]>
> Date: Wed, 12 Apr 2017 05:31:17 +0800
> Subject: [PATCH 1/3] MLK-14285-1 usb: phy: add usb mode for usb_phy
>
> USB phy driver may need to know the current working mode of
> the controller, and does some different settings according to
> host mode or device mode.
>
> Signed-off-by: Li Jun <[e-mail]>
> (cherry picked from commit 2286cb30feedd6f4a5cb82a0f0af5aa3a04ab698)
> Signed-off-by: Vipul Kumar <[e-mail]>
> Signed-off-by: Srikanth Krishnakar <[e-mail]>
> ---
>  include/linux/usb/phy.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index e4de6bc1f69b..93963a018a13 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -63,6 +63,13 @@ enum usb_otg_state {
>       OTG_STATE_A_VBUS_ERR,
>  };
>
> +/* The usb role of phy to be working with */
> +enum usb_current_mode {
> +     CUR_USB_MODE_NONE,
> +     CUR_USB_MODE_HOST,
> +     CUR_USB_MODE_DEVICE,
> +};
> +
>  struct usb_phy;
>  struct usb_otg;
>
> @@ -155,6 +162,10 @@ struct usb_phy {
>        * manually detect the charger type.
>        */
>       enum usb_charger_type (*charger_detect)(struct usb_phy *x);
> +
> +     int     (*set_mode)(struct usb_phy *x,
> +                     enum usb_current_mode mode);
> +
>  };
>
>  /* for board-specific init logic */
> @@ -213,6 +224,15 @@ usb_phy_vbus_off(struct usb_phy *x)
>       return x->set_vbus(x, false);
>  }
>
> +static inline int
> +usb_phy_set_mode(struct usb_phy *x, enum usb_current_mode mode)
> +{
> +     if (!x || !x->set_mode)
> +             return 0;
> +
> +     return x->set_mode(x, mode);
> +}
> +
>  /* for usb host and peripheral controller drivers */
>  #if IS_ENABLED(CONFIG_USB_PHY)
>  extern struct usb_phy *usb_get_phy(enum usb_phy_type type);
>
>
> From cf3f741f4b7ab5d139938c4c0cd65d4547d386ff Mon Sep 17 00:00:00 2001
> From: Li Jun <[e-mail]>
> Date: Wed, 12 Apr 2017 05:41:21 +0800
> Subject: [PATCH 2/3] MLK-14285-3 usb: phy: mxs: optimize disconnect line
>  condition
>
> We only have below cases to disconnect line when suspend:
> 1. Device mode without connection to any host/charger(no vbus).
> 2. Device mode connect to a charger(w/ vbus), usb suspend when
>    system is entering suspend.
> This patch can fix usb phy wrongly does disconnect line in case
> some usb host enters suspend but vbus is off.
>
> Signed-off-by: Li Jun <[e-mail]>
> (cherry picked from commit 2af48913f77cec3658f5863b13f63619d8101279)
> ---
>  drivers/usb/phy/phy-mxs-usb.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c
> b/drivers/usb/phy/phy-mxs-usb.c
> index 8a262c5a0408..31f40dbb71c0 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * Copyright 2012-2014 Freescale Semiconductor, Inc.
> + * Copyright 2012-2016 Freescale Semiconductor, Inc.
> + * Copyright 2017 NXP
>   * Copyright (C) 2012 Marek Vasut <[e-mail]>
>   * on behalf of DENX Software Engineering GmbH
>   */
> @@ -204,6 +205,7 @@ struct mxs_phy {
>       int port_id;
>       u32 tx_reg_set;
>       u32 tx_reg_mask;
> +     enum usb_current_mode mode;
>  };
>
>  static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy)
> @@ -386,18 +388,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy
> *mxs_phy, bool disconnect)
>               usleep_range(500, 1000);
>  }
>
> -static bool mxs_phy_is_otg_host(struct mxs_phy *mxs_phy)
> -{
> -     void __iomem *base = mxs_phy->phy.io_priv;
> -     u32 phyctrl = readl(base + HW_USBPHY_CTRL);
> -
> -     if (IS_ENABLED(CONFIG_USB_OTG) &&
> -                     !(phyctrl & BM_USBPHY_CTRL_OTG_ID_VALUE))
> -             return true;
> -
> -     return false;
> -}
> -
>  static void mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool on)
>  {
>       bool vbus_is_on = false;
> @@ -412,7 +402,7 @@ static void mxs_phy_disconnect_line(struct mxs_phy
> *mxs_phy, bool on)
>
>       vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);
>
> -     if (on && !vbus_is_on && !mxs_phy_is_otg_host(mxs_phy))
> +     if (on && ((!vbus_is_on && mxs_phy->mode != CUR_USB_MODE_HOST)))
>               __mxs_phy_disconnect_line(mxs_phy, true);
>       else
>               __mxs_phy_disconnect_line(mxs_phy, false);
> @@ -708,6 +698,19 @@ static enum usb_charger_type
> mxs_phy_charger_detect(struct usb_phy *phy)
>       return chgr_type;
>  }
>
> +/*
> + * Set the usb current role for phy.
> + */
> +static int mxs_phy_set_mode(struct usb_phy *phy,
> +             enum usb_current_mode mode)
> +{
> +     struct mxs_phy *mxs_phy = to_mxs_phy(phy);
> +
> +     mxs_phy->mode = mode;
> +
> +     return 0;
> +}
> +
>  static int mxs_phy_probe(struct platform_device *pdev)
>  {
>       void __iomem *base;
> @@ -793,6 +796,7 @@ static int mxs_phy_probe(struct platform_device *pdev)
>
>       mxs_phy->clk = clk;
>       mxs_phy->data = of_device_get_match_data(&pdev->dev);
> +     mxs_phy->phy.set_mode           = mxs_phy_set_mode;
>
>       platform_set_drvdata(pdev, mxs_phy);
>
>
> From d4c4385b997f32b081e859d491f25f5beb738ae9 Mon Sep 17 00:00:00 2001
> From: Li Jun <[e-mail]>
> Date: Wed, 12 Apr 2017 05:38:48 +0800
> Subject: [PATCH 3/3] MLK-14285-2 usb: chipidea: set mode for usb phy driver
>
> After enters one specific role, notify usb phy driver.
>
> Signed-off-by: Li Jun <[e-mail]>
> (cherry picked from commit d3aa2a13f4e47bc7fae7f2eee1e86291d7513312)
> ---
>  drivers/usb/chipidea/ci.h | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 99440baa6458..9d7ba902246d 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -274,9 +274,21 @@ static inline int ci_role_start(struct ci_hdrc *ci,
> enum ci_role role)
>               return -ENXIO;
>
>       ret = ci->roles[role]->start(ci);
> -     if (!ret)
> -             ci->role = role;
> -     return ret;
> +     if (ret)
> +             return ret;
> +
> +     ci->role = role;
> +
> +     if (ci->usb_phy) {
> +             if (role == CI_ROLE_HOST)
> +                     usb_phy_set_mode(ci->usb_phy,
> +                                     CUR_USB_MODE_HOST);
> +             else
> +                     usb_phy_set_mode(ci->usb_phy,
> +                                     CUR_USB_MODE_DEVICE);
> +     }
> +
> +     return 0;
>  }
>
>  static inline void ci_role_stop(struct ci_hdrc *ci)
> @@ -289,6 +301,9 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
>       ci->role = CI_ROLE_END;
>
>       ci->roles[role]->stop(ci);
> +
> +     if (ci->usb_phy)
> +             usb_phy_set_mode(ci->usb_phy, CUR_USB_MODE_NONE);
>  }
>
>  static inline enum usb_role ci_role_to_usb_role(struct ci_hdrc *ci)
>
>
> Francesco

Luca Ceresoli May 5, 2023, 10:06 a.m. UTC | #11

Hello Jun,

On Fri, 5 May 2023 09:49:16 +0000
Jun Li <[e-mail]> wrote:

> > -----Original Message-----
> > From: Francesco Dolcini <[e-mail]>
> > Sent: Friday, May 5, 2023 12:50 AM
> > To: Luca Ceresoli <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Jun Li <[e-mail]>;
> > [e-mail]; [e-mail];
> > dl-linux-imx <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Krzysztof Kozlowski
> > <[e-mail]>; Francesco Dolcini
> > <[e-mail]>
> > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> >
> > Hello Luca,
> > I guess your mail to Philippe bounced, let me try to answer since I am aware
> > of the issue here.
> >
> > On Thu, May 04, 2023 at 06:23:12PM +0200, Luca Ceresoli wrote:  
> > > I found this thread after several hours spent in debugging why USB host  
> > is  
> > > not detecting new devices on a custom board using the iMX6ULL Colibri
> > > SoM.
> > >
> > > My best workaround at the moment is:  
> > We have the same workaround in our BSP since quite some time, see
> > https://git.t/
> > oradex.com%2Fcgit%2Fmeta-toradex-bsp-common.git%2Ftree%2Frecipes-kernel
> > %2Flinux%2Flinux-toradex-mainline-git%2F0002-drivers-chipidea-disable-r
> > untime-pm-for-imx6ul.patch&data=05%7C01%7Cjun.li%40nxp.com%7C776dc1e71a
> > 554ee20ed908db4cbfa5aa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > 188158228795706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> > uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3cFsUurgKBfic
> > LNfRxMOErukGiMczhJREFGvaFYTH34%3D&reserved=0
> >  
> > > I haven't found any follow-up patches from you, so I'm wondering whether
> > > you have made any progress on this issue.  
> > You can find the latest discussion on that regard here
> > https://lore/.
> > kernel.org%2Fall%2FY1vLpaxpc5WBCuGD%40francesco-nb.int.toradex.com%2F&d
> > ata=05%7C01%7Cjun.li%40nxp.com%7C776dc1e71a554ee20ed908db4cbfa5aa%7C686
> > ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638188158228795706%7CUnknown%7C
> > TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > 6Mn0%3D%7C3000%7C%7C%7C&sdata=xgIAlY5Az9zQgOBaUTHeGJDXNIyNRmEBlcR49JOVB
> > sI%3D&reserved=0
> >  
> > > As I see it, a quirk in the driver would make sense.  
> > I am not sure.
> >
> > The reason this is not working is that the VBUS is not directly
> > connected to the SOC and the USB IP is powered in a different way (all
> > of that was reviewed/acked by NXP when the board was designed).  
> 
> Hi Luca,
> 
> Is your board design similar like Francesco's as below?

Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
i.MX6ULL SoM, whose schematics are not public.

Best regards,
Luca

Francesco Dolcini May 5, 2023, 10:58 a.m. UTC | #12

On Fri, May 05, 2023 at 11:23:51AM +0200, Luca Ceresoli wrote:
> On Thu, 4 May 2023 18:50:14 +0200
> Francesco Dolcini <[e-mail]> wrote:
> 
> > Hello Luca,
> > I guess your mail to Philippe bounced, let me try to answer since I am aware
> > of the issue here.
> > 
> > On Thu, May 04, 2023 at 06:23:12PM +0200, Luca Ceresoli wrote:
> > > I found this thread after several hours spent in debugging why USB host is
> > > not detecting new devices on a custom board using the iMX6ULL Colibri
> > > SoM.
> > > 
> > > My best workaround at the moment is:  
> > We have the same workaround in our BSP since quite some time, see
> > https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-kernel/linux/linux-toradex-mainline-git/0002-drivers-chipidea-disable-runtime-pm-for-imx6ul.patch
> > 
> > > I haven't found any follow-up patches from you, so I'm wondering whether
> > > you have made any progress on this issue.  
> > You can find the latest discussion on that regard here
> > https://lore.kernel.org/all/[e-mail]/
> 
> Thanks for this pointer! I have read the discussion and it was a bit
> confusing, especially about whether the hardware can work at all.

It can work. Hardware wise is just fine, software wise I tried to
explain how in my previous answer to you. Worst case we would have to
describe the hardware in a different way in the DTS, but I do not expect
this to be required.

> Are you planning to continue on that work?

Hopefully NXP / Li Jun will be able to help.

Francesco

Francesco Dolcini May 5, 2023, 11 a.m. UTC | #13

On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> On Fri, 5 May 2023 09:49:16 +0000
> Jun Li <[e-mail]> wrote:
> > Is your board design similar like Francesco's as below?
> 
> Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> i.MX6ULL SoM, whose schematics are not public.

I can confirm that it's the same.

Francesco

Jun Li May 6, 2023, 9:02 a.m. UTC | #14

> -----Original Message-----
> From: Francesco Dolcini <[e-mail]>
> Sent: Friday, May 5, 2023 7:00 PM
> To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> Cc: Francesco Dolcini <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Krzysztof Kozlowski <[e-mail]>;
> Francesco Dolcini <[e-mail]>
> Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> 
> On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> > On Fri, 5 May 2023 09:49:16 +0000
> > Jun Li <[e-mail]> wrote:
> > > Is your board design similar like Francesco's as below?
> >
> > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > i.MX6ULL SoM, whose schematics are not public.
> 
> I can confirm that it's the same.

Thanks Francesco for the confirmation, had a check with design team,
there is no status bit which can be used to judge the VDD_USB_CAP is
powered or not, so we have to add a board level dts property to tell
this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.

Before send a formal patch, I want to confirm this should work for your
HW design, like below simple hack:

diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index e1a2b2ea098b..ec5ee790455e 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
 };
 
 static const struct mxs_phy_data imx6ul_phy_data = {
-       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
 };
 
 static const struct mxs_phy_data imx7ulp_phy_data = {

Thanks
Li Jun

> 
> Francesco

Francesco Dolcini May 8, 2023, 11:17 a.m. UTC | #15

On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > -----Original Message-----
> > From: Francesco Dolcini <[e-mail]>
> > Sent: Friday, May 5, 2023 7:00 PM
> > To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> > Cc: Francesco Dolcini <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Krzysztof Kozlowski <[e-mail]>;
> > Francesco Dolcini <[e-mail]>
> > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> > 
> > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> > > On Fri, 5 May 2023 09:49:16 +0000
> > > Jun Li <[e-mail]> wrote:
> > > > Is your board design similar like Francesco's as below?
> > >
> > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > i.MX6ULL SoM, whose schematics are not public.
> > 
> > I can confirm that it's the same.
> 
> Thanks Francesco for the confirmation, had a check with design team,
> there is no status bit which can be used to judge the VDD_USB_CAP is
> powered or not, so we have to add a board level dts property to tell
> this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> 
> Before send a formal patch, I want to confirm this should work for your
> HW design, like below simple hack:

Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
I would be happy to test the patch as soon as you send it.


With that said I had another issue that I assume is unrelated.
In addition to the USB Host port, we have an additional OTG one. This
interface has the same circuit WRT to the VBUS, however in this case
it's possible to read the VBUS using extcon, e.g. a standard GPIO input.

With that setup, while doing a role switch, I had a couple of time this
error:

[  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
[  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC

that was recovered only doing an additional transition.

More complete logs here:

[  184.997619] usb 2-1: USB disconnect, device number 9
[  185.019620] ci_hdrc ci_hdrc.0: remove, state 1
[  185.024271] usb usb2: USB disconnect, device number 1
[  185.334975] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
[  185.353857] ci_hdrc ci_hdrc.0: EHCI Host Controller
[  185.389670] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 2
[  185.470170] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
[  185.476097] usb usb2: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.01
[  185.484527] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[  185.491811] usb usb2: Product: EHCI Host Controller
[  185.496704] usb usb2: Manufacturer: Linux 6.1.22-6.2.0+git.3b29299e5f60 ehci_hcd
[  185.504148] usb usb2: SerialNumber: ci_hdrc.0
[  185.531121] hub 2-0:1.0: USB hub found
[  185.542636] hub 2-0:1.0: 1 port detected
[  185.556586] mxs_phy 20c9000.usbphy: vbus is not valid
[  187.271684] ci_hdrc ci_hdrc.0: remove, state 4
[  187.276281] usb usb2: USB disconnect, device number 1
[  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
[  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC


> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index e1a2b2ea098b..ec5ee790455e 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
>  };
>  
>  static const struct mxs_phy_data imx6ul_phy_data = {
> -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
>  };
>  
>  static const struct mxs_phy_data imx7ulp_phy_data = {

Francesco

Jun Li May 8, 2023, 11:53 a.m. UTC | #16

> -----Original Message-----
> From: Francesco Dolcini <[e-mail]>
> Sent: Monday, May 8, 2023 7:17 PM
> To: Jun Li <[e-mail]>
> Cc: Francesco Dolcini <[e-mail]>; Luca Ceresoli
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Krzysztof Kozlowski <[e-mail]>; Francesco Dolcini
> <[e-mail]>; Xu Yang <[e-mail]>
> Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> 
> On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > -----Original Message-----
> > > From: Francesco Dolcini <[e-mail]>
> > > Sent: Friday, May 5, 2023 7:00 PM
> > > To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> > > Cc: Francesco Dolcini <[e-mail]>;
> [e-mail];
> > > [e-mail]; [e-mail];
> [e-mail];
> > > [e-mail]; dl-linux-imx
> <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Krzysztof Kozlowski
> <[e-mail]>;
> > > Francesco Dolcini <[e-mail]>
> > > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling
> runtime-pm
> > >
> > > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> > > > On Fri, 5 May 2023 09:49:16 +0000
> > > > Jun Li <[e-mail]> wrote:
> > > > > Is your board design similar like Francesco's as below?
> > > >
> > > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > > i.MX6ULL SoM, whose schematics are not public.
> > >
> > > I can confirm that it's the same.
> >
> > Thanks Francesco for the confirmation, had a check with design team,
> > there is no status bit which can be used to judge the VDD_USB_CAP is
> > powered or not, so we have to add a board level dts property to tell
> > this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> >
> > Before send a formal patch, I want to confirm this should work for your
> > HW design, like below simple hack:
> 
> Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
> I would be happy to test the patch as soon as you send it.
> 
> 
> With that said I had another issue that I assume is unrelated.
> In addition to the USB Host port, we have an additional OTG one. This
> interface has the same circuit WRT to the VBUS, however in this case
> it's possible to read the VBUS using extcon, e.g. a standard GPIO input.
> 
> With that setup, while doing a role switch, I had a couple of time this
> error:
> 
> [  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> [  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC
> 
> that was recovered only doing an additional transition.
> 
> More complete logs here:
> 
> [  184.997619] usb 2-1: USB disconnect, device number 9
> [  185.019620] ci_hdrc ci_hdrc.0: remove, state 1
> [  185.024271] usb usb2: USB disconnect, device number 1
> [  185.334975] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> [  185.353857] ci_hdrc ci_hdrc.0: EHCI Host Controller
> [  185.389670] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number
> 2
> [  185.470170] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
> [  185.476097] usb usb2: New USB device found, idVendor=1d6b, idProduct=0002,
> bcdDevice= 6.01
> [  185.484527] usb usb2: New USB device strings: Mfr=3, Product=2,
> SerialNumber=1
> [  185.491811] usb usb2: Product: EHCI Host Controller
> [  185.496704] usb usb2: Manufacturer: Linux 6.1.22-6.2.0+git.3b29299e5f60
> ehci_hcd
> [  185.504148] usb usb2: SerialNumber: ci_hdrc.0
> [  185.531121] hub 2-0:1.0: USB hub found
> [  185.542636] hub 2-0:1.0: 1 port detected
> [  185.556586] mxs_phy 20c9000.usbphy: vbus is not valid
> [  187.271684] ci_hdrc ci_hdrc.0: remove, state 4
> [  187.276281] usb usb2: USB disconnect, device number 1
> [  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> [  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC

I have 2 questions:
1. Can your OTG port work fine in device/gadget mode if you did not
   do usb role switch?(e.g. the OTG port *never* switch to host mode
   after system boot).
2. Please show me your dts file node of your USB OTG port, I want to
   Know how you config your OTG port. 

Thanks
Li Jun
> 
> 
> > diff --git a/drivers/usb/phy/phy-mxs-usb.c
> b/drivers/usb/phy/phy-mxs-usb.c
> > index e1a2b2ea098b..ec5ee790455e 100644
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data =
> {
> >  };
> >
> >  static const struct mxs_phy_data imx6ul_phy_data = {
> > -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> >  };
> >
> >  static const struct mxs_phy_data imx7ulp_phy_data = {
> 
> Francesco

Luca Ceresoli May 8, 2023, 1:17 p.m. UTC | #17

Hello Jun, Francesco,

On Mon, 8 May 2023 13:17:11 +0200
Francesco Dolcini <[e-mail]> wrote:

> On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > -----Original Message-----
> > > From: Francesco Dolcini <[e-mail]>
> > > Sent: Friday, May 5, 2023 7:00 PM
> > > To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> > > Cc: Francesco Dolcini <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; dl-linux-imx <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Krzysztof Kozlowski <[e-mail]>;
> > > Francesco Dolcini <[e-mail]>
> > > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> > > 
> > > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:  
> > > > On Fri, 5 May 2023 09:49:16 +0000
> > > > Jun Li <[e-mail]> wrote:  
> > > > > Is your board design similar like Francesco's as below?  
> > > >
> > > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > > i.MX6ULL SoM, whose schematics are not public.  
> > > 
> > > I can confirm that it's the same.  
> > 
> > Thanks Francesco for the confirmation, had a check with design team,
> > there is no status bit which can be used to judge the VDD_USB_CAP is
> > powered or not, so we have to add a board level dts property to tell
> > this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> > 
> > Before send a formal patch, I want to confirm this should work for your
> > HW design, like below simple hack:  
> 
> Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
> I would be happy to test the patch as soon as you send it.

Thanks Jun, it works here as well, on 6.1.27!

Best regards,
Luca

Francesco Dolcini May 8, 2023, 2:35 p.m. UTC | #18

On Mon, May 08, 2023 at 11:53:07AM +0000, Jun Li wrote:
> > -----Original Message-----
> > From: Francesco Dolcini <[e-mail]>
> > Sent: Monday, May 8, 2023 7:17 PM
> > To: Jun Li <[e-mail]>
> > Cc: Francesco Dolcini <[e-mail]>; Luca Ceresoli
> > <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; dl-linux-imx <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Krzysztof Kozlowski <[e-mail]>; Francesco Dolcini
> > <[e-mail]>; Xu Yang <[e-mail]>
> > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> > 
> > On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > > -----Original Message-----
> > > > From: Francesco Dolcini <[e-mail]>
> > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> > > > Cc: Francesco Dolcini <[e-mail]>;
> > [e-mail];
> > > > [e-mail]; [e-mail];
> > [e-mail];
> > > > [e-mail]; dl-linux-imx
> > <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Krzysztof Kozlowski
> > <[e-mail]>;
> > > > Francesco Dolcini <[e-mail]>
> > > > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling
> > runtime-pm
> > > >
> > > > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> > > > > On Fri, 5 May 2023 09:49:16 +0000
> > > > > Jun Li <[e-mail]> wrote:
> > > > > > Is your board design similar like Francesco's as below?
> > > > >
> > > > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > > > i.MX6ULL SoM, whose schematics are not public.
> > > >
> > > > I can confirm that it's the same.
> > >
> > > Thanks Francesco for the confirmation, had a check with design team,
> > > there is no status bit which can be used to judge the VDD_USB_CAP is
> > > powered or not, so we have to add a board level dts property to tell
> > > this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> > >
> > > Before send a formal patch, I want to confirm this should work for your
> > > HW design, like below simple hack:
> > 
> > Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
> > I would be happy to test the patch as soon as you send it.
> > 
> > 
> > With that said I had another issue that I assume is unrelated.
> > In addition to the USB Host port, we have an additional OTG one. This
> > interface has the same circuit WRT to the VBUS, however in this case
> > it's possible to read the VBUS using extcon, e.g. a standard GPIO input.
> > 
> > With that setup, while doing a role switch, I had a couple of time this
> > error:
> > 
> > [  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> > [  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC
> > 
> > that was recovered only doing an additional transition.
> > 
> > More complete logs here:
> > 
> > [  184.997619] usb 2-1: USB disconnect, device number 9
> > [  185.019620] ci_hdrc ci_hdrc.0: remove, state 1
> > [  185.024271] usb usb2: USB disconnect, device number 1
> > [  185.334975] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> > [  185.353857] ci_hdrc ci_hdrc.0: EHCI Host Controller
> > [  185.389670] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number
> > 2
> > [  185.470170] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
> > [  185.476097] usb usb2: New USB device found, idVendor=1d6b, idProduct=0002,
> > bcdDevice= 6.01
> > [  185.484527] usb usb2: New USB device strings: Mfr=3, Product=2,
> > SerialNumber=1
> > [  185.491811] usb usb2: Product: EHCI Host Controller
> > [  185.496704] usb usb2: Manufacturer: Linux 6.1.22-6.2.0+git.3b29299e5f60
> > ehci_hcd
> > [  185.504148] usb usb2: SerialNumber: ci_hdrc.0
> > [  185.531121] hub 2-0:1.0: USB hub found
> > [  185.542636] hub 2-0:1.0: 1 port detected
> > [  185.556586] mxs_phy 20c9000.usbphy: vbus is not valid
> > [  187.271684] ci_hdrc ci_hdrc.0: remove, state 4
> > [  187.276281] usb usb2: USB disconnect, device number 1
> > [  187.310421] ci_hdrc ci_hdrc.0: USB bus 2 deregistered
> > [  192.351452] ci_hdrc ci_hdrc.0: timeout waiting for 00000800 in OTGSC
> 
> I have 2 questions:
> 1. Can your OTG port work fine in device/gadget mode if you did not
>    do usb role switch?(e.g. the OTG port *never* switch to host mode
>    after system boot).

It works fine when started as device, never switching to host.

The switch to host and back to device is also working, but not in a
reliable way.

When not working doing a second tries always work.

> 2. Please show me your dts file node of your USB OTG port, I want to
>    Know how you config your OTG port. 

The DTS file is arch/arm/boot/dts/imx6ull-colibri-emmc-eval-v3.dts.

Just walk the dtsi includes till arch/arm/boot/dts/imx6ull-colibri.dtsi
and it's &usbotg1 node there.

Thanks
Francesco

Luca Ceresoli May 29, 2023, 10:18 a.m. UTC | #19

Hello Jun,

On Mon, 8 May 2023 15:17:56 +0200
Luca Ceresoli <[e-mail]> wrote:

> Hello Jun, Francesco,
> 
> On Mon, 8 May 2023 13:17:11 +0200
> Francesco Dolcini <[e-mail]> wrote:
> 
> > On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:  
> > > > -----Original Message-----
> > > > From: Francesco Dolcini <[e-mail]>
> > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > To: Luca Ceresoli <[e-mail]>; Jun Li <[e-mail]>
> > > > Cc: Francesco Dolcini <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; dl-linux-imx <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Krzysztof Kozlowski <[e-mail]>;
> > > > Francesco Dolcini <[e-mail]>
> > > > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> > > > 
> > > > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:    
> > > > > On Fri, 5 May 2023 09:49:16 +0000
> > > > > Jun Li <[e-mail]> wrote:    
> > > > > > Is your board design similar like Francesco's as below?    
> > > > >
> > > > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > > > i.MX6ULL SoM, whose schematics are not public.    
> > > > 
> > > > I can confirm that it's the same.    
> > > 
> > > Thanks Francesco for the confirmation, had a check with design team,
> > > there is no status bit which can be used to judge the VDD_USB_CAP is
> > > powered or not, so we have to add a board level dts property to tell
> > > this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> > > 
> > > Before send a formal patch, I want to confirm this should work for your
> > > HW design, like below simple hack:    
> > 
> > Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
> > I would be happy to test the patch as soon as you send it.  
> 
> Thanks Jun, it works here as well, on 6.1.27!

Have you managed to make progress on the patch after Francesco's and my
tests?

As I see it, a proper fix for mainline could be as simple as a new DT
property to describe this specific hardware configuration and a patch
to ignore the flag when the property is present. Is my understanding
correct?

Best regards,
Luca

Jun Li May 30, 2023, 11:22 a.m. UTC | #20

> -----Original Message-----
> From: Luca Ceresoli <[e-mail]>
> Sent: Monday, May 29, 2023 6:18 PM
> To: Jun Li <[e-mail]>
> Cc: Francesco Dolcini <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Krzysztof Kozlowski <[e-mail]>; Francesco Dolcini
> <[e-mail]>; Xu Yang <[e-mail]>
> Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm
> 
> Hello Jun,
> 
> On Mon, 8 May 2023 15:17:56 +0200
> Luca Ceresoli <[e-mail]> wrote:
> 
> > Hello Jun, Francesco,
> >
> > On Mon, 8 May 2023 13:17:11 +0200
> > Francesco Dolcini <[e-mail]> wrote:
> >
> > > On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > > > -----Original Message-----
> > > > > From: Francesco Dolcini <[e-mail]>
> > > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > > To: Luca Ceresoli <[e-mail]>; Jun Li
> <[e-mail]>
> > > > > Cc: Francesco Dolcini <[e-mail]>;
> [e-mail];
> > > > > [e-mail]; [e-mail];
> [e-mail];
> > > > > [e-mail]; dl-linux-imx
> <[e-mail]>;
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail]; [e-mail];
> > > > > [e-mail]; Krzysztof Kozlowski
> <[e-mail]>;
> > > > > Francesco Dolcini <[e-mail]>
> > > > > Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling
> runtime-pm
> > > > >
> > > > > On Fri, May 05, 2023 at 12:06:18PM +0200, Luca Ceresoli wrote:
> > > > > > On Fri, 5 May 2023 09:49:16 +0000
> > > > > > Jun Li <[e-mail]> wrote:
> > > > > > > Is your board design similar like Francesco's as below?
> > > > > >
> > > > > > Possibly, but I'm afraid I can't say: I am using the Toradex Colibri
> > > > > > i.MX6ULL SoM, whose schematics are not public.
> > > > >
> > > > > I can confirm that it's the same.
> > > >
> > > > Thanks Francesco for the confirmation, had a check with design team,
> > > > there is no status bit which can be used to judge the VDD_USB_CAP is
> > > > powered or not, so we have to add a board level dts property to tell
> > > > this usb phy driver to bypass MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS.
> > > >
> > > > Before send a formal patch, I want to confirm this should work for
> your
> > > > HW design, like below simple hack:
> > >
> > > Thanks Li Jun, I tested it with v6.3.1 kernel and it's all good.
> > > I would be happy to test the patch as soon as you send it.
> >
> > Thanks Jun, it works here as well, on 6.1.27!
> 
> Have you managed to make progress on the patch after Francesco's and my
> tests?
> 
> As I see it, a proper fix for mainline could be as simple as a new DT
> property to describe this specific hardware configuration and a patch
> to ignore the flag when the property is present. Is my understanding
> correct?

Yes, your understanding is correct, talked with Xu(in CC), he will take this
soon.

Thanks
Li Jun

> 
> Best regards,
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootl
> in.com%2F&data=05%7C01%7Cjun.li%40nxp.com%7C54d7d4bc194545db610608db602
> e0ca6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638209523121178887%7
> CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3SbXTUvvRwcsJoBIhPoypWDLDMpj2m
> z09JC34t3rNh4%3D&reserved=0

Francesco Dolcini July 6, 2023, 10:23 a.m. UTC | #21

Hello Luca,

On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> Yes, your understanding is correct, talked with Xu(in CC), he will take this
> soon.

A series was posted
https://lore.kernel.org/all/[e-mail]/,
I had no time to try or look at it yet.

Francesco

Luca Ceresoli July 17, 2023, 4:45 p.m. UTC | #22

Ciao Francesco,

On Thu, 6 Jul 2023 12:23:43 +0200
Francesco Dolcini <[e-mail]> wrote:

> Hello Luca,
> 
> On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > soon.  
> 
> A series was posted
> https://lore.kernel.org/all/[e-mail]/,
> I had no time to try or look at it yet.

Thanks for keeping me up to date on this topic, which is still totally
relevant to me.

I looked at the series, but it does not seem to be addressing the
problem with USB host not detecting new devices when VBUS is not
directly connected, e.g. in the Colibri imx6ull SoM.

Xu, do you confirm the series at the link is _not_ solving the problem
being discussed here?

Best regards,

Luca

Francesco Dolcini July 18, 2023, 8:14 a.m. UTC | #23

On Mon, Jul 17, 2023 at 06:45:37PM +0200, Luca Ceresoli wrote:
> On Thu, 6 Jul 2023 12:23:43 +0200
> Francesco Dolcini <[e-mail]> wrote:
> > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > soon.  
> > 
> > A series was posted
> > https://lore.kernel.org/all/[e-mail]/,
> > I had no time to try or look at it yet.
> 
> Thanks for keeping me up to date on this topic, which is still totally
> relevant to me.
> 
> I looked at the series, but it does not seem to be addressing the
> problem with USB host not detecting new devices when VBUS is not
> directly connected, e.g. in the Colibri imx6ull SoM.

I recall having tried something similar, it could handle also the
colibri imx6ull use case. Were you able to try it? These days I am busy
with other tasks and I was not able to find the time (yet).

Francesco

Xu Yang July 18, 2023, 8:31 a.m. UTC | #24

> -----Original Message-----
> 
> Ciao Francesco,
> 
> On Thu, 6 Jul 2023 12:23:43 +0200
> Francesco Dolcini <[e-mail]> wrote:
> 
> > Hello Luca,
> >
> > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > soon.
> >
> > A series was posted
> >
> > I had no time to try or look at it yet.
> 
> Thanks for keeping me up to date on this topic, which is still totally
> relevant to me.
> 
> I looked at the series, but it does not seem to be addressing the
> problem with USB host not detecting new devices when VBUS is not
> directly connected, e.g. in the Colibri imx6ull SoM.
> 
> Xu, do you confirm the series at the link is _not_ solving the problem
> being discussed here?

Have you tried this patchset? The upstream driver couldn't get correct
USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
what this patchset is trying to fix. With this patch, condition 
"(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
will never be called. So I think it doesn't matter whether VBUS is connected
or not when act as host mode. If you still have issue after apply this patchset,
please let me know.

Thanks,
Xu Yang

> 
> Best regards,
> 
> Luca
>

Luca Ceresoli July 18, 2023, 12:25 p.m. UTC | #25

Hello Xu,

On Tue, 18 Jul 2023 08:31:48 +0000
Xu Yang <[e-mail]> wrote:

> > -----Original Message-----
> > 
> > Ciao Francesco,
> > 
> > On Thu, 6 Jul 2023 12:23:43 +0200
> > Francesco Dolcini <[e-mail]> wrote:
> >   
> > > Hello Luca,
> > >
> > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:  
> > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > soon.  
> > >
> > > A series was posted
> > >
> > > I had no time to try or look at it yet.  
> > 
> > Thanks for keeping me up to date on this topic, which is still totally
> > relevant to me.
> > 
> > I looked at the series, but it does not seem to be addressing the
> > problem with USB host not detecting new devices when VBUS is not
> > directly connected, e.g. in the Colibri imx6ull SoM.
> > 
> > Xu, do you confirm the series at the link is _not_ solving the problem
> > being discussed here?  
> 
> Have you tried this patchset? The upstream driver couldn't get correct
> USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> what this patchset is trying to fix. With this patch, condition 
> "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> will never be called. So I think it doesn't matter whether VBUS is connected
> or not when act as host mode. If you still have issue after apply this patchset,
> please let me know.

I tested this patchset on top of v6.5-rc2 and I confirm USB detection
is still broken on the Colibri iMX6ULL. With or without the patches
the behavior is the same: USB devices are detected only during boot,
and anything connected after boot are never detected.

For the archives, I'm replying also to the patch series.

Luca

Xu Yang July 19, 2023, 11:23 a.m. UTC | #26

Hi Luca,

> -----Original Message-----
> 
> Hello Xu,
> 
> On Tue, 18 Jul 2023 08:31:48 +0000
> Xu Yang <[e-mail]> wrote:
> 
> > > -----Original Message-----
> > >
> > > Ciao Francesco,
> > >
> > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > Francesco Dolcini <[e-mail]> wrote:
> > >
> > > > Hello Luca,
> > > >
> > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > soon.
> > > >
> > > > A series was posted
> > > >
> > > > I had no time to try or look at it yet.
> > >
> > > Thanks for keeping me up to date on this topic, which is still totally
> > > relevant to me.
> > >
> > > I looked at the series, but it does not seem to be addressing the
> > > problem with USB host not detecting new devices when VBUS is not
> > > directly connected, e.g. in the Colibri imx6ull SoM.
> > >
> > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > being discussed here?
> >
> > Have you tried this patchset? The upstream driver couldn't get correct
> > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > what this patchset is trying to fix. With this patch, condition
> > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > will never be called. So I think it doesn't matter whether VBUS is connected
> > or not when act as host mode. If you still have issue after apply this patchset,
> > please let me know.
> 
> I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> is still broken on the Colibri iMX6ULL. With or without the patches
> the behavior is the same: USB devices are detected only during boot,
> and anything connected after boot are never detected.

Thanks for your feedback. As you said this issue will disappear with below change, right?

	diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
	index e1a2b2ea098b..ec5ee790455e 100644
	--- a/drivers/usb/phy/phy-mxs-usb.c
	+++ b/drivers/usb/phy/phy-mxs-usb.c
	@@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
	 };

	 static const struct mxs_phy_data imx6ul_phy_data = {
	-       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
	 };

	 static const struct mxs_phy_data imx7ulp_phy_data = {

So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
Could you please help to find which line to comment to make this issue disappear?

Thanks,
Xu Yang

> 
> For the archives, I'm replying also to the patch series.
> 
> Luca
>

Luca Ceresoli July 19, 2023, 4:48 p.m. UTC | #27

Hello Xu,

thanks for the follow up.

On Wed, 19 Jul 2023 11:23:26 +0000
Xu Yang <[e-mail]> wrote:

> Hi Luca,
> 
> > -----Original Message-----
> > 
> > Hello Xu,
> > 
> > On Tue, 18 Jul 2023 08:31:48 +0000
> > Xu Yang <[e-mail]> wrote:
> >   
> > > > -----Original Message-----
> > > >
> > > > Ciao Francesco,
> > > >
> > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > Francesco Dolcini <[e-mail]> wrote:
> > > >  
> > > > > Hello Luca,
> > > > >
> > > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:  
> > > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > > soon.  
> > > > >
> > > > > A series was posted
> > > > >
> > > > > I had no time to try or look at it yet.  
> > > >
> > > > Thanks for keeping me up to date on this topic, which is still totally
> > > > relevant to me.
> > > >
> > > > I looked at the series, but it does not seem to be addressing the
> > > > problem with USB host not detecting new devices when VBUS is not
> > > > directly connected, e.g. in the Colibri imx6ull SoM.
> > > >
> > > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > > being discussed here?  
> > >
> > > Have you tried this patchset? The upstream driver couldn't get correct
> > > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > > what this patchset is trying to fix. With this patch, condition
> > > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > > will never be called. So I think it doesn't matter whether VBUS is connected
> > > or not when act as host mode. If you still have issue after apply this patchset,
> > > please let me know.  
> > 
> > I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> > is still broken on the Colibri iMX6ULL. With or without the patches
> > the behavior is the same: USB devices are detected only during boot,
> > and anything connected after boot are never detected.  
> 
> Thanks for your feedback. As you said this issue will disappear with below change, right?
> 
> 	diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> 	index e1a2b2ea098b..ec5ee790455e 100644
> 	--- a/drivers/usb/phy/phy-mxs-usb.c
> 	+++ b/drivers/usb/phy/phy-mxs-usb.c
> 	@@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> 	 };
> 
> 	 static const struct mxs_phy_data imx6ul_phy_data = {
> 	-       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> 	 };
> 
> 	 static const struct mxs_phy_data imx7ulp_phy_data = {

Exactly.

> So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
> Could you please help to find which line to comment to make this issue disappear?

I did some tests and detection works by doing _any_ of the following
two changes (or both of them).

Change 1:

--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -359,10 +359,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
        void __iomem *base = mxs_phy->phy.io_priv;
        u32 reg;
 
-       if (disconnect)
-               writel_relaxed(BM_USBPHY_DEBUG_CLKGATE,
-                       base + HW_USBPHY_DEBUG_CLR);
-
        if (mxs_phy->port_id == 0) {
                reg = disconnect ? ANADIG_USB1_LOOPBACK_SET
                        : ANADIG_USB1_LOOPBACK_CLR;

Change 2:

--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -372,9 +372,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
        } else if (mxs_phy->port_id == 1) {
                reg = disconnect ? ANADIG_USB2_LOOPBACK_SET
                        : ANADIG_USB2_LOOPBACK_CLR;
-               regmap_write(mxs_phy->regmap_anatop, reg,
-                       BM_ANADIG_USB2_LOOPBACK_UTMI_DIG_TST1 |
-                       BM_ANADIG_USB2_LOOPBACK_TSTI_TX_EN);
        }
 
        if (!disconnect)

I hope this clarifies something to you.

Luca

Xu Yang July 20, 2023, 10:13 a.m. UTC | #28

Hi Luca,

> > > -----Original Message-----
> > >
> > > Hello Xu,
> > >
> > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > Xu Yang <[e-mail]> wrote:
> > >
> > > > > -----Original Message-----
> > > > >
> > > > > Ciao Francesco,
> > > > >
> > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > Francesco Dolcini <[e-mail]> wrote:
> > > > >
> > > > > > Hello Luca,
> > > > > >
> > > > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > > > soon.
> > > > > >
> > > > > > A series was posted
> > > > > >
> > > > > > I had no time to try or look at it yet.
> > > > >
> > > > > Thanks for keeping me up to date on this topic, which is still totally
> > > > > relevant to me.
> > > > >
> > > > > I looked at the series, but it does not seem to be addressing the
> > > > > problem with USB host not detecting new devices when VBUS is not
> > > > > directly connected, e.g. in the Colibri imx6ull SoM.
> > > > >
> > > > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > > > being discussed here?
> > > >
> > > > Have you tried this patchset? The upstream driver couldn't get correct
> > > > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > > > what this patchset is trying to fix. With this patch, condition
> > > > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > > > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > > > will never be called. So I think it doesn't matter whether VBUS is connected
> > > > or not when act as host mode. If you still have issue after apply this patchset,
> > > > please let me know.
> > >
> > > I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> > > is still broken on the Colibri iMX6ULL. With or without the patches
> > > the behavior is the same: USB devices are detected only during boot,
> > > and anything connected after boot are never detected.
> >
> > Thanks for your feedback. As you said this issue will disappear with below change, right?
> >
> >       diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> >       index e1a2b2ea098b..ec5ee790455e 100644
> >       --- a/drivers/usb/phy/phy-mxs-usb.c
> >       +++ b/drivers/usb/phy/phy-mxs-usb.c
> >       @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> >        };
> >
> >        static const struct mxs_phy_data imx6ul_phy_data = {
> >       -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> >        };
> >
> >        static const struct mxs_phy_data imx7ulp_phy_data = {
> 
> Exactly.
> 
> > So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
> > Could you please help to find which line to comment to make this issue disappear?

To correct what I said:  __mxs_phy_disconnect_line(mxs_phy, false) should
be called.

I think the enable wakeup sequence should be follow for host-only port:
mxs_phy_set_wakeup(mxs_phy, true)
    mxs_phy_disconnect_line(mxs_phy, true);
        __mxs_phy_disconnect_line(mxs_phy, false);

And disable wakeup sequence:
mxs_phy_set_wakeup(mxs_phy, false)
    mxs_phy_disconnect_line(mxs_phy, false);
        __mxs_phy_disconnect_line(mxs_phy, false);

So "bool variable disconnect is false" all the time.

> 
> I did some tests and detection works by doing _any_ of the following
> two changes (or both of them).
> 
> Change 1:
> 
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -359,10 +359,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
>         void __iomem *base = mxs_phy->phy.io_priv;
>         u32 reg;
> 
> -       if (disconnect)
> -               writel_relaxed(BM_USBPHY_DEBUG_CLKGATE,
> -                       base + HW_USBPHY_DEBUG_CLR);
> -

Since disconnect = false, this code didn't get executed all the time.
Remove this will have no impact. But your test results didn't align
to this. Could you please help check the sequence? Is disconnect
true or false when __mxs_phy_disconnect_line is called?

Thanks,
Xu Yang

>         if (mxs_phy->port_id == 0) {
>                 reg = disconnect ? ANADIG_USB1_LOOPBACK_SET
>                         : ANADIG_USB1_LOOPBACK_CLR;
> 
> Change 2:
> 
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -372,9 +372,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
>         } else if (mxs_phy->port_id == 1) {
>                 reg = disconnect ? ANADIG_USB2_LOOPBACK_SET
>                         : ANADIG_USB2_LOOPBACK_CLR;
> -               regmap_write(mxs_phy->regmap_anatop, reg,
> -                       BM_ANADIG_USB2_LOOPBACK_UTMI_DIG_TST1 |
> -                       BM_ANADIG_USB2_LOOPBACK_TSTI_TX_EN);
>         }
> 
>         if (!disconnect)
> 
> I hope this clarifies something to you.
> 
> Luca
>

Luca Ceresoli July 20, 2023, 12:49 p.m. UTC | #29

Hi Xu,

On Thu, 20 Jul 2023 10:13:57 +0000
Xu Yang <[e-mail]> wrote:

> Hi Luca,
> 
> > > > -----Original Message-----
> > > >
> > > > Hello Xu,
> > > >
> > > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > > Xu Yang <[e-mail]> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > >
> > > > > > Ciao Francesco,
> > > > > >
> > > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > > Francesco Dolcini <[e-mail]> wrote:
> > > > > >  
> > > > > > > Hello Luca,
> > > > > > >
> > > > > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:  
> > > > > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > > > > soon.  
> > > > > > >
> > > > > > > A series was posted
> > > > > > >
> > > > > > > I had no time to try or look at it yet.  
> > > > > >
> > > > > > Thanks for keeping me up to date on this topic, which is still totally
> > > > > > relevant to me.
> > > > > >
> > > > > > I looked at the series, but it does not seem to be addressing the
> > > > > > problem with USB host not detecting new devices when VBUS is not
> > > > > > directly connected, e.g. in the Colibri imx6ull SoM.
> > > > > >
> > > > > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > > > > being discussed here?  
> > > > >
> > > > > Have you tried this patchset? The upstream driver couldn't get correct
> > > > > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > > > > what this patchset is trying to fix. With this patch, condition
> > > > > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > > > > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > > > > will never be called. So I think it doesn't matter whether VBUS is connected
> > > > > or not when act as host mode. If you still have issue after apply this patchset,
> > > > > please let me know.  
> > > >
> > > > I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> > > > is still broken on the Colibri iMX6ULL. With or without the patches
> > > > the behavior is the same: USB devices are detected only during boot,
> > > > and anything connected after boot are never detected.  
> > >
> > > Thanks for your feedback. As you said this issue will disappear with below change, right?
> > >
> > >       diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> > >       index e1a2b2ea098b..ec5ee790455e 100644
> > >       --- a/drivers/usb/phy/phy-mxs-usb.c
> > >       +++ b/drivers/usb/phy/phy-mxs-usb.c
> > >       @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> > >        };
> > >
> > >        static const struct mxs_phy_data imx6ul_phy_data = {
> > >       -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> > >        };
> > >
> > >        static const struct mxs_phy_data imx7ulp_phy_data = {  
> > 
> > Exactly.
> >   
> > > So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
> > > Could you please help to find which line to comment to make this issue disappear?  
> 
> To correct what I said:  __mxs_phy_disconnect_line(mxs_phy, false) should
> be called.
> 
> I think the enable wakeup sequence should be follow for host-only port:
> mxs_phy_set_wakeup(mxs_phy, true)
>     mxs_phy_disconnect_line(mxs_phy, true);
>         __mxs_phy_disconnect_line(mxs_phy, false);
> 
> And disable wakeup sequence:
> mxs_phy_set_wakeup(mxs_phy, false)
>     mxs_phy_disconnect_line(mxs_phy, false);
>         __mxs_phy_disconnect_line(mxs_phy, false);
> 
> So "bool variable disconnect is false" all the time.
> 
> > 
> > I did some tests and detection works by doing _any_ of the following
> > two changes (or both of them).
> > 
> > Change 1:
> > 
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -359,10 +359,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
> >         void __iomem *base = mxs_phy->phy.io_priv;
> >         u32 reg;
> > 
> > -       if (disconnect)
> > -               writel_relaxed(BM_USBPHY_DEBUG_CLKGATE,
> > -                       base + HW_USBPHY_DEBUG_CLR);
> > -  
> 
> Since disconnect = false, this code didn't get executed all the time.
> Remove this will have no impact. But your test results didn't align
> to this. Could you please help check the sequence? Is disconnect
> true or false when __mxs_phy_disconnect_line is called?

What I observe is that __mxs_phy_disconnect_line(..., true) is called.
This happens during boot and after unplugging a device in case one was
detected during boot.

This is because in mxs_phy_disconnect_line() [0]:
 - on = 1
 - !vbus_is_on = 1
 - !mxs_phy_is_otg_host(mxs_phy) = 1

Which one(s) of those three would you expect to be 0?

Some additional info about the !mxs_phy_is_otg_host(mxs_phy) value:
with or without CONFIG_USB_OTG, it always has the same value because
phyctrl always has the BM_USBPHY_CTRL_OTG_ID_VALUE bit set.

[0]
https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/usb/phy/phy-mxs-usb.c#L415

Luca

Xu Yang July 21, 2023, 2:06 a.m. UTC | #30

Hi Luca,

> -----Original Message-----
> 
> Hi Xu,
> 
> On Thu, 20 Jul 2023 10:13:57 +0000
> Xu Yang <[e-mail]> wrote:
> 
> > Hi Luca,
> >
> > > > > -----Original Message-----
> > > > >
> > > > > Hello Xu,
> > > > >
> > > > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > > > Xu Yang <[e-mail]> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > >
> > > > > > > Ciao Francesco,
> > > > > > >
> > > > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > > > Francesco Dolcini <[e-mail]> wrote:
> > > > > > >
> > > > > > > > Hello Luca,
> > > > > > > >
> > > > > > > > On Tue, May 30, 2023 at 11:22:51AM +0000, Jun Li wrote:
> > > > > > > > > Yes, your understanding is correct, talked with Xu(in CC), he will take this
> > > > > > > > > soon.
> > > > > > > >
> > > > > > > > A series was posted
> > > > > > > >
> > > > > > > > I had no time to try or look at it yet.
> > > > > > >
> > > > > > > Thanks for keeping me up to date on this topic, which is still totally
> > > > > > > relevant to me.
> > > > > > >
> > > > > > > I looked at the series, but it does not seem to be addressing the
> > > > > > > problem with USB host not detecting new devices when VBUS is not
> > > > > > > directly connected, e.g. in the Colibri imx6ull SoM.
> > > > > > >
> > > > > > > Xu, do you confirm the series at the link is _not_ solving the problem
> > > > > > > being discussed here?
> > > > > >
> > > > > > Have you tried this patchset? The upstream driver couldn't get correct
> > > > > > USB role from HW_USBPHY_CTRL register when the ID pin is float. This is
> > > > > > what this patchset is trying to fix. With this patch, condition
> > > > > > "(!vbus_is_on && !mxs_phy_is_otg_host(mxs_phy)" will always be false when
> > > > > > controller acts as host role, then __mxs_phy_disconnect_line(phy, true)
> > > > > > will never be called. So I think it doesn't matter whether VBUS is connected
> > > > > > or not when act as host mode. If you still have issue after apply this patchset,
> > > > > > please let me know.
> > > > >
> > > > > I tested this patchset on top of v6.5-rc2 and I confirm USB detection
> > > > > is still broken on the Colibri iMX6ULL. With or without the patches
> > > > > the behavior is the same: USB devices are detected only during boot,
> > > > > and anything connected after boot are never detected.
> > > >
> > > > Thanks for your feedback. As you said this issue will disappear with below change, right?
> > > >
> > > >       diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> > > >       index e1a2b2ea098b..ec5ee790455e 100644
> > > >       --- a/drivers/usb/phy/phy-mxs-usb.c
> > > >       +++ b/drivers/usb/phy/phy-mxs-usb.c
> > > >       @@ -178,7 +178,6 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> > > >        };
> > > >
> > > >        static const struct mxs_phy_data imx6ul_phy_data = {
> > > >       -       .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> > > >        };
> > > >
> > > >        static const struct mxs_phy_data imx7ulp_phy_data = {
> > >
> > > Exactly.
> > >
> > > > So I guess something in __mxs_phy_disconnect_line(mxs_phy, true) is causing this behavior.
> > > > Could you please help to find which line to comment to make this issue disappear?
> >
> > To correct what I said:  __mxs_phy_disconnect_line(mxs_phy, false) should
> > be called.
> >
> > I think the enable wakeup sequence should be follow for host-only port:
> > mxs_phy_set_wakeup(mxs_phy, true)
> >     mxs_phy_disconnect_line(mxs_phy, true);
> >         __mxs_phy_disconnect_line(mxs_phy, false);
> >
> > And disable wakeup sequence:
> > mxs_phy_set_wakeup(mxs_phy, false)
> >     mxs_phy_disconnect_line(mxs_phy, false);
> >         __mxs_phy_disconnect_line(mxs_phy, false);
> >
> > So "bool variable disconnect is false" all the time.
> >
> > >
> > > I did some tests and detection works by doing _any_ of the following
> > > two changes (or both of them).
> > >
> > > Change 1:
> > >
> > > --- a/drivers/usb/phy/phy-mxs-usb.c
> > > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > > @@ -359,10 +359,6 @@ static void __mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool disconnect)
> > >         void __iomem *base = mxs_phy->phy.io_priv;
> > >         u32 reg;
> > >
> > > -       if (disconnect)
> > > -               writel_relaxed(BM_USBPHY_DEBUG_CLKGATE,
> > > -                       base + HW_USBPHY_DEBUG_CLR);
> > > -
> >
> > Since disconnect = false, this code didn't get executed all the time.
> > Remove this will have no impact. But your test results didn't align
> > to this. Could you please help check the sequence? Is disconnect
> > true or false when __mxs_phy_disconnect_line is called?
> 
> What I observe is that __mxs_phy_disconnect_line(..., true) is called.
> This happens during boot and after unplugging a device in case one was
> detected during boot.
> 
> This is because in mxs_phy_disconnect_line() [0]:
>  - on = 1
>  - !vbus_is_on = 1
>  - !mxs_phy_is_otg_host(mxs_phy) = 1
> 
> Which one(s) of those three would you expect to be 0?
> 
> Some additional info about the !mxs_phy_is_otg_host(mxs_phy) value:
> with or without CONFIG_USB_OTG, it always has the same value because
> phyctrl always has the BM_USBPHY_CTRL_OTG_ID_VALUE bit set.
> 

I guess you didn't apply my whole patchset.

[1/3] usb: chipidea: add USB PHY event
[2/3] usb: phy: mxs: fix getting wrong state with mxs_phy_is_otg_host()
[3/3] usb: phy: mxs: disconnect line when USB charger is attached

The 2nd patch is going to fix the issue of mxs_phy_is_otg_host():
https://lore.kernel.org/all/[e-mail]/

Our design is not to disconnect line when the controller is acting as host mode.

Thanks,
Xu Yang

Luca Ceresoli Sept. 29, 2023, 1:37 p.m. UTC | #31

Hello Xu,

On Tue, 25 Jul 2023 12:23:43 +0000
Xu Yang <[e-mail]> wrote:
...
> > I tested again now with the 3 patches applied and found that with
> > CONFIG_USB_OTG=y it works, and detects a device when plugged. Good!
> > 
> > However with CONFIG_USB_OTG disabled it is still not working. In this
> > case obviously mxs_phy_is_otg_host() returns always false, even though
> > las_event is 2 (USB_EVENT_ID). This is what I get during boot with no
> > device connected:  
> 
> Yes, I may need to remove CONFIG_USB_OTG condifion. Will handle this later.

Did you manage to make progress with this work? It looked like very
close to be fixed in the proper way with your latest patch iteration.

I'd definitely happy to test new patches as soon as you have any.

Best regards,
Luca

Xu Yang Oct. 10, 2023, 10:52 a.m. UTC | #32

Hi Luca,

> Hello Xu,
> 
> On Tue, 25 Jul 2023 12:23:43 +0000
> Xu Yang <[e-mail]> wrote:
> ...
> > > I tested again now with the 3 patches applied and found that with
> > > CONFIG_USB_OTG=y it works, and detects a device when plugged. Good!
> > >
> > > However with CONFIG_USB_OTG disabled it is still not working. In this
> > > case obviously mxs_phy_is_otg_host() returns always false, even though
> > > las_event is 2 (USB_EVENT_ID). This is what I get during boot with no
> > > device connected:
> >
> > Yes, I may need to remove CONFIG_USB_OTG condifion. Will handle this later.
> 
> Did you manage to make progress with this work? It looked like very
> close to be fixed in the proper way with your latest patch iteration.

I'm a little busy these days. I may handle this when I'm free or you can also
fix it.

Thanks,
Xu Yang

> 
> I'd definitely happy to test new patches as soon as you have any.
> 
> Best regards,
> Luca
>

Patch

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 5ae16368a0c7..5078d0695eb7 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -434,6 +434,9 @@  static int ci_hdrc_imx_probe(struct platform_device *pdev)
 		usb_phy_init(pdata.usb_phy);
 	}
 
+	if (of_property_read_bool(np, "disable-runtime-pm"))
+		pdata.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
+
 	if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
 		data->supports_runtime_pm = true;