diff mbox series

[v2] i2c: i2c-qcom-geni: Serve transfer during early resume stage

Message ID 20241011121757.2267336-1-quic_msavaliy@quicinc.com
State New
Headers show
Series [v2] i2c: i2c-qcom-geni: Serve transfer during early resume stage | expand

Commit Message

Mukesh Kumar Savaliya Oct. 11, 2024, 12:17 p.m. UTC 
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(-)

Comments

Bjorn Andersson Oct. 11, 2024, 5:58 p.m. UTC | #1 
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
>
kernel test robot Oct. 13, 2024, 1:41 p.m. UTC | #2 
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
kernel test robot Oct. 13, 2024, 2:23 p.m. UTC | #3 
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: 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
Mukesh Kumar Savaliya Nov. 19, 2024, 2:29 p.m. UTC | #4 
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
>>
Bjorn Andersson Nov. 19, 2024, 2:59 p.m. UTC | #5 
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
Mukesh Kumar Savaliya Nov. 23, 2024, 12:13 p.m. UTC | #6 
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
>
diff mbox series

Patch

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",