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

[PATCH v3 0/5] 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()

Changes in v3:
* Remove the host workaround that was not working on every platform
* Add a property to the devicetree node of phy to force the phy in a specific
  mode (host or device).
* Only use dr_mode to configure the controller mode and let the phy in OTG mode.
  The main goal of this change is to prevent the phy to be set in host or
  device mode because these modes have some issues.

Alexandre Bailon (5):
  usb: musb: da8xx: Call earlier clk_prepare_enable()
  phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround
  dt/bindings: Add a new property to DA8xx USB PHY
  phy: da8xx-usb: Use usb20_force_mode property to configure the phy
    mode
  usb: musb: da8xx: Remove set_mode callback

 .../devicetree/bindings/phy/phy-da8xx-usb.txt      |  5 +++
 drivers/phy/phy-da8xx-usb.c                        | 28 ++++++++++++---
 drivers/usb/musb/da8xx.c                           | 40 +++++-----------------
 3 files changed, 36 insertions(+), 37 deletions(-)

-- 
2.7.3

[PATCH v3 1/5] 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 v3 2/5] 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 v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Alexandre Bailon]

The USB PHY is able to operate in OTG, host or peripheral.
Some board may be wired to work act only as host or peripheral.
In such case, the dr_mode property of controller must be set to
host or peripheral. But doing that will also configure the PHY
in host or peripheral mode whereas OTG is able to detect which
role the USB controller should take.
The PHY's host or peripheral mode are actually only useful when
hardware doesn't allow OTG to detect it's role.

Add the usb20_force_mode property to force the PHY to operate
in host or peripheral mode.
When usb20_force_mode is used, dr_mode should also be configured
to host or peripheral.
The controller uses dr_mode to configure itself, but the phy use
it to get the mode to use to configure the PHY mode.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
index c26478b..9fc87fb 100644
--- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
+++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
@@ -4,6 +4,11 @@ Required properties:
  - compatible: must be "ti,da830-usb-phy".
  - #phy-cells: must be 1.
 
+Optional properties:
+- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
+		    It should only be defined if the hardware is not capable correctly
+		    detect the role of USB by using VBUS and ID pin.
+
 This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
 controllers on DA8xx SoCs. Consumers of this device should use index 0 for
 the USB 2.0 phy device and index 1 for the USB 1.1 phy device.
-- 
2.7.3

[PATCH v3 4/5] phy: da8xx-usb: Use usb20_force_mode property to configure the phy mode

[Alexandre Bailon]

The USB PHY is able to operate in OTG, host or peripheral.
Some board may be wired to work act only as host or peripheral.
In such case, the dr_mode property of controller must be set to
host or peripheral. But doing that will also configure the PHY
in host or peripheral mode whereas OTG is able to detect which
role the USB controller should take.
The PHY's host or peripheral mode are actually only useful when
hardware doesn't allow OTG to detect it's role.

There is some issues when the PHY is forced in peripheral or host mode,
and it worth to let the PHY in OTG when that is possible.
Only force the PHY in a specific mode if usb20_force_mode is set in DT.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/phy/phy-da8xx-usb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
index fd39292..bf1b3af 100644
--- a/drivers/phy/phy-da8xx-usb.c
+++ b/drivers/phy/phy-da8xx-usb.c
@@ -22,6 +22,7 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/usb/of.h>
 
 struct da8xx_usb_phy {
 	struct phy_provider	*phy_provider;
@@ -109,6 +110,7 @@ static int da8xx_usb20_phy_set_mode(struct phy *phy, enum phy_mode mode)
 	case PHY_MODE_USB_DEVICE:	/* Force VBUS valid, ID = 1 */
 		val |= CFGCHIP2_OTGMODE_FORCE_DEVICE;
 		break;
+	case PHY_MODE_INVALID:
 	case PHY_MODE_USB_OTG:	/* Don't override the VBUS/ID comparators */
 		val |= CFGCHIP2_OTGMODE_NO_OVERRIDE |
 			CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN;
@@ -152,6 +154,7 @@ static int da8xx_usb_phy_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	struct device_node	*node = dev->of_node;
 	struct da8xx_usb_phy	*d_phy;
+	enum usb_dr_mode	dr_mode = PHY_MODE_INVALID;
 
 	d_phy = devm_kzalloc(dev, sizeof(*d_phy), GFP_KERNEL);
 	if (!d_phy)
@@ -202,6 +205,13 @@ static int da8xx_usb_phy_probe(struct platform_device *pdev)
 			dev_err(dev, "Failed to create phy provider\n");
 			return PTR_ERR(d_phy->phy_provider);
 		}
+
+		if (of_find_property(node, "usb20-force-mode", NULL)) {
+			dr_mode = of_usb_get_dr_mode_by_phy(node, 0);
+			if (dr_mode == USB_DR_MODE_UNKNOWN ||
+				dr_mode == USB_DR_MODE_OTG)
+				dev_warn(dev, "dr_mode is not properly set\n");
+		}
 	} else {
 		int ret;
 
@@ -213,6 +223,7 @@ static int da8xx_usb_phy_probe(struct platform_device *pdev)
 		if (ret)
 			dev_warn(dev, "Failed to create usb20 phy lookup\n");
 	}
+	da8xx_usb20_phy_set_mode(d_phy->usb20_phy, dr_mode);
 
 	return 0;
 }
-- 
2.7.3

[PATCH v3 5/5] usb: musb: da8xx: Remove set_mode callback

[Alexandre Bailon]

The USB PHY is able to operate in OTG, host or peripheral.
Some board may be wired to work act only as host or peripheral.
In such case, the dr_mode property of controller must be set to
host or peripheral. But doing that will also configure the PHY
in host or peripheral mode whereas OTG is able to detect which
role the USB controller should take.
The PHY's host or peripheral mode are actually only useful when
hardware doesn't allow OTG to detect it's role.

The set_mode callback is used by the musb driver to set mode
of the PHY. But in the case of DA8xx, the PHY have some issues.
The OTG mode work correctly but the host and peripheral don't.
In host mode, the PHY stops to work after the first disconnect.
In device mode, the PHY doesn't detect any disconnect.
As the OTG mode is working properly, let the PHY in OTG mode,
whatever is the controller mode.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/usb/musb/da8xx.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 6749aa1..581f830 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -335,28 +335,6 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
 	return ret;
 }
 
-static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
-{
-	struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
-	enum phy_mode phy_mode;
-
-	switch (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_OTG:		/* Don't override the VBUS/ID comparators */
-		phy_mode = PHY_MODE_USB_OTG;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return phy_set_mode(glue->phy, phy_mode);
-}
-
 static int da8xx_musb_init(struct musb *musb)
 {
 	struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
@@ -445,7 +423,6 @@ static const struct musb_platform_ops da8xx_ops = {
 	.enable		= da8xx_musb_enable,
 	.disable	= da8xx_musb_disable,
 
-	.set_mode	= da8xx_musb_set_mode,
 	.try_idle	= da8xx_musb_try_idle,
 
 	.set_vbus	= da8xx_musb_set_vbus,
-- 
2.7.3

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 03 November 2016 08:56 PM, Alexandre Bailon wrote:
> The USB PHY is able to operate in OTG, host or peripheral.
> Some board may be wired to work act only as host or peripheral.
> In such case, the dr_mode property of controller must be set to
> host or peripheral. But doing that will also configure the PHY
> in host or peripheral mode whereas OTG is able to detect which
> role the USB controller should take.
> The PHY's host or peripheral mode are actually only useful when
> hardware doesn't allow OTG to detect it's role.
> 
> Add the usb20_force_mode property to force the PHY to operate
> in host or peripheral mode.

I think we do just that if we populate dr_mode with host or peripheral. Why do
we need another property to control dr_mode property?
> When usb20_force_mode is used, dr_mode should also be configured
> to host or peripheral.
> The controller uses dr_mode to configure itself, but the phy use
> it to get the mode to use to configure the PHY mode.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> index c26478b..9fc87fb 100644
> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> @@ -4,6 +4,11 @@ Required properties:
>   - compatible: must be "ti,da830-usb-phy".
>   - #phy-cells: must be 1.
>  
> +Optional properties:
> +- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
> +		    It should only be defined if the hardware is not capable correctly
> +		    detect the role of USB by using VBUS and ID pin.

>From what I understand from the previous patch, if VBUS sense and the session
end comparator is enabled, the controller can work in host mode or device mode.

Thanks
Kishon

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[David Lechner]

On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
> The USB PHY is able to operate in OTG, host or peripheral.
> Some board may be wired to work act only as host or peripheral.
> In such case, the dr_mode property of controller must be set to
> host or peripheral. But doing that will also configure the PHY
> in host or peripheral mode whereas OTG is able to detect which
> role the USB controller should take.
> The PHY's host or peripheral mode are actually only useful when
> hardware doesn't allow OTG to detect it's role.
>
> Add the usb20_force_mode property to force the PHY to operate
> in host or peripheral mode.

Device tree describes the hardware, not the configuration, so this is 
not acceptable.

Besides, this setting should not be fixed to one value anyway.

> When usb20_force_mode is used, dr_mode should also be configured
> to host or peripheral.
> The controller uses dr_mode to configure itself, but the phy use
> it to get the mode to use to configure the PHY mode.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> index c26478b..9fc87fb 100644
> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> @@ -4,6 +4,11 @@ Required properties:
>   - compatible: must be "ti,da830-usb-phy".
>   - #phy-cells: must be 1.
>
> +Optional properties:
> +- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
> +		    It should only be defined if the hardware is not capable correctly
> +		    detect the role of USB by using VBUS and ID pin.
> +
>  This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
>  controllers on DA8xx SoCs. Consumers of this device should use index 0 for
>  the USB 2.0 phy device and index 1 for the USB 1.1 phy device.
>

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

[David Lechner]

On 11/03/2016 10:26 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;

I still think this is wrong that you set these bits but never clear 
them. For example, if you start in host mode, these bits will not bit 
set, but if you start in otg mode and switch to host mode, these bits 
will still be set.

And I still think that these bits (CFGCHIP2_SESENDEN | 
CFGCHIP2_VBDTCTEN) should just be enabled during driver probe rather 
than here since I don't know of a reason to turn them off.

>  		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 v3 5/5] usb: musb: da8xx: Remove set_mode callback

[David Lechner]

On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
> The USB PHY is able to operate in OTG, host or peripheral.
> Some board may be wired to work act only as host or peripheral.
> In such case, the dr_mode property of controller must be set to
> host or peripheral. But doing that will also configure the PHY
> in host or peripheral mode whereas OTG is able to detect which
> role the USB controller should take.
> The PHY's host or peripheral mode are actually only useful when
> hardware doesn't allow OTG to detect it's role.
>
> The set_mode callback is used by the musb driver to set mode
> of the PHY. But in the case of DA8xx, the PHY have some issues.
> The OTG mode work correctly but the host and peripheral don't.
> In host mode, the PHY stops to work after the first disconnect.
> In device mode, the PHY doesn't detect any disconnect.
> As the OTG mode is working properly, let the PHY in OTG mode,
> whatever is the controller mode.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/usb/musb/da8xx.c | 23 -----------------------
>  1 file changed, 23 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index 6749aa1..581f830 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -335,28 +335,6 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
>  	return ret;
>  }
>
> -static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)

Bin suggested using some sort of quirks flag. So instead of removing 
this callback, I think this is where to incorporate the quirks flags.

I suppose the quirks could be kernel config options. Perhaps someone 
else has a better idea?

> -{
> -	struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
> -	enum phy_mode phy_mode;
> -
> -	switch (musb_mode) {
> -	case MUSB_HOST:		/* Force VBUS valid, ID = 0 */
> -		phy_mode = PHY_MODE_USB_HOST;

this should be something like...

		phy_mode = force_host_mode_quirk ? PHY_MODE_USB_HOST
					 	 : PHY_MODE_USB_OTG;


> -		break;
> -	case MUSB_PERIPHERAL:	/* Force VBUS valid, ID = 1 */
> -		phy_mode = PHY_MODE_USB_DEVICE;

and...

		phy_mode = force_peripheral_mode_quirk ?
				PHY_MODE_USB_DEVICE : PHY_MODE_USB_OTG;


> -		break;
> -	case MUSB_OTG:		/* Don't override the VBUS/ID comparators */
> -		phy_mode = PHY_MODE_USB_OTG;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	return phy_set_mode(glue->phy, phy_mode);
> -}
> -
>  static int da8xx_musb_init(struct musb *musb)
>  {
>  	struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
> @@ -445,7 +423,6 @@ static const struct musb_platform_ops da8xx_ops = {
>  	.enable		= da8xx_musb_enable,
>  	.disable	= da8xx_musb_disable,
>
> -	.set_mode	= da8xx_musb_set_mode,
>  	.try_idle	= da8xx_musb_try_idle,
>
>  	.set_vbus	= da8xx_musb_set_vbus,
>

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Alexandre Bailon]

On 11/03/2016 05:34 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 03 November 2016 08:56 PM, Alexandre Bailon wrote:
>> The USB PHY is able to operate in OTG, host or peripheral.
>> Some board may be wired to work act only as host or peripheral.
>> In such case, the dr_mode property of controller must be set to
>> host or peripheral. But doing that will also configure the PHY
>> in host or peripheral mode whereas OTG is able to detect which
>> role the USB controller should take.
>> The PHY's host or peripheral mode are actually only useful when
>> hardware doesn't allow OTG to detect it's role.
>>
>> Add the usb20_force_mode property to force the PHY to operate
>> in host or peripheral mode.
> 
> I think we do just that if we populate dr_mode with host or peripheral. Why do
> we need another property to control dr_mode property?
Because the phy doesn't work correctly when it is in host or
device mode.
>> When usb20_force_mode is used, dr_mode should also be configured
>> to host or peripheral.
>> The controller uses dr_mode to configure itself, but the phy use
>> it to get the mode to use to configure the PHY mode.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> index c26478b..9fc87fb 100644
>> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> @@ -4,6 +4,11 @@ Required properties:
>>   - compatible: must be "ti,da830-usb-phy".
>>   - #phy-cells: must be 1.
>>  
>> +Optional properties:
>> +- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
>> +		    It should only be defined if the hardware is not capable correctly
>> +		    detect the role of USB by using VBUS and ID pin.
> 
> From what I understand from the previous patch, if VBUS sense and the session
> end comparator is enabled, the controller can work in host mode or device mode.
I but VBUS sense and and session end comparator only seems to work when
the phy is in otg mode.
In host mode, the phy stop to work after the first disconnect.
In device mode, the phy never detect a disconnect.
In otg mode, these issues go away.
I'm working on workaround for both of them but I think it is
better to keep the phy in otg when it is possible.
> 
> Thanks
> Kishon
> 
Thanks,
Alexandre

Re: [PATCH v3 5/5] usb: musb: da8xx: Remove set_mode callback

[Bin Liu]

On Thu, Nov 03, 2016 at 12:18:53PM -0500, David Lechner wrote:
> On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
> >The USB PHY is able to operate in OTG, host or peripheral.
> >Some board may be wired to work act only as host or peripheral.
> >In such case, the dr_mode property of controller must be set to
> >host or peripheral. But doing that will also configure the PHY
> >in host or peripheral mode whereas OTG is able to detect which
> >role the USB controller should take.
> >The PHY's host or peripheral mode are actually only useful when
> >hardware doesn't allow OTG to detect it's role.
> >
> >The set_mode callback is used by the musb driver to set mode
> >of the PHY. But in the case of DA8xx, the PHY have some issues.
> >The OTG mode work correctly but the host and peripheral don't.
> >In host mode, the PHY stops to work after the first disconnect.
> >In device mode, the PHY doesn't detect any disconnect.
> >As the OTG mode is working properly, let the PHY in OTG mode,
> >whatever is the controller mode.
> >
> >Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> >---
> > drivers/usb/musb/da8xx.c | 23 -----------------------
> > 1 file changed, 23 deletions(-)
> >
> >diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> >index 6749aa1..581f830 100644
> >--- a/drivers/usb/musb/da8xx.c
> >+++ b/drivers/usb/musb/da8xx.c
> >@@ -335,28 +335,6 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
> > 	return ret;
> > }
> >
> >-static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
> 
> Bin suggested using some sort of quirks flag. So instead of removing
> this callback, I think this is where to incorporate the quirks
> flags.
> 
> I suppose the quirks could be kernel config options. Perhaps someone
> else has a better idea?

I didn't closely follow this thread, but I was thinking about to reuse
musb->io.quirks, and define the quirks in device tree...

I am debugging an issue in dsps, and might need a quirk for the
solution...

Regards,
-Bin.

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Alexandre Bailon]

On 11/03/2016 05:53 PM, David Lechner wrote:
> On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
>> The USB PHY is able to operate in OTG, host or peripheral.
>> Some board may be wired to work act only as host or peripheral.
>> In such case, the dr_mode property of controller must be set to
>> host or peripheral. But doing that will also configure the PHY
>> in host or peripheral mode whereas OTG is able to detect which
>> role the USB controller should take.
>> The PHY's host or peripheral mode are actually only useful when
>> hardware doesn't allow OTG to detect it's role.
>>
>> Add the usb20_force_mode property to force the PHY to operate
>> in host or peripheral mode.
> 
> Device tree describes the hardware, not the configuration, so this is
> not acceptable.
I think that is really hardware (and board) dependent.
We will only need to set it for unusual hardware that doesn't
let the otg phy automatically find the it's role.
What do you think I should do?
> 
> Besides, this setting should not be fixed to one value anyway.
Actually, a bool is enough. If we need to force the phy in a specific
mode, we can reuse dr_mode of controller.
> 
>> When usb20_force_mode is used, dr_mode should also be configured
>> to host or peripheral.
>> The controller uses dr_mode to configure itself, but the phy use
>> it to get the mode to use to configure the PHY mode.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> index c26478b..9fc87fb 100644
>> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> @@ -4,6 +4,11 @@ Required properties:
>>   - compatible: must be "ti,da830-usb-phy".
>>   - #phy-cells: must be 1.
>>
>> +Optional properties:
>> +- usb20-force-mode: Force the phy to operate in same mode than the
>> USB OTG controller.
>> +            It should only be defined if the hardware is not capable
>> correctly
>> +            detect the role of USB by using VBUS and ID pin.
>> +
>>  This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
>>  controllers on DA8xx SoCs. Consumers of this device should use index
>> 0 for
>>  the USB 2.0 phy device and index 1 for the USB 1.1 phy device.
>>
> 

Re: [PATCH v3 5/5] usb: musb: da8xx: Remove set_mode callback

[Alexandre Bailon]

On 11/03/2016 06:27 PM, Bin Liu wrote:
> On Thu, Nov 03, 2016 at 12:18:53PM -0500, David Lechner wrote:
>> On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
>>> The USB PHY is able to operate in OTG, host or peripheral.
>>> Some board may be wired to work act only as host or peripheral.
>>> In such case, the dr_mode property of controller must be set to
>>> host or peripheral. But doing that will also configure the PHY
>>> in host or peripheral mode whereas OTG is able to detect which
>>> role the USB controller should take.
>>> The PHY's host or peripheral mode are actually only useful when
>>> hardware doesn't allow OTG to detect it's role.
>>>
>>> The set_mode callback is used by the musb driver to set mode
>>> of the PHY. But in the case of DA8xx, the PHY have some issues.
>>> The OTG mode work correctly but the host and peripheral don't.
>>> In host mode, the PHY stops to work after the first disconnect.
>>> In device mode, the PHY doesn't detect any disconnect.
>>> As the OTG mode is working properly, let the PHY in OTG mode,
>>> whatever is the controller mode.
>>>
>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>>> ---
>>> drivers/usb/musb/da8xx.c | 23 -----------------------
>>> 1 file changed, 23 deletions(-)
>>>
>>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
>>> index 6749aa1..581f830 100644
>>> --- a/drivers/usb/musb/da8xx.c
>>> +++ b/drivers/usb/musb/da8xx.c
>>> @@ -335,28 +335,6 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
>>> 	return ret;
>>> }
>>>
>>> -static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
>>
>> Bin suggested using some sort of quirks flag. So instead of removing
>> this callback, I think this is where to incorporate the quirks
>> flags.
>>
>> I suppose the quirks could be kernel config options. Perhaps someone
>> else has a better idea?
> 
> I didn't closely follow this thread, but I was thinking about to reuse
> musb->io.quirks, and define the quirks in device tree...
I have understood that.
It seems to be a better solution than what I did.
> 
> I am debugging an issue in dsps, and might need a quirk for the
> solution...
> 
> Regards,
> -Bin.
> 
Thanks,
Alexandre

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 03 November 2016 10:56 PM, Alexandre Bailon wrote:
> On 11/03/2016 05:34 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 03 November 2016 08:56 PM, Alexandre Bailon wrote:
>>> The USB PHY is able to operate in OTG, host or peripheral.
>>> Some board may be wired to work act only as host or peripheral.
>>> In such case, the dr_mode property of controller must be set to
>>> host or peripheral. But doing that will also configure the PHY
>>> in host or peripheral mode whereas OTG is able to detect which
>>> role the USB controller should take.
>>> The PHY's host or peripheral mode are actually only useful when
>>> hardware doesn't allow OTG to detect it's role.
>>>
>>> Add the usb20_force_mode property to force the PHY to operate
>>> in host or peripheral mode.
>>
>> I think we do just that if we populate dr_mode with host or peripheral. Why do
>> we need another property to control dr_mode property?
> Because the phy doesn't work correctly when it is in host or
> device mode.

That would be the same even with usb20_force_mode property. How does
usb20_force_mode property help?

Thanks
Kishon

>>> When usb20_force_mode is used, dr_mode should also be configured
>>> to host or peripheral.
>>> The controller uses dr_mode to configure itself, but the phy use
>>> it to get the mode to use to configure the PHY mode.
>>>
>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>>> ---
>>>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> index c26478b..9fc87fb 100644
>>> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> @@ -4,6 +4,11 @@ Required properties:
>>>   - compatible: must be "ti,da830-usb-phy".
>>>   - #phy-cells: must be 1.
>>>  
>>> +Optional properties:
>>> +- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
>>> +		    It should only be defined if the hardware is not capable correctly
>>> +		    detect the role of USB by using VBUS and ID pin.
>>
>> From what I understand from the previous patch, if VBUS sense and the session
>> end comparator is enabled, the controller can work in host mode or device mode.
> I but VBUS sense and and session end comparator only seems to work when
> the phy is in otg mode.
> In host mode, the phy stop to work after the first disconnect.
> In device mode, the phy never detect a disconnect.
> In otg mode, these issues go away.
> I'm working on workaround for both of them but I think it is
> better to keep the phy in otg when it is possible.
>>
>> Thanks
>> Kishon
>>
> Thanks,
> Alexandre
> 

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[David Lechner]

On 11/03/2016 12:33 PM, Alexandre Bailon wrote:
> On 11/03/2016 05:53 PM, David Lechner wrote:
>> On 11/03/2016 10:26 AM, Alexandre Bailon wrote:
>>> The USB PHY is able to operate in OTG, host or peripheral.
>>> Some board may be wired to work act only as host or peripheral.
>>> In such case, the dr_mode property of controller must be set to
>>> host or peripheral. But doing that will also configure the PHY
>>> in host or peripheral mode whereas OTG is able to detect which
>>> role the USB controller should take.
>>> The PHY's host or peripheral mode are actually only useful when
>>> hardware doesn't allow OTG to detect it's role.
>>>
>>> Add the usb20_force_mode property to force the PHY to operate
>>> in host or peripheral mode.
>>
>> Device tree describes the hardware, not the configuration, so this is
>> not acceptable.
> I think that is really hardware (and board) dependent.
> We will only need to set it for unusual hardware that doesn't
> let the otg phy automatically find the it's role.
> What do you think I should do?

I am staring to think that maybe it was a mistake for me to add the mode 
setting to the phy driver. It is not very clear to me where the division 
is between what is the responsibility of the phy and what is the 
responsibility of the musb.

Looking at the bigger picture, forcing the PHY mode and never changing 
it is not useful. It is the transition from not forced to forced or vice 
versa that is important because it is at that moment that interrupts are 
triggered.

The forcing of a PHY mode is really being used to simulate the plugging 
or unplugging of a USB device. And the only reason you would need to do 
this is that the hardware lacks the proper circuitry between the USB 
connector and the SoC in order to detect such an event.

So, what I think you should do is try to get MUSB working with the PHY 
in OTG mode without worrying about forcing other PHY modes. Then once 
that has been sorted out, we can talk about how to handle quirks for 
hardware that needs it.

>>
>> Besides, this setting should not be fixed to one value anyway.
> Actually, a bool is enough. If we need to force the phy in a specific
> mode, we can reuse dr_mode of controller.
>>
>>> When usb20_force_mode is used, dr_mode should also be configured
>>> to host or peripheral.
>>> The controller uses dr_mode to configure itself, but the phy use
>>> it to get the mode to use to configure the PHY mode.
>>>
>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>>> ---
>>>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> index c26478b..9fc87fb 100644
>>> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>> @@ -4,6 +4,11 @@ Required properties:
>>>   - compatible: must be "ti,da830-usb-phy".
>>>   - #phy-cells: must be 1.
>>>
>>> +Optional properties:
>>> +- usb20-force-mode: Force the phy to operate in same mode than the
>>> USB OTG controller.
>>> +            It should only be defined if the hardware is not capable
>>> correctly
>>> +            detect the role of USB by using VBUS and ID pin.
>>> +
>>>  This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
>>>  controllers on DA8xx SoCs. Consumers of this device should use index
>>> 0 for
>>>  the USB 2.0 phy device and index 1 for the USB 1.1 phy device.
>>>
>>
>

Re: [PATCH v3 3/5] dt/bindings: Add a new property to DA8xx USB PHY

[Alexandre Bailon]

On 11/03/2016 06:50 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 03 November 2016 10:56 PM, Alexandre Bailon wrote:
>> On 11/03/2016 05:34 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 03 November 2016 08:56 PM, Alexandre Bailon wrote:
>>>> The USB PHY is able to operate in OTG, host or peripheral.
>>>> Some board may be wired to work act only as host or peripheral.
>>>> In such case, the dr_mode property of controller must be set to
>>>> host or peripheral. But doing that will also configure the PHY
>>>> in host or peripheral mode whereas OTG is able to detect which
>>>> role the USB controller should take.
>>>> The PHY's host or peripheral mode are actually only useful when
>>>> hardware doesn't allow OTG to detect it's role.
>>>>
>>>> Add the usb20_force_mode property to force the PHY to operate
>>>> in host or peripheral mode.
>>>
>>> I think we do just that if we populate dr_mode with host or peripheral. Why do
>>> we need another property to control dr_mode property?
>> Because the phy doesn't work correctly when it is in host or
>> device mode.
> 
> That would be the same even with usb20_force_mode property. How does
> usb20_force_mode property help?
usb20_force_mode doesn't fix the issue. It only help when it is not
set, be letting the phy in otg mode.
I'm working on on workaround for the peripheral and host mode,
but again, I think it is better if we can keep the phy in otg mode and
doesn't use them.
> 
> Thanks
> Kishon
> 
>>>> When usb20_force_mode is used, dr_mode should also be configured
>>>> to host or peripheral.
>>>> The controller uses dr_mode to configure itself, but the phy use
>>>> it to get the mode to use to configure the PHY mode.
>>>>
>>>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>>> index c26478b..9fc87fb 100644
>>>> --- a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>>> @@ -4,6 +4,11 @@ Required properties:
>>>>   - compatible: must be "ti,da830-usb-phy".
>>>>   - #phy-cells: must be 1.
>>>>  
>>>> +Optional properties:
>>>> +- usb20-force-mode: Force the phy to operate in same mode than the USB OTG controller.
>>>> +		    It should only be defined if the hardware is not capable correctly
>>>> +		    detect the role of USB by using VBUS and ID pin.
>>>
>>> From what I understand from the previous patch, if VBUS sense and the session
>>> end comparator is enabled, the controller can work in host mode or device mode.
>> I but VBUS sense and and session end comparator only seems to work when
>> the phy is in otg mode.
>> In host mode, the phy stop to work after the first disconnect.
>> In device mode, the phy never detect a disconnect.
>> In otg mode, these issues go away.
>> I'm working on workaround for both of them but I think it is
>> better to keep the phy in otg when it is possible.
>>>
>>> Thanks
>>> Kishon
>>>
>> Thanks,
>> Alexandre
>>
Thanks,
Alexandre