Messages in this thread | | | Subject | Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state | From | Hans de Goede <> | Date | Thu, 19 Jul 2018 15:13:40 +0200 |
| |
Hi,
On 13-07-18 17:06, Srinivas Pandruvada wrote: > Hi Hans, > On Fri, 2018-07-13 at 11:08 +0200, Hans de Goede wrote: >> Hi, >> >> On 13-07-18 00:27, Srinivas Pandruvada wrote: >>> Currently when min_power policy is selected, the partial low power >>> state >>> is not entered and link will try aggressively enter to only slumber >>> state. >>> Add a new policy which still enable DEVSLP but also try to enter >>> partial >>> low power state. >>> >>> For information the difference between partial and slumber >>> Partial – PHY logic is powered up, and in a reduced power state. >>> The link >>> PM exit latency to active state maximum is 10 ns. >>> Slumber – PHY logic is powered up, and in a reduced power state. >>> The link >>> PM exit latency to active state maximum is 10 ms. >>> Devslp – PHY logic is powered down. The link PM exit latency from >>> this >>> state to active state maximum is 20 ms, unless otherwise specified >>> by >>> DETO. >>> >>> Suggested-by: Hans de Goede <hdegoede@redhat.com> >>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel >>> .com> >> >> You got this the wrong way around, when ALP > You mean ASP, as per spec in section 3.3.7 of the spec. > >> is not set you only get >> partial, > Not exactly. > It means that the system will try to enter aggressively partial . I > have seen slumber residency also when this is reset. When "set" you > don't see any any partial. But when you reset you see both. So based on > idle duration link will transition from partial to slumber even if "not > set". > >> the ALP bit is active high, not active low as your commit >> suggests. So the name of the new policy should be >> min_power_without_asp. > > > The policy name I am suggesting is not the actual bit value of ASP (I > interpreted ASP as name for partial), but whether you will get to > partial or not. So I think it is better to rename to > "min_power_with_partial". User should not have to worry about the bit > value. > > > So user will see with the rename from "min_power_with_asp" to > "min_power_with_partial": > > #cat /sys/class/scsi_host/host*/link_power_management_policy > min_power_with_partial > min_power_with_partial > min_power_with_partial > > Output of test (You will see both partial and slumber) > > SATA LPM Summary - Sampled: Residency (Percentage) > Disk Name , Off (%), Active (%), Partial (%), Slumber (%), Devsleep (%), Other (%), Uninitialized (%) > --------- , -------, ----------, -----------, -----------, ------------, ---------, ----------------- > INTEL SSDSCKJF180A5, 0.0 , 4.6 , 13.1 , 30.3 , 52.0 , 0.0 , 0.0 > > SATA LPM Summary - Sampled: Counts > Disk Name , Off, Active, Partial, Slumber, Devsleep, Other, Uninitialized > --------- , ---, ------, -------, -------, --------, -----, ------------- > INTEL SSDSCKJF180A5, 0 , 9 , 27 , 60 , 103 , 0 , 0 > > > # cat /sys/class/scsi_host/host*/link_power_management_policy > min_power > min_power > min_power > > Output of test (You will see only slumber) > > SATA LPM Summary - Sampled: Residency (Percentage) > Disk Name , Off (%), Active (%), Partial (%), Slumber (%), Devsleep (%), Other (%), Uninitialized (%) > --------- , -------, ----------, -----------, -----------, ------------, ---------, ----------------- > INTEL SSDSCKJF180A5, 0.0 , 6.8 , 0.0 , 53.8 , 39.3 , 0.0 , 0.0 > > SATA LPM Summary - Sampled: Counts > Disk Name , Off, Active, Partial, Slumber, Devsleep, Other, Uninitialized > --------- , ---, ------, -------, -------, --------, -----, ------------- > INTEL SSDSCKJF180A5, 0 , 11 , 0 , 107 , 76 , 0 , 0
Ah, I see thank you for clarifying that. Yes I agree that min_power_with_partial is the best name for the new policy.
For the next version do not forget to make the overriding of the lpm policy a per host thing, rather then directly overriding the global value as I mentioned in a previous mail.
One other thing is to NOT override the policy if it is set on the kernel-commandline, which I believe will require setting the global value to -1 and only set another value if the global value is not -1 (using you settings where applicable and falling back to CONFIG_SATA_MOBILE_LPM_POLICY where not applicable).
Regards,
Hans
>>> --- >>> drivers/ata/libahci.c | 6 +++++- >>> drivers/ata/libata-core.c | 1 + >>> drivers/ata/libata-scsi.c | 1 + >>> include/linux/libata.h | 3 ++- >>> 4 files changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c >>> index 511fb67f363d..8cf2cf49537d 100644 >>> --- a/drivers/ata/libahci.c >>> +++ b/drivers/ata/libahci.c >>> @@ -799,8 +799,11 @@ static int ahci_set_lpm(struct ata_link *link, >>> enum ata_lpm_policy policy, >>> return 0; >>> } else { >>> cmd |= PORT_CMD_ALPE; >>> + >>> if (policy == ATA_LPM_MIN_POWER) >>> cmd |= PORT_CMD_ASP; >>> + else if (policy == >>> ATA_LPM_MIN_POWER_WITH_ASP) >>> + cmd &= ~PORT_CMD_ASP; >>> >>> /* write out new cmd value */ >>> writel(cmd, port_mmio + PORT_CMD); >>> @@ -811,7 +814,8 @@ static int ahci_set_lpm(struct ata_link *link, >>> enum ata_lpm_policy policy, >>> if ((hpriv->cap2 & HOST_CAP2_SDS) && >>> (hpriv->cap2 & HOST_CAP2_SADM) && >>> (link->device->flags & ATA_DFLAG_DEVSLP)) { >>> - if (policy == ATA_LPM_MIN_POWER) >>> + if (policy == ATA_LPM_MIN_POWER || >>> + policy == ATA_LPM_MIN_POWER_WITH_ASP) >>> ahci_set_aggressive_devslp(ap, true); >>> else >>> ahci_set_aggressive_devslp(ap, false); >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index cc71c63df381..245a59e6cb18 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -3970,6 +3970,7 @@ int sata_link_scr_lpm(struct ata_link *link, >>> enum ata_lpm_policy policy, >>> scontrol |= (0x6 << 8); >>> break; >>> case ATA_LPM_MED_POWER_WITH_DIPM: >>> + case ATA_LPM_MIN_POWER_WITH_ASP: >>> case ATA_LPM_MIN_POWER: >>> if (ata_link_nr_enabled(link) > 0) >>> /* no restrictions on LPM transitions */ >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index aad1b01447de..2d683db50ceb 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -110,6 +110,7 @@ static const char *ata_lpm_policy_names[] = { >>> [ATA_LPM_MAX_POWER] = "max_performance", >>> [ATA_LPM_MED_POWER] = "medium_power", >>> [ATA_LPM_MED_POWER_WITH_DIPM] = >>> "med_power_with_dipm", >>> + [ATA_LPM_MIN_POWER_WITH_ASP] = >>> "min_power_with_asp", >>> [ATA_LPM_MIN_POWER] = "min_power", >>> }; >>> >>> diff --git a/include/linux/libata.h b/include/linux/libata.h >>> index 32f247cb5e9e..1e154f1f7e8f 100644 >>> --- a/include/linux/libata.h >>> +++ b/include/linux/libata.h >>> @@ -523,7 +523,8 @@ enum ata_lpm_policy { >>> ATA_LPM_MAX_POWER, >>> ATA_LPM_MED_POWER, >>> ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win >>> IRST does */ >>> - ATA_LPM_MIN_POWER, >>> + ATA_LPM_MIN_POWER_WITH_ASP, /* Min Power + partial and >>> slumber */ >>> + ATA_LPM_MIN_POWER, /* Min power + no partial (slumber >>> only) */ >>> }; >>> >>> enum ata_lpm_hints { >>>
| |