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

[Marek Vasut]

On 06/09/2017 04:01 PM, rock-chips(daniel.meng) wrote:
> 
> 
> 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")  ?

Yes

>>>>
>>>>> +#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.

Ugh, what ? You can certainly have a host port which does support
switching VBUS on/off. In fact, that's the majority case. You can
just toggle VBUS and reenumerate the whole bus, that's normal and
majority of the USB controller driver(s) do it.

> In this case, setting regulator will return fail, but it's
> also a right case.

No, if something fails, it's definitely not right. If you use dummy
regulator on your hardware because your hardware doesn't support VBUS
switching (because it's sub-par), that's fine, but setting the regulator
on/off must not fail in that case.

> So I think it can be treated as  a debug or warning message.

No, this is an error.

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

No, printing a message and ignoring error is NOT enough, the error must
be propagated.

>>>>>        return 0;
>>>>>    }
>>>>>   
>>>
>>
> 
> 


-- 
Best regards,
Marek Vasut