From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Thu, 9 Feb 2017 09:32:36 +0100 Subject: [U-Boot] [PATCH v3 4/6] mvebu: usb: xhci: Add VBUS regulator supply to the host driver In-Reply-To: References: <1486568056-724-1-git-send-email-kostap@marvell.com> <1486568056-724-5-git-send-email-kostap@marvell.com> <60ce539c-3da3-4c91-3577-5a9c39b27e25@denx.de> <657d672a-ec19-8bc9-e04d-fc513999fa6b@marvell.com> <1e808bb0-22c0-f00c-b4fa-9cad96dd98e0@denx.de> <93bf1d43-40b4-d2aa-c1bc-c7fed360d718@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>> >>>>>>> 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 >>>>>>> Cc: Stefan Roese >>>>>>> Cc: Marek Vasut >>>>>>> Cc: Nadav Haklai >>>>>>> Cc: Neta Zur Hershkovits >>>>>>> Cc: Igal Liberman >>>>>>> Cc: Haim Boot >>>>>>> --- >>>>>>> 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 = ; >>>>>>> + 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