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

From: Hans de Goede
Date: Fri Jan 16 2015 - 05:11:30 EST

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@xxxxxxxxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/