[PATCH 0/2] i2c: designware: PM fixes for i2c-bus used by system PMIC

[PATCH 0/2] i2c: designware: PM fixes for i2c-bus used by system PMIC

[Hans de Goede]

Hi All,

Note these 2 patches apply on top of my previous i2c-designware patches.
Daniel Vetter should merge these soon (together with the dependent i915
patches) and then provide a stable branch to merge into the i2c subsys
next branch.

These will not apply cleanly without these patches in places, but otherwise
they are pretty much stand alone and ready for review (and merging once
the other patches are in place).

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] i2c: designware: Never suspend i2c-busses used for accessing the system PMIC

[Hans de Goede]

Currently we are already setting a pm_runtime_disabled flag and disabling
runtime-pm for i2c-busses used for accessing the system PMIC on x86.
But this is not enough, there are ACPI opregions which may want to access
the PMIC during late-suspend and early-resume, so we need to completely
disable pm to be safe.

This commit renames the flag from pm_runtime_disabled to pm_disabled and
adds the following new behavior if the flag is set:

1) Call dev_pm_syscore_device(dev, true) which disables normal suspend /
   resume and remove the pm_runtime_disabled check from dw_i2c_plat_resume
   since that will now never get called. This fixes suspend_late handlers
   which use ACPI PMIC opregions causing errors like these:

  PM: Suspending system (freeze)
  PM: suspend of devices complete after 1127.751 msecs
  i2c_designware 808622C1:06: timeout waiting for bus ready
  ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion]
  acpi 80860F14:02: Failed to change power state to D3hot
  PM: late suspend of devices failed

2) Set IRQF_NO_SUSPEND irq flag. This fixes resume_early handlers which
   handlers which use ACPI PMIC opregions causing errors like these:

  PM: resume from suspend-to-idle
  i2c_designware 808622C1:06: controller timed out
  ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c |  2 +-
 drivers/i2c/busses/i2c-designware-core.c     | 10 ++++++++--
 drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
 drivers/i2c/busses/i2c-designware-platdrv.c  | 10 ++++------
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1749a0f..b8cc226 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -170,7 +170,7 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 	dev_info(dev->dev, "I2C bus managed by PUNIT\n");
 	dev->acquire_lock = baytrail_i2c_acquire;
 	dev->release_lock = baytrail_i2c_release;
-	dev->pm_runtime_disabled = true;
+	dev->pm_disabled = true;
 
 	pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
 			   PM_QOS_DEFAULT_VALUE);
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 8c3ba42..3d86059 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -962,6 +962,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
 int i2c_dw_probe(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
+	unsigned long irq_flags;
 	int r;
 	u32 reg;
 
@@ -998,9 +999,14 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 	adap->dev.parent = 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;
+
 	i2c_dw_disable_int(dev);
-	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
-			     IRQF_SHARED | IRQF_COND_SUSPEND,
+	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags,
 			     dev_name(dev->dev), dev);
 	if (r) {
 		dev_err(dev->dev, "failure requesting irq %i: %d\n",
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index a8e74ca..a4ac473 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -79,7 +79,7 @@
  * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
  * @acquire_lock: function to acquire a hardware lock on the bus
  * @release_lock: function to release a hardware lock on the bus
- * @pm_runtime_disabled: true if pm runtime is disabled
+ * @pm_disabled: true if power-management should be disabled for this i2c-bus
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -127,7 +127,7 @@ struct dw_i2c_dev {
 	struct pm_qos_request	pm_qos;
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
-	bool			pm_runtime_disabled;
+	bool			pm_disabled;
 	bool			dynamic_tar_update_enabled;
 };
 
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index df0ff7d..8ed96dd 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -276,7 +276,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
-	if (dev->pm_runtime_disabled) {
+	if (dev->pm_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
 		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
@@ -286,7 +286,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
+	if (r && !dev->pm_disabled)
 		pm_runtime_disable(&pdev->dev);
 
 	return r;
@@ -304,7 +304,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_runtime_disabled)
+	if (!dev->pm_disabled)
 		pm_runtime_disable(&pdev->dev);
 
 	i2c_dw_remove_lock_support(dev);
@@ -354,9 +354,7 @@ static int dw_i2c_plat_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	i2c_dw_plat_prepare_clk(i_dev, true);
-
-	if (!i_dev->pm_runtime_disabled)
-		i2c_dw_init(i_dev);
+	i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] i2c: designware: Disable pm for PMIC i2c-bus even if there is no _SEM method

[Hans de Goede]

Cherrytrail devices use the dw i2c-bus with uid 7 to access their PMIC.
Even if the i2c-bus to the PMIC is not shared with the SoC's P-Unit
and i2c-designware-baytrail.c thus does not set the pm_disabled flag,
we still need to disable pm so that ACPI PMIC opregions can access the
PMIC during late-suspend and early-resume.

This fixes errors like these blocking suspend:

  i2c_designware 808622C1:06: timeout waiting for bus ready
  ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion]
  acpi 80860F14:02: Failed to change power state to D3hot
  PM: late suspend of devices failed

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 8ed96dd..08d609e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -94,7 +94,10 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
 static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
 	const struct acpi_device_id *id;
+	struct acpi_device *adev;
+	const char *uid;
 
 	dev->adapter.nr = -1;
 	dev->tx_fifo_depth = 32;
@@ -114,6 +117,18 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	if (id && id->driver_data)
 		dev->flags |= (u32)id->driver_data;
 
+	if (acpi_bus_get_device(handle, &adev))
+		return -ENODEV;
+
+	/*
+	 * Cherrytrail I2C7 gets used for the PMIC which gets accessed
+	 * through ACPI opregions during late suspend / early resume
+	 * disable pm for it.
+	 */
+	uid = adev->pnp.unique_id;
+	if ((dev->flags & MODEL_CHERRYTRAIL) && !strcmp(uid, "7"))
+		dev->pm_disabled = true;
+
 	return 0;
 }
 
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i2c: designware: Never suspend i2c-busses used for accessing the system PMIC

[Takashi Iwai]

On Mon, 27 Feb 2017 10:38:29 +0100,
Hans de Goede wrote:
> 
> index a8e74ca..a4ac473 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -79,7 +79,7 @@
>   * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
>   * @acquire_lock: function to acquire a hardware lock on the bus
>   * @release_lock: function to release a hardware lock on the bus
> - * @pm_runtime_disabled: true if pm runtime is disabled
> + * @pm_disabled: true if power-management should be disabled for this i2c-bus
>   *
>   * HCNT and LCNT parameters can be used if the platform knows more accurate
>   * values than the one computed based only on the input clock frequency.
> @@ -127,7 +127,7 @@ struct dw_i2c_dev {
>  	struct pm_qos_request	pm_qos;
>  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
>  	void			(*release_lock)(struct dw_i2c_dev *dev);
> -	bool			pm_runtime_disabled;
> +	bool			pm_disabled;
>  	bool			dynamic_tar_update_enabled;

I couldn't find this dynamic_tar_update_enabled field in your previous
patchset.  What am I missing?


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i2c: designware: Never suspend i2c-busses used for accessing the system PMIC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

On Mon, Feb 27, 2017 at 11:25:01AM +0100, Takashi Iwai wrote:
> On Mon, 27 Feb 2017 10:38:29 +0100,
> Hans de Goede wrote:
> > 
> > index a8e74ca..a4ac473 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.h
> > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > @@ -79,7 +79,7 @@
> >   * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
> >   * @acquire_lock: function to acquire a hardware lock on the bus
> >   * @release_lock: function to release a hardware lock on the bus
> > - * @pm_runtime_disabled: true if pm runtime is disabled
> > + * @pm_disabled: true if power-management should be disabled for this i2c-bus
> >   *
> >   * HCNT and LCNT parameters can be used if the platform knows more accurate
> >   * values than the one computed based only on the input clock frequency.
> > @@ -127,7 +127,7 @@ struct dw_i2c_dev {
> >  	struct pm_qos_request	pm_qos;
> >  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> >  	void			(*release_lock)(struct dw_i2c_dev *dev);
> > -	bool			pm_runtime_disabled;
> > +	bool			pm_disabled;
> >  	bool			dynamic_tar_update_enabled;
> 
> I couldn't find this dynamic_tar_update_enabled field in your previous
> patchset.  What am I missing?

It got reverted with 12688dc21f71f4 ("Revert "i2c: designware: detect
when dynamic tar update is possible"") around 4.10-rc7 time.

I also wondered that Hans didn't get a merge conflict somewhere.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c: designware: Never suspend i2c-busses used for accessing the system PMIC

[Hans de Goede]

Hi,

On 27-02-17 11:25, Takashi Iwai wrote:
> On Mon, 27 Feb 2017 10:38:29 +0100,
> Hans de Goede wrote:
>>
>> index a8e74ca..a4ac473 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -79,7 +79,7 @@
>>   * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
>>   * @acquire_lock: function to acquire a hardware lock on the bus
>>   * @release_lock: function to release a hardware lock on the bus
>> - * @pm_runtime_disabled: true if pm runtime is disabled
>> + * @pm_disabled: true if power-management should be disabled for this i2c-bus
>>   *
>>   * HCNT and LCNT parameters can be used if the platform knows more accurate
>>   * values than the one computed based only on the input clock frequency.
>> @@ -127,7 +127,7 @@ struct dw_i2c_dev {
>>  	struct pm_qos_request	pm_qos;
>>  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
>>  	void			(*release_lock)(struct dw_i2c_dev *dev);
>> -	bool			pm_runtime_disabled;
>> +	bool			pm_disabled;
>>  	bool			dynamic_tar_update_enabled;
>
> I couldn't find this dynamic_tar_update_enabled field in your previous
> patchset.  What am I missing?

My testing branch for all this stuff is based on intel-drm-next-queued, which
is still based on 4.10-rc$, and it seems that 4.11-rc1 will have this:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=12688dc21f71f4dcc9e2b8b5556b0c6cc8df1491

Removing the dynamic_tar_update_enabled member from that struct.

I will send out a new rebased version when 4.11-rc1 gets merged in
intel-drm-next-queued.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i2c: designware: Never suspend i2c-busses used for accessing the system PMIC

[Takashi Iwai]

On Mon, 27 Feb 2017 11:32:45 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 27-02-17 11:25, Takashi Iwai wrote:
> > On Mon, 27 Feb 2017 10:38:29 +0100,
> > Hans de Goede wrote:
> >>
> >> index a8e74ca..a4ac473 100644
> >> --- a/drivers/i2c/busses/i2c-designware-core.h
> >> +++ b/drivers/i2c/busses/i2c-designware-core.h
> >> @@ -79,7 +79,7 @@
> >>   * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
> >>   * @acquire_lock: function to acquire a hardware lock on the bus
> >>   * @release_lock: function to release a hardware lock on the bus
> >> - * @pm_runtime_disabled: true if pm runtime is disabled
> >> + * @pm_disabled: true if power-management should be disabled for this i2c-bus
> >>   *
> >>   * HCNT and LCNT parameters can be used if the platform knows more accurate
> >>   * values than the one computed based only on the input clock frequency.
> >> @@ -127,7 +127,7 @@ struct dw_i2c_dev {
> >>  	struct pm_qos_request	pm_qos;
> >>  	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> >>  	void			(*release_lock)(struct dw_i2c_dev *dev);
> >> -	bool			pm_runtime_disabled;
> >> +	bool			pm_disabled;
> >>  	bool			dynamic_tar_update_enabled;
> >
> > I couldn't find this dynamic_tar_update_enabled field in your previous
> > patchset.  What am I missing?
> 
> My testing branch for all this stuff is based on intel-drm-next-queued, which
> is still based on 4.10-rc$, and it seems that 4.11-rc1 will have this:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=12688dc21f71f4dcc9e2b8b5556b0c6cc8df1491
> 
> Removing the dynamic_tar_update_enabled member from that struct.
> 
> I will send out a new rebased version when 4.11-rc1 gets merged in
> intel-drm-next-queued.

Alright, thanks!

I'm testing the stuff right now.  Since I didn't have the issue on my
machine, I can't say whether it fixes or not.   But at least it
doesn't give any regression, so far ;)

I've put it to topic/i2c-dw-cherrytrail branch for anyone who wants to
try.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx