i2c: designware: Resume PMICs shared with the PUNIT earlier
diff mbox series

Message ID 20180908182959.5038-2-hdegoede@redhat.com
State Not Applicable
Headers show
Series
  • i2c: designware: Resume PMICs shared with the PUNIT earlier
Related show

Commit Message

Hans de Goede Sept. 8, 2018, 6:29 p.m. UTC
After recent kernel changes Bay and Cherry Trail devices where the I2C bus
to the PMIC is shared with PUNIT properly reach S0ix states when suspended.

As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
designware: Re-init controllers with pm_disabled set on resume"), the
I2C controller for this bus has an effectively empty _PS3 ACPI method and
thus is left in D0 during suspend. It seems to somehow automatically
powerdown once S0i3 is reached, which means that it needs to be
reinitialized on resume for us to be able to use it again.

The referred commit adds the necessary code to re-init the controller
to the drivers resume_early method. It turns out that for some devices
this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
from resume_early leads to the device rebooting on resume.

This commit moves the re-init to resume_noirq, fixing this. Note this
means that the controller re-init now happens before the early_resume
handler from drivers/acpi/acpi_lpss.c runs. Which means that the
controller was not put in D0 yet when we re-init, this is not a problem
since the controller was never put in D3 in the first place.

This has been tested on the Peaq C1010 and one other Bay Trail device and
Cherry Trail device, both of which have an I2C bus shared with the PUNIT
too.

Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 22 ++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Mika Westerberg Sept. 10, 2018, 12:29 p.m. UTC | #1
On Sat, Sep 08, 2018 at 08:29:59PM +0200, Hans de Goede wrote:
> After recent kernel changes Bay and Cherry Trail devices where the I2C bus
> to the PMIC is shared with PUNIT properly reach S0ix states when suspended.
> 
> As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
> designware: Re-init controllers with pm_disabled set on resume"), the
> I2C controller for this bus has an effectively empty _PS3 ACPI method and
> thus is left in D0 during suspend. It seems to somehow automatically
> powerdown once S0i3 is reached, which means that it needs to be
> reinitialized on resume for us to be able to use it again.
> 
> The referred commit adds the necessary code to re-init the controller
> to the drivers resume_early method. It turns out that for some devices
> this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
> from resume_early leads to the device rebooting on resume.
> 
> This commit moves the re-init to resume_noirq, fixing this. Note this
> means that the controller re-init now happens before the early_resume
> handler from drivers/acpi/acpi_lpss.c runs. Which means that the
> controller was not put in D0 yet when we re-init, this is not a problem
> since the controller was never put in D3 in the first place.
> 
> This has been tested on the Peaq C1010 and one other Bay Trail device and
> Cherry Trail device, both of which have an I2C bus shared with the PUNIT
> too.
> 
> Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Jarkko Nikula Sept. 11, 2018, 11:39 a.m. UTC | #2
On 09/10/2018 03:29 PM, Mika Westerberg wrote:
> On Sat, Sep 08, 2018 at 08:29:59PM +0200, Hans de Goede wrote:
>> After recent kernel changes Bay and Cherry Trail devices where the I2C bus
>> to the PMIC is shared with PUNIT properly reach S0ix states when suspended.
>>
>> As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
>> designware: Re-init controllers with pm_disabled set on resume"), the
>> I2C controller for this bus has an effectively empty _PS3 ACPI method and
>> thus is left in D0 during suspend. It seems to somehow automatically
>> powerdown once S0i3 is reached, which means that it needs to be
>> reinitialized on resume for us to be able to use it again.
>>
>> The referred commit adds the necessary code to re-init the controller
>> to the drivers resume_early method. It turns out that for some devices
>> this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
>> from resume_early leads to the device rebooting on resume.
>>
>> This commit moves the re-init to resume_noirq, fixing this. Note this
>> means that the controller re-init now happens before the early_resume
>> handler from drivers/acpi/acpi_lpss.c runs. Which means that the
>> controller was not put in D0 yet when we re-init, this is not a problem
>> since the controller was never put in D3 in the first place.
>>
>> This has been tested on the Peaq C1010 and one other Bay Trail device and
>> Cherry Trail device, both of which have an I2C bus shared with the PUNIT
>> too.
>>
>> Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
Would it be better to leave out Fixes tag in order to not needlessly 
consume Greg's time or state explicitly that 9d9a152ebaa8 becomes 
problem only if f11fc4bc669b ("ACPI / LPSS: Force LPSS quirks on boot") 
is applied?

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Hans de Goede Sept. 11, 2018, 12:01 p.m. UTC | #3
HI,

On 11-09-18 13:39, Jarkko Nikula wrote:
> On 09/10/2018 03:29 PM, Mika Westerberg wrote:
>> On Sat, Sep 08, 2018 at 08:29:59PM +0200, Hans de Goede wrote:
>>> After recent kernel changes Bay and Cherry Trail devices where the I2C bus
>>> to the PMIC is shared with PUNIT properly reach S0ix states when suspended.
>>>
>>> As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
>>> designware: Re-init controllers with pm_disabled set on resume"), the
>>> I2C controller for this bus has an effectively empty _PS3 ACPI method and
>>> thus is left in D0 during suspend. It seems to somehow automatically
>>> powerdown once S0i3 is reached, which means that it needs to be
>>> reinitialized on resume for us to be able to use it again.
>>>
>>> The referred commit adds the necessary code to re-init the controller
>>> to the drivers resume_early method. It turns out that for some devices
>>> this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
>>> from resume_early leads to the device rebooting on resume.
>>>
>>> This commit moves the re-init to resume_noirq, fixing this. Note this
>>> means that the controller re-init now happens before the early_resume
>>> handler from drivers/acpi/acpi_lpss.c runs. Which means that the
>>> controller was not put in D0 yet when we re-init, this is not a problem
>>> since the controller was never put in D3 in the first place.
>>>
>>> This has been tested on the Peaq C1010 and one other Bay Trail device and
>>> Cherry Trail device, both of which have an I2C bus shared with the PUNIT
>>> too.
>>>
>>> Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
> Would it be better to leave out Fixes tag in order to not needlessly consume Greg's time or state explicitly that 9d9a152ebaa8 becomes problem only if f11fc4bc669b ("ACPI / LPSS: Force LPSS quirks on boot") is applied?

Things are a bit more complicated.

9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")

Is necessary to fix issues when BYT/CHT devices with a shared I2C bus
are able to reach S0ix states:

f11fc4bc669b ("ACPI / LPSS: Force LPSS quirks on boot")

Which is expected to go to stable actually (re)breaks S0ix states
on these devices, but it fixes some devices not booting, so we
still want it in stable.

With f11fc4bc669b in place 9d9a152ebaa8 itself is thus no longer
necessary since S0ix states are no longer reached and it could
be dropped / removed from stable, but that would require reverting
it which feels like needless churn.

This patch fixes 9d9a152ebaa8, which as said itself is not needed
as long as these systems do not reach S0ix. But if we have it
in stable anyways it seems like a good idea to have the full fix
in stable, so that if someone backports a fix which re-enabled
S0ix states again things don't break.

> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thanks.

Regards,

Hans
Hans de Goede Sept. 16, 2018, 12:11 p.m. UTC | #4
Hi,

On 08-09-18 20:29, Hans de Goede wrote:
> After recent kernel changes Bay and Cherry Trail devices where the I2C bus
> to the PMIC is shared with PUNIT properly reach S0ix states when suspended.
> 
> As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
> designware: Re-init controllers with pm_disabled set on resume"), the
> I2C controller for this bus has an effectively empty _PS3 ACPI method and
> thus is left in D0 during suspend. It seems to somehow automatically
> powerdown once S0i3 is reached, which means that it needs to be
> reinitialized on resume for us to be able to use it again.
> 
> The referred commit adds the necessary code to re-init the controller
> to the drivers resume_early method. It turns out that for some devices
> this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
> from resume_early leads to the device rebooting on resume.
> 
> This commit moves the re-init to resume_noirq, fixing this. Note this
> means that the controller re-init now happens before the early_resume
> handler from drivers/acpi/acpi_lpss.c runs. Which means that the
> controller was not put in D0 yet when we re-init, this is not a problem
> since the controller was never put in D3 in the first place.
> 
> This has been tested on the Peaq C1010 and one other Bay Trail device and
> Cherry Trail device, both of which have an I2C bus shared with the PUNIT
> too.
> 
> Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Self NACK, drivers having a suspend / resume no_irq method will override
the bus's suspend / resume no_irq method rather then being chained by
those.

I've another patchset which I will send out soon (this week) which fixes
this in a different manner which does not have that down side and this
patch actually gets in the way of that series.

Regards,

Hans






> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 22 ++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e92fdb46b5a0..3f96d048ce2d 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -429,8 +429,27 @@ static int dw_i2c_plat_resume(struct device *dev)
>   {
>   	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>   
> +	if (i_dev->shared_with_punit)
> +		return 0;
> +
> +	i2c_dw_prepare_clk(i_dev, true);
> +	i_dev->init(i_dev);
> +
> +	return 0;
> +}
> +
> +/*
> + * This is only for (Intel BYT/CHT) devices where the I2C bus to the PMIC
> + * is shared with the SoC's PUNIT. On these devices the controller is never
> + * turned off by us, but is automatically powered down in hardware. So we
> + * need to re-init it and we need to do this as early as possible.
> + */
> +static int dw_i2c_plat_resume_noirq(struct device *dev)
> +{
> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> +
>   	if (!i_dev->shared_with_punit)
> -		i2c_dw_prepare_clk(i_dev, true);
> +		return 0;
>   
>   	i_dev->init(i_dev);
>   
> @@ -440,6 +459,7 @@ static int dw_i2c_plat_resume(struct device *dev)
>   static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>   	.prepare = dw_i2c_plat_prepare,
>   	.complete = dw_i2c_plat_complete,
> +	.resume_noirq = dw_i2c_plat_resume_noirq,
>   	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>   	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
>   };
>
Hans de Goede Sept. 16, 2018, 12:39 p.m. UTC | #5
Hi,

On 16-09-18 14:11, Hans de Goede wrote:
> Hi,
> 
> On 08-09-18 20:29, Hans de Goede wrote:
>> After recent kernel changes Bay and Cherry Trail devices where the I2C bus
>> to the PMIC is shared with PUNIT properly reach S0ix states when suspended.
>>
>> As explained in detail in the commit message of commit 9d9a152ebaa8 ("i2c:
>> designware: Re-init controllers with pm_disabled set on resume"), the
>> I2C controller for this bus has an effectively empty _PS3 ACPI method and
>> thus is left in D0 during suspend. It seems to somehow automatically
>> powerdown once S0i3 is reached, which means that it needs to be
>> reinitialized on resume for us to be able to use it again.
>>
>> The referred commit adds the necessary code to re-init the controller
>> to the drivers resume_early method. It turns out that for some devices
>> this is too late. On a Peaq C1010 2-in-1 tablet/laptop doing the re-init
>> from resume_early leads to the device rebooting on resume.
>>
>> This commit moves the re-init to resume_noirq, fixing this. Note this
>> means that the controller re-init now happens before the early_resume
>> handler from drivers/acpi/acpi_lpss.c runs. Which means that the
>> controller was not put in D0 yet when we re-init, this is not a problem
>> since the controller was never put in D3 in the first place.
>>
>> This has been tested on the Peaq C1010 and one other Bay Trail device and
>> Cherry Trail device, both of which have an I2C bus shared with the PUNIT
>> too.
>>
>> Fixes: 9d9a152ebaa8 ("i2c: designware: Re-init controllers with ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Self NACK, drivers having a suspend / resume no_irq method will override
> the bus's suspend / resume no_irq method rather then being chained by
> those.

Correction, I was misreading the drivers/base/power/main.c code, the
above is not true.

> I've another patchset which I will send out soon (this week) which fixes
> this in a different manner which does not have that down side and this
> patch actually gets in the way of that series.

Still my new patch-set will solve this in another way, so the self NACK
still stands.

Regards,

Hans





>> ---
>>   drivers/i2c/busses/i2c-designware-platdrv.c | 22 ++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index e92fdb46b5a0..3f96d048ce2d 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -429,8 +429,27 @@ static int dw_i2c_plat_resume(struct device *dev)
>>   {
>>       struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +    if (i_dev->shared_with_punit)
>> +        return 0;
>> +
>> +    i2c_dw_prepare_clk(i_dev, true);
>> +    i_dev->init(i_dev);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * This is only for (Intel BYT/CHT) devices where the I2C bus to the PMIC
>> + * is shared with the SoC's PUNIT. On these devices the controller is never
>> + * turned off by us, but is automatically powered down in hardware. So we
>> + * need to re-init it and we need to do this as early as possible.
>> + */
>> +static int dw_i2c_plat_resume_noirq(struct device *dev)
>> +{
>> +    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>>       if (!i_dev->shared_with_punit)
>> -        i2c_dw_prepare_clk(i_dev, true);
>> +        return 0;
>>       i_dev->init(i_dev);
>> @@ -440,6 +459,7 @@ static int dw_i2c_plat_resume(struct device *dev)
>>   static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>>       .prepare = dw_i2c_plat_prepare,
>>       .complete = dw_i2c_plat_complete,
>> +    .resume_noirq = dw_i2c_plat_resume_noirq,
>>       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>>       SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
>>   };
>>

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index e92fdb46b5a0..3f96d048ce2d 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -429,8 +429,27 @@  static int dw_i2c_plat_resume(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
+	if (i_dev->shared_with_punit)
+		return 0;
+
+	i2c_dw_prepare_clk(i_dev, true);
+	i_dev->init(i_dev);
+
+	return 0;
+}
+
+/*
+ * This is only for (Intel BYT/CHT) devices where the I2C bus to the PMIC
+ * is shared with the SoC's PUNIT. On these devices the controller is never
+ * turned off by us, but is automatically powered down in hardware. So we
+ * need to re-init it and we need to do this as early as possible.
+ */
+static int dw_i2c_plat_resume_noirq(struct device *dev)
+{
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
+
 	if (!i_dev->shared_with_punit)
-		i2c_dw_prepare_clk(i_dev, true);
+		return 0;
 
 	i_dev->init(i_dev);
 
@@ -440,6 +459,7 @@  static int dw_i2c_plat_resume(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
 	.complete = dw_i2c_plat_complete,
+	.resume_noirq = dw_i2c_plat_resume_noirq,
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
 	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
 };