[v3,04/11] i2c: riic: Enable runtime PM autosuspend support

Commit Message

claudiu beznea July 11, 2024, 11:52 a.m. UTC

From: Claudiu Beznea <[e-mail]>

Enable runtime PM autosuspend support for the RIIC driver. With this, in
case there are consecutive xfer requests the device wouldn't be runtime
enabled/disabled after each consecutive xfer but after the
the delay configured by user. With this, we can avoid touching hardware
registers involved in runtime PM suspend/resume saving in this way some
cycles. The default chosen autosuspend delay is zero to keep the
previous driver behavior.

Signed-off-by: Claudiu Beznea <[e-mail]>
---

Changes in v3:
- none

Changes in v2:
- none

 drivers/i2c/busses/i2c-riic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Biju Das July 12, 2024, 7:15 a.m. UTC | #1

Hi Claudiu,

> -----Original Message-----
> From: Claudiu <[e-mail]>
> Sent: Thursday, July 11, 2024 12:52 PM
> Subject: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend support
> 
> From: Claudiu Beznea <[e-mail]>
> 
> Enable runtime PM autosuspend support for the RIIC driver. With this, in case there are consecutive
> xfer requests the device wouldn't be runtime enabled/disabled after each consecutive xfer but after
> the the delay configured by user. With this, we can avoid touching hardware registers involved in
> runtime PM suspend/resume saving in this way some cycles. The default chosen autosuspend delay is
> zero to keep the previous driver behavior.

On the other hand, you are saving power. Currently the driver is highly optimized for 
Power usage.

Before transfer turn on the clock
After transfer turn off the clock, this is the optimal power usage correspond to suspend delay.

By adding suspend delay, you are consuming power corresponding to
that delay.

Cheers,
Biju

claudiu beznea July 12, 2024, 7:40 a.m. UTC | #2

Hi, Biju,

On 12.07.2024 10:15, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <[e-mail]>
>> Sent: Thursday, July 11, 2024 12:52 PM
>> Subject: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend support
>>
>> From: Claudiu Beznea <[e-mail]>
>>
>> Enable runtime PM autosuspend support for the RIIC driver. With this, in case there are consecutive
>> xfer requests the device wouldn't be runtime enabled/disabled after each consecutive xfer but after
>> the the delay configured by user. With this, we can avoid touching hardware registers involved in
>> runtime PM suspend/resume saving in this way some cycles. The default chosen autosuspend delay is
>> zero to keep the previous driver behavior.
> 
> On the other hand, you are saving power. Currently the driver is highly optimized for 
> Power usage.
> 
> Before transfer turn on the clock
> After transfer turn off the clock, this is the optimal power usage correspond to suspend delay.
> 
> By adding suspend delay, you are consuming power corresponding to
> that delay.

The default delay is zero, see the following diff in this patch:

@@ -479,6 +481,8 @@ static int riic_i2c_probe(struct platform_device *pdev)

 	i2c_parse_fw_timings(dev, &i2c_t, true);

+	pm_runtime_set_autosuspend_delay(dev, 0);

With this, the previous behavior of the driver is preserved, nothing is
changed.

Having it this way preserves the previous behavior and give the the user
the possibility to adjust the autosuspend delay (e.g., though sysfs) to
avoid enable/disable the power to this IP in scenarios where this might be
a bottleneck.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju

Biju Das July 12, 2024, 7:45 a.m. UTC | #3

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <[e-mail]>
> Sent: Friday, July 12, 2024 8:41 AM
> Subject: Re: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend support
> 
> Hi, Biju,
> 
> On 12.07.2024 10:15, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <[e-mail]>
> >> Sent: Thursday, July 11, 2024 12:52 PM
> >> Subject: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend
> >> support
> >>
> >> From: Claudiu Beznea <[e-mail]>
> >>
> >> Enable runtime PM autosuspend support for the RIIC driver. With this,
> >> in case there are consecutive xfer requests the device wouldn't be
> >> runtime enabled/disabled after each consecutive xfer but after the
> >> the delay configured by user. With this, we can avoid touching
> >> hardware registers involved in runtime PM suspend/resume saving in this way some cycles. The
> default chosen autosuspend delay is zero to keep the previous driver behavior.
> >
> > On the other hand, you are saving power. Currently the driver is
> > highly optimized for Power usage.
> >
> > Before transfer turn on the clock
> > After transfer turn off the clock, this is the optimal power usage correspond to suspend delay.
> >
> > By adding suspend delay, you are consuming power corresponding to that
> > delay.
> 
> The default delay is zero, see the following diff in this patch:
> 
> @@ -479,6 +481,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
> 
>  	i2c_parse_fw_timings(dev, &i2c_t, true);
> 
> +	pm_runtime_set_autosuspend_delay(dev, 0);

I just provided justification, why you addes 0  msec here, compared to xx msec
in the original internal version.

Cheers,
Biju

claudiu beznea July 12, 2024, 7:49 a.m. UTC | #4

On 12.07.2024 10:45, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <[e-mail]>
>> Sent: Friday, July 12, 2024 8:41 AM
>> Subject: Re: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend support
>>
>> Hi, Biju,
>>
>> On 12.07.2024 10:15, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <[e-mail]>
>>>> Sent: Thursday, July 11, 2024 12:52 PM
>>>> Subject: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend
>>>> support
>>>>
>>>> From: Claudiu Beznea <[e-mail]>
>>>>
>>>> Enable runtime PM autosuspend support for the RIIC driver. With this,
>>>> in case there are consecutive xfer requests the device wouldn't be
>>>> runtime enabled/disabled after each consecutive xfer but after the
>>>> the delay configured by user. With this, we can avoid touching
>>>> hardware registers involved in runtime PM suspend/resume saving in this way some cycles. The
>> default chosen autosuspend delay is zero to keep the previous driver behavior.
>>>
>>> On the other hand, you are saving power. Currently the driver is
>>> highly optimized for Power usage.
>>>
>>> Before transfer turn on the clock
>>> After transfer turn off the clock, this is the optimal power usage correspond to suspend delay.
>>>
>>> By adding suspend delay, you are consuming power corresponding to that
>>> delay.
>>
>> The default delay is zero, see the following diff in this patch:
>>
>> @@ -479,6 +481,8 @@ static int riic_i2c_probe(struct platform_device *pdev)
>>
>>  	i2c_parse_fw_timings(dev, &i2c_t, true);
>>
>> +	pm_runtime_set_autosuspend_delay(dev, 0);
> 
> I just provided justification, why you addes 0  msec here, compared to xx msec
> in the original internal version.

Isn't it in the commit description already?

"The default chosen autosuspend delay is zero to keep the
previous driver behavior."

> 
> Cheers,
> Biju

Biju Das July 12, 2024, 7:53 a.m. UTC | #5

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <[e-mail]>
> Sent: Friday, July 12, 2024 8:49 AM
> Subject: Re: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend support
> 
> 
> 
> On 12.07.2024 10:45, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: claudiu beznea <[e-mail]>
> >> Sent: Friday, July 12, 2024 8:41 AM
> >> Subject: Re: [PATCH v3 04/11] i2c: riic: Enable runtime PM
> >> autosuspend support
> >>
> >> Hi, Biju,
> >>
> >> On 12.07.2024 10:15, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu <[e-mail]>
> >>>> Sent: Thursday, July 11, 2024 12:52 PM
> >>>> Subject: [PATCH v3 04/11] i2c: riic: Enable runtime PM autosuspend
> >>>> support
> >>>>
> >>>> From: Claudiu Beznea <[e-mail]>
> >>>>
> >>>> Enable runtime PM autosuspend support for the RIIC driver. With
> >>>> this, in case there are consecutive xfer requests the device
> >>>> wouldn't be runtime enabled/disabled after each consecutive xfer
> >>>> but after the the delay configured by user. With this, we can avoid
> >>>> touching hardware registers involved in runtime PM suspend/resume
> >>>> saving in this way some cycles. The
> >> default chosen autosuspend delay is zero to keep the previous driver behavior.
> >>>
> >>> On the other hand, you are saving power. Currently the driver is
> >>> highly optimized for Power usage.
> >>>
> >>> Before transfer turn on the clock
> >>> After transfer turn off the clock, this is the optimal power usage correspond to suspend delay.
> >>>
> >>> By adding suspend delay, you are consuming power corresponding to
> >>> that delay.
> >>
> >> The default delay is zero, see the following diff in this patch:
> >>
> >> @@ -479,6 +481,8 @@ static int riic_i2c_probe(struct platform_device
> >> *pdev)
> >>
> >>  	i2c_parse_fw_timings(dev, &i2c_t, true);
> >>
> >> +	pm_runtime_set_autosuspend_delay(dev, 0);
> >
> > I just provided justification, why you addes 0  msec here, compared to
> > xx msec in the original internal version.
> 
> Isn't it in the commit description already?
> 
> "The default chosen autosuspend delay is zero to keep the previous driver behavior."

That is correct. Some people may have different opinion like other driver
have non zero delays.  Just to get wider opinion.

Note:
Even the original internal patch has non zero delay and then I suggested you to
put 0. It is trade off between frequent transfer vs power usage.

Cheers,
Biju

Wolfram Sang Aug. 8, 2024, 3:08 p.m. UTC | #6

On Thu, Jul 11, 2024 at 02:52:00PM +0300, Claudiu wrote:
> From: Claudiu Beznea <[e-mail]>
> 
> Enable runtime PM autosuspend support for the RIIC driver. With this, in
> case there are consecutive xfer requests the device wouldn't be runtime
> enabled/disabled after each consecutive xfer but after the
> the delay configured by user. With this, we can avoid touching hardware
> registers involved in runtime PM suspend/resume saving in this way some
> cycles. The default chosen autosuspend delay is zero to keep the
> previous driver behavior.
> 
> Signed-off-by: Claudiu Beznea <[e-mail]>

Basically, OK with me. I'd just like a comment here like:

/* Default 0 to save power. Can be overridden via sysfs for lower latency */
> +	pm_runtime_set_autosuspend_delay(dev, 0);
> +	pm_runtime_use_autosuspend(dev);

Other than that:

Reviewed-by: Wolfram Sang <[e-mail]>

claudiu beznea Aug. 9, 2024, 6:58 a.m. UTC | #7

Hi, Wolfram,

On 08.08.2024 18:08, Wolfram Sang wrote:
> On Thu, Jul 11, 2024 at 02:52:00PM +0300, Claudiu wrote:
>> From: Claudiu Beznea <[e-mail]>
>>
>> Enable runtime PM autosuspend support for the RIIC driver. With this, in
>> case there are consecutive xfer requests the device wouldn't be runtime
>> enabled/disabled after each consecutive xfer but after the
>> the delay configured by user. With this, we can avoid touching hardware
>> registers involved in runtime PM suspend/resume saving in this way some
>> cycles. The default chosen autosuspend delay is zero to keep the
>> previous driver behavior.
>>
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> 
> Basically, OK with me. I'd just like a comment here like:
> 
> /* Default 0 to save power. Can be overridden via sysfs for lower latency */

Ok, I'll update it in the next version.

Thank you,
Claudiu Beznea

>> +	pm_runtime_set_autosuspend_delay(dev, 0);
>> +	pm_runtime_use_autosuspend(dev);
> 
> Other than that:
> 
> Reviewed-by: Wolfram Sang <[e-mail]>
>

Patch

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 068f90ecf27e..46765715d39f 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -171,7 +171,8 @@  static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	}
 
  out:
-	pm_runtime_put(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return riic->err ?: num;
 }
@@ -399,7 +400,8 @@  static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 
 	riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
 
-	pm_runtime_put(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	return 0;
 }
 
@@ -479,6 +481,8 @@  static int riic_i2c_probe(struct platform_device *pdev)
 
 	i2c_parse_fw_timings(dev, &i2c_t, true);
 
+	pm_runtime_set_autosuspend_delay(dev, 0);
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	ret = riic_init_hw(riic, &i2c_t);
@@ -496,6 +500,7 @@  static int riic_i2c_probe(struct platform_device *pdev)
 
 out:
 	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 	return ret;
 }
 
@@ -512,6 +517,7 @@  static void riic_i2c_remove(struct platform_device *pdev)
 	}
 	i2c_del_adapter(&riic->adapter);
 	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 }
 
 static const struct riic_of_data riic_rz_a_info = {