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

[Geert Uytterhoeven]

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"?

    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...

37.7.2 for Ether ("sh-eth") states:

    After returning from the standby state, the ether should be reset
and initialized.

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"....

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds