Re: [PATCH 3/3] net: stmmac: dwmac-socfpga: Add mac-mode DT property support

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 6/30/26 15:31, muhammad.nazim.amirul.nazle.asmade@altera.com wrote:
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> 
> Russell King's commit de696c63c1dc ("net: stmmac: socfpga: convert to
> use phy_interface") replaced mac_interface with phy_interface in
> socfpga_get_plat_phymode(), noting that no upstream DTS files set the
> "mac-mode" property, making the two values identical.
> 
> The Agilex5 SoCDK TSN Config2 board is an exception: its gmac1 TSN
> port uses GMII internally in the MAC while the PHY-side interface is
> RGMII, so mac-mode and phy-mode differ. Without restoring mac_interface
> support, the MAC is configured with RGMII instead of GMII, causing
> connectivity failures on this board.
> 
> Add socfpga_of_get_mac_mode() to read the optional "mac-mode" DT
> property and store it in a new mac_interface field. When the property
> is absent, mac_interface falls back to phy_interface, preserving
> the existing behaviour for all other boards.

After our discussions, indeed mac-mode seems to be the way to go, however
I have some remarks on how it's handled right now.

> 
> Fixes: de696c63c1dc ("net: stmmac: socfpga: convert to use phy_interface")
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 23 ++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 1d7f0a57d288..6a6837c4a414 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -69,12 +69,30 @@ struct socfpga_dwmac {
>  	void __iomem *tse_pcs_base;
>  	void __iomem *sgmii_adapter_base;
>  	bool f2h_ptp_ref_clk;
> +	phy_interface_t mac_interface;
>  	const struct socfpga_dwmac_ops *ops;
>  };
>  
> +static int socfpga_of_get_mac_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "mac-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> +		if (!strcasecmp(pm, phy_modes(i)))
> +			return i;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  static phy_interface_t socfpga_get_plat_phymode(struct socfpga_dwmac *dwmac)
>  {
> -	return dwmac->plat_dat->phy_interface;
> +	return dwmac->mac_interface;
>  }

Taking a look at the logic in socfpga_gen{5|10}_set_phy_mode(), we have :

  phy_interface_t phymode = socfpga_get_plat_phymode(dwmac);
  u32 val;

  socfpga_set_phy_mode_common(phymode, &val)

  if (dwmac->splitter_base)
	  val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;

[...]

  if (phymode == PHY_INTERFACE_MODE_SGMII)
	  socfpga_sgmii_config(dwmac, true);



With this new patch, we now have 2 different ways of handling this
converter block (splitter presence, and mac-mode presence in DT)

Can you unify this a bit ?

One thing could be adding a helper to get the macmode such as
socfpga_get_plat_macmode()

I think we should move the splitter handling before calling 
socfpga_get_plat_macmode() :

 if (dwmac->splitter_base)
   	dwmav->mac_interface = PHY_INTERFACE_MODE_GMII

We'd get the intf_sel value based on the macmode, and also calls to
helpers such as socfpga_sgmii_config(phymode, xxx) need the actual
phymode as a parameter, not the macmode.

Thanks :)

Maxime