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

[Lad, Prabhakar]

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.

         if (ndev->phydev)
                 phy_eee_rx_clock_stop(ndev->phydev, false);

Note, on another platform where I can issue a reset to the PHY I
issued the reset after unbind operation and monitored the Rx clock
using CLK_MON and I noticed they reported  Rx clocks were OFF. But on
the current platform I cannot issue a reset to the PHY after unbind
operation.

> > + * The Rx and Tx clocks are supplied as follows for the GBETH IP.
> > + *
> > + *                         Rx / Tx
> > + *   -------+------------- on / off -------
> > + *          |
> > + *          |            Rx-180 / Tx-180
> > + *          +---- not ---- on / off -------
>
> Thanks for the diagram.
>
> > +struct renesas_gbeth {
> > +     struct device *dev;
> > +     void __iomem *regs;
> > +     unsigned int num_clks;
> > +     struct clk *clk_tx_i;
> > +     struct clk_bulk_data *clks;
> > +     struct reset_control *rstc;
> > +};
>
> If you stored a pointer to struct plat_stmmacenet_data, then you
> wouldn't need num_clks, clk_tx_i or clks. If you look at
> dwmac-dwc-qos-eth.c, I recently added a helper (dwc_eth_find_clk())
> which could be made generic.
>
> You can then include the clk_tx_i clock in the bulk clock, and
> use the helper to set plat_dat->clk_tx_i.
>
Thanks for the pointer, I'll switch to that.

> > +     plat_dat->flags |= STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY |
> > +                        STMMAC_FLAG_EN_TX_LPI_CLOCKGATING |
>
> Didn't I send you a patch that provides
> STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP so we can move towards the PHY
> saying whether it permits the TX clock to be disabled?
>
I'll rebase my changes on top of [0]. Do you want me to run any
specific tests for this?

[0] https://patchwork.kernel.org/project/netdevbpf/patch/E1trCPy-005jZf-Ou@xxxxxxxxxxxxxxxxxxxxxx/

Cheers,
Prabhakar