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

>
>> +
>>   /*
>>    * 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;
>>
>