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

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

Alexandre Bailon (3):
  usb: musb: da8xx: Only execute the OTG workaround when phy in OTG mode
  usb: musb: da8xx: Fixup the OTG workaround
  usb: musb: da8xx: Call earlier clk_prepare_enable()

 drivers/phy/phy-da8xx-usb.c | 17 ++++++++++++-----
 drivers/usb/musb/da8xx.c    | 23 +++++++++++++++++------
 2 files changed, 29 insertions(+), 11 deletions(-)

-- 
2.7.3

[PATCH 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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 210b7e4..ec93602 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");
+		goto fail;
+	}
+
 	/* 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 */
-- 
2.7.3

[PATCH 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 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 ec93602..be10c51 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 1/3] usb: musb: da8xx: Call earlier clk_prepare_enable()

[David Lechner]

On 10/25/2016 08:52 AM, Alexandre Bailon wrote:
> 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 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index 210b7e4..ec93602 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");
> +		goto fail;

You can simply return ret; here instead of goto fail;

> +	}
> +
>  	/* 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;

The unwinding on error needs to be updated so that 
clk_disable_unprepare() is called at this point and subsequently. 
Currently, all goto fail; will return without disabling the clock.

>  	}
>
> -	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 */
>

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

[David Lechner]

On 10/25/2016 08:52 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;

Do we need to clear CFGCHIP2_SESENDEN and/or CFGCHIP2_VBDTCTEN in case 
PHY_MODE_USB_HOST: or case PHY_MODE_USB_DEVICE:? Or are they ignored in 
those modes?

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

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

[Alexandre Bailon]

On 10/25/2016 09:16 PM, David Lechner wrote:
> On 10/25/2016 08:52 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;
> 
> Do we need to clear CFGCHIP2_SESENDEN and/or CFGCHIP2_VBDTCTEN in case
> PHY_MODE_USB_HOST: or case PHY_MODE_USB_DEVICE:? Or are they ignored in
> those modes?
Actually, I don't know. The datasheet is quite confusing.
The "USB PHY Initialization" section seems to speak only about OTG mode.
But my observation let me think they are ignored when the phy is forced
in device or host 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;
>>  }
>>
>