| Message ID | 1477479503-5131-4-git-send-email-abailon@baylibre.com |
|---|---|
| State | New |
| Headers | show |
On 10/26/2016 05:58 AM, Alexandre Bailon wrote: > When the phy is forced in host mode, only the first hot plug and > hot remove works. That is actually because the driver execute the > OTG workaround, whereas it is not applicable in host or device mode. > Indeed, to work correctly, the VBUS sense and session end comparator > must be enabled, what is only possible when the phy is in OTG mode. > Only execute the workaround if the phy is in OTG mode. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/usb/musb/da8xx.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c > index 6749aa1..b8a6b65 100644 > --- a/drivers/usb/musb/da8xx.c > +++ b/drivers/usb/musb/da8xx.c > @@ -145,6 +145,17 @@ static void otg_timer(unsigned long _musb) > unsigned long flags; > > /* > + * We should only execute the OTG workaround when the phy is in OTG > + * mode. The workaround require the VBUS sense and the session end > + * comparator to be enabled, what is only possible if the phy is in > + * OTG mode. As the workaround is only required to detect if the > + * controller must act as host or device, we can safely exit OTG is > + * not in use. > + */ > + if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) musb->port_mode is not valid if we have changed the mode via sysfs. It only reflects the mode set during driver probe. Furthermore, this breaks the host mode completely for me. The first hot plug is not even detected. > + return; > + > + /* > * We poll because DaVinci's won't expose several OTG-critical > * status change events (from the transceiver) otherwise. > */ > The way this is working for me (on AM1808) is this: The problem is not that the OTG workaround is being used. The problem is that after disconnect, the VBUSDRV is turned off. If you look at the handler for DA8XX_INTR_DRVVBUS in da8xx_musb_interrupt(), you will see that if VBUSDRV is off, then drvvbus == 0, which puts the musb state back to device mode. I also ran into a similar problem a while back[1] that if you use a self-powered device in host mode, it immediately becomes disconnected. This is for the exact same reason. When a port detects a self-powered device, it turns of VBUSDRV, which triggers the DA8XX_INTR_DRVVBUS interrupt. As we have seen above, this takes the port out of host mode. The workaround that I have found that seems to fix both cases is to add and else if statement that toggles the PHY host override when we are forcing host mode and the VBUSDRV is turned off. Here is a partial diff of drivers/usb/musb/da8xx.c to show what I mean: @@ -304,10 +309,14 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci) * Also, DRVVBUS pulses for SRP (but not at 5 V)... */ if (status & (DA8XX_INTR_DRVVBUS << DA8XX_INTR_USB_SHIFT)) { + struct da8xx_glue *glue = + dev_get_drvdata(musb->controller->parent); int drvvbus = musb_readl(reg_base, DA8XX_USB_STAT_REG); void __iomem *mregs = musb->mregs; u8 devctl = musb_readb(mregs, MUSB_DEVCTL); - int err; + int cfgchip2, err; + + regmap_read(glue->cfgchip, CFGCHIP(2), &cfgchip2); err = musb->int_usb & MUSB_INTR_VBUSERROR; if (err) { @@ -332,10 +341,25 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci) musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE; portstate(musb->port1_status |= USB_PORT_STAT_POWER); del_timer(&otg_workaround); + } else if ((cfgchip2 & CFGCHIP2_OTGMODE_MASK) + == CFGCHIP2_OTGMODE_FORCE_HOST) { + /* + * If we are forcing host mode, VBUSDRV is turned off + * after a device is disconnected. We need to toggle the + * VBUS/ID override to trigger turn it back on, which + * has the effect of triggering DA8XX_INTR_DRVVBUS again. + */ + regmap_write_bits(glue->cfgchip, CFGCHIP(2), + CFGCHIP2_OTGMODE_MASK, + CFGCHIP2_OTGMODE_NO_OVERRIDE); + regmap_write_bits(glue->cfgchip, CFGCHIP(2), + CFGCHIP2_OTGMODE_MASK, + CFGCHIP2_OTGMODE_FORCE_HOST); } else { musb->is_active = 0; MUSB_DEV_MODE(musb);
On 10/28/2016 07:39 AM, Alexandre Bailon wrote: > On 10/28/2016 04:56 AM, David Lechner wrote: >> On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >>> When the phy is forced in host mode, only the first hot plug and >>> hot remove works. That is actually because the driver execute the >>> OTG workaround, whereas it is not applicable in host or device mode. >>> Indeed, to work correctly, the VBUS sense and session end comparator >>> must be enabled, what is only possible when the phy is in OTG mode. >>> Only execute the workaround if the phy is in OTG mode. >>> >>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >>> --- >>> drivers/usb/musb/da8xx.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c >>> index 6749aa1..b8a6b65 100644 >>> --- a/drivers/usb/musb/da8xx.c >>> +++ b/drivers/usb/musb/da8xx.c >>> @@ -145,6 +145,17 @@ static void otg_timer(unsigned long _musb) >>> unsigned long flags; >>> >>> /* >>> + * We should only execute the OTG workaround when the phy is in OTG >>> + * mode. The workaround require the VBUS sense and the session end >>> + * comparator to be enabled, what is only possible if the phy is in >>> + * OTG mode. As the workaround is only required to detect if the >>> + * controller must act as host or device, we can safely exit OTG is >>> + * not in use. >>> + */ >>> + if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) >> >> musb->port_mode is not valid if we have changed the mode via sysfs. It >> only reflects the mode set during driver probe. >> >> Furthermore, this breaks the host mode completely for me. The first hot >> plug is not even detected. >> >>> + return; >>> + >>> + /* >>> * We poll because DaVinci's won't expose several OTG-critical >>> * status change events (from the transceiver) otherwise. >>> */ >>> >> >> >> The way this is working for me (on AM1808) is this: >> >> The problem is not that the OTG workaround is being used. The problem is >> that after disconnect, the VBUSDRV is turned off. If you look at the >> handler for DA8XX_INTR_DRVVBUS in da8xx_musb_interrupt(), you will see >> that if VBUSDRV is off, then drvvbus == 0, which puts the musb state >> back to device mode. >> >> I also ran into a similar problem a while back[1] that if you use a >> self-powered device in host mode, it immediately becomes disconnected. >> This is for the exact same reason. When a port detects a self-powered >> device, it turns of VBUSDRV, which triggers the DA8XX_INTR_DRVVBUS >> interrupt. As we have seen above, this takes the port out of host mode. >> >> The workaround that I have found that seems to fix both cases is to add >> and else if statement that toggles the PHY host override when we are >> forcing host mode and the VBUSDRV is turned off. > I like this workaround. >> >> Here is a partial diff of drivers/usb/musb/da8xx.c to show what I mean: >> >> @@ -304,10 +309,14 @@ static irqreturn_t da8xx_musb_interrupt(int irq, >> void *hci) >> * Also, DRVVBUS pulses for SRP (but not at 5 V)... >> */ >> if (status & (DA8XX_INTR_DRVVBUS << DA8XX_INTR_USB_SHIFT)) { >> + struct da8xx_glue *glue = >> + dev_get_drvdata(musb->controller->parent); >> int drvvbus = musb_readl(reg_base, DA8XX_USB_STAT_REG); >> void __iomem *mregs = musb->mregs; >> u8 devctl = musb_readb(mregs, MUSB_DEVCTL); >> - int err; >> + int cfgchip2, err; >> + >> + regmap_read(glue->cfgchip, CFGCHIP(2), &cfgchip2); >> >> err = musb->int_usb & MUSB_INTR_VBUSERROR; >> if (err) { >> @@ -332,10 +341,25 @@ static irqreturn_t da8xx_musb_interrupt(int irq, >> void *hci) >> musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE; >> portstate(musb->port1_status |= >> USB_PORT_STAT_POWER); >> del_timer(&otg_workaround); >> + } else if ((cfgchip2 & CFGCHIP2_OTGMODE_MASK) >> + == CFGCHIP2_OTGMODE_FORCE_HOST) { >> + /* >> + * If we are forcing host mode, VBUSDRV is >> turned off >> + * after a device is disconnected. We need to >> toggle the >> + * VBUS/ID override to trigger turn it back on, >> which >> + * has the effect of triggering >> DA8XX_INTR_DRVVBUS again. >> + */ >> + regmap_write_bits(glue->cfgchip, CFGCHIP(2), >> + CFGCHIP2_OTGMODE_MASK, >> + CFGCHIP2_OTGMODE_NO_OVERRIDE); >> + regmap_write_bits(glue->cfgchip, CFGCHIP(2), >> + CFGCHIP2_OTGMODE_MASK, >> + CFGCHIP2_OTGMODE_FORCE_HOST); >> } else { >> musb->is_active = 0; >> MUSB_DEV_MODE(musb); >> > I haven't thought to this workaround. > Actually, my goal with this patch was to prevent VBUSDRV to be turned > off. When I hit the issues, I captured some traces and these traces > let think VBUSDRV is turned off when the OTG workaround clear > the bit MUSB_DEVCTL_SESSION. > After having slept on this, I am realizing that the "correct" thing to do here is highly hardware dependent. If you look at musb_probe() in musb_core.c, you will see the true purpose of musb->port_mode. In host mode, it only sets up a host device, in peripheral mode, it only sets up a peripheral device and in otg mode, it sets up both. So really, this musb->port_mode setting does not say anything about what the hardware is actually capable of. It is just telling which devices you want registered in the kernel. (As a side note, this means that the dr_mode property is really not appropriate for device tree in your other patch series - even though many existing USB devices use dr_mode anyway). The CFGCHIP2_OTGMODE_* options are to work around hardware deficiencies. They are only needed when a port is missing the required external circuitry to function correctly. I think it is wrong to assume that if someone selects a specific musb->port_mode then they need to enable one of CFGCHIP2_OTGMODE_FORCE_*. If the port has the proper circuitry for OTG, then one should be able to select any of host, peripheral or otg mode without needing to set any of CFGCHIP2_OTGMODE_FORCE_*. So, I think if we want to be able to use CFGCHIP2_OTGMODE_FORCE_* for special cases, then we need to add a module parameter (or this might fit in device tree if we can figure out how to express it as "describing the hardware"). The parameter will basically say "override PHY VBUS/ID in host mode if and only if this parameter is enabled". We could also have a parameter for peripheral mode that says "override PHY VBUS/ID in peripheral mode if and only if this parameter is enabled". As an example, on LEGO MINDSTORMS EV3, the USB port is wired for peripheral only. There is nothing connected to the VBUSDRV or ID pins. Furthermore, the VBUS pin is only connected to the USB jack and there is not a way to generate VBUS power. So, we can set musb->port_mode = MUSB_PORT_MODE_GADGET and everything will work as expected as long as we don't set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL. Overriding the PHY breaks VBUS sense and we never get back to b_idle after a device disconnect. (In fact, the only time one would ever need to set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL is if the VBUS pin is not connected at all and/or the ID pin is hardwired to ground). If we want to be crazy though and be able to switch between host and peripheral mode anyway, even though the required circuits are missing, we can set musb->port_mode = MUSB_PORT_MODE_OTG. Then we can write "host" to the "mode" sysfs attribute to force the port into host mode. However, in order for this to work, it requires that CFGCHIP2_OTGMODE_FORCE_HOST is set because of the missing circuitry for host mode. You have to supply your own external VBUS, but it does work. TL;DR I think you fill find that if we remove the da8xx_musb_set_mode() callback completely, that both host and peripheral mode will work for you. Overriding the PHY is only needed for unusual cases, like my example where we are forcing host mode when the required circuitry is missing.
On Fri, Oct 28, 2016 at 12:11:21PM -0500, David Lechner wrote: > On 10/28/2016 07:39 AM, Alexandre Bailon wrote: > >On 10/28/2016 04:56 AM, David Lechner wrote: > >>On 10/26/2016 05:58 AM, Alexandre Bailon wrote: > >>>When the phy is forced in host mode, only the first hot plug and > >>>hot remove works. That is actually because the driver execute the > >>>OTG workaround, whereas it is not applicable in host or device mode. > >>>Indeed, to work correctly, the VBUS sense and session end comparator > >>>must be enabled, what is only possible when the phy is in OTG mode. > >>>Only execute the workaround if the phy is in OTG mode. > >>> > >>>Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > >>>--- > >>> drivers/usb/musb/da8xx.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>>diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c > >>>index 6749aa1..b8a6b65 100644 > >>>--- a/drivers/usb/musb/da8xx.c > >>>+++ b/drivers/usb/musb/da8xx.c > >>>@@ -145,6 +145,17 @@ static void otg_timer(unsigned long _musb) > >>> unsigned long flags; > >>> > >>> /* > >>>+ * We should only execute the OTG workaround when the phy is in OTG > >>>+ * mode. The workaround require the VBUS sense and the session end > >>>+ * comparator to be enabled, what is only possible if the phy is in > >>>+ * OTG mode. As the workaround is only required to detect if the > >>>+ * controller must act as host or device, we can safely exit OTG is > >>>+ * not in use. > >>>+ */ > >>>+ if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) > >> > >>musb->port_mode is not valid if we have changed the mode via sysfs. It > >>only reflects the mode set during driver probe. > >> > >>Furthermore, this breaks the host mode completely for me. The first hot > >>plug is not even detected. > >> > >>>+ return; > >>>+ > >>>+ /* > >>> * We poll because DaVinci's won't expose several OTG-critical > >>> * status change events (from the transceiver) otherwise. > >>> */ > >>> > >> > >> > >>The way this is working for me (on AM1808) is this: > >> > >>The problem is not that the OTG workaround is being used. The problem is > >>that after disconnect, the VBUSDRV is turned off. If you look at the > >>handler for DA8XX_INTR_DRVVBUS in da8xx_musb_interrupt(), you will see > >>that if VBUSDRV is off, then drvvbus == 0, which puts the musb state > >>back to device mode. > >> > >>I also ran into a similar problem a while back[1] that if you use a > >>self-powered device in host mode, it immediately becomes disconnected. > >>This is for the exact same reason. When a port detects a self-powered > >>device, it turns of VBUSDRV, which triggers the DA8XX_INTR_DRVVBUS > >>interrupt. As we have seen above, this takes the port out of host mode. > >> > >>The workaround that I have found that seems to fix both cases is to add > >>and else if statement that toggles the PHY host override when we are > >>forcing host mode and the VBUSDRV is turned off. > >I like this workaround. > >> > >>Here is a partial diff of drivers/usb/musb/da8xx.c to show what I mean: > >> > >>@@ -304,10 +309,14 @@ static irqreturn_t da8xx_musb_interrupt(int irq, > >>void *hci) > >> * Also, DRVVBUS pulses for SRP (but not at 5 V)... > >> */ > >> if (status & (DA8XX_INTR_DRVVBUS << DA8XX_INTR_USB_SHIFT)) { > >>+ struct da8xx_glue *glue = > >>+ dev_get_drvdata(musb->controller->parent); > >> int drvvbus = musb_readl(reg_base, DA8XX_USB_STAT_REG); > >> void __iomem *mregs = musb->mregs; > >> u8 devctl = musb_readb(mregs, MUSB_DEVCTL); > >>- int err; > >>+ int cfgchip2, err; > >>+ > >>+ regmap_read(glue->cfgchip, CFGCHIP(2), &cfgchip2); > >> > >> err = musb->int_usb & MUSB_INTR_VBUSERROR; > >> if (err) { > >>@@ -332,10 +341,25 @@ static irqreturn_t da8xx_musb_interrupt(int irq, > >>void *hci) > >> musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE; > >> portstate(musb->port1_status |= > >>USB_PORT_STAT_POWER); > >> del_timer(&otg_workaround); > >>+ } else if ((cfgchip2 & CFGCHIP2_OTGMODE_MASK) > >>+ == CFGCHIP2_OTGMODE_FORCE_HOST) { > >>+ /* > >>+ * If we are forcing host mode, VBUSDRV is > >>turned off > >>+ * after a device is disconnected. We need to > >>toggle the > >>+ * VBUS/ID override to trigger turn it back on, > >>which > >>+ * has the effect of triggering > >>DA8XX_INTR_DRVVBUS again. > >>+ */ > >>+ regmap_write_bits(glue->cfgchip, CFGCHIP(2), > >>+ CFGCHIP2_OTGMODE_MASK, > >>+ CFGCHIP2_OTGMODE_NO_OVERRIDE); > >>+ regmap_write_bits(glue->cfgchip, CFGCHIP(2), > >>+ CFGCHIP2_OTGMODE_MASK, > >>+ CFGCHIP2_OTGMODE_FORCE_HOST); > >> } else { > >> musb->is_active = 0; > >> MUSB_DEV_MODE(musb); > >> > >I haven't thought to this workaround. > >Actually, my goal with this patch was to prevent VBUSDRV to be turned > >off. When I hit the issues, I captured some traces and these traces > >let think VBUSDRV is turned off when the OTG workaround clear > >the bit MUSB_DEVCTL_SESSION. > > > > > After having slept on this, I am realizing that the "correct" thing > to do here is highly hardware dependent. If you look at musb_probe() > in musb_core.c, you will see the true purpose of musb->port_mode. In > host mode, it only sets up a host device, in peripheral mode, it > only sets up a peripheral device and in otg mode, it sets up both. > > So really, this musb->port_mode setting does not say anything about > what the hardware is actually capable of. It is just telling which > devices you want registered in the kernel. (As a side note, this > means that the dr_mode property is really not appropriate for device > tree in your other patch series - even though many existing USB > devices use dr_mode anyway). > > The CFGCHIP2_OTGMODE_* options are to work around hardware > deficiencies. They are only needed when a port is missing the > required external circuitry to function correctly. I think it is > wrong to assume that if someone selects a specific musb->port_mode > then they need to enable one of CFGCHIP2_OTGMODE_FORCE_*. > > If the port has the proper circuitry for OTG, then one should be > able to select any of host, peripheral or otg mode without needing > to set any of CFGCHIP2_OTGMODE_FORCE_*. > > So, I think if we want to be able to use CFGCHIP2_OTGMODE_FORCE_* > for special cases, then we need to add a module parameter (or this > might fit in device tree if we can figure out how to express it as > "describing the hardware"). The parameter will basically say > "override PHY VBUS/ID in host mode if and only if this parameter is > enabled". We could also have a parameter for peripheral mode that > says "override PHY VBUS/ID in peripheral mode if and only if this > parameter is enabled". Module parameters are no longer acceptable, but we can introduce quirk flags to solve this. > > As an example, on LEGO MINDSTORMS EV3, the USB port is wired for > peripheral only. There is nothing connected to the VBUSDRV or ID > pins. Furthermore, the VBUS pin is only connected to the USB jack > and there is not a way to generate VBUS power. So, we can set > musb->port_mode = MUSB_PORT_MODE_GADGET and everything will work as > expected as long as we don't set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL. > Overriding the PHY breaks VBUS sense and we never get back to b_idle > after a device disconnect. (In fact, the only time one would ever > need to set CFGCHIP2_OTGMODE_FORCE_PERIPHERAL is if the VBUS pin is > not connected at all and/or the ID pin is hardwired to ground). > > If we want to be crazy though and be able to switch between host and > peripheral mode anyway, even though the required circuits are > missing, we can set musb->port_mode = MUSB_PORT_MODE_OTG. Then we > can write "host" to the "mode" sysfs attribute to force the port > into host mode. However, in order for this to work, it requires that > CFGCHIP2_OTGMODE_FORCE_HOST is set because of the missing circuitry > for host mode. You have to supply your own external VBUS, but it > does work. > > TL;DR > > I think you fill find that if we remove the da8xx_musb_set_mode() > callback completely, that both host and peripheral mode will work > for you. Overriding the PHY is only needed for unusual cases, like > my example where we are forcing host mode when the required > circuitry is missing. TL;DR, but it all sounds similar to that in the musb_dsps glue, you might find some ideas from there. Regards, -Bin.
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c index 6749aa1..b8a6b65 100644 --- a/drivers/usb/musb/da8xx.c +++ b/drivers/usb/musb/da8xx.c @@ -145,6 +145,17 @@ static void otg_timer(unsigned long _musb) unsigned long flags; /* + * We should only execute the OTG workaround when the phy is in OTG + * mode. The workaround require the VBUS sense and the session end + * comparator to be enabled, what is only possible if the phy is in + * OTG mode. As the workaround is only required to detect if the + * controller must act as host or device, we can safely exit OTG is + * not in use. + */ + if (musb->port_mode != MUSB_PORT_MODE_DUAL_ROLE) + return; + + /* * We poll because DaVinci's won't expose several OTG-critical * status change events (from the transceiver) otherwise. */
When the phy is forced in host mode, only the first hot plug and hot remove works. That is actually because the driver execute the OTG workaround, whereas it is not applicable in host or device mode. Indeed, to work correctly, the VBUS sense and session end comparator must be enabled, what is only possible when the phy is in OTG mode. Only execute the workaround if the phy is in OTG mode. Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- drivers/usb/musb/da8xx.c | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 2.7.3