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

[PATCH v4 0/4] 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.

Changes in v4:
* Enable VBUS sense and session end comparator during probe of phy.
* Add a quirk flag to skip the phy set mode made during the init of musb.
  In that way, we can still can configure the phy mode by using mode file in
  sysfs and configure the controller mode by using dr_mode or platform_data.
* Init the phy in otg mode in da8xx glue driver.

Alexandre Bailon (4):
  usb: musb: da8xx: Call earlier clk_prepare_enable()
  phy: da8xx-usb: Configure CFGCHIP2 to support OTG workaround
  usb: musb: Add a quirk flag to skip the phy set mode
  usb: musb: da8xx: Set phy in OTG mode by default

 drivers/phy/phy-da8xx-usb.c  | 20 +++++++++++++++-----
 drivers/usb/musb/da8xx.c     | 27 +++++++++++++++++----------
 drivers/usb/musb/musb_core.c | 15 ++++++++++-----
 drivers/usb/musb/musb_core.h |  1 +
 4 files changed, 43 insertions(+), 20 deletions(-)

-- 
2.7.3

[PATCH v4 1/4] 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 v4 2/4] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
index 32ae78c..089c13b 100644
--- a/drivers/phy/phy-da8xx-usb.c
+++ b/drivers/phy/phy-da8xx-usb.c
@@ -23,6 +23,8 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
+#define PHY_INIT_BITS	(CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN)
+
 struct da8xx_usb_phy {
 	struct phy_provider	*phy_provider;
 	struct phy		*usb11_phy;
@@ -207,6 +209,9 @@ static int da8xx_usb_phy_probe(struct platform_device *pdev)
 			dev_warn(dev, "Failed to create usb20 phy lookup\n");
 	}
 
+	regmap_write_bits(d_phy->regmap, CFGCHIP(2),
+			  PHY_INIT_BITS, PHY_INIT_BITS);
+
 	return 0;
 }
 
-- 
2.7.3

[PATCH v4 3/4] usb: musb: Add a quirk flag to skip the phy set mode

[Alexandre Bailon]

During the init, the driver will use the mode to configure
the controller mode and the phy mode.
The PHY of DA8xx has some issues when the phy is forced in host or device.
Add way to skip the set mode and let the da8xx glue manage the phy mode.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/usb/musb/musb_core.c | 15 ++++++++++-----
 drivers/usb/musb/musb_core.h |  1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 27dadc0..6f5f039 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2046,6 +2046,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 {
 	int			status;
 	struct musb		*musb;
+	enum musb_mode		mode = MUSB_UNDEFINED;
 	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
 
 	/* The driver might handle more features than the board; OK.
@@ -2261,13 +2262,13 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 		status = musb_host_setup(musb, plat->power);
 		if (status < 0)
 			goto fail3;
-		status = musb_platform_set_mode(musb, MUSB_HOST);
+		mode = MUSB_HOST;
 		break;
 	case MUSB_PORT_MODE_GADGET:
 		status = musb_gadget_setup(musb);
 		if (status < 0)
 			goto fail3;
-		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
+		mode = MUSB_PERIPHERAL;
 		break;
 	case MUSB_PORT_MODE_DUAL_ROLE:
 		status = musb_host_setup(musb, plat->power);
@@ -2278,15 +2279,19 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 			musb_host_cleanup(musb);
 			goto fail3;
 		}
-		status = musb_platform_set_mode(musb, MUSB_OTG);
+		mode = MUSB_OTG;
 		break;
 	default:
 		dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
 		break;
 	}
 
-	if (status < 0)
-		goto fail3;
+	if (mode != MUSB_UNDEFINED &&
+		!(musb->io.quirks & MUSB_SKIP_SET_MODE)) {
+		status = musb_platform_set_mode(musb, mode);
+		if (status < 0)
+			goto fail3;
+	}
 
 	status = musb_init_debugfs(musb);
 	if (status < 0)
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 2cb88a49..9bd8b6b 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -172,6 +172,7 @@ struct musb_io;
  */
 struct musb_platform_ops {
 
+#define MUSB_SKIP_SET_MODE	BIT(7)
 #define MUSB_DMA_UX500		BIT(6)
 #define MUSB_DMA_CPPI41		BIT(5)
 #define MUSB_DMA_CPPI		BIT(4)
-- 
2.7.3

Re: [PATCH v4 3/4] usb: musb: Add a quirk flag to skip the phy set mode

[David Lechner]

On 11/04/2016 11:43 AM, Alexandre Bailon wrote:
> During the init, the driver will use the mode to configure
> the controller mode and the phy mode.
> The PHY of DA8xx has some issues when the phy is forced in host or device.
> Add way to skip the set mode and let the da8xx glue manage the phy mode.
>

I think you are on the right track here. But I think a cleaner way to do 
this would be to add a bool parameter to musb_platform_set_mode() that 
is passed musb->ops->set_mode() callback. You could call this parameter 
init or first_call or something like that. This way, the set_mode() 
callback can have a different behavior depending on when it is called.

In musb_init_controller(), for example, you would have...

	status = musb_platform_set_mode(musb, MUSB_HOST, true);

and in musb_mode_store(), you would have...

	status = musb_platform_set_mode(musb, MUSB_HOST, false);



> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/usb/musb/musb_core.c | 15 ++++++++++-----
>  drivers/usb/musb/musb_core.h |  1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 27dadc0..6f5f039 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2046,6 +2046,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  {
>  	int			status;
>  	struct musb		*musb;
> +	enum musb_mode		mode = MUSB_UNDEFINED;
>  	struct musb_hdrc_platform_data *plat = dev_get_platdata(dev);
>
>  	/* The driver might handle more features than the board; OK.
> @@ -2261,13 +2262,13 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  		status = musb_host_setup(musb, plat->power);
>  		if (status < 0)
>  			goto fail3;
> -		status = musb_platform_set_mode(musb, MUSB_HOST);
> +		mode = MUSB_HOST;
>  		break;
>  	case MUSB_PORT_MODE_GADGET:
>  		status = musb_gadget_setup(musb);
>  		if (status < 0)
>  			goto fail3;
> -		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
> +		mode = MUSB_PERIPHERAL;
>  		break;
>  	case MUSB_PORT_MODE_DUAL_ROLE:
>  		status = musb_host_setup(musb, plat->power);
> @@ -2278,15 +2279,19 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  			musb_host_cleanup(musb);
>  			goto fail3;
>  		}
> -		status = musb_platform_set_mode(musb, MUSB_OTG);
> +		mode = MUSB_OTG;
>  		break;
>  	default:
>  		dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
>  		break;
>  	}
>
> -	if (status < 0)
> -		goto fail3;
> +	if (mode != MUSB_UNDEFINED &&
> +		!(musb->io.quirks & MUSB_SKIP_SET_MODE)) {
> +		status = musb_platform_set_mode(musb, mode);
> +		if (status < 0)
> +			goto fail3;
> +	}
>
>  	status = musb_init_debugfs(musb);
>  	if (status < 0)
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 2cb88a49..9bd8b6b 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -172,6 +172,7 @@ struct musb_io;
>   */
>  struct musb_platform_ops {
>
> +#define MUSB_SKIP_SET_MODE	BIT(7)

This would be better described as MUSB_SKIP_SET_MODE_IN_INIT

>  #define MUSB_DMA_UX500		BIT(6)
>  #define MUSB_DMA_CPPI41		BIT(5)
>  #define MUSB_DMA_CPPI		BIT(4)
>

Re: [PATCH v4 3/4] usb: musb: Add a quirk flag to skip the phy set mode

[Sergei Shtylyov]

Hello.

On 11/04/2016 07:43 PM, Alexandre Bailon wrote:

> During the init, the driver will use the mode to configure
> the controller mode and the phy mode.

    PHY -- be consistent please...

> The PHY of DA8xx has some issues when the phy is forced in host or device.

    Again.

> Add way to skip the set mode and let the da8xx glue manage the phy mode.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/usb/musb/musb_core.c | 15 ++++++++++-----
>  drivers/usb/musb/musb_core.h |  1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 27dadc0..6f5f039 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
[...]
> @@ -2278,15 +2279,19 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
[...]
> -	if (status < 0)
> -		goto fail3;
> +	if (mode != MUSB_UNDEFINED &&
> +		!(musb->io.quirks & MUSB_SKIP_SET_MODE)) {

    Please either add one more tab here or align to 'mode', so it's easier on 
the eyes.

> +		status = musb_platform_set_mode(musb, mode);
> +		if (status < 0)
> +			goto fail3;
> +	}
>
>  	status = musb_init_debugfs(musb);
>  	if (status < 0)
[...]

MBR, Sergei

[PATCH v4 4/4] usb: musb: da8xx: Set phy in OTG mode by default

[Alexandre Bailon]

The DA8xx OTG PHY has some issues when it is forced in host or
peripheral mode. Actually, most of the time, OTG is the best mode
because host or peripheral mode are only required for hardware that
miss some circuitry.
Init the PHY mode OTG mode by default.

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

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 6749aa1..e6f41232 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -401,6 +401,12 @@ static int da8xx_musb_init(struct musb *musb)
 		goto err_phy_power_on;
 	}
 
+	ret = phy_set_mode(glue->phy, PHY_MODE_USB_OTG);
+	if (ret) {
+		dev_err(glue->dev, "Failed to set the phy mode.\n");
+		goto err_phy_set_mode;
+	}
+
 	msleep(5);
 
 	/* NOTE: IRQs are in mixed mode, not bypass to pure MUSB */
@@ -410,6 +416,8 @@ static int da8xx_musb_init(struct musb *musb)
 	musb->isr = da8xx_musb_interrupt;
 	return 0;
 
+err_phy_set_mode:
+	phy_power_off(glue->phy);
 err_phy_power_on:
 	phy_exit(glue->phy);
 fail:
@@ -433,7 +441,7 @@ static int da8xx_musb_exit(struct musb *musb)
 }
 
 static const struct musb_platform_ops da8xx_ops = {
-	.quirks		= MUSB_DMA_CPPI | MUSB_INDEXED_EP,
+	.quirks		= MUSB_DMA_CPPI | MUSB_INDEXED_EP | MUSB_SKIP_SET_MODE,
 	.init		= da8xx_musb_init,
 	.exit		= da8xx_musb_exit,
 
-- 
2.7.3