[PATCH v4 0/4] ata: libahci: Allow using a regulator for each port

[PATCH v4 0/4] ata: libahci: Allow using a regulator for each port

[Gregory CLEMENT]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1828 bytes --]

The current implementation of the libahci allows using one PHY per
port but we still have one single regulator for the whole
controller. This series adds the support of multiple regulators.

This is the forth version of the series.

The improvement of this version is the use of
ahci_platform_put_resources to put the reference to the regulators.

Thanks,

Grégory

Changelog:

 v3 -> v4
- Moved putting the reference to the regulators into the
  ahci_platform_put_resources function.
- Tested the port_dev pointer before dereferencing.

 v2 -> v3:
- put back the regulator inside the sub-node ports
- made the ports platform devices when the device tree is used
- released the regulator in case of error in the probe function

 v1 -> v2:
- Kept the case when no child node are present under the ahci node
- Fix the test done under the label disable_target_pwrs
- No more use an of_ version of the regulator framework and instead
  associate each regulator of a port with an unique name.
- Added the acked-by on the clean-up patch

Gregory CLEMENT (4):
  ata: libahci: Clean-up the ahci_platform_en/disable_phys functions
  Documentation: bindings: Add the regulator property to the sub-nodes
    AHCI bindings
  ata: libahci: Allow using multiple regulators
  ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

 .../devicetree/bindings/ata/ahci-platform.txt      |   9 +-
 arch/arm/boot/dts/armada-388-gp.dts                | 126 +++++++++++
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_imx.c                             |  14 +-
 drivers/ata/libahci_platform.c                     | 236 ++++++++++++++-------
 include/linux/ahci_platform.h                      |   2 +
 6 files changed, 305 insertions(+), 84 deletions(-)

-- 
1.9.1

[PATCH v4 1/4] ata: libahci: Clean-up the ahci_platform_en/disable_phys functions

[Gregory CLEMENT]

The phy_ functions handle the NULL pointer case, so there is no need
to skip them if there is a NULL pointer. Moreover, after the error
label there is already no check on the pointer. This patch removes the
unnecessary tests and brings some consistency.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/libahci_platform.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 0b03f9056692..a147aaadca85 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -54,9 +54,6 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 	int rc, i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
@@ -89,9 +86,6 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	int i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		phy_power_off(hpriv->phys[i]);
 		phy_exit(hpriv->phys[i]);
 	}
-- 
1.9.1

[PATCH v4 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Gregory CLEMENT]

It is now possible to use a regulator property for each port of the
AHCI controller.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 4ab09f2202d4..c2340eeeb97f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -37,9 +37,10 @@ Required properties when using sub-nodes:
 
 
 Sub-nodes required properties:
-- reg               : the port number
-- phys              : reference to the SATA PHY node
-
+- reg		    : the port number
+And at least one of the following properties:
+- phys		    : reference to the SATA PHY node
+- target-supply    : regulator for SATA target power
 
 Examples:
         sata@ffe08000 {
@@ -68,10 +69,12 @@ With sub-nodes:
 		sata0: sata-port@0 {
 			reg = <0>;
 			phys = <&sata_phy 0>;
+			target-supply = <&reg_sata0>;
 		};
 
 		sata1: sata-port@1 {
 			reg = <1>;
 			phys = <&sata_phy 1>;
+			target-supply = <&reg_sata1>;;
 		};
 	};
-- 
1.9.1

[PATCH v4 3/4] ata: libahci: Allow using multiple regulators

[Gregory CLEMENT]

The current implementation of the libahci allows using multiple PHYs
but not multiple regulators. This patch adds the support of multiple
regulators. Until now it was mandatory to have a PHY under a subnode,
now a port subnode can contain either a regulator or a PHY (or both).

In order to be able to asociate a port with a regulator the port are
now a platform device in the device tree case.

There was only one driver which used directly the regulator field of
the ahci_host_priv structure. To preserve the bisectability the change
in the ahci_imx driver was done in the same patch.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/ata/ahci.h             |   2 +-
 drivers/ata/ahci_imx.c         |  14 +--
 drivers/ata/libahci_platform.c | 230 +++++++++++++++++++++++++++++------------
 include/linux/ahci_platform.h  |   2 +
 4 files changed, 173 insertions(+), 75 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 40f0e34f17af..275358ae0b3f 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -333,7 +333,7 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
-	struct regulator	*target_pwr;	/* Optional */
+	struct regulator	**target_pwrs;	/* Optional */
 	/*
 	 * If platform uses PHYs. There is a 1:1 relation between the port number and
 	 * the PHY position in this array.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 35d51c59a370..41632e57d46f 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 	if (imxpriv->no_device)
 		return 0;
 
-	if (hpriv->target_pwr) {
-		ret = regulator_enable(hpriv->target_pwr);
-		if (ret)
-			return ret;
-	}
+	ret = ahci_platform_enable_regulators(hpriv);
+	if (ret)
+		return ret;
 
 	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
 	if (ret < 0)
@@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 disable_clk:
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 
 	return ret;
 }
@@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
 
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 
 static void ahci_imx_error_handler(struct ata_port *ap)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index a147aaadca85..73a086664ee7 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -24,6 +24,7 @@
 #include <linux/ahci_platform.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_platform.h>
 #include "ahci.h"
 
 static void ahci_host_stop(struct ata_host *host);
@@ -138,6 +139,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
 EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
 
 /**
+ * ahci_platform_enable_regulators - Enable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function enables all the regulators found in
+ * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
+ * disables all the regulators already enabled in reverse order and
+ * returns an error.
+ *
+ * RETURNS:
+ * 0 on success otherwise a negative error code
+ */
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
+{
+	int rc, i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+
+		rc = regulator_enable(hpriv->target_pwrs[i]);
+		if (rc)
+			goto disable_target_pwrs;
+	}
+
+	return 0;
+
+disable_target_pwrs:
+	while (--i >= 0)
+		if (hpriv->target_pwrs[i])
+			regulator_disable(hpriv->target_pwrs[i]);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
+
+/**
+ * ahci_platform_disable_regulators - Disable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function disables all regulators found in hpriv->target_pwrs.
+ */
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
+{
+	int i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+		regulator_disable(hpriv->target_pwrs[i]);
+	}
+}
+EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
+/**
  * ahci_platform_enable_resources - Enable platform resources
  * @hpriv: host private area to store config values
  *
@@ -157,11 +211,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 {
 	int rc;
 
-	if (hpriv->target_pwr) {
-		rc = regulator_enable(hpriv->target_pwr);
-		if (rc)
-			return rc;
-	}
+	rc = ahci_platform_enable_regulators(hpriv);
+	if (rc)
+		return rc;
 
 	rc = ahci_platform_enable_clks(hpriv);
 	if (rc)
@@ -177,8 +229,8 @@ disable_clks:
 	ahci_platform_disable_clks(hpriv);
 
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
@@ -199,8 +251,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
 
 	ahci_platform_disable_clks(hpriv);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
 
@@ -216,6 +267,68 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
 
 	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
 		clk_put(hpriv->clks[c]);
+	/*
+	 * The regulators are tied to child node device and not to the
+	 * SATA device itself. So we can't use devm for automatically
+	 * releasing them. We have to do it manually here.
+	 */
+	for (c = 0; c < hpriv->nports; c++)
+		if (hpriv->target_pwrs && hpriv->target_pwrs[c])
+			regulator_put(hpriv->target_pwrs[c]);
+
+}
+
+static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev, struct device_node *node)
+{
+	int rc;
+
+	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
+
+	if (!IS_ERR(hpriv->phys[port]))
+		return 0;
+
+	rc = PTR_ERR(hpriv->phys[port]);
+	switch (rc) {
+	case -ENOSYS:
+		/* No PHY support. Check if PHY is required. */
+		if (of_find_property(node, "phys", NULL)) {
+			dev_err(dev,
+				"couldn't get PHY in node %s: ENOSYS\n",
+				node->name);
+			break;
+		}
+	case -ENODEV:
+		/* continue normally */
+		hpriv->phys[port] = NULL;
+		rc = 0;
+		break;
+
+	default:
+		dev_err(dev,
+			"couldn't get PHY in node %s: %d\n",
+			node->name, rc);
+
+		break;
+	}
+
+	return rc;
+}
+
+static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev)
+{
+	struct regulator *target_pwr;
+	int rc = 0;
+
+	target_pwr = regulator_get_optional(dev, "target");
+
+	if (!IS_ERR(target_pwr))
+		hpriv->target_pwrs[port] = target_pwr;
+	else
+		rc = PTR_ERR(target_pwr);
+
+	return rc;
 }
 
 /**
@@ -240,7 +353,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, enabled_ports = 0, rc = -ENOMEM;
+	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -261,14 +374,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		goto err_out;
 	}
 
-	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
-	if (IS_ERR(hpriv->target_pwr)) {
-		rc = PTR_ERR(hpriv->target_pwr);
-		if (rc == -EPROBE_DEFER)
-			goto err_out;
-		hpriv->target_pwr = NULL;
-	}
-
 	for (i = 0; i < AHCI_MAX_CLKS; i++) {
 		/*
 		 * For now we must use clk_get(dev, NULL) for the first clock,
@@ -290,19 +395,33 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
-	hpriv->nports = of_get_child_count(dev->of_node);
+	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
 
-	if (hpriv->nports) {
-		hpriv->phys = devm_kzalloc(dev,
-					   hpriv->nports * sizeof(*hpriv->phys),
-					   GFP_KERNEL);
-		if (!hpriv->phys) {
-			rc = -ENOMEM;
-			goto err_out;
-		}
+	/*
+	 * If no sub-node was found, we still need to set nports to
+	 * one in order to be able to use the
+	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
+	 */
+	if (!child_nodes)
+		hpriv->nports = 1;
 
+	sz = hpriv->nports * sizeof(*hpriv->phys);
+	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->phys) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
+	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->target_pwrs) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	if (child_nodes) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
+			struct platform_device *port_dev;
 
 			if (!of_device_is_available(child))
 				continue;
@@ -316,18 +435,23 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 				dev_warn(dev, "invalid port number %d\n", port);
 				continue;
 			}
-
 			mask_port_map |= BIT(port);
 
-			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
-			if (IS_ERR(hpriv->phys[port])) {
-				rc = PTR_ERR(hpriv->phys[port]);
-				dev_err(dev,
-					"couldn't get PHY in node %s: %d\n",
-					child->name, rc);
-				goto err_out;
+			of_platform_device_create(child, NULL, NULL);
+
+			port_dev = of_find_device_by_node(child);
+
+			if (port_dev) {
+				rc = ahci_platform_get_regulator(hpriv, port,
+								&port_dev->dev);
+				if (rc == -EPROBE_DEFER)
+					goto err_out;
 			}
 
+			rc = ahci_platform_get_phy(hpriv, port, dev, child);
+			if (rc)
+				goto err_out;
+
 			enabled_ports++;
 		}
 		if (!enabled_ports) {
@@ -343,38 +467,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
-		struct phy *phy = devm_phy_get(dev, "sata-phy");
-		if (!IS_ERR(phy)) {
-			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
-						   GFP_KERNEL);
-			if (!hpriv->phys) {
-				rc = -ENOMEM;
-				goto err_out;
-			}
-
-			hpriv->phys[0] = phy;
-			hpriv->nports = 1;
-		} else {
-			rc = PTR_ERR(phy);
-			switch (rc) {
-				case -ENOSYS:
-					/* No PHY support. Check if PHY is required. */
-					if (of_find_property(dev->of_node, "phys", NULL)) {
-						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
-						goto err_out;
-					}
-				case -ENODEV:
-					/* continue normally */
-					hpriv->phys = NULL;
-					break;
-
-				default:
-					goto err_out;
+		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
+		if (rc)
+			goto err_out;
 
-			}
-		}
+		rc = ahci_platform_get_regulator(hpriv, 0, dev);
+		if (rc == -EPROBE_DEFER)
+			goto err_out;
 	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 642d6ae4030c..f65b33809170 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -24,6 +24,8 @@ struct platform_device;
 
 int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
 int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 struct ahci_host_priv *ahci_platform_get_resources(
-- 
1.9.1

[PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Gregory CLEMENT]

Add the regulators to each SATA port.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
index 4df22bf91683..590b383db323 100644
--- a/arch/arm/boot/dts/armada-388-gp.dts
+++ b/arch/arm/boot/dts/armada-388-gp.dts
@@ -173,6 +173,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				sata0: sata-port@0 {
+					reg = <0>;
+					target-supply = <&reg_5v_sata0>;
+				};
+
+				sata1: sata-port@1 {
+					reg = <1>;
+					target-supply = <&reg_5v_sata1>;
+				};
 			};
 
 			sata@e0000 {
@@ -181,6 +191,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				sata2: sata-port@0 {
+					reg = <0>;
+					target-supply = <&reg_5v_sata2>;
+				};
+
+				sata3: sata-port@1 {
+					reg = <1>;
+					target-supply = <&reg_5v_sata3>;
+				};
 			};
 
 			sdhci@d8000 {
@@ -278,6 +298,112 @@
 		regulator-always-on;
 		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
 	};
+
+	reg_sata0: pwr-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata0";
+		enable-active-high;
+		regulator-always-on;
+
+	};
+
+	reg_5v_sata0: v5-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata0";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_12v_sata0: v12-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata0";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_sata1: pwr-sata1 {
+		regulator-name = "pwr_en_sata1";
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 3 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata1: v5-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata1";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_12v_sata1: v12-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata1";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_sata2: pwr-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata2";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 11 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata2: v5-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata2";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_12v_sata2: v12-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata2";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_sata3: pwr-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata3";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata3: v5-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata3";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
+
+	reg_12v_sata3: v12-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata3";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
 };
 
 &pinctrl {
-- 
1.9.1

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 15-01-15 15:09, Gregory CLEMENT wrote:
> Add the regulators to each SATA port.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 126 insertions(+)
>
> diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
> index 4df22bf91683..590b383db323 100644
> --- a/arch/arm/boot/dts/armada-388-gp.dts
> +++ b/arch/arm/boot/dts/armada-388-gp.dts
> @@ -173,6 +173,16 @@
>   				status = "okay";
>   				#address-cells = <1>;
>   				#size-cells = <0>;
> +
> +				sata0: sata-port@0 {
> +					reg = <0>;
> +					target-supply = <&reg_5v_sata0>;
> +				};
> +
> +				sata1: sata-port@1 {
> +					reg = <1>;
> +					target-supply = <&reg_5v_sata1>;
> +				};
>   			};
>
>   			sata@e0000 {
> @@ -181,6 +191,16 @@
>   				status = "okay";
>   				#address-cells = <1>;
>   				#size-cells = <0>;
> +
> +				sata2: sata-port@0 {
> +					reg = <0>;
> +					target-supply = <&reg_5v_sata2>;
> +				};
> +
> +				sata3: sata-port@1 {
> +					reg = <1>;
> +					target-supply = <&reg_5v_sata3>;
> +				};
>   			};
>
>   			sdhci@d8000 {
> @@ -278,6 +298,112 @@
>   		regulator-always-on;
>   		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
>   	};
> +
> +	reg_sata0: pwr-sata0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "pwr_en_sata0";
> +		enable-active-high;
> +		regulator-always-on;
> +
> +	};
> +
> +	reg_5v_sata0: v5-sata0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v5.0-sata0";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata0>;
> +	};
> +
> +	reg_12v_sata0: v12-sata0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v12.0-sata0";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata0>;
> +	};

AFAIK the separate v5 / 12v regulators you're creating here
are not used anywhere. So I guess there just here to
accurately / completely describe the power topology ?

> +
> +	reg_sata1: pwr-sata1 {
> +		regulator-name = "pwr_en_sata1";
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		enable-active-high;
> +		regulator-always-on;


The always on here seems to a bit weird, wasn't the
whole purpose of this patch set to teach ahci_platform
to turn it on as needed ?

You do probably want to put a regulator-boot-on here so
that disks do not get an unwanted powercycle (bad for
their lifetime) when the firmware has already turned on
the disk. Downside of using regulator-boot-on is that if
the power is not actually turned on no power-on-delay is
done, but we're not using a power on delay anyways.

The same goes for reg_sata2 & reg_sata3.

> +		gpio = <&expander0 3 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	reg_5v_sata1: v5-sata1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v5.0-sata1";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata1>;
> +	};
> +
> +	reg_12v_sata1: v12-sata1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v12.0-sata1";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata1>;
> +	};
> +
> +	reg_sata2: pwr-sata2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "pwr_en_sata2";
> +		enable-active-high;
> +		regulator-always-on;
> +		gpio = <&expander0 11 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	reg_5v_sata2: v5-sata2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v5.0-sata2";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata2>;
> +	};
> +
> +	reg_12v_sata2: v12-sata2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v12.0-sata2";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata2>;
> +	};
> +
> +	reg_sata3: pwr-sata3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "pwr_en_sata3";
> +		enable-active-high;
> +		regulator-always-on;
> +		gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	reg_5v_sata3: v5-sata3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v5.0-sata3";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata3>;
> +	};
> +
> +	reg_12v_sata3: v12-sata3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v12.0-sata3";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-always-on;
> +		vin-supply = <&reg_sata3>;
> +	};
>   };
>
>   &pinctrl {
>

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Gregory CLEMENT]

Hi Hans,

On 16/01/2015 09:17, Hans de Goede wrote:
> Hi,
> 
> On 15-01-15 15:09, Gregory CLEMENT wrote:
>> Add the regulators to each SATA port.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>   arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 126 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
>> index 4df22bf91683..590b383db323 100644
>> --- a/arch/arm/boot/dts/armada-388-gp.dts
>> +++ b/arch/arm/boot/dts/armada-388-gp.dts
>> @@ -173,6 +173,16 @@
>>   				status = "okay";
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>> +
>> +				sata0: sata-port@0 {
>> +					reg = <0>;
>> +					target-supply = <&reg_5v_sata0>;
>> +				};
>> +
>> +				sata1: sata-port@1 {
>> +					reg = <1>;
>> +					target-supply = <&reg_5v_sata1>;
>> +				};
>>   			};
>>
>>   			sata@e0000 {
>> @@ -181,6 +191,16 @@
>>   				status = "okay";
>>   				#address-cells = <1>;
>>   				#size-cells = <0>;
>> +
>> +				sata2: sata-port@0 {
>> +					reg = <0>;
>> +					target-supply = <&reg_5v_sata2>;
>> +				};
>> +
>> +				sata3: sata-port@1 {
>> +					reg = <1>;
>> +					target-supply = <&reg_5v_sata3>;
>> +				};
>>   			};
>>
>>   			sdhci@d8000 {
>> @@ -278,6 +298,112 @@
>>   		regulator-always-on;
>>   		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
>>   	};
>> +
>> +	reg_sata0: pwr-sata0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "pwr_en_sata0";
>> +		enable-active-high;
>> +		regulator-always-on;
>> +
>> +	};
>> +
>> +	reg_5v_sata0: v5-sata0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v5.0-sata0";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata0>;
>> +	};
>> +
>> +	reg_12v_sata0: v12-sata0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v12.0-sata0";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata0>;
>> +	};
> 
> AFAIK the separate v5 / 12v regulators you're creating here
> are not used anywhere. So I guess there just here to
> accurately / completely describe the power topology ?

Yes it was the point.

> 
>> +
>> +	reg_sata1: pwr-sata1 {
>> +		regulator-name = "pwr_en_sata1";
>> +		compatible = "regulator-fixed";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		enable-active-high;
>> +		regulator-always-on;
> 
> 
> The always on here seems to a bit weird, wasn't the
> whole purpose of this patch set to teach ahci_platform
> to turn it on as needed ?

Maybe I misunderstood the regulator binding, but I thought
that (once the suspend will be available on this platform) I
could use:

regulator-state-mem {
			regulator-off-in-suspend;
		};

> 
> You do probably want to put a regulator-boot-on here so
> that disks do not get an unwanted powercycle (bad for
> their lifetime) when the firmware has already turned on
> the disk. Downside of using regulator-boot-on is that if
> the power is not actually turned on no power-on-delay is

That's why I didn't use it

> done, but we're not using a power on delay anyways.

But if regulator-always-on prevent to switch it off in
suspend then yes using regulator-boot-on is better.

Thanks,

Gregory

> 
> The same goes for reg_sata2 & reg_sata3.
> 
>> +		gpio = <&expander0 3 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	reg_5v_sata1: v5-sata1 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v5.0-sata1";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata1>;
>> +	};
>> +
>> +	reg_12v_sata1: v12-sata1 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v12.0-sata1";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata1>;
>> +	};
>> +
>> +	reg_sata2: pwr-sata2 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "pwr_en_sata2";
>> +		enable-active-high;
>> +		regulator-always-on;
>> +		gpio = <&expander0 11 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	reg_5v_sata2: v5-sata2 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v5.0-sata2";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata2>;
>> +	};
>> +
>> +	reg_12v_sata2: v12-sata2 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v12.0-sata2";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata2>;
>> +	};
>> +
>> +	reg_sata3: pwr-sata3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "pwr_en_sata3";
>> +		enable-active-high;
>> +		regulator-always-on;
>> +		gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	reg_5v_sata3: v5-sata3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v5.0-sata3";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata3>;
>> +	};
>> +
>> +	reg_12v_sata3: v12-sata3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "v12.0-sata3";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		regulator-always-on;
>> +		vin-supply = <&reg_sata3>;
>> +	};
>>   };
>>
>>   &pinctrl {
>>
> 
> Regards,
> 
> Hans
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 16-01-15 10:27, Gregory CLEMENT wrote:
> Hi Hans,
>
> On 16/01/2015 09:17, Hans de Goede wrote:
>> Hi,
>>
>> On 15-01-15 15:09, Gregory CLEMENT wrote:
>>> Add the regulators to each SATA port.
>>>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> ---
>>>    arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 126 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
>>> index 4df22bf91683..590b383db323 100644
>>> --- a/arch/arm/boot/dts/armada-388-gp.dts
>>> +++ b/arch/arm/boot/dts/armada-388-gp.dts
>>> @@ -173,6 +173,16 @@
>>>    				status = "okay";
>>>    				#address-cells = <1>;
>>>    				#size-cells = <0>;
>>> +
>>> +				sata0: sata-port@0 {
>>> +					reg = <0>;
>>> +					target-supply = <&reg_5v_sata0>;
>>> +				};
>>> +
>>> +				sata1: sata-port@1 {
>>> +					reg = <1>;
>>> +					target-supply = <&reg_5v_sata1>;
>>> +				};
>>>    			};
>>>
>>>    			sata@e0000 {
>>> @@ -181,6 +191,16 @@
>>>    				status = "okay";
>>>    				#address-cells = <1>;
>>>    				#size-cells = <0>;
>>> +
>>> +				sata2: sata-port@0 {
>>> +					reg = <0>;
>>> +					target-supply = <&reg_5v_sata2>;
>>> +				};
>>> +
>>> +				sata3: sata-port@1 {
>>> +					reg = <1>;
>>> +					target-supply = <&reg_5v_sata3>;
>>> +				};
>>>    			};
>>>
>>>    			sdhci@d8000 {
>>> @@ -278,6 +298,112 @@
>>>    		regulator-always-on;
>>>    		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
>>>    	};
>>> +
>>> +	reg_sata0: pwr-sata0 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "pwr_en_sata0";
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>> +
>>> +	};
>>> +
>>> +	reg_5v_sata0: v5-sata0 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "v5.0-sata0";
>>> +		regulator-min-microvolt = <5000000>;
>>> +		regulator-max-microvolt = <5000000>;
>>> +		regulator-always-on;
>>> +		vin-supply = <&reg_sata0>;
>>> +	};
>>> +
>>> +	reg_12v_sata0: v12-sata0 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "v12.0-sata0";
>>> +		regulator-min-microvolt = <12000000>;
>>> +		regulator-max-microvolt = <12000000>;
>>> +		regulator-always-on;
>>> +		vin-supply = <&reg_sata0>;
>>> +	};
>>
>> AFAIK the separate v5 / 12v regulators you're creating here
>> are not used anywhere. So I guess there just here to
>> accurately / completely describe the power topology ?
>
> Yes it was the point.

Ok.

>>> +
>>> +	reg_sata1: pwr-sata1 {
>>> +		regulator-name = "pwr_en_sata1";
>>> +		compatible = "regulator-fixed";
>>> +		regulator-min-microvolt = <12000000>;
>>> +		regulator-max-microvolt = <12000000>;
>>> +		enable-active-high;
>>> +		regulator-always-on;
>>
>>
>> The always on here seems to a bit weird, wasn't the
>> whole purpose of this patch set to teach ahci_platform
>> to turn it on as needed ?
>
> Maybe I misunderstood the regulator binding, but I thought
> that (once the suspend will be available on this platform) I
> could use:
>
> regulator-state-mem {
> 			regulator-off-in-suspend;
> 		};
>
>>
>> You do probably want to put a regulator-boot-on here so
>> that disks do not get an unwanted powercycle (bad for
>> their lifetime) when the firmware has already turned on
>> the disk. Downside of using regulator-boot-on is that if
>> the power is not actually turned on no power-on-delay is
>
> That's why I didn't use it
>
>> done, but we're not using a power on delay anyways.
>
> But if regulator-always-on prevent to switch it off in
> suspend then yes using regulator-boot-on is better.

AFAIK regulator-always-on means exactly that and thus likely
is not what you want. As for using regulator-off-in-suspend
that is not necessary as the suspend method for the acpi
driver will already turn it off.

Note that you can test this today by doing (IIRC):

echo devices > /sys/power/pm_test
echo mem > /sys/power/state

This is how I tested ahci_platform suspend handling on
the freescale imx processor on the wandboard.

It is probably a good idea to use regulator-boot-on and
then test things this way, and if that works use
regulator-boot-on.

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On Fri, Jan 16, 2015 at 11:10:18AM +0100, Hans de Goede wrote:
> On 16-01-15 10:27, Gregory CLEMENT wrote:

> >>>+	reg_sata0: pwr-sata0 {
> >>>+		compatible = "regulator-fixed";
> >>>+		regulator-name = "pwr_en_sata0";
> >>>+		enable-active-high;
> >>>+		regulator-always-on;

> >>done, but we're not using a power on delay anyways.

> >But if regulator-always-on prevent to switch it off in
> >suspend then yes using regulator-boot-on is better.

> AFAIK regulator-always-on means exactly that and thus likely
> is not what you want. As for using regulator-off-in-suspend
> that is not necessary as the suspend method for the acpi
> driver will already turn it off.

regulator-always-on is a bit fuzzy for suspend, if the regulator has
suspend control it'll kick in - it's really about the Linux refcounting
while it's running.  What's more concerning here is that the quick
sample of the regulators flagged as always on like the above that I
looked at in the patch don't seem to have any enable control in the DT
so this will have absolutely no effect.

> It is probably a good idea to use regulator-boot-on and
> then test things this way, and if that works use
> regulator-boot-on.

No, it's unlikely that boot-on makes sense here - it's there for cases
where we can't read back the hardware state at power on.  Generally
drivers should work regardless of the initial state of the regulator
(and modular drivers will actually break if they try to rely on boot-on
since we clean up unused regulators at boot).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Gregory CLEMENT]

Hi Mark and Hans,

On 16/01/2015 13:37, Mark Brown wrote:
> On Fri, Jan 16, 2015 at 11:10:18AM +0100, Hans de Goede wrote:
>> On 16-01-15 10:27, Gregory CLEMENT wrote:
> 
>>>>> +	reg_sata0: pwr-sata0 {
>>>>> +		compatible = "regulator-fixed";
>>>>> +		regulator-name = "pwr_en_sata0";
>>>>> +		enable-active-high;
>>>>> +		regulator-always-on;
> 
>>>> done, but we're not using a power on delay anyways.
> 
>>> But if regulator-always-on prevent to switch it off in
>>> suspend then yes using regulator-boot-on is better.
> 
>> AFAIK regulator-always-on means exactly that and thus likely
>> is not what you want. As for using regulator-off-in-suspend
>> that is not necessary as the suspend method for the acpi
>> driver will already turn it off.
> 
> regulator-always-on is a bit fuzzy for suspend, if the regulator has
> suspend control it'll kick in - it's really about the Linux refcounting
> while it's running.  What's more concerning here is that the quick
> sample of the regulators flagged as always on like the above that I
> looked at in the patch don't seem to have any enable control in the DT
> so this will have absolutely no effect.

Actually the reg_sata[0-4] are controlled by gpio, so there is a mean
to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4]
they depend on their respective reg_sata and I just propagated the
regulator-always-on, this was maybe a mistake.

> 
>> It is probably a good idea to use regulator-boot-on and
>> then test things this way, and if that works use
>> regulator-boot-on.
> 
> No, it's unlikely that boot-on makes sense here - it's there for cases
> where we can't read back the hardware state at power on.  Generally
> drivers should work regardless of the initial state of the regulator
> (and modular drivers will actually break if they try to rely on boot-on
> since we clean up unused regulators at boot).

As pointed by Hans my concern here was be sure that during boot the disk
are not power off. In this case which property would be accurate?


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Fri, Jan 16, 2015 at 03:27:00PM +0100, Gregory CLEMENT wrote:
> On 16/01/2015 13:37, Mark Brown wrote:

> > regulator-always-on is a bit fuzzy for suspend, if the regulator has
> > suspend control it'll kick in - it's really about the Linux refcounting
> > while it's running.  What's more concerning here is that the quick
> > sample of the regulators flagged as always on like the above that I
> > looked at in the patch don't seem to have any enable control in the DT
> > so this will have absolutely no effect.

> Actually the reg_sata[0-4] are controlled by gpio, so there is a mean
> to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4]
> they depend on their respective reg_sata and I just propagated the
> regulator-always-on, this was maybe a mistake.

It certainly makes everything confusing if you have control related
stuff on regulators that are not directly controllable.

> >> It is probably a good idea to use regulator-boot-on and
> >> then test things this way, and if that works use
> >> regulator-boot-on.

> > No, it's unlikely that boot-on makes sense here - it's there for cases
> > where we can't read back the hardware state at power on.  Generally
> > drivers should work regardless of the initial state of the regulator
> > (and modular drivers will actually break if they try to rely on boot-on
> > since we clean up unused regulators at boot).

> As pointed by Hans my concern here was be sure that during boot the disk
> are not power off. In this case which property would be accurate?

None, the core won't do anything with the regulator until the end of
init anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 16-01-15 16:34, Mark Brown wrote:
> On Fri, Jan 16, 2015 at 03:27:00PM +0100, Gregory CLEMENT wrote:
>> On 16/01/2015 13:37, Mark Brown wrote:
>
>>> regulator-always-on is a bit fuzzy for suspend, if the regulator has
>>> suspend control it'll kick in - it's really about the Linux refcounting
>>> while it's running.  What's more concerning here is that the quick
>>> sample of the regulators flagged as always on like the above that I
>>> looked at in the patch don't seem to have any enable control in the DT
>>> so this will have absolutely no effect.
>
>> Actually the reg_sata[0-4] are controlled by gpio, so there is a mean
>> to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4]
>> they depend on their respective reg_sata and I just propagated the
>> regulator-always-on, this was maybe a mistake.
>
> It certainly makes everything confusing if you have control related
> stuff on regulators that are not directly controllable.
>
>>>> It is probably a good idea to use regulator-boot-on and
>>>> then test things this way, and if that works use
>>>> regulator-boot-on.
>
>>> No, it's unlikely that boot-on makes sense here - it's there for cases
>>> where we can't read back the hardware state at power on.  Generally
>>> drivers should work regardless of the initial state of the regulator
>>> (and modular drivers will actually break if they try to rely on boot-on
>>> since we clean up unused regulators at boot).
>
>> As pointed by Hans my concern here was be sure that during boot the disk
>> are not power off. In this case which property would be accurate?
>
> None, the core won't do anything with the regulator until the end of
> init anyway.

That us simply not true, see my other mail gpio enabled regulators will
be turned off *at register time* unless they have regulator-boot-on set.

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Fri, Jan 16, 2015 at 08:13:36PM +0100, Hans de Goede wrote:
> On 16-01-15 16:34, Mark Brown wrote:

> >>As pointed by Hans my concern here was be sure that during boot the disk
> >>are not power off. In this case which property would be accurate?

> >None, the core won't do anything with the regulator until the end of
> >init anyway.

> That us simply not true, see my other mail gpio enabled regulators will
> be turned off *at register time* unless they have regulator-boot-on set.

That's not the core, that's the specific driver (which I didn't know
this board happened to be using).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 16-01-15 13:37, Mark Brown wrote:
> On Fri, Jan 16, 2015 at 11:10:18AM +0100, Hans de Goede wrote:
>> On 16-01-15 10:27, Gregory CLEMENT wrote:
>
>>>>> +	reg_sata0: pwr-sata0 {
>>>>> +		compatible = "regulator-fixed";
>>>>> +		regulator-name = "pwr_en_sata0";
>>>>> +		enable-active-high;
>>>>> +		regulator-always-on;
>
>>>> done, but we're not using a power on delay anyways.
>
>>> But if regulator-always-on prevent to switch it off in
>>> suspend then yes using regulator-boot-on is better.
>
>> AFAIK regulator-always-on means exactly that and thus likely
>> is not what you want. As for using regulator-off-in-suspend
>> that is not necessary as the suspend method for the acpi
>> driver will already turn it off.
>
> regulator-always-on is a bit fuzzy for suspend, if the regulator has
> suspend control it'll kick in - it's really about the Linux refcounting
> while it's running.  What's more concerning here is that the quick
> sample of the regulators flagged as always on like the above that I
> looked at in the patch don't seem to have any enable control in the DT
> so this will have absolutely no effect.
>
>> It is probably a good idea to use regulator-boot-on and
>> then test things this way, and if that works use
>> regulator-boot-on.
>
> No, it's unlikely that boot-on makes sense here - it's there for cases
> where we can't read back the hardware state at power on.

Which we cannot here since we've a gpio driven regulator, with the pin
in output mode, so we cannot read it back. Or at least the current kernel
implementation relies on regulator-boot-on rather then reading the
state back from the hardware.

If we do not specify regulator-boot-on then drivers/regulator/fixed.c :
of_get_fixed_voltage_config() will set the cfg.ena_gpio_flags
GPIOF_OUT_INIT_HIGH / GPIOF_OUT_INIT_LOW flags such that the regulator
will get turned off when registered (*) and then it will get turned back
on when the ahci driver loads causing the disk to powercycle while
spinning seriously deteriorating its live time, so regulator nodes
for supplies which power spinning disks definitely need
regulator-boot-on.

An alternative would be using regulator-always-on, but using
regulator-always-on here is wrong because it removes all of control from
the driver, making e.g. runtime pm for unused disks or empty drive-bays
impossible.

*) It will turn it off as soon as it requests the gpio.

> Generally
> drivers should work regardless of the initial state of the regulator
> (and modular drivers will actually break if they try to rely on boot-on
> since we clean up unused regulators at boot).

This is not about drivers, the driver will work fine, the problem is
that power-cycling disks which have already been spun-up by the bootloader
is very bad for those disks.

While we're discussing this I would also like to advocate to change the
behavior for regulator-boot-on to be the same as always-on when it comes
to regulator_init_complete(), iow regulator-boot-on regulators should
not be touched by regulator_init_complete(). It makes no sense to have
different behavior for build in versus module drivers here.

For build-in the regulator is left on (or turned on even) as soon as the
regulator registers, and then stays that way until explicitly turned off
by a driver,

I believe we should still leave these on once userspace starts running,
there is a reason they are marked as regulator-boot-on and the fact that
we're ready to start running userspace does not take away that reason, to
me regulator-boot-on means "do not turn off until explicitly told so by
a driver which (hopefully) knows wtf it is doing".

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

On Fri, Jan 16, 2015 at 08:12:28PM +0100, Hans de Goede wrote:
> On 16-01-15 13:37, Mark Brown wrote:

> >>It is probably a good idea to use regulator-boot-on and
> >>then test things this way, and if that works use
> >>regulator-boot-on.

> >No, it's unlikely that boot-on makes sense here - it's there for cases
> >where we can't read back the hardware state at power on.

> Which we cannot here since we've a gpio driven regulator, with the pin
> in output mode, so we cannot read it back. Or at least the current kernel
> implementation relies on regulator-boot-on rather then reading the
> state back from the hardware.

Right, this always seems like a problem with the GPIO API - not being
able to read the state back is a total pain for boot handover, we should
really have a way of reading back the current output state.  This is one
of the few cases where boot-on can be useful, though ideally it'd be set
by the bootloader rather than as a static thing since otherwise you have
the opposite problem in the case where the disk isn't being used at the
current time.

> While we're discussing this I would also like to advocate to change the
> behavior for regulator-boot-on to be the same as always-on when it comes
> to regulator_init_complete(), iow regulator-boot-on regulators should
> not be touched by regulator_init_complete(). It makes no sense to have
> different behavior for build in versus module drivers here.

Like I say boot-on is *only* intended to support bootstrapping, if you
want the regulator to be always on then mark it as always on.  If you
want some way of marking the end of userspace init so we can defer
cleanup of unused resources then go do that - it's a change I've
advocated before, it'd help with modular drivers in general.

> I believe we should still leave these on once userspace starts running,
> there is a reason they are marked as regulator-boot-on and the fact that
> we're ready to start running userspace does not take away that reason, to
> me regulator-boot-on means "do not turn off until explicitly told so by
> a driver which (hopefully) knows wtf it is doing".

No, that's not it at all - it just means that either the hardware didn't
support readback or we're working around our own limitations in not
being capable of doing that.  Remember, regulators are reference counted
so with a few exceptions drivers aren't saying "turn this off now"
they're saying "this device doesn't need this regulator right now, turn
it off if you like" and there's no guarantee that a driver will ever be
loaded in the first place.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 16-01-15 21:25, Mark Brown wrote:
> On Fri, Jan 16, 2015 at 08:12:28PM +0100, Hans de Goede wrote:
>> On 16-01-15 13:37, Mark Brown wrote:
>
>>>> It is probably a good idea to use regulator-boot-on and
>>>> then test things this way, and if that works use
>>>> regulator-boot-on.
>
>>> No, it's unlikely that boot-on makes sense here - it's there for cases
>>> where we can't read back the hardware state at power on.
>
>> Which we cannot here since we've a gpio driven regulator, with the pin
>> in output mode, so we cannot read it back. Or at least the current kernel
>> implementation relies on regulator-boot-on rather then reading the
>> state back from the hardware.
>
> Right, this always seems like a problem with the GPIO API - not being
> able to read the state back is a total pain for boot handover, we should
> really have a way of reading back the current output state.  This is one
> of the few cases where boot-on can be useful, though ideally it'd be set
> by the bootloader rather than as a static thing since otherwise you have
> the opposite problem in the case where the disk isn't being used at the
> current time.
>
>> While we're discussing this I would also like to advocate to change the
>> behavior for regulator-boot-on to be the same as always-on when it comes
>> to regulator_init_complete(), iow regulator-boot-on regulators should
>> not be touched by regulator_init_complete(). It makes no sense to have
>> different behavior for build in versus module drivers here.
>
> Like I say boot-on is *only* intended to support bootstrapping, if you
> want the regulator to be always on then mark it as always on.  If you
> want some way of marking the end of userspace init so we can defer
> cleanup of unused resources then go do that - it's a change I've
> advocated before, it'd help with modular drivers in general.

The problem is there is no clear end of userspace init in modern event
driven systems, I think it would be much clearer to simply be consistent
and leave these regulators on, rather then turn them of $random time,
because as I've advocated before (see the simplefb thread) there is
no simple magic moment when it is right to turn things off, so doing
it when userspace starts is more or less as good as any moment, and if
it turns out that that is not a good moment, then maybe we need to
re-think the turning off in general.

With that said, since this is a re-occuring theme I've send a mail to
the systemd-devel list with you (Mark) in the CC, as well as the ide,
arm and devicetree lists. In this mail I'm asking the udev developers
if they would be willing to implement some sort of call into the kernel
to tell the kernel that udev is done with loading modules. This will
only cover devices which are already enumerated at late_init_call time,
if we've some slow scanning bus still enumerating stuff in a thread
(e.g. usb, scsi) then all bets are of, which is why I'm not enthusiastic
about this solution TBH.

A simpler solution for the use-case at hand may be to simply disallow
building of the ahci_platform driver as a module.

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

On Sat, Jan 17, 2015 at 09:48:57AM +0100, Hans de Goede wrote:

> and leave these regulators on, rather then turn them of $random time,
> because as I've advocated before (see the simplefb thread) there is
> no simple magic moment when it is right to turn things off, so doing
> it when userspace starts is more or less as good as any moment, and if
> it turns out that that is not a good moment, then maybe we need to
> re-think the turning off in general.

Following your argument to the logical conclusion means we can never
turn any regualtor off - we always have the risk that there's another
shared user which is going to get a power bounce if we power down.  More
directly we'll also get people complaining that we're burning power
pointlessly on their systems for devices they've not even got drivers
enabled for.  This powering down is something there's been user demand
for.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 17-01-15 14:14, Mark Brown wrote:
> On Sat, Jan 17, 2015 at 09:48:57AM +0100, Hans de Goede wrote:
>
>> and leave these regulators on, rather then turn them of $random time,
>> because as I've advocated before (see the simplefb thread) there is
>> no simple magic moment when it is right to turn things off, so doing
>> it when userspace starts is more or less as good as any moment, and if
>> it turns out that that is not a good moment, then maybe we need to
>> re-think the turning off in general.
>
> Following your argument to the logical conclusion means we can never
> turn any regualtor off - we always have the risk that there's another
> shared user which is going to get a power bounce if we power down.  More
> directly we'll also get people complaining that we're burning power
> pointlessly on their systems for devices they've not even got drivers
> enabled for.  This powering down is something there's been user demand
> for.

Right, note I'm only advocating to not turn off regulators marked as
regulator-boot-on. I would expect any regulator to have such a
marking to have at least one user with an actual driver. If people decide
to not build that driver, and then complain we can simply tell them to
build the driver ...

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On Sat, Jan 17, 2015 at 03:28:39PM +0100, Hans de Goede wrote:
> On 17-01-15 14:14, Mark Brown wrote:

> >Following your argument to the logical conclusion means we can never
> >turn any regualtor off - we always have the risk that there's another
> >shared user which is going to get a power bounce if we power down.  More
> >directly we'll also get people complaining that we're burning power
> >pointlessly on their systems for devices they've not even got drivers
> >enabled for.  This powering down is something there's been user demand
> >for.

> Right, note I'm only advocating to not turn off regulators marked as
> regulator-boot-on. I would expect any regulator to have such a
> marking to have at least one user with an actual driver. If people decide
> to not build that driver, and then complain we can simply tell them to
> build the driver ...

Right, but that's not what regulator-boot-on actually means (and I'm not
sure why you would think it would TBH) so this will disrupt existing
users who are expecting the current behaviour.  We could try adding a
new property but it doesn't feel very idiomatic for DT which isn't very
nice.

Telling people not to build the driver doesn't in general work any
better than telling them to build it in I fear, it seems like it's
essentially just shuffling things around so people have to change their
kernel config in a different way to avoid issues.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Hans de Goede]

Hi,

On 18-01-15 13:35, Mark Brown wrote:
> On Sat, Jan 17, 2015 at 03:28:39PM +0100, Hans de Goede wrote:
>> On 17-01-15 14:14, Mark Brown wrote:
>
>>> Following your argument to the logical conclusion means we can never
>>> turn any regualtor off - we always have the risk that there's another
>>> shared user which is going to get a power bounce if we power down.  More
>>> directly we'll also get people complaining that we're burning power
>>> pointlessly on their systems for devices they've not even got drivers
>>> enabled for.  This powering down is something there's been user demand
>>> for.
>
>> Right, note I'm only advocating to not turn off regulators marked as
>> regulator-boot-on. I would expect any regulator to have such a
>> marking to have at least one user with an actual driver. If people decide
>> to not build that driver, and then complain we can simply tell them to
>> build the driver ...
>
> Right, but that's not what regulator-boot-on actually means (and I'm not
> sure why you would think it would TBH)

Well, the meaning of regulator-boot-on is not clearly defined really, to
begin we need with fixing that, currently all the bindings file says is:

- regulator-boot-on: bootloader/firmware enabled regulator

One could easily argue that the bootloader likely has a good reason to turn
the regulator on, and that unless there is a specific driver which claims
the regulator and thus knows what to do with it it is best left alone ...

> so this will disrupt existing
> users who are expecting the current behaviour.  We could try adding a
> new property but it doesn't feel very idiomatic for DT which isn't very
> nice.
>
> Telling people not to build the driver doesn't in general work any
> better than telling them to build it in I fear, it seems like it's
> essentially just shuffling things around so people have to change their
> kernel config in a different way to avoid issues.

Regards,

Hans

Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On Sun, Jan 18, 2015 at 04:29:02PM +0100, Hans de Goede wrote:
> On 18-01-15 13:35, Mark Brown wrote:

> >Right, but that's not what regulator-boot-on actually means (and I'm not
> >sure why you would think it would TBH)

> Well, the meaning of regulator-boot-on is not clearly defined really, to
> begin we need with fixing that, currently all the bindings file says is:

> - regulator-boot-on: bootloader/firmware enabled regulator

If that meant anything about what to do with the regulator at runtime it
would say so - it means exactly what it says.

> One could easily argue that the bootloader likely has a good reason to turn
> the regulator on, and that unless there is a specific driver which claims
> the regulator and thus knows what to do with it it is best left alone ...

That's an excessively big stretch; it doesn't reflect the reality that
the bootloader is often just leaving the settings it found on initial
power up alone nor the general quality of implementation concerns one
often sees with bootloaders.  A big use case for this feature is that
there is a fairly large class of systems where the bootloader can't be
relied on.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 0/4] ata: libahci: Allow using a regulator for each port

[Hans de Goede]

Hi,

On 15-01-15 15:09, Gregory CLEMENT wrote:
> The current implementation of the libahci allows using one PHY per
> port but we still have one single regulator for the whole
> controller. This series adds the support of multiple regulators.
>
> This is the forth version of the series.
>
> The improvement of this version is the use of
> ahci_platform_put_resources to put the reference to the regulators.
>
> Thanks,
>
> Grégory

Thanks, patches 1 - 3 look good and are:

Acked-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Tejun, can you please queue up 1 - 3 ?

I still have some remarks wrt patch 4 (*), and it should probably go
upstream through another tree anyways.

Regards,

Hans


*) I didn't look closely at patch 4 as it seemed trivial, until now
that is ...



>
> Changelog:
>
>   v3 -> v4
> - Moved putting the reference to the regulators into the
>    ahci_platform_put_resources function.
> - Tested the port_dev pointer before dereferencing.
>
>   v2 -> v3:
> - put back the regulator inside the sub-node ports
> - made the ports platform devices when the device tree is used
> - released the regulator in case of error in the probe function
>
>   v1 -> v2:
> - Kept the case when no child node are present under the ahci node
> - Fix the test done under the label disable_target_pwrs
> - No more use an of_ version of the regulator framework and instead
>    associate each regulator of a port with an unique name.
> - Added the acked-by on the clean-up patch
>
> Gregory CLEMENT (4):
>    ata: libahci: Clean-up the ahci_platform_en/disable_phys functions
>    Documentation: bindings: Add the regulator property to the sub-nodes
>      AHCI bindings
>    ata: libahci: Allow using multiple regulators
>    ARM: mvebu: Armada 385 GP: Add regulators to the SATA port
>
>   .../devicetree/bindings/ata/ahci-platform.txt      |   9 +-
>   arch/arm/boot/dts/armada-388-gp.dts                | 126 +++++++++++
>   drivers/ata/ahci.h                                 |   2 +-
>   drivers/ata/ahci_imx.c                             |  14 +-
>   drivers/ata/libahci_platform.c                     | 236 ++++++++++++++-------
>   include/linux/ahci_platform.h                      |   2 +
>   6 files changed, 305 insertions(+), 84 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/4] ata: libahci: Allow using a regulator for each port

[Tejun Heo]

On Fri, Jan 16, 2015 at 08:58:18AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 15-01-15 15:09, Gregory CLEMENT wrote:
> >The current implementation of the libahci allows using one PHY per
> >port but we still have one single regulator for the whole
> >controller. This series adds the support of multiple regulators.
> >
> >This is the forth version of the series.
> >
> >The improvement of this version is the use of
> >ahci_platform_put_resources to put the reference to the regulators.
> >
> >Thanks,
> >
> >Grégory
> 
> Thanks, patches 1 - 3 look good and are:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Tejun, can you please queue up 1 - 3 ?

Applied 1-3 to libata/for-3.20.

Thanks.

-- 
tejun

Re: [PATCH v4 0/4] ata: libahci: Allow using a regulator for each port

[Andrew Lunn]

On Mon, Jan 19, 2015 at 09:54:13AM -0500, Tejun Heo wrote:
> On Fri, Jan 16, 2015 at 08:58:18AM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 15-01-15 15:09, Gregory CLEMENT wrote:
> > >The current implementation of the libahci allows using one PHY per
> > >port but we still have one single regulator for the whole
> > >controller. This series adds the support of multiple regulators.
> > >
> > >This is the forth version of the series.
> > >
> > >The improvement of this version is the use of
> > >ahci_platform_put_resources to put the reference to the regulators.
> > >
> > >Thanks,
> > >
> > >Grégory
> > 
> > Thanks, patches 1 - 3 look good and are:
> > 
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Tejun, can you please queue up 1 - 3 ?
> 
> Applied 1-3 to libata/for-3.20.

Thanks

I will add 4/4 to mvebu/dt.

  Andrew