i2c: designware: Do nothing in system suspend/resume when RT suspended

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +6 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	i2c_dw_disable(i_dev);
 	i2c_dw_plat_prepare_clk(i_dev, false);
 

@@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	i2c_dw_plat_prepare_clk(i_dev, true);
 	i2c_dw_init(i_dev);
 

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 32 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

Quoted 4 lines

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

Because of the last paragraph, I'd like a positive comment or tag from one of the PM people before I apply this one.

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +6 -1

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+       if (pm_runtime_suspended(dev))
+               return 0;
+
        i2c_dw_disable(i_dev);
        i2c_dw_plat_prepare_clk(i_dev, false);
 

@@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+       if (pm_runtime_suspended(dev))
+               return 0;
+
        i2c_dw_plat_prepare_clk(i_dev, true);
        i2c_dw_init(i_dev);
 
--

2.11.0

Quoted 32 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

Quoted 4 lines

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

In that case the core expects that the ->prepare callback for the slave will also return 1 (or a positive number in general).

If that doesn't happen, then from the core's perspective it is not safe to allow the master's system PM callbacks to be skipped and that's why direct_complete is unset for it.

Quoted 27 lines

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +6 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+       if (pm_runtime_suspended(dev))
+               return 0;
+
        i2c_dw_disable(i_dev);
        i2c_dw_plat_prepare_clk(i_dev, false);
 

@@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+       if (pm_runtime_suspended(dev))
+               return 0;
+
        i2c_dw_plat_prepare_clk(i_dev, true);
        i2c_dw_init(i_dev);

Hi

Quoted 48 lines · Rafael J. Wysocki

Quoted 34 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets
the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

Quoted 6 lines

I'm not sure is this the right thing to do. It feels something the PM core
should do but I'm not sure that either. One alternative could be to resume
runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

In that case the core expects that the ->prepare callback for the slave will also return 1 (or a positive number in general).

Quoted 4 lines

If that doesn't happen, then from the core's perspective it is not safe to allow the master's system PM callbacks to be skipped and that's why direct_complete is unset for it.

So it's then right thing to check runtime PM status in driver as patch does below?

Quoted 52 lines · Jarkko Nikula

Hi

Quoted 48 lines · Rafael J. Wysocki

Quoted 34 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets
the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

Quoted 6 lines

I'm not sure is this the right thing to do. It feels something the PM core
should do but I'm not sure that either. One alternative could be to resume
runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

In that case the core expects that the ->prepare callback for the slave will also return 1 (or a positive number in general).

Quoted 6 lines

Quoted 4 lines

If that doesn't happen, then from the core's perspective it is not safe to allow the master's system PM callbacks to be skipped and that's why direct_complete is unset for it.

So it's then right thing to check runtime PM status in driver as patch does below?

If you know for a fact that none of the device's children and none of the children thereof and so on and nothing that may depend on the device via a device_link, either directly or indirectly, will ever need to be resumed during system suspend, then yes, it is.

Otherwise, no, it isn't.

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index a597ba32de7e..42a9cd09aa64 100644

drivers/i2c/busses/i2c-designware-platdrv.c +0 -1

        i2c_dw_plat_prepare_clk(i_dev, true);
        i2c_dw_init(i_dev);
 
--

2.11.0

Quoted 48 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core should do but I'm not sure that either. One alternative could be to resume runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 381

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+       if (pm_runtime_suspended(dev))
+               return 0;

This looks weird. I don't find any other drivers that needs this, what is so different with this one?

I have been following the development for the i2c-designware driver for a while. Some time back I also tried to fix the similar problems as you are currently [1] are. Back then, I picked the same approach as John Stultz did recently [2].

To summarize my view, I don't understand the justification of using the direct_complete feature for i2c-designware. To me, it just add complexity to the driver that we really should try to avoid. I think we need something else here.

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM domain currently prevents us from doing this. That said, perhaps we should instead try to make the ACPI PM domain to better collaborate with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have not yet been able to post something. If you think it could make sense, I can pick it up.

Hi

Quoted 56 lines · Ulf Hansson

Quoted 53 lines · Jarkko Nikula

There is possibility to enter dw_i2c_plat_suspend() callback twice during system suspend under certain cases which is causing here warnings from clk_core_disable() and clk_core_unprepare() as well as accessing the registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") implemented a prepare callback that checks for runtime suspended device which allow PM core to set direct_complete flag and skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system syspend (e.g. acpi_subsys_suspend()) and there is a slave device which unsets the direct_complete flag of the parent in __device_suspend() thus causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks and return directly if device is runtime suspended. This affects only system suspend/resume since during runtime suspend/resume runtime status is suspending (not suspended) or resuming.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm able to trigger system suspend callback while device is runtime suspended by removing the pm_runtime_resume() call from drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C slave (ACPI enumerated but doesn't bind due an error in probe function). In that case __device_suspend() for that unbound device has NULL suspend callback, and thus doesn't cause any runtime resume chain but still unsets
the parent's direct_complete flag.
John Stult <[email protected]> has reported he can trigger this on HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core
should do but I'm not sure that either. One alternative could be to resume
runtime suspended parent in in __device_suspend() right after where parent's direct_complete flag is unset.

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 381

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+       if (pm_runtime_suspended(dev))
+               return 0;

This looks weird. I don't find any other drivers that needs this, what is so different with this one?

There are some drivers that are checking pm_runtime_suspended() or pm_runtime_status_suspended() in their suspend/resume callbacks and doing things only when they return false so the problem is not so unique here.

I didn't try to find are there drivers that are using some own state variable for the same purpose but I wouldn't surprise if there are.

Quoted 10 lines

I have been following the development for the i2c-designware driver for a while. Some time back I also tried to fix the similar problems as you are currently [1] are. Back then, I picked the same approach as John Stultz did recently [2].

To summarize my view, I don't understand the justification of using the direct_complete feature for i2c-designware. To me, it just add complexity to the driver that we really should try to avoid. I think we need something else here.

Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") but I agree it's worth to consider.

Quoted 10 lines

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM domain currently prevents us from doing this. That said, perhaps we should instead try to make the ACPI PM domain to better collaborate with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have not yet been able to post something. If you think it could make sense, I can pick it up.

That's a good idea. I didn't think about it at all.

Quoted 18 lines · Jarkko Nikula

Quoted 15 lines · Ulf Hansson

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM
domain currently prevents us from doing this. That said, perhaps we
should instead try to make the ACPI PM domain to better collaborate
with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have
not yet been able to post something. If you think it could make sense,
I can pick it up.

Quoted 1 line

That's a good idea. I didn't think about it at all.

Okay, thanks! I will continue to look into this and try to submit something within a reasonable time frame, keep you posted.

Quoted 20 lines · Ulf Hansson

Quoted 18 lines · Jarkko Nikula

Quoted 15 lines · Ulf Hansson

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM
domain currently prevents us from doing this. That said, perhaps we
should instead try to make the ACPI PM domain to better collaborate
with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have
not yet been able to post something. If you think it could make sense,
I can pick it up.

Quoted 4 lines

Quoted 1 line

That's a good idea. I didn't think about it at all.

Okay, thanks! I will continue to look into this and try to submit something within a reasonable time frame, keep you posted.

But please keep in mind that ACPI PM domain for BayTrail / CherryTrail is used by several IPs including most nasty GPDMA case. If something breaks that it would be show stopper.

Quoted 46 lines

Quoted 9 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 381

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+       if (pm_runtime_suspended(dev))
+               return 0;

This looks weird. I don't find any other drivers that needs this, what is so different with this one?

There are some drivers that are checking pm_runtime_suspended() or pm_runtime_status_suspended() in their suspend/resume callbacks and doing things only when they return false so the problem is not so unique here.

I didn't try to find are there drivers that are using some own state variable for the same purpose but I wouldn't surprise if there are.

Quoted 10 lines

I have been following the development for the i2c-designware driver for a while. Some time back I also tried to fix the similar problems as you are currently [1] are. Back then, I picked the same approach as John Stultz did recently [2].

To summarize my view, I don't understand the justification of using the direct_complete feature for i2c-designware. To me, it just add complexity to the driver that we really should try to avoid. I think we need something else here.

Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") but I agree it's worth to consider.

Quoted 10 lines

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM domain currently prevents us from doing this. That said, perhaps we should instead try to make the ACPI PM domain to better collaborate with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have not yet been able to post something. If you think it could make sense, I can pick it up.

Quoted 1 line

That's a good idea. I didn't think about it at all.

Okay, thanks! I will continue to look into this and try to submit something within a reasonable time frame, keep you posted.

Kind regards
Uffe

[...]

Quoted 51 lines

Quoted 46 lines

Quoted 9 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index a597ba32de7e..42a9cd09aa64 100644
@@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);
        struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 381

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +2 -0

+       if (pm_runtime_suspended(dev))
+               return 0;

This looks weird. I don't find any other drivers that needs this, what is so different with this one?

There are some drivers that are checking pm_runtime_suspended() or pm_runtime_status_suspended() in their suspend/resume callbacks and doing things only when they return false so the problem is not so unique here.

I didn't try to find are there drivers that are using some own state variable for the same purpose but I wouldn't surprise if there are.

Quoted 10 lines

I have been following the development for the i2c-designware driver for a while. Some time back I also tried to fix the similar problems as you are currently [1] are. Back then, I picked the same approach as John Stultz did recently [2].

To summarize my view, I don't understand the justification of using the direct_complete feature for i2c-designware. To me, it just add complexity to the driver that we really should try to avoid. I think we need something else here.

Dates back to commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend") but I agree it's worth to consider.

Quoted 10 lines

To me, the proper solution is to use the pm_runtime_force_suspend|resume() helpers to deal with system suspend/resume. However I understand that the behavior of the ACPI PM domain currently prevents us from doing this. That said, perhaps we should instead try to make the ACPI PM domain to better collaborate with drivers using pm_runtime_force_suspend|resume()? I have been investigating that and started to cook some patches, although I have not yet been able to post something. If you think it could make sense, I can pick it up.

Quoted 1 line

That's a good idea. I didn't think about it at all.

Okay, thanks! I will continue to look into this and try to submit something within a reasonable time frame, keep you posted.

Quoted 2 lines

Kind regards
Uffe

Jarkko, Andy,

I just wanted to mention that I haven't forgot about this, I am doing the final changes for the ACPI PM domain at this very moment, however I need a couple of more days more before I can post something.

Quoted 8 lines · Ulf Hansson

Jarkko, Andy,

I just wanted to mention that I haven't forgot about this, I am doing
the final changes for the ACPI PM domain at this very moment, however
I need a couple of more days more before I can post something.

I have now published a branch at:

git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
acpi_pm_domain_wip

It contains both changes for the i2c designware platform driver and for the ACPI PM domain. If you by coincidence happen to have slack time (probably not), then please try it out.

The branch is based upon today's version of Rafael's pm tree linux- next branch.

If everything goes well, I intend to post the patches after some more tests.

Quoted 23 lines · Ulf Hansson

Quoted 8 lines · Ulf Hansson

Jarkko, Andy,

I just wanted to mention that I haven't forgot about this, I am doing
the final changes for the ACPI PM domain at this very moment, however
I need a couple of more days more before I can post something.

I have now published a branch at:

git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
acpi_pm_domain_wip

It contains both changes for the i2c designware platform driver and for the ACPI PM domain. If you by coincidence happen to have slack time (probably not), then please try it out.

The branch is based upon today's version of Rafael's pm tree linux- next branch.

Quoted 3 lines

If everything goes well, I intend to post the patches after some more tests.

I have gone through the series briefly.
My concern is a quite nasty bug we have workaround for in acpi_lpss.c, i.e. auto power gating of DesignWare DMA on Intel Braswell (CherryTrail) platforms when it's enumerated via ACPI.

Below is the sequence to test if it works and survives removal and system sleep (lpss-power.sh is the script which shows a power state of selected devices along with PMC Atom status registers and runtime PM):

0 lpss-power.sh
1 mount -t debugfs none /sys/kernel/debug
2 lpss-power.sh
3 modprobe i2c-designware-platform
4 lpss-power.sh
5 modprobe sdhci-acpi
6 lpss-power.sh
7 lsmod
8 modprobe dw-dmac
9 lpss-power.sh
10 modprobe -r dw-dmac
11 lpss-power.sh
12 modprobe dw-dmac
13 lpss-power.sh
14 rtcwake -m mem -s3
15 lpss-power.sh
16 modprobe -r dw-dmac
17 lpss-power.sh
18 modprobe dw-dmac
19 lpss-power.sh
20 stty -F /dev/ttyS2 921600
21 dmesg > /dev/ttyS2
22 cat /proc/interrupts
23 cat < /dev/ttyS1
24 lpss-power.sh
25 cat < /dev/ttyS1 &
26 lpss-power.sh
27 rtcwake -m mem -s3
28 lpss-power.sh
29 fg
30 lpss-power.sh

Be aware that DMA is not enabled for I2C!

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Ulf Hansson

Quoted 8 lines · Ulf Hansson

Jarkko, Andy,

I just wanted to mention that I haven't forgot about this, I am doing
the final changes for the ACPI PM domain at this very moment, however
I need a couple of more days more before I can post something.

Quoted 6 lines

I have gone through the series briefly.
My concern is a quite nasty bug we have workaround for in acpi_lpss.c,
i.e. auto power gating of DesignWare DMA on Intel Braswell (CherryTrail)
platforms when it's enumerated via ACPI.

First, as long as there is no driver calling the new API acpi_dev_disable_direct_comlete(), the ACPI PM domain should behave exactly the same as before these changes.

However, regarding your concern, can you please be a bit more precise on how you deal with the problems. I would appreciate if you could give me real pointers to the code for the workaround, the above is too hand wavy for me to understand.

So where things really starts to change is in the final i2c patch in the series, which converts the i2c designware platform driver to use the runtime PM centric approach, and to do that, it calls the acpi_dev_disable_direct_complete().

Quoted 39 lines

Below is the sequence to test if it works and survives removal and system sleep (lpss-power.sh is the script which shows a power state of
selected devices along with PMC Atom status registers and runtime PM):

   0 lpss-power.sh
   1 mount -t debugfs none /sys/kernel/debug
   2 lpss-power.sh
   3 modprobe i2c-designware-platform
   4 lpss-power.sh
   5 modprobe sdhci-acpi
   6 lpss-power.sh
   7 lsmod
   8 modprobe dw-dmac
   9 lpss-power.sh
  10 modprobe -r dw-dmac
  11 lpss-power.sh
  12 modprobe dw-dmac
  13 lpss-power.sh
  14 rtcwake -m mem -s3
  15 lpss-power.sh
  16 modprobe -r dw-dmac
  17 lpss-power.sh
  18 modprobe dw-dmac
  19 lpss-power.sh
  20 stty -F /dev/ttyS2 921600
  21 dmesg > /dev/ttyS2
  22 cat /proc/interrupts
  23 cat < /dev/ttyS1
  24 lpss-power.sh
  25 cat < /dev/ttyS1 &
  26 lpss-power.sh
  27 rtcwake -m mem -s3
  28 lpss-power.sh
  29 fg
  30 lpss-power.sh

Quoted 1 line

Be aware that DMA is not enabled for I2C!

So if DMA isn't enabled for I2C, what is there to worry about?