>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:[email protected]] >>>> >>> >>> 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 <[email protected]> >>>> Signed-off-by: Rajesh Bhagat <[email protected]> >>>> --- >>>> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
