[PATCH net v2 0/5] net: dsa: b53: fix RGMII ports

[PATCH net v2 0/5] net: dsa: b53: fix RGMII ports

[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, and fix additional minor rgmii related issues
found while working on it.

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)

Changes v1 -> v2:

* add tested by, reviewed by
* do not enable RGMII delays on bcm63xx at all
* allow RGMII only on RGMII ports, not cpu ports
* prevent bcm63xx's CPU port from being configured
* prevent DLL_IQQD from being changed on BCM53115

Jonas Gorski (5):
  net: dsa: b53: do not enable EEE on bcm63xx
  net: dsa: b53: do not enable RGMII delay on bcm63xx
  net: dsa: b53: do not configure bcm63xx's IMP port interface
  net: dsa: b53: allow RGMII for bcm63xx RGMII ports
  net: dsa: b53: do not touch DLL_IQQD on bcm53115

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

-- 
2.43.0

[PATCH net v2 1/5] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Tested-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* add Reviewed-by, Tested-by

 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 132683ed3abe..8a6a370c8580 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2353,6 +2353,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;
@@ -2367,7 +2370,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

[PATCH net v2 2/5] net: dsa: b53: do not enable RGMII delay on bcm63xx

[Jonas Gorski]

bcm63xx's RGMII ports are always in MAC mode, never in PHY mode, so we
shouldn't enable any delays and let the PHY handle any delays as
necessary.

This fixes using RGMII ports with normal PHYs like BCM54612E, which will
handle the delay in the PHY.

Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* do not enable delays at all

 drivers/net/dsa/b53/b53_common.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 8a6a370c8580..c186ee3fb28d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1330,24 +1330,7 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
 		off = B53_RGMII_CTRL_P(port);
 
 	b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
-
-	switch (interface) {
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		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;
-		break;
-	case PHY_INTERFACE_MODE_RGMII_TXID:
-		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);
-		break;
-	}
+	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
 
 	if (port != dev->imp_port) {
 		if (is63268(dev))
-- 
2.43.0

[PATCH net v2 3/5] net: dsa: b53: do not configure bcm63xx's IMP port interface

[Jonas Gorski]

The IMP port is not a valid RGMII interface, but hard wired to internal,
so we shouldn't touch the undefined register B53_RGMII_CTRL_IMP.

While this does not seem to have any side effects, let's not touch it at
all, so limit RGMII configuration on bcm63xx to the actual RGMII ports.

Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* new patch

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

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index c186ee3fb28d..3f4934f974c8 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -22,6 +22,7 @@
 #include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/math.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/platform_data/b53.h>
 #include <linux/phy.h>
@@ -1322,24 +1323,17 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
 				  phy_interface_t interface)
 {
 	struct b53_device *dev = ds->priv;
-	u8 rgmii_ctrl = 0, off;
+	u8 rgmii_ctrl = 0;
 
-	if (port == dev->imp_port)
-		off = B53_RGMII_CTRL_IMP;
-	else
-		off = B53_RGMII_CTRL_P(port);
-
-	b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
+	b53_read8(dev, B53_CTRL_PAGE, B53_RGMII_CTRL_P(port), &rgmii_ctrl);
 	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
 
-	if (port != dev->imp_port) {
-		if (is63268(dev))
-			rgmii_ctrl |= RGMII_CTRL_MII_OVERRIDE;
+	if (is63268(dev))
+		rgmii_ctrl |= RGMII_CTRL_MII_OVERRIDE;
 
-		rgmii_ctrl |= RGMII_CTRL_ENABLE_GMII;
-	}
+	rgmii_ctrl |= RGMII_CTRL_ENABLE_GMII;
 
-	b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
+	b53_write8(dev, B53_CTRL_PAGE, B53_RGMII_CTRL_P(port), rgmii_ctrl);
 
 	dev_dbg(ds->dev, "Configured port %d for %s\n", port,
 		phy_modes(interface));
@@ -1484,7 +1478,7 @@ static void b53_phylink_mac_config(struct phylink_config *config,
 	struct b53_device *dev = ds->priv;
 	int port = dp->index;
 
-	if (is63xx(dev) && port >= B53_63XX_RGMII0)
+	if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4))
 		b53_adjust_63xx_rgmii(ds, port, interface);
 
 	if (mode == MLO_AN_FIXED) {
-- 
2.43.0

[PATCH net v2 4/5] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* add reviewed-by
* do not enable RGMII for CPU port (internal only, no rgmii)

 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 3f4934f974c8..be4493b769f4 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1439,6 +1439,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) && in_range(port, B53_63XX_RGMII0, 4))
+		phy_interface_set_rgmii(config->supported_interfaces);
+
 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100;
 
-- 
2.43.0

[PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

[Jonas Gorski]

According to OpenMDK, bit 2 of the RGMII register has a different
meaning for BCM53115 [1]:

"DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
                  mode"

Configuring RGMII delay works without setting this bit, so let's keep it
at the default. For other chips, we always set it, so not clearing it
is not an issue.

One would assume BCM53118 works the same, but OpenMDK is not quite sure
what this bit actually means [2]:

"BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
                    function mode1: Bypass dll65_2ns_del IP0: Use
                    dll65_2ns_del IP"

So lets keep setting it for now.

[1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
[2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* new patch

 drivers/net/dsa/b53/b53_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index be4493b769f4..862bdccb7439 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
 	 * tx_clk aligned timing (restoring to reset defaults)
 	 */
 	b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
-	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
-			RGMII_CTRL_TIMING_SEL);
+	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
 
 	/* PHY_INTERFACE_MODE_RGMII_TXID means TX internal delay, make
 	 * sure that we enable the port TX clock internal delay to
@@ -1375,7 +1374,10 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
 		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
 	if (interface == PHY_INTERFACE_MODE_RGMII)
 		rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
-	rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
+
+	if (dev->chip_id != BCM53115_DEVICE_ID)
+		rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
+
 	b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
 
 	dev_info(ds->dev, "Configured port %d for %s\n", port,
-- 
2.43.0

Re: [PATCH net v2 2/5] net: dsa: b53: do not enable RGMII delay on bcm63xx

[Florian Fainelli]

On 6/2/25 12:39, Jonas Gorski wrote:
> bcm63xx's RGMII ports are always in MAC mode, never in PHY mode, so we
> shouldn't enable any delays and let the PHY handle any delays as
> necessary.
> 
> This fixes using RGMII ports with normal PHYs like BCM54612E, which will
> handle the delay in the PHY.
> 
> 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

Re: [PATCH net v2 3/5] net: dsa: b53: do not configure bcm63xx's IMP port interface

[Florian Fainelli]

On 6/2/25 12:39, Jonas Gorski wrote:
> The IMP port is not a valid RGMII interface, but hard wired to internal,
> so we shouldn't touch the undefined register B53_RGMII_CTRL_IMP.
> 
> While this does not seem to have any side effects, let's not touch it at
> all, so limit RGMII configuration on bcm63xx to the actual RGMII ports.
> 
> 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

Re: [PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

[Florian Fainelli]

On 6/2/25 12:39, Jonas Gorski wrote:
> According to OpenMDK, bit 2 of the RGMII register has a different
> meaning for BCM53115 [1]:
> 
> "DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
>                    mode"
> 
> Configuring RGMII delay works without setting this bit, so let's keep it
> at the default. For other chips, we always set it, so not clearing it
> is not an issue.
> 
> One would assume BCM53118 works the same, but OpenMDK is not quite sure
> what this bit actually means [2]:
> 
> "BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
>                      function mode1: Bypass dll65_2ns_del IP0: Use
>                      dll65_2ns_del IP"
> 
> So lets keep setting it for now.
> 
> [1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
> [2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392
> 
> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> ---
> v1 -> v2:
> * new patch
> 
>   drivers/net/dsa/b53/b53_common.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index be4493b769f4..862bdccb7439 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
>   	 * tx_clk aligned timing (restoring to reset defaults)
>   	 */
>   	b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
> -	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
> -			RGMII_CTRL_TIMING_SEL);
> +	rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);

Are not we missing a:

if (dev->chip_id != BCM53115_DEVICE_ID)
	rgmii_ctrl &= ~RGMII_CTRL_TIMING_SEL;

here to be strictly identical before/after?

>   
>   	/* PHY_INTERFACE_MODE_RGMII_TXID means TX internal delay, make
>   	 * sure that we enable the port TX clock internal delay to
> @@ -1375,7 +1374,10 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
>   		rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
>   	if (interface == PHY_INTERFACE_MODE_RGMII)
>   		rgmii_ctrl |= RGMII_CTRL_DLL_TXC | RGMII_CTRL_DLL_RXC;
> -	rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> +
> +	if (dev->chip_id != BCM53115_DEVICE_ID)
> +		rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> +
>   	b53_write8(dev, B53_CTRL_PAGE, off, rgmii_ctrl);
>   
>   	dev_info(ds->dev, "Configured port %d for %s\n", port,

-- 
Florian

Re: [PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

[Jonas Gorski]

On Mon, Jun 2, 2025 at 11:40 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 6/2/25 12:39, Jonas Gorski wrote:
> > According to OpenMDK, bit 2 of the RGMII register has a different
> > meaning for BCM53115 [1]:
> >
> > "DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
> >                    mode"
> >
> > Configuring RGMII delay works without setting this bit, so let's keep it
> > at the default. For other chips, we always set it, so not clearing it
> > is not an issue.
> >
> > One would assume BCM53118 works the same, but OpenMDK is not quite sure
> > what this bit actually means [2]:
> >
> > "BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
> >                      function mode1: Bypass dll65_2ns_del IP0: Use
> >                      dll65_2ns_del IP"
> >
> > So lets keep setting it for now.
> >
> > [1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
> > [2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392
> >
> > Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> > ---
> > v1 -> v2:
> > * new patch
> >
> >   drivers/net/dsa/b53/b53_common.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index be4493b769f4..862bdccb7439 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
> >        * tx_clk aligned timing (restoring to reset defaults)
> >        */
> >       b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
> > -     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
> > -                     RGMII_CTRL_TIMING_SEL);
> > +     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>
> Are not we missing a:
>
> if (dev->chip_id != BCM53115_DEVICE_ID)
>         rgmii_ctrl &= ~RGMII_CTRL_TIMING_SEL;
>
> here to be strictly identical before/after?

We could add it for symmetry, but it would be purely decorational. We
unconditionally set this bit again later, so clearing it before has no
actual effect, which is why I didn't add it.

Regards,
Jonas

Re: [PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

[Paolo Abeni]

On 6/3/25 8:15 AM, Jonas Gorski wrote:
> On Mon, Jun 2, 2025 at 11:40 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>> On 6/2/25 12:39, Jonas Gorski wrote:
>>> According to OpenMDK, bit 2 of the RGMII register has a different
>>> meaning for BCM53115 [1]:
>>>
>>> "DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
>>>                    mode"
>>>
>>> Configuring RGMII delay works without setting this bit, so let's keep it
>>> at the default. For other chips, we always set it, so not clearing it
>>> is not an issue.
>>>
>>> One would assume BCM53118 works the same, but OpenMDK is not quite sure
>>> what this bit actually means [2]:
>>>
>>> "BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
>>>                      function mode1: Bypass dll65_2ns_del IP0: Use
>>>                      dll65_2ns_del IP"
>>>
>>> So lets keep setting it for now.
>>>
>>> [1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
>>> [2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392
>>>
>>> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>> ---
>>> v1 -> v2:
>>> * new patch
>>>
>>>   drivers/net/dsa/b53/b53_common.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index be4493b769f4..862bdccb7439 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
>>>        * tx_clk aligned timing (restoring to reset defaults)
>>>        */
>>>       b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
>>> -     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
>>> -                     RGMII_CTRL_TIMING_SEL);
>>> +     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>
>> Are not we missing a:
>>
>> if (dev->chip_id != BCM53115_DEVICE_ID)
>>         rgmii_ctrl &= ~RGMII_CTRL_TIMING_SEL;
>>
>> here to be strictly identical before/after?
> 
> We could add it for symmetry, but it would be purely decorational. We
> unconditionally set this bit again later, so clearing it before has no
> actual effect, which is why I didn't add it.

Makes sense, and the code in this patch is IMHO more readable.

/P

Re: [PATCH net v2 0/5] net: dsa: b53: fix RGMII ports

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon,  2 Jun 2025 21:39:48 +0200 you wrote:
> 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, and fix additional minor rgmii related issues
> found while working on it.
> 
> With a BCM96328BU-P300:
> 
> [...]

Here is the summary with links:
  - [net,v2,1/5] net: dsa: b53: do not enable EEE on bcm63xx
    https://git.kernel.org/netdev/net/c/1237c2d4a8db
  - [net,v2,2/5] net: dsa: b53: do not enable RGMII delay on bcm63xx
    https://git.kernel.org/netdev/net/c/4af523551d87
  - [net,v2,3/5] net: dsa: b53: do not configure bcm63xx's IMP port interface
    https://git.kernel.org/netdev/net/c/75f4f7b2b130
  - [net,v2,4/5] net: dsa: b53: allow RGMII for bcm63xx RGMII ports
    https://git.kernel.org/netdev/net/c/5ea0d42c1980
  - [net,v2,5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115
    https://git.kernel.org/netdev/net/c/bc1a65eb81a2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2 5/5] net: dsa: b53: do not touch DLL_IQQD on bcm53115

[Florian Fainelli]

On 6/5/25 02:06, Paolo Abeni wrote:
> On 6/3/25 8:15 AM, Jonas Gorski wrote:
>> On Mon, Jun 2, 2025 at 11:40 PM Florian Fainelli
>> <florian.fainelli@broadcom.com> wrote:
>>> On 6/2/25 12:39, Jonas Gorski wrote:
>>>> According to OpenMDK, bit 2 of the RGMII register has a different
>>>> meaning for BCM53115 [1]:
>>>>
>>>> "DLL_IQQD         1: In the IDDQ mode, power is down0: Normal function
>>>>                     mode"
>>>>
>>>> Configuring RGMII delay works without setting this bit, so let's keep it
>>>> at the default. For other chips, we always set it, so not clearing it
>>>> is not an issue.
>>>>
>>>> One would assume BCM53118 works the same, but OpenMDK is not quite sure
>>>> what this bit actually means [2]:
>>>>
>>>> "BYPASS_IMP_2NS_DEL #1: In the IDDQ mode, power is down#0: Normal
>>>>                       function mode1: Bypass dll65_2ns_del IP0: Use
>>>>                       dll65_2ns_del IP"
>>>>
>>>> So lets keep setting it for now.
>>>>
>>>> [1] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53115/bcm53115_a0_defs.h#L19871
>>>> [2] https://github.com/Broadcom-Network-Switching-Software/OpenMDK/blob/master/cdk/PKG/chip/bcm53118/bcm53118_a0_defs.h#L14392
>>>>
>>>> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>> ---
>>>> v1 -> v2:
>>>> * new patch
>>>>
>>>>    drivers/net/dsa/b53/b53_common.c | 8 +++++---
>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>>> index be4493b769f4..862bdccb7439 100644
>>>> --- a/drivers/net/dsa/b53/b53_common.c
>>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>>> @@ -1354,8 +1354,7 @@ static void b53_adjust_531x5_rgmii(struct dsa_switch *ds, int port,
>>>>         * tx_clk aligned timing (restoring to reset defaults)
>>>>         */
>>>>        b53_read8(dev, B53_CTRL_PAGE, off, &rgmii_ctrl);
>>>> -     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC |
>>>> -                     RGMII_CTRL_TIMING_SEL);
>>>> +     rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
>>>
>>> Are not we missing a:
>>>
>>> if (dev->chip_id != BCM53115_DEVICE_ID)
>>>          rgmii_ctrl &= ~RGMII_CTRL_TIMING_SEL;
>>>
>>> here to be strictly identical before/after?
>>
>> We could add it for symmetry, but it would be purely decorational. We
>> unconditionally set this bit again later, so clearing it before has no
>> actual effect, which is why I didn't add it.
> 
> Makes sense, and the code in this patch is IMHO more readable.

Agreed, thanks for taking those patches.
-- 
Florian