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

Message ID 20170227093830.3416-2-hdegoede@redhat.com
State Superseded
Headers show

Commit Message

Hans de Goede Feb. 27, 2017, 9:38 a.m. UTC
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(-)

Comments

Takashi Iwai Feb. 27, 2017, 10:25 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Feb. 27, 2017, 10:30 a.m. UTC | #2
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.
Hans de Goede Feb. 27, 2017, 10:32 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Takashi Iwai Feb. 27, 2017, 11:06 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 }