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

Sriram Dash sriram.dash at nxp.com
Thu Sep 15 08:40:52 CEST 2016


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

>>>> 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 at nxp.com>
>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at 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



More information about the U-Boot mailing list