Skip to contentSkip to conversation

usb: Extend ehci and ohci generic drivers

PATCH Superseded patrice.chotard at st.com 4 participants 24 messages Jun 20, 2017 – Jun 21, 2017

Subsystems (5)
USB Common clk Reset controller framework USB ehci driver +1 more subsystems

v7 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

patrice.chotard at st.com

author
#0 lore raw

v7 cover usb: Extend ehci and ohci generic drivers

From: Patrice Chotard <[email protected]>

This series improves generic ehci and ohci drivers by addition of :
_ error path during probe (clocks, resets and phy release) _ .remove callback
_ add generic PHY framework for both generic ehci and ohci drivers _ add RESET and CLOCK framework for generic ohci driver

To implement these features, some new methods are needed in reset, clock and in dm/core framework:
_ add reset_request() and reset_assert_all() methods in RESET framework _ add clk_count() and clk_disable_all() methods in CLOCK framework _ add ofnode_count_phandle_with_args() in dm/core

v7: _ replace clk_count() and reset_count() methods by
ofnode_count_phandle_with_args() in patches 3, 4 and 5

v6: _ replace clk_get_by_index() by dev_read_phandle_with_args() in
clk_count() in patch 4
_ add Reviewed-by Simon Glass for patch 2 and 5

v5: _ rebase on top of dm/master requested by Simon Glass in order to use
livetree update
_ replace fdtdec_parse_phandle_with_args() by dev_read_phandle_with_args() in patch 2

v4: _ add clk_disable_all() and reset_assert_all() methods into CLOCK and
RESET framework as suggested by Simon Glass and Marek Vasut
_ add reset_count() and clk_count() methods which returns respectively the
number of resets and clocks declared into "resets" and "clocks" DT properties. This allows to allocate the right amount of memory to keep resets and clocks reference
_ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.

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 (10):
reset: add reset_request()
reset: add reset_assert_all()
clk: add clk_disable_all()
dm: core: add ofnode_count_phandle_with_args()
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/clk/clk-uclass.c        |  22 ++++++
 drivers/core/of_access.c        |   7 ++
 drivers/core/ofnode.c           |  12 ++++
 drivers/reset/reset-uclass.c    |  31 ++++++++
 drivers/usb/host/ehci-generic.c | 153 ++++++++++++++++++++++++++++++++++------
 drivers/usb/host/ohci-generic.c | 132 +++++++++++++++++++++++++++++++++-
 include/clk.h                   |  10 +++
 include/dm/of_access.h          |  18 +++++
 include/dm/ofnode.h             |  17 +++++
 include/reset.h                 |  26 +++++++
 10 files changed, 403 insertions(+), 25 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v7 1/10 reset: add reset_request()

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v7: _ none
v6: _ none
v5: _ none
v4: _ none
v3: _ none
v2: _ none

 drivers/reset/reset-uclass.c | 9 +++++++++
 include/reset.h              | 9 +++++++++
 2 files changed, 18 insertions(+)
drivers/reset/reset-uclass.c +9 -0
index de3695f..4fd82b9 100644
@@ -97,6 +97,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);
include/reset.h +9 -0
index f45fcf8..4f2e35f 100644
@@ -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
Signature · 1 line
1.9.1
author
#2 lore raw

PATCH v7 2/10 reset: add reset_assert_all()

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Add reset_assert_all() method which Request/Assert/Free an array of resets signal that has been previously successfully requested by reset_get_by_*()

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v7: _ none
v6: _ none
v5: _ none
v4: _ add reset_assert_all() method as suggested by Marek Vasut
and Simon Glass

 drivers/reset/reset-uclass.c | 22 ++++++++++++++++++++++
 include/reset.h              | 17 +++++++++++++++++
 2 files changed, 39 insertions(+)
drivers/reset/reset-uclass.c +22 -0
index 4fd82b9..c39ce14 100644
@@ -133,6 +133,28 @@ int reset_deassert(struct reset_ctl *reset_ctl)
 	return ops->rst_deassert(reset_ctl);
 }
 
+int reset_assert_all(struct reset_ctl *reset_ctl, int count)
+{
+	int i, ret;
+
+	for (i = 0; i < count; i++) {
+		debug("%s(reset_ctl[%d]=%p)\n", __func__, i, &reset_ctl[i]);
+
+		ret = reset_request(&reset_ctl[i]);
+		if (ret)
+			return ret;
+
+		ret = reset_assert(&reset_ctl[i]);
+		if (ret)
+			return ret;
+
+		ret = reset_free(&reset_ctl[i]);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 UCLASS_DRIVER(reset) = {
 	.id		= UCLASS_RESET,
 	.name		= "reset",
include/reset.h +17 -0
index 4f2e35f..2678257 100644
@@ -144,6 +144,17 @@ int reset_assert(struct reset_ctl *reset_ctl);
  */
 int reset_deassert(struct reset_ctl *reset_ctl);
 
+/**
+ * reset_assert_all - Request/Assert/Free resets.
+ *
+ * This function will request, assert and free array of clocks,
+ *
+ * @reset_ctl:	A reset struct array that was previously successfully
+ *		requested by reset_get_by_*().
+ * @count	Number of reset contained in the array
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_assert_all(struct reset_ctl *reset_ctl, int count);
 #else
 static inline int reset_get_by_index(struct udevice *dev, int index,
 				     struct reset_ctl *reset_ctl)

@@ -171,6 +182,12 @@ static inline int reset_deassert(struct reset_ctl *reset_ctl)
 {
 	return 0;
 }
+
+static inline int reset_assert_all(struct reset_ctl *reset_ctl, int count)
+{
+	return 0;
+}
+
 #endif
 
 #endif
Signature · 1 line
1.9.1
author
#3 lore raw

PATCH v7 3/10 clk: add clk_disable_all()

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Add clk_disable_all() method which Request/Disable/Free an array of clocks that has been previously requested by clk_request/get_by_*()

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---

v7: _ none
v6: _ none
v5: _ none
v4: _ none
v3: _ add commit message
v2: _ create this independant path for printf() replacement

 drivers/clk/clk-uclass.c | 22 ++++++++++++++++++++++
 include/clk.h            | 10 ++++++++++
 2 files changed, 32 insertions(+)
drivers/clk/clk-uclass.c +22 -0
index 83b6328..e732514 100644
@@ -187,6 +187,28 @@ int clk_disable(struct clk *clk)
 	return ops->disable(clk);
 }
 
+int clk_disable_all(struct clk *clk, int count)
+{
+	int i, ret;
+
+	debug("%s(clk=%p count=%d)\n", __func__, clk, count);
+
+	for (i = 0; i < count; i++) {
+		ret = clk_request(clk->dev, &clk[i]);
+		if (ret && ret != -ENOSYS)
+			return ret;
+
+		ret = clk_disable(&clk[i]);
+		if (ret && ret != -ENOSYS)
+			return ret;
+
+		ret = clk_free(&clk[i]);
+		if (ret && ret != -ENOSYS)
+			return ret;
+	}
+	return 0;
+}
+
 UCLASS_DRIVER(clk) = {
 	.id		= UCLASS_CLK,
 	.name		= "clk",
include/clk.h +10 -0
index 5a5c2ff..e04c274 100644
@@ -174,6 +174,16 @@ int clk_enable(struct clk *clk);
  */
 int clk_disable(struct clk *clk);
 
+/**
+ * clk_disable_all() - Request/Disable (turn off)/Free clocks.
+ *
+ * @clk:	A clock struct array that was previously successfully
+ *		requested by clk_request/get_by_*().
+ * @count	Number of clock contained in the array
+ * @return zero on success, or -ve error code.
+ */
+int clk_disable_all(struct clk *clk, int count);
+
 int soc_clk_dump(void);
 
 #endif
Signature · 1 line
1.9.1
#4 lore raw

PATCH v7 3/10 clk: add clk_disable_all()

Hi,

Quoted 28 lines · On Tue, 20 Jun 2017 11:59:04 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

Add clk_disable_all() method which Request/Disable/Free an array of clocks that has been previously requested by clk_request/get_by_*()

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---

v7: _ none
v6: _ none
v5: _ none
v4: _ none
v3: _ add commit message
v2: _ create this independant path for printf() replacement

 drivers/clk/clk-uclass.c | 22 ++++++++++++++++++++++
 include/clk.h            | 10 ++++++++++
 2 files changed, 32 insertions(+)
drivers/clk/clk-uclass.c +0 -0
index 83b6328..e732514 100644
@@ -187,6 +187,28 @@ int clk_disable(struct clk *clk)
 	return ops->disable(clk);
 }

drivers/clk/clk-uclass.c int clk_disable(struct clk *clk) · line 198

Quoted 10 lines
drivers/clk/clk-uclass.c +8 -0
 
+int clk_disable_all(struct clk *clk, int count)
+{
+	int i, ret;
+
+	debug("%s(clk=%p count=%d)\n", __func__, clk, count);
+
+	for (i = 0; i < count; i++) {
+		ret = clk_request(clk->dev, &clk[i]);

Shouldn't this be:

		ret = clk_request(clk[i].dev, &clk[i]);

Lothar Waßmann

author
#5 lore raw

PATCH v7 3/10 clk: add clk_disable_all()

Hi Lothar

Quoted 42 lines · Lothar Waßmann

Hi,

Quoted 28 lines · On Tue, 20 Jun 2017 11:59:04 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

Add clk_disable_all() method which Request/Disable/Free an array of clocks that has been previously requested by clk_request/get_by_*()

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---

v7: _ none
v6: _ none
v5: _ none
v4: _ none
v3: _ add commit message
v2: _ create this independant path for printf() replacement

  drivers/clk/clk-uclass.c | 22 ++++++++++++++++++++++
  include/clk.h            | 10 ++++++++++
  2 files changed, 32 insertions(+)
drivers/clk/clk-uclass.c +0 -0
index 83b6328..e732514 100644
@@ -187,6 +187,28 @@ int clk_disable(struct clk *clk)
  	return ops->disable(clk);
  }

drivers/clk/clk-uclass.c int clk_disable(struct clk *clk) · line 198

Quoted 10 lines
drivers/clk/clk-uclass.c +8 -0
 
+int clk_disable_all(struct clk *clk, int count)
+{
+	int i, ret;
+
+	debug("%s(clk=%p count=%d)\n", __func__, clk, count);
+
+	for (i = 0; i < count; i++) {
+		ret = clk_request(clk->dev, &clk[i]);

Shouldn't this be:

Quoted 1 line
		ret = clk_request(clk[i].dev, &clk[i]);

Yes, i will fix it

Thanks

Patrice

Quoted 5 lines

Lothar Waßmann

author
#6 lore raw

PATCH v7 4/10 dm: core: add ofnode_count_phandle_with_args()

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

This function is usefull to get phandle number contained in a property list.
For example, this allows to allocate the right amount of memory to keep clock's reference contained into the "clocks" property.

To implement it, either of_count_phandle_with_args() or fdtdec_parse_phandle_with_args() are used respectively for live tree and flat tree.
By passing index = -1, these 2 functions returns the number of phandle contained into the property list.

Signed-off-by Patrice Chotard <[email protected]>

---

v7: _ add ofnode_count_phandle_with_args() which returns
the phandle number contained into a property list.

 drivers/core/of_access.c |  7 +++++++
 drivers/core/ofnode.c    | 12 ++++++++++++
 include/dm/of_access.h   | 18 ++++++++++++++++++
 include/dm/ofnode.h      | 17 +++++++++++++++++
 4 files changed, 54 insertions(+)
drivers/core/of_access.c +7 -0
index 93a6560..76d5996 100644
@@ -641,6 +641,13 @@ int of_parse_phandle_with_args(const struct device_node *np,
 					    index, out_args);
 }
 
+int of_count_phandle_with_args(const struct device_node *np,
+			       const char *list_name, const char *cells_name)
+{
+	return __of_parse_phandle_with_args(np, list_name, cells_name, 0,
+					    -1, NULL);
+}
+
 static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 			 int id, const char *stem, int stem_len)
 {
drivers/core/ofnode.c +12 -0
index ac312d6..02bc5d2 100644
@@ -307,6 +307,18 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name,
 	return 0;
 }
 
+int ofnode_count_phandle_with_args(ofnode node, const char *list_name,
+				   const char *cells_name)
+{
+	if (ofnode_is_np(node))
+		return of_count_phandle_with_args(ofnode_to_np(node),
+				list_name, cells_name);
+	else
+		return fdtdec_parse_phandle_with_args(gd->fdt_blob,
+				ofnode_to_offset(node), list_name, cells_name,
+				0, -1, NULL);
+}
+
 ofnode ofnode_path(const char *path)
 {
 	if (of_live_active())
include/dm/of_access.h +18 -0
index 142f0f4..5be29b1 100644
@@ -315,6 +315,24 @@ int of_parse_phandle_with_args(const struct device_node *np,
 			       int index, struct of_phandle_args *out_args);
 
 /**
+ * of_count_phandle_with_args() - Count the number of phandle in a list
+ *
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @return number of phandle found, -ENOENT if
+ *	@list_name does not exist, -EINVAL if a phandle was not found,
+ *	@cells_name could not be found, the arguments were truncated or there
+ *	were too many arguments.
+ *
+ * Returns number of phandle found on success, on error returns appropriate
+ * errno value.
+ *
+ */
+int of_count_phandle_with_args(const struct device_node *np,
+			       const char *list_name, const char *cells_name);
+
+/**
  * of_alias_scan() - Scan all properties of the 'aliases' node
  *
  * The function scans all the properties of the 'aliases' node and populates
include/dm/ofnode.h +17 -0
index 149622a..3f2cfbb 100644
@@ -423,6 +423,23 @@ int ofnode_parse_phandle_with_args(ofnode node, const char *list_name,
 				   struct ofnode_phandle_args *out_args);
 
 /**
+ * ofnode_count_phandle_with_args() - Count number of phandle in a list
+ *
+ * This function is useful to count phandles into a list.
+ * Returns number of phandle on success, on error returns appropriate
+ * errno value.
+ *
+ * @node:	device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @return number of phandle on success, -ENOENT if @list_name does not
+ *      exist, -EINVAL if a phandle was not found, @cells_name could not
+ *      be found.
+ */
+int ofnode_count_phandle_with_args(ofnode node, const char *list_name,
+				   const char *cells_name);
+
+/**
  * ofnode_path() - find a node by full path
  *
  * @path: Full path to node, e.g. "/bus/spi at 1"
Signature · 1 line
1.9.1
#7 lore raw

PATCH v7 4/10 dm: core: add ofnode_count_phandle_with_args()

Quoted 21 lines · On 20 June 2017 at 03:59, <[email protected]> wrote:

From: Patrice Chotard <[email protected]>

This function is usefull to get phandle number contained in a property list.
For example, this allows to allocate the right amount of memory to keep clock's reference contained into the "clocks" property.

To implement it, either of_count_phandle_with_args() or fdtdec_parse_phandle_with_args() are used respectively for live tree and flat tree.
By passing index = -1, these 2 functions returns the number of phandle contained into the property list.

Signed-off-by Patrice Chotard <[email protected]>

---

v7: _ add ofnode_count_phandle_with_args() which returns
the phandle number contained into a property list.

Quoted 5 lines
 drivers/core/of_access.c |  7 +++++++
 drivers/core/ofnode.c    | 12 ++++++++++++
 include/dm/of_access.h   | 18 ++++++++++++++++++
 include/dm/ofnode.h      | 17 +++++++++++++++++
 4 files changed, 54 insertions(+)
Reviewed-by Simon Glass <[email protected]>
author
#8 lore raw

PATCH v7 5/10 usb: host: ehci-generic: replace printf() by error()

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

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

Signed-off-by patrice chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v7: _ none
v6: _ none
v5: _ none
v4: _ none
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(-)
drivers/usb/host/ehci-generic.c +2 -2
index fb78462..2116ae1 100644
@@ -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);
 	}
Signature · 1 line
1.9.1
author
#9 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
 1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +102 -24
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
 #include <dm.h>
 #include "ehci.h"
 
+#include <dm/ofnode.h>
+
 /*
  * 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 +20,119 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
 };
 
 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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+
+			if (err < 0)
+				break;
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+			priv->clock_count++;
 
-		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);
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
 	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}
 	hccr = map_physmem(devfdt_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);
+	err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!err)
+		return err;
+
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
+}
+
+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 =  reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
 }
 
 static const struct udevice_id ehci_usb_ids[] = {

@@ -67,7 +145,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,
Signature · 1 line
1.9.1
#10 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

Quoted 7 lines · patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

Nitpicks below

Quoted 61 lines

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
 1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +7 -12
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
 #include <dm.h>
 #include "ehci.h"
 
+#include <dm/ofnode.h>
+
 /*
  * 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 +20,119 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
 };
 
 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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);

drivers/usb/host/ehci-generic.c line 42

Quoted 10 lines
drivers/usb/host/ehci-generic.c +9 -1
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);

devm_kzalloc()

drivers/usb/host/ehci-generic.c line 44

Quoted 2 lines
drivers/usb/host/ehci-generic.c +2 -0
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");

If you run out of memory, you're screwed anyway, drop this print.

Quoted 56 lines
drivers/usb/host/ehci-generic.c +43 -9
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+
+			if (err < 0)
+				break;
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+			priv->clock_count++;
 
-		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);
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
 	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);

drivers/usb/host/ehci-generic.c line 101

Quoted 10 lines
drivers/usb/host/ehci-generic.c +10 -0
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}

Newline

Quoted 48 lines
drivers/usb/host/ehci-generic.c +31 -2
 	hccr = map_physmem(devfdt_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);
+	err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!err)
+		return err;
+
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
+}
+
+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 =  reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
 }
 
 static const struct udevice_id ehci_usb_ids[] = {

@@ -67,7 +145,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,
Signature · 2 lines
Best regards,
Marek Vasut
#11 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

Hi,

Quoted 76 lines · On Tue, 20 Jun 2017 12:09:22 +0200 Marek Vasut wrote:
Quoted 7 lines · patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

Nitpicks below

Quoted 65 lines

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
 1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +10 -13
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
 #include <dm.h>
 #include "ehci.h"
 
+#include <dm/ofnode.h>
+
 /*
  * 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 +20,119 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
 };
 
 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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
Quoted 9 lines

drivers/usb/host/ehci-generic.c line 42

Quoted 6 lines
drivers/usb/host/ehci-generic.c +6 -0
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);

devm_kzalloc()

devm_kcalloc(dev, clock_nb, sizeof()...)

Lothar Waßmann

author
#12 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

In reply to Marek Vasut

Hi Marek

Quoted 76 lines · Marek Vasut
Quoted 7 lines · patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

Nitpicks below

Quoted 65 lines

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
  1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +10 -13
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
  #include <dm.h>
  #include "ehci.h"
 
+#include <dm/ofnode.h>
+
  /*
   * 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 +20,119 @@
   */
  struct generic_ehci {
  	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
  };
 
  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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
Quoted 8 lines

drivers/usb/host/ehci-generic.c line 42

Quoted 6 lines
drivers/usb/host/ehci-generic.c +6 -0
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);

devm_kzalloc()

Ok

Quoted 5 lines

drivers/usb/host/ehci-generic.c line 44

Quoted 3 lines
drivers/usb/host/ehci-generic.c +2 -0
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");

If you run out of memory, you're screwed anyway, drop this print.

Ok, i will remove it

Quoted 61 lines
Quoted 61 lines
drivers/usb/host/ehci-generic.c +47 -9
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+
+			if (err < 0)
+				break;
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+			priv->clock_count++;
 
-		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);
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
  	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
Quoted 8 lines

drivers/usb/host/ehci-generic.c line 101

Quoted 6 lines
drivers/usb/host/ehci-generic.c +6 -0
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}

Newline

ok

Quoted 51 lines
Quoted 49 lines
drivers/usb/host/ehci-generic.c +31 -2
  	hccr = map_physmem(devfdt_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);
+	err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!err)
+		return err;
+
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
+}
+
+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 =  reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
  }
 
  static const struct udevice_id ehci_usb_ids[] = {

@@ -67,7 +145,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,
#13 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

In reply to patrice.chotard at st.com

Hi,

Quoted 94 lines · On Tue, 20 Jun 2017 11:59:07 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
 1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +27 -16
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
 #include <dm.h>
 #include "ehci.h"
 
+#include <dm/ofnode.h>
+
 /*
  * 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 +20,119 @@
  */
 struct generic_ehci {
 	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
 };
 
 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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+
+			if (err < 0)
+				break;
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+			priv->clock_count++;

drivers/usb/host/ehci-generic.c line 59

Quoted 10 lines
drivers/usb/host/ehci-generic.c +3 -6
-		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);
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);

You free the clock here and again with clk_disable_all() in the error path. Also you do not have a valid error code in 'err' which is returned at the end of the function!

Quoted 47 lines
drivers/usb/host/ehci-generic.c +41 -0
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
 	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}
 	hccr = map_physmem(devfdt_get_addr(dev), 0x100, MAP_NOCACHE);
 	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
 				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));

drivers/usb/host/ehci-generic.c line 137

Quoted 10 lines
drivers/usb/host/ehci-generic.c +8 -1
-	return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!err)
+		return err;
+
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;

You should do the clock cleanup even though the reset cleanup failed.

drivers/usb/host/ehci-generic.c line 137

Quoted 5 lines
drivers/usb/host/ehci-generic.c +4 -0
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;

You should return the error code of the operation that failed initially (err), not some error code that has occured during cleanup.

Quoted 21 lines
drivers/usb/host/ehci-generic.c +17 -0
+	return err;
+}
+
+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 =  reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
 }
 
 static const struct udevice_id ehci_usb_ids[] = {

drivers/usb/host/ehci-generic.c U_BOOT_DRIVER(ehci_generic) = { · line 151

Quoted 8 lines
drivers/usb/host/ehci-generic.c +1 -1
@@ -67,7 +145,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,

Lothar Waßmann

author
#14 lore raw

PATCH v7 6/10 usb: host: ehci-generic: add error path and .remove callback

Hi Lothar

Quoted 97 lines · Lothar Waßmann

Hi,

Quoted 94 lines · On Tue, 20 Jun 2017 11:59:07 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

Use an array to save enabled clocks reference 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 <[email protected]>

---

v7: _ replace clk_count() and reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 125 ++++++++++++++++++++++++++++++++--------
  1 file changed, 101 insertions(+), 24 deletions(-)
drivers/usb/host/ehci-generic.c +27 -16
index 2116ae1..388b242 100644
@@ -11,6 +11,8 @@
  #include <dm.h>
  #include "ehci.h"
 
+#include <dm/ofnode.h>
+
  /*
   * 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 +20,119 @@
   */
  struct generic_ehci {
  	struct ehci_ctrl ctrl;
+	struct clk *clocks;
+	struct reset_ctl *resets;
+	int clock_count;
+	int reset_count;
  };
 
  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;
-
-	for (i = 0; ; i++) {
-		struct clk clk;
-		int ret;
-
-		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);
-	}
+	int i, err, ret, clock_nb, reset_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+
+			if (err < 0)
+				break;
 
-	for (i = 0; ; i++) {
-		struct reset_ctl reset;
-		int ret;
+			priv->clock_count++;
Quoted 13 lines

drivers/usb/host/ehci-generic.c line 69

Quoted 10 lines
drivers/usb/host/ehci-generic.c +3 -6
-		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);
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);

You free the clock here and again with clk_disable_all() in the error path. Also you do not have a valid error code in 'err' which is returned at the end of the function!

Good catch, to fix it, i will move the priv->clock_count++ after the clk_enable statement. In this case, in the error path, only clocks which have been previously correctly enabled will be requested/disabled/freed

Ok i will catch the error code in err and use it as return value.

Quoted 48 lines
Quoted 48 lines
drivers/usb/host/ehci-generic.c +41 -0
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
  	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}
  	hccr = map_physmem(devfdt_get_addr(dev), 0x100, MAP_NOCACHE);
  	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
  				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
Quoted 11 lines

drivers/usb/host/ehci-generic.c line 137

Quoted 10 lines
drivers/usb/host/ehci-generic.c +8 -1
-	return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
+	if (!err)
+		return err;
+
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;

You should do the clock cleanup even though the reset cleanup failed.

Agree, i will fix it

Quoted 8 lines

drivers/usb/host/ehci-generic.c line 137

Quoted 6 lines
drivers/usb/host/ehci-generic.c +4 -0
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;

You should return the error code of the operation that failed initially (err), not some error code that has occured during cleanup.

Agree again

Thanks

Patrice

Quoted 34 lines
Quoted 22 lines
drivers/usb/host/ehci-generic.c +17 -0
+	return err;
+}
+
+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 =  reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
  }
 
  static const struct udevice_id ehci_usb_ids[] = {

drivers/usb/host/ehci-generic.c U_BOOT_DRIVER(ehci_generic) = { · line 151

Quoted 8 lines
drivers/usb/host/ehci-generic.c +1 -1
@@ -67,7 +145,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,

Lothar Waßmann

author
#15 lore raw

PATCH v7 7/10 usb: host: ehci-generic: add generic PHY support

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Extend ehci-generic driver with generic PHY framework

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---

v7: _ none

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets and enabled
clocks reference list. Replace lists by arrays.
_ usage of new RESET and CLOCK methods clk_count(), reset_count(),
reset_assert_all() and clk_disable_all().

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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
drivers/usb/host/ehci-generic.c +29 -0
index 388b242..00a7772 100644
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <clk.h>
+#include <generic-phy.h>
 #include <reset.h>
 #include <asm/io.h>
 #include <dm.h>

@@ -22,6 +23,7 @@ struct generic_ehci {
 	struct ehci_ctrl ctrl;
 	struct clk *clocks;
 	struct reset_ctl *resets;
+	struct phy phy;
 	int clock_count;
 	int reset_count;
 };

@@ -99,6 +101,21 @@ static int ehci_usb_probe(struct udevice *dev)
 			goto clk_err;
 		}
 	}
+
+	err = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (err) {
+		if (err != -ENOENT) {
+			error("failed to get usb phy\n");
+			goto reset_err;
+		}
+	} else {
+		err = generic_phy_init(&priv->phy);
+		if (err) {
+			error("failed to init usb phy\n");
+			goto reset_err;
+		}
+	}
+
 	hccr = map_physmem(devfdt_get_addr(dev), 0x100, MAP_NOCACHE);
 	hcor = (struct ehci_hcor *)((uintptr_t)hccr +
 				    HC_LENGTH(ehci_readl(&hccr->cr_capbase)));

@@ -107,6 +124,12 @@ static int ehci_usb_probe(struct udevice *dev)
 	if (!err)
 		return err;
 
+	if (generic_phy_valid(&priv->phy)) {
+		ret = generic_phy_exit(&priv->phy);
+		if (ret)
+			return ret;
+	}
+
 reset_err:
 	ret = reset_assert_all(priv->resets, priv->reset_count);
 	if (ret)

@@ -128,6 +151,12 @@ static int ehci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (generic_phy_valid(&priv->phy)) {
+		ret = generic_phy_exit(&priv->phy);
+		if (ret)
+			return ret;
+	}
+
 	ret =  reset_assert_all(priv->resets, priv->reset_count);
 	if (ret)
 		return ret;
Signature · 1 line
1.9.1
author
#16 lore raw

PATCH v7 8/10 usb: host: ohci-generic: add CLOCK support

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace clk_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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 | 59 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)
drivers/usb/host/ohci-generic.c +58 -2
index f85738f..f76a1c2 100644
@@ -5,27 +5,83 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include "ohci.h"
 
+#include <dm/ofnode.h>
+
 #if !defined(CONFIG_USB_OHCI_NEW)
 # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
 #endif
 
 struct generic_ohci {
 	ohci_t ohci;
+	struct clk *clocks;
+	int clock_count;
 };
 
 static int ohci_usb_probe(struct udevice *dev)
 {
 	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, err, ret, clock_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+			if (err < 0)
+				break;
+
+			priv->clock_count++;
+
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);
+		}
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
+	}
 
-	return ohci_register(dev, regs);
+	err = ohci_register(dev, regs);
+	if (!err)
+		return err;
+
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
 }
 
 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 clk_disable_all(priv->clocks, priv->clock_count);
 }
 
 static const struct udevice_id ohci_usb_ids[] = {
Signature · 1 line
1.9.1
#17 lore raw

PATCH v7 8/10 usb: host: ohci-generic: add CLOCK support

Hi,

Quoted 72 lines · On Tue, 20 Jun 2017 11:59:09 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace clk_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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 | 59 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)
drivers/usb/host/ohci-generic.c +24 -0
index f85738f..f76a1c2 100644
@@ -5,27 +5,83 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include "ohci.h"
 
+#include <dm/ofnode.h>
+
 #if !defined(CONFIG_USB_OHCI_NEW)
 # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
 #endif
 
 struct generic_ohci {
 	ohci_t ohci;
+	struct clk *clocks;
+	int clock_count;
 };
 
 static int ohci_usb_probe(struct udevice *dev)
 {
 	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, err, ret, clock_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+			if (err < 0)
+				break;

drivers/usb/host/ohci-generic.c line 55

Quoted 10 lines
drivers/usb/host/ohci-generic.c +9 -0
+
+			priv->clock_count++;
+
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);

You free the freshly allocated clock right away and use it lateron?

drivers/usb/host/ohci-generic.c line 66

Quoted 12 lines
drivers/usb/host/ohci-generic.c +10 -1
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
+	}
 
-	return ohci_register(dev, regs);
+	err = ohci_register(dev, regs);
+	if (!err)
+		return err;
+

	if (err)
		goto clk_err;
	return 0;

would be more easy to follow.

Quoted 12 lines
drivers/usb/host/ohci-generic.c +7 -1
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
 }
 
 static int ohci_usb_remove(struct udevice *dev)
 {
-	return ohci_deregister(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);

drivers/usb/host/ohci-generic.c line 87

Quoted 10 lines
drivers/usb/host/ohci-generic.c +7 -0
+	int ret;
+
+	ret = ohci_deregister(dev);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
 }
 
 static const struct udevice_id ohci_usb_ids[] = {

Lothar Waßmann

author
#18 lore raw

PATCH v7 8/10 usb: host: ohci-generic: add CLOCK support

Hi Lothar

Quoted 78 lines · Lothar Waßmann

Hi,

Quoted 75 lines · On Tue, 20 Jun 2017 11:59:09 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace clk_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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 | 59 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 57 insertions(+), 2 deletions(-)
drivers/usb/host/ohci-generic.c +27 -0
index f85738f..f76a1c2 100644
@@ -5,27 +5,83 @@
   */
 
  #include <common.h>
+#include <clk.h>
  #include <dm.h>
  #include "ohci.h"
 
+#include <dm/ofnode.h>
+
  #if !defined(CONFIG_USB_OHCI_NEW)
  # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
  #endif
 
  struct generic_ohci {
  	ohci_t ohci;
+	struct clk *clocks;
+	int clock_count;
  };
 
  static int ohci_usb_probe(struct udevice *dev)
  {
  	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, err, ret, clock_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+			if (err < 0)
+				break;
+
+			priv->clock_count++;
+
Quoted 8 lines

drivers/usb/host/ohci-generic.c line 62

Quoted 7 lines
drivers/usb/host/ohci-generic.c +6 -0
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);

You free the freshly allocated clock right away and use it lateron?

clk_free() didn't free clock resources, it's the opposite of clk_request(), see comments in include.clk.h

Quoted 16 lines

drivers/usb/host/ohci-generic.c line 66

Quoted 13 lines
drivers/usb/host/ohci-generic.c +10 -1
+	} else {
+		if (clock_nb != -ENOENT) {
+			error("failed to get clock phandle(%d)\n", clock_nb);
+			return clock_nb;
+		}
+	}
 
-	return ohci_register(dev, regs);
+	err = ohci_register(dev, regs);
+	if (!err)
+		return err;
+

	if (err)
		goto clk_err;
	return 0;
Quoted 1 line

would be more easy to follow.

ok, i will fix it

Thanks

Patrice

Quoted 27 lines
Quoted 13 lines
drivers/usb/host/ohci-generic.c +7 -1
+clk_err:
+	ret = clk_disable_all(priv->clocks, priv->clock_count);
+	if (ret)
+		return ret;
+
+	return err;
  }
 
  static int ohci_usb_remove(struct udevice *dev)
  {
-	return ohci_deregister(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);

drivers/usb/host/ohci-generic.c line 87

Quoted 10 lines
drivers/usb/host/ohci-generic.c +7 -0
+	int ret;
+
+	ret = ohci_deregister(dev);
+	if (ret)
+		return ret;
+
+	return clk_disable_all(priv->clocks, priv->clock_count);
  }
 
  static const struct udevice_id ohci_usb_ids[] = {

Lothar Waßmann

#19 lore raw

PATCH v7 8/10 usb: host: ohci-generic: add CLOCK support

Hi,

Quoted 90 lines · On Tue, 20 Jun 2017 12:56:48 +0000 Patrice CHOTARD wrote:

Hi Lothar

Quoted 78 lines · Lothar Waßmann

Hi,

Quoted 75 lines · On Tue, 20 Jun 2017 11:59:09 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace clk_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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 | 59 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 57 insertions(+), 2 deletions(-)
drivers/usb/host/ohci-generic.c +27 -0
index f85738f..f76a1c2 100644
@@ -5,27 +5,83 @@
   */
 
  #include <common.h>
+#include <clk.h>
  #include <dm.h>
  #include "ohci.h"
 
+#include <dm/ofnode.h>
+
  #if !defined(CONFIG_USB_OHCI_NEW)
  # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
  #endif
 
  struct generic_ohci {
  	ohci_t ohci;
+	struct clk *clocks;
+	int clock_count;
  };
 
  static int ohci_usb_probe(struct udevice *dev)
  {
  	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, err, ret, clock_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+			if (err < 0)
+				break;
+
+			priv->clock_count++;
+
Quoted 8 lines

drivers/usb/host/ohci-generic.c line 62

Quoted 7 lines
drivers/usb/host/ohci-generic.c +6 -0
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);

You free the freshly allocated clock right away and use it lateron?

Quoted 3 lines

clk_free() didn't free clock resources, it's the opposite of clk_request(), see comments in include.clk.h

But as long as you are working with some resource (whether it's a clock, a gpio, a 'reset' or whatever), you should keep it requested and free it only after you relinquished using it.
Otherwise the object you are using may be destroyed underneath your feet.

Lothar Waßmann

author
#20 lore raw

PATCH v7 8/10 usb: host: ohci-generic: add CLOCK support

Hi Lothar

Quoted 93 lines · Lothar Waßmann

Hi,

Quoted 90 lines · On Tue, 20 Jun 2017 12:56:48 +0000 Patrice CHOTARD wrote:

Hi Lothar

Quoted 86 lines · Lothar Waßmann

Hi,

Quoted 72 lines · On Tue, 20 Jun 2017 11:59:09 +0200 patrice.chotard at st.com wrote:

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace clk_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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 | 59 +++++++++++++++++++++++++++++++++++++++--
   1 file changed, 57 insertions(+), 2 deletions(-)
drivers/usb/host/ohci-generic.c +24 -0
index f85738f..f76a1c2 100644
@@ -5,27 +5,83 @@
    */
 
   #include <common.h>
+#include <clk.h>
   #include <dm.h>
   #include "ohci.h"
 
+#include <dm/ofnode.h>
+
   #if !defined(CONFIG_USB_OHCI_NEW)
   # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
   #endif
 
   struct generic_ohci {
   	ohci_t ohci;
+	struct clk *clocks;
+	int clock_count;
   };
 
   static int ohci_usb_probe(struct udevice *dev)
   {
   	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, err, ret, clock_nb;
+
+	err = 0;
+	priv->clock_count = 0;
+	clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
+						  "#clock-cells");
+	if (clock_nb > 0) {
+		priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
+					    GFP_KERNEL);
+		if (!priv->clocks) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < clock_nb; i++) {
+			err = clk_get_by_index(dev, i, &priv->clocks[i]);
+			if (err < 0)
+				break;

drivers/usb/host/ohci-generic.c line 55

Quoted 10 lines
drivers/usb/host/ohci-generic.c +9 -0
+
+			priv->clock_count++;
+
+			if (clk_enable(&priv->clocks[i])) {
+				error("failed to enable clock %d\n", i);
+				clk_free(&priv->clocks[i]);
+				goto clk_err;
+			}
+			clk_free(&priv->clocks[i]);

You free the freshly allocated clock right away and use it lateron?

Quoted 8 lines
Quoted 3 lines

clk_free() didn't free clock resources, it's the opposite of clk_request(), see comments in include.clk.h

But as long as you are working with some resource (whether it's a clock, a gpio, a 'reset' or whatever), you should keep it requested and free it only after you relinquished using it.
Otherwise the object you are using may be destroyed underneath your feet.

Ah yes, my bad, i focused on the clk_free() inside the previous statement.

I will fix it

Thanks

Quoted 4 lines

Lothar Waßmann

author
#21 lore raw

PATCH v7 9/10 usb: host: ohci-generic: add RESET support

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets. Replace lists by arrays.
_ usage of new RESET methods reset_assert_all() and clk_disable_all().

v3: _ extract in this patch the RESET support add-on from previous patch 5
_ keep deasserted 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 | 46 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)
drivers/usb/host/ohci-generic.c +45 -1
index f76a1c2..5bd52ff 100644
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
+#include <reset.h>
 #include "ohci.h"
 
 #include <dm/ofnode.h>

@@ -18,14 +19,16 @@
 struct generic_ohci {
 	ohci_t ohci;
 	struct clk *clocks;
+	struct reset_ctl *resets;
 	int clock_count;
+	int reset_count;
 };
 
 static int ohci_usb_probe(struct udevice *dev)
 {
 	struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
 	struct generic_ohci *priv = dev_get_priv(dev);
-	int i, err, ret, clock_nb;
+	int i, err, ret, clock_nb, reset_nb;
 
 	err = 0;
 	priv->clock_count = 0;

@@ -60,10 +63,47 @@ static int ohci_usb_probe(struct udevice *dev)
 		}
 	}
 
+	priv->reset_count = 0;
+	reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
+						  "#reset-cells");
+	if (reset_nb > 0) {
+		priv->resets = devm_kmalloc(dev,
+					    sizeof(struct reset_ctl) * reset_nb,
+					    GFP_KERNEL);
+		if (!priv->resets) {
+			error("Can't allocate resource\n");
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < reset_nb; i++) {
+			err = reset_get_by_index(dev, i, &priv->resets[i]);
+			if (err < 0)
+				break;
+
+			priv->reset_count++;
+
+			if (reset_deassert(&priv->resets[i])) {
+				error("failed to deassert reset %d\n", i);
+				reset_free(&priv->resets[i]);
+				goto reset_err;
+			}
+			reset_free(&priv->resets[i]);
+		}
+	} else {
+		if (reset_nb != -ENOENT) {
+			error("failed to get reset phandle(%d)\n", reset_nb);
+			goto clk_err;
+		}
+	}
+
 	err = ohci_register(dev, regs);
 	if (!err)
 		return err;
 
+reset_err:
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
 clk_err:
 	ret = clk_disable_all(priv->clocks, priv->clock_count);
 	if (ret)

@@ -81,6 +121,10 @@ static int ohci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	ret = reset_assert_all(priv->resets, priv->reset_count);
+	if (ret)
+		return ret;
+
 	return clk_disable_all(priv->clocks, priv->clock_count);
 }
Signature · 1 line
1.9.1
#22 lore raw

PATCH v7 9/10 usb: host: ohci-generic: add RESET support

Quoted 21 lines · On 20 June 2017 at 03:59, <[email protected]> wrote:

From: Patrice Chotard <[email protected]>

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

Signed-off-by Patrice Chotard <[email protected]>

---
v7: _ replace reset_count() by ofnode_count_phandle_with_args()

v6: _ none

v5: _ none

v4: _ update the memory allocation for deasserted resets. Replace lists by arrays.
_ usage of new RESET methods reset_assert_all() and clk_disable_all().

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

Quoted 5 lines

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

 drivers/usb/host/ohci-generic.c | 46 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>
author
#23 lore raw

PATCH v7 10/10 usb: host: ohci-generic: add generic PHY support

In reply to patrice.chotard at st.com

From: Patrice Chotard <[email protected]>

Extend ohci-generic driver with generic PHY framework

Signed-off-by Patrice Chotard <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v7: _ none

v6: _ none

v5: _ none

v4: _ use generic_phy_valid() before generic_phy_exit() call

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

 drivers/usb/host/ohci-generic.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
drivers/usb/host/ohci-generic.c +28 -0
index 5bd52ff..63cc25e 100644
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
+#include <generic-phy.h>
 #include <reset.h>
 #include "ohci.h"
 

@@ -20,6 +21,7 @@ struct generic_ohci {
 	ohci_t ohci;
 	struct clk *clocks;
 	struct reset_ctl *resets;
+	struct phy phy;
 	int clock_count;
 	int reset_count;
 };

@@ -96,10 +98,30 @@ static int ohci_usb_probe(struct udevice *dev)
 		}
 	}
 
+	err = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (err) {
+		if (err != -ENOENT) {
+			error("failed to get usb phy\n");
+			goto reset_err;
+		}
+	} else {
+		err = generic_phy_init(&priv->phy);
+		if (err) {
+			error("failed to init usb phy\n");
+			goto reset_err;
+		}
+	}
+
 	err = ohci_register(dev, regs);
 	if (!err)
 		return err;
 
+	if (generic_phy_valid(&priv->phy)) {
+		ret = generic_phy_exit(&priv->phy);
+		if (ret)
+			return ret;
+	}
+
 reset_err:
 	ret = reset_assert_all(priv->resets, priv->reset_count);
 	if (ret)

@@ -121,6 +143,12 @@ static int ohci_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (generic_phy_valid(&priv->phy)) {
+		ret = generic_phy_exit(&priv->phy);
+		if (ret)
+			return ret;
+	}
+
 	ret = reset_assert_all(priv->resets, priv->reset_count);
 	if (ret)
 		return ret;
Signature · 1 line
1.9.1