[U-Boot] [PATCH v2] drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree
Marek Vasut
marex at denx.de
Fri Sep 16 00:01:36 CEST 2016
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.
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 returned whenever fdt_fixup_usb_erratum() returns negative
value , instead of propagating errors.
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
More information about the U-Boot
mailing list