[U-Boot] [PATCH v5 0/4] Make usb device-tree fixup independent of USB controller

Makes usb device-tree fixup independent of Controller type. This enables the usage of device-tree fixup as a common framework for EHCI and XHCI controllers
Sriram Dash (4): drivers:usb:common:fsl-dt-fixup: Move device-tree fixup framework to common file drivers:usb:common:fsl-dt-fixup: Remove code duplication for fdt_usb_get_node_type drivers:usb:common:fsl-dt-fixup: Add device-tree fixup support for xhci controller drivers:usb:common:fsl-dt-fixup: fix return value of fdt_usb_get_node_type
Makefile | 1 + drivers/usb/common/Makefile | 7 ++ drivers/usb/common/fsl-dt-fixup.c | 203 ++++++++++++++++++++++++++++++++++++++ drivers/usb/host/ehci-fsl.c | 195 ------------------------------------ include/fdt_support.h | 4 +- 5 files changed, 213 insertions(+), 197 deletions(-) create mode 100644 drivers/usb/common/Makefile create mode 100644 drivers/usb/common/fsl-dt-fixup.c

Move usb device-tree fixup framework from ehci-fsl.c to common place so that it can be used by other drivers as well (xhci-fsl.c).
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com --- Changes in v5: - No update Changes in v4: - Retain copyright info - Remove #include from fsl-dt-fixup.c which are not used currently. Changes in v3: - git commit -M -C to generate patches - Break the patch 1(Moving dt fix up and removing code duplication) into 2 patches Changes in v2: - Remove the #defines from the patch
Makefile | 1 + drivers/usb/common/Makefile | 6 + .../usb/{host/ehci-fsl.c => common/fsl-dt-fixup.c} | 157 ----------------- drivers/usb/host/ehci-fsl.c | 195 --------------------- 4 files changed, 7 insertions(+), 352 deletions(-) create mode 100644 drivers/usb/common/Makefile copy drivers/usb/{host/ehci-fsl.c => common/fsl-dt-fixup.c} (54%)
diff --git a/Makefile b/Makefile index 53569e8..97a78d3 100644 --- a/Makefile +++ b/Makefile @@ -648,6 +648,7 @@ libs-$(CONFIG_SYS_FSL_DDR) += drivers/ddr/fsl/ libs-$(CONFIG_ALTERA_SDRAM) += drivers/ddr/altera/ libs-y += drivers/serial/ libs-y += drivers/usb/dwc3/ +libs-y += drivers/usb/common/ libs-y += drivers/usb/emul/ libs-y += drivers/usb/eth/ libs-y += drivers/usb/gadget/ diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile new file mode 100644 index 0000000..a38ee4a --- /dev/null +++ b/drivers/usb/common/Makefile @@ -0,0 +1,6 @@ +# (C) Copyright 2016 Freescale Semiconductor, Inc. +# +# SPDX-License-Identifier: GPL-2.0+ +# + +obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/common/fsl-dt-fixup.c similarity index 54% copy from drivers/usb/host/ehci-fsl.c copy to drivers/usb/common/fsl-dt-fixup.c index 97b7f14..92adb46 100644 --- a/drivers/usb/host/ehci-fsl.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -9,172 +9,16 @@ */
#include <common.h> -#include <pci.h> #include <usb.h> #include <asm/io.h> -#include <usb/ehci-fsl.h> #include <hwconfig.h> #include <fsl_usb.h> #include <fdt_support.h>
-#include "ehci.h" - #ifndef CONFIG_USB_MAX_CONTROLLER_COUNT #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
-static void set_txfifothresh(struct usb_ehci *, u32); - -/* Check USB PHY clock valid */ -static int usb_phy_clk_valid(struct usb_ehci *ehci) -{ - if (!((in_be32(&ehci->control) & PHY_CLK_VALID) || - in_be32(&ehci->prictrl))) { - printf("USB PHY clock invalid!\n"); - return 0; - } else { - return 1; - } -} - -/* - * Create the appropriate control structures to manage - * a new EHCI host controller. - * - * Excerpts from linux ehci fsl driver. - */ -int ehci_hcd_init(int index, enum usb_init_type init, - struct ehci_hccr **hccr, struct ehci_hcor **hcor) -{ - struct usb_ehci *ehci = NULL; - const char *phy_type = NULL; - size_t len; - char current_usb_controller[5]; -#ifdef CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY - char usb_phy[5]; - - usb_phy[0] = '\0'; -#endif - if (has_erratum_a007075()) { - /* - * A 5ms delay is needed after applying soft-reset to the - * controller to let external ULPI phy come out of reset. - * This delay needs to be added before re-initializing - * the controller after soft-resetting completes - */ - mdelay(5); - } - memset(current_usb_controller, '\0', 5); - snprintf(current_usb_controller, 4, "usb%d", index+1); - - switch (index) { - case 0: - ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB1_ADDR; - break; - case 1: - ehci = (struct usb_ehci *)CONFIG_SYS_FSL_USB2_ADDR; - break; - default: - printf("ERROR: wrong controller index!!\n"); - return -EINVAL; - }; - - *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); - *hcor = (struct ehci_hcor *)((uint32_t) *hccr + - HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); - - /* Set to Host mode */ - setbits_le32(&ehci->usbmode, CM_HOST); - - out_be32(&ehci->snoop1, SNOOP_SIZE_2GB); - out_be32(&ehci->snoop2, 0x80000000 | SNOOP_SIZE_2GB); - - /* Init phy */ - if (hwconfig_sub(current_usb_controller, "phy_type")) - phy_type = hwconfig_subarg(current_usb_controller, - "phy_type", &len); - else - phy_type = getenv("usb_phy_type"); - - if (!phy_type) { -#ifdef CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY - /* if none specified assume internal UTMI */ - strcpy(usb_phy, "utmi"); - phy_type = usb_phy; -#else - printf("WARNING: USB phy type not defined !!\n"); - return -1; -#endif - } - - if (!strncmp(phy_type, "utmi", 4)) { -#if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY) - clrsetbits_be32(&ehci->control, CONTROL_REGISTER_W1C_MASK, - PHY_CLK_SEL_UTMI); - clrsetbits_be32(&ehci->control, CONTROL_REGISTER_W1C_MASK, - UTMI_PHY_EN); - udelay(1000); /* delay required for PHY Clk to appear */ -#endif - out_le32(&(*hcor)->or_portsc[0], PORT_PTS_UTMI); - clrsetbits_be32(&ehci->control, CONTROL_REGISTER_W1C_MASK, - USB_EN); - } else { - clrsetbits_be32(&ehci->control, CONTROL_REGISTER_W1C_MASK, - PHY_CLK_SEL_ULPI); - clrsetbits_be32(&ehci->control, UTMI_PHY_EN | - CONTROL_REGISTER_W1C_MASK, USB_EN); - udelay(1000); /* delay required for PHY Clk to appear */ - if (!usb_phy_clk_valid(ehci)) - return -EINVAL; - out_le32(&(*hcor)->or_portsc[0], PORT_PTS_ULPI); - } - - out_be32(&ehci->prictrl, 0x0000000c); - out_be32(&ehci->age_cnt_limit, 0x00000040); - out_be32(&ehci->sictrl, 0x00000001); - - in_le32(&ehci->usbmode); - - if (has_erratum_a007798()) - set_txfifothresh(ehci, TXFIFOTHRESH); - - if (has_erratum_a004477()) { - /* - * When reset is issued while any ULPI transaction is ongoing - * then it may result to corruption of ULPI Function Control - * Register which eventually causes phy clock to enter low - * power mode which stops the clock. Thus delay is required - * before reset to let ongoing ULPI transaction complete. - */ - udelay(1); - } - return 0; -} - -/* - * Destroy the appropriate control structures corresponding - * the the EHCI host controller. - */ -int ehci_hcd_stop(int index) -{ - return 0; -} - -/* - * Setting the value of TXFIFO_THRESH field in TXFILLTUNING register - * to counter DDR latencies in writing data into Tx buffer. - * This prevents Tx buffer from getting underrun - */ -static void set_txfifothresh(struct usb_ehci *ehci, u32 txfifo_thresh) -{ - u32 cmd; - cmd = ehci_readl(&ehci->txfilltuning); - cmd &= ~TXFIFO_THRESH_MASK; - cmd |= TXFIFO_THRESH(txfifo_thresh); - ehci_writel(&ehci->txfilltuning, cmd); -} - -#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, const char *phy_type, int start_offset) { @@ -367,4 +211,3 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd) } } } -#endif diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index 97b7f14..ec6b8fe 100644 --- a/drivers/usb/host/ehci-fsl.c +++ b/drivers/usb/host/ehci-fsl.c @@ -173,198 +173,3 @@ static void set_txfifothresh(struct usb_ehci *ehci, u32 txfifo_thresh) cmd |= TXFIFO_THRESH(txfifo_thresh); ehci_writel(&ehci->txfilltuning, cmd); } - -#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) -static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, - const char *phy_type, int start_offset) -{ - const char *compat_dr = "fsl-usb2-dr"; - const char *compat_mph = "fsl-usb2-mph"; - const char *prop_mode = "dr_mode"; - const char *prop_type = "phy_type"; - const char *node_type = NULL; - int node_offset; - int err; - - node_offset = fdt_node_offset_by_compatible(blob, - start_offset, compat_mph); - if (node_offset < 0) { - node_offset = fdt_node_offset_by_compatible(blob, - start_offset, - compat_dr); - if (node_offset < 0) { - printf("WARNING: could not find compatible node: %s", - fdt_strerror(node_offset)); - return -1; - } - node_type = compat_dr; - } else { - node_type = compat_mph; - } - - 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)); - } - - if (phy_type) { - err = fdt_setprop(blob, node_offset, prop_type, phy_type, - strlen(phy_type) + 1); - if (err < 0) - printf("WARNING: could not set %s for %s: %s.\n", - prop_type, node_type, fdt_strerror(err)); - } - - return node_offset; -} - -static const char *fdt_usb_get_node_type(void *blob, int start_offset, - int *node_offset) -{ - const char *compat_dr = "fsl-usb2-dr"; - const char *compat_mph = "fsl-usb2-mph"; - const char *node_type = NULL; - - *node_offset = fdt_node_offset_by_compatible(blob, start_offset, - compat_mph); - if (*node_offset < 0) { - *node_offset = fdt_node_offset_by_compatible(blob, - start_offset, - compat_dr); - if (*node_offset < 0) { - printf("ERROR: could not find compatible node: %s\n", - fdt_strerror(*node_offset)); - } else { - node_type = compat_dr; - } - } else { - node_type = compat_mph; - } - - return node_type; -} - -static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum, - int start_offset) -{ - int node_offset, err; - const char *node_type = NULL; - - node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset); - if (!node_type) - return -1; - - 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 node_offset; -} - -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_mode_off = -1; - int usb_phy_off = -1; - char str[5]; - int i, j; - - for (i = 1; i <= CONFIG_USB_MAX_CONTROLLER_COUNT; i++) { - 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); - if (hwconfig(str)) { - for (j = 0; j < ARRAY_SIZE(modes); j++) { - if (hwconfig_subarg_cmp(str, "dr_mode", - modes[j])) { - mode_idx = j; - break; - } - } - - for (j = 0; j < ARRAY_SIZE(phys); j++) { - if (hwconfig_subarg_cmp(str, "phy_type", - phys[j])) { - phy_idx = j; - break; - } - } - - if (mode_idx < 0 && phy_idx < 0) { - printf("WARNING: invalid phy or mode\n"); - return; - } - - if (mode_idx > -1) - dr_mode_type = modes[mode_idx]; - - if (phy_idx > -1) - dr_phy_type = phys[phy_idx]; - } - - 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) - return; - - usb_phy_off = fdt_fixup_usb_mode_phy_type(blob, - NULL, dr_phy_type, - usb_phy_off); - - if (usb_phy_off < 0) - return; - - if (has_erratum_a006261()) { - usb_erratum_a006261_off = fdt_fixup_usb_erratum - (blob, - "fsl,usb-erratum-a006261", - usb_erratum_a006261_off); - if (usb_erratum_a006261_off < 0) - return; - } - - if (has_erratum_a007075()) { - usb_erratum_a007075_off = fdt_fixup_usb_erratum - (blob, - "fsl,usb-erratum-a007075", - usb_erratum_a007075_off); - if (usb_erratum_a007075_off < 0) - return; - } - - if (has_erratum_a007792()) { - usb_erratum_a007792_off = fdt_fixup_usb_erratum - (blob, - "fsl,usb-erratum-a007792", - usb_erratum_a007792_off); - if (usb_erratum_a007792_off < 0) - return; - } - if (has_erratum_a005697()) { - usb_erratum_a005697_off = fdt_fixup_usb_erratum - (blob, - "fsl,usb-erratum-a005697", - usb_erratum_a005697_off); - if (usb_erratum_a005697_off < 0) - return; - } - } -} -#endif

On 03/22/2016 08:10 AM, Sriram Dash wrote:
Move usb device-tree fixup framework from ehci-fsl.c to common place so that it can be used by other drivers as well (xhci-fsl.c).
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com
Acked-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut

Call fdt_usb_get_node_type() from fdt_fixup_usb_mode_phy_type() to avoid code duplication.
Signed-off-by: Sriram Dash sriram.dash@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com --- Changes in v5: - reorder the functions and gets rid of the forward declaration Changes in v4: - Make minimal modification to code Changes in v3: - Move the duplication of code to new patch
drivers/usb/common/fsl-dt-fixup.c | 72 ++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 43 deletions(-)
diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index 92adb46..eb13f12 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,33 +19,45 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
-static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, - const char *phy_type, int start_offset) +static const char *fdt_usb_get_node_type(void *blob, int start_offset, + int *node_offset) { const char *compat_dr = "fsl-usb2-dr"; const char *compat_mph = "fsl-usb2-mph"; - const char *prop_mode = "dr_mode"; - const char *prop_type = "phy_type"; const char *node_type = NULL; - int node_offset; - int err;
- node_offset = fdt_node_offset_by_compatible(blob, - start_offset, compat_mph); - if (node_offset < 0) { - node_offset = fdt_node_offset_by_compatible(blob, - start_offset, - compat_dr); - if (node_offset < 0) { - printf("WARNING: could not find compatible node: %s", - fdt_strerror(node_offset)); - return -1; + *node_offset = fdt_node_offset_by_compatible(blob, start_offset, + compat_mph); + if (*node_offset < 0) { + *node_offset = fdt_node_offset_by_compatible(blob, + start_offset, + compat_dr); + if (*node_offset < 0) { + printf("ERROR: could not find compatible node: %s\n", + fdt_strerror(*node_offset)); + } else { + node_type = compat_dr; } - node_type = compat_dr; } else { node_type = compat_mph; }
+ return node_type; +} + +static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, + const char *phy_type, int start_offset) +{ + const char *prop_mode = "dr_mode"; + const char *prop_type = "phy_type"; + const char *node_type = NULL; + int node_offset; + int err; + + node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset); + if (!node_type) + return -1; + if (mode) { err = fdt_setprop(blob, node_offset, prop_mode, mode, strlen(mode) + 1); @@ -65,32 +77,6 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, return node_offset; }
-static const char *fdt_usb_get_node_type(void *blob, int start_offset, - int *node_offset) -{ - const char *compat_dr = "fsl-usb2-dr"; - const char *compat_mph = "fsl-usb2-mph"; - const char *node_type = NULL; - - *node_offset = fdt_node_offset_by_compatible(blob, start_offset, - compat_mph); - if (*node_offset < 0) { - *node_offset = fdt_node_offset_by_compatible(blob, - start_offset, - compat_dr); - if (*node_offset < 0) { - printf("ERROR: could not find compatible node: %s\n", - fdt_strerror(*node_offset)); - } else { - node_type = compat_dr; - } - } else { - node_type = compat_mph; - } - - return node_type; -} - static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum, int start_offset) {

On 03/22/2016 08:10 AM, Sriram Dash wrote:
Call fdt_usb_get_node_type() from fdt_fixup_usb_mode_phy_type() to avoid code duplication.
Signed-off-by: Sriram Dash sriram.dash@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
Acked-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut

Enables usb device-tree fixup code to incorporate xhci controller
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com --- Changes in v5: - Make the array static const Changes in v4: - Use a terminating entry in the array for getting node type for controller Changes in v3: - Modify the Makefile to remove comparison - Put the supported controllers in array and checking from array Changes in v2: - Remove the #defines from the patch and adding controller support
drivers/usb/common/Makefile | 1 + drivers/usb/common/fsl-dt-fixup.c | 33 +++++++++++++++++---------------- include/fdt_support.h | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile index a38ee4a..2f3d43d 100644 --- a/drivers/usb/common/Makefile +++ b/drivers/usb/common/Makefile @@ -4,3 +4,4 @@ #
obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o +obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index eb13f12..13f9fb8 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,27 +19,28 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
+static const char compat_usb_fsl[] = { + "fsl-usb2-mph" "\0" + "fsl-usb2-dr" "\0" + "snps,dwc3" "\0" +}; + static const char *fdt_usb_get_node_type(void *blob, int start_offset, int *node_offset) { - const char *compat_dr = "fsl-usb2-dr"; - const char *compat_mph = "fsl-usb2-mph"; const char *node_type = NULL; - - *node_offset = fdt_node_offset_by_compatible(blob, start_offset, - compat_mph); - if (*node_offset < 0) { - *node_offset = fdt_node_offset_by_compatible(blob, - start_offset, - compat_dr); - if (*node_offset < 0) { - printf("ERROR: could not find compatible node: %s\n", - fdt_strerror(*node_offset)); - } else { - node_type = compat_dr; + const char *node_name, *nxt; + + for (node_name = compat_usb_fsl; *node_name; node_name = nxt + 1) { + nxt = node_name; + while (*nxt) + ++nxt; + *node_offset = fdt_node_offset_by_compatible + (blob, start_offset, node_name); + if (*node_offset >= 0) { + node_type = node_name; + break; } - } else { - node_type = compat_mph; }
return node_type; diff --git a/include/fdt_support.h b/include/fdt_support.h index 296add0..d34e959 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -113,11 +113,11 @@ void fdt_fixup_qe_firmware(void *fdt); */ int fdt_fixup_display(void *blob, const char *path, const char *display);
-#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) +#if defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL) void fdt_fixup_dr_usb(void *blob, bd_t *bd); #else static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {} -#endif /* defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) */ +#endif /* defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL) */
#if defined(CONFIG_SYS_FSL_SEC_COMPAT) void fdt_fixup_crypto_node(void *blob, int sec_rev);

On 03/22/2016 08:10 AM, Sriram Dash wrote:
Enables usb device-tree fixup code to incorporate xhci controller
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com
Changes in v5:
- Make the array static const
Changes in v4:
- Use a terminating entry in the array for getting node type for controller
Changes in v3:
- Modify the Makefile to remove comparison
- Put the supported controllers in array and checking from array
Changes in v2:
- Remove the #defines from the patch and adding controller support
drivers/usb/common/Makefile | 1 + drivers/usb/common/fsl-dt-fixup.c | 33 +++++++++++++++++---------------- include/fdt_support.h | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile index a38ee4a..2f3d43d 100644 --- a/drivers/usb/common/Makefile +++ b/drivers/usb/common/Makefile @@ -4,3 +4,4 @@ #
obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o +obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index eb13f12..13f9fb8 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,27 +19,28 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
+static const char compat_usb_fsl[] = {
- "fsl-usb2-mph" "\0"
- "fsl-usb2-dr" "\0"
- "snps,dwc3" "\0"
+};
This is supposed to be static constant array of strings. Can you tell me, based on your knowledge of the C language, what is wrong with this construct ?
Best regards, Marek Vasut

-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Tuesday, March 22, 2016 12:54 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH v5 3/4] drivers:usb:common:fsl-dt-fixup: Add device-tree fixup support for xhci controller
On 03/22/2016 08:10 AM, Sriram Dash wrote:
Enables usb device-tree fixup code to incorporate xhci controller
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com
Changes in v5:
- Make the array static const
Changes in v4:
- Use a terminating entry in the array for getting node type for
controller Changes in v3:
- Modify the Makefile to remove comparison
- Put the supported controllers in array and checking from array
Changes in v2:
- Remove the #defines from the patch and adding controller support
drivers/usb/common/Makefile | 1 + drivers/usb/common/fsl-dt-fixup.c | 33 +++++++++++++++++---------------- include/fdt_support.h | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile index a38ee4a..2f3d43d 100644 --- a/drivers/usb/common/Makefile +++ b/drivers/usb/common/Makefile @@ -4,3 +4,4 @@ #
obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o +obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index eb13f12..13f9fb8 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,27 +19,28 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
+static const char compat_usb_fsl[] = {
- "fsl-usb2-mph" "\0"
- "fsl-usb2-dr" "\0"
- "snps,dwc3" "\0"
+};
This is supposed to be static constant array of strings. Can you tell me, based on your knowledge of the C language, what is wrong with this construct ?
IMO, above construct is correct. Originally, we proposed array of strings but later we changed to character array with terminating NULL entry for each element to incorporate your below review comment given in v3. "My opinion is to use a terminating NULL entry and iterate over the array until you reach it."
Best regards, Marek Vasut

On 03/23/2016 09:30 AM, Sriram Dash wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Tuesday, March 22, 2016 12:54 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH v5 3/4] drivers:usb:common:fsl-dt-fixup: Add device-tree fixup support for xhci controller
On 03/22/2016 08:10 AM, Sriram Dash wrote:
Enables usb device-tree fixup code to incorporate xhci controller
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com
Changes in v5:
- Make the array static const
Changes in v4:
- Use a terminating entry in the array for getting node type for
controller Changes in v3:
- Modify the Makefile to remove comparison
- Put the supported controllers in array and checking from array
Changes in v2:
- Remove the #defines from the patch and adding controller support
drivers/usb/common/Makefile | 1 + drivers/usb/common/fsl-dt-fixup.c | 33 +++++++++++++++++---------------- include/fdt_support.h | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile index a38ee4a..2f3d43d 100644 --- a/drivers/usb/common/Makefile +++ b/drivers/usb/common/Makefile @@ -4,3 +4,4 @@ #
obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o +obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index eb13f12..13f9fb8 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,27 +19,28 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
+static const char compat_usb_fsl[] = {
- "fsl-usb2-mph" "\0"
- "fsl-usb2-dr" "\0"
- "snps,dwc3" "\0"
+};
This is supposed to be static constant array of strings. Can you tell me, based on your knowledge of the C language, what is wrong with this construct ?
IMO, above construct is correct. Originally, we proposed array of strings but later we changed to character array with terminating NULL entry for each element to incorporate your below review comment given in v3. "My opinion is to use a terminating NULL entry and iterate over the array until you reach it."
Do this:
static const char *compat_usb_fsl[] = { "fsl-usb2-mph", "fsl-usb2-dr", "snps,dwc3", NULL };
and then do
i = 0; while (compat_usb_fsl[i]) { ... do some stuff ... i++; }

On 03/23/2016 09:41 AM, Marek Vasut wrote:
On 03/23/2016 09:30 AM, Sriram Dash wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Tuesday, March 22, 2016 12:54 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH v5 3/4] drivers:usb:common:fsl-dt-fixup: Add device-tree fixup support for xhci controller
On 03/22/2016 08:10 AM, Sriram Dash wrote:
Enables usb device-tree fixup code to incorporate xhci controller
Signed-off-by: Ramneek Mehresh ramneek.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@nxp.com
Changes in v5:
- Make the array static const
Changes in v4:
- Use a terminating entry in the array for getting node type for
controller Changes in v3:
- Modify the Makefile to remove comparison
- Put the supported controllers in array and checking from array
Changes in v2:
- Remove the #defines from the patch and adding controller support
drivers/usb/common/Makefile | 1 + drivers/usb/common/fsl-dt-fixup.c | 33 +++++++++++++++++---------------- include/fdt_support.h | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile index a38ee4a..2f3d43d 100644 --- a/drivers/usb/common/Makefile +++ b/drivers/usb/common/Makefile @@ -4,3 +4,4 @@ #
obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o +obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index eb13f12..13f9fb8 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -19,27 +19,28 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif
+static const char compat_usb_fsl[] = {
- "fsl-usb2-mph" "\0"
- "fsl-usb2-dr" "\0"
- "snps,dwc3" "\0"
+};
This is supposed to be static constant array of strings. Can you tell me, based on your knowledge of the C language, what is wrong with this construct ?
IMO, above construct is correct. Originally, we proposed array of strings but later we changed to character array with terminating NULL entry for each element to incorporate your below review comment given in v3. "My opinion is to use a terminating NULL entry and iterate over the array until you reach it."
Do this:
static const char *compat_usb_fsl[] = { "fsl-usb2-mph", "fsl-usb2-dr", "snps,dwc3", NULL };
and then do
i = 0; while (compat_usb_fsl[i]) { ... do some stuff ... i++; }
Or even use for(i = 0; compat_usb_fsl[i]; i++) { do something }

Use int as it is native (and widely used) return type.
Signed-off-by: Sriram Dash sriram.dash@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com --- Changes in v5: - Modified title and description - Using error codes for return type.
drivers/usb/common/fsl-dt-fixup.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c index 13f9fb8..7183b80 100644 --- a/drivers/usb/common/fsl-dt-fixup.c +++ b/drivers/usb/common/fsl-dt-fixup.c @@ -25,11 +25,11 @@ static const char compat_usb_fsl[] = { "snps,dwc3" "\0" };
-static const char *fdt_usb_get_node_type(void *blob, int start_offset, - int *node_offset) +static int fdt_usb_get_node_type(void *blob, int start_offset, + int *node_offset, const char **node_type) { - const char *node_type = NULL; const char *node_name, *nxt; + int ret = -ENOENT;
for (node_name = compat_usb_fsl; *node_name; node_name = nxt + 1) { nxt = node_name; @@ -38,12 +38,13 @@ static const char *fdt_usb_get_node_type(void *blob, int start_offset, *node_offset = fdt_node_offset_by_compatible (blob, start_offset, node_name); if (*node_offset >= 0) { - node_type = node_name; + *node_type = node_name; + ret = 0; break; } }
- return node_type; + return ret; }
static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, @@ -55,9 +56,10 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, int node_offset; int err;
- node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset); - if (!node_type) - return -1; + err = fdt_usb_get_node_type(blob, start_offset, + &node_offset, &node_type); + if (err < 0) + return err;
if (mode) { err = fdt_setprop(blob, node_offset, prop_mode, mode, @@ -84,9 +86,10 @@ static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum, int node_offset, err; const char *node_type = NULL;
- node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset); - if (!node_type) - return -1; + err = fdt_usb_get_node_type(blob, start_offset, + &node_offset, &node_type); + if (err < 0) + return err;
err = fdt_setprop(blob, node_offset, prop_erratum, NULL, 0); if (err < 0) {

On 03/22/2016 08:10 AM, Sriram Dash wrote:
Use int as it is native (and widely used) return type.
What do you mean by "int ... is native (and widely used) return type" ?
It's just a signed type, that's all there is to it.
Signed-off-by: Sriram Dash sriram.dash@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
[...]
Best regards, Marek Vasut

-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Tuesday, March 22, 2016 12:56 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH v5 4/4] drivers:usb:common:fsl-dt-fixup: fix return value of fdt_usb_get_node_type
On 03/22/2016 08:10 AM, Sriram Dash wrote:
Use int as it is native (and widely used) return type.
What do you mean by "int ... is native (and widely used) return type" ?
It's just a signed type, that's all there is to it.
Ok that may sound incorrect, Shall I change the description as below: "Changes the return type of fdt_usb_get_node_type from char* to int"
Signed-off-by: Sriram Dash sriram.dash@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
[...]
Best regards, Marek Vasut

On 03/23/2016 09:31 AM, Sriram Dash wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Tuesday, March 22, 2016 12:56 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH v5 4/4] drivers:usb:common:fsl-dt-fixup: fix return value of fdt_usb_get_node_type
On 03/22/2016 08:10 AM, Sriram Dash wrote:
Use int as it is native (and widely used) return type.
What do you mean by "int ... is native (and widely used) return type" ?
It's just a signed type, that's all there is to it.
Ok that may sound incorrect, Shall I change the description as below: "Changes the return type of fdt_usb_get_node_type from char* to int"
Yeah, excellent.
participants (2)
-
Marek Vasut
-
Sriram Dash