LinuxLists.cc - [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

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

2023-05-04 16:51:53

[permalink] [raw]

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.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/[email protected]/

> 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 <[email protected]>
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 <[email protected]>
(cherry picked from commit 2286cb30feedd6f4a5cb82a0f0af5aa3a04ab698)
Signed-off-by: Vipul Kumar <[email protected]>
Signed-off-by: Srikanth Krishnakar <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
(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 <[email protected]>
* 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 <[email protected]>
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 <[email protected]>
(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

2023-05-05 09:41:09

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hello Francesco,

On Thu, 4 May 2023 18:50:14 +0200
Francesco Dolcini <[email protected]> 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/[email protected]/

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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-05-05 10:03:11

by Jun Li

[permalink] [raw]

Subject: RE: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: Friday, May 5, 2023 12:50 AM
> To: Luca Ceresoli <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Jun Li <[email protected]>;
> [email protected]; [email protected];
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Krzysztof Kozlowski
> <[email protected]>; Francesco Dolcini
> <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> (cherry picked from commit 2286cb30feedd6f4a5cb82a0f0af5aa3a04ab698)
> Signed-off-by: Vipul Kumar <[email protected]>
> Signed-off-by: Srikanth Krishnakar <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> (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 <[email protected]>
> * 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 <[email protected]>
> 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 <[email protected]>
> (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

2023-05-05 10:36:21

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hello Jun,

On Fri, 5 May 2023 09:49:16 +0000
Jun Li <[email protected]> wrote:

> > -----Original Message-----
> > From: Francesco Dolcini <[email protected]>
> > Sent: Friday, May 5, 2023 12:50 AM
> > To: Luca Ceresoli <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; Jun Li <[email protected]>;
> > [email protected]; [email protected];
> > dl-linux-imx <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Krzysztof Kozlowski
> > <[email protected]>; Francesco Dolcini
> > <[email protected]>
> > 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-05-05 11:06:32

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

On Fri, May 05, 2023 at 11:23:51AM +0200, Luca Ceresoli wrote:
> On Thu, 4 May 2023 18:50:14 +0200
> Francesco Dolcini <[email protected]> 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/[email protected]/
>
> 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

2023-05-05 11:10:52

[permalink] [raw]

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 <[email protected]> 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

2023-05-06 09:10:56

by Jun Li

[permalink] [raw]

Subject: RE: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: Friday, May 5, 2023 7:00 PM
> To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> Cc: Francesco Dolcini <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Krzysztof Kozlowski <[email protected]>;
> Francesco Dolcini <[email protected]>
> 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 <[email protected]> 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

2023-05-08 11:56:19

[permalink] [raw]

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 <[email protected]>
> > Sent: Friday, May 5, 2023 7:00 PM
> > To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> > Cc: Francesco Dolcini <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Krzysztof Kozlowski <[email protected]>;
> > Francesco Dolcini <[email protected]>
> > 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 <[email protected]> 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

2023-05-08 12:29:29

by Jun Li

[permalink] [raw]

Subject: RE: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: Monday, May 8, 2023 7:17 PM
> To: Jun Li <[email protected]>
> Cc: Francesco Dolcini <[email protected]>; Luca Ceresoli
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Krzysztof Kozlowski <[email protected]>; Francesco Dolcini
> <[email protected]>; Xu Yang <[email protected]>
> 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 <[email protected]>
> > > Sent: Friday, May 5, 2023 7:00 PM
> > > To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> > > Cc: Francesco Dolcini <[email protected]>;
> [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; dl-linux-imx
> <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; Krzysztof Kozlowski
> <[email protected]>;
> > > Francesco Dolcini <[email protected]>
> > > 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 <[email protected]> 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

2023-05-08 13:24:22

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hello Jun, Francesco,

On Mon, 8 May 2023 13:17:11 +0200
Francesco Dolcini <[email protected]> wrote:

> On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > -----Original Message-----
> > > From: Francesco Dolcini <[email protected]>
> > > Sent: Friday, May 5, 2023 7:00 PM
> > > To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> > > Cc: Francesco Dolcini <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; dl-linux-imx <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; Krzysztof Kozlowski <[email protected]>;
> > > Francesco Dolcini <[email protected]>
> > > 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 <[email protected]> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-05-08 14:59:00

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

On Mon, May 08, 2023 at 11:53:07AM +0000, Jun Li wrote:
> > -----Original Message-----
> > From: Francesco Dolcini <[email protected]>
> > Sent: Monday, May 8, 2023 7:17 PM
> > To: Jun Li <[email protected]>
> > Cc: Francesco Dolcini <[email protected]>; Luca Ceresoli
> > <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Krzysztof Kozlowski <[email protected]>; Francesco Dolcini
> > <[email protected]>; Xu Yang <[email protected]>
> > 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 <[email protected]>
> > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> > > > Cc: Francesco Dolcini <[email protected]>;
> > [email protected];
> > > > [email protected]; [email protected];
> > [email protected];
> > > > [email protected]; dl-linux-imx
> > <[email protected]>;
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; Krzysztof Kozlowski
> > <[email protected]>;
> > > > Francesco Dolcini <[email protected]>
> > > > 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 <[email protected]> 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

2023-05-29 10:32:30

[permalink] [raw]

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 <[email protected]> wrote:

> Hello Jun, Francesco,
>
> On Mon, 8 May 2023 13:17:11 +0200
> Francesco Dolcini <[email protected]> wrote:
>
> > On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > > -----Original Message-----
> > > > From: Francesco Dolcini <[email protected]>
> > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > To: Luca Ceresoli <[email protected]>; Jun Li <[email protected]>
> > > > Cc: Francesco Dolcini <[email protected]>; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; dl-linux-imx <[email protected]>;
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; Krzysztof Kozlowski <[email protected]>;
> > > > Francesco Dolcini <[email protected]>
> > > > 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 <[email protected]> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-05-30 11:34:33

by Jun Li

[permalink] [raw]

Subject: RE: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

> -----Original Message-----
> From: Luca Ceresoli <[email protected]>
> Sent: Monday, May 29, 2023 6:18 PM
> To: Jun Li <[email protected]>
> Cc: Francesco Dolcini <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Krzysztof Kozlowski <[email protected]>; Francesco Dolcini
> <[email protected]>; Xu Yang <[email protected]>
> 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 <[email protected]> wrote:
>
> > Hello Jun, Francesco,
> >
> > On Mon, 8 May 2023 13:17:11 +0200
> > Francesco Dolcini <[email protected]> wrote:
> >
> > > On Sat, May 06, 2023 at 09:02:39AM +0000, Jun Li wrote:
> > > > > -----Original Message-----
> > > > > From: Francesco Dolcini <[email protected]>
> > > > > Sent: Friday, May 5, 2023 7:00 PM
> > > > > To: Luca Ceresoli <[email protected]>; Jun Li
> <[email protected]>
> > > > > Cc: Francesco Dolcini <[email protected]>;
> [email protected];
> > > > > [email protected]; [email protected];
> [email protected];
> > > > > [email protected]; dl-linux-imx
> <[email protected]>;
> > > > > [email protected]; [email protected];
> > > > > [email protected]; [email protected]; [email protected];
> > > > > [email protected]; Krzysztof Kozlowski
> <[email protected]>;
> > > > > Francesco Dolcini <[email protected]>
> > > > > 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 <[email protected]> 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

2023-07-06 10:29:30

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

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/[email protected]/,
I had no time to try or look at it yet.

Francesco

2023-07-17 17:00:22

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Ciao Francesco,

On Thu, 6 Jul 2023 12:23:43 +0200
Francesco Dolcini <[email protected]> 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/[email protected]/,
> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-07-18 08:32:59

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

On Mon, Jul 17, 2023 at 06:45:37PM +0200, Luca Ceresoli wrote:
> On Thu, 6 Jul 2023 12:23:43 +0200
> Francesco Dolcini <[email protected]> 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/[email protected]/,
> > 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

2023-07-18 08:46:28

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

> -----Original Message-----
>
> Ciao Francesco,
>
> On Thu, 6 Jul 2023 12:23:43 +0200
> Francesco Dolcini <[email protected]> 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
>

2023-07-18 12:46:08

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hello Xu,

On Tue, 18 Jul 2023 08:31:48 +0000
Xu Yang <[email protected]> wrote:

> > -----Original Message-----
> >
> > Ciao Francesco,
> >
> > On Thu, 6 Jul 2023 12:23:43 +0200
> > Francesco Dolcini <[email protected]> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-07-19 11:38:19

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hi Luca,

> -----Original Message-----
>
> Hello Xu,
>
> On Tue, 18 Jul 2023 08:31:48 +0000
> Xu Yang <[email protected]> wrote:
>
> > > -----Original Message-----
> > >
> > > Ciao Francesco,
> > >
> > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > Francesco Dolcini <[email protected]> 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
>

2023-07-19 17:30:54

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hello Xu,

thanks for the follow up.

On Wed, 19 Jul 2023 11:23:26 +0000
Xu Yang <[email protected]> wrote:

> Hi Luca,
>
> > -----Original Message-----
> >
> > Hello Xu,
> >
> > On Tue, 18 Jul 2023 08:31:48 +0000
> > Xu Yang <[email protected]> wrote:
> >
> > > > -----Original Message-----
> > > >
> > > > Ciao Francesco,
> > > >
> > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > Francesco Dolcini <[email protected]> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-07-20 10:39:42

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hi Luca,

> > > -----Original Message-----
> > >
> > > Hello Xu,
> > >
> > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > Xu Yang <[email protected]> wrote:
> > >
> > > > > -----Original Message-----
> > > > >
> > > > > Ciao Francesco,
> > > > >
> > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > Francesco Dolcini <[email protected]> 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
>

2023-07-20 13:15:58

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH v2 2/3] usb: chipidea: imx: support disabling runtime-pm

Hi Xu,

On Thu, 20 Jul 2023 10:13:57 +0000
Xu Yang <[email protected]> wrote:

> Hi Luca,
>
> > > > -----Original Message-----
> > > >
> > > > Hello Xu,
> > > >
> > > > On Tue, 18 Jul 2023 08:31:48 +0000
> > > > Xu Yang <[email protected]> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > >
> > > > > > Ciao Francesco,
> > > > > >
> > > > > > On Thu, 6 Jul 2023 12:23:43 +0200
> > > > > > Francesco Dolcini <[email protected]> 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

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-07-21 02:43:46