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

Thu Feb 9 10:42:59 UTC 2017


On 02/09/2017 11:09 AM, Marek Vasut wrote:
> 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 ?
As far as I know the SERDES entry defines the clock speed, which affects 
the initialization flow including the clock dividers.
The clock in USb entry looks like for gating only.
>
>>>>>> 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 ?
>
> [...]
>