[U-Boot] [PATCH v3] drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree
Marek Vasut
marex at denx.de
Wed Nov 2 10:36:55 CET 2016
On 11/02/2016 09:30 AM, Sriram Dash wrote:
>> 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.
So this code is not future-proof, fix it. Why do we even have to discuss
that ?
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list