Hi Marek, On 29 November 2016 at 19:00, Marek Vasut <ma...@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.y...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot