* [RFC PATCH v2 0/2] ata: ahci: Enable DEVSLP by default on SLP_S0 support @ 2018-07-12 22:27 Srinivas Pandruvada 2018-07-12 22:27 ` [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state Srinivas Pandruvada 2018-07-12 22:27 ` [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 Srinivas Pandruvada 0 siblings, 2 replies; 7+ messages in thread From: Srinivas Pandruvada @ 2018-07-12 22:27 UTC (permalink / raw) To: tj, hdegoede Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw, Srinivas Pandruvada One of the requirement for modern x86 system to enter lowest power mode (SLP_S0) is SATA IP block to be off. This is true even during when platform is suspended to idle and not only in opportunistic (runtime) suspend. This series is to enable DEVSLP by default. v2 - As suggested by Hans, it is possible to have ASP with DEVSLP, so add a new state. - Removed usage of mem_sleep_current, instead just rely on low power idle flag. Don't feel good to EXPORT from core suspend code. - Depending host policy to decide if we enable DEVSLP by default. Srinivas (1): ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 Srinivas Pandruvada (1): ata: ahci: Support state with min power but Partial low power state drivers/ata/ahci.c | 15 +++++++++++++++ drivers/ata/libahci.c | 6 +++++- drivers/ata/libata-core.c | 1 + drivers/ata/libata-scsi.c | 1 + include/linux/libata.h | 3 ++- 5 files changed, 24 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state 2018-07-12 22:27 [RFC PATCH v2 0/2] ata: ahci: Enable DEVSLP by default on SLP_S0 support Srinivas Pandruvada @ 2018-07-12 22:27 ` Srinivas Pandruvada 2018-07-13 9:08 ` Hans de Goede 2018-07-12 22:27 ` [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 Srinivas Pandruvada 1 sibling, 1 reply; 7+ messages in thread From: Srinivas Pandruvada @ 2018-07-12 22:27 UTC (permalink / raw) To: tj, hdegoede Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw, Srinivas Pandruvada 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> --- 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 { -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state 2018-07-12 22:27 ` [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state Srinivas Pandruvada @ 2018-07-13 9:08 ` Hans de Goede 2018-07-13 15:06 ` Srinivas Pandruvada 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2018-07-13 9:08 UTC (permalink / raw) To: Srinivas Pandruvada, tj Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw 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 is not set you only get partial, 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. 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 { > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state 2018-07-13 9:08 ` Hans de Goede @ 2018-07-13 15:06 ` Srinivas Pandruvada 2018-07-19 13:13 ` Hans de Goede 0 siblings, 1 reply; 7+ messages in thread From: Srinivas Pandruvada @ 2018-07-13 15:06 UTC (permalink / raw) To: Hans de Goede, tj Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw 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 Thanks, Srinivas > > 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 { > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state 2018-07-13 15:06 ` Srinivas Pandruvada @ 2018-07-19 13:13 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2018-07-19 13:13 UTC (permalink / raw) To: Srinivas Pandruvada, tj Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw 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 { >>> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 2018-07-12 22:27 [RFC PATCH v2 0/2] ata: ahci: Enable DEVSLP by default on SLP_S0 support Srinivas Pandruvada 2018-07-12 22:27 ` [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state Srinivas Pandruvada @ 2018-07-12 22:27 ` Srinivas Pandruvada 2018-07-13 9:13 ` Hans de Goede 1 sibling, 1 reply; 7+ messages in thread From: Srinivas Pandruvada @ 2018-07-12 22:27 UTC (permalink / raw) To: tj, hdegoede Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw, Srinivas From: Srinivas <srinivas.pandruvada@linux.intel.com> One of the requirement for modern x86 system to enter lowest power mode (SLP_S0) is SATA IP block to be off. This is true even during when platform is suspended to idle and not only in opportunistic (runtime) suspend. Several of these system don't have traditional ACPI S3, so it is important that they enter SLP_S0 state, to avoid draining battery even during suspend. So it is important that out of the box Linux installation reach this state. 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 with partial (preferred) or slumber support on idle 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 SLP_S0 capable using ACPI low power idle flag This combination will make sure that systems are fairly recent and since getting shipped with SLP_S0 support, the DEVSLP function is already validated. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/ata/ahci.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index b2b9eba1d214..e8d425823f9b 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1604,6 +1604,19 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports, return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX); } +static void ahci_update_mobile_policy(struct ahci_host_priv *hpriv) +{ +#ifdef CONFIG_ACPI + if (mobile_lpm_policy > ATA_LPM_MED_POWER && + (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { + if (hpriv->cap & HOST_CAP_PART) + mobile_lpm_policy = ATA_LPM_MIN_POWER_WITH_ASP; + else if (hpriv->cap & HOST_CAP_SSC) + mobile_lpm_policy = ATA_LPM_MIN_POWER; + } +#endif +} + static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { unsigned int board_id = ent->driver_data; @@ -1796,6 +1809,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (pi.flags & ATA_FLAG_EM) ahci_reset_em(host); + ahci_update_mobile_policy(hpriv); + for (i = 0; i < host->n_ports; i++) { struct ata_port *ap = host->ports[i]; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 2018-07-12 22:27 ` [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 Srinivas Pandruvada @ 2018-07-13 9:13 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2018-07-13 9:13 UTC (permalink / raw) To: Srinivas Pandruvada, tj Cc: linux-ide, linux-kernel, alan.cox, Mario.Limonciello, rjw Hi, On 13-07-18 00:27, Srinivas Pandruvada wrote: > From: Srinivas <srinivas.pandruvada@linux.intel.com> > > One of the requirement for modern x86 system to enter lowest power mode > (SLP_S0) is SATA IP block to be off. This is true even during when > platform is suspended to idle and not only in opportunistic (runtime) > suspend. > > Several of these system don't have traditional ACPI S3, so it is > important that they enter SLP_S0 state, to avoid draining battery even > during suspend. So it is important that out of the box Linux installation > reach this state. > > 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 > with partial (preferred) or slumber support on idle 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 SLP_S0 capable using ACPI low power idle flag > This combination will make sure that systems are fairly recent and > since getting shipped with SLP_S0 support, the DEVSLP function > is already validated. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/ata/ahci.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index b2b9eba1d214..e8d425823f9b 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1604,6 +1604,19 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports, > return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX); > } > > +static void ahci_update_mobile_policy(struct ahci_host_priv *hpriv) > +{ > +#ifdef CONFIG_ACPI > + if (mobile_lpm_policy > ATA_LPM_MED_POWER && > + (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) { > + if (hpriv->cap & HOST_CAP_PART) > + mobile_lpm_policy = ATA_LPM_MIN_POWER_WITH_ASP; > + else if (hpriv->cap & HOST_CAP_SSC) > + mobile_lpm_policy = ATA_LPM_MIN_POWER; You are checking hpriv flags here and changing global state based on that. Instead it would be better to add a new: int ahci_get_initial_lpm_policy(struct ahci_host_priv *hpriv); Function and replace this: if ((hpriv->flags & AHCI_HFLAG_IS_MOBILE) && mobile_lpm_policy >= ATA_LPM_UNKNOWN && mobile_lpm_policy <= ATA_LPM_MIN_POWER) ap->target_lpm_policy = mobile_lpm_policy; With: ap->target_lpm_policy = ahci_get_initial_lpm_policy(priv); And do the original checks + your new checks all in the ahci_get_initial_lpm_policy() function. Regards, Hans > + } > +#endif > +} > + > static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > { > unsigned int board_id = ent->driver_data; > @@ -1796,6 +1809,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (pi.flags & ATA_FLAG_EM) > ahci_reset_em(host); > > + ahci_update_mobile_policy(hpriv); > + > for (i = 0; i < host->n_ports; i++) { > struct ata_port *ap = host->ports[i]; > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-19 13:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-12 22:27 [RFC PATCH v2 0/2] ata: ahci: Enable DEVSLP by default on SLP_S0 support Srinivas Pandruvada 2018-07-12 22:27 ` [RFC PATCH v2 1/2] ata: ahci: Support state with min power and Partial low power state Srinivas Pandruvada 2018-07-13 9:08 ` Hans de Goede 2018-07-13 15:06 ` Srinivas Pandruvada 2018-07-19 13:13 ` Hans de Goede 2018-07-12 22:27 ` [RFC PATCH v2 2/2] ata: ahci: Enable DEVSLP by default on x86 with SLP_S0 Srinivas Pandruvada 2018-07-13 9:13 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).