| Message ID | 20161018015636.11701-12-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > ---- > <extcon notifier chain> > 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() > > <extcon notifier event> // 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 0000800 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 <peter.chen@nxp.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > 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 = &ci->platdata->vbus_extcon; > if (!IS_ERR(cable->edev)) { > - if (data & mask & OTGSC_BSVIS) > - cable->changed = false; > - > /* Don't enable vbus interrupt if using external notifier */ > if (data & mask & OTGSC_BSVIE) { > + if (cable->enabled == false && cable->changed == true) > + raise_irq = true; > cable->enabled = true; > data &= ~OTGSC_BSVIE; > } else if (mask & OTGSC_BSVIE) { > cable->enabled = false; > } > + > + if (data & mask & OTGSC_BSVIS && !raise_irq) > + cable->changed = false; > } > > cable = &ci->platdata->id_extcon; > if (!IS_ERR(cable->edev)) { > - if (data & mask & OTGSC_IDIS) > - cable->changed = false; > - > /* Don't enable id interrupt if using external notifier */ > if (data & mask & OTGSC_IDIE) { > + if (cable->enabled == false && cable->changed == true) > + raise_irq = true; > cable->enabled = true; > data &= ~OTGSC_IDIE; > } else if (mask & OTGSC_IDIE) { > cable->enabled = false; > } > + > + if (data & mask & OTGSC_IDIS && !raise_irq) > + cable->changed = false; > } > > hw_write(ci, OP_OTGSC, mask | OTGSC_INT_STATUS_BITS, data); > + > + if (raise_irq) > + __ci_irq(ci->irq, ci); > } > > /** > @@ -175,7 +183,16 @@ static void ci_handle_id_switch(struct ci_hdrc *ci) > > ci_role_stop(ci); > > - if (role == CI_ROLE_GADGET) > + /* > + * BSV could be set "immediately" if we're using extcon for > + * VBUS because sometimes it's a single GPIO for ID and VBUS > + * like in the case of extcon-usb-gpio. In that case we ignore > + * waiting for a BSV transition. Really we can't tell when BSV > + * is low and the cable is connected, all we know is that the > + * BSV is high when we update BSV state. > + */ > + if (role == CI_ROLE_GADGET && > + IS_ERR(ci->platdata->vbus_extcon.edev)) > /* > * wait vbus lower than OTGSC_BSV before connecting > * to host > -- > 2.10.0.297.gf6727b0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. > - 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. > > Peter > > > > The following scenario can happen: > > > > CPU0 > > ---- > > <extcon notifier chain> > > 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() > > > > <extcon notifier event> // 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 0000800 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.
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 = &ci->platdata->vbus_extcon; if (!IS_ERR(cable->edev)) { - if (data & mask & OTGSC_BSVIS) - cable->changed = false; - /* Don't enable vbus interrupt if using external notifier */ if (data & mask & OTGSC_BSVIE) { + if (cable->enabled == false && cable->changed == true) + raise_irq = true; cable->enabled = true; data &= ~OTGSC_BSVIE; } else if (mask & OTGSC_BSVIE) { cable->enabled = false; } + + if (data & mask & OTGSC_BSVIS && !raise_irq) + cable->changed = false; } cable = &ci->platdata->id_extcon; if (!IS_ERR(cable->edev)) { - if (data & mask & OTGSC_IDIS) - cable->changed = false; - /* Don't enable id interrupt if using external notifier */ if (data & mask & OTGSC_IDIE) { + if (cable->enabled == false && cable->changed == true) + raise_irq = true; cable->enabled = true; data &= ~OTGSC_IDIE; } else if (mask & OTGSC_IDIE) { cable->enabled = false; } + + if (data & mask & OTGSC_IDIS && !raise_irq) + cable->changed = false; } hw_write(ci, OP_OTGSC, mask | OTGSC_INT_STATUS_BITS, data); + + if (raise_irq) + __ci_irq(ci->irq, ci); } /** @@ -175,7 +183,16 @@ static void ci_handle_id_switch(struct ci_hdrc *ci) ci_role_stop(ci); - if (role == CI_ROLE_GADGET) + /* + * BSV could be set "immediately" if we're using extcon for + * VBUS because sometimes it's a single GPIO for ID and VBUS + * like in the case of extcon-usb-gpio. In that case we ignore + * waiting for a BSV transition. Really we can't tell when BSV + * is low and the cable is connected, all we know is that the + * BSV is high when we update BSV state. + */ + if (role == CI_ROLE_GADGET && + IS_ERR(ci->platdata->vbus_extcon.edev)) /* * wait vbus lower than OTGSC_BSV before connecting * to host
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. The following scenario can happen: CPU0 ---- <extcon notifier chain> 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() <extcon notifier event> // 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 0000800 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 <peter.chen@nxp.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- 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(-)