On some platform, when OS enables LPM by default (eg, min_power),
then, PhyRdy Change cannot be detected if ahci supports no LPM;
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, PhyRdy Change cannot coexist with LPM;
when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM
field must be programmed to disallow device initiated
Partial/Slumber request.
Adds suports to control these cases on actual LPM capability.
Runa Guo-oc (2):
ahci: Add PhyRdy Change control on actual LPM capability
ahci: Add PxSCTL.IPM control on actual LPM capability
drivers/ata/ahci.c | 9 +++++++++
drivers/ata/libata-eh.c | 4 ++++
drivers/ata/libata-sata.c | 12 +++++++++++-
include/linux/libata.h | 4 ++++
4 files changed, 28 insertions(+), 1 deletion(-)
--
2.7.4
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.
Signed-off-by: Runa Guo-oc <[email protected]>
---
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);
+
+ /* if controller does not support partial, then disallows it,
+ * the same for slumber
+ */
+ if (!(link->ap->host->flags & ATA_HOST_PART))
+ scontrol |= (0x1 << 8);
+
+ if (!(link->ap->host->flags & ATA_HOST_SSC))
+ scontrol |= (0x2 << 8);
+ }
else {
/* empty port, power off */
scontrol &= ~0xf;
--
2.7.4
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 <[email protected]>
> ---
> 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
On 2022/4/21 18:53, Damien Le Moal wrote:
> 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
Sorry, here should use 'Add' instead of 'Adds'.
> Overall, I need to reread the specs to confirm all this.
Ok.
>> Signed-off-by: Runa Guo-oc <[email protected]>
>> ---
>> 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;
> }
I'll change it like below,
+ if (ata_link_nr_enabled(link) > 0) {
- /* no restrictions on LPM transitions */
+ /* Assume no restrictions on LPM transitions */
scontrol &= ~(0x7 << 8);
+ /*
+ * 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 {
/* empty port, power off */
scontrol &= ~0xf;
}