[U-Boot] [EXT] Re: [PATCH v2 4/7] mvebu: usb: xhci: Add support for VBUS controlled by GPIO

[Marek Vasut]

On 01/24/2017 03:57 PM, Kostya Porotchkin wrote:
> Hi, Marek,

Hi!

[...]

>>> +Required properties :
>>> + - compatible: should be one or more of:
>>> +   - "marvell,armada3700-xhci", "generic-xhci" for Armada 37xx SoCs
>>> +   - "marvell,armada-8k-xhci", "generic-xhci" for Armada A8K SoCs
>>> + - reg: should contain address and length of the standard XHCI
>>> +    register set for the device.
>>> + - interrupts: one XHCI interrupt should be described here.
>>> +
>>> +Optional properties:
>>> + - clocks: reference to a clock
> 
>> Reference clock are optional ?
> Actually the basis for this file was taken from kernel documentation and clocks are listed as "optional" here.

Oh ok, interesting.

>>> + - marvell,vbus-gpio : If present, specifies a gpio that needs to be
>>> +   activated for the bus to be powered.
> 
>> Shouldn't this be a regulator instead ?
> 
> If it is required, I can implement the regulator method.
> However as you mentioned the regulator core has to be extended for the ramp-up time support.
> This is kind of a shortcut, that enables immediate community board support.
> The same GPIO VBUS implementation exists in Marvell SDK.

Switching to regulator should be pretty painless and much better than
new ad-hoc property, yes.

[...]

>>> +#ifdef CONFIG_DM_GPIO
>>> +    struct gpio_desc vbus_gpio;
>>> +
>>> +    gpio_request_by_name(dev, "marvell,vbus-gpio", 0, &vbus_gpio,
>>> +                 GPIOD_IS_OUT);
>>> +
>>> +    if (dm_gpio_is_valid(&vbus_gpio)) {
>>> +        dm_gpio_set_value(&vbus_gpio, 1);
>>> +        /* Wait for the GPIO VBUS output set */
>>> +        mdelay(500);
> 
>> I think if the GPIO was instead a regulator-fixed, the regulator could
>> handle the delay with something like "ramp-up" delay. But I don't think
>> we do support the ramp-up delay yet ... maybe this can be easily added?
> 
> So I assume you suggest to drop this one and implement the VBUS through regulator GPIO?

Yeah, that'd be neat and if you retain the mdelay() here, it should also
be very straightforward without the need for additional patches.

If you want to extend the regulator-fixed driver to support the ramp-up
delay the same way Linux does, that'd be awesome and you'd be able to
drop even the mdelay() here, which'd be much more systematic solution.

>>> +    }
>>> +#else
>>> +    debug("USB VBUS on GPIO support is missing\n");
>>> +#endif /* CONFIG_DM_GPIO */
>>>
>>>      ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
>>>      len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
>>>
>>
>> Marek, do you have some comments on this USB related patch? If not,
>> are you okay with me pushing it via the Marvell repository?
> 
>> I have one .
> 
> Thank you for your review!
> 
> Best Regards
> Konstantin Porotchkin
> 
> 
> --
> Best regards,
> Marek Vasut
> 


-- 
Best regards,
Marek Vasut