| Message ID | 1477551400-21921-1-git-send-email-sriram.dash@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | York Sun |
| Headers | show |
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@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); > } >
On Freitag, 28. Oktober 2016 01:52:26 CEST Marek Vasut wrote: [...] > > + > > +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. Not true. According to http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.7.2.2 Enumeration specifiers ... An enumerator with = defines its enumeration constant as the value of the constant expression. If the first enumerator has no =, the value of its enumeration constant is 0. Each subsequent enumerator with no = defines its enumeration constant as the value of the constant expression obtained by adding 1 to the value of the previous enumeration constant. Kind regards, Stefan
>From: Marek Vasut [mailto:marex@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.dash@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
On 11/02/2016 09:30 AM, Sriram Dash wrote: >> From: Marek Vasut [mailto:marex@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.dash@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. So this code is not future-proof, fix it. Why do we even have to discuss that ?
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 +}; + +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"}, +}; + 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++) { + if (erratum_tlb[i][j].has_fsl_usb_erratum != NULL) { + 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; 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++); 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); }
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> --- 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(-)