Re: [PATCH 13/13] net: ravb: Add runtime PM support

[Sergey Shtylyov]

On 11/27/23 5:05 PM, Geert Uytterhoeven wrote:

[...]
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>>
>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>
>>>>>    That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>> RPM calls already in the probe() and remove() methods...
>>>>> And the sh_eth driver
>>>>> has RPM support since 2009...
>>>>>
>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>
>>>>>    I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>> done though ravb_close()).
>>>>>>
>>>>>> If any request arrives to the driver while the interface is down the
>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

[...]

> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?

   Well, even with the original Magnus' commit that added the RPM support (bcd5149ded6b2edbf3732fa1483600a716b1cba6) it wasn't so -- sh_eth_open()
indeed seemed to re-init everything (but not TSU!) but sh_eth_get_stats()
surely didn't (the RPM calls there have been removed since); other RPM
"wrappers" have been added to the driver methods since -- which also
don't init anything... thus the comment in sh_eth_runtime_nop(() seems
to be wrong from the very start...

[...]

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergey