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 <[email protected]>
---
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
On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <[email protected]> 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
Hi,
On 07/27/2018 12:44 PM, Andy Shevchenko wrote:
> On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <[email protected]> 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
On Fri, Jul 27, 2018 at 5:07 AM Hans de Goede <[email protected]> wrote:
> Hi,
>
> On 07/27/2018 12:44 PM, Andy Shevchenko wrote:
> > On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <[email protected]>
> 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
>
>
+Cc: Jarkko
On Fri, Jul 27, 2018 at 1:55 AM, Derek Basehore <[email protected]> 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 <[email protected]>
> ---
> 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
Hi Derek,
On Fri, 27 Mar 2020 at 17:26, dbasehore . <[email protected]> wrote:
>
> On Fri, Mar 27, 2020 at 8:43 AM Wolfram Sang <[email protected]> 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 <[email protected]>
> > > Reviewed-on: https://chromium-review.googlesource.com/1152411
> > > Tested-by: Venkateswarlu V Vinjamuri <[email protected]>
> > > Reviewed-by: Venkateswarlu V Vinjamuri <[email protected]>
> > > Reviewed-by: Justin TerAvest <[email protected]>
> > > Signed-off-by: Guenter Roeck <[email protected]>
> > > Signed-off-by: Ricardo Cañuelo <[email protected]>
> > > ---
> >
> > 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
<[email protected]> wrote:
>
> Hi Derek,
>
> On Fri, 27 Mar 2020 at 17:26, dbasehore . <[email protected]> wrote:
> >
> > On Fri, Mar 27, 2020 at 8:43 AM Wolfram Sang <[email protected]> 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 <[email protected]>
> > > > Reviewed-on: https://chromium-review.googlesource.com/1152411
> > > > Tested-by: Venkateswarlu V Vinjamuri <[email protected]>
> > > > Reviewed-by: Venkateswarlu V Vinjamuri <[email protected]>
> > > > Reviewed-by: Justin TerAvest <[email protected]>
> > > > Signed-off-by: Guenter Roeck <[email protected]>
> > > > Signed-off-by: Ricardo Cañuelo <[email protected]>
> > > > ---
> > >
> > > 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
> > > >