[U-Boot] [PATCH v2 0/4] Enable the host controller and hub on PopMetal board

[U-Boot] [PATCH v2 0/4] Enable the host controller and hub on PopMetal board

[Kever Yang]

The PopMetal board based on rk3288 SoC have 3 USB 2.0 host ports from
a usb 2.0 hub which connect to the rk3288 usb host1.

This patch set enable those ports by enable the rk3288 usb host controller
driver and usb host function like storage and ether, enable the port
power and de-assert the hub reset signal.

Test with U-disk on Popmetal board.
=> usb start
starting USB...
USB0:   Core Release: 3.10a
scanning bus 0 for devices... ** First descriptor is NOT a primary desc
on 0:1 **
3 USB Device(s) found
       scanning usb for ethernet devices... 0 Ethernet Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |   U-Boot Root Hub
  |
  +-2  Hub (480 Mb/s, 100mA)
    |   USB 2.0 Hub
    |
    +-3  Mass Storage (480 Mb/s, 300mA)
         Kingston DataTraveler G2 001D92AD7612B91113680066


Changes in v2:
- move the vbus power enable into dwc2 driver

Kever Yang (4):
  usb: dwc2: add support for external vbus supply
  board: popmetal: de-assert the host rst pin in board init
  config: popmetal: enable the USB host controller and function
  dts: popmetal: add usb host power supply node

 arch/arm/dts/rk3288-popmetal.dtsi                 | 23 ++++++++++++++++++
 board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17 +++++++++++++
 configs/popmetal-rk3288_defconfig                 |  3 +++
 drivers/usb/host/dwc2.c                           | 29 +++++++++++++++++++++++
 include/configs/rk3288_common.h                   |  7 ++++++
 5 files changed, 79 insertions(+)

-- 
1.9.1

[U-Boot] [PATCH v2 1/4] usb: dwc2: add support for external vbus supply

[Kever Yang]

Some board do not use the dwc2 internal VBUS_DRV signal, but
use a gpio pin to enable the 5.0V VBUS power, add interface to
enable the power in dwc2 driver.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2: None

 drivers/usb/host/dwc2.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index d08879d..8292aa8 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -15,6 +15,7 @@
 #include <usbroothubdes.h>
 #include <wait_bit.h>
 #include <asm/io.h>
+#include <power/regulator.h>
 
 #include "dwc2.h"
 
@@ -55,6 +56,8 @@ DEFINE_ALIGN_BUFFER(uint8_t, status_buffer_addr, DWC2_STATUS_BUF_SIZE,
 static struct dwc2_priv local;
 #endif
 
+static struct udevice *g_dwc2_udev;
+
 /*
  * DWC2 IP interface
  */
@@ -159,6 +162,29 @@ static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
 	mdelay(100);
 }
 
+static int dwc_vbus_supply_init(void)
+{
+#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) && \
+	!defined(CONFIG_SPL_BUILD)
+	struct udevice *vbus_supply;
+	int ret;
+
+	ret = device_get_supply_regulator(g_dwc2_udev, "vbus-supply",
+					  &vbus_supply);
+	if (ret) {
+		debug("%s: No vbus supply\n", g_dwc2_udev->name);
+		return 0;
+	}
+
+	ret = regulator_set_enable(vbus_supply, true);
+	if (ret) {
+		error("Error enabling vbus supply\n");
+		return ret;
+	}
+#endif
+	return 0;
+}
+
 /*
  * This function initializes the DWC_otg controller registers for
  * host mode.
@@ -248,6 +274,8 @@ static void dwc_otg_core_host_init(struct dwc2_core_regs *regs)
 			writel(hprt0, &regs->hprt0);
 		}
 	}
+
+	dwc_vbus_supply_init();
 }
 
 /*
@@ -1194,6 +1222,7 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
 	const void *prop;
 	fdt_addr_t addr;
 
+	g_dwc2_udev = dev;
 	addr = dev_get_addr(dev);
 	if (addr == FDT_ADDR_T_NONE)
 		return -EINVAL;
-- 
1.9.1

[U-Boot] [PATCH v2 2/4] board: popmetal: de-assert the host rst pin in board init

[Kever Yang]

The PopMetal board have a on board FE1.1 usb 2.0 hub which connect to
the usb host port, we need to de-assert its reset pin to enable it.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2:
- move the vbus power enable into dwc2 driver

 board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
index aad74ef..ed82b2b 100644
--- a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
+++ b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <spl.h>
+#include <asm/gpio.h>
 
 void board_boot_order(u32 *spl_boot_list)
 {
@@ -13,3 +14,19 @@ void board_boot_order(u32 *spl_boot_list)
 	spl_boot_list[0] = BOOT_DEVICE_MMC2;
 	spl_boot_list[1] = BOOT_DEVICE_MMC1;
 }
+
+#define GPIO7A3_HUB_RST	227
+
+int rk_board_late_init(void)
+{
+	int ret;
+
+	ret = gpio_request(GPIO7A3_HUB_RST, "hub_rst");
+	if (ret)
+		return ret;
+	ret = gpio_direction_output(GPIO7A3_HUB_RST, 1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
-- 
1.9.1

[U-Boot] [PATCH v2 3/4] config: popmetal: enable the USB host controller and function

[Kever Yang]

RK3288 using the dwc2 USB host controller, enable it and other usb host
funtion like storage and ether.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Acked-by: Simon Glass <sjg@chromium.org>
---

Changes in v2: None

 configs/popmetal-rk3288_defconfig | 3 +++
 include/configs/rk3288_common.h   | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/configs/popmetal-rk3288_defconfig b/configs/popmetal-rk3288_defconfig
index 1e70ae0..a2d1d65 100644
--- a/configs/popmetal-rk3288_defconfig
+++ b/configs/popmetal-rk3288_defconfig
@@ -63,3 +63,6 @@ CONFIG_USE_TINY_PRINTF=y
 CONFIG_CMD_DHRYSTONE=y
 CONFIG_ERRNO_STR=y
 CONFIG_ROCKCHIP_SPL_BACK_TO_BROM=y
+CONFIG_CMD_USB=y
+CONFIG_USB=y
+CONFIG_USB_STORAGE=y
diff --git a/include/configs/rk3288_common.h b/include/configs/rk3288_common.h
index 0868612..8006aac 100644
--- a/include/configs/rk3288_common.h
+++ b/include/configs/rk3288_common.h
@@ -90,6 +90,13 @@
 #define CONFIG_G_DNL_VENDOR_NUM		0x2207
 #define CONFIG_G_DNL_PRODUCT_NUM	0x320a
 
+/* usb host support */
+#ifdef CONFIG_CMD_USB
+#define CONFIG_USB_DWC2
+#define CONFIG_USB_HOST_ETHER
+#define CONFIG_USB_ETHER_SMSC95XX
+#define CONFIG_USB_ETHER_ASIX
+#endif
 #define ENV_MEM_LAYOUT_SETTINGS \
 	"scriptaddr=0x00000000\0" \
 	"pxefile_addr_r=0x00100000\0" \
-- 
1.9.1

[U-Boot] [PATCH v2 4/4] dts: popmetal: add usb host power supply node

[Kever Yang]

The popmetal board using a HOST_VBUS_DRV gpio signal to control the
USB host port 5V power, add a fix regulator and pinctrl for it, and
enable the USB host1 controller with the vbus-supply.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v2: None

 arch/arm/dts/rk3288-popmetal.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/dts/rk3288-popmetal.dtsi b/arch/arm/dts/rk3288-popmetal.dtsi
index f3bd468..e5be4cb 100644
--- a/arch/arm/dts/rk3288-popmetal.dtsi
+++ b/arch/arm/dts/rk3288-popmetal.dtsi
@@ -145,6 +145,18 @@
 		regulator-always-on;
 		vin-supply = <&vcc_io>;
 	};
+
+	vcc5v0_host: usb-host-regulator {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio0 14 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&host_vbus_drv>;
+		regulator-name = "vcc5v0_host";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+	};
 };
 
 &cpu0 {
@@ -471,6 +483,12 @@
 			rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 	};
+
+	usb_host {
+		host_vbus_drv: host-vbus-drv {
+			rockchip,pins = <0 14 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
 };
 
 &tsadc {
@@ -515,6 +533,11 @@
 	status = "okay";
 };
 
+&usb_host1 {
+	vbus-supply = <&vcc5v0_host>;
+	status = "okay";
+};
+
 &usbphy {
 	status = "okay";
 };
-- 
1.9.1

[U-Boot] [PATCH v2 1/4] usb: dwc2: add support for external vbus supply

[Marek Vasut]

On 11/24/2016 08:29 AM, Kever Yang wrote:
> Some board do not use the dwc2 internal VBUS_DRV signal, but
> use a gpio pin to enable the 5.0V VBUS power, add interface to
> enable the power in dwc2 driver.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/usb/host/dwc2.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index d08879d..8292aa8 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -15,6 +15,7 @@
>  #include <usbroothubdes.h>
>  #include <wait_bit.h>
>  #include <asm/io.h>
> +#include <power/regulator.h>
>  
>  #include "dwc2.h"
>  
> @@ -55,6 +56,8 @@ DEFINE_ALIGN_BUFFER(uint8_t, status_buffer_addr, DWC2_STATUS_BUF_SIZE,
>  static struct dwc2_priv local;
>  #endif
>  
> +static struct udevice *g_dwc2_udev;

Can we avoid the static global var ?

> +
>  /*
>   * DWC2 IP interface
>   */
> @@ -159,6 +162,29 @@ static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
>  	mdelay(100);
>  }
>  
> +static int dwc_vbus_supply_init(void)
> +{
> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) && \
> +	!defined(CONFIG_SPL_BUILD)

Why does this not work for SPL ?

btw, I'd rather see this as:

#if FOO
function bar()
{
 ...
}
#else
static inline function bar()
{
 return 0;
}
#endif

> +	struct udevice *vbus_supply;
> +	int ret;
> +
> +	ret = device_get_supply_regulator(g_dwc2_udev, "vbus-supply",
> +					  &vbus_supply);

Is this compatible with the Linux DWC2 DT bindings ?

> +	if (ret) {
> +		debug("%s: No vbus supply\n", g_dwc2_udev->name);
> +		return 0;
> +	}
> +
> +	ret = regulator_set_enable(vbus_supply, true);

Shouldn't the regulator be enabled when we enable VBUS for a port
instead of here ? And where is it disabled ?

> +	if (ret) {
> +		error("Error enabling vbus supply\n");
> +		return ret;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  /*
>   * This function initializes the DWC_otg controller registers for
>   * host mode.
> @@ -248,6 +274,8 @@ static void dwc_otg_core_host_init(struct dwc2_core_regs *regs)
>  			writel(hprt0, &regs->hprt0);
>  		}
>  	}
> +
> +	dwc_vbus_supply_init();
>  }
>  
>  /*
> @@ -1194,6 +1222,7 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
>  	const void *prop;
>  	fdt_addr_t addr;
>  
> +	g_dwc2_udev = dev;
>  	addr = dev_get_addr(dev);
>  	if (addr == FDT_ADDR_T_NONE)
>  		return -EINVAL;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/4] board: popmetal: de-assert the host rst pin in board init

[Simon Glass]

Hi Kever,

On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com> wrote:
> The PopMetal board have a on board FE1.1 usb 2.0 hub which connect to
> the usb host port, we need to de-assert its reset pin to enable it.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
> Changes in v2:
> - move the vbus power enable into dwc2 driver
>
>  board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
> index aad74ef..ed82b2b 100644
> --- a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
> +++ b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
> @@ -6,6 +6,7 @@
>
>  #include <common.h>
>  #include <spl.h>
> +#include <asm/gpio.h>
>
>  void board_boot_order(u32 *spl_boot_list)
>  {
> @@ -13,3 +14,19 @@ void board_boot_order(u32 *spl_boot_list)
>         spl_boot_list[0] = BOOT_DEVICE_MMC2;
>         spl_boot_list[1] = BOOT_DEVICE_MMC1;
>  }
> +
> +#define GPIO7A3_HUB_RST        227
> +
> +int rk_board_late_init(void)
> +{
> +       int ret;
> +
> +       ret = gpio_request(GPIO7A3_HUB_RST, "hub_rst");
> +       if (ret)
> +               return ret;
> +       ret = gpio_direction_output(GPIO7A3_HUB_RST, 1);
> +       if (ret)

Can we get this from the device tree instead of hard-coding it? Then
it can go in generic code.

> +               return ret;
> +
> +       return 0;
> +}
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH v2 4/4] dts: popmetal: add usb host power supply node

[Simon Glass]

On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com> wrote:
> The popmetal board using a HOST_VBUS_DRV gpio signal to control the
> USB host port 5V power, add a fix regulator and pinctrl for it, and
> enable the USB host1 controller with the vbus-supply.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
> Changes in v2: None
>
>  arch/arm/dts/rk3288-popmetal.dtsi | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 1/4] usb: dwc2: add support for external vbus supply

[Kever Yang]

Hi Marek,

On 11/26/2016 12:46 AM, Marek Vasut wrote:
> On 11/24/2016 08:29 AM, Kever Yang wrote:
>> Some board do not use the dwc2 internal VBUS_DRV signal, but
>> use a gpio pin to enable the 5.0V VBUS power, add interface to
>> enable the power in dwc2 driver.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>>   drivers/usb/host/dwc2.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
>> index d08879d..8292aa8 100644
>> --- a/drivers/usb/host/dwc2.c
>> +++ b/drivers/usb/host/dwc2.c
>> @@ -15,6 +15,7 @@
>>   #include <usbroothubdes.h>
>>   #include <wait_bit.h>
>>   #include <asm/io.h>
>> +#include <power/regulator.h>
>>   
>>   #include "dwc2.h"
>>   
>> @@ -55,6 +56,8 @@ DEFINE_ALIGN_BUFFER(uint8_t, status_buffer_addr, DWC2_STATUS_BUF_SIZE,
>>   static struct dwc2_priv local;
>>   #endif
>>   
>> +static struct udevice *g_dwc2_udev;
> Can we avoid the static global var ?

I don't want to use this global var either, but I don't know how to get 
this udev structure
without this var, do you have any good suggestion?
BTW, of_container() is not available to find the udevice structure with 
dwc2_priv pointer.

>
>> +
>>   /*
>>    * DWC2 IP interface
>>    */
>> @@ -159,6 +162,29 @@ static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
>>   	mdelay(100);
>>   }
>>   
>> +static int dwc_vbus_supply_init(void)
>> +{
>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) && \
>> +	!defined(CONFIG_SPL_BUILD)
> Why does this not work for SPL ?

We do not enable the USB driver in SPL, in this case I can just drop the 
CONFIG_SPL_BUILD
because the driver in not compiled in SPL.

>
> btw, I'd rather see this as:
>
> #if FOO
> function bar()
> {
>   ...
> }
> #else
> static inline function bar()
> {
>   return 0;
> }
> #endif
>
>> +	struct udevice *vbus_supply;
>> +	int ret;
>> +
>> +	ret = device_get_supply_regulator(g_dwc2_udev, "vbus-supply",
>> +					  &vbus_supply);
> Is this compatible with the Linux DWC2 DT bindings ?

This compatible in kernel is in in phy dts node, the dwc2 driver and phy 
driver
in U-Boot are both very different from kernel now, so I chose a place 
which is
reasonable to enable the vbus.

>
>> +	if (ret) {
>> +		debug("%s: No vbus supply\n", g_dwc2_udev->name);
>> +		return 0;
>> +	}
>> +
>> +	ret = regulator_set_enable(vbus_supply, true);
> Shouldn't the regulator be enabled when we enable VBUS for a port
> instead of here ? And where is it disabled ?

I add this function just after the controller enable the port power bit, 
isn't it?
I didn't found anywhere the driver disable the vbus power, we do not need it
in U-Boot?

Thanks,
- Kever
>
>> +	if (ret) {
>> +		error("Error enabling vbus supply\n");
>> +		return ret;
>> +	}
>> +#endif
>> +	return 0;
>> +}
>> +
>>   /*
>>    * This function initializes the DWC_otg controller registers for
>>    * host mode.
>> @@ -248,6 +274,8 @@ static void dwc_otg_core_host_init(struct dwc2_core_regs *regs)
>>   			writel(hprt0, &regs->hprt0);
>>   		}
>>   	}
>> +
>> +	dwc_vbus_supply_init();
>>   }
>>   
>>   /*
>> @@ -1194,6 +1222,7 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
>>   	const void *prop;
>>   	fdt_addr_t addr;
>>   
>> +	g_dwc2_udev = dev;
>>   	addr = dev_get_addr(dev);
>>   	if (addr == FDT_ADDR_T_NONE)
>>   		return -EINVAL;
>>
>

[U-Boot] [PATCH v2 2/4] board: popmetal: de-assert the host rst pin in board init

[Kever Yang]

Hi Simon,

On 11/26/2016 03:39 AM, Simon Glass wrote:
> Hi Kever,
>
> On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com> wrote:
>> The PopMetal board have a on board FE1.1 usb 2.0 hub which connect to
>> the usb host port, we need to de-assert its reset pin to enable it.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - move the vbus power enable into dwc2 driver
>>
>>   board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>> index aad74ef..ed82b2b 100644
>> --- a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>> +++ b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>> @@ -6,6 +6,7 @@
>>
>>   #include <common.h>
>>   #include <spl.h>
>> +#include <asm/gpio.h>
>>
>>   void board_boot_order(u32 *spl_boot_list)
>>   {
>> @@ -13,3 +14,19 @@ void board_boot_order(u32 *spl_boot_list)
>>          spl_boot_list[0] = BOOT_DEVICE_MMC2;
>>          spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>   }
>> +
>> +#define GPIO7A3_HUB_RST        227
>> +
>> +int rk_board_late_init(void)
>> +{
>> +       int ret;
>> +
>> +       ret = gpio_request(GPIO7A3_HUB_RST, "hub_rst");
>> +       if (ret)
>> +               return ret;
>> +       ret = gpio_direction_output(GPIO7A3_HUB_RST, 1);
>> +       if (ret)
> Can we get this from the device tree instead of hard-coding it? Then
> it can go in generic code.

I don't understand how to get this from device tree, this board is the
only one based on rk3288 with a USB hub on board and need the de-assert
its reset pin. I think it is reasonable to hard coding it in its board file.

Thanks,
- Kever
>
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
>
>

[U-Boot] [PATCH v2 2/4] board: popmetal: de-assert the host rst pin in board init

[Simon Glass]

On 29 November 2016 at 01:49, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
>
> On 11/26/2016 03:39 AM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com>
>> wrote:
>>>
>>> The PopMetal board have a on board FE1.1 usb 2.0 hub which connect to
>>> the usb host port, we need to de-assert its reset pin to enable it.
>>>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>>
>>> Changes in v2:
>>> - move the vbus power enable into dwc2 driver
>>>
>>>   board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17
>>> +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>> b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>> index aad74ef..ed82b2b 100644
>>> --- a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>> +++ b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>> @@ -6,6 +6,7 @@
>>>
>>>   #include <common.h>
>>>   #include <spl.h>
>>> +#include <asm/gpio.h>
>>>
>>>   void board_boot_order(u32 *spl_boot_list)
>>>   {
>>> @@ -13,3 +14,19 @@ void board_boot_order(u32 *spl_boot_list)
>>>          spl_boot_list[0] = BOOT_DEVICE_MMC2;
>>>          spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>   }
>>> +
>>> +#define GPIO7A3_HUB_RST        227
>>> +
>>> +int rk_board_late_init(void)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = gpio_request(GPIO7A3_HUB_RST, "hub_rst");
>>> +       if (ret)
>>> +               return ret;
>>> +       ret = gpio_direction_output(GPIO7A3_HUB_RST, 1);
>>> +       if (ret)
>>
>> Can we get this from the device tree instead of hard-coding it? Then
>> it can go in generic code.
>
>
> I don't understand how to get this from device tree, this board is the
> only one based on rk3288 with a USB hub on board and need the de-assert
> its reset pin. I think it is reasonable to hard coding it in its board file.

OK, since it's just one board.

Acked-by: Simon Glass <sjg@chromium.org>

In general we can support this sort of thing by adding a driver for
UCLASS_USB_HUB, but I suspect it would need refactoring of
common/usb.c.

Regards,
Simon

[U-Boot] [PATCH v2 1/4] usb: dwc2: add support for external vbus supply

[Marek Vasut]

On 11/29/2016 09:46 AM, Kever Yang wrote:
> Hi Marek,
> 
> On 11/26/2016 12:46 AM, Marek Vasut wrote:
>> On 11/24/2016 08:29 AM, Kever Yang wrote:
>>> Some board do not use the dwc2 internal VBUS_DRV signal, but
>>> use a gpio pin to enable the 5.0V VBUS power, add interface to
>>> enable the power in dwc2 driver.
>>>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>>
>>> Changes in v2: None
>>>
>>>   drivers/usb/host/dwc2.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
>>> index d08879d..8292aa8 100644
>>> --- a/drivers/usb/host/dwc2.c
>>> +++ b/drivers/usb/host/dwc2.c
>>> @@ -15,6 +15,7 @@
>>>   #include <usbroothubdes.h>
>>>   #include <wait_bit.h>
>>>   #include <asm/io.h>
>>> +#include <power/regulator.h>
>>>     #include "dwc2.h"
>>>   @@ -55,6 +56,8 @@ DEFINE_ALIGN_BUFFER(uint8_t, status_buffer_addr,
>>> DWC2_STATUS_BUF_SIZE,
>>>   static struct dwc2_priv local;
>>>   #endif
>>>   +static struct udevice *g_dwc2_udev;
>> Can we avoid the static global var ?
> 
> I don't want to use this global var either, but I don't know how to get
> this udev structure
> without this var, do you have any good suggestion?
> BTW, of_container() is not available to find the udevice structure with
> dwc2_priv pointer.

But you can ie. store the udevice in priv ? Simon , any hints ?

>>
>>> +
>>>   /*
>>>    * DWC2 IP interface
>>>    */
>>> @@ -159,6 +162,29 @@ static void dwc_otg_core_reset(struct
>>> dwc2_core_regs *regs)
>>>       mdelay(100);
>>>   }
>>>   +static int dwc_vbus_supply_init(void)
>>> +{
>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) && \
>>> +    !defined(CONFIG_SPL_BUILD)
>> Why does this not work for SPL ?
> 
> We do not enable the USB driver in SPL, in this case I can just drop the
> CONFIG_SPL_BUILD
> because the driver in not compiled in SPL.

Correct

>> btw, I'd rather see this as:
>>
>> #if FOO
>> function bar()
>> {
>>   ...
>> }
>> #else
>> static inline function bar()
>> {
>>   return 0;
>> }
>> #endif
>>
>>> +    struct udevice *vbus_supply;
>>> +    int ret;
>>> +
>>> +    ret = device_get_supply_regulator(g_dwc2_udev, "vbus-supply",
>>> +                      &vbus_supply);
>> Is this compatible with the Linux DWC2 DT bindings ?
> 
> This compatible in kernel is in in phy dts node, the dwc2 driver and phy
> driver
> in U-Boot are both very different from kernel now, so I chose a place
> which is
> reasonable to enable the vbus.

DT is driver and OS agnostic, so if there is already agreed-upon binding
for specifying external VBUS to DWC2, we should use it.
Why would that be a problem ?

>>> +    if (ret) {
>>> +        debug("%s: No vbus supply\n", g_dwc2_udev->name);
>>> +        return 0;
>>> +    }
>>> +
>>> +    ret = regulator_set_enable(vbus_supply, true);
>> Shouldn't the regulator be enabled when we enable VBUS for a port
>> instead of here ? And where is it disabled ?
> 
> I add this function just after the controller enable the port power bit,
> isn't it?
> I didn't found anywhere the driver disable the vbus power, we do not
> need it
> in U-Boot?

So we leave it enabled and pass the hardware to kernel in some weird
state ? That's a bit odd, isn't it. dwc2_lowlevel_stop() might be a
good place to disable the regulator ...

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/4] usb: dwc2: add support for external vbus supply

[Simon Glass]

Hi Marek,

On 29 November 2016 at 19:00, Marek Vasut <marex@denx.de> wrote:
> On 11/29/2016 09:46 AM, Kever Yang wrote:
>> Hi Marek,
>>
>> On 11/26/2016 12:46 AM, Marek Vasut wrote:
>>> On 11/24/2016 08:29 AM, Kever Yang wrote:
>>>> Some board do not use the dwc2 internal VBUS_DRV signal, but
>>>> use a gpio pin to enable the 5.0V VBUS power, add interface to
>>>> enable the power in dwc2 driver.
>>>>
>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>>   drivers/usb/host/dwc2.c | 29 +++++++++++++++++++++++++++++
>>>>   1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
>>>> index d08879d..8292aa8 100644
>>>> --- a/drivers/usb/host/dwc2.c
>>>> +++ b/drivers/usb/host/dwc2.c
>>>> @@ -15,6 +15,7 @@
>>>>   #include <usbroothubdes.h>
>>>>   #include <wait_bit.h>
>>>>   #include <asm/io.h>
>>>> +#include <power/regulator.h>
>>>>     #include "dwc2.h"
>>>>   @@ -55,6 +56,8 @@ DEFINE_ALIGN_BUFFER(uint8_t, status_buffer_addr,
>>>> DWC2_STATUS_BUF_SIZE,
>>>>   static struct dwc2_priv local;
>>>>   #endif
>>>>   +static struct udevice *g_dwc2_udev;
>>> Can we avoid the static global var ?
>>
>> I don't want to use this global var either, but I don't know how to get
>> this udev structure
>> without this var, do you have any good suggestion?
>> BTW, of_container() is not available to find the udevice structure with
>> dwc2_priv pointer.
>
> But you can ie. store the udevice in priv ? Simon , any hints ?
>

I think it can just be a parameter. I sent a v3 patch to show how to do it.

>>>
>>>> +
>>>>   /*
>>>>    * DWC2 IP interface
>>>>    */
>>>> @@ -159,6 +162,29 @@ static void dwc_otg_core_reset(struct
>>>> dwc2_core_regs *regs)
>>>>       mdelay(100);
>>>>   }
>>>>   +static int dwc_vbus_supply_init(void)
>>>> +{
>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) && \
>>>> +    !defined(CONFIG_SPL_BUILD)
>>> Why does this not work for SPL ?
>>
>> We do not enable the USB driver in SPL, in this case I can just drop the
>> CONFIG_SPL_BUILD
>> because the driver in not compiled in SPL.
>
> Correct
>
>>> btw, I'd rather see this as:
>>>
>>> #if FOO
>>> function bar()
>>> {
>>>   ...
>>> }
>>> #else
>>> static inline function bar()
>>> {
>>>   return 0;
>>> }
>>> #endif
>>>
>>>> +    struct udevice *vbus_supply;
>>>> +    int ret;
>>>> +
>>>> +    ret = device_get_supply_regulator(g_dwc2_udev, "vbus-supply",
>>>> +                      &vbus_supply);
>>> Is this compatible with the Linux DWC2 DT bindings ?
>>
>> This compatible in kernel is in in phy dts node, the dwc2 driver and phy
>> driver
>> in U-Boot are both very different from kernel now, so I chose a place
>> which is
>> reasonable to enable the vbus.
>
> DT is driver and OS agnostic, so if there is already agreed-upon binding
> for specifying external VBUS to DWC2, we should use it.
> Why would that be a problem ?

You can bring in the DT node, then use special code to find it. But I
looked in the linux kernel and cannot find vbus-supply. Are you going
to send that patch to the kernel at some point?

>
>>>> +    if (ret) {
>>>> +        debug("%s: No vbus supply\n", g_dwc2_udev->name);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    ret = regulator_set_enable(vbus_supply, true);
>>> Shouldn't the regulator be enabled when we enable VBUS for a port
>>> instead of here ? And where is it disabled ?
>>
>> I add this function just after the controller enable the port power bit,
>> isn't it?
>> I didn't found anywhere the driver disable the vbus power, we do not
>> need it
>> in U-Boot?
>
> So we leave it enabled and pass the hardware to kernel in some weird
> state ? That's a bit odd, isn't it. dwc2_lowlevel_stop() might be a
> good place to disable the regulator ...
>
> --
> Best regards,
> Marek Vasut

Regards,
Simon

[U-Boot] [PATCH v2 2/4] board: popmetal: de-assert the host rst pin in board init

[Simon Glass]

On 29 November 2016 at 17:34, Simon Glass <sjg@chromium.org> wrote:
> On 29 November 2016 at 01:49, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Simon,
>>
>>
>> On 11/26/2016 03:39 AM, Simon Glass wrote:
>>>
>>> Hi Kever,
>>>
>>> On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com>
>>> wrote:
>>>>
>>>> The PopMetal board have a on board FE1.1 usb 2.0 hub which connect to
>>>> the usb host port, we need to de-assert its reset pin to enable it.
>>>>
>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - move the vbus power enable into dwc2 driver
>>>>
>>>>   board/chipspark/popmetal_rk3288/popmetal-rk3288.c | 17
>>>> +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>>> b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>>> index aad74ef..ed82b2b 100644
>>>> --- a/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>>> +++ b/board/chipspark/popmetal_rk3288/popmetal-rk3288.c
>>>> @@ -6,6 +6,7 @@
>>>>
>>>>   #include <common.h>
>>>>   #include <spl.h>
>>>> +#include <asm/gpio.h>
>>>>
>>>>   void board_boot_order(u32 *spl_boot_list)
>>>>   {
>>>> @@ -13,3 +14,19 @@ void board_boot_order(u32 *spl_boot_list)
>>>>          spl_boot_list[0] = BOOT_DEVICE_MMC2;
>>>>          spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>>   }
>>>> +
>>>> +#define GPIO7A3_HUB_RST        227
>>>> +
>>>> +int rk_board_late_init(void)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = gpio_request(GPIO7A3_HUB_RST, "hub_rst");
>>>> +       if (ret)
>>>> +               return ret;
>>>> +       ret = gpio_direction_output(GPIO7A3_HUB_RST, 1);
>>>> +       if (ret)
>>>
>>> Can we get this from the device tree instead of hard-coding it? Then
>>> it can go in generic code.
>>
>>
>> I don't understand how to get this from device tree, this board is the
>> only one based on rk3288 with a USB hub on board and need the de-assert
>> its reset pin. I think it is reasonable to hard coding it in its board file.
>
> OK, since it's just one board.
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> In general we can support this sort of thing by adding a driver for
> UCLASS_USB_HUB, but I suspect it would need refactoring of
> common/usb.c.

Added 'rockchip' tag,

Applied to u-boot-rockchip, thanks.

[U-Boot] [PATCH v2 3/4] config: popmetal: enable the USB host controller and function

[Simon Glass]

On 24 November 2016 at 00:29, Kever Yang <kever.yang@rock-chips.com> wrote:
> RK3288 using the dwc2 USB host controller, enable it and other usb host
> funtion like storage and ether.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2: None
>
>  configs/popmetal-rk3288_defconfig | 3 +++
>  include/configs/rk3288_common.h   | 7 +++++++
>  2 files changed, 10 insertions(+)
>

Added 'rockchip' tag,

Applied to u-boot-rockchip, thanks.