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@xxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxx>
[...]
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
[...]
>>> 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
