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

[claudiu beznea]

Hi, Geert,

On 27.11.2023 16:05, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
> <claudiu.beznea@xxxxxxxxx> wrote:
>> On 23.11.2023 21:19, Sergey Shtylyov wrote:
>>> On 11/23/23 8:04 PM, claudiu beznea 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>
> 
>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>>>    unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>>>>>>    unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
>>>>>>    unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>>>>>> +  unsigned rpm:1;                 /* Runtime PM available. */
>>>>>
>>>>>    No, I don't think this flag makes any sense. We should support RPM
>>>>> unconditionally...
>>>
>>>    If RPM calls work in the probe()/remove() methods, they should work
>>> in the ndo_{open|stop}() methods, right?
>>
>> It might depend on hardware support... E.g.
>>
>> I debugged it further the issue I had with this implementation on other
>> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
>> is configured.
>>
>> I wiped out the RPM code from this patch and just called:
>>
>> pm_runtime_put_sync();          // [1]
>> usleep_range(300000, 400000);   // [2]
>> pm_runtime_get_sync();          // [3]
>>
>> at the end of ravb_probe(); with this the interfaces fails to work. I
>> continue debugging it and interrogated CSR and this returns RESET after
>> [3]. I tried to switched it back to configuration mode after [3] but fails
>> to restore to a proper working state.
>>
>> Then continued to debug it further to see what happens on the clock driver.
>> The clk enable/disable reaches function at [4] which sets control_regs[reg]
>> which is one of the System module stop control registers. Setting this
>> activates module standby (AFICT). Switch to reset state on Ethernet IP
>> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
>> chapter of the G1H HW manual (which I don't fully understand).
> 
> You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?

Yes!

> 
>     The AVB-DMAC completes the bus master access in progress,
>     and then shifts to reset mode. At this time, the operating mode
>     configuration bits in the AVB-DMAC mode register (CCC.OPC) are
>     set to B'00.
> 
> "reset mode" could be interpreted as "register contents are reset (lost)".
> However, the R-Car Gen3 documentation contains the same paragraph,
> and register contents are known not to be lost...

I remember (from the debugging session I've run few weeks ago) that I
checked on G1H an Ethernet register before point [1] and after point [3]
and the values were the same (but I may be wrong, I need to double check it).

I will double check also the value of MSTOP for Ethernet on RZ/G3S (though
I checked that this worked on my code), maybe RZ/G3S doesn't go to standby,
I have a bug in my code and that's why it works for RZ/G3S...

Also, I see that the STANDBY state is missing from CCC.OPC documentation
(chapter "37A.3.1 AVB-DMAC Operating Modes" on RZ/G1H vs "31.5.1 DMAC
Operating Modes" on RZ/G3S).

> 
> 37.7.2 for Ether ("sh-eth") states:
> 
>     After returning from the standby state, the ether should be reset
> and initialized.

Ok, I found that one in my G1H manual. It is not available on RZ/G3S
manual, though.

> 
> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?
> 
>> Also, the manual of G1H states from some IPs that register state is
>> preserved in standby mode but not for AVB.
> 
> Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
> their register contents when in standby-mode (module standby enabled).
> On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
> contents are lost when the power area is powered down.
> So I'd be surprised if EtherAVB behaves differently.  Of course that
> is still possible, there are big difference between EtherAVB in R-Car
> Gen2 and RZ/G1, and the revision found in later SoC families.
> 
>>>> The reasons I've limited only to RZ/G3S are:
>>>> 1/ I don't have all the platforms to test it
>>>
>>>    That's a usual problem with the kernel development...
>>>
>>>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>>>    platform at hand, only remotely, and is hardly to debug once the
>>>>    ethernet fails to work: probe is working(), open is executed, PHY is
>>>>    initialized and then TX/RX is not working... don't know why ATM.
>>>
>>>    That's why we have the long bug fixing period after -rc1...
>>
>> I prefer to not introduce any bug by intention.
> 
> Iff register contents are lost on RZ/G1H, I'd rather add
> an extra clk_prepare_enable(priv->clk) to ravb_probe() on
> "renesas,etheravb-rcar-gen2"....

This should work, though I would go with a pm_runtime_put_noidle().

Thank you,
Claudiu Beznea

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