[PATCH] i2c: enable async suspend/resume on i2c devices

[PATCH] i2c: enable async suspend/resume on i2c devices

[Derek Basehore]

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).

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 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 1ba40bb2b966..3382bb7e1dcc 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -749,6 +749,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->dev.of_node = 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.345.g5c9ce644c3-goog

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Andy Shevchenko]

On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <dbasehore@chromium.org> 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).

How did you test this?

Especially on Chromebooks based on Intel Cherrytrail / Braswell
platforms, they have a painful PMIC vs. OS design solution.

+Cc: Hans, who did a lot in this area.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Hans de Goede]

Hi,

On 07/27/2018 12:44 PM, Andy Shevchenko wrote:
> On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <dbasehore@chromium.org> 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).
> 
> How did you test this?
> 
> Especially on Chromebooks based on Intel Cherrytrail / Braswell
> platforms, they have a painful PMIC vs. OS design solution.
> 
> +Cc: Hans, who did a lot in this area.

We disable suspend of the i2c controller for the i2c-bus
to which the PMIc is connected on these platforms, so I do not
expect this to cause any new issues.

But this is something to keep an eye on,

Regards,

Hans

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[dbasehore .]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On Fri, Jul 27, 2018 at 5:07 AM Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
>
> On 07/27/2018 12:44 PM, Andy Shevchenko wrote:
> > On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <dbasehore@chromium.org>
> 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).
> >
> > How did you test this?
>

It was tested on Apollolake Chromebook devices.


> >
> > Especially on Chromebooks based on Intel Cherrytrail / Braswell
> > platforms, they have a painful PMIC vs. OS design solution.
> >
> > +Cc: Hans, who did a lot in this area.
>
> We disable suspend of the i2c controller for the i2c-bus
> to which the PMIc is connected on these platforms, so I do not
> expect this to cause any new issues.
>
> But this is something to keep an eye on,
>
> Regards,
>
> Hans
>
>

[-- Attachment #2: Type: text/html, Size: 1503 bytes --]

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Andy Shevchenko]

+Cc: Jarkko

On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <dbasehore@chromium.org> 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).
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  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 1ba40bb2b966..3382bb7e1dcc 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -749,6 +749,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>         client->dev.of_node = 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.345.g5c9ce644c3-goog
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH] i2c: enable async suspend/resume on i2c devices

[Ricardo Cañuelo]

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>
---
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

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[dbasehore .]

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
> >

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Ricardo Cañuelo]

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

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[Ezequiel Garcia]

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
> > >

Re: [PATCH] i2c: enable async suspend/resume on i2c devices

[dbasehore .]

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
> > > >