Hi, Geert,
On 27.11.2023 16:46, claudiu beznea wrote:
> 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 checked again DBAT before point [1] and after point [3]. Before point [1]
DBAT=0x6c040000, after point [3] DBAT=0x00000000.
However, if all the register settings done before point [1] are re-executed
after point [3] the Ethernet connection seems usable. I tried the above
settings after point [3] to confirm this:
ravb_set_config_mode(ndev);
usleep_range(1000, 2000);
pr_err("%s(): 2: mode=%08x\n", __func__, ravb_read(ndev, CSR));
if (info->gptp || info->ccc_gac) {
/* Set GTI value */
error = ravb_set_gti(ndev);
if (error)
goto out_disable_refclk;
/* Request GTI loading */
ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
}
if (info->internal_delay) {
ravb_parse_delay_mode(np, ndev);
ravb_set_delay_mode(ndev);
}
ravb_write(ndev, priv->desc_bat_dma, DBAT);
/* Initialise PTP Clock driver */
ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
However, I don't have a PTP setup to check.
>
> 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...
All is good in RZ/G3S. MSTOP is set accordingly and no issues.
Thank you,
Claudiu Beznea
>
> 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
>>
