Re: [PATCH net-next v3 07/19] net: ravb: Move reference clock enable/disable on runtime PM APIs

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 1/8/24 11:03 AM, claudiu beznea wrote:

   Oops, looks like I forgot to reply to this one...

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Reference clock could be or not part of the power domain. If it is part of
>>> the power domain, the power domain takes care of propertly setting it. In
>>> case it is not part of the power domain and full runtime PM support is
>>> available in driver the clock will not be propertly disabled/enabled at
>>> runtime. For this, keep the prepare/unprepare operations in the driver's
>>> probe()/remove() functions and move the enable/disable in runtime PM
>>> functions.
>>>
>>> Along with it, the other clock request operations were moved close to
>>> reference clock request and prepare to have all the clock requests
>>> specific code grouped together.
>>>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 844ac3306e93..4673cc2faec0 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	error = pm_runtime_resume_and_get(&pdev->dev);
>>> +	if (error < 0)
>>> +		goto out_rpm_disable;
>>> +
>>>  	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>>  	if (IS_ERR(priv->addr)) {
>>>  		error = PTR_ERR(priv->addr);
>>> -		goto out_release;
>>> +		goto out_rpm_put;
>>>  	}
>>>  
>>>  	/* The Ether-specific entries in the device structure. */
[...]
>>> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>>>  	return ret;
>>>  }
>>>  
>>> -static int ravb_runtime_nop(struct device *dev)
>>> +static int ravb_runtime_suspend(struct device *dev)
>>>  {
>>> -	/* Runtime PM callback shared between ->runtime_suspend()
>>> -	 * and ->runtime_resume(). Simply returns success.
>>> -	 *
>>> -	 * This driver re-initializes all registers after
>>> -	 * pm_runtime_get_sync() anyway so there is no need
>>> -	 * to save and restore registers here.
>>> -	 */
>>
>>    Perhaps even worth a separate patch to completely remove this function
>> which doesn't seem to make sense?
> 
> Why? With that the refclk will not be properly enabled/disabled when it
> will not be part of the power domain. Take

   That's what you are adding in this patch, right? Before this patch
this was ravb_runtime_nop(), always returning 0. Did it make any sense?

> https://elixir.bootlin.com/linux/v6.7/source/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi#L57
> as an example. Here refclk is from an external source (not part of power
> domain).
> 
> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey