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

Marek Vasut marex at denx.de
Fri Oct 28 01:52:26 CEST 2016


On 10/27/2016 08:56 AM, Sriram Dash wrote:
> 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>
> ---
> Depends on the following patch:
> https://patchwork.ozlabs.org/patch/682139/
> 
> Changes in v3:
>   - Remove ENOSPC.
>   - Create a table of USB erratum, to be used to fix dt.
> 
> Changes in v2:
>   - Modify patch description and title
>   - Remove counting the dt nodes
> 
>  drivers/usb/common/fsl-dt-fixup.c | 209 ++++++++++++++++++++------------------
>  1 file changed, 109 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
> index 63a24f7..addeb8c 100644
> --- a/drivers/usb/common/fsl-dt-fixup.c
> +++ b/drivers/usb/common/fsl-dt-fixup.c
> @@ -16,10 +16,6 @@
>  #include <fsl_usb.h>
>  #include <fdt_support.h>
>  
> -#ifndef CONFIG_USB_MAX_CONTROLLER_COUNT
> -#define CONFIG_USB_MAX_CONTROLLER_COUNT 1
> -#endif
> -
>  /* USB Controllers */
>  #define FSL_USB2_MPH	"fsl-usb2-mph"
>  #define FSL_USB2_DR	"fsl-usb2-dr"
> @@ -33,6 +29,52 @@ static const char * const compat_usb_fsl[] = {
>  	NULL
>  };
>  
> +enum usb_node_type {
> +	USB2_MPH = 0,
> +	USB2_DR,
> +	SNPS,
> +	USB_COMPAT_END
> +};
> +
> +enum fdt_fsl_usb_erratum {
> +	A006261,
> +	A007075,
> +	A007792,
> +	A005697,
> +	A008751,
> +	FSL_USB_ERRATUM_END

The compiler can assign completely arbitrary numbers to the enum
elements. Moreover, you don't need this "terminating entry" anyway, see
below.

> +};
> +
> +struct fsl_dt_usb_erratum {
> +	bool (*has_fsl_usb_erratum)(void);
> +	char *dt_set_prop;
> +};
> +
> +struct fsl_dt_usb_erratum
> +	erratum_tlb[USB_COMPAT_END][FSL_USB_ERRATUM_END] = {
> +/*MPH Erratum */
> +	[USB2_MPH][A006261] = {&has_erratum_a006261,
> +			       "fsl,usb-erratum-a006261"},
> +	[USB2_MPH][A007075] = {&has_erratum_a007075,
> +			       "fsl,usb-erratum-a007075"},
> +	[USB2_MPH][A007792] = {&has_erratum_a007792,
> +			       "fsl,usb-erratum-a007792"},
> +	[USB2_MPH][A005697] = {&has_erratum_a005697,
> +			       "fsl,usb-erratum-a005697"},
> +/*DR Erratum */
> +	[USB2_DR][A006261] = {&has_erratum_a006261,
> +			      "fsl,usb-erratum-a006261"},
> +	[USB2_DR][A007075] = {&has_erratum_a007075,
> +			      "fsl,usb-erratum-a007075"},
> +	[USB2_DR][A007792] = {&has_erratum_a007792,
> +			      "fsl,usb-erratum-a007792"},
> +	[USB2_DR][A005697] = {&has_erratum_a005697,
> +			      "fsl,usb-erratum-a005697"},
> +/*SNPS Erratum */
> +	[SNPS][A008751] = {&has_erratum_a008751,
> +			   "fsl,usb-erratum-a008751"},

Please just split this into three arrays.

> +};
> +
>  static int fdt_usb_get_node_type(void *blob, int start_offset,
>  				 int *node_offset, const char **node_type)
>  {
> @@ -54,25 +96,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,79 +116,77 @@ 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 fsl_fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
> -				     const char *controller_type,
> -				     int start_offset)
> +static int fsl_fdt_fixup_usb_erratum(int node_offset, void *blob,
> +				     int dt_node_type)
>  {
> -	int node_offset, err;
> -	const char *node_type = NULL;
> -	const char *node_name = NULL;
> +	int i, j, ret = -1;
> +	for (i = dt_node_type; i < USB_COMPAT_END; i++) {
> +		for (j = 0; j < FSL_USB_ERRATUM_END; j++) {

You can use ARRAY_SIZE() here if this was linear array, which it should be.

> +			if (erratum_tlb[i][j].has_fsl_usb_erratum != NULL) {

This code looks like crap, the indent is horrible and it's unnecessary.

> +				if (!erratum_tlb[i][j].has_fsl_usb_erratum())
> +					continue;
> +				ret = fdt_setprop(blob, node_offset,
> +					erratum_tlb[i][j].dt_set_prop,
> +					NULL, 0);
> +				if (ret < 0) {
> +					printf("ERROR: could not set %s: %s.\n",
> +					       erratum_tlb[i][j].dt_set_prop,
> +					       fdt_strerror(ret));
> +					return ret;
> +				}
> +			}
> +		}
> +	}
> +	return ret;
> +}
>  
> -	err = fdt_usb_get_node_type(blob, start_offset,
> -				    &node_offset, &node_type);
> -	if (err < 0)
> -		return err;
> +static int fsl_fdt_fixup_erratum(int node_offset, void *blob,
> +				 const char **node_type)
> +{
> +	int dt_node_type;
> +	int ret;
>  
> -	if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type, FSL_USB2_DR))
> -		node_name = CHIPIDEA_USB2;
> +	if (!strcmp(*node_type, FSL_USB2_MPH))
> +		dt_node_type = USB2_MPH;
> +	else if (!strcmp(*node_type, FSL_USB2_DR))
> +		dt_node_type = USB2_DR;
> +	else if (!strcmp(*node_type, SNPS_DWC3))
> +		dt_node_type = SNPS;

So uh ... your code tests node compatibility here only to test it in
fsl_fdt_fixup_usb_erratum() again. This wouldn't be needed if you used
linear array. btw use fdt_node_check_compatible()

>  	else
> -		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));
> -	}
> +		return -ENOENT;
>  
> -	return node_offset;
> -}
> +	ret = fsl_fdt_fixup_usb_erratum(node_offset, blob, dt_node_type);
>  
> -static int fsl_fdt_fixup_erratum(int *usb_erratum_off, void *blob,
> -				 const char *controller_type, char *str,
> -				 bool (*has_erratum)(void))
> -{
> -	char buf[32] = {0};
> -
> -	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
> -	if (!has_erratum())
> -		return -EINVAL;
> -	*usb_erratum_off = fsl_fdt_fixup_usb_erratum(blob, buf, controller_type,
> -						     *usb_erratum_off);
> -	if (*usb_erratum_off < 0)
> -		return -ENOSPC;
> -	debug("Adding USB erratum %s\n", str);
> -	return 0;
> +	return ret;
>  }
>  
>  void fsl_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 i = 1, j;
>  	int ret;
>  
> -	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);
> +		ret = fdt_usb_get_node_type(blob, node_offset,
> +					    &node_offset, &node_type);
> +		if (ret < 0)
> +			return;
> +
> +		snprintf(str, 5, "%s%d", "usb", i++);

What would happen on a hardware with 10+ controllers here ?

>  		if (hwconfig(str)) {
>  			for (j = 0; j < ARRAY_SIZE(modes); j++) {
>  				if (hwconfig_subarg_cmp(str, "dr_mode",
> @@ -185,45 +219,20 @@ void fsl_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)
> +		ret = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
> +						  node_offset, &node_type);
> +		if (ret < 0)
>  			return;
>  
> -		usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
> -							  NULL, dr_phy_type,
> -							  usb_phy_off);
> -
> -		if (usb_phy_off < 0)
> +		ret = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
> +						  node_offset, &node_type);
> +		if (ret < 0)
>  			return;
>  
> -		ret = fsl_fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
> -					    CHIPIDEA_USB2, "a006261",
> -					    has_erratum_a006261);
> -		if (ret == -ENOSPC)
> -			return;
> -		ret = fsl_fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
> -					    CHIPIDEA_USB2, "a007075",
> -					    has_erratum_a007075);
> -		if (ret == -ENOSPC)
> -			return;
> -		ret = fsl_fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
> -					    CHIPIDEA_USB2, "a007792",
> -					    has_erratum_a007792);
> -		if (ret == -ENOSPC)
> -			return;
> -		ret = fsl_fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
> -					    CHIPIDEA_USB2, "a005697",
> -					    has_erratum_a005697);
> -		if (ret == -ENOSPC)
> -			return;
> -		ret = fsl_fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
> -					    SNPS_DWC3, "a008751",
> -					    has_erratum_a008751);
> -		if (ret == -ENOSPC)
> -			return;
> +		ret = fsl_fdt_fixup_erratum(node_offset, blob, &node_type);
> +		if (ret < 0)
> +			printf("WARNING: USB dt fixup fail , %d\n",
> +			       fdt_strerror(ret));
>  
> -	}
> +	} while (node_offset > 0);
>  }
> 


-- 
Best regards,
Marek Vasut


More information about the U-Boot mailing list