ACPI: D3 states cleanup
Subsystems (3)
PATCH v3 1/1 ACPI: D3 states cleanup
There are two ACPI D3 states defined now:ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
But the uses of these states are not clear/correct in some code. For example, some code refer ACPI_STATE_D3 as D3hot and others refer it as D3cold.
This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state. D3hot is only valid if _PR3 present.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
---
v3: D3hot is only valid if _PR3 present
drivers/acpi/power.c | 2 +- drivers/acpi/scan.c | 17 +++++++---------- drivers/pci/pci-acpi.c | 4 ++-- include/acpi/actypes.h | 7 ++++--- 4 files changed, 14 insertions(+), 16 deletions(-)
drivers/acpi/power.c +1 -1
@@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)* We know a device's inferred power state when all the resources * required for a given D-state are 'on'. */ -for (i =ACPI_STATE_D0 ; i <ACPI_STATE_D3 ; i++) { +for (i =ACPI_STATE_D0 ; i <ACPI_STATE_D3_HOT ; i++) { list = &device->power.states[i].resources;if (list->count <1 )continue ;
drivers/acpi/scan.c +7 -10
@@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)/* * Enumerate supported power management states */ -for (i =ACPI_STATE_D0 ; i <=ACPI_STATE_D3 ; i++) { +for (i =ACPI_STATE_D0 ; i <=ACPI_STATE_D3_HOT ; i++) {struct acpi_device_power_state *ps = &device->power.states[i];char object_name[5 ] = {'_' ,'P' ,'R' ,'0' + i,'\0' };
@@ -884,21 +884,18 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)acpi_bus_add_power_resource (ps->resources.handles[j]); } -/* The exist of _PR3 indicates D3Cold support */ -if (i ==ACPI_STATE_D3 ) { - status =acpi_get_handle (device->handle, object_name, &handle); -if (ACPI_SUCCESS (status)) - device->power.states[ACPI_STATE_D3_COLD ].flags.valid =1 ; - } -/* Evaluate "_PSx" to see if we can do explicit sets */ object_name[2 ] ='S' ; status =acpi_get_handle (device->handle, object_name, &handle);if (ACPI_SUCCESS (status)) ps->flags.explicit_set =1 ; -/* State is valid if we have some power control */ -if (ps->resources.count || ps->flags.explicit_set) +/* +* State is valid if there are means to put the device into it. +* D3hot is only valid if _PR3 present. +*/ +if (ps->resources.count || + (ps->flags.explicit_set && i <ACPI_STATE_D3_HOT )) ps->flags.valid =1 ; ps->power =-1 ;/* Unknown - driver assigned */
drivers/pci/pci-acpi.c +2 -2
@@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)return PCI_D1 ;case ACPI_STATE_D2 :return PCI_D2 ; -case ACPI_STATE_D3 : +case ACPI_STATE_D3_HOT :return PCI_D3hot;case ACPI_STATE_D3_COLD :return PCI_D3cold;
@@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) [PCI_D0] = ACPI_STATE_D0, [PCI_D1] = ACPI_STATE_D1, [PCI_D2] = ACPI_STATE_D2, - [PCI_D3hot] = ACPI_STATE_D3, + [PCI_D3hot] = ACPI_STATE_D3_HOT, [PCI_D3cold] = ACPI_STATE_D3 };int error = -EINVAL ;
include/acpi/actypes.h +4 -3
@@ -499,9 +499,10 @@ typedef u64 acpi_integer;#define ACPI_STATE_D0 (u8) 0#define ACPI_STATE_D1 (u8) 1#define ACPI_STATE_D2 (u8) 2 -#define ACPI_STATE_D3 (u8) 3 -#define ACPI_STATE_D3_COLD (u8) 4 -#define ACPI_D_STATES_MAX ACPI_STATE_D3_COLD +#define ACPI_STATE_D3_HOT (u8) 3 +#define ACPI_STATE_D3 (u8) 4 +#define ACPI_STATE_D3_COLD ACPI_STATE_D3 +#define ACPI_D_STATES_MAX ACPI_STATE_D3#define ACPI_D_STATE_COUNT 5#define ACPI_STATE_C0 (u8) 0
Signature · 1 line
1.7.2.5
Quoted 12 lines · Lin Ming
There are two ACPI D3 states defined now:
ACPI_STATE_D3andACPI_STATE_D3_COLD.But the uses of these states are not clear/correct in some code. For example, some code refer
ACPI_STATE_D3as D3hot and others refer it as D3cold.This patch introduces
ACPI_STATE_D3_HOTto refer to ACPI D3hot state. D3hot is only valid if_PR3present.
And changesACPI_STATE_D3to refer to ACPI D3cold state only. Also redefinesACPI_STATE_D3_COLDthe same asACPI_STATE_D3.
Thanks,
Aaron
Quoted 112 lines
Signed-off-by Lin Ming <[email protected]>---
v3: D3hot is only valid if
_PR3presentdrivers/acpi/power.c | 2 +- drivers/acpi/scan.c | 17 +++++++---------- drivers/pci/pci-acpi.c | 4 ++-- include/acpi/actypes.h | 7 ++++--- 4 files changed, 14 insertions(+), 16 deletions(-)drivers/acpi/power.c +1
-1@@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)* We know a device's inferred power state when all the resources * required for a given D-state are 'on'. */ -for (i =ACPI_STATE_D0 ; i <ACPI_STATE_D3 ; i++) { +for (i =ACPI_STATE_D0 ; i <ACPI_STATE_D3_HOT ; i++) { list = &device->power.states[i].resources;if (list->count <1 )continue ;drivers/acpi/scan.c +7
-10@@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)/* * Enumerate supported power management states */ -for (i =ACPI_STATE_D0 ; i <=ACPI_STATE_D3 ; i++) { +for (i =ACPI_STATE_D0 ; i <=ACPI_STATE_D3_HOT ; i++) {struct acpi_device_power_state *ps = &device->power.states[i];char object_name[5 ] = {'_' ,'P' ,'R' ,'0' + i,'\0' };@@ -884,21 +884,18 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)acpi_bus_add_power_resource (ps->resources.handles[j]); } -/* The exist of _PR3 indicates D3Cold support */ -if (i ==ACPI_STATE_D3 ) { - status =acpi_get_handle (device->handle, object_name, &handle); -if (ACPI_SUCCESS (status)) - device->power.states[ACPI_STATE_D3_COLD ].flags.valid =1 ; - } -/* Evaluate "_PSx" to see if we can do explicit sets */ object_name[2 ] ='S' ; status =acpi_get_handle (device->handle, object_name, &handle);if (ACPI_SUCCESS (status)) ps->flags.explicit_set =1 ; -/* State is valid if we have some power control */ -if (ps->resources.count || ps->flags.explicit_set) +/* +* State is valid if there are means to put the device into it. +* D3hot is only valid if _PR3 present. +*/ +if (ps->resources.count || + (ps->flags.explicit_set && i <ACPI_STATE_D3_HOT )) ps->flags.valid =1 ; ps->power =-1 ;/* Unknown - driver assigned */ drivers/pci/pci-acpi.c +2
-2@@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)return PCI_D1 ;case ACPI_STATE_D2 :return PCI_D2 ; -case ACPI_STATE_D3 : +case ACPI_STATE_D3_HOT :return PCI_D3hot;case ACPI_STATE_D3_COLD :return PCI_D3cold;@@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) [PCI_D0] = ACPI_STATE_D0, [PCI_D1] = ACPI_STATE_D1, [PCI_D2] = ACPI_STATE_D2, - [PCI_D3hot] = ACPI_STATE_D3, + [PCI_D3hot] = ACPI_STATE_D3_HOT, [PCI_D3cold] = ACPI_STATE_D3 };int error = -EINVAL ;include/acpi/actypes.h +4
-3@@ -499,9 +499,10 @@ typedef u64 acpi_integer;#define ACPI_STATE_D0 (u8) 0#define ACPI_STATE_D1 (u8) 1#define ACPI_STATE_D2 (u8) 2 -#define ACPI_STATE_D3 (u8) 3 -#define ACPI_STATE_D3_COLD (u8) 4 -#define ACPI_D_STATES_MAX ACPI_STATE_D3_COLD +#define ACPI_STATE_D3_HOT (u8) 3 +#define ACPI_STATE_D3 (u8) 4 +#define ACPI_STATE_D3_COLD ACPI_STATE_D3 +#define ACPI_D_STATES_MAX ACPI_STATE_D3#define ACPI_D_STATE_COUNT 5#define ACPI_STATE_C0 (u8) 0Signature · 1 line
1.7.2.5
Quoted 14 lines · Aaron Lu
Quoted 12 lines · Lin Ming
There are two ACPI D3 states defined now:
ACPI_STATE_D3andACPI_STATE_D3_COLD.But the uses of these states are not clear/correct in some code. For example, some code refer
ACPI_STATE_D3as D3hot and others refer it as D3cold.This patch introduces
ACPI_STATE_D3_HOTto refer to ACPI D3hot state. D3hot is only valid if_PR3present.
And changesACPI_STATE_D3to refer to ACPI D3cold state only. Also redefinesACPI_STATE_D3_COLDthe same asACPI_STATE_D3.
Quoted 6 lines
Reviewed-by Aaron Lu <[email protected]>Thanks,
AaronQuoted 1 line
Len,
Will you apply this patch?
Thanks,
Lin Ming
Quoted 104 lines
Quoted 104 lines
Signed-off-by Lin Ming <[email protected]>---
v3: D3hot is only valid if
_PR3presentdrivers/acpi/power.c | 2 +-
drivers/acpi/scan.c | 17 +++++++----------
drivers/pci/pci-acpi.c | 4 ++--
include/acpi/actypes.h | 7 ++++---4 files changed, 14 insertions(+), 16 deletions(-)drivers/acpi/power.c +1
-1@@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)* We know a device's inferred power state when all the resources * required for a given D-state are 'on'. */ - for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) { + for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) { list = &device->power.states[i].resources; if (list->count < 1) continue;drivers/acpi/scan.c +7
-10@@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device) /* * Enumerate supported power management states */ - for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) { + for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) { struct acpi_device_power_state *ps = &device->power.states[i]; char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };@@ -884,21 +884,18 @@ static int acpi_bus_get_power_flags(struct acpi_device *device) acpi_bus_add_power_resource(ps->resources.handles[j]); } - /* The exist of _PR3 indicates D3Cold support */ - if (i == ACPI_STATE_D3) { - status = acpi_get_handle(device->handle, object_name, &handle); - if (ACPI_SUCCESS(status)) - device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1; - } - /* Evaluate "_PSx" to see if we can do explicit sets */ object_name[2] = 'S'; status = acpi_get_handle(device->handle, object_name, &handle); if (ACPI_SUCCESS(status)) ps->flags.explicit_set = 1; - /* State is valid if we have some power control */ - if (ps->resources.count || ps->flags.explicit_set) + /* + * State is valid if there are means to put the device into it. + * D3hot is only valid if _PR3 present. + */ + if (ps->resources.count || + (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT)) ps->flags.valid = 1;
ps->power= -1; /* Unknown - driver assigned */drivers/pci/pci-acpi.c +0
-0@@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) return PCI_D1;case
ACPI_STATE_D2:
returnPCI_D2;
- caseACPI_STATE_D3:
+ caseACPI_STATE_D3_HOT:
returnPCI_D3hot;
caseACPI_STATE_D3_COLD:
returnPCI_D3cold;drivers/pci/pci-acpi.c +1
-1@@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) [PCI_D0] = ACPI_STATE_D0, [PCI_D1] = ACPI_STATE_D1, [PCI_D2] = ACPI_STATE_D2, - [PCI_D3hot] = ACPI_STATE_D3, + [PCI_D3hot] = ACPI_STATE_D3_HOT,[
PCI_D3cold] =ACPI_STATE_D3
};
int error =-EINVAL;include/acpi/actypes.h +4
-3@@ -499,9 +499,10 @@ typedef u64 acpi_integer; #define ACPI_STATE_D0 (u8) 0 #define ACPI_STATE_D1 (u8) 1 #define ACPI_STATE_D2 (u8) 2 -#define ACPI_STATE_D3 (u8) 3 -#define ACPI_STATE_D3_COLD (u8) 4 -#define ACPI_D_STATES_MAX ACPI_STATE_D3_COLD +#define ACPI_STATE_D3_HOT (u8) 3 +#define ACPI_STATE_D3 (u8) 4 +#define ACPI_STATE_D3_COLD ACPI_STATE_D3 +#define ACPI_D_STATES_MAX ACPI_STATE_D3 #define ACPI_D_STATE_COUNT 5
Quoted 12 lines
Quoted 7 lines
include/acpi/actypes.h +0
-1#define ACPI_STATE_C0 (u8) 0 --1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Ming,
Quoted 11 lines · Lin Ming
There are two ACPI D3 states defined now:
ACPI_STATE_D3andACPI_STATE_D3_COLD.But the uses of these states are not clear/correct in some code. For example, some code refer
ACPI_STATE_D3as D3hot and others refer it as D3cold.This patch introduces
ACPI_STATE_D3_HOTto refer to ACPI D3hot state. D3hot is only valid if_PR3present.
And changesACPI_STATE_D3to refer to ACPI D3cold state only. Also redefinesACPI_STATE_D3_COLDthe same asACPI_STATE_D3.
The changelog should say why we make the change instead of how we make the change. The code itself can tell me how we have done well. I prefer to see the description about
- the history of ACPI_STATE_D3, ACPI_STATE_D3_HOT, ACPI_STATE_D3_COLD
- if no _PR3, _PS3 may put device into D3_HOT or D3_COLD, decided by platform. But we will assume _PS3 will put device into D3_COLD, because it is safe to treat device in D3_HOT as D3_COLD, but not in contrary way.
And I think it will be more clear if we list the power state supporting under various combination, for example,
- has _PS3, no _PR3
- Valid states: D3_COLD
- Set power state
- D3_COLD: exec _PS3
Quoted 54 lines
Signed-off-by Lin Ming <[email protected]>---
v3: D3hot is only valid if
_PR3presentdrivers/acpi/power.c | 2 +-
drivers/acpi/scan.c | 17 +++++++----------
drivers/pci/pci-acpi.c | 4 ++--
include/acpi/actypes.h | 7 ++++---4 files changed, 14 insertions(+), 16 deletions(-)drivers/acpi/power.c +1
-1@@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)* We know a device's inferred power state when all the resources * required for a given D-state are 'on'. */ - for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) { + for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) { list = &device->power.states[i].resources; if (list->count < 1) continue;drivers/acpi/scan.c +1
-8@@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device) /* * Enumerate supported power management states */ - for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) { + for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) { struct acpi_device_power_state *ps = &device->power.states[i]; char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };@@ -884,21 +884,18 @@ static int acpi_bus_get_power_flags(struct acpi_device *device) acpi_bus_add_power_resource(ps->resources.handles[j]); } - /* The exist of _PR3 indicates D3Cold support */ - if (i == ACPI_STATE_D3) { - status = acpi_get_handle(device->handle, object_name, &handle); - if (ACPI_SUCCESS(status)) - device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1; - } - /* Evaluate "_PSx" to see if we can do explicit sets */ object_name[2] = 'S'; status = acpi_get_handle(device->handle, object_name, &handle); if (ACPI_SUCCESS(status)) ps->flags.explicit_set = 1;
drivers/acpi/scan.c static int acpi_bus_get_power_flags(struct acpi_device *device) · line 899
Quoted 10 lines
drivers/acpi/scan.c +6
-2- /* State is valid if we have some power control */ - if (ps->resources.count || ps->flags.explicit_set) + /* + * State is valid if there are means to put the device into it. + * D3hot is only valid if _PR3 present. + */ + if (ps->resources.count || + (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT)) ps->flags.valid = 1;
I think we should add some comments here about why we do that. Why D3_HOT is not valid if only _PS3 presents.
Quoted 31 lines
ps->power = -1; /* Unknown - driver assigned */
drivers/pci/pci-acpi.c +0
-0@@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) return PCI_D1;case
ACPI_STATE_D2:
returnPCI_D2;
- caseACPI_STATE_D3:
+ caseACPI_STATE_D3_HOT:
returnPCI_D3hot;
caseACPI_STATE_D3_COLD:
returnPCI_D3cold;drivers/pci/pci-acpi.c +1
-1@@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) [PCI_D0] = ACPI_STATE_D0, [PCI_D1] = ACPI_STATE_D1, [PCI_D2] = ACPI_STATE_D2, - [PCI_D3hot] = ACPI_STATE_D3, + [PCI_D3hot] = ACPI_STATE_D3_HOT,[PCI_D3cold] =
ACPI_STATE_D3
};
int error =-EINVAL;include/acpi/actypes.h +0
-0@@ -499,9 +499,10 @@ typedef u64 acpi_integer; #define ACPI_STATE_D0 (u8) 0 #define ACPI_STATE_D1 (u8) 1 #define ACPI_STATE_D2 (u8) 2
include/acpi/actypes.h typedef u64 acpi_integer; · line 508
Quoted 10 lines
include/acpi/actypes.h +4
-3-#define ACPI_STATE_D3 (u8) 3 -#define ACPI_STATE_D3_COLD (u8) 4 -#define ACPI_D_STATES_MAX ACPI_STATE_D3_COLD +#define ACPI_STATE_D3_HOT (u8) 3 +#define ACPI_STATE_D3 (u8) 4 +#define ACPI_STATE_D3_COLD ACPI_STATE_D3 +#define ACPI_D_STATES_MAX ACPI_STATE_D3 #define ACPI_D_STATE_COUNT 5 #define ACPI_STATE_C0 (u8) 0
Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Applied using updated check-in comment below.
thanks,
-Len Brown, Intel Open Source Technology Center
From: Lin Ming <[email protected]>
Subject: [PATCH] ACPI: Fix D3hot v D3cold confusion
Before this patch, ACPI_STATE_D3 incorrectly referenced D3hot in some places, but D3cold in other places.
After this patch, ACPI_STATE_D3 always means ACPI_STATE_D3_COLD; and all references to D3hot use ACPI_STATE_D3_HOT.
ACPI's _PR3 method is used to enter both D3hot and D3cold states. What distinguishes D3hot from D3cold is the presence _PR3 (Power Resources for D3hot) If these resources are all ON, then the state is D3hot. If _PR3 is not present,
or all _PR0 resources for the devices are OFF,
then the state is D3cold.
This patch applies after Linux-3.4-rc1.
A future syntax cleanup may remove ACPI_STATE_D3 to emphasize that it always means ACPI_STATE_D3_COLD.
--- drivers/acpi/power.c | 2 +- drivers/acpi/scan.c | 17 +++++++---------- drivers/pci/pci-acpi.c | 4 ++-- include/acpi/actypes.h | 7 ++++--- 4 files changed, 14 insertions(+), 16 deletions(-)