Re: [PATCH net-next v2 3/3] net: stmmac: Add DWMAC glue layer for Renesas GBETH

[Russell King (Oracle)]

On Sun, Mar 09, 2025 at 11:24:57AM +0000, Lad, Prabhakar wrote:
> Hi Russell,
> 
> Thank you for the review.
> 
> On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle)
> <linux@xxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >
> > > Add the DWMAC glue layer for the GBETH IP found in the Renesas RZ/V2H(P)
> > > SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > ---
> > > v1->v2
> > > - Dropped __initconst for renesas_gbeth_clks array
> > > - Added clks_config callback
> > > - Dropped STMMAC_FLAG_RX_CLK_RUNS_IN_LPI flag as this needs
> > >   investigation.
> >
> > I thought you had got to the bottom of this, and it was a bug in your
> > clock driver?
> >
> I have added a fix in the clock driver to ignore CLK_MON bits for
> external clocks. The main reason for dropping this flag was despite
> trying the below i.e. adding phy_eee_rx_clock_stop() just before
> unregister_netdev() in stmmac_dvr_remove() still doesnt stop the Rx
> clocks.

That's not unexpected, because phy_eee_rx_clock_stop() does not control
a clock gate.

What phy_eee_rx_clock_stop() does is control the clock stop enable bit
in the PHY. Please see IEEE 802.3 section 45.2.3.1.4 and other sections
referred to from that section to gain the appropriate understanding.

The point of adding the phy_eee_rx_clock_stop() call before
stmmac_dvr_remove() was to _test_ (and *only as a test* - a point that
I did stress) to see whether preventing the PHY from stopping it's
receive clock solved the reset timeout on module reload. This test
only makes sense if STMMAC_FLAG_RX_CLK_RUNS_IN_LPI has not been set.

As I understand it, you have found the real issue why that occurs, so
it seems there is little need to continue with that test if, and only
if, everything is now working reliably when removing and re-inserting
the module.

The key point here is "reliably". The receive side of the link *could*
enter or exit LPI at *any* moment - it depends in the link partner. If
the PHY has permission to stop it's receive clock, then this might lead
to stmmac_reset() timing out because the PHY has stopped it's receive
clock _if_ the receive-side LPI persists longer than the reset timeout.

At this point, I am not certain what the current situation is. Are you
now setting STMMAC_FLAG_RX_CLK_RUNS_IN_LPI because it solves a problem?
If the answer is yes, then there is still a bug in the driver that needs
to be solved and I've presented several solutions to that.

I want to remove STMMAC_FLAG_RX_CLK_RUNS_IN_LPI from the stmmac driver
so I'm going to NACK patches that add new uses. Sorry, but we need to
solve the root problem, and stop hacking around it with flags to change
behaviours.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!