Hi Bjorn, thanks for the review.
On 10/11/2024 11:28 PM, Bjorn Andersson wrote:
On Fri, Oct 11, 2024 at 05:47:57PM +0530, Mukesh Kumar Savaliya wrote:
Your recipients list is broken. Please check go/upstream and adopt b4 to
help avoid such mistakes.
Sorry, My mistake in keeping output of maintainers script separately
into --to and --cc which i shouldn't do.
pm_runtime_get_sync() function fails during PM early resume and returning
-EACCES because runtime PM for the device is disabled at the early stage
causing i2c transfer to fail. Make changes to serve transfer with forced
resume.
Few i2c clients like PCI OR touch may request i2c transfers during early
resume stage. In order to serve transfer request do :
This problem description is too generic. I am not aware of any use case
upstream where PCI or touch might need to perform i2c transfers during
early resume; your commit message should educate me.
yes, it's generic as of now since we have an internal usecase with PCI
is yet to be enabled in upstream. Not tied up with any usecase in
upstream, i just heard recently.
Provided the scenario is generic and possible by any client, can this
code change be reviewed or shall be kept on halt till PCI usecase gets
enabled ?
1. Register interrupt with IRQF_EARLY_RESUME and IRQF_NO_SUSPEND flags
to avoid timeout of transfer when IRQ is not enabled during early stage.
2. Do force resume if pm_runtime_get_sync() is failing after system
suspend when runtime PM is not enabled.
3. Increment power usage count after forced resume to balance
it against regular runtime suspend.
Please avoid the bullet list form technical description of your patch.
Sure. Made the changes in commit log with free flow writeup.
Co-developed-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
---
v1 -> v2:
- Changed gi2c->se.dev to dev during dev_dbg() calls.
- Addressed review comments from Andi and Bjorn.
That's nice, but spell out the changes you're doing so that reviewers
now what you did.
Sure, changes done are also listed but i will add reviewers name per change.
- Returned 0 instead garbage inside geni_i2c_force_resume().
- Added comments explaining forced resume transfer when runtime PM
remains disabled.
V1 link: https://patches.linaro.org/project/linux-i2c/patch/20240328123743.1713696-1-quic_msavaliy@xxxxxxxxxxx/
---
---
drivers/i2c/busses/i2c-qcom-geni.c | 61 +++++++++++++++++++++++++-----
1 file changed, 51 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 212336f724a6..e1207f1a3de3 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -134,6 +134,8 @@ struct geni_i2c_clk_fld {
u8 t_cycle_cnt;
};
+static int geni_i2c_runtime_resume(struct device *dev);
+
/*
* Hardware uses the underlying formula to calculate time periods of
* SCL clock cycle. Firmware uses some additional cycles excluded from the
@@ -675,22 +677,49 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
return num;
}
+static int geni_i2c_force_resume(struct geni_i2c_dev *gi2c)
+{
+ struct device *dev = gi2c->se.dev;
+ int ret;
+
+ ret = geni_i2c_runtime_resume(dev);
Wouldn't pm_runtime_force_resume() help you do what you're looking for?
Yes, i have reviewed and seems this API can also serve the same purpose.
Made the changes accordingly.
+ if (ret) {
+ dev_err(gi2c->se.dev, "Failed to enable SE resources: %d\n", ret);
+ pm_runtime_put_noidle(dev);
+ pm_runtime_set_suspended(dev);
+ return ret;
+ }
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ return 0;
+}
+
static int geni_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[],
int num)
{
struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
+ struct device *dev = gi2c->se.dev;
int ret;
gi2c->err = 0;
reinit_completion(&gi2c->done);
- ret = pm_runtime_get_sync(gi2c->se.dev);
- if (ret < 0) {
- dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret);
- pm_runtime_put_noidle(gi2c->se.dev);
- /* Set device in suspended since resume failed */
- pm_runtime_set_suspended(gi2c->se.dev);
- return ret;
+
+ /* Serve I2C transfer by forced resume whether Runtime PM is enbled or not */
+ if (!pm_runtime_enabled(dev) && gi2c->suspended) {
+ dev_dbg(dev, "Runtime PM is disabled hence force resume, pm_usage_count: %d\n",
+ atomic_read(&dev->power.usage_count));
+ ret = geni_i2c_force_resume(gi2c);
+ if (ret)
+ return ret;
+ } else {
+ ret = pm_runtime_get_sync(dev);
+ if (ret == -EACCES && gi2c->suspended) {
+ dev_dbg(dev, "pm_runtime_get_sync() failed-%d, force resume\n", ret);
+ ret = geni_i2c_force_resume(gi2c);
+ if (ret)
+ return ret;
+ }
}
qcom_geni_i2c_conf(gi2c);
@@ -700,8 +729,19 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
else
ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
- pm_runtime_mark_last_busy(gi2c->se.dev);
- pm_runtime_put_autosuspend(gi2c->se.dev);
+ /* Does Opposite to Forced Resume when runtime PM was not enabled and served
+ * Transfer via forced resume.
Please polish this comment.
Sure, Enhanced my comments. Thanks.
Regards,
Bjorn
+ */
+ if (!pm_runtime_enabled(dev) && !gi2c->suspended) {
+ pm_runtime_put_noidle(dev);
+ pm_runtime_set_suspended(dev);
+ /* Reset flag same as runtime suspend, next xfer PM can be enabled */
+ gi2c->suspended = 0;
+ } else {
+ pm_runtime_mark_last_busy(gi2c->se.dev);
+ pm_runtime_put_autosuspend(gi2c->se.dev);
+ }
+
gi2c->cur = NULL;
gi2c->err = 0;
return ret;
@@ -818,7 +858,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
init_completion(&gi2c->done);
spin_lock_init(&gi2c->lock);
platform_set_drvdata(pdev, gi2c);
- ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
+ ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq,
+ IRQF_NO_AUTOEN | IRQF_EARLY_RESUME | IRQF_NO_SUSPEND,
dev_name(dev), gi2c);
if (ret) {
dev_err(dev, "Request_irq failed:%d: err:%d\n",
--
2.25.1
