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

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