[U-Boot] [PATCH v4 04/11] usb: host: xhci-rockchip: use fixed regulator to control vbus
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Fri Jun 9 16:05:58 UTC 2017
> On 09 Jun 2017, at 16:01, rock-chips(daniel.meng) <daniel.meng at rock-chips.com> 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") ?
>>>>
>>>>> +#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.
Not necessarily: we sometimes put a USB hub on our modules and turn it
on and off via the vbus_supply when enabling/disabling the USB controller.
In fact we do just that for the RK3399-Q7.
See https://patchwork.ozlabs.org/patch/769256/ <https://patchwork.ozlabs.org/patch/769256/>.
>>
>>>>> + 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;
>>>>> }
>>>>>
>>>
>>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de <mailto:U-Boot at lists.denx.de>
> https://lists.denx.de/listinfo/u-boot <https://lists.denx.de/listinfo/u-boot>
More information about the U-Boot
mailing list