Re: [RFC PATCH] ata: ahci: Enable DEVSLP by default on x86 modern standby platform

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

Hi,

On 03-07-18 10:57, Hans de Goede wrote:
> Hi,
> 
> On 03-07-18 00:08, Srinivas Pandruvada wrote:
>> Hi Hans,
>>
>> On Mon, 2018-07-02 at 23:27 +0200, Hans de Goede wrote:
> 
> <snip>
> 
>>>>> SATA IP block doesn't get turned off till SATA is in DEVSLP mode.
>>>>> Here
>>>>> user has to either use scsi-host sysfs or tools like powertop to
>>>>> set
>>>>> the sata-host link_power_management_policy to min_power.
>>>>>
>>>>> This change sets by default link power management policy to
>>>>> min_power
>>>>> for some platforms.  To avoid regressions, the following
>>>>> conditions
>>>>> are used:
>>>>> - The kernel config is already set to use med_power_with_dipm or
>>>>> deeper
>>>>> - System is a modern standby system using ACPI low power idle
>>>>> flag
>>>>> - The platform is not blacklisted for Suspend to Idle and suspend
>>>>> to idle is used instead of S3
>>>>> This combination will make sure that systems are fairly recent
>>>>> and
>>>>> since getting shipped with modern standby standby, the DEVSLP
>>>>> function
>>>>> is already validated.
>>>>>
>>>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.int
>>>>> el.com>
>>>>
>>>> Seems sane to me.  Hans, what do you think?
>>>
>>> I think this is going in the right direction, but min_power enables
>>> both DEVSLP and HIPM, AFAIK Windows at least in the past did not
>>> enable HIPM be default.
>>
>> Windows and Linux will need to meet the same criteria to reach SLP_S0
>> state. OS is not deciding whether we meet criteria or not, it is
>> firmware deciding, and it will look for SATA IP to be in certain
>> predefined state, before generating this SLP_S0 signal.
>>
>>
>>> Srinivas can you figure out what Windows does wrt HIPM?
>>
>> We can see that under Windows the SATA rail voltage is dropped. I don't
>> know if there is any other method other than DEVSLP configuration. But
>> I will surely check around.
> 
> I understand that we need DEVSLP, but AFAIK DEVSLP is entered automatically
> after being in slumber for a defined time. The question is how we get into
> slumber. Currently with the new med_power_with_dipm policy we always use
> DIPM to enter slumber. Your suggested switch to min_power means also
> enabling HIPM. What I'm suggesting is that it might better to have
> DIPM + DEVSLP rather then DIPM + HIPM + DEVSLP.
> 
> So basically the question is does windows use:
> 
> a) DIPM + DEVSLP; or
> b) DIPM + HIPM + DEVSLP
> 
> If b. is the case, then I'm fine with your patch as is, but if Windows
> does a. then we should mimic that, which requires a new power-level as
> our min_power level is b.

So digging a bit deeper I just realized that another important difference
between med_power_with_dipm and min_power is that min_power by default
set the ASP bits making the link go to the slumber state instead of to
the partial (power-saving) state.

According to:
https://www.intel.com/content/dam/doc/reference-guide/sata-devices-implementation-recommendations.pdf

ASP defaults to off in the iRST drivers. But that is a document from
before DEVSLP got introduced.

So we need to know if Windows and/or the iRST drivers use HIPM and
ASP by default on these systems. If they do then using min_power
is fine. If they don't use one or the other we are going to need
a new policy reflecting those settings and use that.

Regards,

Hans