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, kos...@marvell.com wrote:
>>>>>>>>> From: Konstantin Porotchkin <kos...@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 <kos...@marvell.com>
>>>>>>>>> Cc: Stefan Roese <s...@denx.de>
>>>>>>>>> Cc: Marek Vasut <ma...@denx.de>
>>>>>>>>> Cc: Nadav Haklai <nad...@marvell.com>
>>>>>>>>> Cc: Neta Zur Hershkovits <n...@marvell.com>
>>>>>>>>> Cc: Igal Liberman <ig...@marvell.com>
>>>>>>>>> Cc: Haim Boot <ha...@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@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.
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
