[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
Thu Sep 15 08:40:52 CEST 2016
>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: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 ?
>>
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 <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.
>
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
More information about the U-Boot
mailing list