[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
Fri Oct 28 01:52:26 CEST 2016
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.
> +};
> +
> +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.
> +};
> +
> 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.
> + if (erratum_tlb[i][j].has_fsl_usb_erratum != NULL) {
This code looks like crap, the indent is horrible and it's unnecessary.
> + 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()
> 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 ?
> 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