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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Mar 03, 2025 at 09:41:13AM +0000, Lad, Prabhakar wrote:
> Hi Russell,
> 
> On Sun, Mar 2, 2025 at 9:20 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Russell,
> >
> > On Sun, Mar 2, 2025 at 7:33 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Sun, Mar 02, 2025 at 06:18:08PM +0000, Prabhakar wrote:
> > > > +     gbeth->dev = dev;
> > > > +     gbeth->regs = stmmac_res.addr;
> > > > +     plat_dat->bsp_priv = gbeth;
> > > > +     plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> > >
> > > Thanks for using that!
> > >
> > Yep, it shortens the glue driver further.
> >
> > > > +     plat_dat->flags |= STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY |
> > > > +                        STMMAC_FLAG_EN_TX_LPI_CLOCKGATING |
> > >
> > > I would like to know what value tx_clk_stop is in
> > > stmmac_mac_enable_tx_lpi() for your setup. Ideally, stmmac should
> > > use the capability report from the PHY to decide whether the
> > > transmit clock can be gated, but sadly we haven't had any support
> > > in phylib/phylink for that until recently, and I haven't modified
> > > stmmac to allow use of that. However, it would be good to gain
> > > knowledge in this area.
> > >
> > tx_clk_stop =1,
> >
> > root@rzv2h-evk-alpha:~# ifconfig eth0 up
> > [  587.830436] renesas-gbeth 15c30000.ethernet eth0: Register
> > MEM_TYPE_PAGE_POOL RxQ-0
> > [  587.838636] renesas-gbeth 15c30000.ethernet eth0: Register
> > MEM_TYPE_PAGE_POOL RxQ-1
> > [  587.846792] renesas-gbeth 15c30000.ethernet eth0: Register
> > MEM_TYPE_PAGE_POOL RxQ-2
> > [  587.854734] renesas-gbeth 15c30000.ethernet eth0: Register
> > MEM_TYPE_PAGE_POOL RxQ-3
> > [  587.926860] renesas-gbeth 15c30000.ethernet eth0: PHY [stmmac-0:00]
> > driver [Microchip KSZ9131 Gigabit PHY] (irq=POLL)
> > [  587.949380] dwmac4: Master AXI performs fixed burst length
> > [  587.954910] renesas-gbeth 15c30000.ethernet eth0: No Safety
> > Features support found
> > [  587.962556] renesas-gbeth 15c30000.ethernet eth0: IEEE 1588-2008
> > Advanced Timestamp supported
> > [  587.971420] renesas-gbeth 15c30000.ethernet eth0: registered PTP clock
> > [  587.978004] renesas-gbeth 15c30000.ethernet eth0: configuring for
> > phy/rgmii-id link mode
> > root@rzv2h-evk-alpha:~# [  591.070448] renesas-gbeth 15c30000.ethernet
> > eth0: tx_clk_stop=1
> > [  591.076590] renesas-gbeth 15c30000.ethernet eth0: Link is Up -
> > 1Gbps/Full - flow control rx/tx
> >
> > With the below diff:
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index aec230353ac4..68f1954e6eea 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1100,6 +1100,7 @@ static int stmmac_mac_enable_tx_lpi(struct
> > phylink_config *config, u32 timer,
> >         struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> >         int ret;
> >
> > +       netdev_err(priv->dev, "tx_clk_stop=%d\n", tx_clk_stop);
> >         priv->tx_lpi_timer = timer;
> >         priv->eee_active = true;
> >
> > > > +                        STMMAC_FLAG_RX_CLK_RUNS_IN_LPI |
> > >
> I got some feedback from the HW team, based on the feedback this flag
> depends on the PHY device. I wonder if we should create a DT property
> for this. Please share your thoughts.

Not sure exactly which flag you're referring to, because you first
quote the code that you added to dump the _transmit_ clock stop,
and then you named the _receive_ clock flag.

I assume you're referring to STMMAC_FLAG_EN_TX_LPI_CLOCKGATING, which
is currently used by the driver because it didn't know any better to
check the capabilities of the PHY - and phylib didn't expose an
interface to do that.

tx_clk_stop is basically the flag from the PHY indicating whether the
MAC may be permitted to stop its transmit clock. Unfortunately, we
can't just switch over to using that in stmmac because of it's dumb
history as that may cause regressions. As we haven't used this flag
from the PHY before, we have no idea whether it's reliable or not,
and if it isn't reliable, then using it will cause regressions.

I think that the way forward would be to introduce yet another flag
(maybe STMMAC_FLAG_LPI_TX_CLK_PHY_CAP) and:

	if (priv->plat->flags & STMMAC_FLAG_LPI_TX_CLK_PHY_CAP)
		priv->tx_lpi_clk_stop = tx_clk_stop;
	else
		priv->tx_lpi_clk_stop = priv->plat->flags &
					STMMAC_FLAG_EN_TX_LPI_CLOCKGATING;

and then where STMMAC_FLAG_EN_TX_LPI_CLOCKGATING is checked, that
becomes:

	ret = stmmac_set_lpi_mode(priv, priv->hw, STMMAC_LPI_TIMER,
				  priv->tx_lpi_clk_stop,
				  priv->tx_lpi_timer);

It's rather annoying to have to include a flag to say "use the 802.3
standard behaviour" but given that we want to avoid regressions I don't
see any other choice. It would've been nice to have had the driver
using the PHY capability, but that horse has already bolted. We can now
only try to encourage platform glue authors to try setting
STMMAC_FLAG_LPI_TX_CLK_PHY_CAP with the above in place.

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