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

[rock-chips(daniel.meng)]

On 2017/6/9 16:22, Marek Vasut wrote:
> On 06/09/2017 09:49 AM, rock-chips(daniel.meng) wrote:
>>
>> On 2017/6/8 21:17, Marek Vasut wrote:
>>> On 06/08/2017 09:31 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 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 | 41
>>>> +++++++++++++++++++++++++++++++---------
>>>>    1 file changed, 32 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-rockchip.c
>>>> b/drivers/usb/host/xhci-rockchip.c
>>>> index f559830..dc9cd56 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;
>>>>    };
>>>>      /*
>>>> @@ -66,15 +66,37 @@ static int xhci_usb_ofdata_to_platdata(struct
>>>> udevice *dev)
>>>>            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)
>>>> +    /* Vbus regulator */
>>>> +    ret = device_get_supply_regulator(dev, "vbus-supply",
>>>> +                      &plat->vbus_supply);
>>>>        if (ret)
>>>> -        debug("rockchip,vbus-gpio node missing!");
>>>> +        debug("Can't get vbus supply\n");
>>> VBUS in caps
change the print message?
debug("Can't get VBUS regulator\n")  ?
>>>
>>>> +#endif
>>>>          return 0;
>>>>    }
>>>>    +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR)
>>>> +static int rockchip_xhci_set_vbus(struct rockchip_xhci_platdata *plat,
>>>> +                  bool value)
>>>> +{
>>>> +    int ret = 0;
>>> You don't need to init ret, it's always set right below.
OK, I will correct
>>>
>>>> +
>>>> +    ret = regulator_set_enable(plat->vbus_supply, value);
>>>> +    if (ret)
>>>> +        debug("XHCI: Failed to set vbus supply\n");
>>> That shouldn't be debug, that's a printf() because it's actually a
>>> failure. Or error() I guess ...
>> Considering the case vbus is always on, it is right with no vbus regulator.
>> So maybe it's not an error actually.
> Regulator failed to turn on, that's an error, right ?
> Why would VBUS be always on ? You can just add a GPIO-regulator into the
> DT and then the VBUS is no longer always on.
It's certainly right adding a GPIO-regulator into the DT. But when it's 
designed
as  a host only port, the VBUS would be fixed to on by hardware and 
there is no
gpio for it. In this case, setting regulator will return fail, but it's 
also a right case.
So I think it can be treated as  a debug or warning message.
>
>>>> +    return ret;
>>>> +}
>>>> +#else
>>>> +static int rockchip_xhci_set_vbus(struct rockchip_xhci_platdata *plat,
>>>> +                  bool value)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>    /*
>>>>     * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3
>>>> Core
>>>>     * @dwc: Pointer to our controller context structure
>>>> @@ -153,9 +175,7 @@ static int xhci_usb_probe(struct udevice *dev)
>>>>        hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd +
>>>>                HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase)));
>>>>    -    /* setup the Vbus gpio here */
>>>> -    if (dm_gpio_is_valid(&plat->vbus_gpio))
>>>> -        dm_gpio_set_value(&plat->vbus_gpio, 1);
>>>> +    rockchip_xhci_set_vbus(plat, true);
>>> What about handling the return value ?
>> The return value can be ignored when vbus is always on. So is it enough
>> just print message in
>> the rockchip_xhci_set_vbus function?
> See above, why is VBUS always on
answer above
>
>>>>        ret = rockchip_xhci_core_init(ctx, dev);
>>>>        if (ret) {
>>>> @@ -168,6 +188,7 @@ static int xhci_usb_probe(struct udevice *dev)
>>>>      static int xhci_usb_remove(struct udevice *dev)
>>>>    {
>>>> +    struct rockchip_xhci_platdata *plat = dev_get_platdata(dev);
>>>>        struct rockchip_xhci *ctx = dev_get_priv(dev);
>>>>        int ret;
>>>>    @@ -178,6 +199,8 @@ static int xhci_usb_remove(struct udevice *dev)
>>>>        if (ret)
>>>>            return ret;
>>>>    +    rockchip_xhci_set_vbus(plat, false);
>>> Handle return value
I think print message may be  is enough.
>>>
>>>>        return 0;
>>>>    }
>>>>   
>>
>