Message ID | 20240328123743.1713696-1-quic_msavaliy@quicinc.com |
---|---|
State | New |
Headers | show |
Series | [v1] i2c: i2c-qcom-geni: Serve transfer during early resume stage | expand |
Hi Mukesh, On Thu, Mar 28, 2024 at 06:07:43PM +0530, Mukesh Kumar Savaliya wrote: > 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 force > resume. > > 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. > > Co-developed-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> Is this a fix? O mostly an improvement? ... > 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; > + > + if (!pm_runtime_enabled(dev) && gi2c->suspended) { > + dev_dbg(gi2c->se.dev, "RT_PM disabled, Do force resume, usage_count:%d\n", dev nit: you could leave a space after the ':'. > + 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(gi2c->se.dev, "PM get_sync() failed-%d, force resume\n", ret); dev > + ret = geni_i2c_force_resume(gi2c); > + if (ret) > + return ret; > + } Are we missing some cases here? Do we need a few more else's? Andi > } > > qcom_geni_i2c_conf(gi2c); > @@ -702,8 +730,15 @@ 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); > + if (!pm_runtime_enabled(dev) && !gi2c->suspended) { > + pm_runtime_put_noidle(dev); > + pm_runtime_set_suspended(dev); this looks like repeated code, how about making it a function? > + 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; > @@ -820,7 +855,7 @@ 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, 0, > + ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_EARLY_RESUME | IRQF_NO_SUSPEND, Can you provide a bit more information on how an early interrupt would be handled once the device is resumed? Thanks, Andi > dev_name(dev), gi2c); > if (ret) { > dev_err(dev, "Request_irq failed:%d: err:%d\n", > -- > 2.25.1 >
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index da94df466e83..ed8201983a03 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 @@ -677,22 +679,48 @@ 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); + if (ret) { + dev_err(gi2c->se.dev, "Error turning 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 ret; +} + 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; + + if (!pm_runtime_enabled(dev) && gi2c->suspended) { + dev_dbg(gi2c->se.dev, "RT_PM disabled, Do force resume, 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(gi2c->se.dev, "PM get_sync() failed-%d, force resume\n", ret); + ret = geni_i2c_force_resume(gi2c); + if (ret) + return ret; + } } qcom_geni_i2c_conf(gi2c); @@ -702,8 +730,15 @@ 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); + if (!pm_runtime_enabled(dev) && !gi2c->suspended) { + pm_runtime_put_noidle(dev); + pm_runtime_set_suspended(dev); + 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; @@ -820,7 +855,7 @@ 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, 0, + ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_EARLY_RESUME | IRQF_NO_SUSPEND, dev_name(dev), gi2c); if (ret) { dev_err(dev, "Request_irq failed:%d: err:%d\n",