Re: [PATCH v7 3/3] net: stmmac: dwmac-sophgo: Add phy interface filter

[Simon Horman <horms@kernel.org>]

On Fri, Nov 07, 2025 at 07:17:15PM +0800, Inochi Amaoto wrote:
> As the SG2042 has an internal rx delay, the delay should be removed
> when initializing the mac, otherwise the phy will be misconfigurated.
> 
> Fixes: 543009e2d4cd ("net: stmmac: dwmac-sophgo: Add support for Sophgo SG2042 SoC")
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> Tested-by: Han Gao <rabenda.cn@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c

...

> @@ -50,11 +56,23 @@ static int sophgo_dwmac_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	data = device_get_match_data(&pdev->dev);
> +	if (data && data->has_internal_rx_delay) {
> +		plat_dat->phy_interface = phy_fix_phy_mode_for_mac_delays(plat_dat->phy_interface,
> +									  false, true);
> +		if (plat_dat->phy_interface == PHY_INTERFACE_MODE_NA)
> +			return -EINVAL;

I'm sorry if this is a false positive. Because, more so than Russell [1], I
confused about how about the treatment of phy_interface. But it seems that
there is a miss match between the use of phy_fix_phy_mode_for_mac_delays()
above and the binding.

The call to phy_fix_phy_mode_for_mac_delays() above will return
PHY_INTERFACE_MODE_NA unless phy_interface is PHY_INTERFACE_MODE_RGMII_ID
or PHY_INTERFACE_MODE_RGMII_RXID.

  phy_interface_t phy_fix_phy_mode_for_mac_delays(phy_interface_t interface,
						bool mac_txid, bool mac_rxid)
  ...
	if (mac_rxid) {
		if (interface == PHY_INTERFACE_MODE_RGMII_ID)
			return PHY_INTERFACE_MODE_RGMII_TXID;
		if (interface == PHY_INTERFACE_MODE_RGMII_RXID)
			return PHY_INTERFACE_MODE_RGMII;
		return PHY_INTERFACE_MODE_NA;
	}
  ...

Looking at phy_modes(), unsurprisingly, the following mappings occur:
* "rgmii" -> PHY_INTERFACE_MODE_RGMII
* "rgmii-id" -> PHY_INTERFACE_MODE_RGMII_ID
* "rgmii-rxid" -> PHY_INTERFACE_MODE_RGMII_RXID
* "rgmii-txid" -> PHY_INTERFACE_MODE_RGMII_TXID

And in the binding, patch 1/3 of this series, only phy-mode rgmii-txid or
rgmii-id is allowed.

But if rgmii-txid is used, PHY_INTERFACE_MODE_RGMII_TXID will be passed to
phy_fix_phy_mode_for_mac_delays(), which will return PHY_INTERFACE_MODE_NA.

Again, I'm confused about the mapping in phy_fix_phy_mode_for_mac_delays().
But there does seem to be some inconsistency between the binding and
the driver implementation here.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/ 

[1] https://lore.kernel.org/all/aPSubO4tJjN_ns-t@shell.armlinux.org.uk/

...