[PATCH] net: stmmac: dwmac-sophgo: Add phy interface filter

[PATCH] net: stmmac: dwmac-sophgo: Add phy interface filter

[Inochi Amaoto]

As the SG2042 has an internal rx delay, the delay should be remove
when init 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>
---
 .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 25 ++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
index 3b7947a7a7ba..b2dee1399eb0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
@@ -7,6 +7,7 @@

 #include <linux/clk.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>

@@ -29,8 +30,23 @@ static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
 	return 0;
 }

+static int sophgo_sg2042_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	switch (plat_dat->phy_interface) {
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII_TXID;
+		return 0;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int sophgo_dwmac_probe(struct platform_device *pdev)
 {
+	int (*plat_set_mode)(struct plat_stmmacenet_data *plat_dat);
 	struct plat_stmmacenet_data *plat_dat;
 	struct stmmac_resources stmmac_res;
 	struct device *dev = &pdev->dev;
@@ -50,11 +66,18 @@ static int sophgo_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

+	plat_set_mode = device_get_match_data(&pdev->dev);
+	if (plat_set_mode) {
+		ret = plat_set_mode(plat_dat);
+		if (ret)
+			return ret;
+	}
+
 	return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
 }

 static const struct of_device_id sophgo_dwmac_match[] = {
-	{ .compatible = "sophgo,sg2042-dwmac" },
+	{ .compatible = "sophgo,sg2042-dwmac", .data = sophgo_sg2042_set_mode },
 	{ .compatible = "sophgo,sg2044-dwmac" },
 	{ /* sentinel */ }
 };
--
2.51.0

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

[Andrew Lunn]

On Fri, Oct 17, 2025 at 09:18:01AM +0800, Inochi Amaoto wrote:
> As the SG2042 has an internal rx delay, the delay should be remove
> when init the mac, otherwise the phy will be misconfigurated.

Are there any in tree DT blobs using invalid phy-modes? In theory,
they should not work, but sometimes there is other magic going on. I
just want to make sure this is not going to cause a regression.

Also, does the DT binding document list the valid phy-modes?

      Andrew

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

[Inochi Amaoto]

On Fri, Oct 17, 2025 at 08:16:17PM +0200, Andrew Lunn wrote:
> On Fri, Oct 17, 2025 at 09:18:01AM +0800, Inochi Amaoto wrote:
> > As the SG2042 has an internal rx delay, the delay should be remove
> > when init the mac, otherwise the phy will be misconfigurated.
> 
> Are there any in tree DT blobs using invalid phy-modes? In theory,
> they should not work, but sometimes there is other magic going on. I
> just want to make sure this is not going to cause a regression.
> 

I see no SG2042 board using invalid phy-modes. Only rgmii-id is used,
which is vaild.

> Also, does the DT binding document list the valid phy-modes?
> 

It does not list. Is it better for me to add a list for it?

Regards,
Inochi

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

[Andrew Lunn]

On Sat, Oct 18, 2025 at 08:42:07AM +0800, Inochi Amaoto wrote:
> On Fri, Oct 17, 2025 at 08:16:17PM +0200, Andrew Lunn wrote:
> > On Fri, Oct 17, 2025 at 09:18:01AM +0800, Inochi Amaoto wrote:
> > > As the SG2042 has an internal rx delay, the delay should be remove
> > > when init the mac, otherwise the phy will be misconfigurated.
> > 
> > Are there any in tree DT blobs using invalid phy-modes? In theory,
> > they should not work, but sometimes there is other magic going on. I
> > just want to make sure this is not going to cause a regression.
> > 
> 
> I see no SG2042 board using invalid phy-modes. Only rgmii-id is used,
> which is vaild.

Great, thanks for checking.

> 
> > Also, does the DT binding document list the valid phy-modes?
> > 
> 
> It does not list. Is it better for me to add a list for it?

It would be nice to list the valid modes, to match the code.

   Andrew

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

[Russell King (Oracle)]

On Sat, Oct 18, 2025 at 10:38:17PM +0200, Andrew Lunn wrote:
> On Sat, Oct 18, 2025 at 08:42:07AM +0800, Inochi Amaoto wrote:
> > On Fri, Oct 17, 2025 at 08:16:17PM +0200, Andrew Lunn wrote:
> > > On Fri, Oct 17, 2025 at 09:18:01AM +0800, Inochi Amaoto wrote:
> > > > As the SG2042 has an internal rx delay, the delay should be remove
> > > > when init the mac, otherwise the phy will be misconfigurated.
> > > 
> > > Are there any in tree DT blobs using invalid phy-modes? In theory,
> > > they should not work, but sometimes there is other magic going on. I
> > > just want to make sure this is not going to cause a regression.
> > > 
> > 
> > I see no SG2042 board using invalid phy-modes. Only rgmii-id is used,
> > which is vaild.
> 
> Great, thanks for checking.

Hang on. Is this right?

The commit says that SG2042 has an internal receive delay. This is
presumably the MAC side.

To work around that, you map rgmii-id to rgmii-txid for the PHY, to
prevent the PHY from enabling its receive-side clock delay.

It seems to me that you're saying that rgmii-rxid and rgmii-id
should not be used with these MACs, and you're fixing up to remove
the receive-side delay.

"rgmii-id" doesn't mean "there is a delay _somewhere_ in the system".
It's supposed to mean that the PHY should add delays on both tx and
rx paths.

Confused.

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

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

[Andrew Lunn]

> "rgmii-id" doesn't mean "there is a delay _somewhere_ in the system".
> It's supposed to mean that the PHY should add delays on both tx and
> rx paths.

When passed to the PHY it means that.

However, DT describes the hardware, the PCB. "rgmii-id" means the PCB
does not provide the delays. So the MAC/PHY combination needs to add
the delays. We normally have the PHY provide the delays, so the
phy-mode is normally passed straight to the PHY. However, if the MAC
is adding a delay, which it is in this case, in one direction and
cannot be turned off, the value passed to the PHY needs to reflect
this, to avoid double delays.

And because the MAC delay cannot be turned off, it means there are PCB
designs which don't work, double delays. So it would be nice not to
list them in the binding.

	Andrew

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

[Russell King (Oracle)]

On Sun, Oct 19, 2025 at 02:04:02AM +0200, Andrew Lunn wrote:
> > "rgmii-id" doesn't mean "there is a delay _somewhere_ in the system".
> > It's supposed to mean that the PHY should add delays on both tx and
> > rx paths.
> 
> When passed to the PHY it means that.
> 
> However, DT describes the hardware, the PCB. "rgmii-id" means the PCB
> does not provide the delays. So the MAC/PHY combination needs to add
> the delays. We normally have the PHY provide the delays, so the
> phy-mode is normally passed straight to the PHY. However, if the MAC
> is adding a delay, which it is in this case, in one direction and
> cannot be turned off, the value passed to the PHY needs to reflect
> this, to avoid double delays.
> 
> And because the MAC delay cannot be turned off, it means there are PCB
> designs which don't work, double delays. So it would be nice not to
> list them in the binding.

Well, I find this confusing. I'd suggest there needs to be common code
to deal with it so we can stop thinking about it, and just push everyone
towards using the common code.

phy_interface_t phy_fix_phy_mode_for_mac_delays(phy_interface_t interface,
						bool mac_txid, bool mac_rxid)
{
	if (!phy_interface_mode_is_rgmii(interface))
		return interface;

	if (mac_txid && mac_rxid) {
		if (interface == PHY_INTERFACE_MODE_RGMII_ID)
			return PHY_INTERFACE_MODE_RGMII;
		return PHY_INTERFACE_MODE_NA;
	}

	if (mac_txid) {
		if (interface == PHY_INTERFACE_MODE_RGMII_ID)
			return PHY_INTERFACE_MODE_RGMII_RXID;
		if (interface == PHY_INTERFACE_MODE_RGMII_TXID)
			return PHY_INTERFACE_MODE_RGMII;
		return PHY_INTERFACE_MODE_NA;
	}

	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;
	}

	return interface;
}

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

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

[Yixun Lan]

Hi Inochi,

On 09:18 Fri 17 Oct     , Inochi Amaoto wrote:
> As the SG2042 has an internal rx delay, the delay should be remove
                                                     s/remove/removed/
> when init the mac, otherwise the phy will be misconfigurated.
s/init/initialize/
> 
> 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>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> index 3b7947a7a7ba..b2dee1399eb0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> @@ -7,6 +7,7 @@
> 
>  #include <linux/clk.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> 
> @@ -29,8 +30,23 @@ static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
>  	return 0;
>  }
> 
> +static int sophgo_sg2042_set_mode(struct plat_stmmacenet_data *plat_dat)
> +{
> +	switch (plat_dat->phy_interface) {
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII_TXID;
> +		return 0;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int sophgo_dwmac_probe(struct platform_device *pdev)
>  {
> +	int (*plat_set_mode)(struct plat_stmmacenet_data *plat_dat);
>  	struct plat_stmmacenet_data *plat_dat;
>  	struct stmmac_resources stmmac_res;
>  	struct device *dev = &pdev->dev;
> @@ -50,11 +66,18 @@ static int sophgo_dwmac_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
> 
> +	plat_set_mode = device_get_match_data(&pdev->dev);
> +	if (plat_set_mode) {
> +		ret = plat_set_mode(plat_dat);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
>  }
> 
>  static const struct of_device_id sophgo_dwmac_match[] = {
> -	{ .compatible = "sophgo,sg2042-dwmac" },
> +	{ .compatible = "sophgo,sg2042-dwmac", .data = sophgo_sg2042_set_mode },
I'd personally prefer to introduce a flag for this, it would be more readable and
maintainable, something like
struct sophgo_dwmac_compitable_data {
	bool has_internal_rx_delay;
}

then.
	if (data->has_internal_rx_delay)
		sophgo_sg2042_set_mode(..)


>  	{ .compatible = "sophgo,sg2044-dwmac" },
>  	{ /* sentinel */ }
>  };
> --
> 2.51.0
> 

-- 
Yixun Lan (dlan)

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

[Inochi Amaoto]

On Sat, Oct 18, 2025 at 08:05:48AM +0800, Yixun Lan wrote:
> Hi Inochi,
> 
> On 09:18 Fri 17 Oct     , Inochi Amaoto wrote:
> > As the SG2042 has an internal rx delay, the delay should be remove
>                                                      s/remove/removed/
> > when init the mac, otherwise the phy will be misconfigurated.
> s/init/initialize/
> > 
> > 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>
> > ---
> >  .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 25 ++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> > index 3b7947a7a7ba..b2dee1399eb0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> > @@ -7,6 +7,7 @@
> > 
> >  #include <linux/clk.h>
> >  #include <linux/module.h>
> > +#include <linux/property.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/platform_device.h>
> > 
> > @@ -29,8 +30,23 @@ static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
> >  	return 0;
> >  }
> > 
> > +static int sophgo_sg2042_set_mode(struct plat_stmmacenet_data *plat_dat)
> > +{
> > +	switch (plat_dat->phy_interface) {
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII_TXID;
> > +		return 0;
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int sophgo_dwmac_probe(struct platform_device *pdev)
> >  {
> > +	int (*plat_set_mode)(struct plat_stmmacenet_data *plat_dat);
> >  	struct plat_stmmacenet_data *plat_dat;
> >  	struct stmmac_resources stmmac_res;
> >  	struct device *dev = &pdev->dev;
> > @@ -50,11 +66,18 @@ static int sophgo_dwmac_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> > 
> > +	plat_set_mode = device_get_match_data(&pdev->dev);
> > +	if (plat_set_mode) {
> > +		ret = plat_set_mode(plat_dat);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
> >  }
> > 

> >  static const struct of_device_id sophgo_dwmac_match[] = {
> > -	{ .compatible = "sophgo,sg2042-dwmac" },
> > +	{ .compatible = "sophgo,sg2042-dwmac", .data = sophgo_sg2042_set_mode },
> I'd personally prefer to introduce a flag for this, it would be more readable and
> maintainable, something like
> struct sophgo_dwmac_compitable_data {
> 	bool has_internal_rx_delay;
> }
> 
> then.
> 	if (data->has_internal_rx_delay)
> 		sophgo_sg2042_set_mode(..)
> 
> 
> >  	{ .compatible = "sophgo,sg2044-dwmac" },
> >  	{ /* sentinel */ }
> >  };

Yeah, I think this is a good idea, thanks.

Regards,
Inochi