| Message ID | 20200327151951.18111-1-ricardo.canuelo@collabora.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | i2c: enable async suspend/resume on i2c devices | expand |
On Fri, Mar 27, 2020 at 04:19:51PM +0100, Ricardo Cañuelo wrote: > This enables the async suspend property for i2c devices. This reduces > the suspend/resume time considerably on platforms with multiple i2c > devices (such as a trackpad or touchscreen). > > (am from https://patchwork.ozlabs.org/patch/949922/) > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > Reviewed-on: https://chromium-review.googlesource.com/1152411 > Tested-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > Reviewed-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > Reviewed-by: Justin TerAvest <teravest@chromium.org> > Signed-off-by: Guenter Roeck <groeck@chromium.org> > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> > --- Adding linux-pm to CC. I don't know much about internals of async suspend. Is there a guide like "what every maintainer needs to know about"? > This patch was originally created for chromeos some time ago and I'm > evaluating if it's a good candidate for upstreaming. > > By the looks of it I think it was done with chromebooks in mind, but > AFAICT this would impact every i2c client in every platform, so I'd like > to know your opinion about it. > > As far as I know there was no further investigation or testing on it, so > I don't know if it was tested on any other hardware. > > Best, > Ricardo > > drivers/i2c/i2c-core-base.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index cefad0881942..643bc0fe0281 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -769,6 +769,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > client->dev.of_node = of_node_get(info->of_node); > client->dev.fwnode = info->fwnode; > > + device_enable_async_suspend(&client->dev); > i2c_dev_set_name(adap, client, info); > > if (info->properties) { > -- > 2.18.0 >
On Fri, Mar 27, 2020 at 8:43 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > On Fri, Mar 27, 2020 at 04:19:51PM +0100, Ricardo Cañuelo wrote: > > This enables the async suspend property for i2c devices. This reduces > > the suspend/resume time considerably on platforms with multiple i2c > > devices (such as a trackpad or touchscreen). > > > > (am from https://patchwork.ozlabs.org/patch/949922/) > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > Reviewed-on: https://chromium-review.googlesource.com/1152411 > > Tested-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > Reviewed-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > Reviewed-by: Justin TerAvest <teravest@chromium.org> > > Signed-off-by: Guenter Roeck <groeck@chromium.org> > > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> > > --- > > Adding linux-pm to CC. I don't know much about internals of async > suspend. Is there a guide like "what every maintainer needs to know > about"? For more details, you can look at the function dpm_resume in the drivers/base/power/main.c file and follow from there. I can't find anything in Documentation/, so here's a short overview: Async devices have suspend/resume callbacks scheduled via async_schedule at every step (normal, late, noirq, etc.) for suspending/resuming devices. We wait for all device callbacks to complete at the end of each of these steps before moving onto the next one. This means that you won't have a resume_early callback running when you start the normal device resume callbacks. The async callbacks still wait individually for children on suspend and parents on resume to complete their own callbacks before calling their own. Because some dependencies may not be tracked by the parent/child graph (or other unknown reasons), async is off by default. Enabling async is a confirmation that all dependencies to other devices are properly tracked, whether through the parent/child relationship or otherwise. > > > This patch was originally created for chromeos some time ago and I'm > > evaluating if it's a good candidate for upstreaming. > > > > By the looks of it I think it was done with chromebooks in mind, but > > AFAICT this would impact every i2c client in every platform, so I'd like > > to know your opinion about it. > > > > As far as I know there was no further investigation or testing on it, so > > I don't know if it was tested on any other hardware. > > > > Best, > > Ricardo > > > > drivers/i2c/i2c-core-base.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > index cefad0881942..643bc0fe0281 100644 > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -769,6 +769,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > > client->dev.of_node = of_node_get(info->of_node); > > client->dev.fwnode = info->fwnode; > > > > + device_enable_async_suspend(&client->dev); > > i2c_dev_set_name(adap, client, info); > > > > if (info->properties) { > > -- > > 2.18.0 > >
On Fri, 2020-03-27 at 13:26 -0700, dbasehore . wrote: > > Enabling async is a confirmation that all dependencies to other > devices are properly tracked, whether through the parent/child > relationship or otherwise. Thanks for the info, Derek. Wouldn't it be risky then to enable async for all i2c client devices indiscriminately? Or is it safe to assume that all i2c devices will be idependent from each other? Cheers, Ricardo
Hi Derek, On Fri, 27 Mar 2020 at 17:26, dbasehore . <dbasehore@chromium.org> wrote: > > On Fri, Mar 27, 2020 at 8:43 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > On Fri, Mar 27, 2020 at 04:19:51PM +0100, Ricardo Cañuelo wrote: > > > This enables the async suspend property for i2c devices. This reduces > > > the suspend/resume time considerably on platforms with multiple i2c > > > devices (such as a trackpad or touchscreen). > > > > > > (am from https://patchwork.ozlabs.org/patch/949922/) > > > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > > Reviewed-on: https://chromium-review.googlesource.com/1152411 > > > Tested-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > > Reviewed-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > > Reviewed-by: Justin TerAvest <teravest@chromium.org> > > > Signed-off-by: Guenter Roeck <groeck@chromium.org> > > > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> > > > --- > > > > Adding linux-pm to CC. I don't know much about internals of async > > suspend. Is there a guide like "what every maintainer needs to know > > about"? > > For more details, you can look at the function dpm_resume in the > drivers/base/power/main.c file and follow from there. > > I can't find anything in Documentation/, so here's a short overview: > Async devices have suspend/resume callbacks scheduled via > async_schedule at every step (normal, late, noirq, etc.) for > suspending/resuming devices. We wait for all device callbacks to > complete at the end of each of these steps before moving onto the next > one. This means that you won't have a resume_early callback running > when you start the normal device resume callbacks. > > The async callbacks still wait individually for children on suspend > and parents on resume to complete their own callbacks before calling > their own. Because some dependencies may not be tracked by the > parent/child graph (or other unknown reasons), async is off by > default. > > Enabling async is a confirmation that all dependencies to other > devices are properly tracked, whether through the parent/child > relationship or otherwise. > Have you noticed the async sysfs attribute [1]? Given this allows userspace to enable the async suspend/resume, wouldn't it be simpler to just do that in userspace, on the platforms you want to target (e.g. Apollolake Chromebook devices, and so on) ? Thanks, Ezequiel [1] Documentation/ABI/testing/sysfs-devices-power > > > > > This patch was originally created for chromeos some time ago and I'm > > > evaluating if it's a good candidate for upstreaming. > > > > > > By the looks of it I think it was done with chromebooks in mind, but > > > AFAICT this would impact every i2c client in every platform, so I'd like > > > to know your opinion about it. > > > > > > As far as I know there was no further investigation or testing on it, so > > > I don't know if it was tested on any other hardware. > > > > > > Best, > > > Ricardo > > > > > > drivers/i2c/i2c-core-base.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > > index cefad0881942..643bc0fe0281 100644 > > > --- a/drivers/i2c/i2c-core-base.c > > > +++ b/drivers/i2c/i2c-core-base.c > > > @@ -769,6 +769,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > > > client->dev.of_node = of_node_get(info->of_node); > > > client->dev.fwnode = info->fwnode; > > > > > > + device_enable_async_suspend(&client->dev); > > > i2c_dev_set_name(adap, client, info); > > > > > > if (info->properties) { > > > -- > > > 2.18.0 > > >
On Sun, Mar 29, 2020 at 5:49 AM Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > > Hi Derek, > > On Fri, 27 Mar 2020 at 17:26, dbasehore . <dbasehore@chromium.org> wrote: > > > > On Fri, Mar 27, 2020 at 8:43 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > > On Fri, Mar 27, 2020 at 04:19:51PM +0100, Ricardo Cañuelo wrote: > > > > This enables the async suspend property for i2c devices. This reduces > > > > the suspend/resume time considerably on platforms with multiple i2c > > > > devices (such as a trackpad or touchscreen). > > > > > > > > (am from https://patchwork.ozlabs.org/patch/949922/) > > > > > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > > > Reviewed-on: https://chromium-review.googlesource.com/1152411 > > > > Tested-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > > > Reviewed-by: Venkateswarlu V Vinjamuri <venkateswarlu.v.vinjamuri@intel.com> > > > > Reviewed-by: Justin TerAvest <teravest@chromium.org> > > > > Signed-off-by: Guenter Roeck <groeck@chromium.org> > > > > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> > > > > --- > > > > > > Adding linux-pm to CC. I don't know much about internals of async > > > suspend. Is there a guide like "what every maintainer needs to know > > > about"? > > > > For more details, you can look at the function dpm_resume in the > > drivers/base/power/main.c file and follow from there. > > > > I can't find anything in Documentation/, so here's a short overview: > > Async devices have suspend/resume callbacks scheduled via > > async_schedule at every step (normal, late, noirq, etc.) for > > suspending/resuming devices. We wait for all device callbacks to > > complete at the end of each of these steps before moving onto the next > > one. This means that you won't have a resume_early callback running > > when you start the normal device resume callbacks. > > > > The async callbacks still wait individually for children on suspend > > and parents on resume to complete their own callbacks before calling > > their own. Because some dependencies may not be tracked by the > > parent/child graph (or other unknown reasons), async is off by > > default. > > > > Enabling async is a confirmation that all dependencies to other > > devices are properly tracked, whether through the parent/child > > relationship or otherwise. > > > > Have you noticed the async sysfs attribute [1]? > > Given this allows userspace to enable the async suspend/resume, > wouldn't it be simpler to just do that in userspace, on the > platforms you want to target (e.g. Apollolake Chromebook devices, and so on) ? I don't remember much since I attempted this a long time ago. That sounds like it would be reasonable under many circumstances though. > > Thanks, > Ezequiel > > [1] Documentation/ABI/testing/sysfs-devices-power > > > > > > > > This patch was originally created for chromeos some time ago and I'm > > > > evaluating if it's a good candidate for upstreaming. > > > > > > > > By the looks of it I think it was done with chromebooks in mind, but > > > > AFAICT this would impact every i2c client in every platform, so I'd like > > > > to know your opinion about it. > > > > > > > > As far as I know there was no further investigation or testing on it, so > > > > I don't know if it was tested on any other hardware. > > > > > > > > Best, > > > > Ricardo > > > > > > > > drivers/i2c/i2c-core-base.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > > > index cefad0881942..643bc0fe0281 100644 > > > > --- a/drivers/i2c/i2c-core-base.c > > > > +++ b/drivers/i2c/i2c-core-base.c > > > > @@ -769,6 +769,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > > > > client->dev.of_node = of_node_get(info->of_node); > > > > client->dev.fwnode = info->fwnode; > > > > > > > > + device_enable_async_suspend(&client->dev); > > > > i2c_dev_set_name(adap, client, info); > > > > > > > > if (info->properties) { > > > > -- > > > > 2.18.0 > > > >
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index cefad0881942..643bc0fe0281 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -769,6 +769,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf client->dev.of_node = of_node_get(info->of_node); client->dev.fwnode = info->fwnode; + device_enable_async_suspend(&client->dev); i2c_dev_set_name(adap, client, info); if (info->properties) {