>From: Marek Vasut [mailto:ma...@denx.de] >On 09/16/2016 10:47 AM, Sriram Dash wrote: >>> From: Marek Vasut [mailto:ma...@denx.de] On 09/15/2016 12:29 AM, york >>> sun wrote: >>>> 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. >>> >>> The fdt_fixup_erratum() function is named in the most confusing dumb >>> way, it is not a generic function but a FSL/NXP specific one. Except >>> it does look like a generic one. This should've never made it mainline and >>> it >should be renamed. >>> >> >> Yes. I agree to your point. We will rename the functions and send a >> patch differently for the same. > >Good > >>> Regarding the return above, it's just that whoever implemented that >>> function did it in the most broken way possible. ENOSPC, according to >>> errno(3) means "ENOSPC No space left on device (POSIX.1)", which is >>> completely unrelated to anything USB in this context. Worse yet, the >>> error is >> >> Yes. The ENOSPC is not related to USB. Also, this code fixes the >> device tree, which will not affect usb functionality in U boot. > >OK > >>> returned whenever fdt_fixup_usb_erratum() returns negative value , >>> instead of propagating errors. >>> >> >> The error is returned when fixup_usb_erratum returns a negative value. >> Now there are two possibilities. >> - The erratum is not to be applicable for the Soc, where the >> fdt_fixup_erratum returns EINVAL if (!has_erratum()) >> return -EINVAL; >> In this case, we want to continue applying other errata. >> - The erratum is to be applicable, but the fdt_fixup_usb_erratum fails to >> apply it. >> if (node_offset < 0) >> return -ENOSPC; >> In this case, the failure will be inability to apply the erratum due >> to lack of space in device tree. So, check for the ENOSPC and return >> from the function fdt_fixup_dr_usb, as no other erratum can be applied >> further. > >And what happens if fdt_fixup_usb_erratum() doesn't fail because of not enough >space in DT ? Then the information gets lost in here. So I don't really care >about this >elaborate explanation, this should be fixed and the errors should be propagated >correctly. >
OK. >> As York also pointed out regarding return in case of each error, we >> can avoid the returns and this will make the code clean and simple. >> But, the fdt_fixup_dr_usb will get executed till the device tree is >> traversed completely, regardless of the modifications taking effect over >> device >tree. >> What do you think? > >I think you should fix your error paths before you tackle this error handling >here at >all, otherwise it's gonna be an annoying mess. > >The way I'd do this is I'd have a table of fdt_fixup_erratum() arguments and >just >iterate over the table. Then the code becomes minimal. > OK. Will take it in next patch v3. >>> I think the aforementioned two points should be fixed first and only >>> then should the return value above be handled correctly based on what the >>> value >is. >>> >>> I am real disappointed when looking at this crap. >>> >>>> Other than that, this patch looks OK. I didn't test it by compiling >>>> or running on a board. >>> >>> Please do. >>> >>>> York >>>> >>> >>> >>> -- >>> Best regards, >>> Marek Vasut > > >-- >Best regards, >Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
