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

Thu Feb 9 09:09:50 UTC 2017

On 02/09/2017 10:08 AM, Konstantin Porotchkin wrote:
> 
> 
> On 02/09/2017 10:32 AM, Marek Vasut wrote:
>> 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 ?
>>
> They are not mandatory. This entry can be used for enabling gated clocks
> on a specific platform. However Kernel code does not handle missing
> clock entry as an error. It just assumes that the clock is not gated and
> therefore should not be enabled upon host controller probe.
> So maybe I got you wrong. My feeling was that you requested to make this
> entry mandatory.

I have no idea what close those are (based on the description), so I
cannot decide either way. If this is something which is present only
on selected platforms, then they are optional indeed.

>>> 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 ?
> Because this is an optional parameter and can be used for enabling gated
> clock if one is defined.

So these are different clock from the SERDES clock, yes ?

>>>>> 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 = <&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 ;-)
>>>>>>
>>>>> 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.
> I tried your suggestion and it works for MACCHIATOBin board. However if
> I not surround this new code with "ifdef CONFIG_DM_REGULATOR_FIXED", the
> build for other boards based on same SoC will fail.
> So I have 2 possible solutions for this issue - make the mvebu_xhci
> driver depend on CONFIG_DM_REGULATOR_FIXED and add this configuration
> entry to defconfigs of all affected boards, or use the "ifdef" as in
> previous code. What is preferred?

Probably just enabling the regulator framework should be enough ?

[...]

-- 
Best regards,
Marek Vasut