diff mbox series

[v1,1/2] usb: chipidea: use of extcon framework to work for non OTG case

Message ID 20191021161654.14353-1-igor.opaniuk@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v1,1/2] usb: chipidea: use of extcon framework to work for non OTG case | expand

Commit Message

Igor Opaniuk Oct. 21, 2019, 4:16 p.m. UTC 
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(-)

Comments

Peter Chen Oct. 22, 2019, 2:11 a.m. UTC | #1 
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
>
Igor Opaniuk Oct. 22, 2019, 1:54 p.m. UTC | #2 
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
Peter Chen Oct. 23, 2019, 8:28 a.m. UTC | #3 
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.
diff mbox series

Patch

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