[PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[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

[PATCH v2 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable()

[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

[PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround

[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

[PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[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

Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround

[David Lechner]

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.

Re: [PATCH v2 2/3] phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround

[Alexandre Bailon]

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.
> 
> 

Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[David Lechner]

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.

Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[David Lechner]

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.

Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[David Lechner]

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

Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[Alexandre Bailon]

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.

Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[Alexandre Bailon]

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.
> 

Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[Alexandre Bailon]

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.

Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[David Lechner]

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.

Re: [PATCH v2 0/3] usb: musb: da8xx: Fix few issues

[David Lechner]

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.

Re: [PATCH v2 3/3] usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode

[Bin Liu]

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.