All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case
@ 2019-10-21 16:16 Igor Opaniuk
  2019-10-21 16:16   ` Igor Opaniuk
  2019-10-22  2:11 ` [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Peter Chen
  0 siblings, 2 replies; 8+ messages in thread
From: Igor Opaniuk @ 2019-10-21 16:16 UTC (permalink / raw)
  To: linux-usb
  Cc: Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Sanchayan Maity,
	Igor Opaniuk, Greg Kroah-Hartman, Peter Chen, linux-kernel

From: Stefan Agner <stefan.agner@toradex.com>

The existing usage of extcon in chipidea driver freezes the kernel
presumably due to OTGSC register access.

Prevent accessing any OTG registers for SoC with dual role devices
but no true OTG support. Use the flag CI_HDRC_DUAL_ROLE_NOT_OTG for
those devices and in case extcon is present, do the role switch
using extcon only.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
---

 drivers/usb/chipidea/ci.h   |  2 +
 drivers/usb/chipidea/core.c | 87 +++++++++++++++++++++++++++++++++----
 2 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index cf9cc9402826..3a1a549ed39e 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -170,6 +170,7 @@ struct hw_bank {
  * @enabled_otg_timer_bits: bits of enabled otg timers
  * @next_otg_timer: next nearest enabled timer to be expired
  * @work: work for role changing
+ * @work_dr: work for role changing for non-OTG controllers
  * @wq: workqueue thread
  * @qh_pool: allocation pool for queue heads
  * @td_pool: allocation pool for transfer descriptors
@@ -220,6 +221,7 @@ struct ci_hdrc {
 	enum otg_fsm_timer		next_otg_timer;
 	struct usb_role_switch		*role_switch;
 	struct work_struct		work;
+	struct work_struct		work_dr;
 	struct workqueue_struct		*wq;
 
 	struct dma_pool			*qh_pool;
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index dce5db41501c..48ecc846735c 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -534,6 +534,46 @@ int hw_device_reset(struct ci_hdrc *ci)
 	return 0;
 }
 
+static void usb_roleswitch_workqueue(struct work_struct *work)
+{
+	struct ci_hdrc *ci = container_of(work, struct ci_hdrc, work_dr);
+	struct ci_hdrc_cable *id, *vbus;
+	int ret;
+
+	pm_runtime_get_sync(ci->dev);
+
+	id = &ci->platdata->id_extcon;
+	if (!IS_ERR(id->edev)) {
+		int new_role;
+
+		ci_role_stop(ci);
+		hw_wait_phy_stable();
+
+		ret = extcon_get_state(id->edev, EXTCON_USB_HOST);
+		if (ret) {
+			new_role = CI_ROLE_HOST;
+			dev_info(ci->dev, "switching to host role\n");
+		} else {
+			new_role = CI_ROLE_GADGET;
+			dev_info(ci->dev, "switching to gadget role\n");
+		}
+		ci_role_start(ci, new_role);
+	}
+
+	vbus = &ci->platdata->vbus_extcon;
+	if (!IS_ERR(vbus->edev)) {
+		ret = extcon_get_state(vbus->edev, EXTCON_USB);
+		if (ret)
+			usb_gadget_vbus_connect(&ci->gadget);
+		else
+			usb_gadget_vbus_disconnect(&ci->gadget);
+	}
+
+	pm_runtime_put_sync(ci->dev);
+
+	enable_irq(ci->irq);
+}
+
 static irqreturn_t ci_irq(int irq, void *data)
 {
 	struct ci_hdrc *ci = data;
@@ -593,10 +633,24 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
 	struct ci_hdrc_cable *cbl = container_of(nb, struct ci_hdrc_cable, nb);
 	struct ci_hdrc *ci = cbl->ci;
 
-	cbl->connected = event;
-	cbl->changed = true;
+	if (ci->platdata->flags & CI_HDRC_DUAL_ROLE_NOT_OTG) {
+		disable_irq_nosync(ci->irq);
+
+		/*
+		 * This notifier might get called twice in succession,
+		 * once for the ID pin and once for the VBUS pin. Make
+		 * sure we only disable irq in case we successfully add
+		 * work to the work queue.
+		 */
+		if (!queue_work(system_power_efficient_wq, &ci->work_dr))
+			enable_irq(ci->irq);
+	} else {
+		cbl->connected = event;
+		cbl->changed = true;
+
+		ci_irq(ci->irq, ci);
+	}
 
-	ci_irq(ci->irq, ci);
 	return NOTIFY_DONE;
 }
 
@@ -765,6 +819,7 @@ static int ci_get_platdata(struct device *dev,
 		ext_id = extcon_get_edev_by_phandle(dev, 1);
 		if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
 			return PTR_ERR(ext_id);
+		platdata->flags |= CI_HDRC_DUAL_ROLE_NOT_OTG;
 	}
 
 	cable = &platdata->vbus_extcon;
@@ -1079,7 +1134,15 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	ci_get_otg_capable(ci);
 
+	if (ci->platdata->flags & CI_HDRC_DUAL_ROLE_NOT_OTG)
+		INIT_WORK(&ci->work_dr, usb_roleswitch_workqueue);
+
+	ret = ci_extcon_register(ci);
+	if (ret)
+		goto stop;
+
 	dr_mode = ci->platdata->dr_mode;
+
 	/* initialize role(s) before the interrupt is requested */
 	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
 		ret = ci_hdrc_host_init(ci);
@@ -1145,8 +1208,18 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	if (!ci_otg_is_fsm_mode(ci)) {
 		/* only update vbus status for peripheral */
-		if (ci->role == CI_ROLE_GADGET)
-			ci_handle_vbus_change(ci);
+		if (dr_mode == USB_DR_MODE_PERIPHERAL) {
+			usb_gadget_vbus_connect(&ci->gadget);
+		} else if (ci->role == CI_ROLE_GADGET) {
+			struct ci_hdrc_cable *vbus = &ci->platdata->vbus_extcon;
+
+			/* Use vbus state from extcon if provided */
+			if (!IS_ERR(vbus->edev) &&
+			    extcon_get_state(vbus->edev, EXTCON_USB))
+				usb_gadget_vbus_connect(&ci->gadget);
+			else
+				ci_handle_vbus_change(ci);
+		}
 
 		ret = ci_role_start(ci, ci->role);
 		if (ret) {
@@ -1161,10 +1234,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	if (ret)
 		goto stop;
 
-	ret = ci_extcon_register(ci);
-	if (ret)
-		goto stop;
-
 	if (ci->supports_runtime_pm) {
 		pm_runtime_set_active(&pdev->dev);
 		pm_runtime_enable(&pdev->dev);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v1 2/2] ARM: dts: colibri-imx6ull: add extcon for usbotg1
  2019-10-21 16:16 [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Igor Opaniuk
@ 2019-10-21 16:16   ` Igor Opaniuk
  2019-10-22  2:11 ` [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Peter Chen
  1 sibling, 0 replies; 8+ messages in thread
From: Igor Opaniuk @ 2019-10-21 16:16 UTC (permalink / raw)
  To: linux-usb
  Cc: Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Igor Opaniuk, Fabio Estevam,
	Mark Rutland, NXP Linux Team, Pengutronix Kernel Team,
	Rob Herring, Sascha Hauer, Shawn Guo, devicetree,
	linux-arm-kernel, linux-kernel

From: Igor Opaniuk <igor.opaniuk@toradex.com>

Add extcon usb gpio configuration for support dual roles for usbotg1.

USB host/gadget switching test (1. USB NIC emulation; 2. USB storage):

[   52.491957] ci_hdrc ci_hdrc.1: switching to gadget role
[   52.502911] mxs_phy 20c9000.usbphy: vbus is not valid
[   56.749160] using random self ethernet address
[   56.758637] using random host ethernet address
[   65.768968] usb0: HOST MAC 00:14:2d:ff:ff:fe
[   65.887980] usb0: MAC 00:14:2d:ff:ff:ff
[   66.294961] configfs-gadget gadget: high-speed config #1: c
[   78.741971] ci_hdrc ci_hdrc.1: switching to host role
[   78.747522] ci_hdrc ci_hdrc.1: EHCI Host Controller
[   78.790174] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus
number 2
[   78.868498] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00

Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
---

 arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
index a78849fd2afa..988f1a800d5a 100644
--- a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
+++ b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
@@ -29,6 +29,14 @@
 		clock-frequency = <16000000>;
 	};
 
+	extcon_usbc_det: usbc_det {
+		compatible = "linux,extcon-usb-gpio";
+		debounce = <25>;
+		id-gpio = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_snvs_usbc_det>;
+	};
+
 	panel: panel {
 		compatible = "edt,et057090dhu";
 		backlight = <&bl>;
@@ -150,6 +158,7 @@
 };
 
 &usbotg1 {
+	extcon = <&extcon_usbc_det &extcon_usbc_det>;
 	status = "okay";
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v1 2/2] ARM: dts: colibri-imx6ull: add extcon for usbotg1
@ 2019-10-21 16:16   ` Igor Opaniuk
  0 siblings, 0 replies; 8+ messages in thread
From: Igor Opaniuk @ 2019-10-21 16:16 UTC (permalink / raw)
  To: linux-usb
  Cc: Mark Rutland, devicetree, Igor Opaniuk, Pengutronix Kernel Team,
	Stefan Agner, Marcel Ziswiler, Shawn Guo, Sascha Hauer,
	linux-kernel, Oleksandr Suvorov, Philippe Schenker, Rob Herring,
	NXP Linux Team, Max Krummenacher, Fabio Estevam,
	linux-arm-kernel

From: Igor Opaniuk <igor.opaniuk@toradex.com>

Add extcon usb gpio configuration for support dual roles for usbotg1.

USB host/gadget switching test (1. USB NIC emulation; 2. USB storage):

[   52.491957] ci_hdrc ci_hdrc.1: switching to gadget role
[   52.502911] mxs_phy 20c9000.usbphy: vbus is not valid
[   56.749160] using random self ethernet address
[   56.758637] using random host ethernet address
[   65.768968] usb0: HOST MAC 00:14:2d:ff:ff:fe
[   65.887980] usb0: MAC 00:14:2d:ff:ff:ff
[   66.294961] configfs-gadget gadget: high-speed config #1: c
[   78.741971] ci_hdrc ci_hdrc.1: switching to host role
[   78.747522] ci_hdrc ci_hdrc.1: EHCI Host Controller
[   78.790174] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus
number 2
[   78.868498] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00

Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
---

 arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
index a78849fd2afa..988f1a800d5a 100644
--- a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
+++ b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
@@ -29,6 +29,14 @@
 		clock-frequency = <16000000>;
 	};
 
+	extcon_usbc_det: usbc_det {
+		compatible = "linux,extcon-usb-gpio";
+		debounce = <25>;
+		id-gpio = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_snvs_usbc_det>;
+	};
+
 	panel: panel {
 		compatible = "edt,et057090dhu";
 		backlight = <&bl>;
@@ -150,6 +158,7 @@
 };
 
 &usbotg1 {
+	extcon = <&extcon_usbc_det &extcon_usbc_det>;
 	status = "okay";
 };
 
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case
  2019-10-21 16:16 [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Igor Opaniuk
  2019-10-21 16:16   ` Igor Opaniuk
@ 2019-10-22  2:11 ` Peter Chen
  2019-10-22 13:54   ` Igor Opaniuk
  1 sibling, 1 reply; 8+ messages in thread
From: Peter Chen @ 2019-10-22  2:11 UTC (permalink / raw)
  To: Igor Opaniuk
  Cc: linux-usb, Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Sanchayan Maity,
	Igor Opaniuk, Greg Kroah-Hartman, linux-kernel

On 19-10-21 19:16:53, Igor Opaniuk wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> The existing usage of extcon in chipidea driver freezes the kernel
> presumably due to OTGSC register access.
> 
> Prevent accessing any OTG registers for SoC with dual role devices
> but no true OTG support. Use the flag CI_HDRC_DUAL_ROLE_NOT_OTG for
> those devices and in case extcon is present, do the role switch
> using extcon only.

Hi Igor & Stefan,

I have several questions about the problem you met:
- Which vendor's controller you have used?
- What do you mean "no true OTG"? Does it mean no "OTGSC" register?

>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>  		ret = ci_hdrc_host_init(ci);
> @@ -1145,8 +1208,18 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  
>  	if (!ci_otg_is_fsm_mode(ci)) {
>  		/* only update vbus status for peripheral */
> -		if (ci->role == CI_ROLE_GADGET)
> -			ci_handle_vbus_change(ci);
> +		if (dr_mode == USB_DR_MODE_PERIPHERAL) {
> +			usb_gadget_vbus_connect(&ci->gadget);

We only use ci->role at runtime, since it has already considered the
dts setting, kernel configuration and hardware setting.

If your controller doesn't otgsc register, but do need to support
role switch, you may enhance the function ci_get_role

Peter

> +		} else if (ci->role == CI_ROLE_GADGET) {
> +			struct ci_hdrc_cable *vbus = &ci->platdata->vbus_extcon;
> +
> +			/* Use vbus state from extcon if provided */
> +			if (!IS_ERR(vbus->edev) &&
> +			    extcon_get_state(vbus->edev, EXTCON_USB))
> +				usb_gadget_vbus_connect(&ci->gadget);
> +			else
> +				ci_handle_vbus_change(ci);
> +		}
>  
>  		ret = ci_role_start(ci, ci->role);
>  		if (ret) {
> @@ -1161,10 +1234,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto stop;
>  
> -	ret = ci_extcon_register(ci);
> -	if (ret)
> -		goto stop;
> -
>  	if (ci->supports_runtime_pm) {
>  		pm_runtime_set_active(&pdev->dev);
>  		pm_runtime_enable(&pdev->dev);
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case
  2019-10-22  2:11 ` [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Peter Chen
@ 2019-10-22 13:54   ` Igor Opaniuk
  2019-10-23  8:28     ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Igor Opaniuk @ 2019-10-22 13:54 UTC (permalink / raw)
  To: Peter Chen
  Cc: linux-usb, Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Sanchayan Maity,
	Igor Opaniuk, Greg Kroah-Hartman, linux-kernel

Hi Peter,

On Tue, Oct 22, 2019 at 5:11 AM Peter Chen <peter.chen@nxp.com> wrote:
>
> On 19-10-21 19:16:53, Igor Opaniuk wrote:
> > From: Stefan Agner <stefan.agner@toradex.com>
> >
> > The existing usage of extcon in chipidea driver freezes the kernel
> > presumably due to OTGSC register access.
> >
> > Prevent accessing any OTG registers for SoC with dual role devices
> > but no true OTG support. Use the flag CI_HDRC_DUAL_ROLE_NOT_OTG for
> > those devices and in case extcon is present, do the role switch
> > using extcon only.
>
> Hi Igor & Stefan,
>
> I have several questions about the problem you met:
> - Which vendor's controller you have used?
> - What do you mean "no true OTG"? Does it mean no "OTGSC" register?

Probably the commit message adds a bit of confusion here
(I've kept the original one from the patch in our downstream kernel,
but will probably reword it).

The actual problem is that USB_OTG1_ID pin isn't wired, so we can't rely
on the value of ID pin state in OTGSC for the role detection.
In our SoM (Colibri iMX6ULL) ID pin from USB connector is wired
to SNVS_TAMPER2 which is pinmuxed as GPIO pin (GPIO5_02),
[1] (this is schematic for the Carrier Board, not SoM (isn't publicly
available),
but there is a pretty good explanation + schematic
in the section "2.3.2.2 USB 2.0 OTG Schematic Example ").

>
> >       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >               ret = ci_hdrc_host_init(ci);
> > @@ -1145,8 +1208,18 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> >
> >       if (!ci_otg_is_fsm_mode(ci)) {
> >               /* only update vbus status for peripheral */
> > -             if (ci->role == CI_ROLE_GADGET)
> > -                     ci_handle_vbus_change(ci);
> > +             if (dr_mode == USB_DR_MODE_PERIPHERAL) {
> > +                     usb_gadget_vbus_connect(&ci->gadget);
>
> We only use ci->role at runtime, since it has already considered the
> dts setting, kernel configuration and hardware setting.
>
> If your controller doesn't otgsc register, but do need to support
> role switch, you may enhance the function ci_get_role

Btw, ci_get_role() implementation still resides in the NXP dowstream kernel
and I've never seen anything posted to the ML (if it was, could you
please point me to
the patch?). I can introduce the new one, which wraps both OTGSC handling
+ extcon for CI_HDRC_DUAL_ROLE_NOT_OTG controllers.

Frankly speaking, I don't know the reason why additional workqueue (ci->work_dr)
was introduced (will try to reach Stefan regarding this).
As I see it's valid to call extcon_get_state() from the atomic
context, so probably
using something like ci_get_role()(or ci_detect_role(), whatever)
instead of explicitly
retrieving bits from OTGSC in every ID pin check is a good choice.

Thanks for your feedback!


>
> Peter
>
> > +             } else if (ci->role == CI_ROLE_GADGET) {
> > +                     struct ci_hdrc_cable *vbus = &ci->platdata->vbus_extcon;
> > +
> > +                     /* Use vbus state from extcon if provided */
> > +                     if (!IS_ERR(vbus->edev) &&
> > +                         extcon_get_state(vbus->edev, EXTCON_USB))
> > +                             usb_gadget_vbus_connect(&ci->gadget);
> > +                     else
> > +                             ci_handle_vbus_change(ci);
> > +             }
> >
> >               ret = ci_role_start(ci, ci->role);
> >               if (ret) {
> > @@ -1161,10 +1234,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto stop;
> >
> > -     ret = ci_extcon_register(ci);
> > -     if (ret)
> > -             goto stop;
> > -
> >       if (ci->supports_runtime_pm) {
> >               pm_runtime_set_active(&pdev->dev);
> >               pm_runtime_enable(&pdev->dev);
> > --
> > 2.17.1
> >
>
> --
>
> Thanks,
> Peter Chen

[1] https://docs.toradex.com/102491-colibri-arm-carrier-board-design-guide.pdf

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case
  2019-10-22 13:54   ` Igor Opaniuk
@ 2019-10-23  8:28     ` Peter Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Chen @ 2019-10-23  8:28 UTC (permalink / raw)
  To: Igor Opaniuk
  Cc: linux-usb, Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Sanchayan Maity,
	Igor Opaniuk, Greg Kroah-Hartman, linux-kernel

On 19-10-22 16:54:30, Igor Opaniuk wrote:
> Hi Peter,
> 
> On Tue, Oct 22, 2019 at 5:11 AM Peter Chen <peter.chen@nxp.com> wrote:
> >
> > On 19-10-21 19:16:53, Igor Opaniuk wrote:
> > > From: Stefan Agner <stefan.agner@toradex.com>
> > >
> > > The existing usage of extcon in chipidea driver freezes the kernel
> > > presumably due to OTGSC register access.
> > >
> > > Prevent accessing any OTG registers for SoC with dual role devices
> > > but no true OTG support. Use the flag CI_HDRC_DUAL_ROLE_NOT_OTG for
> > > those devices and in case extcon is present, do the role switch
> > > using extcon only.
> >
> > Hi Igor & Stefan,
> >
> > I have several questions about the problem you met:
> > - Which vendor's controller you have used?
> > - What do you mean "no true OTG"? Does it mean no "OTGSC" register?
> 
> Probably the commit message adds a bit of confusion here
> (I've kept the original one from the patch in our downstream kernel,
> but will probably reword it).
> 
> The actual problem is that USB_OTG1_ID pin isn't wired, so we can't rely
> on the value of ID pin state in OTGSC for the role detection.
> In our SoM (Colibri iMX6ULL) ID pin from USB connector is wired
> to SNVS_TAMPER2 which is pinmuxed as GPIO pin (GPIO5_02),
> [1] (this is schematic for the Carrier Board, not SoM (isn't publicly
> available),
> but there is a pretty good explanation + schematic
> in the section "2.3.2.2 USB 2.0 OTG Schematic Example ").

Ok, I clear now. Then, you may not use CI_HDRC_DUAL_ROLE_NOT_OTG which
is for the controller without OTGSC. For imx6ull, access OTGSC will not
hang the system if USB is NOT at suspend mode.

Current upstream design has already considered the user case for switch
role through GPIO, but there is an issue that the external cable
wakeup doesn't work, I will submit it later (see ci_extcon_wakeup_int
implementation at downstream kernel).

You could try to disable runtime-pm to see if the behaviour is expected
or not, if it is NOT expected, please report what's that?

> 
> >
> > >       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> > >               ret = ci_hdrc_host_init(ci);
> > > @@ -1145,8 +1208,18 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > >
> > >       if (!ci_otg_is_fsm_mode(ci)) {
> > >               /* only update vbus status for peripheral */
> > > -             if (ci->role == CI_ROLE_GADGET)
> > > -                     ci_handle_vbus_change(ci);
> > > +             if (dr_mode == USB_DR_MODE_PERIPHERAL) {
> > > +                     usb_gadget_vbus_connect(&ci->gadget);
> >
> > We only use ci->role at runtime, since it has already considered the
> > dts setting, kernel configuration and hardware setting.
> >
> > If your controller doesn't otgsc register, but do need to support
> > role switch, you may enhance the function ci_get_role
> 
> Btw, ci_get_role() implementation still resides in the NXP dowstream kernel
> and I've never seen anything posted to the ML (if it was, could you
> please point me to
> the patch?). I can introduce the new one, which wraps both OTGSC handling
> + extcon for CI_HDRC_DUAL_ROLE_NOT_OTG controllers.

Sorry about that, I just read code for the upstream kernel with some
downstream patches on it.

> 
> Frankly speaking, I don't know the reason why additional workqueue (ci->work_dr)
> was introduced (will try to reach Stefan regarding this).
> As I see it's valid to call extcon_get_state() from the atomic
> context, so probably
> using something like ci_get_role()(or ci_detect_role(), whatever)
> instead of explicitly
> retrieving bits from OTGSC in every ID pin check is a good choice.
> 

There are VBUS and ID events handling which are not non-atomic.

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] ARM: dts: colibri-imx6ull: add extcon for usbotg1
  2019-10-21 16:16   ` Igor Opaniuk
@ 2019-10-28  6:09     ` Shawn Guo
  -1 siblings, 0 replies; 8+ messages in thread
From: Shawn Guo @ 2019-10-28  6:09 UTC (permalink / raw)
  To: Igor Opaniuk
  Cc: linux-usb, Marcel Ziswiler, Philippe Schenker, Stefan Agner,
	Max Krummenacher, Oleksandr Suvorov, Igor Opaniuk, Fabio Estevam,
	Mark Rutland, NXP Linux Team, Pengutronix Kernel Team,
	Rob Herring, Sascha Hauer, devicetree, linux-arm-kernel,
	linux-kernel

On Mon, Oct 21, 2019 at 07:16:54PM +0300, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> Add extcon usb gpio configuration for support dual roles for usbotg1.
> 
> USB host/gadget switching test (1. USB NIC emulation; 2. USB storage):
> 
> [   52.491957] ci_hdrc ci_hdrc.1: switching to gadget role
> [   52.502911] mxs_phy 20c9000.usbphy: vbus is not valid
> [   56.749160] using random self ethernet address
> [   56.758637] using random host ethernet address
> [   65.768968] usb0: HOST MAC 00:14:2d:ff:ff:fe
> [   65.887980] usb0: MAC 00:14:2d:ff:ff:ff
> [   66.294961] configfs-gadget gadget: high-speed config #1: c
> [   78.741971] ci_hdrc ci_hdrc.1: switching to host role
> [   78.747522] ci_hdrc ci_hdrc.1: EHCI Host Controller
> [   78.790174] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus
> number 2
> [   78.868498] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> ---
> 
>  arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> index a78849fd2afa..988f1a800d5a 100644
> --- a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> @@ -29,6 +29,14 @@
>  		clock-frequency = <16000000>;
>  	};
>  
> +	extcon_usbc_det: usbc_det {

Can we find a more generic name for the node?  Also hyphen is preferred
over underscore in node name.

Shawn

> +		compatible = "linux,extcon-usb-gpio";
> +		debounce = <25>;
> +		id-gpio = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_snvs_usbc_det>;
> +	};
> +
>  	panel: panel {
>  		compatible = "edt,et057090dhu";
>  		backlight = <&bl>;
> @@ -150,6 +158,7 @@
>  };
>  
>  &usbotg1 {
> +	extcon = <&extcon_usbc_det &extcon_usbc_det>;
>  	status = "okay";
>  };
>  
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] ARM: dts: colibri-imx6ull: add extcon for usbotg1
@ 2019-10-28  6:09     ` Shawn Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Guo @ 2019-10-28  6:09 UTC (permalink / raw)
  To: Igor Opaniuk
  Cc: Mark Rutland, devicetree, Igor Opaniuk, Pengutronix Kernel Team,
	Stefan Agner, Marcel Ziswiler, Sascha Hauer, linux-usb,
	linux-kernel, Oleksandr Suvorov, Philippe Schenker, Rob Herring,
	NXP Linux Team, Max Krummenacher, Fabio Estevam,
	linux-arm-kernel

On Mon, Oct 21, 2019 at 07:16:54PM +0300, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> Add extcon usb gpio configuration for support dual roles for usbotg1.
> 
> USB host/gadget switching test (1. USB NIC emulation; 2. USB storage):
> 
> [   52.491957] ci_hdrc ci_hdrc.1: switching to gadget role
> [   52.502911] mxs_phy 20c9000.usbphy: vbus is not valid
> [   56.749160] using random self ethernet address
> [   56.758637] using random host ethernet address
> [   65.768968] usb0: HOST MAC 00:14:2d:ff:ff:fe
> [   65.887980] usb0: MAC 00:14:2d:ff:ff:ff
> [   66.294961] configfs-gadget gadget: high-speed config #1: c
> [   78.741971] ci_hdrc ci_hdrc.1: switching to host role
> [   78.747522] ci_hdrc ci_hdrc.1: EHCI Host Controller
> [   78.790174] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus
> number 2
> [   78.868498] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> ---
> 
>  arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> index a78849fd2afa..988f1a800d5a 100644
> --- a/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx6ull-colibri-eval-v3.dtsi
> @@ -29,6 +29,14 @@
>  		clock-frequency = <16000000>;
>  	};
>  
> +	extcon_usbc_det: usbc_det {

Can we find a more generic name for the node?  Also hyphen is preferred
over underscore in node name.

Shawn

> +		compatible = "linux,extcon-usb-gpio";
> +		debounce = <25>;
> +		id-gpio = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_snvs_usbc_det>;
> +	};
> +
>  	panel: panel {
>  		compatible = "edt,et057090dhu";
>  		backlight = <&bl>;
> @@ -150,6 +158,7 @@
>  };
>  
>  &usbotg1 {
> +	extcon = <&extcon_usbc_det &extcon_usbc_det>;
>  	status = "okay";
>  };
>  
> -- 
> 2.17.1
> 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-28  6:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 16:16 [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Igor Opaniuk
2019-10-21 16:16 ` [PATCH v1 2/2] ARM: dts: colibri-imx6ull: add extcon for usbotg1 Igor Opaniuk
2019-10-21 16:16   ` Igor Opaniuk
2019-10-28  6:09   ` Shawn Guo
2019-10-28  6:09     ` Shawn Guo
2019-10-22  2:11 ` [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case Peter Chen
2019-10-22 13:54   ` Igor Opaniuk
2019-10-23  8:28     ` Peter Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.