[PATCH net 0/3] net: dsa: fix RGMII ports on BCM63xx

[PATCH net 0/3] net: dsa: fix RGMII ports on BCM63xx

[Jonas Gorski]

RGMII ports on BCM63xx were not really working, especially with PHYs
that support EEE and are capable of configuring their own RGMII delays.

So let's make them work.

With a BCM96328BU-P300:

Before:

[    3.580000] b53-switch 10700000.switch GbE3 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
[    3.600000] b53-switch 10700000.switch GbE3 (uninitialized): failed to connect to PHY: -EINVAL
[    3.610000] b53-switch 10700000.switch GbE3 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4
[    3.620000] b53-switch 10700000.switch GbE1 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
[    3.640000] b53-switch 10700000.switch GbE1 (uninitialized): failed to connect to PHY: -EINVAL
[    3.650000] b53-switch 10700000.switch GbE1 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 5
[    3.660000] b53-switch 10700000.switch GbE4 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
[    3.680000] b53-switch 10700000.switch GbE4 (uninitialized): failed to connect to PHY: -EINVAL
[    3.690000] b53-switch 10700000.switch GbE4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 6
[    3.700000] b53-switch 10700000.switch GbE5 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
[    3.720000] b53-switch 10700000.switch GbE5 (uninitialized): failed to connect to PHY: -EINVAL
[    3.730000] b53-switch 10700000.switch GbE5 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 7

After:

[    3.700000] b53-switch 10700000.switch GbE3 (uninitialized): PHY [mdio_mux-0.1:00] driver [Broadcom BCM54612E] (irq=POLL)
[    3.770000] b53-switch 10700000.switch GbE1 (uninitialized): PHY [mdio_mux-0.1:01] driver [Broadcom BCM54612E] (irq=POLL)
[    3.850000] b53-switch 10700000.switch GbE4 (uninitialized): PHY [mdio_mux-0.1:18] driver [Broadcom BCM54612E] (irq=POLL)
[    3.920000] b53-switch 10700000.switch GbE5 (uninitialized): PHY [mdio_mux-0.1:19] driver [Broadcom BCM54612E] (irq=POLL)

Jonas Gorski (3):
  net: dsa: b53: do not enable EEE on bcm63xx
  net: dsa: b53: fix configuring RGMII delay on bcm63xx
  net: dsa: b53: allow RGMII for bcm63xx RGMII ports

 drivers/net/dsa/b53/b53_common.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH net 1/3] net: dsa: b53: do not enable EEE on bcm63xx

[Jonas Gorski]

BCM63xx internal switches do not support EEE, but provide multiple RGMII
ports where external PHYs may be connected. If one of these PHYs are EEE
capable, we may try to enable EEE for the MACs, which then hangs the
system on access of the (non-existent) EEE registers.

Fix this by checking if the switch actually supports EEE before
attempting to configure it.

Fixes: 22256b0afb12 ("net: dsa: b53: Move EEE functions to b53")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 7216eb8f9493..a316f8c01d0a 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2348,6 +2348,9 @@ int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	int ret;
 
+	if (!b53_support_eee(ds, port))
+		return 0;
+
 	ret = phy_init_eee(phy, false);
 	if (ret)
 		return 0;
@@ -2362,7 +2365,7 @@ bool b53_support_eee(struct dsa_switch *ds, int port)
 {
 	struct b53_device *dev = ds->priv;
 
-	return !is5325(dev) && !is5365(dev);
+	return !is5325(dev) && !is5365(dev) && !is63xx(dev);
 }
 EXPORT_SYMBOL(b53_support_eee);
 
-- 
2.43.0

Re: [PATCH net 1/3] net: dsa: b53: do not enable EEE on bcm63xx

[Florian Fainelli]

On 5/19/25 10:45, Jonas Gorski wrote:
> BCM63xx internal switches do not support EEE, but provide multiple RGMII
> ports where external PHYs may be connected. If one of these PHYs are EEE
> capable, we may try to enable EEE for the MACs, which then hangs the
> system on access of the (non-existent) EEE registers.
> 
> Fix this by checking if the switch actually supports EEE before
> attempting to configure it.
> 
> Fixes: 22256b0afb12 ("net: dsa: b53: Move EEE functions to b53")
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

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

[PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Jonas Gorski]

The RGMII delay type of the PHY interface is intended for the PHY, not
the MAC, so we need to configure the opposite. Else we double the delay
or don't add one at all if the PHY also supports configuring delays.

Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
actually being effective.

Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
delays in its driver.

Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a316f8c01d0a..b00975189dab 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII_ID:
-		rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
+		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
 		break;
 	case PHY_INTERFACE_MODE_RGMII_RXID:
-		rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
-		rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
+		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
+		rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
 		break;
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
-		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
+		rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
+		rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
 		break;
 	case PHY_INTERFACE_MODE_RGMII:
 	default:
-		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
+		rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
 		break;
 	}
 
@@ -1350,6 +1350,7 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
 
 		rgmii_ctrl |= RGMII_CTRL_ENABLE_GMII;
 	}
+	rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
 
 	b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
 
-- 
2.43.0

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Andrew Lunn]

On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
> The RGMII delay type of the PHY interface is intended for the PHY, not
> the MAC, so we need to configure the opposite. Else we double the delay
> or don't add one at all if the PHY also supports configuring delays.
> 
> Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
> actually being effective.
> 
> Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
> delays in its driver.

We have to be careful here not to cause regressions. It might be
wrong, but are there systems using this which actually work? Does this
change break them?

> 
> Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> ---
>  drivers/net/dsa/b53/b53_common.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index a316f8c01d0a..b00975189dab 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
>  
>  	switch (interface) {
>  	case PHY_INTERFACE_MODE_RGMII_ID:
> -		rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> +		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
> -		rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
> -		rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> +		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> +		rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
> -		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> +		rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> +		rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII:
>  	default:
> -		rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> +		rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
>  		break;

These changes look wrong. There is more background here:

https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

	Andrew

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Jonas Gorski]

On Mon, May 19, 2025 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
> > The RGMII delay type of the PHY interface is intended for the PHY, not
> > the MAC, so we need to configure the opposite. Else we double the delay
> > or don't add one at all if the PHY also supports configuring delays.
> >
> > Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
> > actually being effective.
> >
> > Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
> > delays in its driver.
>
> We have to be careful here not to cause regressions. It might be
> wrong, but are there systems using this which actually work? Does this
> change break them?

The only user (of bcm63xx and b53 dsa) I am aware of is OpenWrt, and
we are capable of updating our dts files in case they were using
broken configuration. Though having PHYs on the RGMII ports is a very
rare configuration, and usually there is switch connected with a fixed
link, so likely the issue was never detected.

> >
> > Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> > ---
> >  drivers/net/dsa/b53/b53_common.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index a316f8c01d0a..b00975189dab 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
> >
> >       switch (interface) {
> >       case PHY_INTERFACE_MODE_RGMII_ID:
> > -             rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > +             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII_RXID:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
> > -             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > +             rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII_TXID:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
> > -             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > +             rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII:
> >       default:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
> >               break;
>
> These changes look wrong. There is more background here:
>
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

This is what makes it work for me (I tested all four modes, rgmii,
rgmii-id, rgmii-txid and rgmii-rxid). Without this change, b53 will
configure the same delays on the MAC layer as the PHY driver (bcm54xx,
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/phy/broadcom.c#L73
), which breaks connectivity at least for me.

E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
enable rx and tx delays, causing the delays to be 4 ns instead of 2
ns. So I don't see how this could have ever worked.

Also note that b53_adjust_531x5_rgmii()
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/dsa/b53/b53_common.c#L1360
already behaves that way, this just makes bcm63xx now work the same
(so these functions could now even be merged).

I did see the part where the document says the MAC driver is supposed
to change the phy mode, but I haven't really found out how I am
supposed to do that within phylink/dsa, since it never passes any phy
modes to any PHYs anywhere, just reports what it supports, then
phylink tells it what to configure AFAICT.

Best regards,
Jonas

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Andrew Lunn]

> > These changes look wrong. There is more background here:
> >
> > https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
> 
> This is what makes it work for me (I tested all four modes, rgmii,
> rgmii-id, rgmii-txid and rgmii-rxid).

So you have rgmii-id which works, and the rest are broken?

> E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
> enable rx and tx delays, causing the delays to be 4 ns instead of 2
> ns. So I don't see how this could have ever worked.

In this situation, the switch port is playing the role of MAC. Hence i
would stick to what is suggested in ethernet-controller.yaml. The MAC
does not add any delays, and leaves it to the PHY.
phylink_fwnode_phy_connect() gets the phy-mode from the DT blob, and
passes it to phylib when it calls phy_attach_direct().

There is a second use case for DSA, when the port is connected to the
host, i.e. the port is a CPU port. In that setup, the port is playing
the PHY side of the RGMII connection, with the host conduit interface
being the MAC. In that setup, the above recommendations is that the
conduit interface does not add delays, with the expectation the 'PHY'
adds the delays. Then the DSA port does need to add delays. So you
might want to use dsa_is_cpu_port() to decide if to add delays or not.

    Andrew

---
pw-bot: cr

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Jonas Gorski]

On Mon, May 19, 2025 at 10:34 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > These changes look wrong. There is more background here:
> > >
> > > https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
> >
> > This is what makes it work for me (I tested all four modes, rgmii,
> > rgmii-id, rgmii-txid and rgmii-rxid).
>
> So you have rgmii-id which works, and the rest are broken?

I have four rgmii ports with PHYs, I configured each one to a
different mode (rgmii, rgmii-txid, rgmii-rxid, rgmii-id).

Without this change no mode/port works, since there is always either a
0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no
equipment to measure).

With this change all modes/ports work. With "rgmii-id" the mac doesn't
configure any delays (and the phy does instead), with "rgmii" it's
vice versa, so there is always the expected 2 ns delay. Same for rxid
and txid.

Though on testing again RGMII_CTRL_TIMING_SEL doesn't seem to be needed.

> > E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
> > enable rx and tx delays, causing the delays to be 4 ns instead of 2
> > ns. So I don't see how this could have ever worked.
>
> In this situation, the switch port is playing the role of MAC. Hence i
> would stick to what is suggested in ethernet-controller.yaml. The MAC
> does not add any delays, and leaves it to the PHY.
> phylink_fwnode_phy_connect() gets the phy-mode from the DT blob, and
> passes it to phylib when it calls phy_attach_direct().
>
> There is a second use case for DSA, when the port is connected to the
> host, i.e. the port is a CPU port. In that setup, the port is playing
> the PHY side of the RGMII connection, with the host conduit interface
> being the MAC. In that setup, the above recommendations is that the
> conduit interface does not add delays, with the expectation the 'PHY'
> adds the delays. Then the DSA port does need to add delays. So you
> might want to use dsa_is_cpu_port() to decide if to add delays or not.

The Switch is always integrated into the host SoC, so there is no
(r)gmii cpu port to configure. There's basically directly attached DMA
to/from the buffers of the cpu port. Not sure if there are even
buffers, or if it is a direct to DMA delivery.

Best Regards,
Jonas

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Andrew Lunn]

> Without this change no mode/port works, since there is always either a
> 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no
> equipment to measure).
> 
> With this change all modes/ports work.

Which is wrong. 

> With "rgmii-id" the mac doesn't
> configure any delays (and the phy does instead), with "rgmii" it's
> vice versa, so there is always the expected 2 ns delay. Same for rxid
> and txid.

If you read the description of what these four modes mean, you should
understand why only one should work. And given the most likely PCB
design, the only mode that should work is rgmii-id. You would have to
change the PCB design, to make the other modes work.

> The Switch is always integrated into the host SoC, so there is no
> (r)gmii cpu port to configure. There's basically directly attached DMA
> to/from the buffers of the cpu port. Not sure if there are even
> buffers, or if it is a direct to DMA delivery.

That makes it a lot simpler. It always plays the MAC side. So i
recommend you just hard code it no delay, and let the PHY add the
delays as needed.

	Andrew

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Jonas Gorski]

On Tue, May 20, 2025 at 2:15 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Without this change no mode/port works, since there is always either a
> > 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no
> > equipment to measure).
> >
> > With this change all modes/ports work.
>
> Which is wrong.
>
> > With "rgmii-id" the mac doesn't
> > configure any delays (and the phy does instead), with "rgmii" it's
> > vice versa, so there is always the expected 2 ns delay. Same for rxid
> > and txid.
>
> If you read the description of what these four modes mean, you should
> understand why only one should work. And given the most likely PCB
> design, the only mode that should work is rgmii-id. You would have to
> change the PCB design, to make the other modes work.

Since I also have BCM6368 with a BCM53115 connected to one of the
RGMII ports lying around, I played around with it, and it was
surprisingly hard to make it *not* work. Only if I enabled delay on
*both* sides it stopped working, no delay or delay only on one side
continued working (and I used iperf to try if larger amounts of
traffic break it).

So in way, with BCM6368 enabling no (sampling) delays on its MAC side,
all four modes work. I understand that they shouldn't, but the reality
is that they do. Maybe the switches can auto-detect/adapt to (missing)
delays for a certain amount.

@Florian, do you know if this is expected? And yes, I even added the
RGMII delay workaround (change link speed to force the pll to resync)
to ensure that the delays are applied. Though I guess the way Linux
works it isn't needed, and only when changing delays while the link is
up.

> > The Switch is always integrated into the host SoC, so there is no
> > (r)gmii cpu port to configure. There's basically directly attached DMA
> > to/from the buffers of the cpu port. Not sure if there are even
> > buffers, or if it is a direct to DMA delivery.
>
> That makes it a lot simpler. It always plays the MAC side. So i
> recommend you just hard code it no delay, and let the PHY add the
> delays as needed.

Sure thing. I saw that there are device tree properties that can be
used to explicitly enable delays on the MAC side, so in case we ever
need them we can implement them (e.g. a PHY that can't do delays).

Best regards,
Jonas

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Andrew Lunn]

On Fri, May 23, 2025 at 11:08:55AM +0200, Jonas Gorski wrote:
> On Tue, May 20, 2025 at 2:15 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Without this change no mode/port works, since there is always either a
> > > 0 ns delay or a 4 ns delay in the rx/tx paths (I assume, I have no
> > > equipment to measure).
> > >
> > > With this change all modes/ports work.
> >
> > Which is wrong.
> >
> > > With "rgmii-id" the mac doesn't
> > > configure any delays (and the phy does instead), with "rgmii" it's
> > > vice versa, so there is always the expected 2 ns delay. Same for rxid
> > > and txid.
> >
> > If you read the description of what these four modes mean, you should
> > understand why only one should work. And given the most likely PCB
> > design, the only mode that should work is rgmii-id. You would have to
> > change the PCB design, to make the other modes work.
> 
> Since I also have BCM6368 with a BCM53115 connected to one of the
> RGMII ports lying around, I played around with it, and it was
> surprisingly hard to make it *not* work. Only if I enabled delay on
> *both* sides it stopped working, no delay or delay only on one side
> continued working (and I used iperf to try if larger amounts of
> traffic break it).

Interesting. You see the Rockchip people insisting their devices need
fine tuning, 2ns is not good enough, it needs to be 1.9ns. And then
they end up in a mess with interpreting what phy-mode actually means.

In some ways, this just as bad. You can get away with using 'rgmii'
which is wrong, when it should be 'rgmii-id'. It would be better if
only just one mode worked, then DT developers would get it correct.

	Andrew

Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

On 5/19/25 12:44, Jonas Gorski wrote:
> On Mon, May 19, 2025 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
>>> The RGMII delay type of the PHY interface is intended for the PHY, not
>>> the MAC, so we need to configure the opposite. Else we double the delay
>>> or don't add one at all if the PHY also supports configuring delays.
>>>
>>> Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
>>> actually being effective.
>>>
>>> Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
>>> delays in its driver.
>>
>> We have to be careful here not to cause regressions. It might be
>> wrong, but are there systems using this which actually work? Does this
>> change break them?
> 
> The only user (of bcm63xx and b53 dsa) I am aware of is OpenWrt, and
> we are capable of updating our dts files in case they were using
> broken configuration. Though having PHYs on the RGMII ports is a very
> rare configuration, and usually there is switch connected with a fixed
> link, so likely the issue was never detected.
> 
>>>
>>> Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>> ---
>>>   drivers/net/dsa/b53/b53_common.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index a316f8c01d0a..b00975189dab 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
>>>
>>>        switch (interface) {
>>>        case PHY_INTERFACE_MODE_RGMII_ID:
>>> -             rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>> +             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII_RXID:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
>>> -             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
>>> +             rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII_TXID:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
>>> -             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
>>> +             rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
>>>                break;
>>>        case PHY_INTERFACE_MODE_RGMII:
>>>        default:
>>> -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>> +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
>>>                break;
>>
>> These changes look wrong. There is more background here:
>>
>> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
> 
> This is what makes it work for me (I tested all four modes, rgmii,
> rgmii-id, rgmii-txid and rgmii-rxid). Without this change, b53 will
> configure the same delays on the MAC layer as the PHY driver (bcm54xx,
> https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/phy/broadcom.c#L73
> ), which breaks connectivity at least for me.
> 
> E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
> enable rx and tx delays, causing the delays to be 4 ns instead of 2
> ns. So I don't see how this could have ever worked.

It's possible this was tested with an external PHY which was not 
configured by drivers/net/phy/broadcom.c?

> 
> Also note that b53_adjust_531x5_rgmii()
> https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/dsa/b53/b53_common.c#L1360
> already behaves that way, this just makes bcm63xx now work the same
> (so these functions could now even be merged).

Yes, I was going to suggest we should be doing that.

There are precedents with other Broadcom drivers for doing things 
incorrectly, and having to put quirks to deal with that, yours truly 
having greatly contributed to doing that:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b972b54a68b2512a7528658ecd023aea108c03a5
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4208 bytes --]

[PATCH net 3/3] net: dsa: b53: allow RGMII for bcm63xx RGMII ports

[Jonas Gorski]

Add RGMII to supported interfaces for BCM63xx RGMII ports so they can be
actually used in RGMII mode.

Without this, phylink will fail to configure them:

[    3.580000] b53-switch 10700000.switch GbE3 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
[    3.600000] b53-switch 10700000.switch GbE3 (uninitialized): failed to connect to PHY: -EINVAL
[    3.610000] b53-switch 10700000.switch GbE3 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4

Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index b00975189dab..9d97ad146ce4 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1458,6 +1458,10 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 	__set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
 
+	/* BCM63xx RGMII ports support RGMII */
+	if (is63xx(dev) && port >= B53_63XX_RGMII0)
+		phy_interface_set_rgmii(config->supported_interfaces);
+
 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100;
 
-- 
2.43.0

Re: [PATCH net 3/3] net: dsa: b53: allow RGMII for bcm63xx RGMII ports

[Florian Fainelli]

On 5/19/25 10:45, Jonas Gorski wrote:
> Add RGMII to supported interfaces for BCM63xx RGMII ports so they can be
> actually used in RGMII mode.
> 
> Without this, phylink will fail to configure them:
> 
> [    3.580000] b53-switch 10700000.switch GbE3 (uninitialized): validation of rgmii with support 0000000,00000000,00000000,000062ff and advertisement 0000000,00000000,00000000,000062ff failed: -EINVAL
> [    3.600000] b53-switch 10700000.switch GbE3 (uninitialized): failed to connect to PHY: -EINVAL
> [    3.610000] b53-switch 10700000.switch GbE3 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 4
> 
> Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

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