* [PATCH v2 0/3] usb: musb: da8xx: Fix few issues
@ 2016-10-26 10:58 Alexandre Bailon
2016-10-26 10:58 ` [PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable() Alexandre Bailon
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Alexandre Bailon @ 2016-10-26 10:58 UTC (permalink / raw)
To: khilman, david, b-liu, balbi; +Cc: linux-kernel, linux-usb, Alexandre Bailon
Currently, the USB OTG of the da8xx doesn't work.
This series intend to fix them.
Change in v2:
* Fix the error path da8xx_musb_init()
Alexandre Bailon (3):
usb: musb: da8xx: Call earlier clk_prepare_enable()
phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround
usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode
drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++-----
drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++---------
2 files changed, 31 insertions(+), 14 deletions(-)
--
2.7.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable() 2016-10-26 10:58 [PATCH v2 0/3] usb: musb: da8xx: Fix few issues Alexandre Bailon @ 2016-10-26 10:58 ` Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround Alexandre Bailon ` (2 subsequent siblings) 3 siblings, 0 replies; 15+ messages in thread From: Alexandre Bailon @ 2016-10-26 10:58 UTC (permalink / raw) To: khilman, david, b-liu, balbi; +Cc: linux-kernel, linux-usb, Alexandre Bailon The first attempt to read a register may fail because the clock may not be enabled, and then the probe of musb driver will fail. Call clk_prepare_enable() before the first register read. Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- drivers/usb/musb/da8xx.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c index 210b7e4..6749aa1 100644 --- a/drivers/usb/musb/da8xx.c +++ b/drivers/usb/musb/da8xx.c @@ -366,6 +366,12 @@ static int da8xx_musb_init(struct musb *musb) musb->mregs += DA8XX_MENTOR_CORE_OFFSET; + ret = clk_prepare_enable(glue->clk); + if (ret) { + dev_err(glue->dev, "failed to enable clock\n"); + return ret; + } + /* Returns zero if e.g. not clocked */ rev = musb_readl(reg_base, DA8XX_USB_REVISION_REG); if (!rev) @@ -377,12 +383,6 @@ static int da8xx_musb_init(struct musb *musb) goto fail; } - ret = clk_prepare_enable(glue->clk); - if (ret) { - dev_err(glue->dev, "failed to enable clock\n"); - goto fail; - } - setup_timer(&otg_workaround, otg_timer, (unsigned long)musb); /* Reset the controller */ @@ -392,7 +392,7 @@ static int da8xx_musb_init(struct musb *musb) ret = phy_init(glue->phy); if (ret) { dev_err(glue->dev, "Failed to init phy.\n"); - goto err_phy_init; + goto fail; } ret = phy_power_on(glue->phy); @@ -412,9 +412,8 @@ static int da8xx_musb_init(struct musb *musb) err_phy_power_on: phy_exit(glue->phy); -err_phy_init: - clk_disable_unprepare(glue->clk); fail: + clk_disable_unprepare(glue->clk); return ret; } -- 2.7.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround 2016-10-26 10:58 [PATCH v2 0/3] usb: musb: da8xx: Fix few issues Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable() Alexandre Bailon @ 2016-10-26 10:58 ` Alexandre Bailon 2016-10-26 15:40 ` David Lechner 2016-10-26 10:58 ` [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode Alexandre Bailon 2016-10-27 17:16 ` [PATCH v2 0/3] usb: musb: da8xx: Fix few issues David Lechner 3 siblings, 1 reply; 15+ messages in thread From: Alexandre Bailon @ 2016-10-26 10:58 UTC (permalink / raw) To: khilman, david, b-liu, balbi; +Cc: linux-kernel, linux-usb, Alexandre Bailon If we configure the da8xx OTG phy in OTG mode, neither device or host mode will work. That is because the PHY is not able to detect and notify the driver that value of ID pin changed. To work despite this hardware limitation, the da8xx glue implement a workaround. But to work, the workaround require the VBUS sense and the session end comparator to enabled. Enable them if the phy is configured in OTG mode. Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c index 32ae78c..fd39292 100644 --- a/drivers/phy/phy-da8xx-usb.c +++ b/drivers/phy/phy-da8xx-usb.c @@ -93,24 +93,31 @@ static int da8xx_usb20_phy_power_off(struct phy *phy) static int da8xx_usb20_phy_set_mode(struct phy *phy, enum phy_mode mode) { struct da8xx_usb_phy *d_phy = phy_get_drvdata(phy); + int ret; u32 val; + ret = regmap_read(d_phy->regmap, CFGCHIP(2), &val); + if (ret) + return ret; + + val &= ~CFGCHIP2_OTGMODE_MASK; + switch (mode) { case PHY_MODE_USB_HOST: /* Force VBUS valid, ID = 0 */ - val = CFGCHIP2_OTGMODE_FORCE_HOST; + val |= CFGCHIP2_OTGMODE_FORCE_HOST; break; case PHY_MODE_USB_DEVICE: /* Force VBUS valid, ID = 1 */ - val = CFGCHIP2_OTGMODE_FORCE_DEVICE; + val |= CFGCHIP2_OTGMODE_FORCE_DEVICE; break; case PHY_MODE_USB_OTG: /* Don't override the VBUS/ID comparators */ - val = CFGCHIP2_OTGMODE_NO_OVERRIDE; + val |= CFGCHIP2_OTGMODE_NO_OVERRIDE | + CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN; break; default: return -EINVAL; } - regmap_write_bits(d_phy->regmap, CFGCHIP(2), CFGCHIP2_OTGMODE_MASK, - val); + regmap_write(d_phy->regmap, CFGCHIP(2), val); return 0; } -- 2.7.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround 2016-10-26 10:58 ` [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround Alexandre Bailon @ 2016-10-26 15:40 ` David Lechner 2016-10-26 16:43 ` Alexandre Bailon 0 siblings, 1 reply; 15+ messages in thread From: David Lechner @ 2016-10-26 15:40 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi Cc: linux-kernel, linux-usb, Kishon Vijay Abraham I On 10/26/2016 05:58 AM, Alexandre Bailon wrote: > If we configure the da8xx OTG phy in OTG mode, neither device or host > mode will work. That is because the PHY is not able to detect and notify > the driver that value of ID pin changed. > To work despite this hardware limitation, the da8xx glue implement a > workaround. > But to work, the workaround require the VBUS sense and the session end > comparator to enabled. > Enable them if the phy is configured in OTG mode. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c > index 32ae78c..fd39292 100644 > --- a/drivers/phy/phy-da8xx-usb.c > +++ b/drivers/phy/phy-da8xx-usb.c > @@ -93,24 +93,31 @@ static int da8xx_usb20_phy_power_off(struct phy *phy) > static int da8xx_usb20_phy_set_mode(struct phy *phy, enum phy_mode mode) > { > struct da8xx_usb_phy *d_phy = phy_get_drvdata(phy); > + int ret; > u32 val; > > + ret = regmap_read(d_phy->regmap, CFGCHIP(2), &val); > + if (ret) > + return ret; > + > + val &= ~CFGCHIP2_OTGMODE_MASK; > + > switch (mode) { > case PHY_MODE_USB_HOST: /* Force VBUS valid, ID = 0 */ > - val = CFGCHIP2_OTGMODE_FORCE_HOST; > + val |= CFGCHIP2_OTGMODE_FORCE_HOST; > break; > case PHY_MODE_USB_DEVICE: /* Force VBUS valid, ID = 1 */ > - val = CFGCHIP2_OTGMODE_FORCE_DEVICE; > + val |= CFGCHIP2_OTGMODE_FORCE_DEVICE; > break; > case PHY_MODE_USB_OTG: /* Don't override the VBUS/ID comparators */ > - val = CFGCHIP2_OTGMODE_NO_OVERRIDE; > + val |= CFGCHIP2_OTGMODE_NO_OVERRIDE | > + CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN; The AM1808 TRM indicates that CFGCHIP2_SESENDEN and CFGCHIP2_VBDTCTEN (and CFGCHIP2_DATPOL) should be on for normal operation of the USB 2.0 PHY. They do not appear to be associated with the OTG mode specifically. It seems to me that it would be more appropriate to set these in da8xx_usb20_phy_power_on() instead of here in da8xx_usb20_phy_set_mode(). > break; > default: > return -EINVAL; > } > > - regmap_write_bits(d_phy->regmap, CFGCHIP(2), CFGCHIP2_OTGMODE_MASK, > - val); > + regmap_write(d_phy->regmap, CFGCHIP(2), val); > > return 0; > } > Also cc'ing phy maintainer since this is a phy driver. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround 2016-10-26 15:40 ` David Lechner @ 2016-10-26 16:43 ` Alexandre Bailon 0 siblings, 0 replies; 15+ messages in thread From: Alexandre Bailon @ 2016-10-26 16:43 UTC (permalink / raw) To: David Lechner, khilman, b-liu, balbi Cc: linux-kernel, linux-usb, Kishon Vijay Abraham I On 10/26/2016 05:40 PM, David Lechner wrote: > On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >> If we configure the da8xx OTG phy in OTG mode, neither device or host >> mode will work. That is because the PHY is not able to detect and notify >> the driver that value of ID pin changed. >> To work despite this hardware limitation, the da8xx glue implement a >> workaround. >> But to work, the workaround require the VBUS sense and the session end >> comparator to enabled. >> Enable them if the phy is configured in OTG mode. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c >> index 32ae78c..fd39292 100644 >> --- a/drivers/phy/phy-da8xx-usb.c >> +++ b/drivers/phy/phy-da8xx-usb.c >> @@ -93,24 +93,31 @@ static int da8xx_usb20_phy_power_off(struct phy *phy) >> static int da8xx_usb20_phy_set_mode(struct phy *phy, enum phy_mode mode) >> { >> struct da8xx_usb_phy *d_phy = phy_get_drvdata(phy); >> + int ret; >> u32 val; >> >> + ret = regmap_read(d_phy->regmap, CFGCHIP(2), &val); >> + if (ret) >> + return ret; >> + >> + val &= ~CFGCHIP2_OTGMODE_MASK; >> + >> switch (mode) { >> case PHY_MODE_USB_HOST: /* Force VBUS valid, ID = 0 */ >> - val = CFGCHIP2_OTGMODE_FORCE_HOST; >> + val |= CFGCHIP2_OTGMODE_FORCE_HOST; >> break; >> case PHY_MODE_USB_DEVICE: /* Force VBUS valid, ID = 1 */ >> - val = CFGCHIP2_OTGMODE_FORCE_DEVICE; >> + val |= CFGCHIP2_OTGMODE_FORCE_DEVICE; >> break; >> case PHY_MODE_USB_OTG: /* Don't override the VBUS/ID >> comparators */ >> - val = CFGCHIP2_OTGMODE_NO_OVERRIDE; >> + val |= CFGCHIP2_OTGMODE_NO_OVERRIDE | >> + CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN; > > The AM1808 TRM indicates that CFGCHIP2_SESENDEN and CFGCHIP2_VBDTCTEN > (and CFGCHIP2_DATPOL) should be on for normal operation of the USB 2.0 > PHY. They do not appear to be associated with the OTG mode specifically. I agree but the function assigned to these bit is highly tied to OTG. And in TI BSP, there is a comment that let me think that is only needed for OTG. /* * We have to override VBUS/ID signals when MUSB is configured into the * host-only mode -- ID pin will float if no cable is connected, so the * controller won't be able to drive VBUS thinking that it's a B-device. * Otherwise, we want to use the OTG mode and enable VBUS comparators. */ cfgchip2 &= ~CFGCHIP2_OTGMODE; #ifdef CONFIG_USB_MUSB_HOST cfgchip2 |= CFGCHIP2_FORCE_HOST; #else cfgchip2 |= CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN; #endif > > It seems to me that it would be more appropriate to set these in > da8xx_usb20_phy_power_on() instead of here in da8xx_usb20_phy_set_mode(). I tried both the phy forced in device or host mode with theses bit set or clear and I haven't see any issues, so I think we could set them in da8xx_usb20_phy_power_on() as you suggested. > > >> break; >> default: >> return -EINVAL; >> } >> >> - regmap_write_bits(d_phy->regmap, CFGCHIP(2), CFGCHIP2_OTGMODE_MASK, >> - val); >> + regmap_write(d_phy->regmap, CFGCHIP(2), val); >> >> return 0; >> } >> > > Also cc'ing phy maintainer since this is a phy driver. > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode 2016-10-26 10:58 [PATCH v2 0/3] usb: musb: da8xx: Fix few issues Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable() Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround Alexandre Bailon @ 2016-10-26 10:58 ` Alexandre Bailon 2016-10-28 2:56 ` David Lechner 2016-10-27 17:16 ` [PATCH v2 0/3] usb: musb: da8xx: Fix few issues David Lechner 3 siblings, 1 reply; 15+ messages in thread From: Alexandre Bailon @ 2016-10-26 10:58 UTC (permalink / raw) To: khilman, david, b-liu, balbi; +Cc: linux-kernel, linux-usb, Alexandre Bailon 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) + return; + + /* * We poll because DaVinci's won't expose several OTG-critical * status change events (from the transceiver) otherwise. */ -- 2.7.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode 2016-10-26 10:58 ` [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode Alexandre Bailon @ 2016-10-28 2:56 ` David Lechner 2016-10-28 12:39 ` Alexandre Bailon 0 siblings, 1 reply; 15+ messages in thread From: David Lechner @ 2016-10-28 2:56 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb 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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode 2016-10-28 2:56 ` David Lechner @ 2016-10-28 12:39 ` Alexandre Bailon 2016-10-28 17:11 ` David Lechner 0 siblings, 1 reply; 15+ messages in thread From: Alexandre Bailon @ 2016-10-28 12:39 UTC (permalink / raw) To: David Lechner, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode 2016-10-28 12:39 ` Alexandre Bailon @ 2016-10-28 17:11 ` David Lechner 2016-10-29 3:11 ` Bin Liu 0 siblings, 1 reply; 15+ messages in thread From: David Lechner @ 2016-10-28 17:11 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode 2016-10-28 17:11 ` David Lechner @ 2016-10-29 3:11 ` Bin Liu 0 siblings, 0 replies; 15+ messages in thread From: Bin Liu @ 2016-10-29 3:11 UTC (permalink / raw) To: David Lechner; +Cc: Alexandre Bailon, khilman, balbi, linux-kernel, linux-usb 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues 2016-10-26 10:58 [PATCH v2 0/3] usb: musb: da8xx: Fix few issues Alexandre Bailon ` (2 preceding siblings ...) 2016-10-26 10:58 ` [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode Alexandre Bailon @ 2016-10-27 17:16 ` David Lechner 2016-10-27 18:44 ` David Lechner 3 siblings, 1 reply; 15+ messages in thread From: David Lechner @ 2016-10-27 17:16 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb On 10/26/2016 05:58 AM, Alexandre Bailon wrote: > Currently, the USB OTG of the da8xx doesn't work. > This series intend to fix them. > > Change in v2: > * Fix the error path da8xx_musb_init() > > Alexandre Bailon (3): > usb: musb: da8xx: Call earlier clk_prepare_enable() > phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround > usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode > > drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- > drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++--------- > 2 files changed, 31 insertions(+), 14 deletions(-) > I have found another problem with peripheral mode. When we force peripheral mode, the glue layer currently uses CFGCHIP2 to override the VBUS and ID. This causes it to not be able to detect disconnection because the VBUS is overridden. Here is a patch to fix the problem. I have tested this on LEGO MINDSTORMS EV3 (AM1808). This works because the ID pin is internally pulled up on the SoC, so we don't need to override it. --- diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c index 2bc12a2..33daa3b 100644 --- a/drivers/usb/musb/da8xx.c +++ b/drivers/usb/musb/da8xx.c @@ -374,9 +374,7 @@ static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode) case MUSB_HOST: /* Force VBUS valid, ID = 0 */ phy_mode = PHY_MODE_USB_HOST; break; - case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */ - phy_mode = PHY_MODE_USB_DEVICE; - break; + case MUSB_PERIPHERAL: case MUSB_OTG: /* Don't override the VBUS/ID comparators */ phy_mode = PHY_MODE_USB_OTG; break; --- If this works for other SoCs/boards, I think we should make this change. If it doesn't work, we could work around the VBUS problem by polling VBUSSENSE in CFGCHIP2. But, I like the simple solution above better. ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues 2016-10-27 17:16 ` [PATCH v2 0/3] usb: musb: da8xx: Fix few issues David Lechner @ 2016-10-27 18:44 ` David Lechner 2016-10-28 9:31 ` Alexandre Bailon 0 siblings, 1 reply; 15+ messages in thread From: David Lechner @ 2016-10-27 18:44 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb On 10/27/2016 12:16 PM, David Lechner wrote: > On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >> Currently, the USB OTG of the da8xx doesn't work. >> This series intend to fix them. >> >> Change in v2: >> * Fix the error path da8xx_musb_init() >> >> Alexandre Bailon (3): >> usb: musb: da8xx: Call earlier clk_prepare_enable() >> phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround >> usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode >> >> drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- >> drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++--------- >> 2 files changed, 31 insertions(+), 14 deletions(-) >> > > I have found another problem with peripheral mode. When we force > peripheral mode, the glue layer currently uses CFGCHIP2 to override the > VBUS and ID. This causes it to not be able to detect disconnection > because the VBUS is overridden. > > Here is a patch to fix the problem. I have tested this on LEGO > MINDSTORMS EV3 (AM1808). This works because the ID pin is internally > pulled up on the SoC, so we don't need to override it. > > --- > > diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c > index 2bc12a2..33daa3b 100644 > --- a/drivers/usb/musb/da8xx.c > +++ b/drivers/usb/musb/da8xx.c > @@ -374,9 +374,7 @@ static int da8xx_musb_set_mode(struct musb *musb, u8 > musb_mode) > case MUSB_HOST: /* Force VBUS valid, ID = 0 */ > phy_mode = PHY_MODE_USB_HOST; > break; > - case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */ > - phy_mode = PHY_MODE_USB_DEVICE; > - break; > + case MUSB_PERIPHERAL: > case MUSB_OTG: /* Don't override the VBUS/ID > comparators */ > phy_mode = PHY_MODE_USB_OTG; > break; > > --- > > If this works for other SoCs/boards, I think we should make this change. > If it doesn't work, we could work around the VBUS problem by polling > VBUSSENSE in CFGCHIP2. But, I like the simple solution above better. I have realized that due to the way my device is wired, I can actually use OTG mode and it will behave exactly as peripheral mode because the ID pin is not connected. So, maybe this patch is not needed after all. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues 2016-10-27 18:44 ` David Lechner @ 2016-10-28 9:31 ` Alexandre Bailon 2016-10-28 10:16 ` Alexandre Bailon 2016-10-28 17:50 ` David Lechner 0 siblings, 2 replies; 15+ messages in thread From: Alexandre Bailon @ 2016-10-28 9:31 UTC (permalink / raw) To: David Lechner, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb On 10/27/2016 08:44 PM, David Lechner wrote: > On 10/27/2016 12:16 PM, David Lechner wrote: >> On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >>> Currently, the USB OTG of the da8xx doesn't work. >>> This series intend to fix them. >>> >>> Change in v2: >>> * Fix the error path da8xx_musb_init() >>> >>> Alexandre Bailon (3): >>> usb: musb: da8xx: Call earlier clk_prepare_enable() >>> phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround >>> usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode >>> >>> drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- >>> drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++--------- >>> 2 files changed, 31 insertions(+), 14 deletions(-) >>> >> >> I have found another problem with peripheral mode. When we force >> peripheral mode, the glue layer currently uses CFGCHIP2 to override the >> VBUS and ID. This causes it to not be able to detect disconnection >> because the VBUS is overridden. How have you found it ? Does it cause any issues ? I mean I had to enable traces to see that disconnect was not happening. >> >> Here is a patch to fix the problem. I have tested this on LEGO >> MINDSTORMS EV3 (AM1808). This works because the ID pin is internally >> pulled up on the SoC, so we don't need to override it. Actually, I'm wonder if that if not related to VBUS sensing. May be we should set CFGCHIP2_VBDTCTEN in device mode. >> >> --- >> >> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c >> index 2bc12a2..33daa3b 100644 >> --- a/drivers/usb/musb/da8xx.c >> +++ b/drivers/usb/musb/da8xx.c >> @@ -374,9 +374,7 @@ static int da8xx_musb_set_mode(struct musb *musb, u8 >> musb_mode) >> case MUSB_HOST: /* Force VBUS valid, ID = 0 */ >> phy_mode = PHY_MODE_USB_HOST; >> break; >> - case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */ >> - phy_mode = PHY_MODE_USB_DEVICE; >> - break; >> + case MUSB_PERIPHERAL: >> case MUSB_OTG: /* Don't override the VBUS/ID >> comparators */ >> phy_mode = PHY_MODE_USB_OTG; >> break; >> >> --- >> >> If this works for other SoCs/boards, I think we should make this change. >> If it doesn't work, we could work around the VBUS problem by polling >> VBUSSENSE in CFGCHIP2. But, I like the simple solution above better. > > I have realized that due to the way my device is wired, I can actually > use OTG mode and it will behave exactly as peripheral mode because the > ID pin is not connected. So, maybe this patch is not needed after all. > Actually, I'm sure that is related to ID I have the same issue except the disconnect is called when I use the OTG mode. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues 2016-10-28 9:31 ` Alexandre Bailon @ 2016-10-28 10:16 ` Alexandre Bailon 2016-10-28 17:50 ` David Lechner 1 sibling, 0 replies; 15+ messages in thread From: Alexandre Bailon @ 2016-10-28 10:16 UTC (permalink / raw) To: David Lechner, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb On 10/28/2016 11:31 AM, Alexandre Bailon wrote: > On 10/27/2016 08:44 PM, David Lechner wrote: >> On 10/27/2016 12:16 PM, David Lechner wrote: >>> On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >>>> Currently, the USB OTG of the da8xx doesn't work. >>>> This series intend to fix them. >>>> >>>> Change in v2: >>>> * Fix the error path da8xx_musb_init() >>>> >>>> Alexandre Bailon (3): >>>> usb: musb: da8xx: Call earlier clk_prepare_enable() >>>> phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround >>>> usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode >>>> >>>> drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- >>>> drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++--------- >>>> 2 files changed, 31 insertions(+), 14 deletions(-) >>>> >>> >>> I have found another problem with peripheral mode. When we force >>> peripheral mode, the glue layer currently uses CFGCHIP2 to override the >>> VBUS and ID. This causes it to not be able to detect disconnection >>> because the VBUS is overridden. > How have you found it ? Does it cause any issues ? > I mean I had to enable traces to see that disconnect was not happening. >>> >>> Here is a patch to fix the problem. I have tested this on LEGO >>> MINDSTORMS EV3 (AM1808). This works because the ID pin is internally >>> pulled up on the SoC, so we don't need to override it. > Actually, I'm wonder if that if not related to VBUS sensing. > May be we should set CFGCHIP2_VBDTCTEN in device mode. >>> >>> --- >>> >>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c >>> index 2bc12a2..33daa3b 100644 >>> --- a/drivers/usb/musb/da8xx.c >>> +++ b/drivers/usb/musb/da8xx.c >>> @@ -374,9 +374,7 @@ static int da8xx_musb_set_mode(struct musb *musb, u8 >>> musb_mode) >>> case MUSB_HOST: /* Force VBUS valid, ID = 0 */ >>> phy_mode = PHY_MODE_USB_HOST; >>> break; >>> - case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */ >>> - phy_mode = PHY_MODE_USB_DEVICE; >>> - break; >>> + case MUSB_PERIPHERAL: >>> case MUSB_OTG: /* Don't override the VBUS/ID >>> comparators */ >>> phy_mode = PHY_MODE_USB_OTG; >>> break; >>> >>> --- >>> >>> If this works for other SoCs/boards, I think we should make this change. I will try it but for me, it should be good. It is actually similar to what is done in TI BSP. >>> If it doesn't work, we could work around the VBUS problem by polling >>> VBUSSENSE in CFGCHIP2. But, I like the simple solution above better. Actually, I don't think we need to do any polling. We can just catch the suspend interrupt and use VBUSSENSE to differentiate a real suspend from a disconnect. >> >> I have realized that due to the way my device is wired, I can actually >> use OTG mode and it will behave exactly as peripheral mode because the >> ID pin is not connected. So, maybe this patch is not needed after all. >> > Actually, I'm sure that is related to ID > I have the same issue except the disconnect is called when I use the > OTG mode. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues 2016-10-28 9:31 ` Alexandre Bailon 2016-10-28 10:16 ` Alexandre Bailon @ 2016-10-28 17:50 ` David Lechner 1 sibling, 0 replies; 15+ messages in thread From: David Lechner @ 2016-10-28 17:50 UTC (permalink / raw) To: Alexandre Bailon, khilman, b-liu, balbi; +Cc: linux-kernel, linux-usb On 10/28/2016 04:31 AM, Alexandre Bailon wrote: > On 10/27/2016 08:44 PM, David Lechner wrote: >> On 10/27/2016 12:16 PM, David Lechner wrote: >>> On 10/26/2016 05:58 AM, Alexandre Bailon wrote: >>>> Currently, the USB OTG of the da8xx doesn't work. >>>> This series intend to fix them. >>>> >>>> Change in v2: >>>> * Fix the error path da8xx_musb_init() >>>> >>>> Alexandre Bailon (3): >>>> usb: musb: da8xx: Call earlier clk_prepare_enable() >>>> phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround >>>> usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode >>>> >>>> drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++----- >>>> drivers/usb/musb/da8xx.c | 28 +++++++++++++++++++--------- >>>> 2 files changed, 31 insertions(+), 14 deletions(-) >>>> >>> >>> I have found another problem with peripheral mode. When we force >>> peripheral mode, the glue layer currently uses CFGCHIP2 to override the >>> VBUS and ID. This causes it to not be able to detect disconnection >>> because the VBUS is overridden. > How have you found it ? Does it cause any issues ? > I mean I had to enable traces to see that disconnect was not happening. I am also using the device tree patch series. I specified dr_mode = "peripheral" in my device tree because the device is only wired for use as a peripheral port. I have actually known about this issue for a long time. See <https://github.com/ev3dev/ev3dev/issues/244> and <http://e2e.ti.com/support/embedded/linux/f/354/p/65332/237094#2370942> >>> >>> Here is a patch to fix the problem. I have tested this on LEGO >>> MINDSTORMS EV3 (AM1808). This works because the ID pin is internally >>> pulled up on the SoC, so we don't need to override it. > Actually, I'm wonder if that if not related to VBUS sensing. > May be we should set CFGCHIP2_VBDTCTEN in device mode. Yes, as we discussed in another thread, we should set CFGCHIP2_VBDTCTEN and CFGCHIP2_SESNDEN *always* regardless of mode. I still have the problem described above with these two enabled if and only if we are also setting CFGCHIP2_OTG_FORCE_PERIPHERAL. >>> >>> --- >>> >>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c >>> index 2bc12a2..33daa3b 100644 >>> --- a/drivers/usb/musb/da8xx.c >>> +++ b/drivers/usb/musb/da8xx.c >>> @@ -374,9 +374,7 @@ static int da8xx_musb_set_mode(struct musb *musb, u8 >>> musb_mode) >>> case MUSB_HOST: /* Force VBUS valid, ID = 0 */ >>> phy_mode = PHY_MODE_USB_HOST; >>> break; >>> - case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */ >>> - phy_mode = PHY_MODE_USB_DEVICE; >>> - break; >>> + case MUSB_PERIPHERAL: >>> case MUSB_OTG: /* Don't override the VBUS/ID >>> comparators */ >>> phy_mode = PHY_MODE_USB_OTG; >>> break; >>> >>> --- >>> >>> If this works for other SoCs/boards, I think we should make this change. >>> If it doesn't work, we could work around the VBUS problem by polling >>> VBUSSENSE in CFGCHIP2. But, I like the simple solution above better. >> >> I have realized that due to the way my device is wired, I can actually >> use OTG mode and it will behave exactly as peripheral mode because the >> ID pin is not connected. So, maybe this patch is not needed after all. >> > Actually, I'm sure that is related to ID The ID pin on my device is not connected, so I don't think this has to do with ID. It is always high (internally pulled up). > I have the same issue except the disconnect is called when I use the > OTG mode. > This is exactly the behavior I am seeing. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-10-29 3:12 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-26 10:58 [PATCH v2 0/3] usb: musb: da8xx: Fix few issues Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable() Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround Alexandre Bailon 2016-10-26 15:40 ` David Lechner 2016-10-26 16:43 ` Alexandre Bailon 2016-10-26 10:58 ` [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode Alexandre Bailon 2016-10-28 2:56 ` David Lechner 2016-10-28 12:39 ` Alexandre Bailon 2016-10-28 17:11 ` David Lechner 2016-10-29 3:11 ` Bin Liu 2016-10-27 17:16 ` [PATCH v2 0/3] usb: musb: da8xx: Fix few issues David Lechner 2016-10-27 18:44 ` David Lechner 2016-10-28 9:31 ` Alexandre Bailon 2016-10-28 10:16 ` Alexandre Bailon 2016-10-28 17:50 ` David Lechner
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.