drivers:usb:common:fsl-dt-fixup: Fix the dt for all type of usb controllers

PATCH New Sriram Dash 3 participants 13 messages Jun 20, 2016 – Sep 16, 2016 USB

Share & tools

Sriram Dash

author

2016-06-20 04:15 UTC #0 lore raw

PATCH 1/1 drivers:usb:common:fsl-dt-fixup: Fix the dt for all type of usb controllers

Currently, U-boot doesn't allow to compile more than one type of USB protocol simultaneously. Hence, EHCI and XHCI controllers cannot co-exist and CONFIG_USB_MAX_CONTROLLER_COUNT macro represents count of only one type of controller.

For the Socs which have more than one type of controller e.g. EHCI and XHCI, and the device tree has to support both of them, the macro CONFIG_USB_MAX_CONTROLLER_COUNT will not work for fixing dt from u boot.

So, instead of taking the hard coded number of controller from U boot, count the total number of controller present in dt and then fix the dt accordingly.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
 drivers/usb/common/fsl-dt-fixup.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

drivers/usb/common/fsl-dt-fixup.c +26 -1

index 9c48852..1edf146 100644
@@ -131,10 +131,29 @@ static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
 	return 0;
 }
 
+static int fdt_max_controller_count(void *blob)
+{
+	int count = 0;
+	int node_offset = -1;
+	int i;
+
+	for (i = 0; compat_usb_fsl[i]; i++) {
+		do {
+			node_offset = fdt_node_offset_by_compatible
+					(blob, node_offset,
+					 compat_usb_fsl[i]);
+			if (node_offset >= 0)
+				count++;
+		} while (node_offset != -FDT_ERR_NOTFOUND);
+	}
+	return count;
+}
+
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
+	unsigned int usb_max_controller_count;
 	int usb_erratum_a006261_off = -1;
 	int usb_erratum_a007075_off = -1;
 	int usb_erratum_a007792_off = -1;

@@ -146,7 +165,13 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 	int i, j;
 	int ret;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	usb_max_controller_count = fdt_max_controller_count(blob);
+	if (!usb_max_controller_count) {
+		printf("ERROR: FDT fixup erratum fail.\n");
+		return;
+	}
+
+	for (i = 1; i <= usb_max_controller_count; i++) {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;

Signature · 1 line

2.1.0

Marek Vasut

2016-06-20 14:16 UTC #1 lore raw

PATCH 1/1 drivers:usb:common:fsl-dt-fixup: Fix the dt for all type of usb controllers

Quoted 4 lines · Sriram Dash

Currently, U-boot doesn't allow to compile more than one type of USB protocol simultaneously. Hence, EHCI and XHCI controllers cannot co-exist and CONFIG_USB_MAX_CONTROLLER_COUNT macro represents count of only one type of controller.

This does not make sense, with DM we can support all sorts of controllers.

Quoted 49 lines

So, instead of taking the hard coded number of controller from U boot, count the total number of controller present in dt and then fix the dt accordingly.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
 drivers/usb/common/fsl-dt-fixup.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

drivers/usb/common/fsl-dt-fixup.c +19 -0

index 9c48852..1edf146 100644
@@ -131,10 +131,29 @@ static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
 	return 0;
 }
 
+static int fdt_max_controller_count(void *blob)
+{
+	int count = 0;
+	int node_offset = -1;
+	int i;
+
+	for (i = 0; compat_usb_fsl[i]; i++) {
+		do {
+			node_offset = fdt_node_offset_by_compatible
+					(blob, node_offset,
+					 compat_usb_fsl[i]);
+			if (node_offset >= 0)
+				count++;
+		} while (node_offset != -FDT_ERR_NOTFOUND);
+	}
+	return count;
+}
+
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
+	unsigned int usb_max_controller_count;
 	int usb_erratum_a006261_off = -1;
 	int usb_erratum_a007075_off = -1;
 	int usb_erratum_a007792_off = -1;

drivers/usb/common/fsl-dt-fixup.c void fdt_fixup_dr_usb(void *blob, bd_t *bd) · line 168

Quoted 5 lines

drivers/usb/common/fsl-dt-fixup.c +1 -1

@@ -146,7 +165,13 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 	int i, j;
 	int ret;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	usb_max_controller_count = fdt_max_controller_count(blob);

Since you walk the tree below anyway, why do you even need to count the elements? Just walk the tree until you can find no more nodes.

Quoted 10 lines

drivers/usb/common/fsl-dt-fixup.c +6 -0

+	if (!usb_max_controller_count) {
+		printf("ERROR: FDT fixup erratum fail.\n");
+		return;
+	}
+
+	for (i = 1; i <= usb_max_controller_count; i++) {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;

Signature · 2 lines

Best regards,
Marek Vasut

Sriram Dash

author

2016-07-20 03:55 UTC #2 lore raw

PATCH 1/1 drivers:usb:common:fsl-dt-fixup: Fix the dt for all type of usb controllers

Quoted 10 lines

From: Marek Vasut [mailto:marex at denx.de]
Quoted 3 lines · Sriram Dash
Currently, U-boot doesn't allow to compile more than one type of USB protocol simultaneously. Hence, EHCI and XHCI controllers cannot co-exist and CONFIG_USB_MAX_CONTROLLER_COUNT macro represents count
of
Quoted 1 line
only one type of controller.
This does not make sense, with DM we can support all sorts of controllers.

Ok. Will change the commit message description in v2.

Quoted 52 lines

drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_erratum(int *usb_erratum_off, void
Quoted 20 lines
For the Socs which have more than one type of controller e.g. EHCI and XHCI, and the device tree has to support both of them, the macro CONFIG_USB_MAX_CONTROLLER_COUNT will not work for fixing dt from u boot.
So, instead of taking the hard coded number of controller from U boot, count the total number of controller present in dt and then fix the dt accordingly.
Signed-off-by Sriram Dash <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
 drivers/usb/common/fsl-dt-fixup.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)
drivers/usb/common/fsl-dt-fixup.c +0 -0
index 9c48852..1edf146 100644
@@ -131,10 +131,29 @@ static int fdt_fixup_erratum(int *usb_erratum_off, void
*blob,
Quoted 31 lines
drivers/usb/common/fsl-dt-fixup.c +18 -0
 	return 0;
 }
 
+static int fdt_max_controller_count(void *blob) {
+	int count = 0;
+	int node_offset = -1;
+	int i;
+
+	for (i = 0; compat_usb_fsl[i]; i++) {
+		do {
+			node_offset = fdt_node_offset_by_compatible
+					(blob, node_offset,
+					 compat_usb_fsl[i]);
+			if (node_offset >= 0)
+				count++;
+		} while (node_offset != -FDT_ERR_NOTFOUND);
+	}
+	return count;
+}
+
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
+	unsigned int usb_max_controller_count;
 	int usb_erratum_a006261_off = -1;
 	int usb_erratum_a007075_off = -1;
 	int usb_erratum_a007792_off = -1;
@@ -146,7 +165,13 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 	int i, j;
 	int ret;
 

Quoted 6 lines

drivers/usb/common/fsl-dt-fixup.c void fdt_fixup_dr_usb(void *blob, bd_t *bd) · line 168
Quoted 2 lines
drivers/usb/common/fsl-dt-fixup.c +1 -1
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	usb_max_controller_count = fdt_max_controller_count(blob);
Since you walk the tree below anyway, why do you even need to count the elements? Just walk the tree until you can find no more nodes.

Ok. Will traverse the dt and find the node. Then fix the node and afterwards, Traverse the device tree till no nodes are left for fixing.

Quoted 15 lines

drivers/usb/common/fsl-dt-fixup.c void fdt_fixup_dr_usb(void *blob, bd_t *bd) · line 177
Quoted 10 lines
drivers/usb/common/fsl-dt-fixup.c +6 -0
+	if (!usb_max_controller_count) {
+		printf("ERROR: FDT fixup erratum fail.\n");
+		return;
+	}
+
+	for (i = 1; i <= usb_max_controller_count; i++) {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
--
Best regards,
Marek Vasut

Marek Vasut

2016-07-20 05:57 UTC #3 lore raw

PATCH 1/1 drivers:usb:common:fsl-dt-fixup: Fix the dt for all type of usb controllers

Quoted 72 lines · Sriram Dash

Quoted 10 lines
From: Marek Vasut [mailto:marex at denx.de]
Quoted 3 lines · Sriram Dash
Currently, U-boot doesn't allow to compile more than one type of USB protocol simultaneously. Hence, EHCI and XHCI controllers cannot co-exist and CONFIG_USB_MAX_CONTROLLER_COUNT macro represents count
of
Quoted 1 line
only one type of controller.
This does not make sense, with DM we can support all sorts of controllers.
Ok. Will change the commit message description in v2.
Quoted 52 lines
drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_erratum(int *usb_erratum_off, void
Quoted 20 lines
For the Socs which have more than one type of controller e.g. EHCI and XHCI, and the device tree has to support both of them, the macro CONFIG_USB_MAX_CONTROLLER_COUNT will not work for fixing dt from u boot.
So, instead of taking the hard coded number of controller from U boot, count the total number of controller present in dt and then fix the dt accordingly.
Signed-off-by Sriram Dash <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
 drivers/usb/common/fsl-dt-fixup.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)
drivers/usb/common/fsl-dt-fixup.c +0 -0
index 9c48852..1edf146 100644
@@ -131,10 +131,29 @@ static int fdt_fixup_erratum(int *usb_erratum_off, void
*blob,
Quoted 31 lines
drivers/usb/common/fsl-dt-fixup.c +18 -0
 	return 0;
 }
 
+static int fdt_max_controller_count(void *blob) {
+	int count = 0;
+	int node_offset = -1;
+	int i;
+
+	for (i = 0; compat_usb_fsl[i]; i++) {
+		do {
+			node_offset = fdt_node_offset_by_compatible
+					(blob, node_offset,
+					 compat_usb_fsl[i]);
+			if (node_offset >= 0)
+				count++;
+		} while (node_offset != -FDT_ERR_NOTFOUND);
+	}
+	return count;
+}
+
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
+	unsigned int usb_max_controller_count;
 	int usb_erratum_a006261_off = -1;
 	int usb_erratum_a007075_off = -1;
 	int usb_erratum_a007792_off = -1;
@@ -146,7 +165,13 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 	int i, j;
 	int ret;
 
Quoted 6 lines
drivers/usb/common/fsl-dt-fixup.c void fdt_fixup_dr_usb(void *blob, bd_t *bd) · line 168
Quoted 2 lines
drivers/usb/common/fsl-dt-fixup.c +1 -1
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	usb_max_controller_count = fdt_max_controller_count(blob);
Since you walk the tree below anyway, why do you even need to count the elements? Just walk the tree until you can find no more nodes.

Quoted 18 lines

Ok. Will traverse the dt and find the node. Then fix the node and afterwards, Traverse the device tree till no nodes are left for fixing.
Quoted 15 lines
drivers/usb/common/fsl-dt-fixup.c void fdt_fixup_dr_usb(void *blob, bd_t *bd) · line 177
Quoted 10 lines
drivers/usb/common/fsl-dt-fixup.c +6 -0
+	if (!usb_max_controller_count) {
+		printf("ERROR: FDT fixup erratum fail.\n");
+		return;
+	}
+
+	for (i = 1; i <= usb_max_controller_count; i++) {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
--
Best regards,
Marek Vasut

It'd also be awesome if you replied to emails with less than 1 month pause.

Signature · 2 lines

Best regards,
Marek Vasut

Sriram Dash

author

2016-08-17 08:16 UTC #4 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

In reply to Sriram Dash

For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more USB nodes are left.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
 drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +47 ~~-61~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int start_offset,
 }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type, FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;
 	debug("Adding USB erratum %s\n", str);
 	return 0;

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode",

@@ -184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 		if (has_dual_phy())
 			dr_phy_type = phys[2];
 
-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)
 			return;
 
-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)
 			return;
 
-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
 					CHIPIDEA_USB2, "a006261",
-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);
 		if (ret == -ENOSPC)
 			return;
-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
 					CHIPIDEA_USB2, "a007075",
-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);
 		if (ret == -ENOSPC)
 			return;
-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
 					CHIPIDEA_USB2, "a007792",
-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);
 		if (ret == -ENOSPC)
 			return;
-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
 					CHIPIDEA_USB2, "a005697",
-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);
 		if (ret == -ENOSPC)
 			return;
-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
 					SNPS_DWC3, "a008751",
-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);
 		if (ret == -ENOSPC)
 			return;
 
-	}
+	} while (node_offset > 0);
 }

Signature · 1 line

2.1.0

Sriram Dash

author

2016-09-14 05:22 UTC #5 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 2 lines

From: Sriram Dash [mailto:sriram.dash at nxp.com]

Hello Marek,

Any comments?

Quoted 212 lines

For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more USB nodes are left.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int
 start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const
 char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-	}
+	} while (node_offset > 0);

}
--
2.1.0

Marek Vasut

2016-09-14 21:22 UTC #6 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 6 lines · Sriram Dash

Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?

Waiting for York to review this.

It's a bulky patch V2 without changelog, shall I review the whole thing again ?

Quoted 213 lines

Quoted 212 lines

For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more USB nodes are left.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int
 start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const
 char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-	}
+	} while (node_offset > 0);

}
--
2.1.0

Signature · 2 lines

Best regards,
Marek Vasut

york sun

2016-09-14 22:29 UTC #7 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 212 lines · Marek Vasut

Quoted 6 lines · Sriram Dash

Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?

Waiting for York to review this.

It's a bulky patch V2 without changelog, shall I review the whole thing again ?

Quoted 199 lines

For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more USB nodes are left.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int
 start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const
 char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

Quoted 8 lines

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);

		if (ret == -ENOSPC)
			return;

Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.

Other than that, this patch looks OK. I didn't test it by compiling or running on a board.

York

Sriram Dash

author

2016-09-15 06:40 UTC #8 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 15 lines

From: york sun
Quoted 13 lines · Marek Vasut
Quoted 6 lines · Sriram Dash
Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?
Waiting for York to review this.
It's a bulky patch V2 without changelog, shall I review the whole thing again ?

Will take care the next time for changelog. I agree the patch is completely changed from v1. The major change is that the dt traversal is reduced.

Quoted 205 lines

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more

USB nodes are left.

Quoted 202 lines

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108
+++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int
 start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void
 *blob, const char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

Quoted 9 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);
		if (ret == -ENOSPC)
			return;
Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.

IMO, it is better to return in case of ENOSPC error, as the device tree will not be modified, so there is no point of continuing further.

Quoted 4 lines

Other than that, this patch looks OK. I didn't test it by compiling or running on a board.
York

Marek Vasut

2016-09-15 22:01 UTC #9 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

In reply to york sun

Quoted 216 lines · york sun

Quoted 215 lines · Marek Vasut

Quoted 6 lines · Sriram Dash

Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?

Waiting for York to review this.

It's a bulky patch V2 without changelog, shall I review the whole thing again ?

Quoted 202 lines

For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no more USB nodes are left.

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob, int
 start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const
 char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);
		if (ret == -ENOSPC)
			return;
Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.

The fdt_fixup_erratum() function is named in the most confusing dumb way, it is not a generic function but a FSL/NXP specific one. Except it does look like a generic one. This should've never made it mainline and it should be renamed.

Regarding the return above, it's just that whoever implemented that function did it in the most broken way possible. ENOSPC, according to
errno(3) means "ENOSPC No space left on device (POSIX.1)", which is
completely unrelated to anything USB in this context. Worse yet, the error is returned whenever fdt_fixup_usb_erratum() returns negative value , instead of propagating errors.

I think the aforementioned two points should be fixed first and only then should the return value above be handled correctly based on what the value is.

I am real disappointed when looking at this crap.

Quoted 2 lines

Other than that, this patch looks OK. I didn't test it by compiling or running on a board.

Please do.

Quoted 2 lines

York

Signature · 2 lines

Best regards,
Marek Vasut

Sriram Dash

author

2016-09-16 08:47 UTC #10 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 230 lines

From: Marek Vasut [mailto:marex at denx.de]

Quoted 9 lines · york sun

Quoted 8 lines · Marek Vasut
Quoted 6 lines · Sriram Dash
Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?

Quoted 7 lines

Quoted 7 lines
Waiting for York to review this.
It's a bulky patch V2 without changelog, shall I review the whole thing again ?
Quoted 2 lines
Quoted 2 lines
For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no

more USB nodes are left.

Quoted 202 lines

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108
+++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob,
 int start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void
 *blob, const char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);
		if (ret == -ENOSPC)
			return;
Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.

Quoted 4 lines

The fdt_fixup_erratum() function is named in the most confusing dumb way, it is not a generic function but a FSL/NXP specific one. Except it does look like a generic one. This should've never made it mainline and it should be renamed.

Yes. I agree to your point. We will rename the functions and send a patch differently for the same.

Quoted 4 lines

Regarding the return above, it's just that whoever implemented that function did it in the most broken way possible. ENOSPC, according to
errno(3) means "ENOSPC No space left on device (POSIX.1)", which is
completely unrelated to anything USB in this context. Worse yet, the error is

Yes. The ENOSPC is not related to USB. Also, this code fixes the device tree, which will not affect usb functionality in U boot.

Quoted 3 lines

returned whenever fdt_fixup_usb_erratum() returns negative value , instead of propagating errors.

The error is returned when fixup_usb_erratum returns a negative value. Now there are two possibilities.
- The erratum is not to be applicable for the Soc, where the fdt_fixup_erratum returns EINVAL
if (!has_erratum())

                return -EINVAL;

In this case, we want to continue applying other errata.
- The erratum is to be applicable, but the fdt_fixup_usb_erratum fails to apply it. if (node_offset < 0)
return -ENOSPC;
In this case, the failure will be inability to apply the erratum due to lack of space in device tree. So, check for the ENOSPC and return from the function fdt_fixup_dr_usb, as no other erratum can be applied further.

As York also pointed out regarding return in case of each error, we can avoid the returns and this will make the code clean and simple. But, the fdt_fixup_dr_usb will get executed till the device tree is traversed completely, regardless of the modifications taking effect over device tree.
What do you think?

Quoted 17 lines

I think the aforementioned two points should be fixed first and only then should the return value above be handled correctly based on what the value is.
I am real disappointed when looking at this crap.
Quoted 2 lines
Other than that, this patch looks OK. I didn't test it by compiling or running on a board.
Please do.
Quoted 2 lines
York
--
Best regards,
Marek Vasut

Marek Vasut

2016-09-16 09:17 UTC #11 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 230 lines · Sriram Dash

Quoted 230 lines

From: Marek Vasut [mailto:marex at denx.de]

Quoted 9 lines · york sun

Quoted 8 lines · Marek Vasut
Quoted 6 lines · Sriram Dash
Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?

Quoted 7 lines

Quoted 7 lines
Waiting for York to review this.
It's a bulky patch V2 without changelog, shall I review the whole thing again ?
Quoted 2 lines
Quoted 2 lines
For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no

more USB nodes are left.

Quoted 210 lines

Quoted 199 lines

Signed-off-by Sriram Dash <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
drivers/usb/common/fsl-dt-fixup.c | 108
+++++++++++++++++---------------------

1 file changed, 47 insertions(+), 61 deletions(-)

drivers/usb/common/fsl-dt-fixup.c +19 ~~-29~~

index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob,
 int start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {

@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void
 *blob, const char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int start_offset)
+				 const char *controller_type, int node_offset,
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char **node_type)
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf, controller_type,
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;

	debug("Adding USB erratum %s\n", str);
	return 0;

drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~

@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)

		if (has_dual_phy())
			dr_phy_type = phys[2];

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type, NULL,
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +3 -5

-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL, dr_phy_type,
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
+						  node_offset, &node_type);
+		if (err < 0)

			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a006261",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a006261);
+					has_erratum_a006261, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007075",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007075);
+					has_erratum_a007075, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a007792",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a007792);
+					has_erratum_a007792, &node_type);

		if (ret == -ENOSPC)
			return;

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					CHIPIDEA_USB2, "a005697",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a005697);
+					has_erratum_a005697, &node_type);

		if (ret == -ENOSPC)
			return;

Quoted 8 lines

drivers/usb/common/fsl-dt-fixup.c +1 -1

-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,

					SNPS_DWC3, "a008751",

drivers/usb/common/fsl-dt-fixup.c +1 -1

-					has_erratum_a008751);
+					has_erratum_a008751, &node_type);

		if (ret == -ENOSPC)
			return;

Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.

Quoted 7 lines

Quoted 4 lines
The fdt_fixup_erratum() function is named in the most confusing dumb way, it is not a generic function but a FSL/NXP specific one. Except it does look like a generic one. This should've never made it mainline and it should be renamed.
Yes. I agree to your point. We will rename the functions and send a patch differently for the same.

Good

Quoted 7 lines

Quoted 4 lines
Regarding the return above, it's just that whoever implemented that function did it in the most broken way possible. ENOSPC, according to
errno(3) means "ENOSPC No space left on device (POSIX.1)", which is
completely unrelated to anything USB in this context. Worse yet, the error is
Yes. The ENOSPC is not related to USB. Also, this code fixes the device tree, which will not affect usb functionality in U boot.

Quoted 9 lines

Quoted 3 lines
returned whenever fdt_fixup_usb_erratum() returns negative value , instead of propagating errors.
The error is returned when fixup_usb_erratum returns a negative value. Now there are two possibilities.
- The erratum is not to be applicable for the Soc, where the fdt_fixup_erratum returns EINVAL
if (!has_erratum())

Quoted 8 lines

                return -EINVAL;
In this case, we want to continue applying other errata.
- The erratum is to be applicable, but the fdt_fixup_usb_erratum fails to apply it. if (node_offset < 0)
return -ENOSPC;
In this case, the failure will be inability to apply the erratum due to lack of space in device tree. So, check for the ENOSPC and return from the function fdt_fixup_dr_usb, as no other erratum can be applied further.

And what happens if fdt_fixup_usb_erratum() doesn't fail because of not enough space in DT ? Then the information gets lost in here. So I don't really care about this elaborate explanation, this should be fixed and the errors should be propagated correctly.

Quoted 5 lines

As York also pointed out regarding return in case of each error, we can avoid the returns and this will make the code clean and simple. But, the fdt_fixup_dr_usb will get executed till the device tree is traversed completely, regardless of the modifications taking effect over device tree.
What do you think?

I think you should fix your error paths before you tackle this error handling here at all, otherwise it's gonna be an annoying mess.

The way I'd do this is I'd have a table of fdt_fixup_erratum() arguments and just iterate over the table. Then the code becomes minimal.

Quoted 17 lines

Quoted 17 lines
I think the aforementioned two points should be fixed first and only then should the return value above be handled correctly based on what the value is.
I am real disappointed when looking at this crap.
Quoted 2 lines
Other than that, this patch looks OK. I didn't test it by compiling or running on a board.
Please do.
Quoted 2 lines
York
--
Best regards,
Marek Vasut

Signature · 2 lines

Best regards,
Marek Vasut

Sriram Dash

author

2016-09-16 10:27 UTC #12 lore raw

PATCH v2 1/1 drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

Quoted 287 lines

From: Marek Vasut [mailto:marex at denx.de]
Quoted 72 lines · Sriram Dash
Quoted 72 lines
From: Marek Vasut [mailto:marex at denx.de] On 09/15/2016 12:29 AM, york
Quoted 9 lines · sun
Quoted 8 lines · Marek Vasut
Quoted 6 lines · Sriram Dash
Quoted 2 lines
From: Sriram Dash [mailto:sriram.dash at nxp.com]
Hello Marek,
Any comments?
Quoted 8 lines
Quoted 8 lines
Waiting for York to review this.
It's a bulky patch V2 without changelog, shall I review the whole thing again ?
Quoted 3 lines
Quoted 3 lines
For FSL USB node fixup, the dt is walked multiple times for fixing erratum and phy type. This patch walks the tree and fixes the node till no
more USB nodes are left.
Quoted 52 lines
Quoted 52 lines
Quoted 52 lines
Quoted 52 lines
Signed-off-by Sriram Dash <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
drivers/usb/common/fsl-dt-fixup.c | 108
+++++++++++++++++---------------------
1 file changed, 47 insertions(+), 61 deletions(-)
drivers/usb/common/fsl-dt-fixup.c +5 ~~-11~~
index 9c48852..df785a6 100644
@@ -54,25 +54,19 @@ static int fdt_usb_get_node_type(void *blob,
 int start_offset,  }
 
 static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
-				       const char *phy_type, int start_offset)
+				       const char *phy_type, int node_offset,
+				       const char **node_type)
 {
 	const char *prop_mode = "dr_mode";
 	const char *prop_type = "phy_type";
-	const char *node_type = NULL;
-	int node_offset;
-	int err;
-
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
+	int err = 0;
 
 	if (mode) {
 		err = fdt_setprop(blob, node_offset, prop_mode, mode,
 				  strlen(mode) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_mode, node_type, fdt_strerror(err));
+			       prop_mode, *node_type, fdt_strerror(err));
 	}
 
 	if (phy_type) {
@@ -80,52 +74,48 @@ static int fdt_fixup_usb_mode_phy_type(void
 *blob, const char *mode,
 				  strlen(phy_type) + 1);
 		if (err < 0)
 			printf("WARNING: could not set %s for %s: %s.\n",
-			       prop_type, node_type, fdt_strerror(err));
+			       prop_type, *node_type, fdt_strerror(err));
 	}
 
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_usb_mode_phy_type(void · line 91
Quoted 6 lines
drivers/usb/common/fsl-dt-fixup.c +1 -2
-	return node_offset;
+	return err;
 }
 
 static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
-				 const char *controller_type, int
start_offset)
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_usb_mode_phy_type(void · line 85
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c +1 -0
+				 const char *controller_type, int
node_offset,
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
drivers/usb/common/fsl-dt-fixup.c +7 ~~-11~~
+				 const char **node_type)
 {
-	int node_offset, err;
-	const char *node_type = NULL;
+	int err = -1;
 	const char *node_name = NULL;
 
-	err = fdt_usb_get_node_type(blob, start_offset,
-				    &node_offset, &node_type);
-	if (err < 0)
-		return err;
-
-	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
 FSL_USB2_DR))
+	if (!strcmp(*node_type, FSL_USB2_MPH) ||
+	    !strcmp(*node_type, FSL_USB2_DR))
 		node_name = CHIPIDEA_USB2;
 	else
-		node_name = node_type;
+		node_name = *node_type;
 	if (strcmp(node_name, controller_type))
 		return err;
 
 	err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0);
 	if (err < 0) {
 		printf("ERROR: could not set %s for %s: %s.\n",
-		       prop_erratum, node_type, fdt_strerror(err));
+		       prop_erratum, *node_type, fdt_strerror(err));
 	}
 
-	return node_offset;
+	return err;
 }
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_usb_mode_phy_type(void · line 111
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c +2 -2
-static int fdt_fixup_erratum(int *usb_erratum_off, void *blob,
+static int fdt_fixup_erratum(int node_offset, void *blob,
 			     const char *controller_type, char *str,
-			     bool (*has_erratum)(void))
+			     bool (*has_erratum)(void), const char
**node_type)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c static int fdt_fixup_usb_mode_phy_type(void · line 128
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c +0 -1
 {
 	char buf[32] = {0};
 
 	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
 	if (!has_erratum())
 		return -EINVAL;
-	*usb_erratum_off = fdt_fixup_usb_erratum(blob, buf,
controller_type,
Quoted 46 lines
Quoted 46 lines
Quoted 46 lines
Quoted 46 lines
Quoted 46 lines
Quoted 46 lines
drivers/usb/common/fsl-dt-fixup.c +3 -2
-						 *usb_erratum_off);
-	if (*usb_erratum_off < 0)
+	node_offset = fdt_fixup_usb_erratum(blob, buf, controller_type,
+					    node_offset, node_type);
+	if (node_offset < 0)
 		return -ENOSPC;
	debug("Adding USB erratum %s\n", str);
	return 0;
drivers/usb/common/fsl-dt-fixup.c +11 ~~-11~~
@@ -135,23 +125,23 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
 	static const char * const modes[] = { "host", "peripheral", "otg" };
 	static const char * const phys[] = { "ulpi", "utmi", "utmi_dual" };
-	int usb_erratum_a006261_off = -1;
-	int usb_erratum_a007075_off = -1;
-	int usb_erratum_a007792_off = -1;
-	int usb_erratum_a005697_off = -1;
-	int usb_erratum_a008751_off = -1;
-	int usb_mode_off = -1;
-	int usb_phy_off = -1;
+	const char *node_type = NULL;
+	int node_offset = -1;
 	char str[5];
-	int i, j;
-	int ret;
+	int i = 1, j;
+	int ret, err;
 
-	for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) {
+	do {
 		const char *dr_mode_type = NULL;
 		const char *dr_phy_type = NULL;
 		int mode_idx = -1, phy_idx = -1;
 
-		snprintf(str, 5, "%s%d", "usb", i);
+		err = fdt_usb_get_node_type(blob, node_offset,
+					    &node_offset, &node_type);
+		if (err < 0)
+			return;
+
+		snprintf(str, 5, "%s%d", "usb", i++);
 		if (hwconfig(str)) {
 			for (j = 0; j < ARRAY_SIZE(modes); j++) {
 				if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
 184,45 +174,41 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
		if (has_dual_phy())
			dr_phy_type = phys[2];
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 2 lines
drivers/usb/common/fsl-dt-fixup.c +0 -2
-		usb_mode_off = fdt_fixup_usb_mode_phy_type(blob,
-							   dr_mode_type,
NULL,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 4 lines
drivers/usb/common/fsl-dt-fixup.c +1 -3
-							   usb_mode_off);
-
-		if (usb_mode_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type,
NULL,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c +1 -0
+						  node_offset,
&node_type);
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 5 lines
drivers/usb/common/fsl-dt-fixup.c +1 -0
+		if (err < 0)
			return;
drivers/usb/common/fsl-dt-fixup.c +0 -2
-		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
-							  NULL,
dr_phy_type,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 4 lines
drivers/usb/common/fsl-dt-fixup.c +1 -3
-							  usb_phy_off);
-
-		if (usb_phy_off < 0)
+		err = fdt_fixup_usb_mode_phy_type(blob, NULL,
dr_phy_type,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c
Quoted 1 line
drivers/usb/common/fsl-dt-fixup.c +1 -0
+						  node_offset,
&node_type);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 8 lines
drivers/usb/common/fsl-dt-fixup.c +1 -0
+		if (err < 0)
			return;
drivers/usb/common/fsl-dt-fixup.c +1 -1
-		ret = fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
					CHIPIDEA_USB2, "a006261",
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a006261);
+					has_erratum_a006261,
&node_type);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 7 lines
		if (ret == -ENOSPC)
			return;
drivers/usb/common/fsl-dt-fixup.c +1 -1
-		ret = fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
					CHIPIDEA_USB2, "a007075",
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a007075);
+					has_erratum_a007075,
&node_type);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 7 lines
		if (ret == -ENOSPC)
			return;
drivers/usb/common/fsl-dt-fixup.c +1 -1
-		ret = fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
					CHIPIDEA_USB2, "a007792",
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a007792);
+					has_erratum_a007792,
&node_type);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 7 lines
		if (ret == -ENOSPC)
			return;
drivers/usb/common/fsl-dt-fixup.c +1 -1
-		ret = fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
					CHIPIDEA_USB2, "a005697",
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a005697);
+					has_erratum_a005697,
&node_type);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/usb/common/fsl-dt-fixup.c
Quoted 7 lines
		if (ret == -ENOSPC)
			return;
drivers/usb/common/fsl-dt-fixup.c +1 -1
-		ret = fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
+		ret = fdt_fixup_erratum(node_offset, blob,
					SNPS_DWC3, "a008751",
drivers/usb/common/fsl-dt-fixup.c +1 -1
-					has_erratum_a008751);
+					has_erratum_a008751,
&node_type);
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
		if (ret == -ENOSPC)
			return;
Do we really want to return in case of each error? I am not USB expert so my comment could be mistaken.
The fdt_fixup_erratum() function is named in the most confusing dumb way, it is not a generic function but a FSL/NXP specific one. Except it does look like a generic one. This should've never made it mainline and it
should be renamed.
Quoted 4 lines
Quoted 1 line
Yes. I agree to your point. We will rename the functions and send a patch differently for the same.
Good
Quoted 8 lines
Quoted 5 lines
Regarding the return above, it's just that whoever implemented that function did it in the most broken way possible. ENOSPC, according to
errno(3) means "ENOSPC No space left on device (POSIX.1)", which is
completely unrelated to anything USB in this context. Worse yet, the error is
Yes. The ENOSPC is not related to USB. Also, this code fixes the device tree, which will not affect usb functionality in U boot.
OK
Quoted 8 lines
Quoted 3 lines
returned whenever fdt_fixup_usb_erratum() returns negative value , instead of propagating errors.
The error is returned when fixup_usb_erratum returns a negative value. Now there are two possibilities.
- The erratum is not to be applicable for the Soc, where the fdt_fixup_erratum returns EINVAL if (!has_erratum())
Quoted 8 lines
                return -EINVAL;
In this case, we want to continue applying other errata.
- The erratum is to be applicable, but the fdt_fixup_usb_erratum fails to apply it. if (node_offset < 0)
return -ENOSPC;
In this case, the failure will be inability to apply the erratum due to lack of space in device tree. So, check for the ENOSPC and return from the function fdt_fixup_dr_usb, as no other erratum can be applied further.

Quoted 5 lines

And what happens if fdt_fixup_usb_erratum() doesn't fail because of not enough space in DT ? Then the information gets lost in here. So I don't really care about this elaborate explanation, this should be fixed and the errors should be propagated correctly.

OK.

Quoted 4 lines

Quoted 4 lines
As York also pointed out regarding return in case of each error, we can avoid the returns and this will make the code clean and simple. But, the fdt_fixup_dr_usb will get executed till the device tree is traversed completely, regardless of the modifications taking effect over device

Quoted 9 lines

tree.
Quoted 1 line
What do you think?
I think you should fix your error paths before you tackle this error handling here at all, otherwise it's gonna be an annoying mess.
The way I'd do this is I'd have a table of fdt_fixup_erratum() arguments and just iterate over the table. Then the code becomes minimal.

OK. Will take it in next patch v3.

Quoted 23 lines

Quoted 2 lines
Quoted 2 lines
I think the aforementioned two points should be fixed first and only then should the return value above be handled correctly based on what the value
is.
Quoted 8 lines
Quoted 8 lines
I am real disappointed when looking at this crap.
Quoted 2 lines
Other than that, this patch looks OK. I didn't test it by compiling or running on a board.
Please do.
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
York
--
Best regards,
Marek Vasut
--
Best regards,
Marek Vasut