[U-Boot] [PATCH v2] drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree
Sriram Dash
sriram.dash at nxp.com
Fri Sep 16 12:27:36 CEST 2016
>From: Marek Vasut [mailto:marex at denx.de]
>On 09/16/2016 10:47 AM, Sriram Dash wrote:
>>> From: Marek Vasut [mailto:marex at 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.dash at 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.dash at nxp.com>
>>>>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at 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
More information about the U-Boot
mailing list