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

Hi Russell,

Thank you for the review.

On Sun, Mar 9, 2025 at 8:50 AM Russell King (Oracle)
[off-list ref] wrote:

On Sat, Mar 08, 2025 at 08:09:21PM +0000, Prabhakar wrote:

quoted

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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@bp.renesas.com>
---
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.

quoted

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

quoted

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

quoted

+     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@rmk-PC.armlinux.org.uk/

Cheers,
Prabhakar