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

[Hans de Goede <hdegoede@redhat.com>]

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