[PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Philippe Schenker]

If the hardware is in low-power-mode and one plugs in device or host
it did not switch the mode due to the early exit out of the interrupt.

This patch fixes that behaviour and lets the rest of the code check if
something has to be done. If nothing has to be done the same return
value gets returned as before.

Fixes: 1f874edcb731 ("usb: chipidea: add runtime power management support")
Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

---

Hi Peter

During my investigation I found a bug in ci_irq. I would appreciate
if you could review this. From what I see this patch should be save to apply.

This patch does not solve all of our issues. When I have RUNTIME_PM
enabled on imx6ull or imx7. RNDIS still does not come up. So there
has to be another bug hiding somewhere in the Runtime PM code to prevent
RNDIS from working properly. I quickly looked through your patches
that added this stuff back in 2015 but couldn't spot anything related to
usb gadget-mode.

If you right away know where the problem could be hiding itself,
I would appreciate a hint.

Thanks,
Philippe

---

 drivers/usb/chipidea/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 9a7c53d09ab4..5159420a23a4 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -518,7 +518,7 @@ static irqreturn_t ci_irq(int irq, void *data)
 		disable_irq_nosync(irq);
 		ci->wakeup_int = true;
 		pm_runtime_get(ci->dev);
-		return IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
 
 	if (ci->is_otg) {
-- 
2.27.0

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Peter Chen]

On 20-06-26 13:03:11, Philippe Schenker wrote:
> If the hardware is in low-power-mode and one plugs in device or host
> it did not switch the mode due to the early exit out of the interrupt.

Do you mean there is no coming call for role-switch? Could you please share
your dts changes? Try below patch:


diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index e8ce300ad490..9e10dcfeb98f 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -1313,6 +1313,29 @@ static void ci_controller_suspend(struct ci_hdrc *ci)
 	enable_irq(ci->irq);
 }
 
+/*
+ * Handle the wakeup interrupt triggered by extcon connector
+ * We need to call ci_irq again for extcon since the first
+ * interrupt (wakeup int) only let the controller be out of
+ * low power mode, but not handle any interrupts.
+ */
+static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
+{
+	struct ci_hdrc_cable *cable_id, *cable_vbus;
+	u32 otgsc = hw_read_otgsc(ci, ~0);
+
+	cable_id = &ci->platdata->id_extcon;
+	cable_vbus = &ci->platdata->vbus_extcon;
+
+	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
+		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
+		ci_irq(ci->irq, ci);
+
+	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
+		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
+		ci_irq(ci->irq, ci);
+}
+
 static int ci_controller_resume(struct device *dev)
 {
 	struct ci_hdrc *ci = dev_get_drvdata(dev);
@@ -1343,6 +1366,7 @@ static int ci_controller_resume(struct device *dev)
 		enable_irq(ci->irq);
 		if (ci_otg_is_fsm_mode(ci))
 			ci_otg_fsm_wakeup_by_srp(ci);
+		ci_extcon_wakeup_int(ci);
 	}
 
 	return 0;

> 
> This patch fixes that behaviour and lets the rest of the code check if
> something has to be done. If nothing has to be done the same return
> value gets returned as before.

Before the controller leaving low power mode, otgsc value may not
correct since the controller/PHY is still in low power mode.

> 
> Fixes: 1f874edcb731 ("usb: chipidea: add runtime power management support")
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> 
> ---
> 
> Hi Peter
> 
> During my investigation I found a bug in ci_irq. I would appreciate
> if you could review this. From what I see this patch should be save to apply.
> 
> This patch does not solve all of our issues. When I have RUNTIME_PM
> enabled on imx6ull or imx7. RNDIS still does not come up. So there
> has to be another bug hiding somewhere in the Runtime PM code to prevent
> RNDIS from working properly. I quickly looked through your patches
> that added this stuff back in 2015 but couldn't spot anything related to
> usb gadget-mode.
> 
> If you right away know where the problem could be hiding itself,
> I would appreciate a hint.
> 

I need information for call trace for further suggestion.

Peter

> 
> ---
> 
>  drivers/usb/chipidea/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 9a7c53d09ab4..5159420a23a4 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -518,7 +518,7 @@ static irqreturn_t ci_irq(int irq, void *data)
>  		disable_irq_nosync(irq);
>  		ci->wakeup_int = true;
>  		pm_runtime_get(ci->dev);
> -		return IRQ_HANDLED;
> +		ret = IRQ_HANDLED;
>  	}
>  
>  	if (ci->is_otg) {
> -- 
> 2.27.0
> 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Philippe Schenker]

On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> On 20-06-26 13:03:11, Philippe Schenker wrote:
> > If the hardware is in low-power-mode and one plugs in device or host
> > it did not switch the mode due to the early exit out of the
> > interrupt.
> 
> Do you mean there is no coming call for role-switch? Could you please
> share
> your dts changes? Try below patch:

Here are my DTS changes:

diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
index 97601375f2640..c424f707a1afa 100644
--- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
+++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
@@ -13,6 +13,13 @@
                stdout-path = "serial0:115200n8";
        };
 
+       extcon_usbc_det: usbc_det {
+               compatible = "linux,extcon-usb-gpio";
+               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
+               pinctrl-names = "default";
+               pinctrl-0 = <&pinctrl_usbc_det>;
+       };
+
        /* fixed crystal dedicated to mpc258x */
        clk16m: clk16m {
                compatible = "fixed-clock";
@@ -174,6 +181,7 @@
 };
 
 &usbotg1 {
+       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
        status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
b/arch/arm/boot/dts/imx7-colibri.dtsi
index e18e89dec8792..caea90d2421fd 100644
--- a/arch/arm/boot/dts/imx7-colibri.dtsi
+++ b/arch/arm/boot/dts/imx7-colibri.dtsi
@@ -457,7 +457,7 @@
 };
 
 &usbotg1 {
-       dr_mode = "host";
+       dr_mode = "otg";
 };
 
 &usdhc1 {
@@ -486,7 +486,7 @@
 &iomuxc {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_gpio1 &pinctrl_gpio2 &pinctrl_gpio3
&pinctrl_gpio4
-                    &pinctrl_gpio7 &pinctrl_usbc_det>;
+                    &pinctrl_gpio7>;
 
        pinctrl_gpio1: gpio1-grp {
                fsl,pins = <

> 
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index e8ce300ad490..9e10dcfeb98f 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1313,6 +1313,29 @@ static void ci_controller_suspend(struct
> ci_hdrc *ci)
>  	enable_irq(ci->irq);
>  }
>  
> +/*
> + * Handle the wakeup interrupt triggered by extcon connector
> + * We need to call ci_irq again for extcon since the first
> + * interrupt (wakeup int) only let the controller be out of
> + * low power mode, but not handle any interrupts.
> + */
> +static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
> +{
> +	struct ci_hdrc_cable *cable_id, *cable_vbus;
> +	u32 otgsc = hw_read_otgsc(ci, ~0);
> +
> +	cable_id = &ci->platdata->id_extcon;
> +	cable_vbus = &ci->platdata->vbus_extcon;
> +
> +	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
> +		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
> +		ci_irq(ci->irq, ci);
> +
> +	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
> +		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
> +		ci_irq(ci->irq, ci);
> +}
> +
>  static int ci_controller_resume(struct device *dev)
>  {
>  	struct ci_hdrc *ci = dev_get_drvdata(dev);
> @@ -1343,6 +1366,7 @@ static int ci_controller_resume(struct device
> *dev)
>  		enable_irq(ci->irq);
>  		if (ci_otg_is_fsm_mode(ci))
>  			ci_otg_fsm_wakeup_by_srp(ci);
> +		ci_extcon_wakeup_int(ci);
>  	}
>  
>  	return 0;

Thanks for creating this patch! I wanted also something like that, but
couldn't find out where to put it. 

I tried this patch and with the above devicetree changes this works
perfectly! Thanks Peter!

Should I send the patch with you as the "Suggested-by" or do you want to
send the patch so you are the Author? I would have it ready in my git
repo.

> 
> > This patch fixes that behaviour and lets the rest of the code check
> > if
> > something has to be done. If nothing has to be done the same return
> > value gets returned as before.
> 
> Before the controller leaving low power mode, otgsc value may not
> correct since the controller/PHY is still in low power mode.
> 
> > Fixes: 1f874edcb731 ("usb: chipidea: add runtime power management
> > support")
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > ---
> > 
> > Hi Peter
> > 
> > During my investigation I found a bug in ci_irq. I would appreciate
> > if you could review this. From what I see this patch should be save
> > to apply.
> > 
> > This patch does not solve all of our issues. When I have RUNTIME_PM
> > enabled on imx6ull or imx7. RNDIS still does not come up. So there
> > has to be another bug hiding somewhere in the Runtime PM code to
> > prevent
> > RNDIS from working properly. I quickly looked through your patches
> > that added this stuff back in 2015 but couldn't spot anything
> > related to
> > usb gadget-mode.
> > 
> > If you right away know where the problem could be hiding itself,
> > I would appreciate a hint.
> > 
> 
> I need information for call trace for further suggestion.
> 
> Peter
> 
> > ---
> > 
> >  drivers/usb/chipidea/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c
> > b/drivers/usb/chipidea/core.c
> > index 9a7c53d09ab4..5159420a23a4 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -518,7 +518,7 @@ static irqreturn_t ci_irq(int irq, void *data)
> >  		disable_irq_nosync(irq);
> >  		ci->wakeup_int = true;
> >  		pm_runtime_get(ci->dev);
> > -		return IRQ_HANDLED;
> > +		ret = IRQ_HANDLED;
> >  	}
> >  
> >  	if (ci->is_otg) {
> > -- 
> > 2.27.0
> > 

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Peter Chen]

On 20-06-29 10:04:13, Philippe Schenker wrote:
> On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > If the hardware is in low-power-mode and one plugs in device or host
> > > it did not switch the mode due to the early exit out of the
> > > interrupt.
> > 
> > Do you mean there is no coming call for role-switch? Could you please
> > share
> > your dts changes? Try below patch:
> 
> Here are my DTS changes:
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> index 97601375f2640..c424f707a1afa 100644
> --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> @@ -13,6 +13,13 @@
>                 stdout-path = "serial0:115200n8";
>         };
>  
> +       extcon_usbc_det: usbc_det {
> +               compatible = "linux,extcon-usb-gpio";
> +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pinctrl_usbc_det>;
> +       };
> +
>         /* fixed crystal dedicated to mpc258x */
>         clk16m: clk16m {
>                 compatible = "fixed-clock";
> @@ -174,6 +181,7 @@
>  };
>  
>  &usbotg1 {
> +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;

If you have only ID extcon, but no VBUS extcon, you only need to
add only phandle, see dt-binding for detail please.

>         status = "okay";
>  };
>  
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index e18e89dec8792..caea90d2421fd 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -457,7 +457,7 @@
>  };
>  
>  &usbotg1 {
> -       dr_mode = "host";
> +       dr_mode = "otg";
>  };
>  
>  &usdhc1 {
> @@ -486,7 +486,7 @@
>  &iomuxc {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_gpio1 &pinctrl_gpio2 &pinctrl_gpio3
> &pinctrl_gpio4
> -                    &pinctrl_gpio7 &pinctrl_usbc_det>;
> +                    &pinctrl_gpio7>;
>  
>         pinctrl_gpio1: gpio1-grp {
>                 fsl,pins = <
> 
> > 
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index e8ce300ad490..9e10dcfeb98f 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -1313,6 +1313,29 @@ static void ci_controller_suspend(struct
> > ci_hdrc *ci)
> >  	enable_irq(ci->irq);
> >  }
> >  
> > +/*
> > + * Handle the wakeup interrupt triggered by extcon connector
> > + * We need to call ci_irq again for extcon since the first
> > + * interrupt (wakeup int) only let the controller be out of
> > + * low power mode, but not handle any interrupts.
> > + */
> > +static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
> > +{
> > +	struct ci_hdrc_cable *cable_id, *cable_vbus;
> > +	u32 otgsc = hw_read_otgsc(ci, ~0);
> > +
> > +	cable_id = &ci->platdata->id_extcon;
> > +	cable_vbus = &ci->platdata->vbus_extcon;
> > +
> > +	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
> > +		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
> > +		ci_irq(ci->irq, ci);
> > +
> > +	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
> > +		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
> > +		ci_irq(ci->irq, ci);
> > +}
> > +
> >  static int ci_controller_resume(struct device *dev)
> >  {
> >  	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > @@ -1343,6 +1366,7 @@ static int ci_controller_resume(struct device
> > *dev)
> >  		enable_irq(ci->irq);
> >  		if (ci_otg_is_fsm_mode(ci))
> >  			ci_otg_fsm_wakeup_by_srp(ci);
> > +		ci_extcon_wakeup_int(ci);
> >  	}
> >  
> >  	return 0;
> 
> Thanks for creating this patch! I wanted also something like that, but
> couldn't find out where to put it. 
> 
> I tried this patch and with the above devicetree changes this works
> perfectly! Thanks Peter!
> 
> Should I send the patch with you as the "Suggested-by" or do you want to
> send the patch so you are the Author? I would have it ready in my git
> repo.

This patch was a part of one patch at downstream, I will submit one and
add your tags.

> > > 
> > > Hi Peter
> > > 
> > > During my investigation I found a bug in ci_irq. I would appreciate
> > > if you could review this. From what I see this patch should be save
> > > to apply.
> > > 
> > > This patch does not solve all of our issues. When I have RUNTIME_PM
> > > enabled on imx6ull or imx7. RNDIS still does not come up. So there
> > > has to be another bug hiding somewhere in the Runtime PM code to
> > > prevent
> > > RNDIS from working properly. I quickly looked through your patches
> > > that added this stuff back in 2015 but couldn't spot anything
> > > related to
> > > usb gadget-mode.
> > > 
> > > If you right away know where the problem could be hiding itself,
> > > I would appreciate a hint.
> > > 
> > 
> > I need information for call trace for further suggestion.

With this patch, does your RNDIS issue be fixed or still not?

Peter
> > 
> > Peter
> > 
> > > ---
> > > 
> > >  drivers/usb/chipidea/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/chipidea/core.c
> > > b/drivers/usb/chipidea/core.c
> > > index 9a7c53d09ab4..5159420a23a4 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -518,7 +518,7 @@ static irqreturn_t ci_irq(int irq, void *data)
> > >  		disable_irq_nosync(irq);
> > >  		ci->wakeup_int = true;
> > >  		pm_runtime_get(ci->dev);
> > > -		return IRQ_HANDLED;
> > > +		ret = IRQ_HANDLED;
> > >  	}
> > >  
> > >  	if (ci->is_otg) {
> > > -- 
> > > 2.27.0
> > > 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Philippe Schenker]

On Tue, 2020-06-30 at 00:43 +0000, Peter Chen wrote:
> On 20-06-29 10:04:13, Philippe Schenker wrote:
> > On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > > If the hardware is in low-power-mode and one plugs in device or
> > > > host
> > > > it did not switch the mode due to the early exit out of the
> > > > interrupt.
> > > 
> > > Do you mean there is no coming call for role-switch? Could you
> > > please
> > > share
> > > your dts changes? Try below patch:
> > 
> > Here are my DTS changes:
> > 
> > diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > index 97601375f2640..c424f707a1afa 100644
> > --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > @@ -13,6 +13,13 @@
> >                 stdout-path = "serial0:115200n8";
> >         };
> >  
> > +       extcon_usbc_det: usbc_det {
> > +               compatible = "linux,extcon-usb-gpio";
> > +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&pinctrl_usbc_det>;
> > +       };
> > +
> >         /* fixed crystal dedicated to mpc258x */
> >         clk16m: clk16m {
> >                 compatible = "fixed-clock";
> > @@ -174,6 +181,7 @@
> >  };
> >  
> >  &usbotg1 {
> > +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
> 
> If you have only ID extcon, but no VBUS extcon, you only need to
> add only phandle, see dt-binding for detail please.

You where right again! Thanks, this actually solves the RNDIS issue for
our colibri-imx7 board:

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

Howevever on this iMX7 board we have VBUS hooked up to the SoC, that's
why it works only with ID.

On Colibri-iMX6ULL we do not have VBUS hooked up. So device/host
switching works only with 'extcon = <&extcon_usbc_det>,
<&extcon_usbc_det>;' but then RNDIS and also a normal thumb-drive does
not work. How could I work around this fact? A dummy-gpio that would
always read "high" for vbus would be a solution for me.

Philippe

> 
> >         status = "okay";
> >  };
> >  
> > diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> > b/arch/arm/boot/dts/imx7-colibri.dtsi
> > index e18e89dec8792..caea90d2421fd 100644
> > --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> > +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> > @@ -457,7 +457,7 @@
> >  };
> >  
> >  &usbotg1 {
> > -       dr_mode = "host";
> > +       dr_mode = "otg";
> >  };
> >  
> >  &usdhc1 {
> > @@ -486,7 +486,7 @@
> >  &iomuxc {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_gpio1 &pinctrl_gpio2 &pinctrl_gpio3
> > &pinctrl_gpio4
> > -                    &pinctrl_gpio7 &pinctrl_usbc_det>;
> > +                    &pinctrl_gpio7>;
> >  
> >         pinctrl_gpio1: gpio1-grp {
> >                 fsl,pins = <
> > 
> > > 
> > > diff --git a/drivers/usb/chipidea/core.c
> > > b/drivers/usb/chipidea/core.c
> > > index e8ce300ad490..9e10dcfeb98f 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -1313,6 +1313,29 @@ static void ci_controller_suspend(struct
> > > ci_hdrc *ci)
> > >  	enable_irq(ci->irq);
> > >  }
> > >  
> > > +/*
> > > + * Handle the wakeup interrupt triggered by extcon connector
> > > + * We need to call ci_irq again for extcon since the first
> > > + * interrupt (wakeup int) only let the controller be out of
> > > + * low power mode, but not handle any interrupts.
> > > + */
> > > +static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
> > > +{
> > > +	struct ci_hdrc_cable *cable_id, *cable_vbus;
> > > +	u32 otgsc = hw_read_otgsc(ci, ~0);
> > > +
> > > +	cable_id = &ci->platdata->id_extcon;
> > > +	cable_vbus = &ci->platdata->vbus_extcon;
> > > +
> > > +	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
> > > +		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
> > > +		ci_irq(ci->irq, ci);
> > > +
> > > +	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
> > > +		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
> > > +		ci_irq(ci->irq, ci);
> > > +}
> > > +
> > >  static int ci_controller_resume(struct device *dev)
> > >  {
> > >  	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > @@ -1343,6 +1366,7 @@ static int ci_controller_resume(struct
> > > device
> > > *dev)
> > >  		enable_irq(ci->irq);
> > >  		if (ci_otg_is_fsm_mode(ci))
> > >  			ci_otg_fsm_wakeup_by_srp(ci);
> > > +		ci_extcon_wakeup_int(ci);
> > >  	}
> > >  
> > >  	return 0;
> > 
> > Thanks for creating this patch! I wanted also something like that,
> > but
> > couldn't find out where to put it. 
> > 
> > I tried this patch and with the above devicetree changes this works
> > perfectly! Thanks Peter!
> > 
> > Should I send the patch with you as the "Suggested-by" or do you
> > want to
> > send the patch so you are the Author? I would have it ready in my
> > git
> > repo.
> 
> This patch was a part of one patch at downstream, I will submit one
> and
> add your tags.

Perfect, thank you!
> 
> > > > Hi Peter
> > > > 
> > > > During my investigation I found a bug in ci_irq. I would
> > > > appreciate
> > > > if you could review this. From what I see this patch should be
> > > > save
> > > > to apply.
> > > > 
> > > > This patch does not solve all of our issues. When I have
> > > > RUNTIME_PM
> > > > enabled on imx6ull or imx7. RNDIS still does not come up. So
> > > > there
> > > > has to be another bug hiding somewhere in the Runtime PM code to
> > > > prevent
> > > > RNDIS from working properly. I quickly looked through your
> > > > patches
> > > > that added this stuff back in 2015 but couldn't spot anything
> > > > related to
> > > > usb gadget-mode.
> > > > 
> > > > If you right away know where the problem could be hiding itself,
> > > > I would appreciate a hint.
> > > > 
> > > 
> > > I need information for call trace for further suggestion.
> 
> With this patch, does your RNDIS issue be fixed or still not?
> 
> Peter
> > > Peter
> > > 
> > > > ---
> > > > 
> > > >  drivers/usb/chipidea/core.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/chipidea/core.c
> > > > b/drivers/usb/chipidea/core.c
> > > > index 9a7c53d09ab4..5159420a23a4 100644
> > > > --- a/drivers/usb/chipidea/core.c
> > > > +++ b/drivers/usb/chipidea/core.c
> > > > @@ -518,7 +518,7 @@ static irqreturn_t ci_irq(int irq, void
> > > > *data)
> > > >  		disable_irq_nosync(irq);
> > > >  		ci->wakeup_int = true;
> > > >  		pm_runtime_get(ci->dev);
> > > > -		return IRQ_HANDLED;
> > > > +		ret = IRQ_HANDLED;
> > > >  	}
> > > >  
> > > >  	if (ci->is_otg) {
> > > > -- 
> > > > 2.27.0
> > > > 

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Peter Chen]

On 20-06-30 11:59:49, Philippe Schenker wrote:
> On Tue, 2020-06-30 at 00:43 +0000, Peter Chen wrote:
> > On 20-06-29 10:04:13, Philippe Schenker wrote:
> > > On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > > > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > > > If the hardware is in low-power-mode and one plugs in device or
> > > > > host
> > > > > it did not switch the mode due to the early exit out of the
> > > > > interrupt.
> > > > 
> > > > Do you mean there is no coming call for role-switch? Could you
> > > > please
> > > > share
> > > > your dts changes? Try below patch:
> > > 
> > > Here are my DTS changes:
> > > 
> > > diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > index 97601375f2640..c424f707a1afa 100644
> > > --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > @@ -13,6 +13,13 @@
> > >                 stdout-path = "serial0:115200n8";
> > >         };
> > >  
> > > +       extcon_usbc_det: usbc_det {
> > > +               compatible = "linux,extcon-usb-gpio";
> > > +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> > > +               pinctrl-names = "default";
> > > +               pinctrl-0 = <&pinctrl_usbc_det>;
> > > +       };
> > > +
> > >         /* fixed crystal dedicated to mpc258x */
> > >         clk16m: clk16m {
> > >                 compatible = "fixed-clock";
> > > @@ -174,6 +181,7 @@
> > >  };
> > >  
> > >  &usbotg1 {
> > > +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
> > 
> > If you have only ID extcon, but no VBUS extcon, you only need to
> > add only phandle, see dt-binding for detail please.
> 
> You where right again! Thanks, this actually solves the RNDIS issue for
> our colibri-imx7 board:
> 
> +       extcon = <0>, <&extcon_usbc_det>;
> 
> Howevever on this iMX7 board we have VBUS hooked up to the SoC, that's
> why it works only with ID.
> 
> On Colibri-iMX6ULL we do not have VBUS hooked up.

So, there is no any events for connecting to Host? If it is, the
workaround for this board is disable runtime pm. And you only need to
write one extcon phandle for ID since you have external event for ID,
but no event for VBUS. ID event is not the same with VBUS, for example,
if you plug cable into host, you will not get the ID event, you could
only get VBUS event if there is an event (eg, through GPIO) for it.

Peter

> So device/host
> switching works only with 'extcon = <&extcon_usbc_det>,
> <&extcon_usbc_det>;' but then RNDIS and also a normal thumb-drive does
> not work. How could I work around this fact? A dummy-gpio that would
> always read "high" for vbus would be a solution for me.
> 
> Philippe
> 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Philippe Schenker]

On Wed, 2020-07-01 at 02:52 +0000, Peter Chen wrote:
> On 20-06-30 11:59:49, Philippe Schenker wrote:
> > On Tue, 2020-06-30 at 00:43 +0000, Peter Chen wrote:
> > > On 20-06-29 10:04:13, Philippe Schenker wrote:
> > > > On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > > > > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > > > > If the hardware is in low-power-mode and one plugs in device
> > > > > > or
> > > > > > host
> > > > > > it did not switch the mode due to the early exit out of the
> > > > > > interrupt.
> > > > > 
> > > > > Do you mean there is no coming call for role-switch? Could you
> > > > > please
> > > > > share
> > > > > your dts changes? Try below patch:
> > > > 
> > > > Here are my DTS changes:
> > > > 
> > > > diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > index 97601375f2640..c424f707a1afa 100644
> > > > --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > @@ -13,6 +13,13 @@
> > > >                 stdout-path = "serial0:115200n8";
> > > >         };
> > > >  
> > > > +       extcon_usbc_det: usbc_det {
> > > > +               compatible = "linux,extcon-usb-gpio";
> > > > +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> > > > +               pinctrl-names = "default";
> > > > +               pinctrl-0 = <&pinctrl_usbc_det>;
> > > > +       };
> > > > +
> > > >         /* fixed crystal dedicated to mpc258x */
> > > >         clk16m: clk16m {
> > > >                 compatible = "fixed-clock";
> > > > @@ -174,6 +181,7 @@
> > > >  };
> > > >  
> > > >  &usbotg1 {
> > > > +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
> > > 
> > > If you have only ID extcon, but no VBUS extcon, you only need to
> > > add only phandle, see dt-binding for detail please.
> > 
> > You where right again! Thanks, this actually solves the RNDIS issue
> > for
> > our colibri-imx7 board:
> > 
> > +       extcon = <0>, <&extcon_usbc_det>;
> > 
> > Howevever on this iMX7 board we have VBUS hooked up to the SoC,
> > that's
> > why it works only with ID.
> > 
> > On Colibri-iMX6ULL we do not have VBUS hooked up.
> 
> So, there is no any events for connecting to Host? If it is, the
> workaround for this board is disable runtime pm. And you only need to
> write one extcon phandle for ID since you have external event for ID,
> but no event for VBUS. ID event is not the same with VBUS, for
> example,
> if you plug cable into host, you will not get the ID event, you could
> only get VBUS event if there is an event (eg, through GPIO) for it.
> 
> Peter

No we don't have extra Host events. We have one GPIO, if it is high USB
should be in gadget mode and if the GPIO is low USB should be in host
mode.

Is there no way we can achieve this on mainline without disabling
runtime PM?

Philippe

> 
> > So device/host
> > switching works only with 'extcon = <&extcon_usbc_det>,
> > <&extcon_usbc_det>;' but then RNDIS and also a normal thumb-drive
> > does
> > not work. How could I work around this fact? A dummy-gpio that would
> > always read "high" for vbus would be a solution for me.
> > 
> > Philippe
> > 

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Peter Chen]

On 20-07-01 08:32:22, Philippe Schenker wrote:
> On Wed, 2020-07-01 at 02:52 +0000, Peter Chen wrote:
> > On 20-06-30 11:59:49, Philippe Schenker wrote:
> > > On Tue, 2020-06-30 at 00:43 +0000, Peter Chen wrote:
> > > > On 20-06-29 10:04:13, Philippe Schenker wrote:
> > > > > On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > > > > > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > > > > > If the hardware is in low-power-mode and one plugs in device
> > > > > > > or
> > > > > > > host
> > > > > > > it did not switch the mode due to the early exit out of the
> > > > > > > interrupt.
> > > > > > 
> > > > > > Do you mean there is no coming call for role-switch? Could you
> > > > > > please
> > > > > > share
> > > > > > your dts changes? Try below patch:
> > > > > 
> > > > > Here are my DTS changes:
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > index 97601375f2640..c424f707a1afa 100644
> > > > > --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > @@ -13,6 +13,13 @@
> > > > >                 stdout-path = "serial0:115200n8";
> > > > >         };
> > > > >  
> > > > > +       extcon_usbc_det: usbc_det {
> > > > > +               compatible = "linux,extcon-usb-gpio";
> > > > > +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> > > > > +               pinctrl-names = "default";
> > > > > +               pinctrl-0 = <&pinctrl_usbc_det>;
> > > > > +       };
> > > > > +
> > > > >         /* fixed crystal dedicated to mpc258x */
> > > > >         clk16m: clk16m {
> > > > >                 compatible = "fixed-clock";
> > > > > @@ -174,6 +181,7 @@
> > > > >  };
> > > > >  
> > > > >  &usbotg1 {
> > > > > +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
> > > > 
> > > > If you have only ID extcon, but no VBUS extcon, you only need to
> > > > add only phandle, see dt-binding for detail please.
> > > 
> > > You where right again! Thanks, this actually solves the RNDIS issue
> > > for
> > > our colibri-imx7 board:
> > > 
> > > +       extcon = <0>, <&extcon_usbc_det>;
> > > 
> > > Howevever on this iMX7 board we have VBUS hooked up to the SoC,
> > > that's
> > > why it works only with ID.
> > > 
> > > On Colibri-iMX6ULL we do not have VBUS hooked up.
> > 
> > So, there is no any events for connecting to Host? If it is, the
> > workaround for this board is disable runtime pm. And you only need to
> > write one extcon phandle for ID since you have external event for ID,
> > but no event for VBUS. ID event is not the same with VBUS, for
> > example,
> > if you plug cable into host, you will not get the ID event, you could
> > only get VBUS event if there is an event (eg, through GPIO) for it.
> > 
> > Peter
> 
> No we don't have extra Host events. We have one GPIO, if it is high USB
> should be in gadget mode and if the GPIO is low USB should be in host
> mode.
> 
> Is there no way we can achieve this on mainline without disabling
> runtime PM?
> 

At least I think so, since you don't have any events to let the
SW know you connects to host if the controller is in low power mode.

Peter

> Philippe
> 
> > 
> > > So device/host
> > > switching works only with 'extcon = <&extcon_usbc_det>,
> > > <&extcon_usbc_det>;' but then RNDIS and also a normal thumb-drive
> > > does
> > > not work. How could I work around this fact? A dummy-gpio that would
> > > always read "high" for vbus would be a solution for me.
> > > 
> > > Philippe
> > > 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: fix ci_irq for role-switching use-case

[Philippe Schenker]

On Wed, 2020-07-01 at 09:02 +0000, Peter Chen wrote:
> On 20-07-01 08:32:22, Philippe Schenker wrote:
> > On Wed, 2020-07-01 at 02:52 +0000, Peter Chen wrote:
> > > On 20-06-30 11:59:49, Philippe Schenker wrote:
> > > > On Tue, 2020-06-30 at 00:43 +0000, Peter Chen wrote:
> > > > > On 20-06-29 10:04:13, Philippe Schenker wrote:
> > > > > > On Mon, 2020-06-29 at 07:26 +0000, Peter Chen wrote:
> > > > > > > On 20-06-26 13:03:11, Philippe Schenker wrote:
> > > > > > > > If the hardware is in low-power-mode and one plugs in
> > > > > > > > device
> > > > > > > > or
> > > > > > > > host
> > > > > > > > it did not switch the mode due to the early exit out of
> > > > > > > > the
> > > > > > > > interrupt.
> > > > > > > 
> > > > > > > Do you mean there is no coming call for role-switch? Could
> > > > > > > you
> > > > > > > please
> > > > > > > share
> > > > > > > your dts changes? Try below patch:
> > > > > > 
> > > > > > Here are my DTS changes:
> > > > > > 
> > > > > > diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > > b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > > index 97601375f2640..c424f707a1afa 100644
> > > > > > --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > > +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> > > > > > @@ -13,6 +13,13 @@
> > > > > >                 stdout-path = "serial0:115200n8";
> > > > > >         };
> > > > > >  
> > > > > > +       extcon_usbc_det: usbc_det {
> > > > > > +               compatible = "linux,extcon-usb-gpio";
> > > > > > +               id-gpio = <&gpio7 14 GPIO_ACTIVE_HIGH>;
> > > > > > +               pinctrl-names = "default";
> > > > > > +               pinctrl-0 = <&pinctrl_usbc_det>;
> > > > > > +       };
> > > > > > +
> > > > > >         /* fixed crystal dedicated to mpc258x */
> > > > > >         clk16m: clk16m {
> > > > > >                 compatible = "fixed-clock";
> > > > > > @@ -174,6 +181,7 @@
> > > > > >  };
> > > > > >  
> > > > > >  &usbotg1 {
> > > > > > +       extcon = <&extcon_usbc_det>, <&extcon_usbc_det>;
> > > > > 
> > > > > If you have only ID extcon, but no VBUS extcon, you only need
> > > > > to
> > > > > add only phandle, see dt-binding for detail please.
> > > > 
> > > > You where right again! Thanks, this actually solves the RNDIS
> > > > issue
> > > > for
> > > > our colibri-imx7 board:
> > > > 
> > > > +       extcon = <0>, <&extcon_usbc_det>;
> > > > 
> > > > Howevever on this iMX7 board we have VBUS hooked up to the SoC,
> > > > that's
> > > > why it works only with ID.
> > > > 
> > > > On Colibri-iMX6ULL we do not have VBUS hooked up.
> > > 
> > > So, there is no any events for connecting to Host? If it is, the
> > > workaround for this board is disable runtime pm. And you only need
> > > to
> > > write one extcon phandle for ID since you have external event for
> > > ID,
> > > but no event for VBUS. ID event is not the same with VBUS, for
> > > example,
> > > if you plug cable into host, you will not get the ID event, you
> > > could
> > > only get VBUS event if there is an event (eg, through GPIO) for
> > > it.
> > > 
> > > Peter
> > 
> > No we don't have extra Host events. We have one GPIO, if it is high
> > USB
> > should be in gadget mode and if the GPIO is low USB should be in
> > host
> > mode.
> > 
> > Is there no way we can achieve this on mainline without disabling
> > runtime PM?
> > 
> 
> At least I think so, since you don't have any events to let the
> SW know you connects to host if the controller is in low power mode.
> 
> Peter

okay, good to know. Thank you very much for your help! I will try now
with a dummy-gpio I guess that could be a nice workaround for the ULL.

Best Regards,
Philippe

> 
> > Philippe
> > 
> > > > So device/host
> > > > switching works only with 'extcon = <&extcon_usbc_det>,
> > > > <&extcon_usbc_det>;' but then RNDIS and also a normal thumb-
> > > > drive
> > > > does
> > > > not work. How could I work around this fact? A dummy-gpio that
> > > > would
> > > > always read "high" for vbus would be a solution for me.
> > > > 
> > > > Philippe
> > > >