[PATCH v2 0/2] i2c: designware: Never suspend i2c-busses used for system PMICs

[PATCH v2 0/2] i2c: designware: Never suspend i2c-busses used for system PMICs

[Hans de Goede]

Hi All,

Here is a v2 of this patch-set rebased on top of 4.11-rc1 +
drm-intel/topic/designware-baytrail :
https://cgit.freedesktop.org/drm-intel/log/?h=topic/designware-baytrail

Note these patches depend on changes in drm-intel/topic/designware-baytrail
and will not apply cleanly without that branch being merged first.

Regards,

Hans

[PATCH v2 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     | 11 +++++++++--
 drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
 drivers/i2c/busses/i2c-designware-platdrv.c  | 10 ++++------
 4 files changed, 16 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 15a5348..c453717 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -960,6 +960,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;
 
 	init_completion(&dev->cmd_complete);
@@ -975,9 +976,15 @@ 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 091c249..1e75ba4 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;
 };
 
 #define ACCESS_SWAP		0x00000001
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

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

[Jarkko Nikula]

On 03/08/17 10:47, Hans de Goede wrote:
> 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     | 11 +++++++++--
>  drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
>  drivers/i2c/busses/i2c-designware-platdrv.c  | 10 ++++------
>  4 files changed, 16 insertions(+), 11 deletions(-)
>

Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Wolfram Sang]

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


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

Acked-by ?


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

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

[Jarkko Nikula]

On 03/09/17 16:39, Wolfram Sang wrote:
>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-baytrail.c |  2 +-
>>>  drivers/i2c/busses/i2c-designware-core.c     | 11 +++++++++--
>>>  drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
>>>  drivers/i2c/busses/i2c-designware-platdrv.c  | 10 ++++------
>>>  4 files changed, 16 insertions(+), 11 deletions(-)
>>>
>>
>> Jarkko Nikula <jarkko.nikula@linux.intel.com>
>
> Acked-by ?
>
Sorry, here it goes

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

[PATCH v2 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

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

[Jarkko Nikula]

+ Tianyu

On 03/08/17 10:47, Hans de Goede wrote:
> 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;
> +

I'm fine with this but wondering can this be detected any other way than 
hardcoded bus number.

Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI 
operation region support"). Do you know is there way to see is there a 
PMIC connected to the bus?

Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Jarkko Nikula]

On 03/09/17 17:00, Jarkko Nikula wrote:
> + Tianyu
>
> On 03/08/17 10:47, Hans de Goede wrote:
>> 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;
>> +
>
> I'm fine with this but wondering can this be detected any other way than
> hardcoded bus number.
>
> Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI
> operation region support"). Do you know is there way to see is there a
> PMIC connected to the bus?
>
> Jarkko Nikula <jarkko.nikula@linux.intel.com>

Here too:

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

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

[Lan Tianyu]

On 2017年03月09日 23:00, Jarkko Nikula wrote:
> + Tianyu
> 
> On 03/08/17 10:47, Hans de Goede wrote:
>> 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;
>> +
> 
> I'm fine with this but wondering can this be detected any other way than
> hardcoded bus number.
> 
> Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI
> operation region support"). Do you know is there way to see is there a
> PMIC connected to the bus?
> 
> Jarkko Nikula <jarkko.nikula@linux.intel.com>
Hi Jarkko:
      PMIC device node in ACPI table should have _DEP() method to return
devices PMIC depends on. I think the general way to resolve the issue
during suspend/resume is to make sure that I2C7 is suspended/resumed
later/earlier than PMIC device.

-- 
Best regards
Tianyu Lan

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

[Hans de Goede]

Hi,

On 10-03-17 03:33, Lan Tianyu wrote:
> On 2017年03月09日 23:00, Jarkko Nikula wrote:
>> + Tianyu
>>
>> On 03/08/17 10:47, Hans de Goede wrote:
>>> 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;
>>> +
>>
>> I'm fine with this but wondering can this be detected any other way than
>> hardcoded bus number.
>>
>> Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI
>> operation region support"). Do you know is there way to see is there a
>> PMIC connected to the bus?
>>
>> Jarkko Nikula <jarkko.nikula@linux.intel.com>
 >
> Hi Jarkko:
>       PMIC device node in ACPI table should have _DEP() method to return
> devices PMIC depends on.

Hmm, interesting point, but I'm afraid by the time the PMIC driver loads
(and can make say an i2c_adapter_set_syscore call) it is already too late,
as we need to request the irq in the i2c-adapter driver with certain
flags and the i2c-adapter needs to be initialized before we can
load the pmic driver.

I really think just doing this for bus number 7 is the best solution
on all cherrytrail devices I've seen the PMIC is always at bus number 7.

 > I think the general way to resolve the issue
> during suspend/resume is to make sure that I2C7 is suspended/resumed
> later/earlier than PMIC device.

That would also require making sure that the PMIC suspends after any
devices whose _PS0 / _PS3 methods may need access to PMIC optegions,
we do not have infrastructure in Linux to do this.

Regards,

Hans

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

[Lan, Tianyu]

On 3/10/2017 3:49 PM, Hans de Goede wrote:
> Hi,
>
> On 10-03-17 03:33, Lan Tianyu wrote:
>> On 2017年03月09日 23:00, Jarkko Nikula wrote:
>>> + Tianyu
>>>
>>> On 03/08/17 10:47, Hans de Goede wrote:
>>>> 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;
>>>> +
>>>
>>> I'm fine with this but wondering can this be detected any other way than
>>> hardcoded bus number.
>>>
>>> Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI
>>> operation region support"). Do you know is there way to see is there a
>>> PMIC connected to the bus?
>>>
>>> Jarkko Nikula <jarkko.nikula@linux.intel.com>
>>
>> Hi Jarkko:
>>       PMIC device node in ACPI table should have _DEP() method to return
>> devices PMIC depends on.
>
> Hmm, interesting point, but I'm afraid by the time the PMIC driver loads
> (and can make say an i2c_adapter_set_syscore call) it is already too late,
> as we need to request the irq in the i2c-adapter driver with certain
> flags and the i2c-adapter needs to be initialized before we can
> load the pmic driver.

Yes, dep_unmet field in the struct acpi_device is designed for this 
case. dep_unmet will be zero after all dependent operation region 
handlers are registered. Please have a look acpi_battery_add() and it 
checks dep_unmet. If dep_unmet isn't equal to zero, it returns 
-EPROBE_DEFER. Device core will try reprobing the device after a device 
driver module is loaded. PMIC driver also should check dep_unmet in probe().


             Device (PMIC)
             {
                 Name (_ADR, Zero)  // _ADR: Address
                 Name (_HID, "INT33FD")  // _HID: Hardware ID
                 Name (_CID, "INT33FD")  // _CID: Compatible ID
                 Name (_DDN, "PMIC GPIO Controller")  // _DDN: DOS 
Device Name
                 Name (_HRV, 0x02)  // _HRV: Hardware Revision
                 Name (_UID, One)  // _UID: Unique ID
                 Name (_DEP, Package (0x01)  // _DEP: Dependencies
                 {
                     I2C7
                 })

...

>
> I really think just doing this for bus number 7 is the best solution
> on all cherrytrail devices I've seen the PMIC is always at bus number 7.

This seems a hack way since PMIC also may have different I2C address. 
This highly depends on vendor design.

>
>> I think the general way to resolve the issue
>> during suspend/resume is to make sure that I2C7 is suspended/resumed
>> later/earlier than PMIC device.
>
> That would also require making sure that the PMIC suspends after any
> devices whose _PS0 / _PS3 methods may need access to PMIC optegions,
> we do not have infrastructure in Linux to do this.

Actually, suspend/resume sequence affects by the device probing
sequence. Please have a look at deferred_probe_work_func() and
device_pm_move_last() which changes the device position in the dpm_list
during reprobing device.



>
> Regards,
>
> Hans
>

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

[Hans de Goede]

Hi,

On 03/10/2017 12:34 PM, Lan, Tianyu wrote:
> On 3/10/2017 3:49 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 10-03-17 03:33, Lan Tianyu wrote:
>>> On 2017年03月09日 23:00, Jarkko Nikula wrote:
>>>> + Tianyu
>>>>
>>>> On 03/08/17 10:47, Hans de Goede wrote:
>>>>> 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;
>>>>> +
>>>>
>>>> I'm fine with this but wondering can this be detected any other way than
>>>> hardcoded bus number.
>>>>
>>>> Tianyu: You are the author of 5d98e61d337c ("I2C/ACPI: Add i2c ACPI
>>>> operation region support"). Do you know is there way to see is there a
>>>> PMIC connected to the bus?
>>>>
>>>> Jarkko Nikula <jarkko.nikula@linux.intel.com>
>>>
>>> Hi Jarkko:
>>>       PMIC device node in ACPI table should have _DEP() method to return
>>> devices PMIC depends on.
>>
>> Hmm, interesting point, but I'm afraid by the time the PMIC driver loads
>> (and can make say an i2c_adapter_set_syscore call) it is already too late,
>> as we need to request the irq in the i2c-adapter driver with certain
>> flags and the i2c-adapter needs to be initialized before we can
>> load the pmic driver.
>
> Yes, dep_unmet field in the struct acpi_device is designed for this case. dep_unmet will be zero after all dependent operation region handlers are registered. Please have a look acpi_battery_add() and it checks dep_unmet. If dep_unmet isn't equal to zero, it returns -EPROBE_DEFER. Device core will try reprobing the device after a device driver module is loaded. PMIC driver also should check dep_unmet in probe().

The problem is not unmet dependencies, the problem is the other way
around we cannot probe the pmic until the i2c-controller driver
is loaded and when it it loaded it is to late to figure out
the bus is used for the pmic since the irq has already
been requested and we need to specify the right flags when
requesting the controller irq.

>             Device (PMIC)
>             {
>                 Name (_ADR, Zero)  // _ADR: Address
>                 Name (_HID, "INT33FD")  // _HID: Hardware ID
>                 Name (_CID, "INT33FD")  // _CID: Compatible ID
>                 Name (_DDN, "PMIC GPIO Controller")  // _DDN: DOS Device Name
>                 Name (_HRV, 0x02)  // _HRV: Hardware Revision
>                 Name (_UID, One)  // _UID: Unique ID
>                 Name (_DEP, Package (0x01)  // _DEP: Dependencies
>                 {
>                     I2C7
>                 })
>
> ...
>
>>
>> I really think just doing this for bus number 7 is the best solution
>> on all cherrytrail devices I've seen the PMIC is always at bus number 7.
>
> This seems a hack way since PMIC also may have different I2C address. This highly depends on vendor design.

The i2c address does not matter, it is about which i2c-controller is used
for the entire i2c bus to which the pmic is attached and that always
is bus number 7 (since all hardware follows the same reference design).

Regards,

Hans



>
>>
>>> I think the general way to resolve the issue
>>> during suspend/resume is to make sure that I2C7 is suspended/resumed
>>> later/earlier than PMIC device.
>>
>> That would also require making sure that the PMIC suspends after any
>> devices whose _PS0 / _PS3 methods may need access to PMIC optegions,
>> we do not have infrastructure in Linux to do this.
>
> Actually, suspend/resume sequence affects by the device probing
> sequence. Please have a look at deferred_probe_work_func() and
> device_pm_move_last() which changes the device position in the dpm_list
> during reprobing device.
>
>
>
>>
>> Regards,
>>
>> Hans
>>