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
>
