[U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers

[U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

v2:     _ add needed reset_request() in RESET framework
	_ add error path in ehci/ohci-generic to disable clocks and to assert
	resets
	_ add .remove callback with clocks, resets and phy release
	_ split the replacement of printf() by error() in an independant patch
	
Patrice Chotard (5):
  reset: add reset_request()
  usb: host: add error path and remove callback in ehci-generic
  usb: host: extend generic EHCI driver with PHY
  usb: host: replace printf() by error() in ehci-generic
  usb: host: extend generic OHCI with CLOCK, RESET and PHY

 drivers/reset/reset-uclass.c    |  9 ++++
 drivers/usb/host/ehci-generic.c | 96 +++++++++++++++++++++++++++++++++------
 drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
 include/reset.h                 |  9 ++++
 4 files changed, 197 insertions(+), 16 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH v2 1/5] reset: add reset_request()

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

This is needed in error path to assert previously deasserted
reset by using a saved reset_ctl reference.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
 drivers/reset/reset-uclass.c | 9 +++++++++
 include/reset.h              | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index e92b24f..916f210 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -98,6 +98,15 @@ int reset_get_by_name(struct udevice *dev, const char *name,
 	return reset_get_by_index(dev, index, reset_ctl);
 }
 
+int reset_request(struct reset_ctl *reset_ctl)
+{
+	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	return ops->request(reset_ctl);
+}
+
 int reset_free(struct reset_ctl *reset_ctl)
 {
 	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
diff --git a/include/reset.h b/include/reset.h
index f45fcf8..4f2e35f 100644
--- a/include/reset.h
+++ b/include/reset.h
@@ -100,6 +100,15 @@ int reset_get_by_name(struct udevice *dev, const char *name,
 		      struct reset_ctl *reset_ctl);
 
 /**
+ * reset_request - Request a reset signal.
+ *
+ * @reset_ctl:	A reset control struct.
+ *
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_request(struct reset_ctl *reset_ctl);
+
+/**
  * reset_free - Free a previously requested reset signal.
  *
  * @reset_ctl:	A reset control struct that was previously successfully
-- 
1.9.1

[U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

Add error path to disable enabled clocks and to assert
deasserted resets
Populate the remove callback

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
v2:	_ split previous path 1, add error path and .remove callback

 drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------
 1 file changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 2190adb..0c29f63 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -11,6 +11,9 @@
 #include <dm.h>
 #include "ehci.h"
 
+#define EHCI_MAX_CLOCKS 3
+#define EHCI_MAX_RESETS 3
+
 /*
  * Even though here we don't explicitly use "struct ehci_ctrl"
  * ehci_register() expects it to be the first thing that resides in
@@ -18,35 +21,74 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct clk clks[EHCI_MAX_CLOCKS];
+	struct reset_ctl resets[EHCI_MAX_RESETS];
 };
 
+static void ehci_assert_resets(struct udevice *dev) {
+	struct generic_ehci *priv = dev_get_priv(dev);
+	struct reset_ctl reset;
+	int i;
+
+	for (i = EHCI_MAX_RESETS; i >= 0; --i) {
+		reset = priv->resets[i];
+
+		if (reset.dev) {
+                        reset_request(&reset);
+                        reset_assert(&reset);
+                        reset_free(&reset);
+		}
+	}
+}
+
+static void ehci_disable_clocks(struct udevice *dev) {
+	struct generic_ehci *priv = dev_get_priv(dev);
+	struct clk clk;
+	int i;
+
+	for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {
+		clk = priv->clks[i];
+
+		if (clk.dev) {
+			clk_request(clk.dev, &clk);
+			clk_disable(&clk);
+			clk_free(&clk);
+		}
+	}
+}
+
 static int ehci_usb_probe(struct udevice *dev)
 {
+	struct generic_ehci *priv = dev_get_priv(dev);
 	struct ehci_hccr *hccr;
 	struct ehci_hcor *hcor;
-	int i;
+	int i, ret;
 
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
+	for (i = 0; i < EHCI_MAX_CLOCKS; i++) {
+		struct clk clk = priv->clks[i];
 
 		ret = clk_get_by_index(dev, i, &clk);
 		if (ret < 0)
 			break;
-		if (clk_enable(&clk))
+		if (clk_enable(&clk)) {
 			printf("failed to enable clock %d\n", i);
+			clk_free(&clk);
+			goto clk_err;
+		}
 		clk_free(&clk);
 	}
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+	for (i = 0; i < EHCI_MAX_RESETS ; i++) {
+		struct reset_ctl reset = priv->resets[i];
 
 		ret = reset_get_by_index(dev, i, &reset);
 		if (ret < 0)
 			break;
-		if (reset_deassert(&reset))
+		if (reset_deassert(&reset)) {
 			printf("failed to deassert reset %d\n", i);
+			reset_free(&reset);
+			goto reset_err;
+		}
 		reset_free(&reset);
 	}
 
@@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev)
 	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
 				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
 
-	return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!ret)
+		return ret;
+
+reset_err:
+	ehci_assert_resets(dev);
+clk_err:
+	ehci_disable_clocks(dev);
+
+	return ret;
+}
+
+static int ehci_usb_remove(struct udevice *dev) {
+
+	ehci_assert_resets(dev);
+	ehci_disable_clocks(dev);
+
+	return ehci_deregister(dev);
 }
 
 static const struct udevice_id ehci_usb_ids[] = {
@@ -67,7 +126,7 @@ U_BOOT_DRIVER(ehci_generic) = {
 	.id	= UCLASS_USB,
 	.of_match = ehci_usb_ids,
 	.probe = ehci_usb_probe,
-	.remove = ehci_deregister,
+	.remove = ehci_usb_remove,
 	.ops	= &ehci_usb_ops,
 	.priv_auto_alloc_size = sizeof(struct generic_ehci),
 	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
-- 
1.9.1

[U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

Add support of generic PHY framework

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

v2:	_ split previous path 1, add generic PHY framework

 drivers/usb/host/ehci-generic.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 0c29f63..0000808 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -6,6 +6,8 @@
 
 #include <common.h>
 #include <clk.h>
+#include <fdtdec.h>
+#include <generic-phy.h>
 #include <reset.h>
 #include <asm/io.h>
 #include <dm.h>
@@ -23,6 +25,7 @@ struct generic_ehci {
 	struct ehci_ctrl ctrl;
 	struct clk clks[EHCI_MAX_CLOCKS];
 	struct reset_ctl resets[EHCI_MAX_RESETS];
+	struct phy phy;
 };
 
 static void ehci_assert_resets(struct udevice *dev) {
@@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
 		reset_free(&reset);
 	}
 
+	if (!generic_phy_get_by_index(dev, 0, &priv->phy))
+		if (generic_phy_init(&priv->phy))
+			error("failed to init usb phy %d\n", i);
+
 	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
 	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
 				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
@@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
 	if (!ret)
 		return ret;
 
+	generic_phy_exit(&priv->phy);
+
 reset_err:
 	ehci_assert_resets(dev);
 clk_err:
@@ -109,7 +118,9 @@ clk_err:
 }
 
 static int ehci_usb_remove(struct udevice *dev) {
+	struct generic_ehci *priv = dev_get_priv(dev);
 
+	generic_phy_exit(&priv->phy);
 	ehci_assert_resets(dev);
 	ehci_disable_clocks(dev);
 
-- 
1.9.1

[U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

v2:	_ create this independant path for printf() replacement

 drivers/usb/host/ehci-generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 0000808..39b56de 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
 		if (ret < 0)
 			break;
 		if (clk_enable(&clk)) {
-			printf("failed to enable clock %d\n", i);
+			error("failed to enable clock %d\n", i);
 			clk_free(&clk);
 			goto clk_err;
 		}
@@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
 		if (ret < 0)
 			break;
 		if (reset_deassert(&reset)) {
-			printf("failed to deassert reset %d\n", i);
+			error("failed to deassert reset %d\n", i);
 			reset_free(&reset);
 			goto reset_err;
 		}
-- 
1.9.1

[U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY

[patrice.chotard at st.com]

From: Patrice Chotard <patrice.chotard@st.com>

Add CLOCK, RESET and generic PHY frameworks support

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

v2:	_ add error path management
	_ add .remove callback

 drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 96 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index f3307f4..f86e223 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -5,26 +5,119 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
+#include <fdtdec.h>
+#include <generic-phy.h>
+#include <reset.h>
+
 #include "ohci.h"
 
 #if !defined(CONFIG_USB_OHCI_NEW)
 # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
 #endif
 
+#define OHCI_MAX_CLOCKS 3
+#define OHCI_MAX_RESETS 3
+
 struct generic_ohci {
 	ohci_t ohci;
+	struct clk clks[OHCI_MAX_CLOCKS];
+	struct reset_ctl resets[OHCI_MAX_RESETS];
+	struct phy phy;
 };
 
+static void ohci_assert_resets(struct udevice *dev) {
+	struct generic_ohci *priv = dev_get_priv(dev);
+	struct reset_ctl reset;
+	int i;
+
+	for (i = OHCI_MAX_RESETS; i >= 0; --i) {
+		reset = priv->resets[i];
+
+		if (reset.dev) {
+			reset_request(&reset);
+			reset_assert(&reset);
+			reset_free(&reset);
+		}
+	}
+}
+
+static void ohci_disable_clocks(struct udevice *dev) {
+	struct generic_ohci *priv = dev_get_priv(dev);
+	struct clk clk;
+	int i;
+
+	for (i = OHCI_MAX_CLOCKS; i >= 0; --i) {
+		clk = priv->clks[i];
+
+		if (clk.dev) {
+			clk_request(clk.dev, &clk);
+			clk_disable(&clk);
+			clk_free(&clk);
+		}
+	}
+}
+
 static int ohci_usb_probe(struct udevice *dev)
 {
 	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, ret;
+
+	for (i = 0; i < OHCI_MAX_CLOCKS; i++) {
+		struct clk clk = priv->clks[i];
 
-	return ohci_register(dev, regs);
+		ret = clk_get_by_index(dev, i, &clk);
+		if (ret < 0)
+			break;
+		if (clk_enable(&clk)) {
+			error("failed to enable clock %d\n", i);
+			clk_free(&clk);
+			goto clk_err;
+		}
+		clk_free(&clk);
+	}
+
+	for (i = 0; i < OHCI_MAX_RESETS ; i++) {
+		struct reset_ctl reset = priv->resets[i];
+
+		ret = reset_get_by_index(dev, i, &reset);
+		if (ret < 0)
+			break;
+		if (reset_deassert(&reset)) {
+			error("failed to deassert reset %d\n", i);
+			reset_free(&reset);
+			goto reset_err;
+		}
+		reset_free(&reset);
+	}
+
+	if (!generic_phy_get_by_index(dev, 0, &priv->phy))
+		if (generic_phy_init(&priv->phy))
+			error("failed to init usb phy %d\n", i);
+
+	ret = ohci_register(dev, regs);
+	if (!ret)
+		return ret;
+
+	generic_phy_exit(&priv->phy);
+
+reset_err:
+	ohci_assert_resets(dev);
+clk_err:
+	ohci_disable_clocks(dev);
+
+	return ret;
 }
 
-static int ohci_usb_remove(struct udevice *dev)
-{
+static int ohci_usb_remove(struct udevice *dev) {
+	struct generic_ohci *priv = dev_get_priv(dev);
+
+	generic_phy_exit(&priv->phy);
+	ohci_assert_resets(dev);
+	ohci_disable_clocks(dev);
+
 	return ohci_deregister(dev);
 }
 
-- 
1.9.1

[U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

[Marek Vasut]

On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> Add error path to disable enabled clocks and to assert
> deasserted resets
> Populate the remove callback
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> v2:	_ split previous path 1, add error path and .remove callback
> 
>  drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 2190adb..0c29f63 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -11,6 +11,9 @@
>  #include <dm.h>
>  #include "ehci.h"
>  
> +#define EHCI_MAX_CLOCKS 3
> +#define EHCI_MAX_RESETS 3

And then someone invents controller with four resets and this whole
thing ... breaks.

>  /*
>   * Even though here we don't explicitly use "struct ehci_ctrl"
>   * ehci_register() expects it to be the first thing that resides in
> @@ -18,35 +21,74 @@
>   */
>  struct generic_ehci {
>  	struct ehci_ctrl ctrl;
> +	struct clk clks[EHCI_MAX_CLOCKS];
> +	struct reset_ctl resets[EHCI_MAX_RESETS];
>  };
>  
> +static void ehci_assert_resets(struct udevice *dev) {
> +	struct generic_ehci *priv = dev_get_priv(dev);
> +	struct reset_ctl reset;
> +	int i;
> +
> +	for (i = EHCI_MAX_RESETS; i >= 0; --i) {

Why start from the end ?

> +		reset = priv->resets[i];
> +
> +		if (reset.dev) {
> +                        reset_request(&reset);
> +                        reset_assert(&reset);
> +                        reset_free(&reset);
> +		}
> +	}
> +}
> +
> +static void ehci_disable_clocks(struct udevice *dev) {
> +	struct generic_ehci *priv = dev_get_priv(dev);
> +	struct clk clk;
> +	int i;
> +
> +	for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {
> +		clk = priv->clks[i];
> +
> +		if (clk.dev) {
> +			clk_request(clk.dev, &clk);
> +			clk_disable(&clk);
> +			clk_free(&clk);
> +		}
> +	}
> +}
> +
>  static int ehci_usb_probe(struct udevice *dev)
>  {
> +	struct generic_ehci *priv = dev_get_priv(dev);
>  	struct ehci_hccr *hccr;
>  	struct ehci_hcor *hcor;
> -	int i;
> +	int i, ret;
>  
> -	for (i = 0; ; i++) {
> -		struct clk clk;
> -		int ret;
> +	for (i = 0; i < EHCI_MAX_CLOCKS; i++) {
> +		struct clk clk = priv->clks[i];
>  
>  		ret = clk_get_by_index(dev, i, &clk);
>  		if (ret < 0)
>  			break;
> -		if (clk_enable(&clk))
> +		if (clk_enable(&clk)) {
>  			printf("failed to enable clock %d\n", i);
> +			clk_free(&clk);
> +			goto clk_err;
> +		}
>  		clk_free(&clk);
>  	}
>  
> -	for (i = 0; ; i++) {
> -		struct reset_ctl reset;
> -		int ret;
> +	for (i = 0; i < EHCI_MAX_RESETS ; i++) {
> +		struct reset_ctl reset = priv->resets[i];
>  
>  		ret = reset_get_by_index(dev, i, &reset);
>  		if (ret < 0)
>  			break;
> -		if (reset_deassert(&reset))
> +		if (reset_deassert(&reset)) {
>  			printf("failed to deassert reset %d\n", i);
> +			reset_free(&reset);
> +			goto reset_err;
> +		}
>  		reset_free(&reset);
>  	}
>  
> @@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev)
>  	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>  				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>  
> -	return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
> +	ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
> +	if (!ret)
> +		return ret;
> +
> +reset_err:
> +	ehci_assert_resets(dev);
> +clk_err:
> +	ehci_disable_clocks(dev);
> +
> +	return ret;
> +}
> +
> +static int ehci_usb_remove(struct udevice *dev) {
> +
> +	ehci_assert_resets(dev);
> +	ehci_disable_clocks(dev);
> +
> +	return ehci_deregister(dev);
>  }
>  
>  static const struct udevice_id ehci_usb_ids[] = {
> @@ -67,7 +126,7 @@ U_BOOT_DRIVER(ehci_generic) = {
>  	.id	= UCLASS_USB,
>  	.of_match = ehci_usb_ids,
>  	.probe = ehci_usb_probe,
> -	.remove = ehci_deregister,
> +	.remove = ehci_usb_remove,
>  	.ops	= &ehci_usb_ops,
>  	.priv_auto_alloc_size = sizeof(struct generic_ehci),
>  	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY

[Marek Vasut]

On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> Add support of generic PHY framework
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v2:	_ split previous path 1, add generic PHY framework
> 
>  drivers/usb/host/ehci-generic.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0c29f63..0000808 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -6,6 +6,8 @@
>  
>  #include <common.h>
>  #include <clk.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
>  #include <reset.h>
>  #include <asm/io.h>
>  #include <dm.h>
> @@ -23,6 +25,7 @@ struct generic_ehci {
>  	struct ehci_ctrl ctrl;
>  	struct clk clks[EHCI_MAX_CLOCKS];
>  	struct reset_ctl resets[EHCI_MAX_RESETS];
> +	struct phy phy;
>  };
>  
>  static void ehci_assert_resets(struct udevice *dev) {
> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>  		reset_free(&reset);
>  	}
>  
> +	if (!generic_phy_get_by_index(dev, 0, &priv->phy))
> +		if (generic_phy_init(&priv->phy))
> +			error("failed to init usb phy %d\n", i);
> +
>  	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>  	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>  				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>  	if (!ret)
>  		return ret;
>  
> +	generic_phy_exit(&priv->phy);

So you probe the EHCI controller driver and then you disable it's PHY ?
That's a bit odd, isn't it ?

>  reset_err:
>  	ehci_assert_resets(dev);
>  clk_err:
> @@ -109,7 +118,9 @@ clk_err:
>  }
>  
>  static int ehci_usb_remove(struct udevice *dev) {
> +	struct generic_ehci *priv = dev_get_priv(dev);
>  
> +	generic_phy_exit(&priv->phy);
>  	ehci_assert_resets(dev);
>  	ehci_disable_clocks(dev);
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic

[Marek Vasut]

On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>

Commit message does not explain WHY this change is needed. In fact ...
commit message is missing altogether ...

> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v2:	_ create this independant path for printf() replacement
> 
>  drivers/usb/host/ehci-generic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0000808..39b56de 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
>  		if (ret < 0)
>  			break;
>  		if (clk_enable(&clk)) {
> -			printf("failed to enable clock %d\n", i);
> +			error("failed to enable clock %d\n", i);
>  			clk_free(&clk);
>  			goto clk_err;
>  		}
> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
>  		if (ret < 0)
>  			break;
>  		if (reset_deassert(&reset)) {
> -			printf("failed to deassert reset %d\n", i);
> +			error("failed to deassert reset %d\n", i);
>  			reset_free(&reset);
>  			goto reset_err;
>  		}
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/5] reset: add reset_request()

[Simon Glass]

On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> This is needed in error path to assert previously deasserted
> reset by using a saved reset_ctl reference.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>  drivers/reset/reset-uclass.c | 9 +++++++++
>  include/reset.h              | 9 +++++++++
>  2 files changed, 18 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

[Simon Glass]

Hi Patrice,

On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add error path to disable enabled clocks and to assert
> deasserted resets
> Populate the remove callback
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> v2:     _ split previous path 1, add error path and .remove callback
>
>  drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 70 insertions(+), 11 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below.

>
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 2190adb..0c29f63 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -11,6 +11,9 @@
>  #include <dm.h>
>  #include "ehci.h"
>
> +#define EHCI_MAX_CLOCKS 3
> +#define EHCI_MAX_RESETS 3
> +
>  /*
>   * Even though here we don't explicitly use "struct ehci_ctrl"
>   * ehci_register() expects it to be the first thing that resides in
> @@ -18,35 +21,74 @@
>   */
>  struct generic_ehci {
>         struct ehci_ctrl ctrl;
> +       struct clk clks[EHCI_MAX_CLOCKS];
> +       struct reset_ctl resets[EHCI_MAX_RESETS];
>  };
>
> +static void ehci_assert_resets(struct udevice *dev) {
> +       struct generic_ehci *priv = dev_get_priv(dev);

You could pass priv instead of dev to avoid this line.

> +       struct reset_ctl reset;
> +       int i;
> +
> +       for (i = EHCI_MAX_RESETS; i >= 0; --i) {
> +               reset = priv->resets[i];
> +
> +               if (reset.dev) {
> +                        reset_request(&reset);
> +                        reset_assert(&reset);
> +                        reset_free(&reset);
> +               }
> +       }
> +}
> +
> +static void ehci_disable_clocks(struct udevice *dev) {
> +       struct generic_ehci *priv = dev_get_priv(dev);
> +       struct clk clk;
> +       int i;
> +
> +       for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {

Do you think it is worth adding a function in the uclass to do this to
a list of clks?

> +               clk = priv->clks[i];
> +
> +               if (clk.dev) {
> +                       clk_request(clk.dev, &clk);
> +                       clk_disable(&clk);
> +                       clk_free(&clk);
> +               }
> +       }
> +}
> +
>  static int ehci_usb_probe(struct udevice *dev)
>  {
> +       struct generic_ehci *priv = dev_get_priv(dev);
>         struct ehci_hccr *hccr;
>         struct ehci_hcor *hcor;
> -       int i;
> +       int i, ret;
>
> -       for (i = 0; ; i++) {
> -               struct clk clk;
> -               int ret;
> +       for (i = 0; i < EHCI_MAX_CLOCKS; i++) {
> +               struct clk clk = priv->clks[i];
>
>                 ret = clk_get_by_index(dev, i, &clk);
>                 if (ret < 0)
>                         break;
> -               if (clk_enable(&clk))
> +               if (clk_enable(&clk)) {
>                         printf("failed to enable clock %d\n", i);
> +                       clk_free(&clk);
> +                       goto clk_err;
> +               }
>                 clk_free(&clk);
>         }
>
> -       for (i = 0; ; i++) {
> -               struct reset_ctl reset;
> -               int ret;
> +       for (i = 0; i < EHCI_MAX_RESETS ; i++) {
> +               struct reset_ctl reset = priv->resets[i];
>
>                 ret = reset_get_by_index(dev, i, &reset);
>                 if (ret < 0)
>                         break;
> -               if (reset_deassert(&reset))
> +               if (reset_deassert(&reset)) {
>                         printf("failed to deassert reset %d\n", i);
> +                       reset_free(&reset);
> +                       goto reset_err;
> +               }
>                 reset_free(&reset);
>         }
>
> @@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev)
>         hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>                                     HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>
> -       return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
> +       ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
> +       if (!ret)
> +               return ret;
> +
> +reset_err:
> +       ehci_assert_resets(dev);
> +clk_err:
> +       ehci_disable_clocks(dev);
> +
> +       return ret;
> +}
> +
> +static int ehci_usb_remove(struct udevice *dev) {
> +
> +       ehci_assert_resets(dev);
> +       ehci_disable_clocks(dev);
> +
> +       return ehci_deregister(dev);
>  }
>
>  static const struct udevice_id ehci_usb_ids[] = {
> @@ -67,7 +126,7 @@ U_BOOT_DRIVER(ehci_generic) = {
>         .id     = UCLASS_USB,
>         .of_match = ehci_usb_ids,
>         .probe = ehci_usb_probe,
> -       .remove = ehci_deregister,
> +       .remove = ehci_usb_remove,
>         .ops    = &ehci_usb_ops,
>         .priv_auto_alloc_size = sizeof(struct generic_ehci),
>         .flags  = DM_FLAG_ALLOC_PRIV_DMA,
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY

[Simon Glass]

Hi Patrice,

On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add support of generic PHY framework
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2:     _ split previous path 1, add generic PHY framework
>
>  drivers/usb/host/ehci-generic.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0c29f63..0000808 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -6,6 +6,8 @@
>
>  #include <common.h>
>  #include <clk.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
>  #include <reset.h>
>  #include <asm/io.h>
>  #include <dm.h>
> @@ -23,6 +25,7 @@ struct generic_ehci {
>         struct ehci_ctrl ctrl;
>         struct clk clks[EHCI_MAX_CLOCKS];
>         struct reset_ctl resets[EHCI_MAX_RESETS];
> +       struct phy phy;
>  };
>
>  static void ehci_assert_resets(struct udevice *dev) {
> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>                 reset_free(&reset);
>         }
>
> +       if (!generic_phy_get_by_index(dev, 0, &priv->phy))

Can you check for the error you expect here when it is not present? I
think it might be -ENOENT? That way you are not ignoring a real error.

> +               if (generic_phy_init(&priv->phy))
> +                       error("failed to init usb phy %d\n", i);

Shouldn't you return the error here? I don't think USB will work
without the PHY.

> +
>         hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>         hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>                                     HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>         if (!ret)
>                 return ret;
>
> +       generic_phy_exit(&priv->phy);
> +
>  reset_err:
>         ehci_assert_resets(dev);
>  clk_err:
> @@ -109,7 +118,9 @@ clk_err:
>  }
>
>  static int ehci_usb_remove(struct udevice *dev) {
> +       struct generic_ehci *priv = dev_get_priv(dev);
>
> +       generic_phy_exit(&priv->phy);
>         ehci_assert_resets(dev);
>         ehci_disable_clocks(dev);
>
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic

[Simon Glass]

On 12 May 2017 at 14:51, Marek Vasut <marex@denx.de> wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>
> Commit message does not explain WHY this change is needed. In fact ...
> commit message is missing altogether ...

Yes it really helps to have a commit message!

>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2:   _ create this independant path for printf() replacement

Reviewed-by: Simon Glass <sjg@chromium.org>

>>
>>  drivers/usb/host/ehci-generic.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0000808..39b56de 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
>>               if (ret < 0)
>>                       break;
>>               if (clk_enable(&clk)) {
>> -                     printf("failed to enable clock %d\n", i);
>> +                     error("failed to enable clock %d\n", i);
>>                       clk_free(&clk);
>>                       goto clk_err;
>>               }
>> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
>>               if (ret < 0)
>>                       break;
>>               if (reset_deassert(&reset)) {
>> -                     printf("failed to deassert reset %d\n", i);
>> +                     error("failed to deassert reset %d\n", i);
>>                       reset_free(&reset);
>>                       goto reset_err;
>>               }
>>
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY

[Simon Glass]

On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add CLOCK, RESET and generic PHY frameworks support
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2:     _ add error path management
>         _ add .remove callback
>
>  drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index f3307f4..f86e223 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -5,26 +5,119 @@
>   */
>
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
> +#include <reset.h>
> +
>  #include "ohci.h"
>
>  #if !defined(CONFIG_USB_OHCI_NEW)
>  # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>  #endif
>
> +#define OHCI_MAX_CLOCKS 3
> +#define OHCI_MAX_RESETS 3
> +
>  struct generic_ohci {
>         ohci_t ohci;
> +       struct clk clks[OHCI_MAX_CLOCKS];
> +       struct reset_ctl resets[OHCI_MAX_RESETS];
> +       struct phy phy;
>  };
>
> +static void ohci_assert_resets(struct udevice *dev) {
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +       struct reset_ctl reset;
> +       int i;
> +
> +       for (i = OHCI_MAX_RESETS; i >= 0; --i) {
> +               reset = priv->resets[i];
> +
> +               if (reset.dev) {
> +                       reset_request(&reset);
> +                       reset_assert(&reset);
> +                       reset_free(&reset);

Don't you need error checking here?

> +               }
> +       }
> +}
> +
> +static void ohci_disable_clocks(struct udevice *dev) {
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +       struct clk clk;
> +       int i;
> +
> +       for (i = OHCI_MAX_CLOCKS; i >= 0; --i) {
> +               clk = priv->clks[i];
> +
> +               if (clk.dev) {
> +                       clk_request(clk.dev, &clk);
> +                       clk_disable(&clk);
> +                       clk_free(&clk);
> +               }
> +       }
> +}
> +
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>         struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +       int i, ret;
> +
> +       for (i = 0; i < OHCI_MAX_CLOCKS; i++) {
> +               struct clk clk = priv->clks[i];
>
> -       return ohci_register(dev, regs);
> +               ret = clk_get_by_index(dev, i, &clk);
> +               if (ret < 0)
> +                       break;
> +               if (clk_enable(&clk)) {
> +                       error("failed to enable clock %d\n", i);
> +                       clk_free(&clk);
> +                       goto clk_err;
> +               }
> +               clk_free(&clk);
> +       }
> +
> +       for (i = 0; i < OHCI_MAX_RESETS ; i++) {
> +               struct reset_ctl reset = priv->resets[i];
> +
> +               ret = reset_get_by_index(dev, i, &reset);
> +               if (ret < 0)
> +                       break;
> +               if (reset_deassert(&reset)) {
> +                       error("failed to deassert reset %d\n", i);
> +                       reset_free(&reset);
> +                       goto reset_err;
> +               }
> +               reset_free(&reset);
> +       }
> +
> +       if (!generic_phy_get_by_index(dev, 0, &priv->phy))
> +               if (generic_phy_init(&priv->phy))
> +                       error("failed to init usb phy %d\n", i);
> +
> +       ret = ohci_register(dev, regs);
> +       if (!ret)
> +               return ret;
> +
> +       generic_phy_exit(&priv->phy);
> +
> +reset_err:
> +       ohci_assert_resets(dev);
> +clk_err:
> +       ohci_disable_clocks(dev);
> +
> +       return ret;
>  }
>
> -static int ohci_usb_remove(struct udevice *dev)
> -{
> +static int ohci_usb_remove(struct udevice *dev) {
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +
> +       generic_phy_exit(&priv->phy);
> +       ohci_assert_resets(dev);
> +       ohci_disable_clocks(dev);
> +
>         return ohci_deregister(dev);
>  }
>
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

[Patrice CHOTARD]

Hi Marek

On 05/12/2017 10:50 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add error path to disable enabled clocks and to assert
>> deasserted resets
>> Populate the remove callback
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>> v2:	_ split previous path 1, add error path and .remove callback
>>
>>  drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------
>>  1 file changed, 70 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 2190adb..0c29f63 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -11,6 +11,9 @@
>>  #include <dm.h>
>>  #include "ehci.h"
>>
>> +#define EHCI_MAX_CLOCKS 3
>> +#define EHCI_MAX_RESETS 3
>
> And then someone invents controller with four resets and this whole
> thing ... breaks.

OK i will use a more generic solution based on list

>
>>  /*
>>   * Even though here we don't explicitly use "struct ehci_ctrl"
>>   * ehci_register() expects it to be the first thing that resides in
>> @@ -18,35 +21,74 @@
>>   */
>>  struct generic_ehci {
>>  	struct ehci_ctrl ctrl;
>> +	struct clk clks[EHCI_MAX_CLOCKS];
>> +	struct reset_ctl resets[EHCI_MAX_RESETS];
>>  };
>>
>> +static void ehci_assert_resets(struct udevice *dev) {
>> +	struct generic_ehci *priv = dev_get_priv(dev);
>> +	struct reset_ctl reset;
>> +	int i;
>> +
>> +	for (i = EHCI_MAX_RESETS; i >= 0; --i) {
>
> Why start from the end ?

Simply to assert resets in reverse order they have been deasserted.
Is it a problematic ?

Thanks

Patrice

>
>> +		reset = priv->resets[i];
>> +
>> +		if (reset.dev) {
>> +                        reset_request(&reset);
>> +                        reset_assert(&reset);
>> +                        reset_free(&reset);
>> +		}
>> +	}
>> +}
>> +
>> +static void ehci_disable_clocks(struct udevice *dev) {
>> +	struct generic_ehci *priv = dev_get_priv(dev);
>> +	struct clk clk;
>> +	int i;
>> +
>> +	for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {
>> +		clk = priv->clks[i];
>> +
>> +		if (clk.dev) {
>> +			clk_request(clk.dev, &clk);
>> +			clk_disable(&clk);
>> +			clk_free(&clk);
>> +		}
>> +	}
>> +}
>> +
>>  static int ehci_usb_probe(struct udevice *dev)
>>  {
>> +	struct generic_ehci *priv = dev_get_priv(dev);
>>  	struct ehci_hccr *hccr;
>>  	struct ehci_hcor *hcor;
>> -	int i;
>> +	int i, ret;
>>
>> -	for (i = 0; ; i++) {
>> -		struct clk clk;
>> -		int ret;
>> +	for (i = 0; i < EHCI_MAX_CLOCKS; i++) {
>> +		struct clk clk = priv->clks[i];
>>
>>  		ret = clk_get_by_index(dev, i, &clk);
>>  		if (ret < 0)
>>  			break;
>> -		if (clk_enable(&clk))
>> +		if (clk_enable(&clk)) {
>>  			printf("failed to enable clock %d\n", i);
>> +			clk_free(&clk);
>> +			goto clk_err;
>> +		}
>>  		clk_free(&clk);
>>  	}
>>
>> -	for (i = 0; ; i++) {
>> -		struct reset_ctl reset;
>> -		int ret;
>> +	for (i = 0; i < EHCI_MAX_RESETS ; i++) {
>> +		struct reset_ctl reset = priv->resets[i];
>>
>>  		ret = reset_get_by_index(dev, i, &reset);
>>  		if (ret < 0)
>>  			break;
>> -		if (reset_deassert(&reset))
>> +		if (reset_deassert(&reset)) {
>>  			printf("failed to deassert reset %d\n", i);
>> +			reset_free(&reset);
>> +			goto reset_err;
>> +		}
>>  		reset_free(&reset);
>>  	}
>>
>> @@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev)
>>  	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>>  				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>>
>> -	return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> +	ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> +	if (!ret)
>> +		return ret;
>> +
>> +reset_err:
>> +	ehci_assert_resets(dev);
>> +clk_err:
>> +	ehci_disable_clocks(dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ehci_usb_remove(struct udevice *dev) {
>> +
>> +	ehci_assert_resets(dev);
>> +	ehci_disable_clocks(dev);
>> +
>> +	return ehci_deregister(dev);
>>  }
>>
>>  static const struct udevice_id ehci_usb_ids[] = {
>> @@ -67,7 +126,7 @@ U_BOOT_DRIVER(ehci_generic) = {
>>  	.id	= UCLASS_USB,
>>  	.of_match = ehci_usb_ids,
>>  	.probe = ehci_usb_probe,
>> -	.remove = ehci_deregister,
>> +	.remove = ehci_usb_remove,
>>  	.ops	= &ehci_usb_ops,
>>  	.priv_auto_alloc_size = sizeof(struct generic_ehci),
>>  	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
>>
>
>

[U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY

[Patrice CHOTARD]

Hi Marek

On 05/12/2017 10:50 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add support of generic PHY framework
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2:	_ split previous path 1, add generic PHY framework
>>
>>  drivers/usb/host/ehci-generic.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0c29f63..0000808 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -6,6 +6,8 @@
>>
>>  #include <common.h>
>>  #include <clk.h>
>> +#include <fdtdec.h>
>> +#include <generic-phy.h>
>>  #include <reset.h>
>>  #include <asm/io.h>
>>  #include <dm.h>
>> @@ -23,6 +25,7 @@ struct generic_ehci {
>>  	struct ehci_ctrl ctrl;
>>  	struct clk clks[EHCI_MAX_CLOCKS];
>>  	struct reset_ctl resets[EHCI_MAX_RESETS];
>> +	struct phy phy;
>>  };
>>
>>  static void ehci_assert_resets(struct udevice *dev) {
>> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>>  		reset_free(&reset);
>>  	}
>>
>> +	if (!generic_phy_get_by_index(dev, 0, &priv->phy))
>> +		if (generic_phy_init(&priv->phy))
>> +			error("failed to init usb phy %d\n", i);
>> +
>>  	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>>  	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>>  				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>>  	if (!ret)
>>  		return ret;
>>
>> +	generic_phy_exit(&priv->phy);
>
> So you probe the EHCI controller driver and then you disable it's PHY ?
> That's a bit odd, isn't it ?

The PHY is disabled in case of ehci_register() didn't succeed.

Patrice

>
>>  reset_err:
>>  	ehci_assert_resets(dev);
>>  clk_err:
>> @@ -109,7 +118,9 @@ clk_err:
>>  }
>>
>>  static int ehci_usb_remove(struct udevice *dev) {
>> +	struct generic_ehci *priv = dev_get_priv(dev);
>>
>> +	generic_phy_exit(&priv->phy);
>>  	ehci_assert_resets(dev);
>>  	ehci_disable_clocks(dev);
>>
>>
>
>

[U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic

[Patrice CHOTARD]

Hi Marek

On 05/12/2017 10:51 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>
> Commit message does not explain WHY this change is needed. In fact ...
> commit message is missing altogether ...

Yes, my bad i forget it, i will add it in the v3

Thanks

Patrice


>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2:	_ create this independant path for printf() replacement
>>
>>  drivers/usb/host/ehci-generic.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0000808..39b56de 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
>>  		if (ret < 0)
>>  			break;
>>  		if (clk_enable(&clk)) {
>> -			printf("failed to enable clock %d\n", i);
>> +			error("failed to enable clock %d\n", i);
>>  			clk_free(&clk);
>>  			goto clk_err;
>>  		}
>> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
>>  		if (ret < 0)
>>  			break;
>>  		if (reset_deassert(&reset)) {
>> -			printf("failed to deassert reset %d\n", i);
>> +			error("failed to deassert reset %d\n", i);
>>  			reset_free(&reset);
>>  			goto reset_err;
>>  		}
>>
>
>

[U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

[Patrice CHOTARD]

Hi Simon

On 05/15/2017 05:03 AM, Simon Glass wrote:
> Hi Patrice,
>
> On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add error path to disable enabled clocks and to assert
>> deasserted resets
>> Populate the remove callback
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>> v2:     _ split previous path 1, add error path and .remove callback
>>
>>  drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------
>>  1 file changed, 70 insertions(+), 11 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please see below.
>
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 2190adb..0c29f63 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -11,6 +11,9 @@
>>  #include <dm.h>
>>  #include "ehci.h"
>>
>> +#define EHCI_MAX_CLOCKS 3
>> +#define EHCI_MAX_RESETS 3
>> +
>>  /*
>>   * Even though here we don't explicitly use "struct ehci_ctrl"
>>   * ehci_register() expects it to be the first thing that resides in
>> @@ -18,35 +21,74 @@
>>   */
>>  struct generic_ehci {
>>         struct ehci_ctrl ctrl;
>> +       struct clk clks[EHCI_MAX_CLOCKS];
>> +       struct reset_ctl resets[EHCI_MAX_RESETS];
>>  };
>>
>> +static void ehci_assert_resets(struct udevice *dev) {
>> +       struct generic_ehci *priv = dev_get_priv(dev);
>
> You could pass priv instead of dev to avoid this line.

Agree, will fix it

>
>> +       struct reset_ctl reset;
>> +       int i;
>> +
>> +       for (i = EHCI_MAX_RESETS; i >= 0; --i) {
>> +               reset = priv->resets[i];
>> +
>> +               if (reset.dev) {
>> +                        reset_request(&reset);
>> +                        reset_assert(&reset);
>> +                        reset_free(&reset);
>> +               }
>> +       }
>> +}
>> +
>> +static void ehci_disable_clocks(struct udevice *dev) {
>> +       struct generic_ehci *priv = dev_get_priv(dev);
>> +       struct clk clk;
>> +       int i;
>> +
>> +       for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {
>
> Do you think it is worth adding a function in the uclass to do this to
> a list of clks?

I will have a look at this

Patrice

>
>> +               clk = priv->clks[i];
>> +
>> +               if (clk.dev) {
>> +                       clk_request(clk.dev, &clk);
>> +                       clk_disable(&clk);
>> +                       clk_free(&clk);
>> +               }
>> +       }
>> +}
>> +
>>  static int ehci_usb_probe(struct udevice *dev)
>>  {
>> +       struct generic_ehci *priv = dev_get_priv(dev);
>>         struct ehci_hccr *hccr;
>>         struct ehci_hcor *hcor;
>> -       int i;
>> +       int i, ret;
>>
>> -       for (i = 0; ; i++) {
>> -               struct clk clk;
>> -               int ret;
>> +       for (i = 0; i < EHCI_MAX_CLOCKS; i++) {
>> +               struct clk clk = priv->clks[i];
>>
>>                 ret = clk_get_by_index(dev, i, &clk);
>>                 if (ret < 0)
>>                         break;
>> -               if (clk_enable(&clk))
>> +               if (clk_enable(&clk)) {
>>                         printf("failed to enable clock %d\n", i);
>> +                       clk_free(&clk);
>> +                       goto clk_err;
>> +               }
>>                 clk_free(&clk);
>>         }
>>
>> -       for (i = 0; ; i++) {
>> -               struct reset_ctl reset;
>> -               int ret;
>> +       for (i = 0; i < EHCI_MAX_RESETS ; i++) {
>> +               struct reset_ctl reset = priv->resets[i];
>>
>>                 ret = reset_get_by_index(dev, i, &reset);
>>                 if (ret < 0)
>>                         break;
>> -               if (reset_deassert(&reset))
>> +               if (reset_deassert(&reset)) {
>>                         printf("failed to deassert reset %d\n", i);
>> +                       reset_free(&reset);
>> +                       goto reset_err;
>> +               }
>>                 reset_free(&reset);
>>         }
>>
>> @@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev)
>>         hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>>                                     HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>>
>> -       return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> +       ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> +       if (!ret)
>> +               return ret;
>> +
>> +reset_err:
>> +       ehci_assert_resets(dev);
>> +clk_err:
>> +       ehci_disable_clocks(dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static int ehci_usb_remove(struct udevice *dev) {
>> +
>> +       ehci_assert_resets(dev);
>> +       ehci_disable_clocks(dev);
>> +
>> +       return ehci_deregister(dev);
>>  }
>>
>>  static const struct udevice_id ehci_usb_ids[] = {
>> @@ -67,7 +126,7 @@ U_BOOT_DRIVER(ehci_generic) = {
>>         .id     = UCLASS_USB,
>>         .of_match = ehci_usb_ids,
>>         .probe = ehci_usb_probe,
>> -       .remove = ehci_deregister,
>> +       .remove = ehci_usb_remove,
>>         .ops    = &ehci_usb_ops,
>>         .priv_auto_alloc_size = sizeof(struct generic_ehci),
>>         .flags  = DM_FLAG_ALLOC_PRIV_DMA,
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY

[Patrice CHOTARD]

Hi Simon

On 05/15/2017 05:03 AM, Simon Glass wrote:
> Hi Patrice,
>
> On 12 May 2017 at 11:27,  <patrice.chotard@st.com> wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add support of generic PHY framework
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2:     _ split previous path 1, add generic PHY framework
>>
>>  drivers/usb/host/ehci-generic.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0c29f63..0000808 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -6,6 +6,8 @@
>>
>>  #include <common.h>
>>  #include <clk.h>
>> +#include <fdtdec.h>
>> +#include <generic-phy.h>
>>  #include <reset.h>
>>  #include <asm/io.h>
>>  #include <dm.h>
>> @@ -23,6 +25,7 @@ struct generic_ehci {
>>         struct ehci_ctrl ctrl;
>>         struct clk clks[EHCI_MAX_CLOCKS];
>>         struct reset_ctl resets[EHCI_MAX_RESETS];
>> +       struct phy phy;
>>  };
>>
>>  static void ehci_assert_resets(struct udevice *dev) {
>> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>>                 reset_free(&reset);
>>         }
>>
>> +       if (!generic_phy_get_by_index(dev, 0, &priv->phy))
>
> Can you check for the error you expect here when it is not present? I
> think it might be -ENOENT? That way you are not ignoring a real error.


ok

>
>> +               if (generic_phy_init(&priv->phy))
>> +                       error("failed to init usb phy %d\n", i);
>
> Shouldn't you return the error here? I don't think USB will work
> without the PHY.

Agree, i forgot it

Thanks

Patrice

>
>> +
>>         hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>>         hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>>                                     HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>>         if (!ret)
>>                 return ret;
>>
>> +       generic_phy_exit(&priv->phy);
>> +
>>  reset_err:
>>         ehci_assert_resets(dev);
>>  clk_err:
>> @@ -109,7 +118,9 @@ clk_err:
>>  }
>>
>>  static int ehci_usb_remove(struct udevice *dev) {
>> +       struct generic_ehci *priv = dev_get_priv(dev);
>>
>> +       generic_phy_exit(&priv->phy);
>>         ehci_assert_resets(dev);
>>         ehci_disable_clocks(dev);
>>
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>