[U-Boot] [PATCH v3 4/6] mvebu: usb: xhci: Add VBUS regulator supply to the host driver
Marek Vasut
marex at denx.de
Thu Feb 9 08:32:36 UTC 2017
On 02/09/2017 09:00 AM, Konstantin Porotchkin wrote:
>
>
> On 02/08/2017 06:42 PM, Marek Vasut wrote:
>> On 02/08/2017 05:28 PM, Konstantin Porotchkin wrote:
>>> 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.
>>
>> DT is describing the hardware, not the software that is using it. These
>> two things are separate. If the clock are mandatory for the hardware to
>> work, they must be mandatory in the DT.
> I see what you saying. I will move the "clocks" entry to the "mandatory"
> section.
Why ? What clock are those ? Are they mandatory ?
> Please keep in mind that it will not be currently enforced by
> the SW. For USB 3.0 case the clock parameters are actually defined by
> SERDES configuration, which has a separate DTS entry.
Then why are these clock here at all ?
>>> Should I simply remove this property from the text file?
>>
>> See above.
>>
>>>>> 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.
>>
>> Oh, right. Then I believe using the regulator framework will help. The
>> driver can depend on the regulator framework, which will abstract away
>> the used regulator.
> Got it. I will change the code for using the regulator framework in USB
> driver.
Thanks :)
>>>>>>> + 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.
>>
>> regulator_get_by_platname("vbus-supply", ®); doesn't work here ?
> Thank you for the tip! i will definitely try this. Unfortunately I am
> not yet fluent in all the available DM APIs.
That's what the review is for .
>>>>>>> + 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?
>>
>> Anything which can turn 5V on/off , be it some i2c chip, spi chip,
>> GPIO ...
>>
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list