On Wed, Aug 29, 2018 at 03:06:31PM +0200, Hans de Goede wrote:
> On Bay Trail and Cherry Trail devices we set the pm_disabled flag for I2C
> busses which the OS shares with the PUNIT as these need special handling.
> Until now we called dev_pm_syscore_device(dev, true) for I2C controllers
> with this flag set to keep these I2C controllers always on.
>
> After commit 12864ff8545f ("ACPI / LPSS: Avoid PM quirks on suspend and
> resume from hibernation"), this no longer works. This commit modifies
> lpss_iosf_exit_d3_state() to only run if lpss_iosf_enter_d3_state() has ran
> before it, so that it does not run on a resume from hibernate (or from S3).
>
> On these systems the conditions for lpss_iosf_enter_d3_state() to run
> never become true, so lpss_iosf_exit_d3_state() never gets called and
> the 2 LPSS DMA controllers never get forced into D0 mode, instead they
> are left in their default automatic power-on when needed mode.
>
> The not forcing of D0 mode for the DMA controllers enables these systems
> to properly enter S0ix modes, which is a good thing.
>
> But after entering S0ix modes the I2C controller connected to the PMIC
> no longer works, leading to e.g. broken battery monitoring.
>
> The _PS3 method for this I2C controller looks like this:
>
> Method (_PS3, 0, NotSerialized) // _PS3: Power State 3
> {
> If ((((PMID == 0x04) || (PMID == 0x05)) || (PMID == 0x06)))
> {
> Return (Zero)
> }
>
> PSAT |= 0x03
> Local0 = PSAT /* \_SB_.I2C5.PSAT */
> }
>
> Where PMID = 0x05, so we enter the Return (Zero) path on these systems.
>
> So even if we were to not call dev_pm_syscore_device(dev, true) the
> I2C controller will be left in D0 rather then be switched to D3.
>
> Yet on other Bay and Cherry Trail devices S0ix is not entered unless *all*
> I2C controllers are in D3 mode. This combined with the I2C controller no
> longer working now that we reach S0ix states on these systems leads to me
> believing that the PUNIT itself puts the I2C controller in D3 when all
> other conditions for entering S0ix states are true.
>
> Since now the I2C controller is put in D3 over a suspend/resume we must
> re-initialize it afterwards and that does indeed fix it no longer working.
>
> This commit implements this fix by:
>
> 1) Making the suspend_late callback a no-op if pm_disabled is set and
> making the resume_early callback skip the clock re-enable (since it now was
> not disabled) while still doing the necessary I2C controller re-init.
>
> 2) Removing the dev_pm_syscore_device(dev, true) call, so that the suspend
> and resume callbacks are actually called. Normally this would cause the
> ACPI pm code to call _PS3 putting the I2C controller in D3, wreaking havoc
> since it is shared with the PUNIT, but in this special case the _PS3 method
> is a no-op so we can safely allow a "fake" suspend / resume.
>
Oh, this hardware platform design in a real PITA.
Thank you for so good analysis!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
for both patches.
> Fixes: 12864ff8545f ("ACPI / LPSS: Avoid PM quirks on suspend and resume ...")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200861
> Cc: 4.15+ <stable@xxxxxxxxxxxxxxx> # 4.15+
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-designware-master.c | 1 -
> drivers/i2c/busses/i2c-designware-platdrv.c | 7 ++++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 27436a937492..54b2a3a86677 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -693,7 +693,6 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> i2c_set_adapdata(adap, dev);
>
> if (dev->pm_disabled) {
> - dev_pm_syscore_device(dev->dev, true);
> irq_flags = IRQF_NO_SUSPEND;
> } else {
> irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 5660daf6c92e..d281d21cdd8e 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -448,6 +448,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
> {
> struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>
> + if (i_dev->pm_disabled)
> + return 0;
> +
> i_dev->disable(i_dev);
> i2c_dw_prepare_clk(i_dev, false);
>
> @@ -458,7 +461,9 @@ static int dw_i2c_plat_resume(struct device *dev)
> {
> struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>
> - i2c_dw_prepare_clk(i_dev, true);
> + if (!i_dev->pm_disabled)
> + i2c_dw_prepare_clk(i_dev, true);
> +
> i_dev->init(i_dev);
>
> return 0;
> --
> 2.19.0.rc0
>
--
With Best Regards,
Andy Shevchenko
