[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 at 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