From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port Date: Fri, 16 Jan 2015 11:10:18 +0100 Message-ID: <54B8E38A.6020709@redhat.com> References: <1421330978-9694-1-git-send-email-gregory.clement@free-electrons.com> <1421330978-9694-5-git-send-email-gregory.clement@free-electrons.com> <54B8C933.7020502@redhat.com> <54B8D97B.3090908@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54B8D97B.3090908@free-electrons.com> Sender: linux-ide-owner@vger.kernel.org To: Gregory CLEMENT , Tejun Heo , linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org Cc: =?windows-1252?Q?Antoine_T=E9nart?= , Liam Girdwood , Mark Brown , Thomas Petazzoni , Ezequiel Garcia , Maxime Ripard , Boris BREZILLON , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , linux-arm-kernel@lists.infradead.org, Lior Amsalem , Tawfik Bayouk , Nadav Haklai , Mark Rutland , devicetree@vger.kernel.org List-Id: devicetree@vger.kernel.org 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 >>> --- >>> 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 = <®_5v_sata0>; >>> + }; >>> + >>> + sata1: sata-port@1 { >>> + reg = <1>; >>> + target-supply = <®_5v_sata1>; >>> + }; >>> }; >>> >>> sata@e0000 { >>> @@ -181,6 +191,16 @@ >>> status = "okay"; >>> #address-cells = <1>; >>> #size-cells = <0>; >>> + >>> + sata2: sata-port@0 { >>> + reg = <0>; >>> + target-supply = <®_5v_sata2>; >>> + }; >>> + >>> + sata3: sata-port@1 { >>> + reg = <1>; >>> + target-supply = <®_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 = <®_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 = <®_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