>From: Marek Vasut [mailto:ma...@denx.de]
>On 10/27/2016 08:56 AM, Sriram Dash wrote:
>> 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>
>> ---
>> Depends on the following patch:
>> https://patchwork.ozlabs.org/patch/682139/
>>
>> Changes in v3:
>>   - Remove ENOSPC.
>>   - Create a table of USB erratum, to be used to fix dt.
>>
>> Changes in v2:
>>   - Modify patch description and title
>>   - Remove counting the dt nodes
>>
>>  drivers/usb/common/fsl-dt-fixup.c | 209
>> ++++++++++++++++++++------------------
>>  1 file changed, 109 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>> b/drivers/usb/common/fsl-dt-fixup.c
>> index 63a24f7..addeb8c 100644
>> --- a/drivers/usb/common/fsl-dt-fixup.c
>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>> @@ -16,10 +16,6 @@
>>  #include <fsl_usb.h>
>>  #include <fdt_support.h>
>>
>> -#ifndef CONFIG_USB_MAX_CONTROLLER_COUNT -#define
>> CONFIG_USB_MAX_CONTROLLER_COUNT 1 -#endif
>> -
>>  /* USB Controllers */
>>  #define FSL_USB2_MPH        "fsl-usb2-mph"
>>  #define FSL_USB2_DR "fsl-usb2-dr"
>> @@ -33,6 +29,52 @@ static const char * const compat_usb_fsl[] = {
>>      NULL
>>  };
>>
>> +enum usb_node_type {
>> +    USB2_MPH = 0,
>> +    USB2_DR,
>> +    SNPS,
>> +    USB_COMPAT_END
>> +};
>> +
>> +enum fdt_fsl_usb_erratum {
>> +    A006261,
>> +    A007075,
>> +    A007792,
>> +    A005697,
>> +    A008751,
>> +    FSL_USB_ERRATUM_END
>
>The compiler can assign completely arbitrary numbers to the enum elements.
>Moreover, you don't need this "terminating entry" anyway, see below.
>

I will be using 3 linear arrays for mph, dr and snps erratum and selecting
the array to fix with node type(fdt_node_check_compatible).
So, enum will not be required now.

>> +};
>> +
>> +struct fsl_dt_usb_erratum {
>> +    bool (*has_fsl_usb_erratum)(void);
>> +    char *dt_set_prop;
>> +};
>> +
>> +struct fsl_dt_usb_erratum
>> +    erratum_tlb[USB_COMPAT_END][FSL_USB_ERRATUM_END] = { /*MPH
>Erratum
>> +*/
>> +    [USB2_MPH][A006261] = {&has_erratum_a006261,
>> +                           "fsl,usb-erratum-a006261"},
>> +    [USB2_MPH][A007075] = {&has_erratum_a007075,
>> +                           "fsl,usb-erratum-a007075"},
>> +    [USB2_MPH][A007792] = {&has_erratum_a007792,
>> +                           "fsl,usb-erratum-a007792"},
>> +    [USB2_MPH][A005697] = {&has_erratum_a005697,
>> +                           "fsl,usb-erratum-a005697"}, /*DR Erratum */
>> +    [USB2_DR][A006261] = {&has_erratum_a006261,
>> +                          "fsl,usb-erratum-a006261"},
>> +    [USB2_DR][A007075] = {&has_erratum_a007075,
>> +                          "fsl,usb-erratum-a007075"},
>> +    [USB2_DR][A007792] = {&has_erratum_a007792,
>> +                          "fsl,usb-erratum-a007792"},
>> +    [USB2_DR][A005697] = {&has_erratum_a005697,
>> +                          "fsl,usb-erratum-a005697"},
>> +/*SNPS Erratum */
>> +    [SNPS][A008751] = {&has_erratum_a008751,
>> +                       "fsl,usb-erratum-a008751"},
>
>Please just split this into three arrays.
>

Ok. Will split the erratum_tlb into 3 linear arrays, namely 
erratum_mph, erratum_dr, erratum_snps, with the respective
has_erratum and strings. Then, select the appropriate array
according to the node_type, via fdt_node_check_compatible().

>> +};
>> +
>>  static int fdt_usb_get_node_type(void *blob, int start_offset,
>>                               int *node_offset, const char **node_type)  { @@
>-54,25 +96,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,79 +116,77 @@ 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 fsl_fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
>> -                                 const char *controller_type,
>> -                                 int start_offset)
>> +static int fsl_fdt_fixup_usb_erratum(int node_offset, void *blob,
>> +                                 int dt_node_type)
>>  {
>> -    int node_offset, err;
>> -    const char *node_type = NULL;
>> -    const char *node_name = NULL;
>> +    int i, j, ret = -1;
>> +    for (i = dt_node_type; i < USB_COMPAT_END; i++) {
>> +            for (j = 0; j < FSL_USB_ERRATUM_END; j++) {
>
>You can use ARRAY_SIZE() here if this was linear array, which it should be.
>

Sure.

>> +                    if (erratum_tlb[i][j].has_fsl_usb_erratum != NULL) {
>
>This code looks like crap, the indent is horrible and it's unnecessary.
>

couldn't avoid it for the current implementation. Will try to fix it
as much as possible with linear array.

>> +                            if (!erratum_tlb[i][j].has_fsl_usb_erratum())
>> +                                    continue;
>> +                            ret = fdt_setprop(blob, node_offset,
>> +                                    erratum_tlb[i][j].dt_set_prop,
>> +                                    NULL, 0);
>> +                            if (ret < 0) {
>> +                                    printf("ERROR: could not set %s: %s.\n",
>> +                                           erratum_tlb[i][j].dt_set_prop,
>> +                                           fdt_strerror(ret));
>> +                                    return ret;
>> +                            }
>> +                    }
>> +            }
>> +    }
>> +    return ret;
>> +}
>>
>> -    err = fdt_usb_get_node_type(blob, start_offset,
>> -                                &node_offset, &node_type);
>> -    if (err < 0)
>> -            return err;
>> +static int fsl_fdt_fixup_erratum(int node_offset, void *blob,
>> +                             const char **node_type)
>> +{
>> +    int dt_node_type;
>> +    int ret;
>>
>> -    if (!strcmp(node_type, FSL_USB2_MPH) || !strcmp(node_type,
>FSL_USB2_DR))
>> -            node_name = CHIPIDEA_USB2;
>> +    if (!strcmp(*node_type, FSL_USB2_MPH))
>> +            dt_node_type = USB2_MPH;
>> +    else if (!strcmp(*node_type, FSL_USB2_DR))
>> +            dt_node_type = USB2_DR;
>> +    else if (!strcmp(*node_type, SNPS_DWC3))
>> +            dt_node_type = SNPS;
>
>So uh ... your code tests node compatibility here only to test it in
>fsl_fdt_fixup_usb_erratum() again. This wouldn't be needed if you used linear 
>array.
>btw use fdt_node_check_compatible()
>

I am planning to have the erratum table with 3 linear arrays, according 
to the node type. So, to select the array among them, i have to check
the compatible. So, I will use fdt_node_check_compatible()
for the comparisons.

>>      else
>> -            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));
>> -    }
>> +            return -ENOENT;
>>
>> -    return node_offset;
>> -}
>> +    ret = fsl_fdt_fixup_usb_erratum(node_offset, blob, dt_node_type);
>>
>> -static int fsl_fdt_fixup_erratum(int *usb_erratum_off, void *blob,
>> -                             const char *controller_type, char *str,
>> -                             bool (*has_erratum)(void))
>> -{
>> -    char buf[32] = {0};
>> -
>> -    snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
>> -    if (!has_erratum())
>> -            return -EINVAL;
>> -    *usb_erratum_off = fsl_fdt_fixup_usb_erratum(blob, buf, controller_type,
>> -                                                 *usb_erratum_off);
>> -    if (*usb_erratum_off < 0)
>> -            return -ENOSPC;
>> -    debug("Adding USB erratum %s\n", str);
>> -    return 0;
>> +    return ret;
>>  }
>>
>>  void fsl_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 i = 1, j;
>>      int ret;
>>
>> -    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);
>> +            ret = fdt_usb_get_node_type(blob, node_offset,
>> +                                        &node_offset, &node_type);
>> +            if (ret < 0)
>> +                    return;
>> +
>> +            snprintf(str, 5, "%s%d", "usb", i++);
>
>What would happen on a hardware with 10+ controllers here ?
>

FSL/NXP qoriq have max 3 controllers. It is unlikely that 9+ controllers
will be used. External hub is a better option.


>>              if (hwconfig(str)) {
>>                      for (j = 0; j < ARRAY_SIZE(modes); j++) {
>>                              if (hwconfig_subarg_cmp(str, "dr_mode", @@ -
>185,45 +219,20 @@
>> void fsl_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)
>> +            ret = fdt_fixup_usb_mode_phy_type(blob, dr_mode_type, NULL,
>> +                                              node_offset, &node_type);
>> +            if (ret < 0)
>>                      return;
>>
>> -            usb_phy_off = fdt_fixup_usb_mode_phy_type(blob,
>> -                                                      NULL, dr_phy_type,
>> -                                                      usb_phy_off);
>> -
>> -            if (usb_phy_off < 0)
>> +            ret = fdt_fixup_usb_mode_phy_type(blob, NULL, dr_phy_type,
>> +                                              node_offset, &node_type);
>> +            if (ret < 0)
>>                      return;
>>
>> -            ret = fsl_fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
>> -                                        CHIPIDEA_USB2, "a006261",
>> -                                        has_erratum_a006261);
>> -            if (ret == -ENOSPC)
>> -                    return;
>> -            ret = fsl_fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
>> -                                        CHIPIDEA_USB2, "a007075",
>> -                                        has_erratum_a007075);
>> -            if (ret == -ENOSPC)
>> -                    return;
>> -            ret = fsl_fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
>> -                                        CHIPIDEA_USB2, "a007792",
>> -                                        has_erratum_a007792);
>> -            if (ret == -ENOSPC)
>> -                    return;
>> -            ret = fsl_fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
>> -                                        CHIPIDEA_USB2, "a005697",
>> -                                        has_erratum_a005697);
>> -            if (ret == -ENOSPC)
>> -                    return;
>> -            ret = fsl_fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
>> -                                        SNPS_DWC3, "a008751",
>> -                                        has_erratum_a008751);
>> -            if (ret == -ENOSPC)
>> -                    return;
>> +            ret = fsl_fdt_fixup_erratum(node_offset, blob, &node_type);
>> +            if (ret < 0)
>> +                    printf("WARNING: USB dt fixup fail , %d\n",
>> +                           fdt_strerror(ret));
>>
>> -    }
>> +    } while (node_offset > 0);
>>  }
>>
>
>
>--
>Best regards,
>Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to