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

[Kostya Porotchkin <kostap@marvell.com>]

Hi, Marek,
________________________________________
From: Marek Vasut <marex@denx.de>
Sent: Tuesday, January 24, 2017 16:11
To: Stefan Roese; Kostya Porotchkin; u-boot at lists.denx.de
Cc: Haim Boot; Hanna Hawa; Omri Itach; Nadav Haklai; Neta Zur Hershkovits; Igal Liberman
Subject: [EXT] Re: [U-Boot] [PATCH v2 4/7] mvebu: usb: xhci: Add support for VBUS controlled by GPIO

----------------------------------------------------------------------
On 01/24/2017 02:53 PM, Stefan Roese wrote:
> Hi Marek,
>
> (adding Marek as USB custodian to Cc)
>
> On 08.01.2017 15:52, kostap at marvell.com wrote:
>> From: Konstantin Porotchkin <kostap@marvell.com>
>>
>> Add support for "marvell,vbus-gpio" property to mvebu XHCI
>> host adapter driver.
>> This option is valid when CONFIG_DM_GPIO=y
>>
>> Change-Id: I930b3ebe001e50ae8d5abe1f3c774bcdb1739e64
>> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Nadav Haklai <nadavh@marvell.com>
>> Cc: Neta Zur Hershkovits <neta@marvell.com>
>> Cc: Omri Itach <omrii@marvell.com>
>> Cc: Igal Liberman <igall@marvell.com>
>> Cc: Haim Boot <hayim@marvell.com>
>> Cc: Hanna Hawa <hannah@marvell.com>
>> ---
>> Changes for v2:
>> - Move VBUS GPIO support from board-specific function to mvebu XHCI
>> driver
>> - Increase delay after VBUS GPIO activation
>>
>>  doc/device-tree-bindings/usb/marvell.xhci-usb.txt | 29
>> +++++++++++++++++++++++
>>  drivers/usb/host/xhci-mvebu.c                     | 14 +++++++++++
>>  2 files changed, 43 insertions(+)
>>  create mode 100644 doc/device-tree-bindings/usb/marvell.xhci-usb.txt
>>
>> diff --git a/doc/device-tree-bindings/usb/marvell.xhci-usb.txt
>> b/doc/device-tree-bindings/usb/marvell.xhci-usb.txt
>> new file mode 100644
>> index 0000000..b0a53ad
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/usb/marvell.xhci-usb.txt
>> @@ -0,0 +1,29 @@
>> +Marvell SOC USB controllers
>> +
>> +This controller is integrated in Armada 3700/8K.
>> +It uses the same properties as a generic XHCI host controller
>> +
>> +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.

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

>> +Example:
>> +    cpm_usb3_0: usb3 at 500000 {
>> +        compatible = "marvell,armada-8k-xhci",
>> +                 "generic-xhci";
>> +        reg = <0x500000 0x4000>;
>> +        interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&cpm_syscon0 1 22>;
>> +        marvell,vbus-gpio = <&cpm_gpio1 15 GPIO_ACTIVE_HIGH>;
>> +        status = "disabled";
>> +    };
>> +
>> diff --git a/drivers/usb/host/xhci-mvebu.c
>> b/drivers/usb/host/xhci-mvebu.c
>> index 46eb937..64801e7 100644
>> --- a/drivers/usb/host/xhci-mvebu.c
>> +++ b/drivers/usb/host/xhci-mvebu.c
>> @@ -45,6 +45,20 @@ static int xhci_usb_probe(struct udevice *dev)
>>      struct mvebu_xhci *ctx = dev_get_priv(dev);
>>      struct xhci_hcor *hcor;
>>      int len;
>> +#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?

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