Re: [PATCH v5 11/23] usb: chipidea: Emulate OTGSC interrupt enable path

[Peter Chen]

On Thu, Oct 20, 2016 at 01:36:30PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2016-10-20 03:10:18)
> > On Wed, Oct 19, 2016 at 11:55:29PM -0700, Stephen Boyd wrote:
> > > Quoting Peter Chen (2016-10-19 01:02:11)
> > > > On Tue, Oct 18, 2016 at 06:53:07PM -0700, Stephen Boyd wrote:
> > > > > If you're asking if I've made modifications to extcon-usb-gpio, then 
> > > > > the
> > > > > answer is no. The branch on linaro.org git server from the 
> > > > > cover-letter
> > > > > is the branch I've used to test this with. This patch is specifically 
> > > > > to
> > > > > fix issues with that design on the db410c board that has only one pin
> > > > > for ID and vbus detection. It's the schematic that we've discussed in
> > > > > another thread.
> > > > > 
> > > > > extcon-usb-gpio sends two extcon events, EXTCON_USB_HOST (for the id
> > > > > pin) and EXTCON_USB (for the vbus). So afaik it does support vbus
> > > > > events.
> > > > > 
> > > > 
> > > > Hmm, in fact, your ID event is the same with vbus event, you take
> > > > external vbus event as ID event for extcon-usb-gpio handling. Yes,
> > > > it can work due to it sends EXTCON_USB_HOST event first.
> > > > 
> > > > Where you change the USB_SW_SEL_PM pin?
> > > 
> > > Currently that is done with the mux driver I sent based on the extcon
> > > event. We don't know if that's before or after the controller handles
> > > the extcon event though, so the pin should probably be changed from the
> > > chipidea driver instead to be more explicit. Why do you ask though?
> > 
> > I was just curious how you handle it, now I am clear. From my point,
> > the suitable way may be: mux driver + user app (echo role through
> > debugfs). The extcon-usb-gpio is not necessary, since you have already
> > known role at mux driver.
> > 
> > The current kernel has no framework to handle dual-role switch at kernel
> > space.
> 
> Ok. I'm planning that as future work after this set of patches is
> merged.
> 
> > > > @@ -1963,6 +1963,8 @@ static int udc_id_switch_for_device(struct 
> > > > ci_hdrc *ci)
> > > > /* Clear and enable BSV irq */
> > > > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> > > > OTGSC_BSVIS | OTGSC_BSVIE);
> > > > +   /* vbus change may has already been occurred */
> > > 
> > > "vbus change may have already occurred"?
> > 
> > Yes, will change.
> > 
> 
> Great. Should I wait to incorporate your change into my series, or will
> you apply the usb patches and Kishon can apply the phy patches?  This
> patch #11 should be dropped, but otherwise I don't think there's
> anything left to do for this series.

I tested my patch, it works well at my side, if it is ok for you, please
ack it, I will apply it as well as your chipidea series after your
gpu fix patch at greg's usb-next tree.

Is it ok for you?

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 11/23] usb: chipidea: Emulate OTGSC interrupt enable path

[Peter Chen]

On Wed, Oct 19, 2016 at 11:55:29PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2016-10-19 01:02:11)
> > On Tue, Oct 18, 2016 at 06:53:07PM -0700, Stephen Boyd wrote:
> > > If you're asking if I've made modifications to extcon-usb-gpio, then the
> > > answer is no. The branch on linaro.org git server from the cover-letter
> > > is the branch I've used to test this with. This patch is specifically to
> > > fix issues with that design on the db410c board that has only one pin
> > > for ID and vbus detection. It's the schematic that we've discussed in
> > > another thread.
> > > 
> > > extcon-usb-gpio sends two extcon events, EXTCON_USB_HOST (for the id
> > > pin) and EXTCON_USB (for the vbus). So afaik it does support vbus
> > > events.
> > > 
> > 
> > Hmm, in fact, your ID event is the same with vbus event, you take
> > external vbus event as ID event for extcon-usb-gpio handling. Yes,
> > it can work due to it sends EXTCON_USB_HOST event first.
> > 
> > Where you change the USB_SW_SEL_PM pin?
> 
> Currently that is done with the mux driver I sent based on the extcon
> event. We don't know if that's before or after the controller handles
> the extcon event though, so the pin should probably be changed from the
> chipidea driver instead to be more explicit. Why do you ask though?

I was just curious how you handle it, now I am clear. From my point,
the suitable way may be: mux driver + user app (echo role through
debugfs). The extcon-usb-gpio is not necessary, since you have already
known role at mux driver.

The current kernel has no framework to handle dual-role switch at kernel
space.

> > At some situations, the vbus may already be there before starting
> > gadget. So we need to check vbus event after switch to gadget in
> > order to handle missing vbus event. The typical use cases are plugging
> > vbus cable before driver load or the vbus has already been there
> > after stopping host but before starting gadget.
> > 
> > Signed-off-by: Peter Chen 
> 
> Yes this should work. Light testing doesn't show any problems so far.
>  
> > ---
> >  drivers/usb/chipidea/core.c |  4 
> >  drivers/usb/chipidea/otg.c  | 10 ++
> >  drivers/usb/chipidea/udc.c  |  2 ++
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index b814d91..a7d2c68 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -992,10 +992,6 @@ 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);
> > -
> > ret = ci_role_start(ci, ci->role);
> > if (ret) {
> > dev_err(dev, "can't start %s role\n",
> > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> > index 695f3fe..99c0709 100644
> > --- a/drivers/usb/chipidea/otg.c
> > +++ b/drivers/usb/chipidea/otg.c
> > @@ -134,9 +134,9 @@ void ci_handle_vbus_change(struct ci_hdrc *ci)
> > if (!ci->is_otg)
> > return;
> >  
> > -   if (hw_read_otgsc(ci, OTGSC_BSV))
> > +   if (hw_read_otgsc(ci, OTGSC_BSV) && !ci->vbus_active)
> > usb_gadget_vbus_connect(>gadget);
> > -   else
> > +   else if (!hw_read_otgsc(ci, OTGSC_BSV) && ci->vbus_active)
> > usb_gadget_vbus_disconnect(>gadget);
> >  }
> >  
> > @@ -175,10 +175,12 @@ static void ci_handle_id_switch(struct ci_hdrc *ci)
> >  
> > ci_role_stop(ci);
> >  
> > -   if (role == CI_ROLE_GADGET)
> > +   if (role == CI_ROLE_GADGET &&
> > +   IS_ERR(ci->platdata->vbus_extcon.edev))
> > /*
> >  * wait vbus lower than OTGSC_BSV before connecting
> > -* to host
> > +* to host. And if vbus's status is an external
> > +* connector, it doesn't need to wait here.
> 
> because OTGSC_BSV will toggle based on the extcon state and not when the
> phy connects to the host? It would be good to explain why it's not
> needed instead of just repeating what the code is doing.
> 

Thanks, will change like below

"
If connecting status is from an external connector instead of register,
we don't need to care vbus on the board, since it will not affect external
connector status.
"

> >  */
> > hw_wait_vbus_lower_bsv(ci);
> >  
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index 001c2fa..184ffba 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -1963,6 +1963,8 @@ static int udc_id_switch_for_device(struct ci_hdrc 
> > *ci)
> > /* Clear and enable 

Re: [PATCH v5 11/23] usb: chipidea: Emulate OTGSC interrupt enable path

[Peter Chen]

On Tue, Oct 18, 2016 at 06:53:07PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2016-10-18 18:15:35)
> > On Mon, Oct 17, 2016 at 06:56:24PM -0700, Stephen Boyd wrote:
> > > In the case of an extcon-usb-gpio device being used with the
> > > chipidea driver we'll sometimes miss the BSVIS event in the OTGSC
> > > register. Consider the case where we don't have a cable attached
> > > and the id pin is indicating "host" mode. When we plug in the usb
> > > cable for "device" mode a gpio goes high and indicates that we
> > > should do the role switch and that vbus is high. When we're in
> > > "host" mode the OTGSC register doesn't have BSVIE set.
> > 
> > I have some questions for your description:
> > 
> > - Do you have any pending or related patches what this patch set
> >   is based on? Afaik, the extcon-usb-gpio has no vbus event supported.
> 
> If you're asking if I've made modifications to extcon-usb-gpio, then the
> answer is no. The branch on linaro.org git server from the cover-letter
> is the branch I've used to test this with. This patch is specifically to
> fix issues with that design on the db410c board that has only one pin
> for ID and vbus detection. It's the schematic that we've discussed in
> another thread.
> 
> extcon-usb-gpio sends two extcon events, EXTCON_USB_HOST (for the id
> pin) and EXTCON_USB (for the vbus). So afaik it does support vbus
> events.
> 

Hmm, in fact, your ID event is the same with vbus event, you take
external vbus event as ID event for extcon-usb-gpio handling. Yes,
it can work due to it sends EXTCON_USB_HOST event first.

Where you change the USB_SW_SEL_PM pin?

> > - When the ID from 0->1, the chipidea driver will do role switch, and
> >   set BSVIE, why it does not occur for your case?
> 
> Right, that happens with this line in the sequence I describe below:
> 
>   hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, OTGSC_BSVIS | OTGSC_BSVIE);
> 
> but that happens much later than when the extcon event happens so we
> miss the interrupt. Technically, the driver isn't expecting the BSVIS
> interrupt to happen until BSVIE is set, but the extcon can come whenever
> it wants regardless of how the registers are configured in the
> controller.  So we have to do some sort of 'caching' here to remember
> that the vbus event happened and replay it when BSVIE is set. At least I
> imagine this is how the hardware would work? Or if vbus goes high before
> we enable the interrupt would it just be missed? It seems like polling
> the BSV bit and then enabling BSVIE is sort of racy there.
> 
> Plus, we poll the BSV bit when we role switch, but in my case id bit
> toggles and vbus goes high at exactly the same time because that is all
> happening from a single cable being connected, so it's not possible for
> BSV to go low and see it after the id pin from 0 to 1.

Now, I understand your case, but your changes are a little complicated.
Would you try if below patch can fix your issue?

>From 8b8baf31dcaca53612d0fd91068c84fe09d66f6c Mon Sep 17 00:00:00 2001
From: Peter Chen 
Date: Wed, 19 Oct 2016 15:32:58 +0800
Subject: [PATCH 1/1] usb: chipidea: vbus event may exist before starting
 gadget

At some situations, the vbus may already be there before starting
gadget. So we need to check vbus event after switch to gadget in
order to handle missing vbus event. The typical use cases are plugging
vbus cable before driver load or the vbus has already been there
after stopping host but before starting gadget.

Signed-off-by: Peter Chen 
---
 drivers/usb/chipidea/core.c |  4 
 drivers/usb/chipidea/otg.c  | 10 ++
 drivers/usb/chipidea/udc.c  |  2 ++
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index b814d91..a7d2c68 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -992,10 +992,6 @@ 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);
-
ret = ci_role_start(ci, ci->role);
if (ret) {
dev_err(dev, "can't start %s role\n",
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 695f3fe..99c0709 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -134,9 +134,9 @@ void ci_handle_vbus_change(struct ci_hdrc *ci)
if (!ci->is_otg)
return;
 
-   if (hw_read_otgsc(ci, OTGSC_BSV))
+   if (hw_read_otgsc(ci, OTGSC_BSV) && !ci->vbus_active)
usb_gadget_vbus_connect(>gadget);
-   else
+   else if (!hw_read_otgsc(ci, OTGSC_BSV) && ci->vbus_active)
usb_gadget_vbus_disconnect(>gadget);
 }
 
@@ -175,10 +175,12 @@ static void ci_handle_id_switch(struct ci_hdrc *ci)
 

Re: [PATCH v5 11/23] usb: chipidea: Emulate OTGSC interrupt enable path

[Peter Chen]

On Mon, Oct 17, 2016 at 06:56:24PM -0700, Stephen Boyd wrote:
> In the case of an extcon-usb-gpio device being used with the
> chipidea driver we'll sometimes miss the BSVIS event in the OTGSC
> register. Consider the case where we don't have a cable attached
> and the id pin is indicating "host" mode. When we plug in the usb
> cable for "device" mode a gpio goes high and indicates that we
> should do the role switch and that vbus is high. When we're in
> "host" mode the OTGSC register doesn't have BSVIE set.

I have some questions for your description:

- Do you have any pending or related patches what this patch set
  is based on? Afaik, the extcon-usb-gpio has no vbus event supported.
- When the ID from 0->1, the chipidea driver will do role switch, and
  set BSVIE, why it does not occur for your case?

Peter
> 
> The following scenario can happen:
> 
> CPU0
> 
> 
>  ci_cable_notifier()
>   update id cable state
>   ci_irq()
>if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { // true
> ci->id_event = true;
> ci_otg_queue_work()
>  schedule()
> 
>  // same task as before
>  ci_cable_notifier()
>   update vbus cable state
>ci_irq()
> if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) // false
>return IRQ_NONE
> 
> ci_otg_work() // switch task to the workqueue now
>  if (ci->id_event)
>   ci_handle_id_switch()
>ci_role_stop()
> host_stop()
>hw_wait_vbus_lower_bsv(ci); // this times out because vbus is already set
>ci_role_start()
> udc_id_switch_for_device()
>  hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, OTGSC_BSVIS | OTGSC_BSVIE);
> 
> At this point, we don't replay the vbus connect event because the
> vbus event has already happened. This causes things like gadget
> instances to never see vbus appear, and thus the gadget is never
> started. Furthermore, we see timeout messages like:
> 
>   timeout waiting for 800 in OTGSC
> 
> Let's workaround this by skiping the wait for BSV when we're
> using an extcon for the vbus notification and let's properly
> emulate the BSVIS event that would happen when we enable the
> vbus interrupt while enabling "device" mode.
> 
> Cc: Peter Chen 
> Cc: Greg Kroah-Hartman 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/usb/chipidea/ci.h   |  2 ++
>  drivers/usb/chipidea/core.c | 23 +--
>  drivers/usb/chipidea/otg.c  | 31 ---
>  3 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 59e22389c10b..e099b8bc79e2 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -437,6 +437,8 @@ static inline void ci_ulpi_exit(struct ci_hdrc *ci) { }
>  static inline int ci_ulpi_resume(struct ci_hdrc *ci) { return 0; }
>  #endif
>  
> +irqreturn_t __ci_irq(int irq, struct ci_hdrc *ci);
> +
>  u32 hw_read_intr_enable(struct ci_hdrc *ci);
>  
>  u32 hw_read_intr_status(struct ci_hdrc *ci);
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 83bc2f2dd6a8..d1ae9a03e0fa 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -524,9 +524,8 @@ int hw_device_reset(struct ci_hdrc *ci)
>   return 0;
>  }
>  
> -static irqreturn_t ci_irq(int irq, void *data)
> +irqreturn_t __ci_irq(int irq, struct ci_hdrc *ci)
>  {
> - struct ci_hdrc *ci = data;
>   irqreturn_t ret = IRQ_NONE;
>   u32 otgsc = 0;
>  
> @@ -570,9 +569,20 @@ static irqreturn_t ci_irq(int irq, void *data)
>   return IRQ_HANDLED;
>   }
>  
> - /* Handle device/host interrupt */
> - if (ci->role != CI_ROLE_END)
> - ret = ci_role(ci)->irq(ci);
> + return ret;
> +}
> +
> +static irqreturn_t ci_irq(int irq, void *data)
> +{
> + irqreturn_t ret;
> + struct ci_hdrc *ci = data;
> +
> + ret = __ci_irq(irq, ci);
> + if (ret == IRQ_NONE) {
> + /* Handle device/host interrupt */
> + if (ci->role != CI_ROLE_END)
> + ret = ci_role(ci)->irq(ci);
> + }
>  
>   return ret;
>  }
> @@ -586,7 +596,8 @@ static int ci_cable_notifier(struct notifier_block *nb, 
> unsigned long event,
>   cbl->connected = event;
>   cbl->changed = true;
>  
> - ci_irq(ci->irq, ci);
> + __ci_irq(ci->irq, ci);
> +
>   return NOTIFY_DONE;
>  }
>  
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 695f3fe3ae21..f4a21ade1901 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -84,36 +84,44 @@ u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask)
>  void hw_write_otgsc(struct ci_hdrc *ci, u32 mask, u32 data)
>  {
>   struct ci_hdrc_cable *cable;
> + bool raise_irq = false;
>  
>   cable = >platdata->vbus_extcon;
>   if (!IS_ERR(cable->edev)) {
> - if (data & mask & OTGSC_BSVIS)
> -