| Message ID | 20210128232729.16064-1-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v9,1/7] ACPI: scan: Obtain device's desired enumeration power state | expand |
Hi Rafael, Thanks for the comments. On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Store a device's desired enumeration power state in struct > > acpi_device_power_flags during acpi_device object's initialisation. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/acpi/scan.c | 6 ++++++ > > include/acpi/acpi_bus.h | 3 ++- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > index 1d7a02ee45e05..b077c645c9845 100644 > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state) > > > > static void acpi_bus_get_power_flags(struct acpi_device *device) > > { > > + unsigned long long pre; > > + acpi_status status; > > u32 i; > > > > /* Presence of _PS0|_PR0 indicates 'power manageable' */ > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device) > > if (acpi_has_method(device->handle, "_DSW")) > > device->power.flags.dsw_present = 1; > > > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre); > > + if (ACPI_SUCCESS(status) && !pre) > > + device->power.flags.allow_low_power_probe = 1; > > While this is what has been discussed and thanks for taking it into > account, I'm now thinking that it may be cleaner to introduce a new > object to return the deepest power state of the device in which it can > be enumerated, say _DSE (Device State for Enumeration) such that 4 > means D3cold, 3 - D3hot and so on, so the above check can be replaced > with something like > > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse); s/_PRE/_DSE/ ? > if (ACPI_FAILURE(status)) ACPI_SUCCESS? > device->power.state_for_enumeratin = dse; > > And then, it is a matter of comparing ->power.state_for_enumeratin > with ->power.state and putting the device into D0 if the former is > shallower than the latter. > > What do you think? Sounds good. How about calling the function e.g. acpi_device_resume_for_probe(), so runtime PM could be used to resume the device if the function returns true?
On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > Thanks for the comments. > > On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote: > > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Store a device's desired enumeration power state in struct > > > acpi_device_power_flags during acpi_device object's initialisation. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/acpi/scan.c | 6 ++++++ > > > include/acpi/acpi_bus.h | 3 ++- > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > index 1d7a02ee45e05..b077c645c9845 100644 > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state) > > > > > > static void acpi_bus_get_power_flags(struct acpi_device *device) > > > { > > > + unsigned long long pre; > > > + acpi_status status; > > > u32 i; > > > > > > /* Presence of _PS0|_PR0 indicates 'power manageable' */ > > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device) > > > if (acpi_has_method(device->handle, "_DSW")) > > > device->power.flags.dsw_present = 1; > > > > > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre); > > > + if (ACPI_SUCCESS(status) && !pre) > > > + device->power.flags.allow_low_power_probe = 1; > > > > While this is what has been discussed and thanks for taking it into > > account, I'm now thinking that it may be cleaner to introduce a new > > object to return the deepest power state of the device in which it can > > be enumerated, say _DSE (Device State for Enumeration) such that 4 > > means D3cold, 3 - D3hot and so on, so the above check can be replaced > > with something like > > > > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse); > > s/_PRE/_DSE/ > > ? Yes, sorry. > > > if (ACPI_FAILURE(status)) > > ACPI_SUCCESS? Yup. > > device->power.state_for_enumeratin = dse; > > > > And then, it is a matter of comparing ->power.state_for_enumeratin > > with ->power.state and putting the device into D0 if the former is > > shallower than the latter. > > > > What do you think? > > Sounds good. How about calling the function e.g. > acpi_device_resume_for_probe(), so runtime PM could be used to resume the > device if the function returns true? I'd rather try to power it up before enabling runtime PM, because in order to do the latter properly, you need to know if the device is active or suspended to start with. So you need something like (pseudo-code) if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) { acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0); pm_runtime_set_active(dev); } else { pm_runtime_set_suspended(dev); } and then you can enable PM-runtime.
On Fri, Jan 29, 2021 at 05:57:17PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > Thanks for the comments. > > > > On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote: > > > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Store a device's desired enumeration power state in struct > > > > acpi_device_power_flags during acpi_device object's initialisation. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/acpi/scan.c | 6 ++++++ > > > > include/acpi/acpi_bus.h | 3 ++- > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > > > index 1d7a02ee45e05..b077c645c9845 100644 > > > > --- a/drivers/acpi/scan.c > > > > +++ b/drivers/acpi/scan.c > > > > @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state) > > > > > > > > static void acpi_bus_get_power_flags(struct acpi_device *device) > > > > { > > > > + unsigned long long pre; > > > > + acpi_status status; > > > > u32 i; > > > > > > > > /* Presence of _PS0|_PR0 indicates 'power manageable' */ > > > > @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device) > > > > if (acpi_has_method(device->handle, "_DSW")) > > > > device->power.flags.dsw_present = 1; > > > > > > > > + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre); > > > > + if (ACPI_SUCCESS(status) && !pre) > > > > + device->power.flags.allow_low_power_probe = 1; > > > > > > While this is what has been discussed and thanks for taking it into > > > account, I'm now thinking that it may be cleaner to introduce a new > > > object to return the deepest power state of the device in which it can > > > be enumerated, say _DSE (Device State for Enumeration) such that 4 > > > means D3cold, 3 - D3hot and so on, so the above check can be replaced > > > with something like > > > > > > status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse); > > > > s/_PRE/_DSE/ > > > > ? > > Yes, sorry. > > > > > > if (ACPI_FAILURE(status)) > > > > ACPI_SUCCESS? > > Yup. > > > > device->power.state_for_enumeratin = dse; > > > > > > And then, it is a matter of comparing ->power.state_for_enumeratin > > > with ->power.state and putting the device into D0 if the former is > > > shallower than the latter. > > > > > > What do you think? > > > > Sounds good. How about calling the function e.g. > > acpi_device_resume_for_probe(), so runtime PM could be used to resume the > > device if the function returns true? > > I'd rather try to power it up before enabling runtime PM, because in > order to do the latter properly, you need to know if the device is > active or suspended to start with. > > So you need something like (pseudo-code) > > if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) { > acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0); > pm_runtime_set_active(dev); > } else { > pm_runtime_set_suspended(dev); I guess the else branch isn't needed? The device remains suspended if its state hasn't been changed. > } > > and then you can enable PM-runtime. Yes, agreed, this is what drivers should do. The I²C framework would use the function and conditionally power the device on before enabling runtime PM. This is how it's implemented by the set already but I think the change in semantics requires a little more still.
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1d7a02ee45e05..b077c645c9845 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state) static void acpi_bus_get_power_flags(struct acpi_device *device) { + unsigned long long pre; + acpi_status status; u32 i; /* Presence of _PS0|_PR0 indicates 'power manageable' */ @@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device) if (acpi_has_method(device->handle, "_DSW")) device->power.flags.dsw_present = 1; + status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre); + if (ACPI_SUCCESS(status) && !pre) + device->power.flags.allow_low_power_probe = 1; + /* * Enumerate supported power management states */ diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 02a716a0af5d4..020b850120883 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -258,7 +258,8 @@ struct acpi_device_power_flags { u32 power_removed:1; /* Optimize Dx->D0 */ u32 ignore_parent:1; /* Power is independent of parent power state */ u32 dsw_present:1; /* _DSW present? */ - u32 reserved:26; + u32 allow_low_power_probe:1; /* Allow low power state in device probe */ + u32 reserved:25; }; struct acpi_device_power_state {
Store a device's desired enumeration power state in struct acpi_device_power_flags during acpi_device object's initialisation. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/scan.c | 6 ++++++ include/acpi/acpi_bus.h | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-)