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@marvell.com wrote: >>>> From: Konstantin Porotchkin kostap@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@marvell.com >>>> Cc: Stefan Roese sr@denx.de >>>> Cc: Marek Vasut marex@denx.de >>>> Cc: Nadav Haklai nadavh@marvell.com >>>> Cc: Neta Zur Hershkovits neta@marvell.com >>>> Cc: Igal Liberman igall@marvell.com >>>> Cc: Haim Boot hayim@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@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 ?
[...]