[U-Boot] [PATCH v3 4/6] mvebu: usb: xhci: Add VBUS regulator supply to the host driver

[Marek Vasut]

On 02/08/2017 04:45 PM, Konstantin Porotchkin wrote:
> Hi, Marek,
> 
> On 02/08/2017 05:35 PM, Marek Vasut wrote:
>> On 02/08/2017 04:34 PM, kostap at marvell.com wrote:
>> > From: Konstantin Porotchkin <kostap at marvell.com>
>> >
>> > The USB device should linked to VBUS regulator through "vbus-supply"
>> > DTS property.
>> > This patch adds handling for "vbus-supply" property inside the USB
>> > device entry for turning on the VBUS regulator upon the host adapter
>> probe.
>> >
>> > Change-Id: Ibcf72d82298be42353ca03fee064ae8077a7b9de
>> > Signed-off-by: Konstantin Porotchkin <kostap at marvell.com>
>> > Cc: Stefan Roese <sr at denx.de>
>> > Cc: Marek Vasut <marex at denx.de>
>> > Cc: Nadav Haklai <nadavh at marvell.com>
>> > Cc: Neta Zur Hershkovits <neta at marvell.com>
>> > Cc: Igal Liberman <igall at marvell.com>
>> > Cc: Haim Boot <hayim at marvell.com>
>> > ---
>> > Changes for v3:
>> > - Moved VBUS control from private GPIO to a fixed regulator
>> > - Rebase on top of master branch
>> >
>> >
>> >  doc/device-tree-bindings/usb/marvell.xhci-usb.txt | 28
>> ++++++++++++++++++++
>> >  drivers/usb/host/xhci-mvebu.c                     | 31
>> +++++++++++++++++++++++
>> >  2 files changed, 59 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..672a829
>> > --- /dev/null
>> > +++ b/doc/device-tree-bindings/usb/marvell.xhci-usb.txt
>> > @@ -0,0 +1,28 @@
>> > +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
>>
>> What clock ? Why are clock optional ?
>> This probably needs clock-names too.
> This is the way it defined in Linux Kernel.

Then it probably could use fixing there too. Like seriously, what clock
are those ? And why are they optional ? If neither you or me understand
that from the documentation, then the documentation is crap and needs
fixing. It being the same way in Linux is not an argument for sticking
to it.

> I took the Documentation/devicetree/bindings/usb/usb-xhci.txt file as a
> base for my add-on
>>
>> > + - vbus-supply : If present, specifies the fixed regulator to be
>> turned on
>> > +   for providing power to the USB VBUS rail.
>> > +
>> > +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>;
>> > +        vbus-supply = <&reg_usb3h0_vbus>;
>> > +        status = "disabled";
>> > +    };
>> > diff --git a/drivers/usb/host/xhci-mvebu.c
>> b/drivers/usb/host/xhci-mvebu.c
>> > index 46eb937..149f6a4 100644
>> > --- a/drivers/usb/host/xhci-mvebu.c
>> > +++ b/drivers/usb/host/xhci-mvebu.c
>> > @@ -45,7 +45,38 @@ 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_REGULATOR_FIXED
>>
>> Just make the driver depend on REGULATOR_FIXED
> I think the driver can work without regulator if the VBUS rail wired to
> the 5V power line.
> We only need regulator support if this is GPIO controlled

In that case, the regulator won't be found and the driver will ignore
it. Btw I think that violates the USB spec ;-)

>> > +    const void *fdt = gd->fdt_blob;
>> > +    int node = dev->of_offset;
>> > +    const fdt32_t *regulator;
>> > +    int size;
>> >
>> > +    /*
>> > +     * The VBUS supply regulator is not probed automatically
>> > +     * Trigger the regulator probe upon USB port bring up
>> > +     */
>> > +    regulator = fdt_getprop(fdt, node, "vbus-supply", &size);

I think this should use regulator_*() calls from
include/power/regulator.h

>> > +    if (regulator) {
>> > +        uint32_t phandle;
>> > +        struct udevice *config;
>> > +        int reg_node, ret;
>> > +
>> > +        phandle = fdt32_to_cpu(*regulator);
>> > +        reg_node = fdt_node_offset_by_phandle(fdt, phandle);
>> > +        if (reg_node < 0) {
>> > +            dev_err(dev, "vbus-supply has invalid phandle\n");
>> > +            return -EINVAL;
>> > +        }
>> > +        ret = uclass_get_device_by_of_offset(UCLASS_REGULATOR,
>> > +                             reg_node, &config);
>> > +        if (ret) {
>> > +            dev_err(dev, "failed to get VBUS regulator device\n");
>> > +            return ret;
>>
>> Where is the regulator enabled ?
> The regulator is fixed and it is "always-on", so assumed it is enough to
> probe it.
> I have found that once it probed, the USB device becomes powered.

And once someone attaches a different regulator than fixed, this will
break :)

>> > +        }
>> > +    }
>> > +#else
>> > +    debug("VBUS regulator support is missing\n");
>> > +#endif
>> >      ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
>> >      len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
>> >      hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
>> >
>>
>>
> 


-- 
Best regards,
Marek Vasut