Re: [PATCH 17/17] i3c: renesas: Add runtime PM support

[Claudiu Beznea <claudiu.beznea@kernel.org>]

On 5/22/26 23:01, Frank Li wrote:
> On Fri, May 22, 2026 at 01:18:15PM +0300, Claudiu Beznea wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
>> the clocks of the IP are managed through a clock PM domain. To keep the
>> I3C code simpler, the explicit clock handling was dropped along with the
>> addition of runtime PM support, in favor of the runtime PM APIs. Only the
>> code for getting tclk was preserved, as it is necessary to compute the
>> I3C clock rate.
>>
>> All the APIs provided to the I3C subsystem through struct
>> i3c_master_controller_ops are guarded with runtime PM APIs to
>> enable/disable the controller at runtime.
>>
>> As the Renesas I3C driver implements an asynchronous transmit model by
>> preparing a transfer and waiting for its completion through the ISR,
>> renesas_i3c_abort_xfer() was added to disable interrupts and synchronize
>> IRQs before runtime suspending the controller. For this, the interrupts
>> were saved in struct renesas_i3c::irqs. Along with this,
>> renesas_i3c_wait_xfer() return type was changed to unsigned long.
>>
>> Along with the clocks, the controller pin configuration is changed
>> through the provided "sleep" pin configuration.
>>
>> Add runtime PM support for the Renesas I3C driver.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>   drivers/i3c/master/renesas-i3c.c | 183 ++++++++++++++++++++++++++-----
>>   1 file changed, 156 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
>> index a070db4d2440..3b9807a89b54 100644
>> --- a/drivers/i3c/master/renesas-i3c.c
>> +++ b/drivers/i3c/master/renesas-i3c.c
>> @@ -21,7 +21,9 @@
> ...
>>   static int renesas_i3c_probe(struct platform_device *pdev)
>>   {
>>   	struct renesas_i3c *i3c;
>> @@ -1360,12 +1448,21 @@ static int renesas_i3c_probe(struct platform_device *pdev)
>>   	if (IS_ERR(i3c->regs))
>>   		return PTR_ERR(i3c->regs);
>>
>> -	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &i3c->clks);
>> -	if (ret <= RENESAS_I3C_TCLK_IDX)
>> -		return dev_err_probe(&pdev->dev, ret < 0 ? ret : -EINVAL,
>> -				     "Failed to get clocks (need > %d, got %d)\n",
>> -				     RENESAS_I3C_TCLK_IDX, ret);
>> -	i3c->num_clks = ret;
> 
> you can still use devm_clk_bulk_get_all(), if need tclk, you iterate clks
> to find 'tclk', in case in future, need more clocks than tcls.

Indeed, but as they are not needed in the current driver form, I would prefer to 
drop handling both of them as the final code looks simpler. As the clocks are 
enabled/disabled through PM domain, getting rid of 
devm_clk_bulk_get_all_enabled() simplifies the code, FMPOV, it drops 1 pointer 
and 1 int. So, I would like to keep it like this if all OK.

Also, if I'm not wrong, Geert (in cc) prefers having the clock controlled 
directly through power domains if that is possible. I concluded that from:

commit d303ce595cac
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Date:   Wed Mar 20 11:30:03 2019 +0100

     i2c: riic: Add Runtime PM support

     - Replace explicit clock handling by Runtime PM calls,
       - Streamline Runtime PM handling in error paths,
       - Enable Runtime PM in .probe(),
       - Disable Runtime PM in .remove(),
       - Make sure the device is runtime-resumed when disabling interrupts in
         .remove().

     Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
     Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
     Tested-by: Chris Brandt <chris.brandt@renesas.com>
     Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

> 
>> +	i3c->tclk = devm_clk_get(&pdev->dev, "tclk");
>> +	if (IS_ERR(i3c->tclk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(i3c->tclk), "Failed to get tclk");
>> +
>> +	i3c->dev = &pdev->dev;
>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
>> +	pm_runtime_use_autosuspend(&pdev->dev);
>> +	ret = devm_add_action_or_reset(&pdev->dev, renesas_i3c_dont_use_autosuspend,
>> +				       i3c->dev);
> 
> do you cleanup resource in renesas_i3c_dont_use_autosuspend(), look likes
> needn't it.

According to documentation at [1] this is necessary.

[1] 
https://elixir.bootlin.com/linux/v7.1-rc4/source/Documentation/power/runtime_pm.rst#L616

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_runtime_enable(&pdev->dev);
>> +	if (ret)
>> +		return ret;
>>
> ...
>>
>> +static int renesas_i3c_runtime_suspend(struct device *dev)
>> +{
>> +	return pinctrl_pm_select_sleep_state(dev);
> 
> Only change pin state, don't disable clock?

Clocks are handled though the I3C PM domain.

On the Renesas SoCs, where this driver is enabled, clocks are controlled though 
clock PM domains. Every runtime PM suspend/resume will call 
clk_disable()/clk_enable() though the PM domains.

-- 
Thank you,
Claudiu


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c