>From: Marek Vasut [mailto:ma...@denx.de] >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.d...@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. >
I will be using 3 linear arrays for mph, dr and snps erratum and selecting the array to fix with node type(fdt_node_check_compatible). So, enum will not be required now. >> +}; >> + >> +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. > Ok. Will split the erratum_tlb into 3 linear arrays, namely erratum_mph, erratum_dr, erratum_snps, with the respective has_erratum and strings. Then, select the appropriate array according to the node_type, via fdt_node_check_compatible(). >> +}; >> + >> 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. > Sure. >> + if (erratum_tlb[i][j].has_fsl_usb_erratum != NULL) { > >This code looks like crap, the indent is horrible and it's unnecessary. > couldn't avoid it for the current implementation. Will try to fix it as much as possible with linear array. >> + 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() > I am planning to have the erratum table with 3 linear arrays, according to the node type. So, to select the array among them, i have to check the compatible. So, I will use fdt_node_check_compatible() for the comparisons. >> 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 ? > FSL/NXP qoriq have max 3 controllers. It is unlikely that 9+ controllers will be used. External hub is a better option. >> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot