* [PATCH v2] ACPI: D3 states cleanup @ 2012-04-20 1:19 Lin Ming 2012-04-20 2:23 ` Aaron Lu 0 siblings, 1 reply; 15+ messages in thread From: Lin Ming @ 2012-04-20 1:19 UTC (permalink / raw) To: Rafael J. Wysocki, lenb Cc: linux-acpi, linux-kernel, Zhang Rui, Huang, Ying, Aaron Lu 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. And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. Acked-by: Rafael J. Wysocki <rjw@sisk.pl> Signed-off-by: Lin Ming <ming.m.lin@intel.com> --- drivers/acpi/power.c | 2 +- drivers/acpi/scan.c | 9 +-------- drivers/pci/pci-acpi.c | 4 ++-- include/acpi/actypes.h | 7 ++++--- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c index 7049a7d..330bb4d 100644 --- a/drivers/acpi/power.c +++ b/drivers/acpi/power.c @@ -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; diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 767e2dc..fbf38ad 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -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,13 +884,6 @@ 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); diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 0f150f2..1929c0c 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -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; diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index eba6604..e8bcc47 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -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 -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 1:19 [PATCH v2] ACPI: D3 states cleanup Lin Ming @ 2012-04-20 2:23 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 2:23 UTC (permalink / raw) To: Lin Ming, Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying Hi, On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > 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. > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > With this patch now, if a device has _PS3, then we will set its D3hot flag valid. This doesn't feel right to me, since per our discussion the other day, we should assume _PS3 will put the device into D3cold. Or do you mean: if _PS3 is available, then both D3hot and D3cold is valid from the perspective of acpi, it is the individual driver's responsibility to decide which state is actually valid and will be used. -Aaron > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > Signed-off-by: Lin Ming <ming.m.lin@intel.com> > --- > drivers/acpi/power.c | 2 +- > drivers/acpi/scan.c | 9 +-------- > drivers/pci/pci-acpi.c | 4 ++-- > include/acpi/actypes.h | 7 ++++--- > 4 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > index 7049a7d..330bb4d 100644 > --- a/drivers/acpi/power.c > +++ b/drivers/acpi/power.c > @@ -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; > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 767e2dc..fbf38ad 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -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,13 +884,6 @@ 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); > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 0f150f2..1929c0c 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -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; > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h > index eba6604..e8bcc47 100644 > --- a/include/acpi/actypes.h > +++ b/include/acpi/actypes.h > @@ -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 > -- > 1.7.2.5 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup @ 2012-04-20 2:23 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 2:23 UTC (permalink / raw) To: Lin Ming, Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying Hi, On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > 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. > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > With this patch now, if a device has _PS3, then we will set its D3hot flag valid. This doesn't feel right to me, since per our discussion the other day, we should assume _PS3 will put the device into D3cold. Or do you mean: if _PS3 is available, then both D3hot and D3cold is valid from the perspective of acpi, it is the individual driver's responsibility to decide which state is actually valid and will be used. -Aaron > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > Signed-off-by: Lin Ming <ming.m.lin@intel.com> > --- > drivers/acpi/power.c | 2 +- > drivers/acpi/scan.c | 9 +-------- > drivers/pci/pci-acpi.c | 4 ++-- > include/acpi/actypes.h | 7 ++++--- > 4 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > index 7049a7d..330bb4d 100644 > --- a/drivers/acpi/power.c > +++ b/drivers/acpi/power.c > @@ -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; > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 767e2dc..fbf38ad 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -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,13 +884,6 @@ 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); > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 0f150f2..1929c0c 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -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; > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h > index eba6604..e8bcc47 100644 > --- a/include/acpi/actypes.h > +++ b/include/acpi/actypes.h > @@ -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 > -- > 1.7.2.5 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 2:23 ` Aaron Lu (?) @ 2012-04-20 3:19 ` Lin Ming 2012-04-20 5:23 ` Aaron Lu -1 siblings, 1 reply; 15+ messages in thread From: Lin Ming @ 2012-04-20 3:19 UTC (permalink / raw) To: Aaron Lu Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote: > Hi, > > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > > 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. > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > With this patch now, if a device has _PS3, then we will set its D3hot > flag valid. This doesn't feel right to me, since per our discussion the > other day, we should assume _PS3 will put the device into D3cold. > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > valid from the perspective of acpi, it is the individual driver's > responsibility to decide which state is actually valid and will be used. Right. ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. Lin Ming > > -Aaron > > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com> > > --- > > drivers/acpi/power.c | 2 +- > > drivers/acpi/scan.c | 9 +-------- > > drivers/pci/pci-acpi.c | 4 ++-- > > include/acpi/actypes.h | 7 ++++--- > > 4 files changed, 8 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > > index 7049a7d..330bb4d 100644 > > --- a/drivers/acpi/power.c > > +++ b/drivers/acpi/power.c > > @@ -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; > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > index 767e2dc..fbf38ad 100644 > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -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,13 +884,6 @@ 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); > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index 0f150f2..1929c0c 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -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; > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h > > index eba6604..e8bcc47 100644 > > --- a/include/acpi/actypes.h > > +++ b/include/acpi/actypes.h > > @@ -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 > > -- > > 1.7.2.5 > > > > > > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 3:19 ` Lin Ming @ 2012-04-20 5:23 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 5:23 UTC (permalink / raw) To: Lin Ming Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote: > On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote: > > Hi, > > > > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > > > 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. > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > flag valid. This doesn't feel right to me, since per our discussion the > > other day, we should assume _PS3 will put the device into D3cold. > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > valid from the perspective of acpi, it is the individual driver's > > responsibility to decide which state is actually valid and will be used. > > Right. > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > I mean, if _PS3 is available, can we say D3hot is valid? -Aaron > Lin Ming > > > > > -Aaron > > > > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > > > Signed-off-by: Lin Ming <ming.m.lin@intel.com> > > > --- > > > drivers/acpi/power.c | 2 +- > > > drivers/acpi/scan.c | 9 +-------- > > > drivers/pci/pci-acpi.c | 4 ++-- > > > include/acpi/actypes.h | 7 ++++--- > > > 4 files changed, 8 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > > > index 7049a7d..330bb4d 100644 > > > --- a/drivers/acpi/power.c > > > +++ b/drivers/acpi/power.c > > > @@ -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; > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > index 767e2dc..fbf38ad 100644 > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -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,13 +884,6 @@ 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); > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > > index 0f150f2..1929c0c 100644 > > > --- a/drivers/pci/pci-acpi.c > > > +++ b/drivers/pci/pci-acpi.c > > > @@ -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; > > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h > > > index eba6604..e8bcc47 100644 > > > --- a/include/acpi/actypes.h > > > +++ b/include/acpi/actypes.h > > > @@ -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 > > > -- > > > 1.7.2.5 > > > > > > > > > > > > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup @ 2012-04-20 5:23 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 5:23 UTC (permalink / raw) To: Lin Ming Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote: > On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote: > > Hi, > > > > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > > > 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. > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > flag valid. This doesn't feel right to me, since per our discussion the > > other day, we should assume _PS3 will put the device into D3cold. > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > valid from the perspective of acpi, it is the individual driver's > > responsibility to decide which state is actually valid and will be used. > > Right. > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > I mean, if _PS3 is available, can we say D3hot is valid? -Aaron > Lin Ming > > > > > -Aaron > > > > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > > > Signed-off-by: Lin Ming <ming.m.lin@intel.com> > > > --- > > > drivers/acpi/power.c | 2 +- > > > drivers/acpi/scan.c | 9 +-------- > > > drivers/pci/pci-acpi.c | 4 ++-- > > > include/acpi/actypes.h | 7 ++++--- > > > 4 files changed, 8 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > > > index 7049a7d..330bb4d 100644 > > > --- a/drivers/acpi/power.c > > > +++ b/drivers/acpi/power.c > > > @@ -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; > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > index 767e2dc..fbf38ad 100644 > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -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,13 +884,6 @@ 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); > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > > index 0f150f2..1929c0c 100644 > > > --- a/drivers/pci/pci-acpi.c > > > +++ b/drivers/pci/pci-acpi.c > > > @@ -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; > > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h > > > index eba6604..e8bcc47 100644 > > > --- a/include/acpi/actypes.h > > > +++ b/include/acpi/actypes.h > > > @@ -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 > > > -- > > > 1.7.2.5 > > > > > > > > > > > > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 5:23 ` Aaron Lu (?) @ 2012-04-20 5:37 ` Lin Ming 2012-04-20 7:47 ` Aaron Lu -1 siblings, 1 reply; 15+ messages in thread From: Lin Ming @ 2012-04-20 5:37 UTC (permalink / raw) To: Aaron Lu Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, 2012-04-20 at 13:23 +0800, Aaron Lu wrote: > On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote: > > On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote: > > > Hi, > > > > > > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote: > > > > 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. > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > > flag valid. This doesn't feel right to me, since per our discussion the > > > other day, we should assume _PS3 will put the device into D3cold. > > > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > > valid from the perspective of acpi, it is the individual driver's > > > responsibility to decide which state is actually valid and will be used. > > > > Right. > > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > > > > I mean, if _PS3 is available, can we say D3hot is valid? Yes. Lin Ming ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 5:37 ` Lin Ming @ 2012-04-20 7:47 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 7:47 UTC (permalink / raw) To: Lin Ming, Rafael J. Wysocki Cc: Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying Hi, On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: > > > > > 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. > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > > > flag valid. This doesn't feel right to me, since per our discussion the > > > > other day, we should assume _PS3 will put the device into D3cold. > > > > > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > > > valid from the perspective of acpi, it is the individual driver's > > > > responsibility to decide which state is actually valid and will be used. > > > > > > Right. > > > > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > > > > > > > I mean, if _PS3 is available, can we say D3hot is valid? > > Yes. > OK, now I'm confused... First, let me try to clarify the meaning of acpi power state's valid flag. By valid, I suppose it means the device can be in that state, instead of we have a way to program this device to go into that state. e.g. D0 is valid means the device can be in D0 state, and D3_cold is valid means the device can be in D3_cold state. We unconditionally set these two states as valid, because we know every device supports these two states. But we might not be able to put the device into that state in software, since we might not have _PS0 or _PS3 control methods for it. And if we do have the _PSx or _PRx control methods, we knows we have a way to put the device into that state, and hence the device should be able to support that power state, so we will set that state as valid too. Is this correct? For D3hot, obviously not all device supports this state, so we will need to figure it out through the acpi table. I remembered Rafael said the following words the other day in a reply to my evaluate_ps3_when_entering_d3_cold_patch: ----------------------------------------------------------------------- I'd rather say that with _PR3 we have the opportunity to avoid removing power completely from the device. In other words, D3_hot is supported (and it is supported _only_ in that case). ----------------------------------------------------------------------- So I think here is a problem, that if a device has only _PS3, why should we say D3hot is supported? Is there a reason for this that I missed? Explanation is appreciated, thanks. -Aaron ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup @ 2012-04-20 7:47 ` Aaron Lu 0 siblings, 0 replies; 15+ messages in thread From: Aaron Lu @ 2012-04-20 7:47 UTC (permalink / raw) To: Lin Ming, Rafael J. Wysocki Cc: Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying Hi, On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: > > > > > 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. > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > > > flag valid. This doesn't feel right to me, since per our discussion the > > > > other day, we should assume _PS3 will put the device into D3cold. > > > > > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > > > valid from the perspective of acpi, it is the individual driver's > > > > responsibility to decide which state is actually valid and will be used. > > > > > > Right. > > > > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > > > > > > > I mean, if _PS3 is available, can we say D3hot is valid? > > Yes. > OK, now I'm confused... First, let me try to clarify the meaning of acpi power state's valid flag. By valid, I suppose it means the device can be in that state, instead of we have a way to program this device to go into that state. e.g. D0 is valid means the device can be in D0 state, and D3_cold is valid means the device can be in D3_cold state. We unconditionally set these two states as valid, because we know every device supports these two states. But we might not be able to put the device into that state in software, since we might not have _PS0 or _PS3 control methods for it. And if we do have the _PSx or _PRx control methods, we knows we have a way to put the device into that state, and hence the device should be able to support that power state, so we will set that state as valid too. Is this correct? For D3hot, obviously not all device supports this state, so we will need to figure it out through the acpi table. I remembered Rafael said the following words the other day in a reply to my evaluate_ps3_when_entering_d3_cold_patch: ----------------------------------------------------------------------- I'd rather say that with _PR3 we have the opportunity to avoid removing power completely from the device. In other words, D3_hot is supported (and it is supported _only_ in that case). ----------------------------------------------------------------------- So I think here is a problem, that if a device has only _PS3, why should we say D3hot is supported? Is there a reason for this that I missed? Explanation is appreciated, thanks. -Aaron ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 7:47 ` Aaron Lu (?) @ 2012-04-20 11:12 ` Rafael J. Wysocki 2012-04-20 15:14 ` Lin Ming 2012-04-21 1:36 ` Lu, Aaron -1 siblings, 2 replies; 15+ messages in thread From: Rafael J. Wysocki @ 2012-04-20 11:12 UTC (permalink / raw) To: Aaron Lu Cc: Lin Ming, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Friday, April 20, 2012, Aaron Lu wrote: > Hi, > > On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: > > > > > > 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. > > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > > > > > > > > > > > > > > > > With this patch now, if a device has _PS3, then we will set its D3hot > > > > > flag valid. This doesn't feel right to me, since per our discussion the > > > > > other day, we should assume _PS3 will put the device into D3cold. > > > > > > > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > > > > > valid from the perspective of acpi, it is the individual driver's > > > > > responsibility to decide which state is actually valid and will be used. > > > > > > > > Right. > > > > > > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > > > > > > > > > > I mean, if _PS3 is available, can we say D3hot is valid? > > > > Yes. > > > > OK, now I'm confused... > > First, let me try to clarify the meaning of acpi power state's valid > flag. > > By valid, I suppose it means the device can be in that state, instead of > we have a way to program this device to go into that state. > > e.g. D0 is valid means the device can be in D0 state, and D3_cold is > valid means the device can be in D3_cold state. We unconditionally set > these two states as valid, because we know every device supports these > two states. But we might not be able to put the device into that state > in software, since we might not have _PS0 or _PS3 control methods for it. > > And if we do have the _PSx or _PRx control methods, we knows we have a > way to put the device into that state, and hence the device should be > able to support that power state, so we will set that state as valid too. > > Is this correct? > > For D3hot, obviously not all device supports this state, so we will need > to figure it out through the acpi table. > I remembered Rafael said the following words the other day in a reply to > my evaluate_ps3_when_entering_d3_cold_patch: > > ----------------------------------------------------------------------- > I'd rather say that with _PR3 we have the opportunity to avoid removing > power completely from the device. In other words, D3_hot is supported (and > it is supported _only_ in that case). > ----------------------------------------------------------------------- > > So I think here is a problem, that if a device has only _PS3, why should > we say D3hot is supported? Is there a reason for this that I missed? OK, I agree. We need to special case the situation in which _PR3 is not present, but _PS3 is. IOW, we should do something like this in the loop in acpi_bus_get_power_flags(): /* State is valid if we have some power control */ if (ps->resources.count || (ps->flags.explicit_set && i < ACPI_STATE_D3)) ps->flags.valid = 1; Thanks, Rafael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 11:12 ` Rafael J. Wysocki @ 2012-04-20 15:14 ` Lin Ming 2012-04-21 1:36 ` Lu, Aaron 1 sibling, 0 replies; 15+ messages in thread From: Lin Ming @ 2012-04-20 15:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, April 20, 2012, Aaron Lu wrote: >> Hi, >> >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: >> > > > > > 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. >> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. >> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. >> > > > > > >> > > > > >> > > > > With this patch now, if a device has _PS3, then we will set its D3hot >> > > > > flag valid. This doesn't feel right to me, since per our discussion the >> > > > > other day, we should assume _PS3 will put the device into D3cold. >> > > > > >> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is >> > > > > valid from the perspective of acpi, it is the individual driver's >> > > > > responsibility to decide which state is actually valid and will be used. >> > > > >> > > > Right. >> > > > >> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. >> > > > >> > > >> > > I mean, if _PS3 is available, can we say D3hot is valid? >> > >> > Yes. >> > >> >> OK, now I'm confused... >> >> First, let me try to clarify the meaning of acpi power state's valid >> flag. >> >> By valid, I suppose it means the device can be in that state, instead of >> we have a way to program this device to go into that state. >> >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is >> valid means the device can be in D3_cold state. We unconditionally set >> these two states as valid, because we know every device supports these >> two states. But we might not be able to put the device into that state >> in software, since we might not have _PS0 or _PS3 control methods for it. >> >> And if we do have the _PSx or _PRx control methods, we knows we have a >> way to put the device into that state, and hence the device should be >> able to support that power state, so we will set that state as valid too. >> >> Is this correct? >> >> For D3hot, obviously not all device supports this state, so we will need >> to figure it out through the acpi table. >> I remembered Rafael said the following words the other day in a reply to >> my evaluate_ps3_when_entering_d3_cold_patch: >> >> ----------------------------------------------------------------------- >> I'd rather say that with _PR3 we have the opportunity to avoid removing >> power completely from the device. In other words, D3_hot is supported (and >> it is supported _only_ in that case). >> ----------------------------------------------------------------------- >> >> So I think here is a problem, that if a device has only _PS3, why should >> we say D3hot is supported? Is there a reason for this that I missed? > > OK, I agree. We need to special case the situation in which _PR3 is not > present, but _PS3 is. IOW, we should do something like this in the loop in > acpi_bus_get_power_flags(): > > > /* State is valid if we have some power control */ > if (ps->resources.count > || (ps->flags.explicit_set && i < ACPI_STATE_D3)) > ps->flags.valid = 1; Will add below change. - /* State is valid if we have some power control */ - if (ps->resources.count || ps->flags.explicit_set) + /* + * State is valid if we have some power control + * D3hot state is only valid if _PR3 present + */ + if (ps->resources.count || + (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT)) ps->flags.valid = 1; Rafael, can I still add your ACK when send updated patch? Thanks, Lin Ming > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup @ 2012-04-20 15:14 ` Lin Ming 0 siblings, 0 replies; 15+ messages in thread From: Lin Ming @ 2012-04-20 15:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, April 20, 2012, Aaron Lu wrote: >> Hi, >> >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: >> > > > > > 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. >> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. >> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. >> > > > > > >> > > > > >> > > > > With this patch now, if a device has _PS3, then we will set its D3hot >> > > > > flag valid. This doesn't feel right to me, since per our discussion the >> > > > > other day, we should assume _PS3 will put the device into D3cold. >> > > > > >> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is >> > > > > valid from the perspective of acpi, it is the individual driver's >> > > > > responsibility to decide which state is actually valid and will be used. >> > > > >> > > > Right. >> > > > >> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. >> > > > >> > > >> > > I mean, if _PS3 is available, can we say D3hot is valid? >> > >> > Yes. >> > >> >> OK, now I'm confused... >> >> First, let me try to clarify the meaning of acpi power state's valid >> flag. >> >> By valid, I suppose it means the device can be in that state, instead of >> we have a way to program this device to go into that state. >> >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is >> valid means the device can be in D3_cold state. We unconditionally set >> these two states as valid, because we know every device supports these >> two states. But we might not be able to put the device into that state >> in software, since we might not have _PS0 or _PS3 control methods for it. >> >> And if we do have the _PSx or _PRx control methods, we knows we have a >> way to put the device into that state, and hence the device should be >> able to support that power state, so we will set that state as valid too. >> >> Is this correct? >> >> For D3hot, obviously not all device supports this state, so we will need >> to figure it out through the acpi table. >> I remembered Rafael said the following words the other day in a reply to >> my evaluate_ps3_when_entering_d3_cold_patch: >> >> ----------------------------------------------------------------------- >> I'd rather say that with _PR3 we have the opportunity to avoid removing >> power completely from the device. In other words, D3_hot is supported (and >> it is supported _only_ in that case). >> ----------------------------------------------------------------------- >> >> So I think here is a problem, that if a device has only _PS3, why should >> we say D3hot is supported? Is there a reason for this that I missed? > > OK, I agree. We need to special case the situation in which _PR3 is not > present, but _PS3 is. IOW, we should do something like this in the loop in > acpi_bus_get_power_flags(): > > > /* State is valid if we have some power control */ > if (ps->resources.count > || (ps->flags.explicit_set && i < ACPI_STATE_D3)) > ps->flags.valid = 1; Will add below change. - /* State is valid if we have some power control */ - if (ps->resources.count || ps->flags.explicit_set) + /* + * State is valid if we have some power control + * D3hot state is only valid if _PR3 present + */ + if (ps->resources.count || + (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT)) ps->flags.valid = 1; Rafael, can I still add your ACK when send updated patch? Thanks, Lin Ming > > Thanks, > Rafael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 15:14 ` Lin Ming (?) @ 2012-04-22 11:54 ` Rafael J. Wysocki -1 siblings, 0 replies; 15+ messages in thread From: Rafael J. Wysocki @ 2012-04-22 11:54 UTC (permalink / raw) To: Lin Ming Cc: Aaron Lu, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying On Friday, April 20, 2012, Lin Ming wrote: > On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Friday, April 20, 2012, Aaron Lu wrote: > >> Hi, > >> > >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: > >> > > > > > 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. > >> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. > >> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. > >> > > > > > > >> > > > > > >> > > > > With this patch now, if a device has _PS3, then we will set its D3hot > >> > > > > flag valid. This doesn't feel right to me, since per our discussion the > >> > > > > other day, we should assume _PS3 will put the device into D3cold. > >> > > > > > >> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is > >> > > > > valid from the perspective of acpi, it is the individual driver's > >> > > > > responsibility to decide which state is actually valid and will be used. > >> > > > > >> > > > Right. > >> > > > > >> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. > >> > > > > >> > > > >> > > I mean, if _PS3 is available, can we say D3hot is valid? > >> > > >> > Yes. > >> > > >> > >> OK, now I'm confused... > >> > >> First, let me try to clarify the meaning of acpi power state's valid > >> flag. > >> > >> By valid, I suppose it means the device can be in that state, instead of > >> we have a way to program this device to go into that state. > >> > >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is > >> valid means the device can be in D3_cold state. We unconditionally set > >> these two states as valid, because we know every device supports these > >> two states. But we might not be able to put the device into that state > >> in software, since we might not have _PS0 or _PS3 control methods for it. > >> > >> And if we do have the _PSx or _PRx control methods, we knows we have a > >> way to put the device into that state, and hence the device should be > >> able to support that power state, so we will set that state as valid too. > >> > >> Is this correct? > >> > >> For D3hot, obviously not all device supports this state, so we will need > >> to figure it out through the acpi table. > >> I remembered Rafael said the following words the other day in a reply to > >> my evaluate_ps3_when_entering_d3_cold_patch: > >> > >> ----------------------------------------------------------------------- > >> I'd rather say that with _PR3 we have the opportunity to avoid removing > >> power completely from the device. In other words, D3_hot is supported (and > >> it is supported _only_ in that case). > >> ----------------------------------------------------------------------- > >> > >> So I think here is a problem, that if a device has only _PS3, why should > >> we say D3hot is supported? Is there a reason for this that I missed? > > > > OK, I agree. We need to special case the situation in which _PR3 is not > > present, but _PS3 is. IOW, we should do something like this in the loop in > > acpi_bus_get_power_flags(): > > > > > > /* State is valid if we have some power control */ > > if (ps->resources.count > > || (ps->flags.explicit_set && i < ACPI_STATE_D3)) > > ps->flags.valid = 1; > > Will add below change. > > - /* State is valid if we have some power control */ > - if (ps->resources.count || ps->flags.explicit_set) > + /* > + * State is valid if we have some power control > + * D3hot state is only valid if _PR3 present > + */ Well, I'd say: + /* + * State is valid 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; > > Rafael, > > can I still add your ACK when send updated patch? Sure. Thanks, Rafael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup 2012-04-20 11:12 ` Rafael J. Wysocki @ 2012-04-21 1:36 ` Lu, Aaron 2012-04-21 1:36 ` Lu, Aaron 1 sibling, 0 replies; 15+ messages in thread From: Lu, Aaron @ 2012-04-21 1:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Lin Ming, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying 在 2012-4-20，下午7:08，"Rafael J. Wysocki" <rjw@sisk.pl> 写道： > On Friday, April 20, 2012, Aaron Lu wrote: >> Hi, >> >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: >>>>>>> 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. >>>>>>> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. >>>>>>> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. >>>>>>> >>>>>> >>>>>> With this patch now, if a device has _PS3, then we will set its D3hot >>>>>> flag valid. This doesn't feel right to me, since per our discussion the >>>>>> other day, we should assume _PS3 will put the device into D3cold. >>>>>> >>>>>> Or do you mean: if _PS3 is available, then both D3hot and D3cold is >>>>>> valid from the perspective of acpi, it is the individual driver's >>>>>> responsibility to decide which state is actually valid and will be used. >>>>> >>>>> Right. >>>>> >>>>> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. >>>>> >>>> >>>> I mean, if _PS3 is available, can we say D3hot is valid? >>> >>> Yes. >>> >> >> OK, now I'm confused... >> >> First, let me try to clarify the meaning of acpi power state's valid >> flag. >> >> By valid, I suppose it means the device can be in that state, instead of >> we have a way to program this device to go into that state. >> >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is >> valid means the device can be in D3_cold state. We unconditionally set >> these two states as valid, because we know every device supports these >> two states. But we might not be able to put the device into that state >> in software, since we might not have _PS0 or _PS3 control methods for it. >> >> And if we do have the _PSx or _PRx control methods, we knows we have a >> way to put the device into that state, and hence the device should be >> able to support that power state, so we will set that state as valid too. >> >> Is this correct? >> >> For D3hot, obviously not all device supports this state, so we will need >> to figure it out through the acpi table. >> I remembered Rafael said the following words the other day in a reply to >> my evaluate_ps3_when_entering_d3_cold_patch: >> >> ----------------------------------------------------------------------- >> I'd rather say that with _PR3 we have the opportunity to avoid removing >> power completely from the device. In other words, D3_hot is supported (and >> it is supported _only_ in that case). >> ----------------------------------------------------------------------- >> >> So I think here is a problem, that if a device has only _PS3, why should >> we say D3hot is supported? Is there a reason for this that I missed? > > OK, I agree. We need to special case the situation in which _PR3 is not > present, but _PS3 is. IOW, we should do something like this in the loop in > acpi_bus_get_power_flags(): > > > /* State is valid if we have some power control */ > if (ps->resources.count > || (ps->flags.explicit_set && i < ACPI_STATE_D3)) > ps->flags.valid = 1; > Looks good to me, thanks. -Aaron ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] ACPI: D3 states cleanup @ 2012-04-21 1:36 ` Lu, Aaron 0 siblings, 0 replies; 15+ messages in thread From: Lu, Aaron @ 2012-04-21 1:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Lin Ming, Len Brown, linux-acpi, linux-kernel, Zhang Rui, Huang, Ying [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3575 bytes --] ÔÚ 2012-4-20£¬ÏÂÎç7:08£¬"Rafael J. Wysocki" <rjw@sisk.pl> дµÀ£º > On Friday, April 20, 2012, Aaron Lu wrote: >> Hi, >> >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote: >>>>>>> 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. >>>>>>> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. >>>>>>> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3. >>>>>>> >>>>>> >>>>>> With this patch now, if a device has _PS3, then we will set its D3hot >>>>>> flag valid. This doesn't feel right to me, since per our discussion the >>>>>> other day, we should assume _PS3 will put the device into D3cold. >>>>>> >>>>>> Or do you mean: if _PS3 is available, then both D3hot and D3cold is >>>>>> valid from the perspective of acpi, it is the individual driver's >>>>>> responsibility to decide which state is actually valid and will be used. >>>>> >>>>> Right. >>>>> >>>>> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid. >>>>> >>>> >>>> I mean, if _PS3 is available, can we say D3hot is valid? >>> >>> Yes. >>> >> >> OK, now I'm confused... >> >> First, let me try to clarify the meaning of acpi power state's valid >> flag. >> >> By valid, I suppose it means the device can be in that state, instead of >> we have a way to program this device to go into that state. >> >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is >> valid means the device can be in D3_cold state. We unconditionally set >> these two states as valid, because we know every device supports these >> two states. But we might not be able to put the device into that state >> in software, since we might not have _PS0 or _PS3 control methods for it. >> >> And if we do have the _PSx or _PRx control methods, we knows we have a >> way to put the device into that state, and hence the device should be >> able to support that power state, so we will set that state as valid too. >> >> Is this correct? >> >> For D3hot, obviously not all device supports this state, so we will need >> to figure it out through the acpi table. >> I remembered Rafael said the following words the other day in a reply to >> my evaluate_ps3_when_entering_d3_cold_patch: >> >> ----------------------------------------------------------------------- >> I'd rather say that with _PR3 we have the opportunity to avoid removing >> power completely from the device. In other words, D3_hot is supported (and >> it is supported _only_ in that case). >> ----------------------------------------------------------------------- >> >> So I think here is a problem, that if a device has only _PS3, why should >> we say D3hot is supported? Is there a reason for this that I missed? > > OK, I agree. We need to special case the situation in which _PR3 is not > present, but _PS3 is. IOW, we should do something like this in the loop in > acpi_bus_get_power_flags(): > > > /* State is valid if we have some power control */ > if (ps->resources.count > || (ps->flags.explicit_set && i < ACPI_STATE_D3)) > ps->flags.valid = 1; > Looks good to me, thanks. -Aaron ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-04-22 11:49 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-20 1:19 [PATCH v2] ACPI: D3 states cleanup Lin Ming 2012-04-20 2:23 ` Aaron Lu 2012-04-20 2:23 ` Aaron Lu 2012-04-20 3:19 ` Lin Ming 2012-04-20 5:23 ` Aaron Lu 2012-04-20 5:23 ` Aaron Lu 2012-04-20 5:37 ` Lin Ming 2012-04-20 7:47 ` Aaron Lu 2012-04-20 7:47 ` Aaron Lu 2012-04-20 11:12 ` Rafael J. Wysocki 2012-04-20 15:14 ` Lin Ming 2012-04-20 15:14 ` Lin Ming 2012-04-22 11:54 ` Rafael J. Wysocki 2012-04-21 1:36 ` Lu, Aaron 2012-04-21 1:36 ` Lu, Aaron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.