Hi,

On 03-07-18 10:57, Hans de Goede wrote:

Hi,

On 03-07-18 00:08, Srinivas Pandruvada wrote:

quoted

Hi Hans,

On Mon, 2018-07-02 at 23:27 +0200, Hans de Goede wrote:

<snip>

quoted

quoted

quoted

quoted

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.

quoted

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