Messages in this thread | | | Date | Thu, 21 Apr 2022 19:53:40 +0900 | Subject | Re: [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability | From | Damien Le Moal <> |
| |
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@zhaoxin.com> > --- > 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
| |