On 4/21/22 18:43, Runa Guo-oc wrote:
> On some platform, when OS enables LPM by default (eg, min_power),
> then, DIPM slumber request cannot be disallowed if ahci's CAP.PSC
> is set to '1' and CAP.SSC is cleared to '0', which may cause ahci
> to be an uncertain state (same for Partial).
>
> In ahci spec, when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM
> field must be programmed to disallow device initiated Partial/
> Slumber request.
>
> Adds support to control this case on actual LPM capability.
s/Adds/Add
Overall, I need to reread the specs to confirm all this.
>
> Signed-off-by: Runa Guo-oc <RunaGuo-oc@xxxxxxxxxxx>
> ---
> drivers/ata/libata-sata.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 7a5fe41..e6195cf 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -394,9 +394,19 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> case ATA_LPM_MED_POWER_WITH_DIPM:
> case ATA_LPM_MIN_POWER_WITH_PARTIAL:
> case ATA_LPM_MIN_POWER:
> - if (ata_link_nr_enabled(link) > 0)
> + if (ata_link_nr_enabled(link) > 0) {
> /* no restrictions on LPM transitions */> scontrol &= ~(0x7 << 8);
Given that the added code below adds restrictions, this comment is
strange. Better change it to something like:
/* Assume no restrictions on LPM transitions */
> +
> + /* if controller does not support partial, then disallows it,
> + * the same for slumber
> + */
Please correctly format the comment and check the grammar. Some like below
is easier to read.
/*
* If the controller does not support partial or
* slumber then disallow these transitions.
*/
> + if (!(link->ap->host->flags & ATA_HOST_PART))
> + scontrol |= (0x1 << 8);
> +
> + if (!(link->ap->host->flags & ATA_HOST_SSC))
> + scontrol |= (0x2 << 8);
> + }
> else {
Please do not leave this else here. Put it on the same line as the closing
bracket and enclose the below statements in brackets too.
> /* empty port, power off */
> scontrol &= ~0xf;
} else {
/* empty port, power off */
scontrol &= ~0xf;
}
--
Damien Le Moal
Western Digital Research
