[U-Boot] [PATCH v3] 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
Wed Nov 2 09:30:34 CET 2016
>From: Marek Vasut [mailto:marex at 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.dash at 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
More information about the U-Boot
mailing list