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

2016-12-02 Thread Simon Glass 
Hi Marek,

On 29 November 2016 at 19:00, Marek Vasut  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 
 ---

 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 
   #include 
   #include 
 +#include 
 #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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-11-29 Thread 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 
>>> ---
>>>
>>> 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 
>>>   #include 
>>>   #include 
>>> +#include 
>>> #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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-11-29 Thread 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 
---

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 
  #include 
  #include 
+#include 
  
  #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, ®s->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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-11-25 Thread 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 
> ---
> 
> 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 
>  #include 
>  #include 
> +#include 
>  
>  #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, ®s->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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-11-23 Thread 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 
---

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 
 #include 
 #include 
+#include 
 
 #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, ®s->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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot