[U-Boot] [PATCH v3 4/6] mvebu: usb: xhci: Add VBUS regulator supply to the host driver
Konstantin Porotchkin
kostap at marvell.com
Wed Feb 8 16:28:49 UTC 2017
Hi, Marek,
On 02/08/2017 06:04 PM, Marek Vasut wrote:
> 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.
From my point of view this clock entry is optional too.
I am not handling it in any way and the core XHCI driver doesn't uses it.
Should I simply remove this property from the text file?
>
>> 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 = <®_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 ;-)
>
Currently the armada-8040-DB/armada-7040-DB boards use i2c connected
VBUS enable control. If I made this driver depend on REGULATOR_FIXED,
it will not work for these boards. They do not have regulator support
enabled so far.
I do not want to break another systems by adding support for this board.
>>>> + 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
I can call regulator_set_enable() at the end, but I have to locate it
first and get the udev for it.
However it will be enabled already at the time I will call this function.
>
>>>> + 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 :)
What other type of regulators can be used for powering on the USB port?
I believe they are always 5V fixed, are they?
>
>>>> + }
>>>> + }
>>>> +#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);
>>>>
>>>
>>>
>>
>
>
More information about the U-Boot
mailing list