drivers/i2c/busses/i2c-qcom-geni.c | 61 +++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 10 deletions(-)
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 :
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>
---
v1 -> v2:
- Changed gi2c->se.dev to dev during dev_dbg() calls.
- Addressed review comments from Andi and Bjorn.
- 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@quicinc.com/
---
---
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);
+ 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.
+ */
+ 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.1Hi Mukesh,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.12-rc2]
[also build test ERROR on linus/master]
[cannot apply to andi-shyti/i2c/i2c-host next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mukesh-Kumar-Savaliya/i2c-i2c-qcom-geni-Serve-transfer-during-early-resume-stage/20241011-202013
base: v6.12-rc2
patch link: https://lore.kernel.org/r/20241011121757.2267336-1-quic_msavaliy%40quicinc.com
patch subject: [PATCH v2] i2c: i2c-qcom-geni: Serve transfer during early resume stage
config: arc-randconfig-001-20241013 (https://download.01.org/0day-ci/archive/20241013/202410132233.P25W2vKq-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241013/202410132233.P25W2vKq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410132233.P25W2vKq-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14,
from drivers/i2c/busses/i2c-qcom-geni.c:4:
drivers/i2c/busses/i2c-qcom-geni.c: In function 'geni_i2c_xfer':
>> drivers/i2c/busses/i2c-qcom-geni.c:711:48: error: 'struct dev_pm_info' has no member named 'usage_count'
711 | atomic_read(&dev->power.usage_count));
| ^
include/linux/dev_printk.h:129:48: note: in definition of macro 'dev_printk'
129 | _dev_printk(level, dev, fmt, ##__VA_ARGS__); \
| ^~~~~~~~~~~
drivers/i2c/busses/i2c-qcom-geni.c:710:17: note: in expansion of macro 'dev_dbg'
710 | dev_dbg(dev, "Runtime PM is disabled hence force resume, pm_usage_count: %d\n",
| ^~~~~~~
vim +711 drivers/i2c/busses/i2c-qcom-geni.c
696
697 static int geni_i2c_xfer(struct i2c_adapter *adap,
698 struct i2c_msg msgs[],
699 int num)
700 {
701 struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
702 struct device *dev = gi2c->se.dev;
703 int ret;
704
705 gi2c->err = 0;
706 reinit_completion(&gi2c->done);
707
708 /* Serve I2C transfer by forced resume whether Runtime PM is enbled or not */
709 if (!pm_runtime_enabled(dev) && gi2c->suspended) {
710 dev_dbg(dev, "Runtime PM is disabled hence force resume, pm_usage_count: %d\n",
> 711 atomic_read(&dev->power.usage_count));
712 ret = geni_i2c_force_resume(gi2c);
713 if (ret)
714 return ret;
715 } else {
716 ret = pm_runtime_get_sync(dev);
717 if (ret == -EACCES && gi2c->suspended) {
718 dev_dbg(dev, "pm_runtime_get_sync() failed-%d, force resume\n", ret);
719 ret = geni_i2c_force_resume(gi2c);
720 if (ret)
721 return ret;
722 }
723 }
724
725 qcom_geni_i2c_conf(gi2c);
726
727 if (gi2c->gpi_mode)
728 ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
729 else
730 ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
731
732 /* Does Opposite to Forced Resume when runtime PM was not enabled and served
733 * Transfer via forced resume.
734 */
735 if (!pm_runtime_enabled(dev) && !gi2c->suspended) {
736 pm_runtime_put_noidle(dev);
737 pm_runtime_set_suspended(dev);
738 /* Reset flag same as runtime suspend, next xfer PM can be enabled */
739 gi2c->suspended = 0;
740 } else {
741 pm_runtime_mark_last_busy(gi2c->se.dev);
742 pm_runtime_put_autosuspend(gi2c->se.dev);
743 }
744
745 gi2c->cur = NULL;
746 gi2c->err = 0;
747 return ret;
748 }
749
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Mukesh,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.12-rc2]
[also build test ERROR on linus/master]
[cannot apply to andi-shyti/i2c/i2c-host next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mukesh-Kumar-Savaliya/i2c-i2c-qcom-geni-Serve-transfer-during-early-resume-stage/20241011-202013
base: v6.12-rc2
patch link: https://lore.kernel.org/r/20241011121757.2267336-1-quic_msavaliy%40quicinc.com
patch subject: [PATCH v2] i2c: i2c-qcom-geni: Serve transfer during early resume stage
config: i386-buildonly-randconfig-001-20241013 (https://download.01.org/0day-ci/archive/20241013/202410132130.a2tOwnm4-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241013/202410132130.a2tOwnm4-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410132130.a2tOwnm4-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-qcom-geni.c:711:28: error: no member named 'usage_count' in 'struct dev_pm_info'
711 | atomic_read(&dev->power.usage_count));
| ~~~~~~~~~~ ^
include/linux/dev_printk.h:165:39: note: expanded from macro 'dev_dbg'
165 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:274:19: note: expanded from macro 'dynamic_dev_dbg'
274 | dev, fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
250 | _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
248 | __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
224 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
1 error generated.
vim +711 drivers/i2c/busses/i2c-qcom-geni.c
696
697 static int geni_i2c_xfer(struct i2c_adapter *adap,
698 struct i2c_msg msgs[],
699 int num)
700 {
701 struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
702 struct device *dev = gi2c->se.dev;
703 int ret;
704
705 gi2c->err = 0;
706 reinit_completion(&gi2c->done);
707
708 /* Serve I2C transfer by forced resume whether Runtime PM is enbled or not */
709 if (!pm_runtime_enabled(dev) && gi2c->suspended) {
710 dev_dbg(dev, "Runtime PM is disabled hence force resume, pm_usage_count: %d\n",
> 711 atomic_read(&dev->power.usage_count));
712 ret = geni_i2c_force_resume(gi2c);
713 if (ret)
714 return ret;
715 } else {
716 ret = pm_runtime_get_sync(dev);
717 if (ret == -EACCES && gi2c->suspended) {
718 dev_dbg(dev, "pm_runtime_get_sync() failed-%d, force resume\n", ret);
719 ret = geni_i2c_force_resume(gi2c);
720 if (ret)
721 return ret;
722 }
723 }
724
725 qcom_geni_i2c_conf(gi2c);
726
727 if (gi2c->gpi_mode)
728 ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
729 else
730 ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
731
732 /* Does Opposite to Forced Resume when runtime PM was not enabled and served
733 * Transfer via forced resume.
734 */
735 if (!pm_runtime_enabled(dev) && !gi2c->suspended) {
736 pm_runtime_put_noidle(dev);
737 pm_runtime_set_suspended(dev);
738 /* Reset flag same as runtime suspend, next xfer PM can be enabled */
739 gi2c->suspended = 0;
740 } else {
741 pm_runtime_mark_last_busy(gi2c->se.dev);
742 pm_runtime_put_autosuspend(gi2c->se.dev);
743 }
744
745 gi2c->cur = NULL;
746 gi2c->err = 0;
747 return ret;
748 }
749
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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.
> 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.
> 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.
> 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>
> ---
> 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.
> - 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@quicinc.com/
> ---
> ---
> 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?
> + 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.
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
>
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@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>> 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@quicinc.com/
>> ---
>> ---
>> 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
>>
On Tue, Nov 19, 2024 at 8:29 AM Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> wrote: > On 10/11/2024 11:28 PM, Bjorn Andersson wrote: > > On Fri, Oct 11, 2024 at 05:47:57PM +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 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 ? > If this is a valid scenario in the upstream kernel, yes. If it solves a problem only manifesting itself based on a downstream design then you need to exactly describe that scenario so that reviewers can decide if this is a problem with the upstream kernel or your downstream design. Regards, Bjonr
Thanks Bjorn. On 11/19/2024 8:29 PM, Bjorn Andersson wrote: > On Tue, Nov 19, 2024 at 8:29 AM Mukesh Kumar Savaliya > <quic_msavaliy@quicinc.com> wrote: >> On 10/11/2024 11:28 PM, Bjorn Andersson wrote: >>> On Fri, Oct 11, 2024 at 05:47:57PM +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 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 ? >> > > If this is a valid scenario in the upstream kernel, yes. If it solves > a problem only manifesting itself based on a downstream design then > you need to exactly describe that scenario so that reviewers can > decide if this is a problem with the upstream kernel or your > downstream design. > The exact scenario from PCIe switch is as below. Hope it describes the usecase at high level to understand PCIe client -> PCIe Driver -> I2C driver. PCIe switch needs certain configurations through i2c after powering on the switch, as part of suspend we are using suspend_noirq() to turn off the switch because some PCIe clients do some transfers till suspend_noirq() phase. And as part of resume_noirq(), we will enable the power to the switch and configures the switch again through i2c. > Regards, > Bjonr >
© 2016 - 2024 Red Hat, Inc.