[U-Boot] [PATCH v3 0/6] arm64: mvebu: Add support for A8K community board

[U-Boot] [PATCH v3 0/6] arm64: mvebu: Add support for A8K community board

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

This patch series adds initil support for A8K community board MACCHIATOBin manufactured by SolidRun.
It should be applied on top of Stefan Roese patches adding support for SD/eMMC devices on Marvell A37x0/A80x0/A70x0 SoCs.
The top level patch is called:
"arm64: mvebu: Enable SDHCI/MMC support for the db-88f7040/8040"
The default board DTS file is based on the original work of Rabeeh Khory from SolidRun.

Changes for v3:
- Remove bubt command fixes - thay were already merged to the master branch
- Moved the USB VBUS control from proprietary GPIO to a fixed regulator
- Added SD and eMMC entries to the DTS
- Aligned default configutration with Armada-8040-DB
- Rebased on top of master branch

Konstantin Porotchkin (5):
  arm64: mvebu: gpio: Add GPIO nodes to A8K family devices
  arm64: mvebu: dts: Add i2c1 pin definitions to CPM
  mvebu: pcie: Add support for GPIO reset for PCIe device
  mvebu: usb: xhci: Add VBUS regulator supply to the host driver
  arm64: mvebu: Add default configuraton for MACCHIATOBin board

Rabeeh Khoury (1):
  arm64: mvebu: dts: Add DTS file for MACCHIATOBin board

 arch/arm/dts/Makefile                             |   1 +
 arch/arm/dts/armada-7040.dtsi                     |   1 +
 arch/arm/dts/armada-8040-mcbin.dts                | 287 ++++++++++++++++++++++
 arch/arm/dts/armada-8040.dtsi                     |   1 +
 arch/arm/dts/armada-ap806.dtsi                    |   8 +
 arch/arm/dts/armada-cp110-master.dtsi             |  22 ++
 arch/arm/dts/armada-cp110-slave.dtsi              |  18 ++
 configs/mvebu_mcbin-88f8040_defconfig             |  65 +++++
 doc/device-tree-bindings/pci/armada8k-pcie.txt    |  49 ++++
 doc/device-tree-bindings/usb/marvell.xhci-usb.txt |  28 +++
 drivers/pci/pcie_dw_mvebu.c                       |  20 ++
 drivers/usb/host/xhci-mvebu.c                     |  31 +++
 12 files changed, 531 insertions(+)
 create mode 100644 arch/arm/dts/armada-8040-mcbin.dts
 create mode 100644 configs/mvebu_mcbin-88f8040_defconfig
 create mode 100644 doc/device-tree-bindings/pci/armada8k-pcie.txt
 create mode 100644 doc/device-tree-bindings/usb/marvell.xhci-usb.txt

-- 
2.7.4

[U-Boot] [PATCH v3 1/6] arm64: mvebu: gpio: Add GPIO nodes to A8K family devices

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

Add GPIO nodes to AP-806 and CP-110-master DTSI files.

Change-Id: I05958698d460cb721b7d8683d34f74a5ea32532c
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Nadav Haklai <nadavh@marvell.com>
Cc: Igal Liberman <igall@marvell.com>
Cc: Haim Boot <hayim@marvell.com>
---
Changes for v3:
- Rebase on top of master branch

 arch/arm/dts/armada-7040.dtsi         |  1 +
 arch/arm/dts/armada-8040.dtsi         |  1 +
 arch/arm/dts/armada-ap806.dtsi        |  8 ++++++++
 arch/arm/dts/armada-cp110-master.dtsi | 18 ++++++++++++++++++
 arch/arm/dts/armada-cp110-slave.dtsi  | 18 ++++++++++++++++++
 5 files changed, 46 insertions(+)

diff --git a/arch/arm/dts/armada-7040.dtsi b/arch/arm/dts/armada-7040.dtsi
index 78d995d..b5be0c4 100644
--- a/arch/arm/dts/armada-7040.dtsi
+++ b/arch/arm/dts/armada-7040.dtsi
@@ -45,6 +45,7 @@
  * one CP110.
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "armada-ap806-quad.dtsi"
 #include "armada-cp110-master.dtsi"
 
diff --git a/arch/arm/dts/armada-8040.dtsi b/arch/arm/dts/armada-8040.dtsi
index 9c1b28c..96cc112 100644
--- a/arch/arm/dts/armada-8040.dtsi
+++ b/arch/arm/dts/armada-8040.dtsi
@@ -45,6 +45,7 @@
  * two CP110.
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "armada-ap806-quad.dtsi"
 #include "armada-cp110-master.dtsi"
 #include "armada-cp110-slave.dtsi"
diff --git a/arch/arm/dts/armada-ap806.dtsi b/arch/arm/dts/armada-ap806.dtsi
index 3042cb1..e0d3016 100644
--- a/arch/arm/dts/armada-ap806.dtsi
+++ b/arch/arm/dts/armada-ap806.dtsi
@@ -158,6 +158,14 @@
 				};
 			};
 
+			ap_gpio0: gpio at 6F5040 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x6F5040 0x40>;
+				ngpios = <20>;
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+
 			xor at 400000 {
 				compatible = "marvell,mv-xor-v2";
 				reg = <0x400000 0x1000>,
diff --git a/arch/arm/dts/armada-cp110-master.dtsi b/arch/arm/dts/armada-cp110-master.dtsi
index 661a696..6095609 100644
--- a/arch/arm/dts/armada-cp110-master.dtsi
+++ b/arch/arm/dts/armada-cp110-master.dtsi
@@ -113,6 +113,24 @@
 				};
 			};
 
+			cpm_gpio0: gpio at 440100 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x440100 0x40>;
+				ngpios = <32>;
+				gpiobase = <20>;
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+
+			cpm_gpio1: gpio at 440140 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x440140 0x40>;
+				ngpios = <31>;
+				gpiobase = <52>;
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+
 			cpm_sata0: sata at 540000 {
 				compatible = "marvell,armada-8k-ahci";
 				reg = <0x540000 0x30000>;
diff --git a/arch/arm/dts/armada-cp110-slave.dtsi b/arch/arm/dts/armada-cp110-slave.dtsi
index 92ef55c..ff3fbed 100644
--- a/arch/arm/dts/armada-cp110-slave.dtsi
+++ b/arch/arm/dts/armada-cp110-slave.dtsi
@@ -100,6 +100,24 @@
 				};
 			};
 
+			cps_gpio0: gpio at 440100 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x440100 0x40>;
+				ngpios = <32>;
+				gpiobase = <20>;
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+
+			cps_gpio1: gpio at 440140 {
+				compatible = "marvell,orion-gpio";
+				reg = <0x440140 0x40>;
+				ngpios = <31>;
+				gpiobase = <52>;
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+
 			cps_sata0: sata at 540000 {
 				compatible = "marvell,armada-8k-ahci";
 				reg = <0x540000 0x30000>;
-- 
2.7.4

[U-Boot] [PATCH v3 1/6] arm64: mvebu: gpio: Add GPIO nodes to A8K family devices

[Stefan Roese]

On 08.02.2017 16:34, kostap at marvell.com wrote:
> From: Konstantin Porotchkin <kostap@marvell.com>
>
> Add GPIO nodes to AP-806 and CP-110-master DTSI files.
>
> Change-Id: I05958698d460cb721b7d8683d34f74a5ea32532c
> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Nadav Haklai <nadavh@marvell.com>
> Cc: Igal Liberman <igall@marvell.com>
> Cc: Haim Boot <hayim@marvell.com>

Complete series applied to u-boot-mavell/master.

Thanks,
Stefan

[U-Boot] [PATCH v3 2/6] arm64: mvebu: dts: Add i2c1 pin definitions to CPM

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

Add i2c-1 pin mappings to CP0(master) DTSI file

Change-Id: I0c6e6de8a557393f518f7df8e6daa6dfce1788b0
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
Cc: Stefan Roese <sr@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:
- Rebase on top of master branch

 arch/arm/dts/armada-cp110-master.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/dts/armada-cp110-master.dtsi b/arch/arm/dts/armada-cp110-master.dtsi
index 6095609..1f0edde 100644
--- a/arch/arm/dts/armada-cp110-master.dtsi
+++ b/arch/arm/dts/armada-cp110-master.dtsi
@@ -94,6 +94,10 @@
 					marvell,pins = < 37 38 >;
 					marvell,function = <2>;
 				};
+				cpm_i2c1_pins: cpm-i2c-pins-1 {
+					marvell,pins = < 35 36 >;
+					marvell,function = <2>;
+				};
 				cpm_ge2_rgmii_pins: cpm-ge-rgmii-pins-0 {
 					marvell,pins = < 44 45 46 47 48 49 50 51
 							 52 53 54 55 >;
-- 
2.7.4

[U-Boot] [PATCH v3 3/6] mvebu: pcie: Add support for GPIO reset for PCIe device

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

Add support for "marvell,reset-gpio" property to mvebu DW PCIe
driver.
This option is valid when CONFIG_DM_GPIO=y

Change-Id: Ic17c500449050c2fbb700731f1a9ca8b83298986
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
Signed-off-by: Rabeeh Khoury <rabeeh@solid-run.com>
Cc: Stefan Roese <sr@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:
- Rebase on top of master branch

 doc/device-tree-bindings/pci/armada8k-pcie.txt | 49 ++++++++++++++++++++++++++
 drivers/pci/pcie_dw_mvebu.c                    | 20 +++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 doc/device-tree-bindings/pci/armada8k-pcie.txt

diff --git a/doc/device-tree-bindings/pci/armada8k-pcie.txt b/doc/device-tree-bindings/pci/armada8k-pcie.txt
new file mode 100644
index 0000000..7230f10
--- /dev/null
+++ b/doc/device-tree-bindings/pci/armada8k-pcie.txt
@@ -0,0 +1,49 @@
+Armada-8K PCIe DT details:
+==========================
+
+Armada-8k uses synopsis designware PCIe controller.
+
+Required properties:
+- compatible : should be "marvell,armada8k-pcie", "snps,dw-pcie".
+- reg: base addresses and lengths of the pcie control and global control registers.
+ "ctrl" registers points to the global control registers, while the "config" space
+ points to the pcie configuration registers as mentioned in dw-pcie dt bindings in the link below.
+- interrupt-map-mask and interrupt-map, standard PCI properties to
+  define the mapping of the PCIe interface to interrupt numbers.
+- All other definitions as per generic PCI bindings
+See Linux kernel documentation:
+"Documentation/devicetree/bindings/pci/designware-pcie.txt"
+
+Optional properties:
+PHY support is still not supported for armada-8k, once it will, the following parameters can be used:
+- phys		    : phandle to phy node associated with pcie controller.
+- phy-names	    : must be "pcie-phy"
+- marvell,reset-gpio :  specifies a gpio that needs to be activated for plug-in
+			card reset signal release.
+Example:
+
+cpm_pcie0: pcie at f2600000 {
+	compatible = "marvell,armada8k-pcie", "snps,dw-pcie";
+	reg = <0 0xf2600000 0 0x10000>,
+	      <0 0xf6f00000 0 0x80000>;
+	reg-names = "ctrl", "config";
+	#address-cells = <3>;
+	#size-cells = <2>;
+	#interrupt-cells = <1>;
+	device_type = "pci";
+	dma-coherent;
+
+	bus-range = <0 0xff>;
+	ranges =
+		/* downstream I/O */
+		<0x81000000 0 0xf9000000 0  0xf9000000 0 0x10000
+		/* non-prefetchable memory */
+		0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;
+	interrupt-map-mask = <0 0 0 0>;
+	interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+	num-lanes = <1>;
+	clocks = <&cpm_syscon0 1 13>;
+	marvell,reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_HIGH>;
+	status = "disabled";
+};
diff --git a/drivers/pci/pcie_dw_mvebu.c b/drivers/pci/pcie_dw_mvebu.c
index 17fa024..d4776a9 100644
--- a/drivers/pci/pcie_dw_mvebu.c
+++ b/drivers/pci/pcie_dw_mvebu.c
@@ -15,6 +15,7 @@
 #include <dm.h>
 #include <pci.h>
 #include <asm/io.h>
+#include <asm-generic/gpio.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -461,6 +462,25 @@ static int pcie_dw_mvebu_probe(struct udevice *dev)
 	struct pcie_dw_mvebu *pcie = dev_get_priv(dev);
 	struct udevice *ctlr = pci_get_controller(dev);
 	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
+#ifdef CONFIG_DM_GPIO
+	struct gpio_desc reset_gpio;
+
+	gpio_request_by_name(dev, "marvell,reset-gpio", 0, &reset_gpio,
+			     GPIOD_IS_OUT);
+	/*
+	 * Issue reset to add-in card trough the dedicated GPIO.
+	 * Some boards are connecting the card reset pin to common system
+	 * reset wire and others are using separate GPIO port.
+	 * In the last case we have to release a reset of the addon card
+	 * using this GPIO.
+	 */
+	if (dm_gpio_is_valid(&reset_gpio)) {
+		dm_gpio_set_value(&reset_gpio, 1);
+		mdelay(200);
+	}
+#else
+	debug("PCIE Reset on GPIO support is missing\n");
+#endif /* CONFIG_DM_GPIO */
 
 	pcie->first_busno = dev->seq;
 
-- 
2.7.4

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

[kostap at marvell.com]

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
+ - 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
+	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);
+	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;
+		}
+	}
+#else
+	debug("VBUS regulator support is missing\n");
+#endif
 	ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
 	len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
 	hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
-- 
2.7.4

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

[Marek Vasut]

On 02/08/2017 04:34 PM, kostap at 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.

> + - 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

> +	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);
> +	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 ?

> +		}
> +	}
> +#else
> +	debug("VBUS regulator support is missing\n");
> +#endif
>  	ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
>  	len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
>  	hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
> 


-- 
Best regards,
Marek Vasut

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

[Konstantin Porotchkin]

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@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.
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
>
> > +    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);
> > +    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.
>
> > +        }
> > +    }
> > +#else
> > +    debug("VBUS regulator support is missing\n");
> > +#endif
> >      ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
> >      len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
> >      hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
> >
>
>

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

[Marek Vasut]

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@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.

> 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 ;-)

>> > +    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

>> > +    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 :)

>> > +        }
>> > +    }
>> > +#else
>> > +    debug("VBUS regulator support is missing\n");
>> > +#endif
>> >      ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
>> >      len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
>> >      hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
>> >
>>
>>
> 


-- 
Best regards,
Marek Vasut

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

[Konstantin Porotchkin]

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@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.
Should I simply remove this property from the text file?

>
>> 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.

>>>> +    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.
>
>>>> +    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?
>
>>>> +        }
>>>> +    }
>>>> +#else
>>>> +    debug("VBUS regulator support is missing\n");
>>>> +#endif
>>>>      ctx->hcd = (struct xhci_hccr *)plat->hcd_base;
>>>>      len = HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase));
>>>>      hcor = (struct xhci_hcor *)((uintptr_t)ctx->hcd + len);
>>>>
>>>
>>>
>>
>
>

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

[Marek Vasut]

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@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.

> 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.

>>>>> +    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", &reg); doesn't work here ?

>>>>> +    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

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

[Konstantin Porotchkin]

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@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. 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.
>
>> 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.
>
>>>>>> +    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", &reg); 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.
>
>>>>>> +    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 ...
>

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

[Marek Vasut]

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@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 ?

> 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 = <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.

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", &reg); 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

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

[Konstantin Porotchkin]

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@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.
>> 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.
>
>>>> 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?
>
> 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", &reg); 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 .
Yes, I understand. And thank you for the fast review!
I wish all my patches get such attention :-)
>
>>>>>>>> +    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 ...
>>>
>
>

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

[Marek Vasut]

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@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 ?

>>>>> 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

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

[Konstantin Porotchkin]

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@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 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 ?
>
> [...]
>

[U-Boot] [PATCH v3 5/6] arm64: mvebu: dts: Add DTS file for MACCHIATOBin board

[kostap at marvell.com]

From: Rabeeh Khoury <rabeeh@solid-run.com>

Added A8040 dts file for community board MACCHIATIBin.
The patch includes the following features:
AP -  Serial console (connected to onboard FTDI usb to serial)
CP0 - PCIe x4, SATA, I2C and 10G KR
      (connected to Marvell 3310 10G copper / SFP+ phy)
CP1 - Boot SPI, USB3 host, 2xSATA, 10G KR
      (connected to Marvell 3310 10G copper / SFP+ phy),
      SGMII connected to onboard 1512 1Gbps copper phy,
      and additional SGMII connected to SFP
      (default 1Gbps can be configured to 2.5Gbps).

Network interface naming -
egiga0 - CP0 KR
egiga1 - CP1 KR
egiga2 - CP1 RJ45 1Gbps connector (recommended for TFTP boot)
egiga3 - CP1 SFP default 1Gbps and can be modified to 2.5Gbps

Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
Signed-off-by: Rabeeh Khoury <rabeeh@solid-run.com>
Cc: Stefan Roese <sr@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:
- Add SD and eMMC entries
- Rebase on top of master branch

 arch/arm/dts/Makefile              |   1 +
 arch/arm/dts/armada-8040-mcbin.dts | 287 +++++++++++++++++++++++++++++++++++++
 2 files changed, 288 insertions(+)
 create mode 100644 arch/arm/dts/armada-8040-mcbin.dts

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 397a0ae..a3b139b 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -76,6 +76,7 @@ dtb-$(CONFIG_ARCH_MVEBU) +=			\
 	armada-385-amc.dtb			\
 	armada-7040-db.dtb			\
 	armada-8040-db.dtb			\
+	armada-8040-mcbin.dtb			\
 	armada-xp-gp.dtb			\
 	armada-xp-maxbcm.dtb			\
 	armada-xp-synology-ds414.dtb		\
diff --git a/arch/arm/dts/armada-8040-mcbin.dts b/arch/arm/dts/armada-8040-mcbin.dts
new file mode 100644
index 0000000..15da53d
--- /dev/null
+++ b/arch/arm/dts/armada-8040-mcbin.dts
@@ -0,0 +1,287 @@
+/*
+ * Copyright (C) 2016 Marvell International Ltd.
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ * https://spdx.org/licenses
+ */
+
+#include "armada-8040.dtsi" /* include SoC device tree */
+
+/ {
+	model = "MACCHIATOBin-8040";
+	compatible = "marvell,armada8040-mcbin",
+		     "marvell,armada8040";
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	aliases {
+		i2c0 = &cpm_i2c0;
+		i2c1 = &cpm_i2c1;
+		spi0 = &cps_spi1;
+		gpio0 = &ap_gpio0;
+		gpio1 = &cpm_gpio0;
+		gpio2 = &cpm_gpio1;
+	};
+
+	memory at 00000000 {
+		device_type = "memory";
+		reg = <0x0 0x0 0x0 0x80000000>;
+	};
+
+reg_usb3h0_vbus: usb3-vbus0 {
+		compatible = "regulator-fixed";
+		pinctrl-names = "default";
+		pinctrl-0 = <&cpm_xhci_vbus_pins>;
+		regulator-name = "reg-usb3h0-vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		startup-delay-us = <500000>;
+		enable-active-high;
+		regulator-always-on;
+		regulator-boot-on;
+		gpio = <&cpm_gpio1 15 GPIO_ACTIVE_HIGH>; /* GPIO[47] */
+	};
+};
+
+/* Accessible over the mini-USB CON9 connector on the main board */
+&uart0 {
+	status = "okay";
+};
+
+&ap_pinctl {
+	/*
+	 * MPP Bus:
+	 * eMMC [0-10]
+	 * UART0 [11,19]
+	 */
+		  /* 0 1 2 3 4 5 6 7 8 9 */
+	pin-func = < 1 1 1 1 1 1 1 1 1 1
+		     1 3 0 0 0 0 0 0 0 3 >;
+};
+
+/* on-board eMMC */
+&ap_sdhci0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ap_emmc_pins>;
+	bus-width= <8>;
+	status = "okay";
+};
+
+&cpm_pinctl {
+	/*
+	 * MPP Bus:
+	 * [0-31] = 0xff: Keep default CP0_shared_pins:
+	 * [11] CLKOUT_MPP_11 (out)
+	 * [23] LINK_RD_IN_CP2CP (in)
+	 * [25] CLKOUT_MPP_25 (out)
+	 * [29] AVS_FB_IN_CP2CP (in)
+	 * [32,34] SMI
+	 * [33]    MSS power down
+	 * [35-38] CP0 I2C1 and I2C0
+	 * [39] MSS CKE Enable
+	 * [40,41] CP0 UART1 TX/RX
+	 * [42,43] XSMI (controls two 10G phys)
+	 * [47] USB VBUS EN
+	 * [48] FAN PWM
+	 * [49] 10G port 1 interrupt
+	 * [50] 10G port 0 interrupt
+	 * [51] 2.5G SFP TX fault
+	 * [52] PCIe reset out
+	 * [53] 2.5G SFP mode
+	 * [54] 2.5G SFP LOS
+	 * [55] Micro SD card detect
+	 * [56-61] Micro SD
+	 * [62] CP1 KR SFP FAULT
+	 */
+		/*   0    1    2    3    4    5    6    7    8    9 */
+	pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0    7    0xa  7    2    2    2    2    0xa
+		     7    7    8    8    0    0    0    0    0    0
+		     0    0    0    0    0    0    0xe  0xe  0xe  0xe
+		     0xe  0xe  0 >;
+
+	cpm_xhci_vbus_pins: cpm-xhci-vbus-pins {
+		marvell,pins = < 47 >;
+		marvell,function = <0>;
+	};
+
+	cpm_pcie_reset_pins: cpm-pcie-reset-pins {
+		marvell,pins = < 52 >;
+		marvell,function = <0>;
+	};
+};
+
+/* uSD slot */
+&cpm_sdhci0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&cpm_sdhci_pins>;
+	bus-width= <4>;
+	status = "okay";
+};
+
+/* PCIe x4 */
+&cpm_pcie0 {
+	num-lanes = <4>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&cpm_pcie_reset_pins>;
+	marvell,reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_HIGH>; /* GPIO[52] */
+	status = "okay";
+};
+
+&cpm_i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&cpm_i2c0_pins>;
+	status = "okay";
+	clock-frequency = <100000>;
+};
+
+&cpm_i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&cpm_i2c1_pins>;
+	status = "okay";
+	clock-frequency = <100000>;
+};
+
+&cpm_sata0 {
+	status = "okay";
+};
+
+&cpm_comphy {
+	/*
+	 * CP0 Serdes Configuration:
+	 * Lane 0: PCIe0 (x4)
+	 * Lane 1: PCIe0 (x4)
+	 * Lane 2: PCIe0 (x4)
+	 * Lane 3: PCIe0 (x4)
+	 * Lane 4: KR (10G)
+	 * Lane 5: SATA1
+	 */
+	phy0 {
+		phy-type = <PHY_TYPE_PEX0>;
+	};
+	phy1 {
+		phy-type = <PHY_TYPE_PEX0>;
+	};
+	phy2 {
+		phy-type = <PHY_TYPE_PEX0>;
+	};
+	phy3 {
+		phy-type = <PHY_TYPE_PEX0>;
+	};
+	phy4 {
+		phy-type = <PHY_TYPE_KR>;
+	};
+	phy5 {
+		phy-type = <PHY_TYPE_SATA1>;
+	};
+};
+
+&cps_sata0 {
+	status = "okay";
+};
+
+&cps_usb3_0 {
+	vbus-supply = <&reg_usb3h0_vbus>;
+	status = "okay";
+};
+
+&cps_utmi0 {
+	status = "okay";
+};
+
+&cps_pinctl {
+	/*
+	 * MPP Bus:
+	 * [0-5] TDM
+	 * [6,7] CP1_UART 0
+	 * [8]   CP1 10G SFP LOS
+	 * [9]   CP1 10G PHY RESET
+	 * [10]  CP1 10G SFP TX Disable
+	 * [11]  CP1 10G SFP Mode
+	 * [12]  SPI1 CS1n
+	 * [13]  SPI1 MISO (TDM and SPI ROM shared)
+	 * [14]  SPI1 CS0n
+	 * [15]  SPI1 MOSI (TDM and SPI ROM shared)
+	 * [16]  SPI1 CLK (TDM and SPI ROM shared)
+	 * [24]  CP1 2.5G SFP TX Disable
+	 * [26]  CP0 10G SFP TX Fault
+	 * [27]  CP0 10G SFP Mode
+	 * [28]  CP0 10G SFP LOS
+	 * [29]  CP0 10G SFP TX Disable
+	 * [30]  USB Over current indication
+	 * [31]  10G Port 0 phy reset
+	 * [32-62] = 0xff: Keep default CP1_shared_pins:
+	 */
+		/*   0    1    2    3    4    5    6    7    8    9 */
+	pin-func = < 0x4  0x4  0x4  0x4  0x4  0x4  0x8  0x8  0x0  0x0
+		     0x0  0x0  0x3  0x3  0x3  0x3  0x3  0xff 0xff 0xff
+		     0xff 0xff 0xff 0xff 0x0  0xff 0x0  0x0  0x0 0x0
+		     0x0  0x0  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
+		     0xff 0xff 0xff>;
+};
+
+&cps_spi1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&cps_spi1_pins>;
+	status = "okay";
+
+	spi-flash at 0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+		spi-max-frequency = <10000000>;
+
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			partition at 0 {
+				label = "U-Boot";
+				reg = <0 0x200000>;
+			};
+			partition at 400000 {
+				label = "Filesystem";
+				reg = <0x200000 0xce0000>;
+			};
+		};
+	};
+};
+
+&cps_comphy {
+	/*
+	 * CP1 Serdes Configuration:
+	 * Lane 0: SGMII2
+	 * Lane 1: SATA 0
+	 * Lane 2: USB HOST 0
+	 * Lane 3: SATA1
+	 * Lane 4: KR (10G)
+	 * Lane 5: SGMII3
+	 */
+	phy0 {
+		phy-type = <PHY_TYPE_SGMII2>;
+		phy-speed = <PHY_SPEED_1_25G>;
+	};
+	phy1 {
+		phy-type = <PHY_TYPE_SATA0>;
+	};
+	phy2 {
+		phy-type = <PHY_TYPE_USB3_HOST0>;
+	};
+	phy3 {
+		phy-type = <PHY_TYPE_SATA1>;
+	};
+	phy4 {
+		phy-type = <PHY_TYPE_KR>;
+	};
+	phy5 {
+		phy-type = <PHY_TYPE_SGMII3>;
+	};
+};
-- 
2.7.4

[U-Boot] [PATCH v3 6/6] arm64: mvebu: Add default configuraton for MACCHIATOBin board

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

Add default configuration for MACHHIATOBin community board
based on Aramda-8040 SoC.
This config has MMC support removed, pending mainline support.

Change-Id: Ic6b562065c0929ec338492452f765115c15a6188
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
Cc: Stefan Roese <sr@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:
- Align the configuration with Armada-8040-DB
- Rebase on top of master branch

 configs/mvebu_mcbin-88f8040_defconfig | 65 +++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 configs/mvebu_mcbin-88f8040_defconfig

diff --git a/configs/mvebu_mcbin-88f8040_defconfig b/configs/mvebu_mcbin-88f8040_defconfig
new file mode 100644
index 0000000..000e400
--- /dev/null
+++ b/configs/mvebu_mcbin-88f8040_defconfig
@@ -0,0 +1,65 @@
+CONFIG_ARM=y
+CONFIG_ARCH_MVEBU=y
+CONFIG_SYS_MALLOC_F_LEN=0x2000
+CONFIG_TARGET_MVEBU_ARMADA_8K=y
+CONFIG_DEFAULT_DEVICE_TREE="armada-8040-mcbin"
+CONFIG_SMBIOS_PRODUCT_NAME=""
+CONFIG_AHCI=y
+# CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
+CONFIG_SYS_CONSOLE_INFO_QUIET=y
+# CONFIG_DISPLAY_CPUINFO is not set
+# CONFIG_DISPLAY_BOARDINFO is not set
+CONFIG_HUSH_PARSER=y
+# CONFIG_CMD_IMLS is not set
+# CONFIG_CMD_FLASH is not set
+CONFIG_CMD_SF=y
+CONFIG_CMD_SPI=y
+CONFIG_CMD_I2C=y
+CONFIG_CMD_USB=y
+# CONFIG_CMD_FPGA is not set
+# CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_TFTPPUT=y
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_MII=y
+CONFIG_CMD_PING=y
+CONFIG_CMD_CACHE=y
+CONFIG_CMD_TIME=y
+CONFIG_CMD_MVEBU_BUBT=y
+CONFIG_CMD_EXT4=y
+CONFIG_CMD_EXT4_WRITE=y
+CONFIG_CMD_FAT=y
+CONFIG_CMD_FS_GENERIC=y
+CONFIG_CMD_GPIO=y
+CONFIG_CMD_REGULATOR=y
+CONFIG_BLOCK_CACHE=y
+CONFIG_DM_I2C=y
+CONFIG_SYS_I2C_MVTWSI=y
+CONFIG_DM_GPIO=y
+CONFIG_MVEBU_GPIO=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_FIXED=y
+CONFIG_MISC=y
+CONFIG_SPI_FLASH=y
+CONFIG_SPI_FLASH_MACRONIX=y
+CONFIG_SPI_FLASH_SPANSION=y
+CONFIG_SPI_FLASH_STMICRO=y
+CONFIG_SPI_FLASH_WINBOND=y
+CONFIG_PHYLIB=y
+CONFIG_PCI=y
+CONFIG_DM_PCI=y
+CONFIG_PCIE_DW_MVEBU=y
+CONFIG_MVEBU_COMPHY_SUPPORT=y
+CONFIG_PINCTRL=y
+# CONFIG_SPL_SERIAL_PRESENT is not set
+CONFIG_DEBUG_UART=y
+CONFIG_DEBUG_UART_BASE=0xf0512000
+CONFIG_DEBUG_UART_CLOCK=200000000
+CONFIG_DEBUG_UART_SHIFT=2
+CONFIG_DEBUG_UART_ANNOUNCE=y
+CONFIG_SYS_NS16550=y
+CONFIG_USB=y
+CONFIG_DM_USB=y
+CONFIG_USB_XHCI_HCD=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_STORAGE=y
+CONFIG_SMBIOS_MANUFACTURER=""
-- 
2.7.4