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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <54B8E38A.6020709@redhat.com>
Date:	Fri, 16 Jan 2015 11:10:18 +0100
From:	Hans de Goede <hdegoede@...hat.com>
To:	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	Tejun Heo <tj@...nel.org>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
CC:	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Lior Amsalem <alior@...vell.com>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Mark Rutland <mark.rutland@....com>, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the
 SATA port

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@...e-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@...00 {
>>> @@ -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@...00 {
>>> @@ -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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/