[U-Boot] [PATCH v5 00/11] rk3328: add support of usb host and gadget
Add support of usb host and gadget function for rk3328.
Changes in v5: - Propagate return value and print error message when failed
Changes in v4: - Splite patch [U-boot,v3,04/10] into two patches - Define set vbus as empty function if the macros aren't set - Prepare a mask and set capabilities in GUSBCFG register once
Changes in v3: - Revert change of macro definition in dwc2 driver - Support host mode without HNP/SRP capability through DTS
Changes in v2: - Add commit messages - Split patch [U-boot,7/8] into two patches - Use fixed regulator to control vbus instead of gpio
Meng Dongyang (11): rockchip: configs: rk3328: support ehci and ohci controller rockchip: dts: rk3328: add ehci and ohci node and enable host0 port rockchip: configs: rk3328: config xhci controller usb: host: xhci-rockchip: use fixed regulator to control vbus usb: host: xhci-rockchip: add support for rk3328 rockchip: dts: rk3328: support and enable xhci rockchip: configs: rk3328: enable dwc2 driver and config fastboot usb: dwc2: force to host mode if not support HNP/SRP rockchip: rk3328: board: add support of dwc2 gadget rockchip: dts: rk3328: support and enable dwc2 rockchip: dts: rk3399: control vbus of typec by fixed regulator
arch/arm/dts/rk3328-evb.dts | 36 +++++++++++++++++++++ arch/arm/dts/rk3328.dtsi | 35 +++++++++++++++++++++ arch/arm/dts/rk3399-evb.dts | 16 ++++++++-- board/rockchip/evb_rk3328/evb-rk3328.c | 43 +++++++++++++++++++++++-- configs/evb-rk3328_defconfig | 25 +++++++++++++++ drivers/usb/host/dwc2.c | 19 ++++++++++-- drivers/usb/host/xhci-rockchip.c | 57 ++++++++++++++++++++++++++-------- include/configs/rk3328_common.h | 9 ++++++ 8 files changed, 219 insertions(+), 21 deletions(-)
Add defconfig for usb and ehci and ohci controller, config maximal number of ports of the root hub for ohci driver.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None
configs/evb-rk3328_defconfig | 8 ++++++++ include/configs/rk3328_common.h | 3 +++ 2 files changed, 11 insertions(+)
diff --git a/configs/evb-rk3328_defconfig b/configs/evb-rk3328_defconfig index 96241f6..a4312b2 100644 --- a/configs/evb-rk3328_defconfig +++ b/configs/evb-rk3328_defconfig @@ -31,3 +31,11 @@ CONFIG_SYS_NS16550=y CONFIG_SYSRESET=y CONFIG_USE_TINY_PRINTF=y CONFIG_ERRNO_STR=y +CONFIG_USB=y +CONFIG_DM_USB=y +CONFIG_CMD_USB=y +CONFIG_USB_STORAGE=y +CONFIG_USB_EHCI_HCD=y +CONFIG_USB_EHCI_GENERIC=y +CONFIG_USB_OHCI_HCD=y +CONFIG_USB_OHCI_GENERIC=y diff --git a/include/configs/rk3328_common.h b/include/configs/rk3328_common.h index b0dcd48..96b71c3 100644 --- a/include/configs/rk3328_common.h +++ b/include/configs/rk3328_common.h @@ -61,4 +61,7 @@
#endif
+/* rockchip ohci host driver */ +#define CONFIG_USB_OHCI_NEW +#define CONFIG_SYS_USB_OHCI_MAX_ROOT_PORTS 1 #endif
Add dts node for ehci and ohci controller, enable the controllers.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
arch/arm/dts/rk3328-evb.dts | 8 ++++++++ arch/arm/dts/rk3328.dtsi | 14 ++++++++++++++ 2 files changed, 22 insertions(+)
diff --git a/arch/arm/dts/rk3328-evb.dts b/arch/arm/dts/rk3328-evb.dts index 01794ed..9920935 100644 --- a/arch/arm/dts/rk3328-evb.dts +++ b/arch/arm/dts/rk3328-evb.dts @@ -43,3 +43,11 @@ pinctrl-0 = <&emmc_clk &emmc_cmd &emmc_bus8>; status = "okay"; }; + +&usb_host0_ehci { + status = "okay"; +}; + +&usb_host0_ohci { + status = "okay"; +}; diff --git a/arch/arm/dts/rk3328.dtsi b/arch/arm/dts/rk3328.dtsi index 8a98ee3..e1219c3 100644 --- a/arch/arm/dts/rk3328.dtsi +++ b/arch/arm/dts/rk3328.dtsi @@ -446,6 +446,20 @@ status = "disabled"; };
+ usb_host0_ehci: usb@ff5c0000 { + compatible = "generic-ehci"; + reg = <0x0 0xff5c0000 0x0 0x10000>; + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + }; + + usb_host0_ohci: usb@ff5d0000 { + compatible = "generic-ohci"; + reg = <0x0 0xff5d0000 0x0 0x10000>; + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + }; + sdmmc_ext: rksdmmc@ff5f0000 { compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc"; reg = <0x0 0xff5f0000 0x0 0x4000>;
Add config of max root ports and add config to enable xhci controller.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None
configs/evb-rk3328_defconfig | 3 +++ include/configs/rk3328_common.h | 3 +++ 2 files changed, 6 insertions(+)
diff --git a/configs/evb-rk3328_defconfig b/configs/evb-rk3328_defconfig index a4312b2..57aee02 100644 --- a/configs/evb-rk3328_defconfig +++ b/configs/evb-rk3328_defconfig @@ -39,3 +39,6 @@ CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_GENERIC=y CONFIG_USB_OHCI_HCD=y CONFIG_USB_OHCI_GENERIC=y +CONFIG_USB_XHCI_DWC3=y +CONFIG_USB_XHCI_HCD=y +CONFIG_USB_XHCI_RK=y diff --git a/include/configs/rk3328_common.h b/include/configs/rk3328_common.h index 96b71c3..cc6c890 100644 --- a/include/configs/rk3328_common.h +++ b/include/configs/rk3328_common.h @@ -64,4 +64,7 @@ /* rockchip ohci host driver */ #define CONFIG_USB_OHCI_NEW #define CONFIG_SYS_USB_OHCI_MAX_ROOT_PORTS 1 + +/* xhci host */ +#define CONFIG_SYS_USB_XHCI_MAX_ROOT_PORTS 2 #endif
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com ---
Changes in v5: - Propagate return value and print error message when failed
Changes in v4: - Splited from patch [Uboot,v3,04/10] - Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2: - Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h>
#include "xhci.h"
@@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base; - struct gpio_desc vbus_gpio; + struct udevice *vbus_supply; };
/* @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) { - debug("Can't get the XHCI register base address\n"); + error("Can't get the XHCI register base address\n"); return -ENXIO; }
@@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) }
if (plat->phy_base == FDT_ADDR_T_NONE) { - debug("Can't get the usbphy register address\n"); + error("Can't get the usbphy register address\n"); return -ENXIO; }
- /* Vbus gpio */ - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, - &plat->vbus_gpio, GPIOD_IS_OUT); +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) + /* Vbus regulator */ + ret = device_get_supply_regulator(dev, "vbus-supply", + &plat->vbus_supply); if (ret) - debug("rockchip,vbus-gpio node missing!"); + debug("Can't get VBus regulator!\n"); +#endif
return 0; }
+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{ + int ret; + + ret = regulator_set_enable(vbus_supply, value); + if (ret) + error("XHCI: Failed to set vbus supply\n"); + + return ret; +} +#else +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{ + return 0; +} +#endif + /* * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core * @dwc: Pointer to our controller context structure @@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci,
ret = dwc3_core_init(rkxhci->dwc3_reg); if (ret) { - debug("failed to initialize core\n"); + error("failed to initialize core\n"); return ret; }
@@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase)));
- /* setup the Vbus gpio here */ - if (dm_gpio_is_valid(&plat->vbus_gpio)) - dm_gpio_set_value(&plat->vbus_gpio, 1); + if (plat->vbus_supply) { + ret = rockchip_xhci_set_vbus(plat->vbus_supply, true); + if (ret) + return ret; + }
ret = rockchip_xhci_core_init(ctx, dev); if (ret) { - debug("XHCI: failed to initialize controller\n"); + error("XHCI: failed to initialize controller\n"); return ret; }
@@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev)
static int xhci_usb_remove(struct udevice *dev) { + struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); struct rockchip_xhci *ctx = dev_get_priv(dev); int ret;
@@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) if (ret) return ret;
+ if (plat->vbus_supply) { + ret = rockchip_xhci_set_vbus(plat->vbus_supply, false); + if (ret) + return ret; + } + return 0; }
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
- Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h>
#include "xhci.h"
@@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
- struct udevice *vbus_supply;
};
/* @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n");@@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) }
if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n");
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);
So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
if (ret)
debug("rockchip,vbus-gpio node missing!");
debug("Can't get VBus regulator!\n");+#endif
return 0; }
+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{
- int ret;
- ret = regulator_set_enable(vbus_supply, value);
- if (ret)
error("XHCI: Failed to set vbus supply\n");
Please be consistent with the VBus usage. You use VBus above and vbus here.
- return ret;
+} +#else +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{
- return 0;
+} +#endif
/*
- rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core
- @dwc: Pointer to our controller context structure
@@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci,
ret = dwc3_core_init(rkxhci->dwc3_reg); if (ret) {
debug("failed to initialize core\n");
error("failed to initialize core\n");@@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase)));
- /* setup the Vbus gpio here */
- if (dm_gpio_is_valid(&plat->vbus_gpio))
dm_gpio_set_value(&plat->vbus_gpio, 1);
if (plat->vbus_supply) {
ret = rockchip_xhci_set_vbus(plat->vbus_supply, true);if (ret)return ret;}
ret = rockchip_xhci_core_init(ctx, dev); if (ret) {
debug("XHCI: failed to initialize controller\n");
error("XHCI: failed to initialize controller\n");@@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev)
static int xhci_usb_remove(struct udevice *dev) {
- struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); struct rockchip_xhci *ctx = dev_get_priv(dev); int ret;
@@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) if (ret) return ret;
- if (plat->vbus_supply) {
ret = rockchip_xhci_set_vbus(plat->vbus_supply, false);if (ret)return ret;- }
- return 0;
Just do return ret here, then you don't need the if (ret) above.
}
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h>
#include "xhci.h"
@@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
struct udevice *vbus_supply; };
/*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n");@@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) }
if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n");
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
if (ret)
debug("rockchip,vbus-gpio node missing!");
debug("Can't get VBus regulator!\n");+#endif
return 0; }
+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{
- int ret;
- ret = regulator_set_enable(vbus_supply, value);
- if (ret)
error("XHCI: Failed to set vbus supply\n");Please be consistent with the VBus usage. You use VBus above and vbus here.
OK
- return ret;
+} +#else +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{
- return 0;
+} +#endif
- /*
- rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core
- @dwc: Pointer to our controller context structure
@@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci,
ret = dwc3_core_init(rkxhci->dwc3_reg); if (ret) {
debug("failed to initialize core\n");
error("failed to initialize core\n");@@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase)));
- /* setup the Vbus gpio here */
- if (dm_gpio_is_valid(&plat->vbus_gpio))
dm_gpio_set_value(&plat->vbus_gpio, 1);
if (plat->vbus_supply) {
ret = rockchip_xhci_set_vbus(plat->vbus_supply, true);if (ret)return ret;}
ret = rockchip_xhci_core_init(ctx, dev); if (ret) {
debug("XHCI: failed to initialize controller\n");
error("XHCI: failed to initialize controller\n");@@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev)
static int xhci_usb_remove(struct udevice *dev) {
- struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); struct rockchip_xhci *ctx = dev_get_priv(dev); int ret;
@@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) if (ret) return ret;
- if (plat->vbus_supply) {
ret = rockchip_xhci_set_vbus(plat->vbus_supply, false);if (ret)return ret;- }
- return 0;
Just do return ret here, then you don't need the if (ret) above.
OK
}
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55
++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h> #include "xhci.h" @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
- struct udevice *vbus_supply; }; /*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n"); return -ENXIO; }udevice *dev) } if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n"); return -ENXIO; }
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
Daniel, I recommend you fix your mailer to insert at least one newline before the reply text.
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55
++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h> #include "xhci.h" @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
- struct udevice *vbus_supply; }; /*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n"); return -ENXIO; }udevice *dev) } if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n"); return -ENXIO; }
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Daniel, I recommend you fix your mailer to insert at least one newline before the reply text.
-- Best regards, Marek Vasut
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55
++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h> #include "xhci.h" @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
- struct udevice *vbus_supply; }; /*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n"); return -ENXIO; }udevice *dev) } if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n"); return -ENXIO; }
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote:
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5:
- Propagate return value and print error message when failed
Changes in v4:
- Splited from patch [Uboot,v3,04/10]
- Define set vbus as empty function if the macros aren't set
Changes in v3: None Changes in v2:
Use fixed regulator to control vbus instead of gpio
drivers/usb/host/xhci-rockchip.c | 55
++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h> #include "xhci.h" @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base;
- struct gpio_desc vbus_gpio;
- struct udevice *vbus_supply; }; /*
@@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) {
debug("Can't get the XHCI register base address\n");
error("Can't get the XHCI register base address\n"); return -ENXIO; }udevice *dev) } if (plat->phy_base == FDT_ADDR_T_NONE) {
debug("Can't get the usbphy register address\n");
error("Can't get the usbphy register address\n"); return -ENXIO; }
- /* Vbus gpio */
- ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
&plat->vbus_gpio, GPIOD_IS_OUT);+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
- /* Vbus regulator */
- ret = device_get_supply_regulator(dev, "vbus-supply",
&plat->vbus_supply);So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Regards, Simon
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote:
On 06/12/2017 11:19 AM, Meng Dongyang wrote: > Use fixed regulator to control the voltage of vbus and turn off > vbus when usb stop. > > Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com > --- > > Changes in v5: > - Propagate return value and print error message when failed > > Changes in v4: > - Splited from patch [Uboot,v3,04/10] > - Define set vbus as empty function if the macros aren't set > > Changes in v3: None > Changes in v2: > - Use fixed regulator to control vbus instead of gpio > > drivers/usb/host/xhci-rockchip.c | 55 > ++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/host/xhci-rockchip.c > b/drivers/usb/host/xhci-rockchip.c > index f559830..15df6ef 100644 > --- a/drivers/usb/host/xhci-rockchip.c > +++ b/drivers/usb/host/xhci-rockchip.c > @@ -11,10 +11,10 @@ > #include <malloc.h> > #include <usb.h> > #include <watchdog.h> > -#include <asm/gpio.h> > #include <linux/errno.h> > #include <linux/compat.h> > #include <linux/usb/dwc3.h> > +#include <power/regulator.h> > #include "xhci.h" > @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; > struct rockchip_xhci_platdata { > fdt_addr_t hcd_base; > fdt_addr_t phy_base; > - struct gpio_desc vbus_gpio; > + struct udevice *vbus_supply; > }; > /* > @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct > udevice *dev) > */ > plat->hcd_base = dev_get_addr(dev); > if (plat->hcd_base == FDT_ADDR_T_NONE) { > - debug("Can't get the XHCI register base address\n"); > + error("Can't get the XHCI register base address\n"); > return -ENXIO; > } > @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct > udevice *dev) > } > if (plat->phy_base == FDT_ADDR_T_NONE) { > - debug("Can't get the usbphy register address\n"); > + error("Can't get the usbphy register address\n"); > return -ENXIO; > } > - /* Vbus gpio */ > - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, > - &plat->vbus_gpio, GPIOD_IS_OUT); > +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway.
Yes, I will remove it in v6.
> + /* Vbus regulator */ > + ret = device_get_supply_regulator(dev, "vbus-supply", > + &plat->vbus_supply); So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ?
This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
On 2017/6/13 17:11, Marek Vasut wrote: > On 06/12/2017 11:19 AM, Meng Dongyang wrote: >> Use fixed regulator to control the voltage of vbus and turn off >> vbus when usb stop. >> >> Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com >> --- >> >> Changes in v5: >> - Propagate return value and print error message when failed >> >> Changes in v4: >> - Splited from patch [Uboot,v3,04/10] >> - Define set vbus as empty function if the macros aren't set >> >> Changes in v3: None >> Changes in v2: >> - Use fixed regulator to control vbus instead of gpio >> >> drivers/usb/host/xhci-rockchip.c | 55 >> ++++++++++++++++++++++++++++++---------- >> 1 file changed, 42 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-rockchip.c >> b/drivers/usb/host/xhci-rockchip.c >> index f559830..15df6ef 100644 >> --- a/drivers/usb/host/xhci-rockchip.c >> +++ b/drivers/usb/host/xhci-rockchip.c >> @@ -11,10 +11,10 @@ >> #include <malloc.h> >> #include <usb.h> >> #include <watchdog.h> >> -#include <asm/gpio.h> >> #include <linux/errno.h> >> #include <linux/compat.h> >> #include <linux/usb/dwc3.h> >> +#include <power/regulator.h> >> #include "xhci.h" >> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >> struct rockchip_xhci_platdata { >> fdt_addr_t hcd_base; >> fdt_addr_t phy_base; >> - struct gpio_desc vbus_gpio; >> + struct udevice *vbus_supply; >> }; >> /* >> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >> udevice *dev) >> */ >> plat->hcd_base = dev_get_addr(dev); >> if (plat->hcd_base == FDT_ADDR_T_NONE) { >> - debug("Can't get the XHCI register base address\n"); >> + error("Can't get the XHCI register base address\n"); >> return -ENXIO; >> } >> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >> udevice *dev) >> } >> if (plat->phy_base == FDT_ADDR_T_NONE) { >> - debug("Can't get the usbphy register address\n"); >> + error("Can't get the usbphy register address\n"); >> return -ENXIO; >> } >> - /* Vbus gpio */ >> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >> - &plat->vbus_gpio, GPIOD_IS_OUT); >> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) > I don't think you need the CONFIG_DM_USB , the driver depends on it (or > should) already anyway.
Yes, I will remove it in v6. > >> + /* Vbus regulator */ >> + ret = device_get_supply_regulator(dev, "vbus-supply", >> + &plat->vbus_supply); > So I was curious, does this expand to empty function or is this not > defined if CONFIG_DM_REGULATOR is not defined ? This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
USB would not work without VBUS, which is handled with regulators, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Meng can you please explain why the #ifdef is needed?
Regards, Simon
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote:
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: > > > On 2017/6/13 17:11, Marek Vasut wrote: >> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>> Use fixed regulator to control the voltage of vbus and turn off >>> vbus when usb stop. >>> >>> Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com >>> --- >>> >>> Changes in v5: >>> - Propagate return value and print error message when failed >>> >>> Changes in v4: >>> - Splited from patch [Uboot,v3,04/10] >>> - Define set vbus as empty function if the macros aren't set >>> >>> Changes in v3: None >>> Changes in v2: >>> - Use fixed regulator to control vbus instead of gpio >>> >>> drivers/usb/host/xhci-rockchip.c | 55 >>> ++++++++++++++++++++++++++++++---------- >>> 1 file changed, 42 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-rockchip.c >>> b/drivers/usb/host/xhci-rockchip.c >>> index f559830..15df6ef 100644 >>> --- a/drivers/usb/host/xhci-rockchip.c >>> +++ b/drivers/usb/host/xhci-rockchip.c >>> @@ -11,10 +11,10 @@ >>> #include <malloc.h> >>> #include <usb.h> >>> #include <watchdog.h> >>> -#include <asm/gpio.h> >>> #include <linux/errno.h> >>> #include <linux/compat.h> >>> #include <linux/usb/dwc3.h> >>> +#include <power/regulator.h> >>> #include "xhci.h" >>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>> struct rockchip_xhci_platdata { >>> fdt_addr_t hcd_base; >>> fdt_addr_t phy_base; >>> - struct gpio_desc vbus_gpio; >>> + struct udevice *vbus_supply; >>> }; >>> /* >>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>> udevice *dev) >>> */ >>> plat->hcd_base = dev_get_addr(dev); >>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>> - debug("Can't get the XHCI register base address\n"); >>> + error("Can't get the XHCI register base address\n"); >>> return -ENXIO; >>> } >>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>> udevice *dev) >>> } >>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>> - debug("Can't get the usbphy register address\n"); >>> + error("Can't get the usbphy register address\n"); >>> return -ENXIO; >>> } >>> - /* Vbus gpio */ >>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >> should) already anyway. > > Yes, I will remove it in v6. >> >>> + /* Vbus regulator */ >>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>> + &plat->vbus_supply); >> So I was curious, does this expand to empty function or is this not >> defined if CONFIG_DM_REGULATOR is not defined ? > This is not defined if CONFIG_DM_REGULATOR is not defined.
Simon, can you comment on this ?
It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.
, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config.
Meng can you please explain why the #ifdef is needed?
Regards, Simon
On 2017/6/18 13:11, Marek Vasut wrote:
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote:
Hi Marek,
On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote: > On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >> >> On 2017/6/13 17:11, Marek Vasut wrote: >>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>> Use fixed regulator to control the voltage of vbus and turn off >>>> vbus when usb stop. >>>> >>>> Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com >>>> --- >>>> >>>> Changes in v5: >>>> - Propagate return value and print error message when failed >>>> >>>> Changes in v4: >>>> - Splited from patch [Uboot,v3,04/10] >>>> - Define set vbus as empty function if the macros aren't set >>>> >>>> Changes in v3: None >>>> Changes in v2: >>>> - Use fixed regulator to control vbus instead of gpio >>>> >>>> drivers/usb/host/xhci-rockchip.c | 55 >>>> ++++++++++++++++++++++++++++++---------- >>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>> b/drivers/usb/host/xhci-rockchip.c >>>> index f559830..15df6ef 100644 >>>> --- a/drivers/usb/host/xhci-rockchip.c >>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>> @@ -11,10 +11,10 @@ >>>> #include <malloc.h> >>>> #include <usb.h> >>>> #include <watchdog.h> >>>> -#include <asm/gpio.h> >>>> #include <linux/errno.h> >>>> #include <linux/compat.h> >>>> #include <linux/usb/dwc3.h> >>>> +#include <power/regulator.h> >>>> #include "xhci.h" >>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>> struct rockchip_xhci_platdata { >>>> fdt_addr_t hcd_base; >>>> fdt_addr_t phy_base; >>>> - struct gpio_desc vbus_gpio; >>>> + struct udevice *vbus_supply; >>>> }; >>>> /* >>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>> udevice *dev) >>>> */ >>>> plat->hcd_base = dev_get_addr(dev); >>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>> - debug("Can't get the XHCI register base address\n"); >>>> + error("Can't get the XHCI register base address\n"); >>>> return -ENXIO; >>>> } >>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>> udevice *dev) >>>> } >>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>> - debug("Can't get the usbphy register address\n"); >>>> + error("Can't get the usbphy register address\n"); >>>> return -ENXIO; >>>> } >>>> - /* Vbus gpio */ >>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>> should) already anyway. >> Yes, I will remove it in v6. >>>> + /* Vbus regulator */ >>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>> + &plat->vbus_supply); >>> So I was curious, does this expand to empty function or is this not >>> defined if CONFIG_DM_REGULATOR is not defined ? >> This is not defined if CONFIG_DM_REGULATOR is not defined. > Simon, can you comment on this ? It looks OK to me.
Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.
, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config.
Meng can you please explain why the #ifdef is needed?
Because the function "device_get_supply_regulator" is undefined if undefined CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator framework can be optimized and make it compiled successful whether define CONFIG_DM_REGULATOR. So is this #ifdef still needed here?
Regards, Simon
On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote:
On 2017/6/18 13:11, Marek Vasut wrote:
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote:
On 06/17/2017 05:41 AM, Simon Glass wrote: > Hi Marek, > > On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote: >> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>> >>> On 2017/6/13 17:11, Marek Vasut wrote: >>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>> vbus when usb stop. >>>>> >>>>> Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com >>>>> --- >>>>> >>>>> Changes in v5: >>>>> - Propagate return value and print error message when failed >>>>> >>>>> Changes in v4: >>>>> - Splited from patch [Uboot,v3,04/10] >>>>> - Define set vbus as empty function if the macros aren't set >>>>> >>>>> Changes in v3: None >>>>> Changes in v2: >>>>> - Use fixed regulator to control vbus instead of gpio >>>>> >>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>> ++++++++++++++++++++++++++++++---------- >>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>> b/drivers/usb/host/xhci-rockchip.c >>>>> index f559830..15df6ef 100644 >>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>> @@ -11,10 +11,10 @@ >>>>> #include <malloc.h> >>>>> #include <usb.h> >>>>> #include <watchdog.h> >>>>> -#include <asm/gpio.h> >>>>> #include <linux/errno.h> >>>>> #include <linux/compat.h> >>>>> #include <linux/usb/dwc3.h> >>>>> +#include <power/regulator.h> >>>>> #include "xhci.h" >>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>> struct rockchip_xhci_platdata { >>>>> fdt_addr_t hcd_base; >>>>> fdt_addr_t phy_base; >>>>> - struct gpio_desc vbus_gpio; >>>>> + struct udevice *vbus_supply; >>>>> }; >>>>> /* >>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>> udevice *dev) >>>>> */ >>>>> plat->hcd_base = dev_get_addr(dev); >>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>> - debug("Can't get the XHCI register base address\n"); >>>>> + error("Can't get the XHCI register base address\n"); >>>>> return -ENXIO; >>>>> } >>>>> @@ -62,19 +62,39 @@ static int >>>>> xhci_usb_ofdata_to_platdata(struct >>>>> udevice *dev) >>>>> } >>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>> - debug("Can't get the usbphy register address\n"); >>>>> + error("Can't get the usbphy register address\n"); >>>>> return -ENXIO; >>>>> } >>>>> - /* Vbus gpio */ >>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>> on it (or >>>> should) already anyway. >>> Yes, I will remove it in v6. >>>>> + /* Vbus regulator */ >>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>> + &plat->vbus_supply); >>>> So I was curious, does this expand to empty function or is >>>> this not >>>> defined if CONFIG_DM_REGULATOR is not defined ? >>> This is not defined if CONFIG_DM_REGULATOR is not defined. >> Simon, can you comment on this ? > It looks OK to me. Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.
, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config.
Meng can you please explain why the #ifdef is needed?
Because the function "device_get_supply_regulator" is undefined if undefined CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator framework can be optimized and make it compiled successful whether define CONFIG_DM_REGULATOR. So is this #ifdef still needed here?
Thus my discussion with Simon. Linux does keep the ifdefs in framework header files , so I think we should do the same.
On 19 June 2017 at 04:15, Marek Vasut marex@denx.de wrote:
On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote:
On 2017/6/18 13:11, Marek Vasut wrote:
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut marex@denx.de wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 00:22, Marek Vasut marex@denx.de wrote: > On 06/17/2017 05:41 AM, Simon Glass wrote: >> Hi Marek, >> >> On 15 June 2017 at 10:53, Marek Vasut marex@denx.de wrote: >>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>> >>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>> vbus when usb stop. >>>>>> >>>>>> Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com >>>>>> --- >>>>>> >>>>>> Changes in v5: >>>>>> - Propagate return value and print error message when failed >>>>>> >>>>>> Changes in v4: >>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>> - Define set vbus as empty function if the macros aren't set >>>>>> >>>>>> Changes in v3: None >>>>>> Changes in v2: >>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>> >>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>> ++++++++++++++++++++++++++++++---------- >>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>> index f559830..15df6ef 100644 >>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>> @@ -11,10 +11,10 @@ >>>>>> #include <malloc.h> >>>>>> #include <usb.h> >>>>>> #include <watchdog.h> >>>>>> -#include <asm/gpio.h> >>>>>> #include <linux/errno.h> >>>>>> #include <linux/compat.h> >>>>>> #include <linux/usb/dwc3.h> >>>>>> +#include <power/regulator.h> >>>>>> #include "xhci.h" >>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>> struct rockchip_xhci_platdata { >>>>>> fdt_addr_t hcd_base; >>>>>> fdt_addr_t phy_base; >>>>>> - struct gpio_desc vbus_gpio; >>>>>> + struct udevice *vbus_supply; >>>>>> }; >>>>>> /* >>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>> udevice *dev) >>>>>> */ >>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>> + error("Can't get the XHCI register base address\n"); >>>>>> return -ENXIO; >>>>>> } >>>>>> @@ -62,19 +62,39 @@ static int >>>>>> xhci_usb_ofdata_to_platdata(struct >>>>>> udevice *dev) >>>>>> } >>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>> - debug("Can't get the usbphy register address\n"); >>>>>> + error("Can't get the usbphy register address\n"); >>>>>> return -ENXIO; >>>>>> } >>>>>> - /* Vbus gpio */ >>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>>> on it (or >>>>> should) already anyway. >>>> Yes, I will remove it in v6. >>>>>> + /* Vbus regulator */ >>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>> + &plat->vbus_supply); >>>>> So I was curious, does this expand to empty function or is >>>>> this not >>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>> Simon, can you comment on this ? >> It looks OK to me. > Shouldn't you have empty stub functions instead to avoid > proliferation > of adhoc #ifdef CONFIG_FOO throughout the codebase ? You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually.
Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the
drivers
with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.
, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config.
Meng can you please explain why the #ifdef is needed?
Because the function "device_get_supply_regulator" is undefined if undefined CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator framework can be optimized and make it compiled successful whether define CONFIG_DM_REGULATOR. So is this #ifdef still needed here?
Thus my discussion with Simon. Linux does keep the ifdefs in framework header files , so I think we should do the same.
OK, but arguably that is a separate issue from this patch.
Also I hope we can always enable DM_REGULATOR for rockchip and drop the #ifdefs.
Regards, Simon
On 06/20/2017 05:22 AM, Simon Glass wrote:
On 19 June 2017 at 04:15, Marek Vasut <marex@denx.de mailto:marex@denx.de> wrote:
On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote:
On 2017/6/18 13:11, Marek Vasut wrote:
On 06/18/2017 12:10 AM, Simon Glass wrote:
Hi Marek,
On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de
mailto:marex@denx.de> wrote:
On 06/17/2017 07:28 PM, Simon Glass wrote: > Hi Marek, > > On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de
mailto:marex@denx.de> wrote:
>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de
mailto:marex@denx.de> wrote:
>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>> >>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>> vbus when usb stop. >>>>>>> >>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com
mailto:daniel.meng@rock-chips.com>
>>>>>>> --- >>>>>>> >>>>>>> Changes in v5: >>>>>>> - Propagate return value and print error message when failed >>>>>>> >>>>>>> Changes in v4: >>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>> >>>>>>> Changes in v3: None >>>>>>> Changes in v2: >>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>> >>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>> index f559830..15df6ef 100644 >>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>> @@ -11,10 +11,10 @@ >>>>>>> #include <malloc.h> >>>>>>> #include <usb.h> >>>>>>> #include <watchdog.h> >>>>>>> -#include <asm/gpio.h> >>>>>>> #include <linux/errno.h> >>>>>>> #include <linux/compat.h> >>>>>>> #include <linux/usb/dwc3.h> >>>>>>> +#include <power/regulator.h> >>>>>>> #include "xhci.h" >>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>> struct rockchip_xhci_platdata { >>>>>>> fdt_addr_t hcd_base; >>>>>>> fdt_addr_t phy_base; >>>>>>> - struct gpio_desc vbus_gpio; >>>>>>> + struct udevice *vbus_supply; >>>>>>> }; >>>>>>> /* >>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>> udevice *dev) >>>>>>> */ >>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>> return -ENXIO; >>>>>>> } >>>>>>> @@ -62,19 +62,39 @@ static int >>>>>>> xhci_usb_ofdata_to_platdata(struct >>>>>>> udevice *dev) >>>>>>> } >>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>> return -ENXIO; >>>>>>> } >>>>>>> - /* Vbus gpio */ >>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>>>> on it (or >>>>>> should) already anyway. >>>>> Yes, I will remove it in v6. >>>>>>> + /* Vbus regulator */ >>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>> + &plat->vbus_supply); >>>>>> So I was curious, does this expand to empty function or is >>>>>> this not >>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>> Simon, can you comment on this ? >>> It looks OK to me. >> Shouldn't you have empty stub functions instead to avoid >> proliferation >> of adhoc #ifdef CONFIG_FOO throughout the codebase ? > You could, but this is just a temporary state while apparently some > rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what > those bad boards are, actually. Temporary state ? What's the final state then ?
Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not.
This is IMO unrelated to rockchip.
Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the
drivers
with ifdefs ?
USB would not work without VBUS, which is handled with regulators
The VBUS can be directly tied to 5V rail without power switching.
, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here.
Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config.
Meng can you please explain why the #ifdef is needed?
Because the function "device_get_supply_regulator" is undefined if undefined CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator framework can be optimized and make it compiled successful whether define CONFIG_DM_REGULATOR. So is this #ifdef still needed here?
Thus my discussion with Simon. Linux does keep the ifdefs in framework header files , so I think we should do the same.
OK, but arguably that is a separate issue from this patch.
It is, and I believe it should be discussed and possibly fixed.
Also I hope we can always enable DM_REGULATOR for rockchip and drop the #ifdefs.
If the driver explicitly depends on DM_REGULATOR in Kconfig , then yes.
Add the compatible "rockchip,rk3328-xhci" in match table for rk3328 to probe xhci controller.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com ---
Chagnes in v5: None Changes in v4: - New patch, splited from patch [Uboot,v3,04/10]
drivers/usb/host/xhci-rockchip.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index 15df6ef..6004bcb 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -212,6 +212,7 @@ static int xhci_usb_remove(struct udevice *dev)
static const struct udevice_id xhci_usb_ids[] = { { .compatible = "rockchip,rk3399-xhci" }, + { .compatible = "rockchip,rk3328-xhci" }, { } };
@@ -231,6 +232,7 @@ U_BOOT_DRIVER(usb_xhci) = {
static const struct udevice_id usb_phy_ids[] = { { .compatible = "rockchip,rk3399-usb3-phy" }, + { .compatible = "rockchip,rk3328-usb3-phy" }, { } };
On 12 June 2017 at 03:19, Meng Dongyang daniel.meng@rock-chips.com wrote:
Add the compatible "rockchip,rk3328-xhci" in match table for rk3328 to probe xhci controller.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Chagnes in v5: None Changes in v4:
- New patch, splited from patch [Uboot,v3,04/10]
drivers/usb/host/xhci-rockchip.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Add dts node of xhci and enable the controller.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
changes in v5: None Changes in v4: None Changes in v3: None
arch/arm/dts/rk3328-evb.dts | 14 ++++++++++++++ arch/arm/dts/rk3328.dtsi | 11 +++++++++++ 2 files changed, 25 insertions(+)
diff --git a/arch/arm/dts/rk3328-evb.dts b/arch/arm/dts/rk3328-evb.dts index 9920935..4cf6d2e 100644 --- a/arch/arm/dts/rk3328-evb.dts +++ b/arch/arm/dts/rk3328-evb.dts @@ -14,6 +14,15 @@ chosen { stdout-path = &uart2; }; + + vcc5v0_host_xhci: vcc5v0-host-xhci-drv { + compatible = "regulator-fixed"; + enable-active-high; + regulator-name = "vcc5v0_host_xhci"; + gpio = <&gpio0 0 GPIO_ACTIVE_HIGH>; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + }; };
&uart2 { @@ -51,3 +60,8 @@ &usb_host0_ohci { status = "okay"; }; + +&usb_host0_xhci { + vbus-supply = <&vcc5v0_host_xhci>; + status = "okay"; +}; diff --git a/arch/arm/dts/rk3328.dtsi b/arch/arm/dts/rk3328.dtsi index e1219c3..f18cfc2 100644 --- a/arch/arm/dts/rk3328.dtsi +++ b/arch/arm/dts/rk3328.dtsi @@ -471,6 +471,17 @@ status = "disabled"; };
+ usb_host0_xhci: usb@ff600000 { + compatible = "rockchip,rk3328-xhci"; + reg = <0x0 0xff600000 0x0 0x100000>; + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; + snps,dis-enblslpm-quirk; + snps,phyif-utmi-bits = <16>; + snps,dis-u2-freeclk-exists-quirk; + snps,dis-u2-susphy-quirk; + status = "disabled"; + }; + gic: interrupt-controller@ffb70000 { compatible = "arm,gic-400"; #interrupt-cells = <3>;
Config dwc2 driver support host and gadget function. Add support of fastboot function.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None
configs/evb-rk3328_defconfig | 14 ++++++++++++++ include/configs/rk3328_common.h | 3 +++ 2 files changed, 17 insertions(+)
diff --git a/configs/evb-rk3328_defconfig b/configs/evb-rk3328_defconfig index 57aee02..833bffc 100644 --- a/configs/evb-rk3328_defconfig +++ b/configs/evb-rk3328_defconfig @@ -42,3 +42,17 @@ CONFIG_USB_OHCI_GENERIC=y CONFIG_USB_XHCI_DWC3=y CONFIG_USB_XHCI_HCD=y CONFIG_USB_XHCI_RK=y +CONFIG_USB_GADGET=y +CONFIG_USB_GADGET_DOWNLOAD=y +CONFIG_USB_GADGET_DUALSPEED=y +CONFIG_USB_GADGET_DWC2_OTG=y +CONFIG_USB_FUNCTION_FASTBOOT=y +CONFIG_G_DNL_MANUFACTURER="Rockchip" +CONFIG_G_DNL_VENDOR_NUM=0x2207 +CONFIG_G_DNL_PRODUCT_NUM=0x330a +CONFIG_FASTBOOT=y +CONFIG_CMD_FASTBOOT=y +CONFIG_FASTBOOT_FLASH=y +CONFIG_FASTBOOT_FLASH_MMC_DEV=1 +CONFIG_FASTBOOT_BUF_ADDR=0x00800800 +CONFIG_FASTBOOT_BUF_SIZE=0x08000000 diff --git a/include/configs/rk3328_common.h b/include/configs/rk3328_common.h index cc6c890..9121db1 100644 --- a/include/configs/rk3328_common.h +++ b/include/configs/rk3328_common.h @@ -61,6 +61,9 @@
#endif
+/* dwc2 otg */ +#define CONFIG_USB_DWC2 + /* rockchip ohci host driver */ #define CONFIG_USB_OHCI_NEW #define CONFIG_SYS_USB_OHCI_MAX_ROOT_PORTS 1
In current code, after running the command of "usb start", the controller will keep in otg mode and can't switch to host mode if not support SNP/SRP capability. So add the property of "hnp-srp-disable" in the DTS to config the contrller work in force mode of host.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com ---
Changes in v5: None Changes in v4: - Prepare a mask and set capabilities in GUSBCFG register once
Changes in v3: - revert change of macro definition in dwc2 driver - support host mode without HNP/SRP capability through DTS
Changes in v2: - Splited from patch [07/08] of v1
drivers/usb/host/dwc2.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 0e5df15..e25f885 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -43,6 +43,7 @@ struct dwc2_priv { struct dwc2_core_regs *regs; int root_hub_devnum; bool ext_vbus; + bool hnp_srp_disable; bool oc_disable; };
@@ -394,6 +395,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; } #endif + if (priv->hnp_srp_disable) + usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE; + writel(usbcfg, ®s->gusbcfg);
/* Program the GAHBCFG Register. */ @@ -422,12 +426,16 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
writel(ahbcfg, ®s->gahbcfg);
- /* Program the GUSBCFG register for HNP/SRP. */ - setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP); + /* Program the capabilities in GUSBCFG Register */ + usbcfg = 0;
+ if (!priv->hnp_srp_disable) + usbcfg |= DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP; #ifdef CONFIG_DWC2_IC_USB_CAP - setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP); + usbcfg |= DWC2_GUSBCFG_IC_USB_CAP; #endif + + setbits_le32(®s->gusbcfg, usbcfg); }
/* @@ -1244,6 +1252,11 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev) if (prop) priv->oc_disable = true;
+ prop = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), + "hnp-srp-disable", NULL); + if (prop) + priv->hnp_srp_disable = true; + return 0; }
Hi Meng,
On 12 June 2017 at 03:19, Meng Dongyang daniel.meng@rock-chips.com wrote:
In current code, after running the command of "usb start", the controller will keep in otg mode and can't switch to host mode if not support SNP/SRP capability. So add the property of "hnp-srp-disable" in the DTS to config the contrller work in force mode of host.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5: None Changes in v4:
- Prepare a mask and set capabilities in GUSBCFG register once
Changes in v3:
- revert change of macro definition in dwc2 driver
- support host mode without HNP/SRP capability through DTS
Changes in v2:
- Splited from patch [07/08] of v1
drivers/usb/host/dwc2.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Please see below.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 0e5df15..e25f885 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -43,6 +43,7 @@ struct dwc2_priv { struct dwc2_core_regs *regs; int root_hub_devnum; bool ext_vbus;
bool hnp_srp_disable;
Please add a comment for this that fully describes its effect.
bool oc_disable;};
@@ -394,6 +395,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; } #endif
if (priv->hnp_srp_disable)usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE;writel(usbcfg, ®s->gusbcfg); /* Program the GAHBCFG Register. */@@ -422,12 +426,16 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
writel(ahbcfg, ®s->gahbcfg);
/* Program the GUSBCFG register for HNP/SRP. */setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP);
/* Program the capabilities in GUSBCFG Register */usbcfg = 0;if (!priv->hnp_srp_disable)usbcfg |= DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP;#ifdef CONFIG_DWC2_IC_USB_CAP
setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP);
usbcfg |= DWC2_GUSBCFG_IC_USB_CAP;#endif
setbits_le32(®s->gusbcfg, usbcfg);}
/* @@ -1244,6 +1252,11 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev) if (prop) priv->oc_disable = true;
prop = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),"hnp-srp-disable", NULL);
Can you use dev_read_boot() (preferred) or fdtdec_get_bool()?
if (prop)priv->hnp_srp_disable = true;return 0;}
-- 1.9.1
Regards, Simon
Sorry I just see this email.
On 2017/6/17 11:40, Simon Glass wrote:
Hi Meng,
On 12 June 2017 at 03:19, Meng Dongyang daniel.meng@rock-chips.com wrote:
In current code, after running the command of "usb start", the controller will keep in otg mode and can't switch to host mode if not support SNP/SRP capability. So add the property of "hnp-srp-disable" in the DTS to config the contrller work in force mode of host.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com
Changes in v5: None Changes in v4:
- Prepare a mask and set capabilities in GUSBCFG register once
Changes in v3:
- revert change of macro definition in dwc2 driver
- support host mode without HNP/SRP capability through DTS
Changes in v2:
Splited from patch [07/08] of v1
drivers/usb/host/dwc2.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Please see below.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 0e5df15..e25f885 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -43,6 +43,7 @@ struct dwc2_priv { struct dwc2_core_regs *regs; int root_hub_devnum; bool ext_vbus;
bool hnp_srp_disable;Please add a comment for this that fully describes its effect.
OK.
bool oc_disable;};
@@ -394,6 +395,9 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; } #endif
if (priv->hnp_srp_disable)usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE;writel(usbcfg, ®s->gusbcfg); /* Program the GAHBCFG Register. */@@ -422,12 +426,16 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
writel(ahbcfg, ®s->gahbcfg);
/* Program the GUSBCFG register for HNP/SRP. */setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP);
/* Program the capabilities in GUSBCFG Register */usbcfg = 0;if (!priv->hnp_srp_disable)usbcfg |= DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP;#ifdef CONFIG_DWC2_IC_USB_CAP
setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP);
usbcfg |= DWC2_GUSBCFG_IC_USB_CAP;#endif
setbits_le32(®s->gusbcfg, usbcfg);}
/*
@@ -1244,6 +1252,11 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev) if (prop) priv->oc_disable = true;
prop = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),"hnp-srp-disable", NULL);Can you use dev_read_boot() (preferred) or fdtdec_get_bool()?
OK.
if (prop)priv->hnp_srp_disable = true;return 0;-- 1.9.1
Regards, Simon
Probe dwc2 udc in the function of board_usb_start to enable usb gadget function.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Chagnes in v5: None Changes in v4: None Changes in v3: None
board/rockchip/evb_rk3328/evb-rk3328.c | 43 +++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/board/rockchip/evb_rk3328/evb-rk3328.c b/board/rockchip/evb_rk3328/evb-rk3328.c index a7895cb..7fc25bb 100644 --- a/board/rockchip/evb_rk3328/evb-rk3328.c +++ b/board/rockchip/evb_rk3328/evb-rk3328.c @@ -31,12 +31,49 @@ int dram_init_banksize(void) return 0; }
-int usb_gadget_handle_interrupts(void) +#if defined(CONFIG_USB_GADGET) && defined(CONFIG_USB_GADGET_DWC2_OTG) +#include <usb.h> +#include <usb/dwc2_udc.h> + +static struct dwc2_plat_otg_data rk3328_otg_data = { + .rx_fifo_sz = 512, + .np_tx_fifo_sz = 16, + .tx_fifo_sz = 128, +}; + +int board_usb_init(int index, enum usb_init_type init) { - return 0; + int node; + const char *mode; + bool matched = false; + const void *blob = gd->fdt_blob; + + /* find the usb_otg node */ + node = fdt_node_offset_by_compatible(blob, -1, + "rockchip,rk3328-usb"); + + while (node > 0) { + mode = fdt_getprop(blob, node, "dr_mode", NULL); + if (mode && strcmp(mode, "otg") == 0) { + matched = true; + break; + } + + node = fdt_node_offset_by_compatible(blob, node, + "rockchip,rk3328-usb"); + } + if (!matched) { + debug("Not found usb_otg device\n"); + return -ENODEV; + } + + rk3328_otg_data.regs_otg = fdtdec_get_addr(blob, node, "reg"); + + return dwc2_udc_probe(&rk3328_otg_data); }
-int board_usb_init(int index, enum usb_init_type init) +int board_usb_cleanup(int index, enum usb_init_type init) { return 0; } +#endif
Enable dwc2 controller and add fixed regulator for dwc2 controller to control vbus.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v5: None Changes in v4: None Changes in v3: - add hnp-srp-disable property
arch/arm/dts/rk3328-evb.dts | 14 ++++++++++++++ arch/arm/dts/rk3328.dtsi | 10 ++++++++++ 2 files changed, 24 insertions(+)
diff --git a/arch/arm/dts/rk3328-evb.dts b/arch/arm/dts/rk3328-evb.dts index 4cf6d2e..220d0ab 100644 --- a/arch/arm/dts/rk3328-evb.dts +++ b/arch/arm/dts/rk3328-evb.dts @@ -15,6 +15,15 @@ stdout-path = &uart2; };
+ vcc5v0_otg: vcc5v0-otg-drv { + compatible = "regulator-fixed"; + enable-active-high; + regulator-name = "vcc5v0_otg"; + gpio = <&gpio0 27 GPIO_ACTIVE_HIGH>; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + }; + vcc5v0_host_xhci: vcc5v0-host-xhci-drv { compatible = "regulator-fixed"; enable-active-high; @@ -61,6 +70,11 @@ status = "okay"; };
+&usb20_otg { + vbus-supply = <&vcc5v0_otg>; + status = "okay"; +}; + &usb_host0_xhci { vbus-supply = <&vcc5v0_host_xhci>; status = "okay"; diff --git a/arch/arm/dts/rk3328.dtsi b/arch/arm/dts/rk3328.dtsi index f18cfc2..1290982 100644 --- a/arch/arm/dts/rk3328.dtsi +++ b/arch/arm/dts/rk3328.dtsi @@ -460,6 +460,16 @@ status = "disabled"; };
+ usb20_otg: usb@ff580000 { + compatible = "rockchip,rk3328-usb", "rockchip,rk3066-usb", + "snps,dwc2"; + reg = <0x0 0xff580000 0x0 0x40000>; + interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>; + hnp-srp-disable; + dr_mode = "otg"; + status = "disabled"; + }; + sdmmc_ext: rksdmmc@ff5f0000 { compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc"; reg = <0x0 0xff5f0000 0x0 0x4000>;
Add fixed regulator for the port of typec0 and typec1 to control vbus instead of gpio.
Signed-off-by: Meng Dongyang daniel.meng@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v5: None Changes in v4: None Changes in v3: None
Changes in v2: - New change, add fixed regulator for rk3399
arch/arm/dts/rk3399-evb.dts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/rk3399-evb.dts b/arch/arm/dts/rk3399-evb.dts index f5af75b..bff00c3 100644 --- a/arch/arm/dts/rk3399-evb.dts +++ b/arch/arm/dts/rk3399-evb.dts @@ -60,6 +60,18 @@ gpio = <&gpio4 25 GPIO_ACTIVE_HIGH>; };
+ vcc5v0_typec0: vcc5v0-typec0-en { + compatible = "regulator-fixed"; + regulator-name = "vcc5v0_typec0"; + gpio = <&gpio1 3 GPIO_ACTIVE_HIGH>; + }; + + vcc5v0_typec1: vcc5v0-typec1-en { + compatible = "regulator-fixed"; + regulator-name = "vcc5v0_typec1"; + gpio = <&gpio1 4 GPIO_ACTIVE_HIGH>; + }; + clkin_gmac: external-gmac-clock { compatible = "fixed-clock"; clock-frequency = <125000000>; @@ -163,7 +175,7 @@ };
&dwc3_typec0 { - rockchip,vbus-gpio = <&gpio1 3 GPIO_ACTIVE_HIGH>; + vbus-supply = <&vcc5v0_typec0>; status = "okay"; };
@@ -176,7 +188,7 @@ };
&dwc3_typec1 { - rockchip,vbus-gpio = <&gpio1 4 GPIO_ACTIVE_HIGH>; + vbus-supply = <&vcc5v0_typec1>; status = "okay"; };
participants (4)
-
Marek Vasut -
Meng Dongyang -
rock-chips(daniel.meng)
-
Simon Glass