[PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rajat Jain]

Ref: https://lkml.org/lkml/2017/9/19/649

The bus controllers should suspend the bus operations only after
all of the devices on the bus have suspended their device
completely. Since the i2c_client drivers could be talking to
their devices in their suspend_late() calls, lets ensure that the
bus is alive by that time. Thus moving the controller suspend logic to
suspend_late().

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0e65b97842b4..66dd7f844c40 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
 	.complete = dw_i2c_plat_complete,
-	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
 	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
 			   dw_i2c_plat_resume,
 			   NULL)
-- 
2.14.1.821.g8fa685d3b7-goog

[PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Rajat Jain]

Ref: https://lkml.org/lkml/2017/9/19/649

The intel-lpss hosts the designware i2c controller device, which
needs to be up and running until all its i2c child devices have
suspended (child devices need to be accessible over i2c until
suspend_late() has been called for all those devices).

So lets delay the resetting of the controller until suspend_late().
I thought of renaming function to say *_late / *_early but that
might cause confusion since the same function is also used for runtime
suspend/resume.

Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index 694116630ffa..865bbeaaf00c 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
-	.freeze = intel_lpss_suspend,		\
-	.thaw = intel_lpss_resume,		\
-	.poweroff = intel_lpss_suspend,		\
-	.restore = intel_lpss_resume,
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rajat Jain]

On Wed, Sep 20, 2017 at 3:31 PM, Rajat Jain <rajatja@google.com> wrote:
> Ref: https://lkml.org/lkml/2017/9/19/649
>
> The bus controllers should suspend the bus operations only after
> all of the devices on the bus have suspended their device
> completely. Since the i2c_client drivers could be talking to
> their devices in their suspend_late() calls, lets ensure that the
> bus is alive by that time. Thus moving the controller suspend logic to
> suspend_late().
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---

Forgot to mention, this was needed because we were seeing problems
when the ACPI routines (called at resume_early() time) for the i2c
client device were trying to access one of the I2C devices, and were
failing because the bus was not ready.

Thanks,

Rajat


>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0e65b97842b4..66dd7f844c40 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>         .prepare = dw_i2c_plat_prepare,
>         .complete = dw_i2c_plat_complete,
> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>                            dw_i2c_plat_resume,
>                            NULL)
> --
> 2.14.1.821.g8fa685d3b7-goog
>

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rafael J. Wysocki]

On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> Ref: https://lkml.org/lkml/2017/9/19/649
>
> The bus controllers should suspend the bus operations only after
> all of the devices on the bus have suspended their device
> completely. Since the i2c_client drivers could be talking to
> their devices in their suspend_late() calls, lets ensure that the
> bus is alive by that time. Thus moving the controller suspend logic to
> suspend_late().
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0e65b97842b4..66dd7f844c40 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>         .prepare = dw_i2c_plat_prepare,
>         .complete = dw_i2c_plat_complete,
> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>                            dw_i2c_plat_resume,
>                            NULL)

No, you can't just do that.

I sent patches to do it properly before my trip to LA last week, it
shouldn't be overly difficult to find them in the mailing list
archives.  I can look them up tomorrow if need be.

Thanks,
Rafael

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Rafael J. Wysocki]

On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> Ref: https://lkml.org/lkml/2017/9/19/649
>
> The intel-lpss hosts the designware i2c controller device, which
> needs to be up and running until all its i2c child devices have
> suspended (child devices need to be accessible over i2c until
> suspend_late() has been called for all those devices).
>
> So lets delay the resetting of the controller until suspend_late().
> I thought of renaming function to say *_late / *_early but that
> might cause confusion since the same function is also used for runtime
> suspend/resume.
>
> Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/mfd/intel-lpss.h | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> index 694116630ffa..865bbeaaf00c 100644
> --- a/drivers/mfd/intel-lpss.h
> +++ b/drivers/mfd/intel-lpss.h
> @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
>  #ifdef CONFIG_PM_SLEEP
>  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>         .prepare = intel_lpss_prepare,          \
> -       .suspend = intel_lpss_suspend,          \
> -       .resume = intel_lpss_resume,            \
> -       .freeze = intel_lpss_suspend,           \
> -       .thaw = intel_lpss_resume,              \
> -       .poweroff = intel_lpss_suspend,         \
> -       .restore = intel_lpss_resume,
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
>  #else
>  #define INTEL_LPSS_SLEEP_PM_OPS
>  #endif

So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/

Thanks,
Rafael

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rajat Jain]

On Wed, Sep 20, 2017 at 5:24 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>> Ref: https://lkml.org/lkml/2017/9/19/649
>>
>> The bus controllers should suspend the bus operations only after
>> all of the devices on the bus have suspended their device
>> completely. Since the i2c_client drivers could be talking to
>> their devices in their suspend_late() calls, lets ensure that the
>> bus is alive by that time. Thus moving the controller suspend logic to
>> suspend_late().
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 0e65b97842b4..66dd7f844c40 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>>         .prepare = dw_i2c_plat_prepare,
>>         .complete = dw_i2c_plat_complete,
>> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>>                            dw_i2c_plat_resume,
>>                            NULL)
>
> No, you can't just do that.
>
> I sent patches to do it properly before my trip to LA last week, it
> shouldn't be overly difficult to find them in the mailing list
> archives.  I can look them up tomorrow if need be.

Thanks, I am guessing you mean this?

https://patchwork.kernel.org/patch/9939807/

>
> Thanks,
> Rafael

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Rajat Jain]

On Wed, Sep 20, 2017 at 5:27 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>> Ref: https://lkml.org/lkml/2017/9/19/649
>>
>> The intel-lpss hosts the designware i2c controller device, which
>> needs to be up and running until all its i2c child devices have
>> suspended (child devices need to be accessible over i2c until
>> suspend_late() has been called for all those devices).
>>
>> So lets delay the resetting of the controller until suspend_late().
>> I thought of renaming function to say *_late / *_early but that
>> might cause confusion since the same function is also used for runtime
>> suspend/resume.
>>
>> Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> ---
>>  drivers/mfd/intel-lpss.h | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
>> index 694116630ffa..865bbeaaf00c 100644
>> --- a/drivers/mfd/intel-lpss.h
>> +++ b/drivers/mfd/intel-lpss.h
>> @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
>>  #ifdef CONFIG_PM_SLEEP
>>  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>>         .prepare = intel_lpss_prepare,          \
>> -       .suspend = intel_lpss_suspend,          \
>> -       .resume = intel_lpss_resume,            \
>> -       .freeze = intel_lpss_suspend,           \
>> -       .thaw = intel_lpss_resume,              \
>> -       .poweroff = intel_lpss_suspend,         \
>> -       .restore = intel_lpss_resume,
>> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
>>  #else
>>  #define INTEL_LPSS_SLEEP_PM_OPS
>>  #endif
>
> So I sent this exact patch several days ago:
> https://patchwork.kernel.org/patch/9939809/

Sorry, I missed it. Please consider my patch withdrawn / self-nack'ed
in favor of Rafael's patch.

>
> Thanks,
> Rafael

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Lee Jones]

On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:

> On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> > Ref: https://lkml.org/lkml/2017/9/19/649
> >
> > The intel-lpss hosts the designware i2c controller device, which
> > needs to be up and running until all its i2c child devices have
> > suspended (child devices need to be accessible over i2c until
> > suspend_late() has been called for all those devices).
> >
> > So lets delay the resetting of the controller until suspend_late().
> > I thought of renaming function to say *_late / *_early but that
> > might cause confusion since the same function is also used for runtime
> > suspend/resume.
> >
> > Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/mfd/intel-lpss.h | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > index 694116630ffa..865bbeaaf00c 100644
> > --- a/drivers/mfd/intel-lpss.h
> > +++ b/drivers/mfd/intel-lpss.h
> > @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
> >  #ifdef CONFIG_PM_SLEEP
> >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
> >         .prepare = intel_lpss_prepare,          \
> > -       .suspend = intel_lpss_suspend,          \
> > -       .resume = intel_lpss_resume,            \
> > -       .freeze = intel_lpss_suspend,           \
> > -       .thaw = intel_lpss_resume,              \
> > -       .poweroff = intel_lpss_suspend,         \
> > -       .restore = intel_lpss_resume,
> > +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
> >  #else
> >  #define INTEL_LPSS_SLEEP_PM_OPS
> >  #endif
> 
> So I sent this exact patch several days ago:
> https://patchwork.kernel.org/patch/9939809/

You did?  Any reason you didn't send it to me?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Rafael J. Wysocki]

On Thursday, September 21, 2017 10:41:33 AM CEST Lee Jones wrote:
> On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:
> 
> > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> > > Ref: https://lkml.org/lkml/2017/9/19/649
> > >
> > > The intel-lpss hosts the designware i2c controller device, which
> > > needs to be up and running until all its i2c child devices have
> > > suspended (child devices need to be accessible over i2c until
> > > suspend_late() has been called for all those devices).
> > >
> > > So lets delay the resetting of the controller until suspend_late().
> > > I thought of renaming function to say *_late / *_early but that
> > > might cause confusion since the same function is also used for runtime
> > > suspend/resume.
> > >
> > > Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > ---
> > >  drivers/mfd/intel-lpss.h | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > > index 694116630ffa..865bbeaaf00c 100644
> > > --- a/drivers/mfd/intel-lpss.h
> > > +++ b/drivers/mfd/intel-lpss.h
> > > @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
> > >  #ifdef CONFIG_PM_SLEEP
> > >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
> > >         .prepare = intel_lpss_prepare,          \
> > > -       .suspend = intel_lpss_suspend,          \
> > > -       .resume = intel_lpss_resume,            \
> > > -       .freeze = intel_lpss_suspend,           \
> > > -       .thaw = intel_lpss_resume,              \
> > > -       .poweroff = intel_lpss_suspend,         \
> > > -       .restore = intel_lpss_resume,
> > > +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
> > >  #else
> > >  #define INTEL_LPSS_SLEEP_PM_OPS
> > >  #endif
> > 
> > So I sent this exact patch several days ago:
> > https://patchwork.kernel.org/patch/9939809/
> 
> You did?

Yes, I did.

> Any reason you didn't send it to me?

Well, Lee Jones <lee@kernel.org> is very much in the CC list of it.

Was I expected to use a different e-mail address for that?

Thanks,
Rafael

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rafael J. Wysocki]

On Thursday, September 21, 2017 3:13:56 AM CEST Rajat Jain wrote:
> On Wed, Sep 20, 2017 at 5:24 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> >> Ref: https://lkml.org/lkml/2017/9/19/649
> >>
> >> The bus controllers should suspend the bus operations only after
> >> all of the devices on the bus have suspended their device
> >> completely. Since the i2c_client drivers could be talking to
> >> their devices in their suspend_late() calls, lets ensure that the
> >> bus is alive by that time. Thus moving the controller suspend logic to
> >> suspend_late().
> >>
> >> Signed-off-by: Rajat Jain <rajatja@google.com>
> >> ---
> >>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >> index 0e65b97842b4..66dd7f844c40 100644
> >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
> >>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> >>         .prepare = dw_i2c_plat_prepare,
> >>         .complete = dw_i2c_plat_complete,
> >> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> >> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> >>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
> >>                            dw_i2c_plat_resume,
> >>                            NULL)
> >
> > No, you can't just do that.
> >
> > I sent patches to do it properly before my trip to LA last week, it
> > shouldn't be overly difficult to find them in the mailing list
> > archives.  I can look them up tomorrow if need be.
> 
> Thanks, I am guessing you mean this?
> 
> https://patchwork.kernel.org/patch/9939807/

Yes, that's what I mean.

Thanks,
Rafael

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Lee Jones]

On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:

> On Thursday, September 21, 2017 10:41:33 AM CEST Lee Jones wrote:
> > On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:
> > 
> > > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
> > > > Ref: https://lkml.org/lkml/2017/9/19/649
> > > >
> > > > The intel-lpss hosts the designware i2c controller device, which
> > > > needs to be up and running until all its i2c child devices have
> > > > suspended (child devices need to be accessible over i2c until
> > > > suspend_late() has been called for all those devices).
> > > >
> > > > So lets delay the resetting of the controller until suspend_late().
> > > > I thought of renaming function to say *_late / *_early but that
> > > > might cause confusion since the same function is also used for runtime
> > > > suspend/resume.
> > > >
> > > > Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > ---
> > > >  drivers/mfd/intel-lpss.h | 7 +------
> > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > > > index 694116630ffa..865bbeaaf00c 100644
> > > > --- a/drivers/mfd/intel-lpss.h
> > > > +++ b/drivers/mfd/intel-lpss.h
> > > > @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
> > > >  #ifdef CONFIG_PM_SLEEP
> > > >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
> > > >         .prepare = intel_lpss_prepare,          \
> > > > -       .suspend = intel_lpss_suspend,          \
> > > > -       .resume = intel_lpss_resume,            \
> > > > -       .freeze = intel_lpss_suspend,           \
> > > > -       .thaw = intel_lpss_resume,              \
> > > > -       .poweroff = intel_lpss_suspend,         \
> > > > -       .restore = intel_lpss_resume,
> > > > +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
> > > >  #else
> > > >  #define INTEL_LPSS_SLEEP_PM_OPS
> > > >  #endif
> > > 
> > > So I sent this exact patch several days ago:
> > > https://patchwork.kernel.org/patch/9939809/
> > 
> > You did?
> 
> Yes, I did.
> 
> > Any reason you didn't send it to me?
> 
> Well, Lee Jones <lee@kernel.org> is very much in the CC list of it.
> 
> Was I expected to use a different e-mail address for that?

I don't monitor that one.  Better to use the one in MAINTAINERS.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

[Rafael J. Wysocki]

On Thu, Sep 21, 2017 at 5:17 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:
>
>> On Thursday, September 21, 2017 10:41:33 AM CEST Lee Jones wrote:
>> > On Thu, 21 Sep 2017, Rafael J. Wysocki wrote:
>> >
>> > > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>> > > > Ref: https://lkml.org/lkml/2017/9/19/649
>> > > >
>> > > > The intel-lpss hosts the designware i2c controller device, which
>> > > > needs to be up and running until all its i2c child devices have
>> > > > suspended (child devices need to be accessible over i2c until
>> > > > suspend_late() has been called for all those devices).
>> > > >
>> > > > So lets delay the resetting of the controller until suspend_late().
>> > > > I thought of renaming function to say *_late / *_early but that
>> > > > might cause confusion since the same function is also used for runtime
>> > > > suspend/resume.
>> > > >
>> > > > Fixes: 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
>> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
>> > > > ---
>> > > >  drivers/mfd/intel-lpss.h | 7 +------
>> > > >  1 file changed, 1 insertion(+), 6 deletions(-)
>> > > >
>> > > > diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
>> > > > index 694116630ffa..865bbeaaf00c 100644
>> > > > --- a/drivers/mfd/intel-lpss.h
>> > > > +++ b/drivers/mfd/intel-lpss.h
>> > > > @@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
>> > > >  #ifdef CONFIG_PM_SLEEP
>> > > >  #define INTEL_LPSS_SLEEP_PM_OPS                        \
>> > > >         .prepare = intel_lpss_prepare,          \
>> > > > -       .suspend = intel_lpss_suspend,          \
>> > > > -       .resume = intel_lpss_resume,            \
>> > > > -       .freeze = intel_lpss_suspend,           \
>> > > > -       .thaw = intel_lpss_resume,              \
>> > > > -       .poweroff = intel_lpss_suspend,         \
>> > > > -       .restore = intel_lpss_resume,
>> > > > +       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
>> > > >  #else
>> > > >  #define INTEL_LPSS_SLEEP_PM_OPS
>> > > >  #endif
>> > >
>> > > So I sent this exact patch several days ago:
>> > > https://patchwork.kernel.org/patch/9939809/
>> >
>> > You did?
>>
>> Yes, I did.
>>
>> > Any reason you didn't send it to me?
>>
>> Well, Lee Jones <lee@kernel.org> is very much in the CC list of it.
>>
>> Was I expected to use a different e-mail address for that?
>
> I don't monitor that one.  Better to use the one in MAINTAINERS.

OK, I will in the future.

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rafael J. Wysocki]

On Thu, Sep 21, 2017 at 4:11 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, September 21, 2017 3:13:56 AM CEST Rajat Jain wrote:
>> On Wed, Sep 20, 2017 at 5:24 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>> >> Ref: https://lkml.org/lkml/2017/9/19/649
>> >>
>> >> The bus controllers should suspend the bus operations only after
>> >> all of the devices on the bus have suspended their device
>> >> completely. Since the i2c_client drivers could be talking to
>> >> their devices in their suspend_late() calls, lets ensure that the
>> >> bus is alive by that time. Thus moving the controller suspend logic to
>> >> suspend_late().
>> >>
>> >> Signed-off-by: Rajat Jain <rajatja@google.com>
>> >> ---
>> >>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> >> index 0e65b97842b4..66dd7f844c40 100644
>> >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> >> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>> >>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>> >>         .prepare = dw_i2c_plat_prepare,
>> >>         .complete = dw_i2c_plat_complete,
>> >> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>> >> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>> >>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>> >>                            dw_i2c_plat_resume,
>> >>                            NULL)
>> >
>> > No, you can't just do that.
>> >
>> > I sent patches to do it properly before my trip to LA last week, it
>> > shouldn't be overly difficult to find them in the mailing list
>> > archives.  I can look them up tomorrow if need be.
>>
>> Thanks, I am guessing you mean this?
>>
>> https://patchwork.kernel.org/patch/9939807/
>
> Yes, that's what I mean.

BTW, does this patchset work for you?

Thanks,
Rafael

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rajat Jain]

On Sat, Sep 23, 2017 at 5:55 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Sep 21, 2017 at 4:11 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, September 21, 2017 3:13:56 AM CEST Rajat Jain wrote:
>>> On Wed, Sep 20, 2017 at 5:24 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>>> >> Ref: https://lkml.org/lkml/2017/9/19/649
>>> >>
>>> >> The bus controllers should suspend the bus operations only after
>>> >> all of the devices on the bus have suspended their device
>>> >> completely. Since the i2c_client drivers could be talking to
>>> >> their devices in their suspend_late() calls, lets ensure that the
>>> >> bus is alive by that time. Thus moving the controller suspend logic to
>>> >> suspend_late().
>>> >>
>>> >> Signed-off-by: Rajat Jain <rajatja@google.com>
>>> >> ---
>>> >>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> >> index 0e65b97842b4..66dd7f844c40 100644
>>> >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> >> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>>> >>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>>> >>         .prepare = dw_i2c_plat_prepare,
>>> >>         .complete = dw_i2c_plat_complete,
>>> >> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>> >> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>> >>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>>> >>                            dw_i2c_plat_resume,
>>> >>                            NULL)
>>> >
>>> > No, you can't just do that.
>>> >
>>> > I sent patches to do it properly before my trip to LA last week, it
>>> > shouldn't be overly difficult to find them in the mailing list
>>> > archives.  I can look them up tomorrow if need be.
>>>
>>> Thanks, I am guessing you mean this?
>>>
>>> https://patchwork.kernel.org/patch/9939807/
>>
>> Yes, that's what I mean.
>
> BTW, does this patchset work for you?

Yes, I don't see the issue I was seeing earlier with your patch.
Please feel free to add

Tested-by: Rajat Jain <rajatja@google.com>

>
> Thanks,
> Rafael

Re: [PATCH 1/2] i2c: designware: switch to suspend_late/resume_early

[Rajat Jain]

On Sat, Sep 23, 2017 at 8:49 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> On Sat, Sep 23, 2017 at 5:55 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Sep 21, 2017 at 4:11 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Thursday, September 21, 2017 3:13:56 AM CEST Rajat Jain wrote:
>>>> On Wed, Sep 20, 2017 at 5:24 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> > On Thu, Sep 21, 2017 at 12:31 AM, Rajat Jain <rajatja@google.com> wrote:
>>>> >> Ref: https://lkml.org/lkml/2017/9/19/649
>>>> >>
>>>> >> The bus controllers should suspend the bus operations only after
>>>> >> all of the devices on the bus have suspended their device
>>>> >> completely. Since the i2c_client drivers could be talking to
>>>> >> their devices in their suspend_late() calls, lets ensure that the
>>>> >> bus is alive by that time. Thus moving the controller suspend logic to
>>>> >> suspend_late().
>>>> >>
>>>> >> Signed-off-by: Rajat Jain <rajatja@google.com>
>>>> >> ---
>>>> >>  drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
>>>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> >>
>>>> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> >> index 0e65b97842b4..66dd7f844c40 100644
>>>> >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> >> @@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
>>>> >>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>>>> >>         .prepare = dw_i2c_plat_prepare,
>>>> >>         .complete = dw_i2c_plat_complete,
>>>> >> -       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>>> >> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>>> >>         SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>>>> >>                            dw_i2c_plat_resume,
>>>> >>                            NULL)
>>>> >
>>>> > No, you can't just do that.
>>>> >
>>>> > I sent patches to do it properly before my trip to LA last week, it
>>>> > shouldn't be overly difficult to find them in the mailing list
>>>> > archives.  I can look them up tomorrow if need be.
>>>>
>>>> Thanks, I am guessing you mean this?
>>>>
>>>> https://patchwork.kernel.org/patch/9939807/
>>>
>>> Yes, that's what I mean.
>>
>> BTW, does this patchset work for you?
>
> Yes, I don't see the issue I was seeing earlier with your patch.
> Please feel free to adds,

I think I made my sentence ambiguous. Let me rephrase: Yes, with your
patch, I don't see the issue I was seeing earlier (without your patch)

>
> Tested-by: Rajat Jain <rajatja@google.com>
>
>>
>> Thanks,
>> Rafael