linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] i2c: enable async suspend/resume on i2c devices
       [not found] <20200327151951.18111-1-ricardo.canuelo@collabora.com>
@ 2020-03-27 15:43 ` Wolfram Sang
  2020-03-27 20:26   ` dbasehore .
  0 siblings, 1 reply; 5+ messages in thread
From: Wolfram Sang @ 2020-03-27 15:43 UTC (permalink / raw)
  To: Ricardo Cañuelo; +Cc: linux-i2c, Derek Basehore, Guenter Roeck, linux-pm

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: enable async suspend/resume on i2c devices
  2020-03-27 15:43 ` [PATCH] i2c: enable async suspend/resume on i2c devices Wolfram Sang
@ 2020-03-27 20:26   ` dbasehore .
  2020-03-29 10:24     ` Ricardo Cañuelo
  2020-03-29 12:49     ` Ezequiel Garcia
  0 siblings, 2 replies; 5+ messages in thread
From: dbasehore . @ 2020-03-27 20:26 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Ricardo Cañuelo, linux-i2c, Guenter Roeck, Linux-pm mailing list

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: enable async suspend/resume on i2c devices
  2020-03-27 20:26   ` dbasehore .
@ 2020-03-29 10:24     ` Ricardo Cañuelo
  2020-03-29 12:49     ` Ezequiel Garcia
  1 sibling, 0 replies; 5+ messages in thread
From: Ricardo Cañuelo @ 2020-03-29 10:24 UTC (permalink / raw)
  To: dbasehore ., Wolfram Sang; +Cc: linux-i2c, Guenter Roeck, Linux-pm mailing list

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: enable async suspend/resume on i2c devices
  2020-03-27 20:26   ` dbasehore .
  2020-03-29 10:24     ` Ricardo Cañuelo
@ 2020-03-29 12:49     ` Ezequiel Garcia
  2020-04-08  5:06       ` dbasehore .
  1 sibling, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2020-03-29 12:49 UTC (permalink / raw)
  To: dbasehore .
  Cc: Wolfram Sang, Ricardo Cañuelo, linux-i2c, Guenter Roeck,
	Linux-pm mailing list, Linux Kernel Mailing List, kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i2c: enable async suspend/resume on i2c devices
  2020-03-29 12:49     ` Ezequiel Garcia
@ 2020-04-08  5:06       ` dbasehore .
  0 siblings, 0 replies; 5+ messages in thread
From: dbasehore . @ 2020-04-08  5:06 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Wolfram Sang, Ricardo Cañuelo, linux-i2c, Guenter Roeck,
	Linux-pm mailing list, Linux Kernel Mailing List, kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-08  5:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200327151951.18111-1-ricardo.canuelo@collabora.com>
2020-03-27 15:43 ` [PATCH] i2c: enable async suspend/resume on i2c devices Wolfram Sang
2020-03-27 20:26   ` dbasehore .
2020-03-29 10:24     ` Ricardo Cañuelo
2020-03-29 12:49     ` Ezequiel Garcia
2020-04-08  5:06       ` dbasehore .

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).