* [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle
@ 2020-11-20 6:07 Shyam Sundar S K
2020-11-23 16:45 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Shyam Sundar S K @ 2020-11-20 6:07 UTC (permalink / raw)
To: rjw, lenb, linux-acpi; +Cc: Shyam Sundar S K
Initial support for S2Idle based on the Intel implementation [1] does not
work for AMD as the BIOS implementation for ACPI methods like the _DSM
are not standardized.
So, the way in which the UUID's were parsed and the ACPI packages were
retrieved out of the ACPI objects are not the same between Intel and AMD.
This patch adds AMD support for S2Idle to parse the UUID, evaluate the
_DSM methods, preparing the Idle constraint list etc.
Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
v2:(https://www.spinics.net/lists/linux-acpi/msg97104.html)
- Remove default case in switch.
- Add a routine acpi_match_vendor_name() required when building on a
non-x86 based environment.
- Fixed spelling mistakes.
---
drivers/acpi/sleep.c | 166 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 157 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index aff13bf4d947..b929fd0d2e04 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -710,6 +710,11 @@ static const struct acpi_device_id lps0_device_ids[] = {
#define ACPI_LPS0_ENTRY 5
#define ACPI_LPS0_EXIT 6
+/* AMD */
+#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
+#define ACPI_LPS0_SCREEN_OFF_AMD 4
+#define ACPI_LPS0_SCREEN_ON_AMD 5
+
static acpi_handle lps0_device_handle;
static guid_t lps0_dsm_guid;
static char lps0_dsm_func_mask;
@@ -733,8 +738,124 @@ struct lpi_constraints {
int min_dstate;
};
+/* AMD */
+/* Device constraint entry structure */
+struct lpi_device_info_amd {
+ int revision;
+ int count;
+ union acpi_object *package;
+};
+
+/* Constraint package structure */
+struct lpi_device_constraint_amd {
+ char *name;
+ int enabled;
+ int function_states;
+ int min_dstate;
+};
+
static struct lpi_constraints *lpi_constraints_table;
static int lpi_constraints_table_size;
+static int rev_id;
+
+static void lpi_device_get_constraints_amd(void)
+{
+ union acpi_object *out_obj;
+ int i, j, k;
+
+ out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
+ 1, ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
+ NULL, ACPI_TYPE_PACKAGE);
+
+ if (!out_obj)
+ return;
+
+ acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
+ out_obj ? "successful" : "failed");
+
+ for (i = 0; i < out_obj->package.count; i++) {
+ union acpi_object *package = &out_obj->package.elements[i];
+ struct lpi_device_info_amd info = { };
+
+ if (package->type == ACPI_TYPE_INTEGER) {
+ switch (i) {
+ case 0:
+ info.revision = package->integer.value;
+ break;
+ case 1:
+ info.count = package->integer.value;
+ break;
+ }
+ } else if (package->type == ACPI_TYPE_PACKAGE) {
+ lpi_constraints_table = kcalloc(package->package.count,
+ sizeof(*lpi_constraints_table),
+ GFP_KERNEL);
+
+ if (!lpi_constraints_table)
+ goto free_acpi_buffer;
+
+ acpi_handle_info(lps0_device_handle,
+ "LPI: constraints list begin:\n");
+
+ for (j = 0; j < package->package.count; ++j) {
+ union acpi_object *info_obj = &package->package.elements[j];
+ struct lpi_device_constraint_amd dev_info = {};
+ struct lpi_constraints *list;
+ acpi_status status;
+
+ for (k = 0; k < info_obj->package.count; ++k) {
+ union acpi_object *obj = &info_obj->package.elements[k];
+ union acpi_object *obj_new;
+
+ list = &lpi_constraints_table[lpi_constraints_table_size];
+ list->min_dstate = -1;
+
+ obj_new = &obj[k];
+ switch (k) {
+ case 0:
+ dev_info.enabled = obj->integer.value;
+ break;
+ case 1:
+ dev_info.name = obj->string.pointer;
+ break;
+ case 2:
+ dev_info.function_states = obj->integer.value;
+ break;
+ case 3:
+ dev_info.min_dstate = obj->integer.value;
+ break;
+ }
+
+ if (!dev_info.enabled || !dev_info.name ||
+ !dev_info.min_dstate)
+ continue;
+
+ status = acpi_get_handle(NULL, dev_info.name,
+ &list->handle);
+ if (ACPI_FAILURE(status))
+ continue;
+
+ acpi_handle_info(lps0_device_handle,
+ "Name:%s\n", dev_info.name);
+
+ list->min_dstate = dev_info.min_dstate;
+
+ if (list->min_dstate < 0) {
+ acpi_handle_info(lps0_device_handle,
+ "Incomplete constraint defined\n");
+ continue;
+ }
+ }
+ lpi_constraints_table_size++;
+ }
+ }
+ }
+
+ acpi_handle_info(lps0_device_handle, "LPI: constraints list end\n");
+
+free_acpi_buffer:
+ ACPI_FREE(out_obj);
+}
static void lpi_device_get_constraints(void)
{
@@ -883,13 +1004,22 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func)
if (!(lps0_dsm_func_mask & (1 << func)))
return;
- out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
+ out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, rev_id, func, NULL);
ACPI_FREE(out_obj);
acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
func, out_obj ? "successful" : "failed");
}
+static bool acpi_match_vendor_name(void)
+{
+#ifdef CONFIG_X86
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+ return true;
+#endif
+ return false;
+}
+
static int lps0_device_attach(struct acpi_device *adev,
const struct acpi_device_id *not_used)
{
@@ -901,9 +1031,17 @@ static int lps0_device_attach(struct acpi_device *adev,
if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
return 0;
- guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
+ if (acpi_match_vendor_name()) {
+ guid_parse(ACPI_LPS0_DSM_UUID_AMD, &lps0_dsm_guid);
+ out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 0, 0, NULL);
+ rev_id = 0;
+ } else {
+ guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
+ out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
+ rev_id = 1;
+ }
+
/* Check if the _DSM is present and as expected. */
- out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER) {
acpi_handle_debug(adev->handle,
"_DSM function 0 evaluation failed\n");
@@ -919,7 +1057,10 @@ static int lps0_device_attach(struct acpi_device *adev,
lps0_device_handle = adev->handle;
- lpi_device_get_constraints();
+ if (acpi_match_vendor_name())
+ lpi_device_get_constraints_amd();
+ else
+ lpi_device_get_constraints();
/*
* Use suspend-to-idle by default if the default suspend mode was not
@@ -974,9 +1115,12 @@ static int acpi_s2idle_prepare_late(void)
if (pm_debug_messages_on)
lpi_check_constraints();
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
-
+ if (acpi_match_vendor_name()) {
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF_AMD);
+ } else {
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
+ }
return 0;
}
@@ -1051,8 +1195,12 @@ static void acpi_s2idle_restore_early(void)
if (!lps0_device_handle || sleep_no_lps0)
return;
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
- acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
+ if (acpi_match_vendor_name()) {
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON_AMD);
+ } else {
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
+ acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
+ }
}
static void acpi_s2idle_restore(void)
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle
2020-11-20 6:07 [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle Shyam Sundar S K
@ 2020-11-23 16:45 ` Rafael J. Wysocki
2020-11-25 6:45 ` Shyam Sundar S K
0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2020-11-23 16:45 UTC (permalink / raw)
To: Shyam Sundar S K; +Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List
On Fri, Nov 20, 2020 at 7:10 AM Shyam Sundar S K
<Shyam-sundar.S-k@amd.com> wrote:
>
> Initial support for S2Idle based on the Intel implementation [1] does not
> work for AMD as the BIOS implementation for ACPI methods like the _DSM
> are not standardized.
>
> So, the way in which the UUID's were parsed and the ACPI packages were
> retrieved out of the ACPI objects are not the same between Intel and AMD.
>
> This patch adds AMD support for S2Idle to parse the UUID, evaluate the
> _DSM methods, preparing the Idle constraint list etc.
>
> Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> v2:(https://www.spinics.net/lists/linux-acpi/msg97104.html)
> - Remove default case in switch.
> - Add a routine acpi_match_vendor_name() required when building on a
> non-x86 based environment.
> - Fixed spelling mistakes.
> ---
> drivers/acpi/sleep.c | 166 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 157 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index aff13bf4d947..b929fd0d2e04 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -710,6 +710,11 @@ static const struct acpi_device_id lps0_device_ids[] = {
> #define ACPI_LPS0_ENTRY 5
> #define ACPI_LPS0_EXIT 6
>
> +/* AMD */
> +#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
> +#define ACPI_LPS0_SCREEN_OFF_AMD 4
> +#define ACPI_LPS0_SCREEN_ON_AMD 5
> +
> static acpi_handle lps0_device_handle;
> static guid_t lps0_dsm_guid;
> static char lps0_dsm_func_mask;
> @@ -733,8 +738,124 @@ struct lpi_constraints {
> int min_dstate;
> };
>
> +/* AMD */
> +/* Device constraint entry structure */
> +struct lpi_device_info_amd {
> + int revision;
> + int count;
> + union acpi_object *package;
> +};
> +
> +/* Constraint package structure */
> +struct lpi_device_constraint_amd {
> + char *name;
> + int enabled;
> + int function_states;
> + int min_dstate;
> +};
> +
> static struct lpi_constraints *lpi_constraints_table;
> static int lpi_constraints_table_size;
> +static int rev_id;
> +
> +static void lpi_device_get_constraints_amd(void)
> +{
> + union acpi_object *out_obj;
> + int i, j, k;
> +
> + out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
> + 1, ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
> + NULL, ACPI_TYPE_PACKAGE);
> +
> + if (!out_obj)
> + return;
> +
> + acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
> + out_obj ? "successful" : "failed");
> +
> + for (i = 0; i < out_obj->package.count; i++) {
> + union acpi_object *package = &out_obj->package.elements[i];
> + struct lpi_device_info_amd info = { };
> +
> + if (package->type == ACPI_TYPE_INTEGER) {
> + switch (i) {
> + case 0:
> + info.revision = package->integer.value;
> + break;
> + case 1:
> + info.count = package->integer.value;
> + break;
> + }
> + } else if (package->type == ACPI_TYPE_PACKAGE) {
> + lpi_constraints_table = kcalloc(package->package.count,
> + sizeof(*lpi_constraints_table),
> + GFP_KERNEL);
> +
> + if (!lpi_constraints_table)
> + goto free_acpi_buffer;
> +
> + acpi_handle_info(lps0_device_handle,
> + "LPI: constraints list begin:\n");
> +
> + for (j = 0; j < package->package.count; ++j) {
> + union acpi_object *info_obj = &package->package.elements[j];
> + struct lpi_device_constraint_amd dev_info = {};
> + struct lpi_constraints *list;
> + acpi_status status;
> +
> + for (k = 0; k < info_obj->package.count; ++k) {
> + union acpi_object *obj = &info_obj->package.elements[k];
> + union acpi_object *obj_new;
> +
> + list = &lpi_constraints_table[lpi_constraints_table_size];
> + list->min_dstate = -1;
> +
> + obj_new = &obj[k];
> + switch (k) {
> + case 0:
> + dev_info.enabled = obj->integer.value;
> + break;
> + case 1:
> + dev_info.name = obj->string.pointer;
> + break;
> + case 2:
> + dev_info.function_states = obj->integer.value;
> + break;
> + case 3:
> + dev_info.min_dstate = obj->integer.value;
> + break;
> + }
> +
> + if (!dev_info.enabled || !dev_info.name ||
> + !dev_info.min_dstate)
> + continue;
> +
> + status = acpi_get_handle(NULL, dev_info.name,
> + &list->handle);
> + if (ACPI_FAILURE(status))
> + continue;
> +
> + acpi_handle_info(lps0_device_handle,
> + "Name:%s\n", dev_info.name);
> +
> + list->min_dstate = dev_info.min_dstate;
> +
> + if (list->min_dstate < 0) {
> + acpi_handle_info(lps0_device_handle,
> + "Incomplete constraint defined\n");
> + continue;
> + }
> + }
> + lpi_constraints_table_size++;
> + }
> + }
> + }
> +
> + acpi_handle_info(lps0_device_handle, "LPI: constraints list end\n");
> +
> +free_acpi_buffer:
> + ACPI_FREE(out_obj);
> +}
>
> static void lpi_device_get_constraints(void)
> {
> @@ -883,13 +1004,22 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func)
> if (!(lps0_dsm_func_mask & (1 << func)))
> return;
>
> - out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
> + out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, rev_id, func, NULL);
> ACPI_FREE(out_obj);
>
> acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
> func, out_obj ? "successful" : "failed");
> }
>
> +static bool acpi_match_vendor_name(void)
> +{
> +#ifdef CONFIG_X86
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> + return true;
> +#endif
> + return false;
> +}
Well, I still don't like this (explicit CONFIG_X86 dependencies are
not welcome in this file), but arguably the damage was done before
already and there is a bunch of X86-specific code in here anyway, so
I'm going to take your patch provisionally and rearrange the code on
top of it so as to relocate the x86-specific part of it into
acpi/x86/.
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle
2020-11-23 16:45 ` Rafael J. Wysocki
@ 2020-11-25 6:45 ` Shyam Sundar S K
2020-12-10 18:56 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Shyam Sundar S K @ 2020-11-25 6:45 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List
On 11/23/2020 10:15 PM, Rafael J. Wysocki wrote:
> [CAUTION: External Email]
>
> On Fri, Nov 20, 2020 at 7:10 AM Shyam Sundar S K
> <Shyam-sundar.S-k@amd.com> wrote:
>>
>> Initial support for S2Idle based on the Intel implementation [1] does not
>> work for AMD as the BIOS implementation for ACPI methods like the _DSM
>> are not standardized.
>>
>> So, the way in which the UUID's were parsed and the ACPI packages were
>> retrieved out of the ACPI objects are not the same between Intel and AMD.
>>
>> This patch adds AMD support for S2Idle to parse the UUID, evaluate the
>> _DSM methods, preparing the Idle constraint list etc.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fsites%2Fdefault%2Ffiles%2Fresources%2FIntel_ACPI_Low_Power_S0_Idle.pdf&data=04%7C01%7CShyam-sundar.S-k%40amd.com%7C640970e0f42a4fd53a1408d88fcf25d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637417467581911776%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=xPFj5OymQkr3xEaU8qQD1y8O95XCszUblRRuknNvD7I%3D&reserved=0 # [1]
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> v2:(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-acpi%2Fmsg97104.html&data=04%7C01%7CShyam-sundar.S-k%40amd.com%7C640970e0f42a4fd53a1408d88fcf25d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637417467581911776%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=r5whv52ruKVbFgsV1qEuWKeePaUQhjXnxQwF7izFRf4%3D&reserved=0)
>> - Remove default case in switch.
>> - Add a routine acpi_match_vendor_name() required when building on a
>> non-x86 based environment.
>> - Fixed spelling mistakes.
>> ---
>> drivers/acpi/sleep.c | 166 ++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 157 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index aff13bf4d947..b929fd0d2e04 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -710,6 +710,11 @@ static const struct acpi_device_id lps0_device_ids[] = {
>> #define ACPI_LPS0_ENTRY 5
>> #define ACPI_LPS0_EXIT 6
>>
>> +/* AMD */
>> +#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
>> +#define ACPI_LPS0_SCREEN_OFF_AMD 4
>> +#define ACPI_LPS0_SCREEN_ON_AMD 5
>> +
>> static acpi_handle lps0_device_handle;
>> static guid_t lps0_dsm_guid;
>> static char lps0_dsm_func_mask;
>> @@ -733,8 +738,124 @@ struct lpi_constraints {
>> int min_dstate;
>> };
>>
>> +/* AMD */
>> +/* Device constraint entry structure */
>> +struct lpi_device_info_amd {
>> + int revision;
>> + int count;
>> + union acpi_object *package;
>> +};
>> +
>> +/* Constraint package structure */
>> +struct lpi_device_constraint_amd {
>> + char *name;
>> + int enabled;
>> + int function_states;
>> + int min_dstate;
>> +};
>> +
>> static struct lpi_constraints *lpi_constraints_table;
>> static int lpi_constraints_table_size;
>> +static int rev_id;
>> +
>> +static void lpi_device_get_constraints_amd(void)
>> +{
>> + union acpi_object *out_obj;
>> + int i, j, k;
>> +
>> + out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
>> + 1, ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
>> + NULL, ACPI_TYPE_PACKAGE);
>> +
>> + if (!out_obj)
>> + return;
>> +
>> + acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
>> + out_obj ? "successful" : "failed");
>> +
>> + for (i = 0; i < out_obj->package.count; i++) {
>> + union acpi_object *package = &out_obj->package.elements[i];
>> + struct lpi_device_info_amd info = { };
>> +
>> + if (package->type == ACPI_TYPE_INTEGER) {
>> + switch (i) {
>> + case 0:
>> + info.revision = package->integer.value;
>> + break;
>> + case 1:
>> + info.count = package->integer.value;
>> + break;
>> + }
>> + } else if (package->type == ACPI_TYPE_PACKAGE) {
>> + lpi_constraints_table = kcalloc(package->package.count,
>> + sizeof(*lpi_constraints_table),
>> + GFP_KERNEL);
>> +
>> + if (!lpi_constraints_table)
>> + goto free_acpi_buffer;
>> +
>> + acpi_handle_info(lps0_device_handle,
>> + "LPI: constraints list begin:\n");
>> +
>> + for (j = 0; j < package->package.count; ++j) {
>> + union acpi_object *info_obj = &package->package.elements[j];
>> + struct lpi_device_constraint_amd dev_info = {};
>> + struct lpi_constraints *list;
>> + acpi_status status;
>> +
>> + for (k = 0; k < info_obj->package.count; ++k) {
>> + union acpi_object *obj = &info_obj->package.elements[k];
>> + union acpi_object *obj_new;
>> +
>> + list = &lpi_constraints_table[lpi_constraints_table_size];
>> + list->min_dstate = -1;
>> +
>> + obj_new = &obj[k];
>> + switch (k) {
>> + case 0:
>> + dev_info.enabled = obj->integer.value;
>> + break;
>> + case 1:
>> + dev_info.name = obj->string.pointer;
>> + break;
>> + case 2:
>> + dev_info.function_states = obj->integer.value;
>> + break;
>> + case 3:
>> + dev_info.min_dstate = obj->integer.value;
>> + break;
>> + }
>> +
>> + if (!dev_info.enabled || !dev_info.name ||
>> + !dev_info.min_dstate)
>> + continue;
>> +
>> + status = acpi_get_handle(NULL, dev_info.name,
>> + &list->handle);
>> + if (ACPI_FAILURE(status))
>> + continue;
>> +
>> + acpi_handle_info(lps0_device_handle,
>> + "Name:%s\n", dev_info.name);
>> +
>> + list->min_dstate = dev_info.min_dstate;
>> +
>> + if (list->min_dstate < 0) {
>> + acpi_handle_info(lps0_device_handle,
>> + "Incomplete constraint defined\n");
>> + continue;
>> + }
>> + }
>> + lpi_constraints_table_size++;
>> + }
>> + }
>> + }
>> +
>> + acpi_handle_info(lps0_device_handle, "LPI: constraints list end\n");
>> +
>> +free_acpi_buffer:
>> + ACPI_FREE(out_obj);
>> +}
>>
>> static void lpi_device_get_constraints(void)
>> {
>> @@ -883,13 +1004,22 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func)
>> if (!(lps0_dsm_func_mask & (1 << func)))
>> return;
>>
>> - out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
>> + out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, rev_id, func, NULL);
>> ACPI_FREE(out_obj);
>>
>> acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
>> func, out_obj ? "successful" : "failed");
>> }
>>
>> +static bool acpi_match_vendor_name(void)
>> +{
>> +#ifdef CONFIG_X86
>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> + return true;
>> +#endif
>> + return false;
>> +}
>
> Well, I still don't like this (explicit CONFIG_X86 dependencies are
> not welcome in this file), but arguably the damage was done before
> already and there is a bunch of X86-specific code in here anyway, so
> I'm going to take your patch provisionally and rearrange the code on
> top of it so as to relocate the x86-specific part of it into
> acpi/x86/.
>
> Thanks!
Thank you Rafael!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle
2020-11-25 6:45 ` Shyam Sundar S K
@ 2020-12-10 18:56 ` Rafael J. Wysocki
0 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2020-12-10 18:56 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, ACPI Devel Maling List
On Wed, Nov 25, 2020 at 7:45 AM Shyam Sundar S K
<Shyam-sundar.S-k@amd.com> wrote:
>
>
>
> On 11/23/2020 10:15 PM, Rafael J. Wysocki wrote:
> > [CAUTION: External Email]
> >
> > On Fri, Nov 20, 2020 at 7:10 AM Shyam Sundar S K
> > <Shyam-sundar.S-k@amd.com> wrote:
> >>
> >> Initial support for S2Idle based on the Intel implementation [1] does not
> >> work for AMD as the BIOS implementation for ACPI methods like the _DSM
> >> are not standardized.
> >>
> >> So, the way in which the UUID's were parsed and the ACPI packages were
> >> retrieved out of the ACPI objects are not the same between Intel and AMD.
> >>
> >> This patch adds AMD support for S2Idle to parse the UUID, evaluate the
> >> _DSM methods, preparing the Idle constraint list etc.
> >>
> >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fsites%2Fdefault%2Ffiles%2Fresources%2FIntel_ACPI_Low_Power_S0_Idle.pdf&data=04%7C01%7CShyam-sundar.S-k%40amd.com%7C640970e0f42a4fd53a1408d88fcf25d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637417467581911776%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=xPFj5OymQkr3xEaU8qQD1y8O95XCszUblRRuknNvD7I%3D&reserved=0 # [1]
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >> v2:(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-acpi%2Fmsg97104.html&data=04%7C01%7CShyam-sundar.S-k%40amd.com%7C640970e0f42a4fd53a1408d88fcf25d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637417467581911776%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=r5whv52ruKVbFgsV1qEuWKeePaUQhjXnxQwF7izFRf4%3D&reserved=0)
> >> - Remove default case in switch.
> >> - Add a routine acpi_match_vendor_name() required when building on a
> >> non-x86 based environment.
> >> - Fixed spelling mistakes.
> >> ---
> >> drivers/acpi/sleep.c | 166 ++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 157 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> >> index aff13bf4d947..b929fd0d2e04 100644
> >> --- a/drivers/acpi/sleep.c
> >> +++ b/drivers/acpi/sleep.c
> >> @@ -710,6 +710,11 @@ static const struct acpi_device_id lps0_device_ids[] = {
> >> #define ACPI_LPS0_ENTRY 5
> >> #define ACPI_LPS0_EXIT 6
> >>
> >> +/* AMD */
> >> +#define ACPI_LPS0_DSM_UUID_AMD "e3f32452-febc-43ce-9039-932122d37721"
> >> +#define ACPI_LPS0_SCREEN_OFF_AMD 4
> >> +#define ACPI_LPS0_SCREEN_ON_AMD 5
> >> +
> >> static acpi_handle lps0_device_handle;
> >> static guid_t lps0_dsm_guid;
> >> static char lps0_dsm_func_mask;
> >> @@ -733,8 +738,124 @@ struct lpi_constraints {
> >> int min_dstate;
> >> };
> >>
> >> +/* AMD */
> >> +/* Device constraint entry structure */
> >> +struct lpi_device_info_amd {
> >> + int revision;
> >> + int count;
> >> + union acpi_object *package;
> >> +};
> >> +
> >> +/* Constraint package structure */
> >> +struct lpi_device_constraint_amd {
> >> + char *name;
> >> + int enabled;
> >> + int function_states;
> >> + int min_dstate;
> >> +};
> >> +
> >> static struct lpi_constraints *lpi_constraints_table;
> >> static int lpi_constraints_table_size;
> >> +static int rev_id;
> >> +
> >> +static void lpi_device_get_constraints_amd(void)
> >> +{
> >> + union acpi_object *out_obj;
> >> + int i, j, k;
> >> +
> >> + out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
> >> + 1, ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
> >> + NULL, ACPI_TYPE_PACKAGE);
> >> +
> >> + if (!out_obj)
> >> + return;
> >> +
> >> + acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
> >> + out_obj ? "successful" : "failed");
> >> +
> >> + for (i = 0; i < out_obj->package.count; i++) {
> >> + union acpi_object *package = &out_obj->package.elements[i];
> >> + struct lpi_device_info_amd info = { };
> >> +
> >> + if (package->type == ACPI_TYPE_INTEGER) {
> >> + switch (i) {
> >> + case 0:
> >> + info.revision = package->integer.value;
> >> + break;
> >> + case 1:
> >> + info.count = package->integer.value;
> >> + break;
> >> + }
> >> + } else if (package->type == ACPI_TYPE_PACKAGE) {
> >> + lpi_constraints_table = kcalloc(package->package.count,
> >> + sizeof(*lpi_constraints_table),
> >> + GFP_KERNEL);
> >> +
> >> + if (!lpi_constraints_table)
> >> + goto free_acpi_buffer;
> >> +
> >> + acpi_handle_info(lps0_device_handle,
> >> + "LPI: constraints list begin:\n");
> >> +
> >> + for (j = 0; j < package->package.count; ++j) {
> >> + union acpi_object *info_obj = &package->package.elements[j];
> >> + struct lpi_device_constraint_amd dev_info = {};
> >> + struct lpi_constraints *list;
> >> + acpi_status status;
> >> +
> >> + for (k = 0; k < info_obj->package.count; ++k) {
> >> + union acpi_object *obj = &info_obj->package.elements[k];
> >> + union acpi_object *obj_new;
> >> +
> >> + list = &lpi_constraints_table[lpi_constraints_table_size];
> >> + list->min_dstate = -1;
> >> +
> >> + obj_new = &obj[k];
> >> + switch (k) {
> >> + case 0:
> >> + dev_info.enabled = obj->integer.value;
> >> + break;
> >> + case 1:
> >> + dev_info.name = obj->string.pointer;
> >> + break;
> >> + case 2:
> >> + dev_info.function_states = obj->integer.value;
> >> + break;
> >> + case 3:
> >> + dev_info.min_dstate = obj->integer.value;
> >> + break;
> >> + }
> >> +
> >> + if (!dev_info.enabled || !dev_info.name ||
> >> + !dev_info.min_dstate)
> >> + continue;
> >> +
> >> + status = acpi_get_handle(NULL, dev_info.name,
> >> + &list->handle);
> >> + if (ACPI_FAILURE(status))
> >> + continue;
> >> +
> >> + acpi_handle_info(lps0_device_handle,
> >> + "Name:%s\n", dev_info.name);
> >> +
> >> + list->min_dstate = dev_info.min_dstate;
> >> +
> >> + if (list->min_dstate < 0) {
> >> + acpi_handle_info(lps0_device_handle,
> >> + "Incomplete constraint defined\n");
> >> + continue;
> >> + }
> >> + }
> >> + lpi_constraints_table_size++;
> >> + }
> >> + }
> >> + }
> >> +
> >> + acpi_handle_info(lps0_device_handle, "LPI: constraints list end\n");
> >> +
> >> +free_acpi_buffer:
> >> + ACPI_FREE(out_obj);
> >> +}
> >>
> >> static void lpi_device_get_constraints(void)
> >> {
> >> @@ -883,13 +1004,22 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func)
> >> if (!(lps0_dsm_func_mask & (1 << func)))
> >> return;
> >>
> >> - out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
> >> + out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, rev_id, func, NULL);
> >> ACPI_FREE(out_obj);
> >>
> >> acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
> >> func, out_obj ? "successful" : "failed");
> >> }
> >>
> >> +static bool acpi_match_vendor_name(void)
> >> +{
> >> +#ifdef CONFIG_X86
> >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> >> + return true;
> >> +#endif
> >> + return false;
> >> +}
> >
> > Well, I still don't like this (explicit CONFIG_X86 dependencies are
> > not welcome in this file), but arguably the damage was done before
> > already and there is a bunch of X86-specific code in here anyway, so
> > I'm going to take your patch provisionally and rearrange the code on
> > top of it so as to relocate the x86-specific part of it into
> > acpi/x86/.
> >
> > Thanks!
>
> Thank you Rafael!
I won't be able to take care of this before the start of the 5.11
merge window, so I'll do my best to get to it during the merge window
so as to be able to still get it into 5.11-rc1.
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-10 19:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 6:07 [PATCH v2] ACPI: PM: s2idle: Add AMD support to handle _DSM during S2Idle Shyam Sundar S K
2020-11-23 16:45 ` Rafael J. Wysocki
2020-11-25 6:45 ` Shyam Sundar S K
2020-12-10 18:56 ` Rafael J. Wysocki
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).