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

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

[patrice.chotard at st.com]

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

v3:     _ keep enabled clocks and deasserted resets reference in list in order to 
	  disable clock or assert resets in error path or in .remove callback
	_ add missing commit message
	_ use struct generic_ehci * instead of struct udevice * as parameter for
	  ehci_release_resets() and ehci_release_clocks()
	_ test return value on generic_phy_get_by_index() and
	  generic_phy_init()
	_ split previous patch 5 in 3 independant patch for CLOCK, RESET and PHY support

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 (7):
  reset: add reset_request()
  usb: host: ehci-generic: replace printf() by error()
  usb: host: ehci-generic: add error path and .remove callback
  usb: host: ehci-generic: add generic PHY support
  usb: host: ohci-generic: add CLOCK support
  usb: host: ohci-generic: add RESET support
  usb: host: ohci-generic: add generic PHY support

 drivers/reset/reset-uclass.c    |   9 ++
 drivers/usb/host/ehci-generic.c | 189 ++++++++++++++++++++++++++++++++++++----
 drivers/usb/host/ohci-generic.c | 184 +++++++++++++++++++++++++++++++++++++-
 include/reset.h                 |   9 ++
 4 files changed, 374 insertions(+), 17 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH v3 1/7] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

v3:	_ none
v2:	_ none

 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 v3 2/7] usb: host: ehci-generic: replace printf() by error()

[patrice.chotard at st.com]

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

This allows to get file, line and function location
of the current error message.

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

v3:	_ add commit message

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 2190adb..6058e9a 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -34,7 +34,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);
 	}
 
@@ -46,7 +46,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);
 	}
 
-- 
1.9.1

[U-Boot] [PATCH v3 3/7] usb: host: ehci-generic: add error path and .remove callback

[patrice.chotard at st.com]

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

use list to save reference to enabled clocks and deasserted resets
in order to respectively disabled and asserted them in case of error
during probe() or during driver removal.

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

v3:	_ keep enabled clocks and deasserted resets reference in list in order to 
	  disable clock or assert resets in error path or in .remove callback
	_ use struct generic_ehci * instead of struct udevice * as parameter for
	  ehci_release_resets() and ehci_release_clocks()

 drivers/usb/host/ehci-generic.c | 162 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 149 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 6058e9a..d281218 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -11,6 +11,16 @@
 #include <dm.h>
 #include "ehci.h"
 
+struct ehci_clock {
+	struct clk *clk;
+	struct list_head list;
+};
+
+struct ehci_reset {
+	struct reset_ctl *reset;
+	struct list_head 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,43 +28,169 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct list_head clks;
+	struct list_head resets;
 };
 
+static int ehci_release_resets(struct generic_ehci *priv)
+{
+	struct ehci_reset *ehci_reset, *tmp;
+	struct reset_ctl *reset;
+	int ret;
+
+	list_for_each_entry_safe(ehci_reset, tmp, &priv->resets, list) {
+		reset = ehci_reset->reset;
+
+		ret = reset_request(reset);
+		if (ret)
+			return ret;
+
+		ret = reset_assert(reset);
+		if (ret)
+			return ret;
+
+		ret = reset_free(reset);
+		if (ret)
+			return ret;
+
+		list_del(&ehci_reset->list);
+	}
+	return 0;
+}
+
+static int ehci_release_clocks(struct generic_ehci *priv)
+{
+	struct ehci_clock *ehci_clock, *tmp;
+	struct clk *clk;
+	int ret;
+
+	list_for_each_entry_safe(ehci_clock, tmp, &priv->clks, list) {
+		clk = ehci_clock->clk;
+
+		clk_request(clk->dev, clk);
+		if (ret)
+			return ret;
+
+		clk_disable(clk);
+
+		ret = clk_free(clk);
+		if (ret)
+			return ret;
+
+		list_del(&ehci_clock->list);
+	}
+	return 0;
+}
+
 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;
+
+	INIT_LIST_HEAD(&priv->clks);
+	INIT_LIST_HEAD(&priv->resets);
 
 	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
+		struct ehci_clock *ehci_clock;
+		struct clk *clk;
 
-		ret = clk_get_by_index(dev, i, &clk);
+		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
+		if (!clk) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+
+		ret = clk_get_by_index(dev, i, clk);
 		if (ret < 0)
 			break;
-		if (clk_enable(&clk))
+
+		if (clk_enable(clk)) {
 			error("failed to enable clock %d\n", i);
-		clk_free(&clk);
+			clk_free(clk);
+			goto clk_err;
+		}
+		clk_free(clk);
+
+		/*
+		 * add enabled clocks into clks list in order to be disabled
+		 * later on ehci_usb_remove() call or in error path if needed
+		 */
+		ehci_clock = devm_kmalloc(dev, sizeof(*ehci_clock), GFP_KERNEL);
+		if (!ehci_clock) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+		ehci_clock->clk = clk;
+		list_add(&ehci_clock->list, &priv->clks);
 	}
 
 	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+		struct ehci_reset *ehci_reset;
+		struct reset_ctl *reset;
+
+		reset = devm_kmalloc(dev, sizeof(*reset), GFP_KERNEL);
+		if (!reset) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
 
-		ret = reset_get_by_index(dev, i, &reset);
+		ret = reset_get_by_index(dev, i, reset);
 		if (ret < 0)
 			break;
-		if (reset_deassert(&reset))
+
+		if (reset_deassert(reset)) {
 			error("failed to deassert reset %d\n", i);
-		reset_free(&reset);
+			reset_free(reset);
+			goto reset_err;
+		}
+		reset_free(reset);
+
+		/*
+		 * add deasserted resets into resets list in order to be
+		 * asserted later on ehci_usb_remove() call or in error
+		 * path if needed
+		 */
+		ehci_reset = devm_kmalloc(dev, sizeof(*ehci_reset), GFP_KERNEL);
+		if (!ehci_reset) {
+			error("Can't allocate resource\n");
+			goto reset_err;
+		}
+		ehci_reset->reset = reset;
+		list_add(&ehci_reset->list, &priv->resets);
 	}
 
 	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
 	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:
+	ret = ehci_release_resets(priv);
+	if (ret)
+		return ret;
+clk_err:
+	return ehci_release_clocks(priv);
+}
+
+static int ehci_usb_remove(struct udevice *dev)
+{
+	struct generic_ehci *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = ehci_deregister(dev);
+	if (ret)
+		return ret;
+
+	ret = ehci_release_resets(priv);
+	if (ret)
+		return ret;
+
+	return ehci_release_clocks(priv);
 }
 
 static const struct udevice_id ehci_usb_ids[] = {
@@ -67,7 +203,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 v3 4/7] usb: host: ehci-generic: add generic PHY support

[patrice.chotard at st.com]

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

Extend ehci-generic driver with generic PHY framework

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

v3:	_ test return value on generic_phy_get_by_index() and
	  generic_phy_init()

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

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index d281218..c360666 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>
@@ -30,6 +32,7 @@ struct generic_ehci {
 	struct ehci_ctrl ctrl;
 	struct list_head clks;
 	struct list_head resets;
+	struct phy phy;
 };
 
 static int ehci_release_resets(struct generic_ehci *priv)
@@ -161,6 +164,18 @@ static int ehci_usb_probe(struct udevice *dev)
 		list_add(&ehci_reset->list, &priv->resets);
 	}
 
+	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (ret) {
+		error("failed to get usb phy\n");
+		goto reset_err;
+	}
+
+	ret = generic_phy_init(&priv->phy);
+	if (ret) {
+		error("failed to init usb phy\n");
+		goto reset_err;
+	}
+
 	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
 	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
 				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
@@ -169,6 +184,10 @@ static int ehci_usb_probe(struct udevice *dev)
 	if (!ret)
 		return ret;
 
+	ret = generic_phy_exit(&priv->phy);
+	if (ret)
+		return ret;
+
 reset_err:
 	ret = ehci_release_resets(priv);
 	if (ret)
@@ -186,6 +205,10 @@ static int ehci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	ret = generic_phy_exit(&priv->phy);
+	if (ret)
+		return ret;
+
 	ret = ehci_release_resets(priv);
 	if (ret)
 		return ret;
-- 
1.9.1

[U-Boot] [PATCH v3 5/7] usb: host: ohci-generic: add CLOCK support

[patrice.chotard at st.com]

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

use list to save reference to enabled clocks in order to
disabled them in case of error during probe() or
during driver removal.

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

v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
	_ keep enabled clocks reference in list in order to 
	  disable clocks in error path or in .remove callback

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

 drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index f3307f4..a6d89a8 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -5,6 +5,7 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include "ohci.h"
 
@@ -12,20 +13,98 @@
 # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
 #endif
 
+struct ohci_clock {
+	struct clk *clk;
+	struct list_head list;
+};
+
 struct generic_ohci {
 	ohci_t ohci;
+	struct list_head clks;
 };
 
+static int ohci_release_clocks(struct generic_ohci *priv)
+{
+	struct ohci_clock *ohci_clock, *tmp;
+	struct clk *clk;
+	int ret;
+
+	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
+		clk = ohci_clock->clk;
+
+		clk_request(clk->dev, clk);
+		if (ret)
+			return ret;
+
+		clk_disable(clk);
+
+		ret = clk_free(clk);
+		if (ret)
+			return ret;
+
+		list_del(&ohci_clock->list);
+	}
+	return 0;
+}
+
 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;
+
+	INIT_LIST_HEAD(&priv->clks);
+
+	for (i = 0; ; i++) {
+		struct ohci_clock *ohci_clock;
+		struct clk *clk;
+
+		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
+		if (!clk) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+
+		ret = clk_get_by_index(dev, i, clk);
+		if (ret < 0)
+			break;
+
+		if (clk_enable(clk)) {
+			error("failed to enable ohci_clock %d\n", i);
+			clk_free(clk);
+			goto clk_err;
+		}
+		clk_free(clk);
+
+		/*
+		 * add enabled clocks into clks list in order to be disabled
+		 * later on ohci_usb_remove() call or in error path if needed
+		 */
+		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
+		if (!ohci_clock) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+		ohci_clock->clk = clk;
+		list_add(&ohci_clock->list, &priv->clks);
+	}
 
 	return ohci_register(dev, regs);
+
+clk_err:
+	return ohci_release_clocks(priv);
 }
 
 static int ohci_usb_remove(struct udevice *dev)
 {
-	return ohci_deregister(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = ohci_deregister(dev);
+	if (ret)
+		return ret;
+
+	return ohci_release_clocks(priv);
 }
 
 static const struct udevice_id ohci_usb_ids[] = {
-- 
1.9.1

[U-Boot] [PATCH v3 6/7] usb: host: ohci-generic: add RESET support

[patrice.chotard at st.com]

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

use list to save reference to deasserted resets in order to
assert them in case of error during probe() or during driver
removal.

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

v3:	_ extract in this patch the RESET support add-on from previous patch 5
	_ keep deassrted resets reference in list in order to 
	  assert resets in error path or in .remove callback

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

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

diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index a6d89a8..bf14ab7 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
+#include <reset.h>
 #include "ohci.h"
 
 #if !defined(CONFIG_USB_OHCI_NEW)
@@ -18,11 +19,43 @@ struct ohci_clock {
 	struct list_head list;
 };
 
+struct ohci_reset {
+	struct reset_ctl *reset;
+	struct list_head list;
+};
+
 struct generic_ohci {
 	ohci_t ohci;
 	struct list_head clks;
+	struct list_head resets;
 };
 
+static int ohci_release_resets(struct generic_ohci *priv)
+{
+	struct ohci_reset *ohci_reset, *tmp;
+	struct reset_ctl *reset;
+	int ret;
+
+	list_for_each_entry_safe(ohci_reset, tmp, &priv->resets, list) {
+		reset = ohci_reset->reset;
+
+		ret = reset_request(reset);
+		if (ret)
+			return ret;
+
+		ret = reset_assert(reset);
+		if (ret)
+			return ret;
+
+		ret = reset_free(reset);
+		if (ret)
+			return ret;
+
+		list_del(&(ohci_reset->list));
+	}
+	return 0;
+}
+
 static int ohci_release_clocks(struct generic_ohci *priv)
 {
 	struct ohci_clock *ohci_clock, *tmp;
@@ -54,6 +87,7 @@ static int ohci_usb_probe(struct udevice *dev)
 	int i, ret;
 
 	INIT_LIST_HEAD(&priv->clks);
+	INIT_LIST_HEAD(&priv->resets);
 
 	for (i = 0; ; i++) {
 		struct ohci_clock *ohci_clock;
@@ -89,8 +123,48 @@ static int ohci_usb_probe(struct udevice *dev)
 		list_add(&ohci_clock->list, &priv->clks);
 	}
 
+	for (i = 0; ; i++) {
+		struct ohci_reset *ohci_reset;
+		struct reset_ctl *reset;
+
+		reset = devm_kmalloc(dev, sizeof(*reset), GFP_KERNEL);
+		if (!reset) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+
+		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);
+
+		/*
+		 * add deasserted resets into resets list in order to be
+		 * asserted later on ohci_usb_remove() call or in error
+		 * path if needed
+		 */
+		ohci_reset = devm_kmalloc(dev, sizeof(*ohci_reset), GFP_KERNEL);
+		if (!ohci_reset) {
+			error("Can't allocate resource\n");
+			goto reset_err;
+		}
+		ohci_reset->reset = reset;
+		list_add(&ohci_reset->list, &priv->resets);
+	}
+
+
 	return ohci_register(dev, regs);
 
+reset_err:
+	ret = ohci_release_resets(priv);
+	if (ret)
+		return ret;
 clk_err:
 	return ohci_release_clocks(priv);
 }
@@ -104,6 +178,10 @@ static int ohci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	ret = ohci_release_resets(priv);
+	if (ret)
+		return ret;
+
 	return ohci_release_clocks(priv);
 }
 
-- 
1.9.1

[U-Boot] [PATCH v3 7/7] usb: host: ohci-generic: add generic PHY support

[patrice.chotard at st.com]

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

Extend ohci-generic driver with generic PHY framework

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

v3:	_ extract in this patch the PHY support add-on from previous patch 5


 drivers/usb/host/ohci-generic.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index bf14ab7..47e522c 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
+#include <generic-phy.h>
 #include <reset.h>
 #include "ohci.h"
 
@@ -28,6 +29,7 @@ struct generic_ohci {
 	ohci_t ohci;
 	struct list_head clks;
 	struct list_head resets;
+	struct phy phy;
 };
 
 static int ohci_release_resets(struct generic_ohci *priv)
@@ -158,8 +160,25 @@ static int ohci_usb_probe(struct udevice *dev)
 		list_add(&ohci_reset->list, &priv->resets);
 	}
 
+	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (ret) {
+		error("failed to get usb phy\n");
+		goto reset_err;
+	}
+
+	ret = generic_phy_init(&priv->phy);
+	if (ret) {
+		error("failed to init usb phy\n");
+		goto reset_err;
+	}
+
+	ret = ohci_register(dev, regs);
+	if (!ret)
+		return ret;
 
-	return ohci_register(dev, regs);
+	ret = generic_phy_exit(&priv->phy);
+	if (ret)
+		return ret;
 
 reset_err:
 	ret = ohci_release_resets(priv);
@@ -178,6 +197,10 @@ static int ohci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	ret = generic_phy_exit(&priv->phy);
+	if (ret)
+		return ret;
+
 	ret = ohci_release_resets(priv);
 	if (ret)
 		return ret;
-- 
1.9.1

[U-Boot] [PATCH v3 5/7] usb: host: ohci-generic: add CLOCK support

[Marek Vasut]

On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> use list to save reference to enabled clocks in order to
> disabled them in case of error during probe() or
> during driver removal.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
> 	_ keep enabled clocks reference in list in order to 
> 	  disable clocks in error path or in .remove callback
> 
> v2:	_ add error path management
> 	_ add .remove callback
> 
>  drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index f3307f4..a6d89a8 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>  #include "ohci.h"
>  
> @@ -12,20 +13,98 @@
>  # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>  #endif
>  
> +struct ohci_clock {
> +	struct clk *clk;
> +	struct list_head list;
> +};
> +
>  struct generic_ohci {
>  	ohci_t ohci;
> +	struct list_head clks;
>  };
>  
> +static int ohci_release_clocks(struct generic_ohci *priv)
> +{
> +	struct ohci_clock *ohci_clock, *tmp;
> +	struct clk *clk;
> +	int ret;
> +
> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
> +		clk = ohci_clock->clk;
> +
> +		clk_request(clk->dev, clk);
> +		if (ret)
> +			return ret;
> +
> +		clk_disable(clk);
> +
> +		ret = clk_free(clk);
> +		if (ret)
> +			return ret;
> +
> +		list_del(&ohci_clock->list);
> +	}
> +	return 0;
> +}
> +
>  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;
> +
> +	INIT_LIST_HEAD(&priv->clks);
> +
> +	for (i = 0; ; i++) {
> +		struct ohci_clock *ohci_clock;
> +		struct clk *clk;
> +
> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);

Since you know how many entries the clock phandle has, you can allocate
an array and drop this while list handling and this per-element kmalloc,
which fragments the allocator pool.

> +		if (!clk) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +
> +		ret = clk_get_by_index(dev, i, clk);
> +		if (ret < 0)
> +			break;
> +
> +		if (clk_enable(clk)) {
> +			error("failed to enable ohci_clock %d\n", i);
> +			clk_free(clk);
> +			goto clk_err;
> +		}
> +		clk_free(clk);
> +
> +		/*
> +		 * add enabled clocks into clks list in order to be disabled
> +		 * later on ohci_usb_remove() call or in error path if needed
> +		 */
> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);

Can't you just embed one structure into the other ?

> +		if (!ohci_clock) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +		ohci_clock->clk = clk;
> +		list_add(&ohci_clock->list, &priv->clks);
> +	}
>  
>  	return ohci_register(dev, regs);
> +
> +clk_err:
> +	return ohci_release_clocks(priv);
>  }
>  
>  static int ohci_usb_remove(struct udevice *dev)
>  {
> -	return ohci_deregister(dev);
> +	struct generic_ohci *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = ohci_deregister(dev);
> +	if (ret)
> +		return ret;
> +
> +	return ohci_release_clocks(priv);
>  }
>  
>  static const struct udevice_id ohci_usb_ids[] = {
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 6/7] usb: host: ohci-generic: add RESET support

[Marek Vasut]

On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> use list to save reference to deasserted resets in order to
> assert them in case of error during probe() or during driver
> removal.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v3:	_ extract in this patch the RESET support add-on from previous patch 5
> 	_ keep deassrted resets reference in list in order to 
> 	  assert resets in error path or in .remove callback
> 
> v2:	_ add error path management
> 	_ add .remove callback
> 
>  drivers/usb/host/ohci-generic.c | 78 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index a6d89a8..bf14ab7 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <clk.h>
>  #include <dm.h>
> +#include <reset.h>
>  #include "ohci.h"
>  
>  #if !defined(CONFIG_USB_OHCI_NEW)
> @@ -18,11 +19,43 @@ struct ohci_clock {
>  	struct list_head list;
>  };
>  
> +struct ohci_reset {
> +	struct reset_ctl *reset;
> +	struct list_head list;
> +};
> +
>  struct generic_ohci {
>  	ohci_t ohci;
>  	struct list_head clks;
> +	struct list_head resets;
>  };
>  
> +static int ohci_release_resets(struct generic_ohci *priv)
> +{
> +	struct ohci_reset *ohci_reset, *tmp;
> +	struct reset_ctl *reset;
> +	int ret;
> +
> +	list_for_each_entry_safe(ohci_reset, tmp, &priv->resets, list) {
> +		reset = ohci_reset->reset;
> +
> +		ret = reset_request(reset);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_assert(reset);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_free(reset);
> +		if (ret)
> +			return ret;
> +
> +		list_del(&(ohci_reset->list));
> +	}
> +	return 0;
> +}
> +
>  static int ohci_release_clocks(struct generic_ohci *priv)
>  {
>  	struct ohci_clock *ohci_clock, *tmp;
> @@ -54,6 +87,7 @@ static int ohci_usb_probe(struct udevice *dev)
>  	int i, ret;
>  
>  	INIT_LIST_HEAD(&priv->clks);
> +	INIT_LIST_HEAD(&priv->resets);
>  
>  	for (i = 0; ; i++) {
>  		struct ohci_clock *ohci_clock;
> @@ -89,8 +123,48 @@ static int ohci_usb_probe(struct udevice *dev)
>  		list_add(&ohci_clock->list, &priv->clks);
>  	}
>  
> +	for (i = 0; ; i++) {
> +		struct ohci_reset *ohci_reset;
> +		struct reset_ctl *reset;
> +
> +		reset = devm_kmalloc(dev, sizeof(*reset), GFP_KERNEL);

Same comment as on 5/7

> +		if (!reset) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +
> +		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);
> +
> +		/*
> +		 * add deasserted resets into resets list in order to be
> +		 * asserted later on ohci_usb_remove() call or in error
> +		 * path if needed
> +		 */
> +		ohci_reset = devm_kmalloc(dev, sizeof(*ohci_reset), GFP_KERNEL);
> +		if (!ohci_reset) {
> +			error("Can't allocate resource\n");
> +			goto reset_err;
> +		}
> +		ohci_reset->reset = reset;
> +		list_add(&ohci_reset->list, &priv->resets);
> +	}
> +
> +
>  	return ohci_register(dev, regs);
>  
> +reset_err:
> +	ret = ohci_release_resets(priv);
> +	if (ret)
> +		return ret;
>  clk_err:
>  	return ohci_release_clocks(priv);
>  }
> @@ -104,6 +178,10 @@ static int ohci_usb_remove(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = ohci_release_resets(priv);
> +	if (ret)
> +		return ret;
> +
>  	return ohci_release_clocks(priv);
>  }
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 3/7] usb: host: ehci-generic: add error path and .remove callback

[Marek Vasut]

On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> use list to save reference to enabled clocks and deasserted resets
> in order to respectively disabled and asserted them in case of error
> during probe() or during driver removal.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v3:	_ keep enabled clocks and deasserted resets reference in list in order to 
> 	  disable clock or assert resets in error path or in .remove callback
> 	_ use struct generic_ehci * instead of struct udevice * as parameter for
> 	  ehci_release_resets() and ehci_release_clocks()
> 
>  drivers/usb/host/ehci-generic.c | 162 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 149 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 6058e9a..d281218 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -11,6 +11,16 @@
>  #include <dm.h>
>  #include "ehci.h"
>  
> +struct ehci_clock {
> +	struct clk *clk;
> +	struct list_head list;
> +};
> +
> +struct ehci_reset {
> +	struct reset_ctl *reset;
> +	struct list_head 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,43 +28,169 @@
>   */
>  struct generic_ehci {
>  	struct ehci_ctrl ctrl;
> +	struct list_head clks;
> +	struct list_head resets;
>  };

These functions look so generic that I see no point in not factoring
them out into the clk and reset frameworks respectively.

> +static int ehci_release_resets(struct generic_ehci *priv)
> +{
> +	struct ehci_reset *ehci_reset, *tmp;
> +	struct reset_ctl *reset;
> +	int ret;
> +
> +	list_for_each_entry_safe(ehci_reset, tmp, &priv->resets, list) {
> +		reset = ehci_reset->reset;
> +
> +		ret = reset_request(reset);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_assert(reset);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_free(reset);
> +		if (ret)
> +			return ret;
> +
> +		list_del(&ehci_reset->list);
> +	}
> +	return 0;
> +}
> +
> +static int ehci_release_clocks(struct generic_ehci *priv)
> +{
> +	struct ehci_clock *ehci_clock, *tmp;
> +	struct clk *clk;
> +	int ret;
> +
> +	list_for_each_entry_safe(ehci_clock, tmp, &priv->clks, list) {
> +		clk = ehci_clock->clk;
> +
> +		clk_request(clk->dev, clk);
> +		if (ret)
> +			return ret;
> +
> +		clk_disable(clk);
> +
> +		ret = clk_free(clk);
> +		if (ret)
> +			return ret;
> +
> +		list_del(&ehci_clock->list);
> +	}
> +	return 0;
> +}
> +
>  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;
> +
> +	INIT_LIST_HEAD(&priv->clks);
> +	INIT_LIST_HEAD(&priv->resets);
>  
>  	for (i = 0; ; i++) {
> -		struct clk clk;
> -		int ret;
> +		struct ehci_clock *ehci_clock;
> +		struct clk *clk;
>  
> -		ret = clk_get_by_index(dev, i, &clk);
> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
> +		if (!clk) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +
> +		ret = clk_get_by_index(dev, i, clk);
>  		if (ret < 0)
>  			break;
> -		if (clk_enable(&clk))
> +
> +		if (clk_enable(clk)) {
>  			error("failed to enable clock %d\n", i);
> -		clk_free(&clk);
> +			clk_free(clk);
> +			goto clk_err;
> +		}
> +		clk_free(clk);
> +
> +		/*
> +		 * add enabled clocks into clks list in order to be disabled
> +		 * later on ehci_usb_remove() call or in error path if needed
> +		 */
> +		ehci_clock = devm_kmalloc(dev, sizeof(*ehci_clock), GFP_KERNEL);
> +		if (!ehci_clock) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +		ehci_clock->clk = clk;
> +		list_add(&ehci_clock->list, &priv->clks);
>  	}
>  
>  	for (i = 0; ; i++) {
> -		struct reset_ctl reset;
> -		int ret;
> +		struct ehci_reset *ehci_reset;
> +		struct reset_ctl *reset;
> +
> +		reset = devm_kmalloc(dev, sizeof(*reset), GFP_KERNEL);
> +		if (!reset) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
>  
> -		ret = reset_get_by_index(dev, i, &reset);
> +		ret = reset_get_by_index(dev, i, reset);
>  		if (ret < 0)
>  			break;
> -		if (reset_deassert(&reset))
> +
> +		if (reset_deassert(reset)) {
>  			error("failed to deassert reset %d\n", i);
> -		reset_free(&reset);
> +			reset_free(reset);
> +			goto reset_err;
> +		}
> +		reset_free(reset);
> +
> +		/*
> +		 * add deasserted resets into resets list in order to be
> +		 * asserted later on ehci_usb_remove() call or in error
> +		 * path if needed
> +		 */
> +		ehci_reset = devm_kmalloc(dev, sizeof(*ehci_reset), GFP_KERNEL);
> +		if (!ehci_reset) {
> +			error("Can't allocate resource\n");
> +			goto reset_err;
> +		}
> +		ehci_reset->reset = reset;
> +		list_add(&ehci_reset->list, &priv->resets);
>  	}
>  
>  	hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>  	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:
> +	ret = ehci_release_resets(priv);
> +	if (ret)
> +		return ret;
> +clk_err:
> +	return ehci_release_clocks(priv);
> +}
> +
> +static int ehci_usb_remove(struct udevice *dev)
> +{
> +	struct generic_ehci *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = ehci_deregister(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ehci_release_resets(priv);
> +	if (ret)
> +		return ret;
> +
> +	return ehci_release_clocks(priv);
>  }
>  
>  static const struct udevice_id ehci_usb_ids[] = {
> @@ -67,7 +203,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 v3 5/7] usb: host: ohci-generic: add CLOCK support

[Patrice CHOTARD]

Hi Marek

On 05/18/2017 11:29 AM, Marek Vasut wrote:
> On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> use list to save reference to enabled clocks in order to
>> disabled them in case of error during probe() or
>> during driver removal.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>> 	_ keep enabled clocks reference in list in order to
>> 	  disable clocks in error path or in .remove callback
>>
>> v2:	_ add error path management
>> 	_ add .remove callback
>>
>>   drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>> index f3307f4..a6d89a8 100644
>> --- a/drivers/usb/host/ohci-generic.c
>> +++ b/drivers/usb/host/ohci-generic.c
>> @@ -5,6 +5,7 @@
>>    */
>>   
>>   #include <common.h>
>> +#include <clk.h>
>>   #include <dm.h>
>>   #include "ohci.h"
>>   
>> @@ -12,20 +13,98 @@
>>   # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>   #endif
>>   
>> +struct ohci_clock {
>> +	struct clk *clk;
>> +	struct list_head list;
>> +};
>> +
>>   struct generic_ohci {
>>   	ohci_t ohci;
>> +	struct list_head clks;
>>   };
>>   
>> +static int ohci_release_clocks(struct generic_ohci *priv)
>> +{
>> +	struct ohci_clock *ohci_clock, *tmp;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>> +		clk = ohci_clock->clk;
>> +
>> +		clk_request(clk->dev, clk);
>> +		if (ret)
>> +			return ret;
>> +
>> +		clk_disable(clk);
>> +
>> +		ret = clk_free(clk);
>> +		if (ret)
>> +			return ret;
>> +
>> +		list_del(&ohci_clock->list);
>> +	}
>> +	return 0;
>> +}
>> +
>>   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;
>> +
>> +	INIT_LIST_HEAD(&priv->clks);
>> +
>> +	for (i = 0; ; i++) {
>> +		struct ohci_clock *ohci_clock;
>> +		struct clk *clk;
>> +
>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
> 
> Since you know how many entries the clock phandle has, you can allocate
> an array and drop this while list handling and this per-element kmalloc,
> which fragments the allocator pool.

I disagree, at this point we don't know how many entries the clock 
phandle has.

But, following your idea to avoid allocator pool fragmentation, in order 
to get the phandle number for array allocation, i can, for example add a 
new fdt API :

int fdtdec_get_phandle_nb(const void *blob, int src_node,
				   const char *list_name,
				   const char *cells_name,
				   int cell_count,
				   int phandle_nb);

Then, with phandle_nb,, we 'll be able to allocate the right array size 
for clocks and resets before populating them.

Thanks

Patrice

> 
>> +		if (!clk) {
>> +			error("Can't allocate resource\n");
>> +			goto clk_err;
>> +		}
>> +
>> +		ret = clk_get_by_index(dev, i, clk);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		if (clk_enable(clk)) {
>> +			error("failed to enable ohci_clock %d\n", i);
>> +			clk_free(clk);
>> +			goto clk_err;
>> +		}
>> +		clk_free(clk);
>> +
>> +		/*
>> +		 * add enabled clocks into clks list in order to be disabled
>> +		 * later on ohci_usb_remove() call or in error path if needed
>> +		 */
>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
> 
> Can't you just embed one structure into the other ?
> 
>> +		if (!ohci_clock) {
>> +			error("Can't allocate resource\n");
>> +			goto clk_err;
>> +		}
>> +		ohci_clock->clk = clk;
>> +		list_add(&ohci_clock->list, &priv->clks);
>> +	}
>>   
>>   	return ohci_register(dev, regs);
>> +
>> +clk_err:
>> +	return ohci_release_clocks(priv);
>>   }
>>   
>>   static int ohci_usb_remove(struct udevice *dev)
>>   {
>> -	return ohci_deregister(dev);
>> +	struct generic_ohci *priv = dev_get_priv(dev);
>> +	int ret;
>> +
>> +	ret = ohci_deregister(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ohci_release_clocks(priv);
>>   }
>>   
>>   static const struct udevice_id ohci_usb_ids[] = {
>>
> 
> 

[U-Boot] [PATCH v3 5/7] usb: host: ohci-generic: add CLOCK support

[Marek Vasut]

On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:
> Hi Marek
> 
> On 05/18/2017 11:29 AM, Marek Vasut wrote:
>> On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>
>>> use list to save reference to enabled clocks in order to
>>> disabled them in case of error during probe() or
>>> during driver removal.
>>>
>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>> ---
>>>
>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>>> 	_ keep enabled clocks reference in list in order to
>>> 	  disable clocks in error path or in .remove callback
>>>
>>> v2:	_ add error path management
>>> 	_ add .remove callback
>>>
>>>   drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>>> index f3307f4..a6d89a8 100644
>>> --- a/drivers/usb/host/ohci-generic.c
>>> +++ b/drivers/usb/host/ohci-generic.c
>>> @@ -5,6 +5,7 @@
>>>    */
>>>   
>>>   #include <common.h>
>>> +#include <clk.h>
>>>   #include <dm.h>
>>>   #include "ohci.h"
>>>   
>>> @@ -12,20 +13,98 @@
>>>   # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>>   #endif
>>>   
>>> +struct ohci_clock {
>>> +	struct clk *clk;
>>> +	struct list_head list;
>>> +};
>>> +
>>>   struct generic_ohci {
>>>   	ohci_t ohci;
>>> +	struct list_head clks;
>>>   };
>>>   
>>> +static int ohci_release_clocks(struct generic_ohci *priv)
>>> +{
>>> +	struct ohci_clock *ohci_clock, *tmp;
>>> +	struct clk *clk;
>>> +	int ret;
>>> +
>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>>> +		clk = ohci_clock->clk;
>>> +
>>> +		clk_request(clk->dev, clk);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		clk_disable(clk);
>>> +
>>> +		ret = clk_free(clk);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		list_del(&ohci_clock->list);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>   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;
>>> +
>>> +	INIT_LIST_HEAD(&priv->clks);
>>> +
>>> +	for (i = 0; ; i++) {
>>> +		struct ohci_clock *ohci_clock;
>>> +		struct clk *clk;
>>> +
>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
>>
>> Since you know how many entries the clock phandle has, you can allocate
>> an array and drop this while list handling and this per-element kmalloc,
>> which fragments the allocator pool.
> 
> I disagree, at this point we don't know how many entries the clock 
> phandle has.

I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,
which is already better.

> But, following your idea to avoid allocator pool fragmentation, in order 
> to get the phandle number for array allocation, i can, for example add a 
> new fdt API :
> 
> int fdtdec_get_phandle_nb(const void *blob, int src_node,
> 				   const char *list_name,
> 				   const char *cells_name,
> 				   int cell_count,
> 				   int phandle_nb);

Uh, why ?

> Then, with phandle_nb,, we 'll be able to allocate the right array size 
> for clocks and resets before populating them.

Just query the number of elements up front and then allocate the array ?

> Thanks
> 
> Patrice
> 
>>
>>> +		if (!clk) {
>>> +			error("Can't allocate resource\n");
>>> +			goto clk_err;
>>> +		}
>>> +
>>> +		ret = clk_get_by_index(dev, i, clk);
>>> +		if (ret < 0)
>>> +			break;
>>> +
>>> +		if (clk_enable(clk)) {
>>> +			error("failed to enable ohci_clock %d\n", i);
>>> +			clk_free(clk);
>>> +			goto clk_err;
>>> +		}
>>> +		clk_free(clk);
>>> +
>>> +		/*
>>> +		 * add enabled clocks into clks list in order to be disabled
>>> +		 * later on ohci_usb_remove() call or in error path if needed
>>> +		 */
>>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
>>
>> Can't you just embed one structure into the other ?
>>
>>> +		if (!ohci_clock) {
>>> +			error("Can't allocate resource\n");
>>> +			goto clk_err;
>>> +		}
>>> +		ohci_clock->clk = clk;
>>> +		list_add(&ohci_clock->list, &priv->clks);
>>> +	}
>>>   
>>>   	return ohci_register(dev, regs);
>>> +
>>> +clk_err:
>>> +	return ohci_release_clocks(priv);
>>>   }
>>>   
>>>   static int ohci_usb_remove(struct udevice *dev)
>>>   {
>>> -	return ohci_deregister(dev);
>>> +	struct generic_ohci *priv = dev_get_priv(dev);
>>> +	int ret;
>>> +
>>> +	ret = ohci_deregister(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return ohci_release_clocks(priv);
>>>   }
>>>   
>>>   static const struct udevice_id ohci_usb_ids[] = {
>>>
>>


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 5/7] usb: host: ohci-generic: add CLOCK support

[Patrice CHOTARD]

On 05/19/2017 01:51 PM, Marek Vasut wrote:
> On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:
>> Hi Marek
>>
>> On 05/18/2017 11:29 AM, Marek Vasut wrote:
>>> On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
>>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>>
>>>> use list to save reference to enabled clocks in order to
>>>> disabled them in case of error during probe() or
>>>> during driver removal.
>>>>
>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>> ---
>>>>
>>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>>>> 	_ keep enabled clocks reference in list in order to
>>>> 	  disable clocks in error path or in .remove callback
>>>>
>>>> v2:	_ add error path management
>>>> 	_ add .remove callback
>>>>
>>>>    drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 80 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>>>> index f3307f4..a6d89a8 100644
>>>> --- a/drivers/usb/host/ohci-generic.c
>>>> +++ b/drivers/usb/host/ohci-generic.c
>>>> @@ -5,6 +5,7 @@
>>>>     */
>>>>    
>>>>    #include <common.h>
>>>> +#include <clk.h>
>>>>    #include <dm.h>
>>>>    #include "ohci.h"
>>>>    
>>>> @@ -12,20 +13,98 @@
>>>>    # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>>>    #endif
>>>>    
>>>> +struct ohci_clock {
>>>> +	struct clk *clk;
>>>> +	struct list_head list;
>>>> +};
>>>> +
>>>>    struct generic_ohci {
>>>>    	ohci_t ohci;
>>>> +	struct list_head clks;
>>>>    };
>>>>    
>>>> +static int ohci_release_clocks(struct generic_ohci *priv)
>>>> +{
>>>> +	struct ohci_clock *ohci_clock, *tmp;
>>>> +	struct clk *clk;
>>>> +	int ret;
>>>> +
>>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>>>> +		clk = ohci_clock->clk;
>>>> +
>>>> +		clk_request(clk->dev, clk);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		clk_disable(clk);
>>>> +
>>>> +		ret = clk_free(clk);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		list_del(&ohci_clock->list);
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    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;
>>>> +
>>>> +	INIT_LIST_HEAD(&priv->clks);
>>>> +
>>>> +	for (i = 0; ; i++) {
>>>> +		struct ohci_clock *ohci_clock;
>>>> +		struct clk *clk;
>>>> +
>>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
>>>
>>> Since you know how many entries the clock phandle has, you can allocate
>>> an array and drop this while list handling and this per-element kmalloc,
>>> which fragments the allocator pool.
>>
>> I disagree, at this point we don't know how many entries the clock
>> phandle has.
> 
> I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,
> which is already better.

Can you elaborate ?

> 
>> But, following your idea to avoid allocator pool fragmentation, in order
>> to get the phandle number for array allocation, i can, for example add a
>> new fdt API :
>>
>> int fdtdec_get_phandle_nb(const void *blob, int src_node,
>> 				   const char *list_name,
>> 				   const char *cells_name,
>> 				   int cell_count,
>> 				   int phandle_nb);
> 
> Uh, why ?
> 
>> Then, with phandle_nb,, we 'll be able to allocate the right array size
>> for clocks and resets before populating them.
> 
> Just query the number of elements up front and then allocate the array ?

Can you indicate me with which API please ?

> 
>> Thanks
>>
>> Patrice
>>
>>>
>>>> +		if (!clk) {
>>>> +			error("Can't allocate resource\n");
>>>> +			goto clk_err;
>>>> +		}
>>>> +
>>>> +		ret = clk_get_by_index(dev, i, clk);
>>>> +		if (ret < 0)
>>>> +			break;
>>>> +
>>>> +		if (clk_enable(clk)) {
>>>> +			error("failed to enable ohci_clock %d\n", i);
>>>> +			clk_free(clk);
>>>> +			goto clk_err;
>>>> +		}
>>>> +		clk_free(clk);
>>>> +
>>>> +		/*
>>>> +		 * add enabled clocks into clks list in order to be disabled
>>>> +		 * later on ohci_usb_remove() call or in error path if needed
>>>> +		 */
>>>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
>>>
>>> Can't you just embed one structure into the other ?
>>>
>>>> +		if (!ohci_clock) {
>>>> +			error("Can't allocate resource\n");
>>>> +			goto clk_err;
>>>> +		}
>>>> +		ohci_clock->clk = clk;
>>>> +		list_add(&ohci_clock->list, &priv->clks);
>>>> +	}
>>>>    
>>>>    	return ohci_register(dev, regs);
>>>> +
>>>> +clk_err:
>>>> +	return ohci_release_clocks(priv);
>>>>    }
>>>>    
>>>>    static int ohci_usb_remove(struct udevice *dev)
>>>>    {
>>>> -	return ohci_deregister(dev);
>>>> +	struct generic_ohci *priv = dev_get_priv(dev);
>>>> +	int ret;
>>>> +
>>>> +	ret = ohci_deregister(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return ohci_release_clocks(priv);
>>>>    }
>>>>    
>>>>    static const struct udevice_id ohci_usb_ids[] = {
>>>>
>>>
> 
> 

[U-Boot] [PATCH v3 5/7] usb: host: ohci-generic: add CLOCK support

[Marek Vasut]

On 05/19/2017 02:16 PM, Patrice CHOTARD wrote:
> On 05/19/2017 01:51 PM, Marek Vasut wrote:
>> On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:
>>> Hi Marek
>>>
>>> On 05/18/2017 11:29 AM, Marek Vasut wrote:
>>>> On 05/17/2017 03:34 PM, patrice.chotard at st.com wrote:
>>>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>>>
>>>>> use list to save reference to enabled clocks in order to
>>>>> disabled them in case of error during probe() or
>>>>> during driver removal.
>>>>>
>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>>> ---
>>>>>
>>>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>>>>> 	_ keep enabled clocks reference in list in order to
>>>>> 	  disable clocks in error path or in .remove callback
>>>>>
>>>>> v2:	_ add error path management
>>>>> 	_ add .remove callback
>>>>>
>>>>>    drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 80 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>>>>> index f3307f4..a6d89a8 100644
>>>>> --- a/drivers/usb/host/ohci-generic.c
>>>>> +++ b/drivers/usb/host/ohci-generic.c
>>>>> @@ -5,6 +5,7 @@
>>>>>     */
>>>>>    
>>>>>    #include <common.h>
>>>>> +#include <clk.h>
>>>>>    #include <dm.h>
>>>>>    #include "ohci.h"
>>>>>    
>>>>> @@ -12,20 +13,98 @@
>>>>>    # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>>>>    #endif
>>>>>    
>>>>> +struct ohci_clock {
>>>>> +	struct clk *clk;
>>>>> +	struct list_head list;
>>>>> +};
>>>>> +
>>>>>    struct generic_ohci {
>>>>>    	ohci_t ohci;
>>>>> +	struct list_head clks;
>>>>>    };
>>>>>    
>>>>> +static int ohci_release_clocks(struct generic_ohci *priv)
>>>>> +{
>>>>> +	struct ohci_clock *ohci_clock, *tmp;
>>>>> +	struct clk *clk;
>>>>> +	int ret;
>>>>> +
>>>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>>>>> +		clk = ohci_clock->clk;
>>>>> +
>>>>> +		clk_request(clk->dev, clk);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		clk_disable(clk);
>>>>> +
>>>>> +		ret = clk_free(clk);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		list_del(&ohci_clock->list);
>>>>> +	}
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>    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;
>>>>> +
>>>>> +	INIT_LIST_HEAD(&priv->clks);
>>>>> +
>>>>> +	for (i = 0; ; i++) {
>>>>> +		struct ohci_clock *ohci_clock;
>>>>> +		struct clk *clk;
>>>>> +
>>>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
>>>>
>>>> Since you know how many entries the clock phandle has, you can allocate
>>>> an array and drop this while list handling and this per-element kmalloc,
>>>> which fragments the allocator pool.
>>>
>>> I disagree, at this point we don't know how many entries the clock
>>> phandle has.
>>
>> I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,
>> which is already better.
> 
> Can you elaborate ?
> 
>>
>>> But, following your idea to avoid allocator pool fragmentation, in order
>>> to get the phandle number for array allocation, i can, for example add a
>>> new fdt API :
>>>
>>> int fdtdec_get_phandle_nb(const void *blob, int src_node,
>>> 				   const char *list_name,
>>> 				   const char *cells_name,
>>> 				   int cell_count,
>>> 				   int phandle_nb);
>>
>> Uh, why ?
>>
>>> Then, with phandle_nb,, we 'll be able to allocate the right array size
>>> for clocks and resets before populating them.
>>
>> Just query the number of elements up front and then allocate the array ?
> 
> Can you indicate me with which API please ?

fdt.*count() or somesuch .

-- 
Best regards,
Marek Vasut

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

[Simon Glass]

Hi Patrice,

On 17 May 2017 at 07:34,  <patrice.chotard@st.com> wrote:

In the cover letter you could explain the purpose for this series.

Regards,
Simon

> From: Patrice Chotard <patrice.chotard@st.com>
>
> v3:     _ keep enabled clocks and deasserted resets reference in list in order to
>           disable clock or assert resets in error path or in .remove callback
>         _ add missing commit message
>         _ use struct generic_ehci * instead of struct udevice * as parameter for
>           ehci_release_resets() and ehci_release_clocks()
>         _ test return value on generic_phy_get_by_index() and
>           generic_phy_init()
>         _ split previous patch 5 in 3 independant patch for CLOCK, RESET and PHY support
>
> 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 (7):
>   reset: add reset_request()
>   usb: host: ehci-generic: replace printf() by error()
>   usb: host: ehci-generic: add error path and .remove callback
>   usb: host: ehci-generic: add generic PHY support
>   usb: host: ohci-generic: add CLOCK support
>   usb: host: ohci-generic: add RESET support
>   usb: host: ohci-generic: add generic PHY support
>
>  drivers/reset/reset-uclass.c    |   9 ++
>  drivers/usb/host/ehci-generic.c | 189 ++++++++++++++++++++++++++++++++++++----
>  drivers/usb/host/ohci-generic.c | 184 +++++++++++++++++++++++++++++++++++++-
>  include/reset.h                 |   9 ++
>  4 files changed, 374 insertions(+), 17 deletions(-)
>
> --
> 1.9.1
>

[U-Boot] [PATCH v3 2/7] usb: host: ehci-generic: replace printf() by error()

[Simon Glass]

On 17 May 2017 at 07:34,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> This allows to get file, line and function location
> of the current error message.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>

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

[U-Boot] [PATCH v3 4/7] usb: host: ehci-generic: add generic PHY support

[Simon Glass]

On 17 May 2017 at 07:34,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Extend ehci-generic driver with generic PHY framework
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v3:     _ test return value on generic_phy_get_by_index() and
>           generic_phy_init()
>
>  drivers/usb/host/ehci-generic.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>

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

[U-Boot] [PATCH v3 7/7] usb: host: ohci-generic: add generic PHY support

[Simon Glass]

On 17 May 2017 at 07:34,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Extend ohci-generic driver with generic PHY framework
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v3:     _ extract in this patch the PHY support add-on from previous patch 5
>
>
>  drivers/usb/host/ohci-generic.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)

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

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

[Patrice CHOTARD]

Hi Simon

On 05/22/2017 10:26 PM, Simon Glass wrote:
> Hi Patrice,
> 
> On 17 May 2017 at 07:34,  <patrice.chotard@st.com> wrote:
> 
> In the cover letter you could explain the purpose for this series.

ok i will add a comment about this

Thanks

> 
> Regards,
> Simon
> 
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> v3:     _ keep enabled clocks and deasserted resets reference in list in order to
>>            disable clock or assert resets in error path or in .remove callback
>>          _ add missing commit message
>>          _ use struct generic_ehci * instead of struct udevice * as parameter for
>>            ehci_release_resets() and ehci_release_clocks()
>>          _ test return value on generic_phy_get_by_index() and
>>            generic_phy_init()
>>          _ split previous patch 5 in 3 independant patch for CLOCK, RESET and PHY support
>>
>> 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 (7):
>>    reset: add reset_request()
>>    usb: host: ehci-generic: replace printf() by error()
>>    usb: host: ehci-generic: add error path and .remove callback
>>    usb: host: ehci-generic: add generic PHY support
>>    usb: host: ohci-generic: add CLOCK support
>>    usb: host: ohci-generic: add RESET support
>>    usb: host: ohci-generic: add generic PHY support
>>
>>   drivers/reset/reset-uclass.c    |   9 ++
>>   drivers/usb/host/ehci-generic.c | 189 ++++++++++++++++++++++++++++++++++++----
>>   drivers/usb/host/ohci-generic.c | 184 +++++++++++++++++++++++++++++++++++++-
>>   include/reset.h                 |   9 ++
>>   4 files changed, 374 insertions(+), 17 deletions(-)
>>
>> --
>> 1.9.1
>>