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.d...@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.d...@nxp.com> >>> Signed-off-by: Rajesh Bhagat <rajesh.bha...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
