[U-Boot] [PATCH v5 04/11] usb: host: xhci-rockchip: use fixed regulator to control vbus

Tue Jun 20 07:19:40 UTC 2017

On 06/20/2017 05:22 AM, Simon Glass wrote:
> On 19 June 2017 at 04:15, Marek Vasut <marex at denx.de
> <mailto:marex at denx.de>> wrote:
>> On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote:
>>>
>>>
>>> On 2017/6/18 13:11, Marek Vasut wrote:
>>>> On 06/18/2017 12:10 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 17 June 2017 at 13:33, Marek Vasut <marex at denx.de
> <mailto:marex at denx.de>> wrote:
>>>>>> On 06/17/2017 07:28 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 17 June 2017 at 00:22, Marek Vasut <marex at denx.de
> <mailto:marex at denx.de>> wrote:
>>>>>>>> On 06/17/2017 05:41 AM, Simon Glass wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On 15 June 2017 at 10:53, Marek Vasut <marex at denx.de
> <mailto:marex at denx.de>> wrote:
>>>>>>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 2017/6/13 17:11, Marek Vasut wrote:
>>>>>>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote:
>>>>>>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off
>>>>>>>>>>>>> vbus when usb stop.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng at rock-chips.com
> <mailto:daniel.meng at rock-chips.com>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in v5:
>>>>>>>>>>>>> - Propagate return value and print error message when failed
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in v4:
>>>>>>>>>>>>> - Splited from patch [Uboot,v3,04/10]
>>>>>>>>>>>>> - Define set vbus as empty function if the macros aren't set
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in v3: None
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> - Use fixed regulator to control vbus instead of gpio
>>>>>>>>>>>>>
>>>>>>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++----------
>>>>>>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c
>>>>>>>>>>>>> b/drivers/usb/host/xhci-rockchip.c
>>>>>>>>>>>>> index f559830..15df6ef 100644
>>>>>>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c
>>>>>>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c
>>>>>>>>>>>>> @@ -11,10 +11,10 @@
>>>>>>>>>>>>> #include <malloc.h>
>>>>>>>>>>>>> #include <usb.h>
>>>>>>>>>>>>> #include <watchdog.h>
>>>>>>>>>>>>> -#include <asm/gpio.h>
>>>>>>>>>>>>> #include <linux/errno.h>
>>>>>>>>>>>>> #include <linux/compat.h>
>>>>>>>>>>>>> #include <linux/usb/dwc3.h>
>>>>>>>>>>>>> +#include <power/regulator.h>
>>>>>>>>>>>>> #include "xhci.h"
>>>>>>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>>>>>> struct rockchip_xhci_platdata {
>>>>>>>>>>>>> fdt_addr_t hcd_base;
>>>>>>>>>>>>> fdt_addr_t phy_base;
>>>>>>>>>>>>> - struct gpio_desc vbus_gpio;
>>>>>>>>>>>>> + struct udevice *vbus_supply;
>>>>>>>>>>>>> };
>>>>>>>>>>>>> /*
>>>>>>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct
>>>>>>>>>>>>> udevice *dev)
>>>>>>>>>>>>> */
>>>>>>>>>>>>> plat->hcd_base = dev_get_addr(dev);
>>>>>>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) {
>>>>>>>>>>>>> - debug("Can't get the XHCI register base address\n");
>>>>>>>>>>>>> + error("Can't get the XHCI register base address\n");
>>>>>>>>>>>>> return -ENXIO;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> @@ -62,19 +62,39 @@ static int
>>>>>>>>>>>>> xhci_usb_ofdata_to_platdata(struct
>>>>>>>>>>>>> udevice *dev)
>>>>>>>>>>>>> }
>>>>>>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) {
>>>>>>>>>>>>> - debug("Can't get the usbphy register address\n");
>>>>>>>>>>>>> + error("Can't get the usbphy register address\n");
>>>>>>>>>>>>> return -ENXIO;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> - /* Vbus gpio */
>>>>>>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0,
>>>>>>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT);
>>>>>>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
>>>>>>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends
>>>>>>>>>>>> on it (or
>>>>>>>>>>>> should) already anyway.
>>>>>>>>>>> Yes, I will remove it in v6.
>>>>>>>>>>>>> + /* Vbus regulator */
>>>>>>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply",
>>>>>>>>>>>>> + &plat->vbus_supply);
>>>>>>>>>>>> So I was curious, does this expand to empty function or is
>>>>>>>>>>>> this not
>>>>>>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ?
>>>>>>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined.
>>>>>>>>>> Simon, can you comment on this ?
>>>>>>>>> It looks OK to me.
>>>>>>>> Shouldn't you have empty stub functions instead to avoid
>>>>>>>> proliferation
>>>>>>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ?
>>>>>>> You could, but this is just a temporary state while apparently some
>>>>>>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what
>>>>>>> those bad boards are, actually.
>>>>>> Temporary state ? What's the final state then ?
>>>>> Well I wasn't aware that any rockchip boards didn't use regulators.
>>>>> Presumably this #ifdef is because of a board that does not.
>>>> This is IMO unrelated to rockchip.
>>>>
>>>>>> Anyway, what if you just disable regulator support (because you don't
>>>>>> need it for whatever reason), but want to keep USB ? Wouldn't it make
>>>>>> more sense to have empty stub functions instead of swamping the
> drivers
>>>>>> with ifdefs ?
>>>>> USB would not work without VBUS, which is handled with regulators
>>>> The VBUS can be directly tied to 5V rail without power switching.
>>>>
>>>>> , so
>>>>> there is something I don't understand here. Anyway I don't see any
>>>>> point in adding stub functions here.
>>>> Not here, into the regulator framework, so you don't have to pollute
>>>> drivers which use the regulators with ifdefs if the regulator framework
>>>> is not enabled in the config.
>>>>
>>>>> Meng can you please explain why the #ifdef is needed?
>>>
>>> Because the function "device_get_supply_regulator" is undefined if
>>> undefined
>>> CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the
>>> regulator
>>> framework can be optimized and make it compiled successful whether define
>>> CONFIG_DM_REGULATOR.
>>> So is this #ifdef still needed here?
>>
>> Thus my discussion with Simon. Linux does keep the ifdefs in framework
>> header files , so I think we should do the same.
> 
> OK, but arguably that is a separate issue from this patch.

It is, and I believe it should be discussed and possibly fixed.

> Also I hope we can always enable DM_REGULATOR for rockchip and drop the
> #ifdefs.

If the driver explicitly depends on DM_REGULATOR in Kconfig , then yes.

-- 
Best regards,
Marek Vasut