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

[Simon Glass]

On 19 June 2017 at 04:15, Marek Vasut <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> 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> 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> 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>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> 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.

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

Regards,
Simon