b53_adjust_531x5_rgmii() incorrectly enable delays in rgmii mode, but
disables them in rgmii-id mode. Only rgmii-txid is correctly handled.

Fix this by correctly enabling rx delay in rgmii-rxid and rgmii-id
modes, and tx delay in rgmii-txid and rgmii-id modes.

Since b53_adjust_531x5_rgmii() is only called for fixed-link ports,
these are usually used as the CPU port, connected to a MAC. This means
the chip is assuming the role of the PHY and enabling delays is
expected.

Since this has the potential to break existing setups, treat rgmii
as rgmii-id to keep the old broken behavior.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Reported-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
Changes v1 -> v2:
* dropped RFC prefix (since it did the opposite, and I got zero comments)
* dropped the KConfig option and just always treat RGMII as RGMII-ID
* adapted the commit message accordingly

 drivers/net/dsa/b53/b53_common.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index eb767edc4c13..ac476cc6d6db 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1447,6 +1447,13 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
 	else
 		off = B53_RGMII_CTRL_P(port);
 
+	/* Older driver versions incorrectly applied delays in
+	 * PHY_INTERFACE_MODE_RGMII mode. In order to not break old users, keep
+	 * interpreting RGMII as RGMII-ID.
+	 */
+	if (interface == PHY_INTERFACE_MODE_RGMII)
+		interface = PHY_INTERFACE_MODE_RGMII_ID;
+
 	/* Configure the port RGMII clock delay by DLL disabled and
 	 * tx_clk aligned timing (restoring to reset defaults)
 	 */
@@ -1458,19 +1465,24 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
 	 * account for this internal delay that is inserted, otherwise
 	 * the switch won't be able to receive correctly.
 	 *
+	 * PHY_INTERFACE_MODE_RGMII_RXID means RX internal delay, make
+	 * sure that we enable the port RX clock internal sampling delay
+	 * to account for this internal delay that is inserted, otherwise
+	 * the switch won't be able to send correctly.
+	 *
+	 * PHY_INTERFACE_MODE_RGMII_ID means both RX and TX internal delay,
+	 * make sure that we enable delays for both.
+	 *
 	 * PHY_INTERFACE_MODE_RGMII means that we are not introducing
 	 * any delay neither on transmission nor reception, so the
-	 * BCM53125 must also be configured accordingly to account for
-	 * the lack of delay and introduce
-	 *
-	 * The BCM53125 switch has its RX clock and TX clock control
-	 * swapped, hence the reason why we modify the TX clock path in
-	 * the "RGMII" case
+	 * BCM53125 must also be configured accordingly.
 	 */
-	if (interface == PHY_INTERFACE_MODE_RGMII_TXID)
+	if (interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_ID)
 		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
-	if (interface == PHY_INTERFACE_MODE_RGMII)
-		rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
+	if (interface == PHY_INTERFACE_MODE_RGMII_RXID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_ID)
+		rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
 
 	if (dev->chip_id != BCM53115_DEVICE_ID)
 		rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;

base-commit: c2c2ccfd4ba72718266a56f3ecc34c989cb5b7a0
-- 
2.43.0

On 11/7/2025 12:30 AM, Jonas Gorski wrote:
> b53_adjust_531x5_rgmii() incorrectly enable delays in rgmii mode, but
> disables them in rgmii-id mode. Only rgmii-txid is correctly handled.
> 
> Fix this by correctly enabling rx delay in rgmii-rxid and rgmii-id
> modes, and tx delay in rgmii-txid and rgmii-id modes.
> 
> Since b53_adjust_531x5_rgmii() is only called for fixed-link ports,
> these are usually used as the CPU port, connected to a MAC. This means
> the chip is assuming the role of the PHY and enabling delays is
> expected.
> 
> Since this has the potential to break existing setups, treat rgmii
> as rgmii-id to keep the old broken behavior.
> 
> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> Reported-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

> +	/* Older driver versions incorrectly applied delays in
> +	 * PHY_INTERFACE_MODE_RGMII mode. In order to not break old users, keep
> +	 * interpreting RGMII as RGMII-ID.
> +	 */
> +	if (interface == PHY_INTERFACE_MODE_RGMII)
> +		interface = PHY_INTERFACE_MODE_RGMII_ID;

Did you look through the in kernel .dts files? How many systems does
this effect?

I would maybe add a dev_warn() here, saying the DT blob is out of date
and needs fixing. And fix all the in kernel .dts files.

	Andrew

Hi,

On Fri, Nov 7, 2025 at 2:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +     /* Older driver versions incorrectly applied delays in
> > +      * PHY_INTERFACE_MODE_RGMII mode. In order to not break old users, keep
> > +      * interpreting RGMII as RGMII-ID.
> > +      */
> > +     if (interface == PHY_INTERFACE_MODE_RGMII)
> > +             interface = PHY_INTERFACE_MODE_RGMII_ID;
>
> Did you look through the in kernel .dts files? How many systems does
> this effect?

I did, and the answer is 0, at least if looking at the cpu port node.
If also looking at the ethernet nodes where it is connected to it
becomes a bit unclear to me.

There is broadcom/bcm53573.dtsi defines a switch node in disabled
without any phy-mode, and all includers either delete the switch node,
or do not define any phy-mode (or phy-connection-type) either. The
ethernet node itself uses "internal".

There is allwinner/sun7i-a20-lamobo-r1.dts, which uses "rgmii-txid",
which is untouched by this patch. The ethernet interface uses "rgmii".

And there is arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dts,
where a comment says that it has a BCM53134, but there is no such
node. The ethernet node uses "rgmii".

So one doesn't define one, one uses rgmii-id on the switch / phy side
and rgmii on the ethernet mac side, and one only defines the ethernet
mac side as rgmii.

Not sure what the correct move here is for the latter two, especially
since I have no hardware to test.

> I would maybe add a dev_warn() here, saying the DT blob is out of date
> and needs fixing. And fix all the in kernel .dts files.

Sure I can add a warning.

Best regards,
Jonas

> There is allwinner/sun7i-a20-lamobo-r1.dts, which uses "rgmii-txid",
> which is untouched by this patch. The ethernet interface uses "rgmii".

Which is odd, but lets leave it alone.

> And there is arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dts,
> where a comment says that it has a BCM53134, but there is no such
> node. The ethernet node uses "rgmii".

aspeed pretty much always get phy-mode wrong. So i would not worry too
much about this.

> So one doesn't define one, one uses rgmii-id on the switch / phy side
> and rgmii on the ethernet mac side, and one only defines the ethernet
> mac side as rgmii.

That is reasonable. It is a lot less clear what is correct for a
MAC-MAC connection. For a MAC-PHY connection we do have documentation,
the preference is that the PHY adds the delays, not the MAC. If the
switch is playing PHY, then having it add delays is sensible.

> > I would maybe add a dev_warn() here, saying the DT blob is out of date
> > and needs fixing. And fix all the in kernel .dts files.
> 
> Sure I can add a warning.

Great, thanks.

	Andrew

On Fri, Nov 07, 2025 at 03:07:48PM +0100, Andrew Lunn wrote:
> > There is allwinner/sun7i-a20-lamobo-r1.dts, which uses "rgmii-txid",
> > which is untouched by this patch. The ethernet interface uses "rgmii".
> 
> Which is odd, but lets leave it alone.
> 
> > And there is arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dts,
> > where a comment says that it has a BCM53134, but there is no such
> > node. The ethernet node uses "rgmii".
> 
> aspeed pretty much always get phy-mode wrong. So i would not worry too
> much about this.
> 
> > So one doesn't define one, one uses rgmii-id on the switch / phy side
> > and rgmii on the ethernet mac side, and one only defines the ethernet
> > mac side as rgmii.
> 
> That is reasonable. It is a lot less clear what is correct for a
> MAC-MAC connection. For a MAC-PHY connection we do have documentation,
> the preference is that the PHY adds the delays, not the MAC. If the
> switch is playing PHY, then having it add delays is sensible.
> 
> > > I would maybe add a dev_warn() here, saying the DT blob is out of date
> > > and needs fixing. And fix all the in kernel .dts files.
> > 
> > Sure I can add a warning.
> 
> Great, thanks.
> 
> 	Andrew

+Russell

Numerous past discussions seem to suggest that a MAC should treat all
phy-mode RGMII values all the same, as they all mean what the _other_
end of the RGMII link has skewed. To apply RGMII delays on a MAC, the
'rx-internal-delay-ps' and 'tx-internal-delay-ps' properties can be
added to the MAC OF node, which absolutely explicitly refer to what the
MAC does and nothing else.

Various compatibility schemes are implemented by sja1105, qca8k,
Microchip KSZ, rtl8365mb, vsc73xx and most recently Lantiq GSWIP, all in
order to tolerate their old (many times unique) interpretations of
phy-mode, while respecting the delays from the explicit DT bindings if
those exist.

Since there is no 'correct' way to apply RGMII delays on a MAC according
to phy-mode, my advice, if possible, would be to leave sleeping dogs lie
and fix broken setups by adding the explicit device tree properties in
the MAC, and adding driver support for parsing these.

On Fri, Nov 07, 2025 at 04:45:15PM +0200, Vladimir Oltean wrote:
> On Fri, Nov 07, 2025 at 03:07:48PM +0100, Andrew Lunn wrote:
> > > There is allwinner/sun7i-a20-lamobo-r1.dts, which uses "rgmii-txid",
> > > which is untouched by this patch. The ethernet interface uses "rgmii".
> > 
> > Which is odd, but lets leave it alone.
> > 
> > > And there is arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dts,
> > > where a comment says that it has a BCM53134, but there is no such
> > > node. The ethernet node uses "rgmii".
> > 
> > aspeed pretty much always get phy-mode wrong. So i would not worry too
> > much about this.
> > 
> > > So one doesn't define one, one uses rgmii-id on the switch / phy side
> > > and rgmii on the ethernet mac side, and one only defines the ethernet
> > > mac side as rgmii.
> > 
> > That is reasonable. It is a lot less clear what is correct for a
> > MAC-MAC connection. For a MAC-PHY connection we do have documentation,
> > the preference is that the PHY adds the delays, not the MAC. If the
> > switch is playing PHY, then having it add delays is sensible.
> > 
> > > > I would maybe add a dev_warn() here, saying the DT blob is out of date
> > > > and needs fixing. And fix all the in kernel .dts files.
> > > 
> > > Sure I can add a warning.
> > 
> > Great, thanks.
> > 
> > 	Andrew
> 
> +Russell

As this is discussing the applicability of RGMII delays for DSA
switches, I've long held out that the situation is a mess, and
diverges from what we decide to do for MACs - so I'd prefer not
to get involved in this, except to say...

> Since there is no 'correct' way to apply RGMII delays on a MAC according
> to phy-mode, my advice, if possible, would be to leave sleeping dogs lie
> and fix broken setups by adding the explicit device tree properties in
> the MAC, and adding driver support for parsing these.

Indeed - let's not break existing working setups. If there is a
problem with them, then that's the time to start thinking about
changing them.

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

On Fri, Nov 7, 2025 at 4:33 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Nov 07, 2025 at 04:45:15PM +0200, Vladimir Oltean wrote:
> > On Fri, Nov 07, 2025 at 03:07:48PM +0100, Andrew Lunn wrote:
> > > > There is allwinner/sun7i-a20-lamobo-r1.dts, which uses "rgmii-txid",
> > > > which is untouched by this patch. The ethernet interface uses "rgmii".
> > >
> > > Which is odd, but lets leave it alone.
> > >
> > > > And there is arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-elbert.dts,
> > > > where a comment says that it has a BCM53134, but there is no such
> > > > node. The ethernet node uses "rgmii".
> > >
> > > aspeed pretty much always get phy-mode wrong. So i would not worry too
> > > much about this.
> > >
> > > > So one doesn't define one, one uses rgmii-id on the switch / phy side
> > > > and rgmii on the ethernet mac side, and one only defines the ethernet
> > > > mac side as rgmii.
> > >
> > > That is reasonable. It is a lot less clear what is correct for a
> > > MAC-MAC connection. For a MAC-PHY connection we do have documentation,
> > > the preference is that the PHY adds the delays, not the MAC. If the
> > > switch is playing PHY, then having it add delays is sensible.
> > >
> > > > > I would maybe add a dev_warn() here, saying the DT blob is out of date
> > > > > and needs fixing. And fix all the in kernel .dts files.
> > > >
> > > > Sure I can add a warning.
> > >
> > > Great, thanks.
> > >
> > >     Andrew
> >
> > +Russell
>
> As this is discussing the applicability of RGMII delays for DSA
> switches, I've long held out that the situation is a mess, and
> diverges from what we decide to do for MACs - so I'd prefer not
> to get involved in this, except to say...
>
> > Since there is no 'correct' way to apply RGMII delays on a MAC according
> > to phy-mode, my advice, if possible, would be to leave sleeping dogs lie
> > and fix broken setups by adding the explicit device tree properties in
> > the MAC, and adding driver support for parsing these.
>
> Indeed - let's not break existing working setups. If there is a
> problem with them, then that's the time to start thinking about
> changing them.

I completely understand the reluctance to change anything, and I'm
trying my best to not break anything:

The only mode used by in-tree device trees is "rgmii-txid". This
already behaved as it would for a PHY, and I did not change the
behavior.
As you probably know "rgmii" is often wrongly used, and the old
behavior was to enable delays in both directions in this case. I did
not change the behavior here either.

So for the known cases, and the suspected "wrong" usages, I did not
change anything, so these will continue working as expected.

My interpretation here so far was/is, and what I'm trying to follow here is:

if this is the CPU port that is connected to a different MAC (that is
controlled by the host), then on that port the switch is the "PHY", so
it is responsible for the delays according to phy-mode, as the other
MAC is supposed to not enable any (and doesn't know that there is a
DSA switch on the other side, unless it also is a DSA switch).

In any other case, don't apply any delays, because here the switch is
the MAC, and the other side is a PHY (or "PHY") and therefore
responsible for any delays that are needed.

Having an external b53 switch connected via its CPU port to an
internal b53 switch is a common setup on BCM63XX, so here b53 must
enable delays on one side, and currently it does not enable them on
either side.

Currently, the only way to configure this is by using the definitely
wrong "rgmii" phy-mode. Anyone writing a new board will just use it,
because it works, and we can't prevent it. I want to give the option
of using the less wrong "rgmii-id" value, which at least (in my
interpretation) matches the spirit of phy-mode.

Would it ease your concerns if I guard enabling delays with
dsa_port_is_cpu())? To make clear that we don't enable delays on any
user/dsa ports, and only the one that goes in the direction of the
kernel/host (the "root" of the DSA tree).

Best regards,
Jonas