[U-Boot,v2] drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

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 <sriram.dash@nxp.com>
Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
---
 drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 61 deletions(-)

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

Hello Marek,

Any comments? 

>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 <sriram.dash@nxp.com>
>Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>---
> drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
> 1 file changed, 47 insertions(+), 61 deletions(-)
>
>diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
>index 9c48852..df785a6 100644
>--- a/drivers/usb/common/fsl-dt-fixup.c
>+++ b/drivers/usb/common/fsl-dt-fixup.c
>@@ -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);
> }
>--
>2.1.0

On 09/14/2016 07:22 AM, Sriram Dash wrote:
>> From: Sriram Dash [mailto:sriram.dash@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 ?

>> 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 <sriram.dash@nxp.com>
>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>> ---
>> drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
>> index 9c48852..df785a6 100644
>> --- a/drivers/usb/common/fsl-dt-fixup.c
>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>> @@ -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);
>> }
>> --
>> 2.1.0
>

On 09/14/2016 02:35 PM, Marek Vasut wrote:
> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>> From: Sriram Dash [mailto:sriram.dash@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 ?
>
>>> 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 <sriram.dash@nxp.com>
>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>> ---
>>> drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
>>> index 9c48852..df785a6 100644
>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>> @@ -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;
>>>

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

>From: york sun
>On 09/14/2016 02:35 PM, Marek Vasut wrote:
>> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>>> From: Sriram Dash [mailto:sriram.dash@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.

>>>> 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 <sriram.dash@nxp.com>
>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>>> ---
>>>> drivers/usb/common/fsl-dt-fixup.c | 108
>>>> +++++++++++++++++---------------------
>>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>>>> b/drivers/usb/common/fsl-dt-fixup.c
>>>> index 9c48852..df785a6 100644
>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>> @@ -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;
>>>>
>
>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.

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

On 09/15/2016 12:29 AM, york sun wrote:
> On 09/14/2016 02:35 PM, Marek Vasut wrote:
>> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>>> From: Sriram Dash [mailto:sriram.dash@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 ?
>>
>>>> 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 <sriram.dash@nxp.com>
>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>>> ---
>>>> drivers/usb/common/fsl-dt-fixup.c | 108 +++++++++++++++++---------------------
>>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
>>>> index 9c48852..df785a6 100644
>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>> @@ -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;
>>>>
> 
> 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.

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

Please do.

> York
>

>From: Marek Vasut [mailto:marex@denx.de]
>On 09/15/2016 12:29 AM, york sun wrote:
>> On 09/14/2016 02:35 PM, Marek Vasut wrote:
>>> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>>>> From: Sriram Dash [mailto:sriram.dash@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 ?
>>>
>>>>> 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 <sriram.dash@nxp.com>
>>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>>>> ---
>>>>> drivers/usb/common/fsl-dt-fixup.c | 108
>>>>> +++++++++++++++++---------------------
>>>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>>>>> b/drivers/usb/common/fsl-dt-fixup.c
>>>>> index 9c48852..df785a6 100644
>>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>>> @@ -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;
>>>>>
>>
>> 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.
>

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

>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.

>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?

>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.
>
>> Other than that, this patch looks OK. I didn't test it by compiling or
>> running on a board.
>
>Please do.
>
>> York
>>
>
>
>--
>Best regards,
>Marek Vasut

On 09/16/2016 10:47 AM, Sriram Dash wrote:
>> From: Marek Vasut [mailto:marex@denx.de]
>> On 09/15/2016 12:29 AM, york sun wrote:
>>> On 09/14/2016 02:35 PM, Marek Vasut wrote:
>>>> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>>>>> From: Sriram Dash [mailto:sriram.dash@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 ?
>>>>
>>>>>> 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 <sriram.dash@nxp.com>
>>>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>>>>> ---
>>>>>> drivers/usb/common/fsl-dt-fixup.c | 108
>>>>>> +++++++++++++++++---------------------
>>>>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>>>>>> b/drivers/usb/common/fsl-dt-fixup.c
>>>>>> index 9c48852..df785a6 100644
>>>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>>>> @@ -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;
>>>>>>
>>>
>>> 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.
>>
> 
> Yes. I agree to your point. We will rename the functions and send a patch
> differently for the same.

Good

>> 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

>> 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.

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.

> 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.

>> 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.
>>
>>> Other than that, this patch looks OK. I didn't test it by compiling or
>>> running on a board.
>>
>> Please do.
>>
>>> York
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut

>From: Marek Vasut [mailto:marex@denx.de]
>On 09/16/2016 10:47 AM, Sriram Dash wrote:
>>> From: Marek Vasut [mailto:marex@denx.de] On 09/15/2016 12:29 AM, york
>>> sun wrote:
>>>> On 09/14/2016 02:35 PM, Marek Vasut wrote:
>>>>> On 09/14/2016 07:22 AM, Sriram Dash wrote:
>>>>>>> From: Sriram Dash [mailto:sriram.dash@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 ?
>>>>>
>>>>>>> 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 <sriram.dash@nxp.com>
>>>>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
>>>>>>> ---
>>>>>>> drivers/usb/common/fsl-dt-fixup.c | 108
>>>>>>> +++++++++++++++++---------------------
>>>>>>> 1 file changed, 47 insertions(+), 61 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>>>>>>> b/drivers/usb/common/fsl-dt-fixup.c
>>>>>>> index 9c48852..df785a6 100644
>>>>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>>>>> @@ -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;
>>>>>>>
>>>>
>>>> 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.
>>>
>>
>> Yes. I agree to your point. We will rename the functions and send a
>> patch differently for the same.
>
>Good
>
>>> 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
>
>>> 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.
>
>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.

>> 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.
>

OK. Will take it in next patch v3.

>>> 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.
>>>
>>>> Other than that, this patch looks OK. I didn't test it by compiling
>>>> or running on a board.
>>>
>>> Please do.
>>>
>>>> York
>>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>
>
>--
>Best regards,
>Marek Vasut