Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x

On 05.09.2021 23:16, Florian Fainelli wrote:

On 9/5/2021 10:23 AM, Rafał Miłecki wrote:

quoted

From: Rafał Miłecki <rafal@milecki.pl>

Broadcom's b53 switches have one IMP (Inband Management Port) that needs
to be programmed using its own designed register. IMP port may be
different than CPU port - especially on devices with multiple CPU ports.

There are two choices: port 5 or port 8,

Yes. Depending on model I assign 5 or 8 in the b53_chip_data. What did
I miss?

quoted

For that reason it's required to explicitly note IMP port index and
check for it when choosing a register to use.

This commit fixes BCM5301x support. Those switches use CPU port 5 while
their IMP port is 8. Before this patch b53 was trying to program port 5
with B53_PORT_OVERRIDE_CTRL instead of B53_GMII_PORT_OVERRIDE_CTRL(5).

It may be possible to also replace "cpu_port" usages with
dsa_is_cpu_port() but that is out of the scope of thix BCM5301x fix.

Actually this would have been well within the scope of this patch.

I guess it's a matter of taste, I prefer to remove "cpu_port" usage
piece by piece. I think it makes it easier to catch mistakes during
review and regression finding easier.

For example I'm not exactly sure how to get rid of "cpu_port" in the:

if (port != dev->cpu_port) {
	b53_force_port_config(dev, dev->cpu_port, 2000,
			      DUPLEX_FULL, true, true);
	b53_force_link(dev, dev->cpu_port, 1);
}

quoted

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

I really don't like the duplication of the "imp_port" and "cpu_port" members, first because this caused us problems before, and second because for all switch entries except the BCM5301X, cpu_port == imp_port, so this a duplication, and a waste of storage space to encode information.

Well, this isn't exactly a duplication as values differ for BCM5301X.

I guess you prefer to handle BCM5301X with extra conditions in code
while I thought it to be cleaner to store that chip data in a struct.

Let's discuss code changes and see how it could be handled differently.

In fact, there is no such thing as CPU port technically you chose either IMP0 or IMP1. IMP0 is port 8 and IMP1 is port 5.

quoted

---
  drivers/net/dsa/b53/b53_common.c | 28 +++++++++++++++++++++++++---
  drivers/net/dsa/b53/b53_priv.h   |  1 +
  2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 5646eb8afe38..604f54112665 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c

@@ -1144,7 +1144,7 @@ static void b53_force_link(struct b53_device *dev, int port, int link)

      u8 reg, val, off;
      /* Override the port settings */
-    if (port == dev->cpu_port) {
+    if (port == dev->imp_port) {

This should be port == 8

What about devices that have IMP port 5? I think that change would break
b53_force_link() for them.

quoted

          off = B53_PORT_OVERRIDE_CTRL;
          val = PORT_OVERRIDE_EN;
      } else {

@@ -1168,7 +1168,7 @@ static void b53_force_port_config(struct b53_device *dev, int port,

      u8 reg, val, off;
      /* Override the port settings */
-    if (port == dev->cpu_port) {
+    if (port == dev->imp_port) {

Likewise

Same question.

quoted

          off = B53_PORT_OVERRIDE_CTRL;
          val = PORT_OVERRIDE_EN;
      } else {

@@ -1236,7 +1236,7 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,

      b53_force_link(dev, port, phydev->link);
      if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
-        if (port == 8)
+        if (port == dev->imp_port)

That use of port 8 was correct.

I tried to avoid some magic.

quoted

              off = B53_RGMII_CTRL_IMP;
          else
              off = B53_RGMII_CTRL_P(port);

@@ -2280,6 +2280,7 @@ struct b53_chip_data {

      const char *dev_name;
      u16 vlans;
      u16 enabled_ports;
+    u8 imp_port;
      u8 cpu_port;
      u8 vta_regs[3];
      u8 arl_bins;

@@ -2304,6 +2305,7 @@ static const struct b53_chip_data b53_switch_chips[] = {

          .enabled_ports = 0x1f,
          .arl_bins = 2,
          .arl_buckets = 1024,
+        .imp_port = 5,

Could have used B53_CPU_PORT_25 here.

I didn't use B53_CPU_PORT* defines as they don't apply anymore. That _25
suffix made sense when support for first devices with CPU/IMP port 5 was
added. They were actually BCM*25 chipsets.

I think we should probably have something like B53_IMP0 and B53_IMP1.
What do you think about proposed names?

quoted

          .cpu_port = B53_CPU_PORT_25,
          .duplex_reg = B53_DUPLEX_STAT_FE,
      },

@@ -2314,6 +2316,7 @@ static const struct b53_chip_data b53_switch_chips[] = {

          .enabled_ports = 0x1f,
          .arl_bins = 2,
          .arl_buckets = 1024,
+        .imp_port = 5,
          .cpu_port = B53_CPU_PORT_25,

and here.

quoted

          .duplex_reg = B53_DUPLEX_STAT_FE,
      },

@@ -2324,6 +2327,7 @@ static const struct b53_chip_data b53_switch_chips[] = {

          .enabled_ports = 0x1f,
          .arl_bins = 4,
          .arl_buckets = 1024,
+        .imp_port = 8,
          .cpu_port = B53_CPU_PORT,

and B53_CPU_PORT here and for each entry below.

I will put this patch into my local test rack and see what breaks, and we can address this more cleanly with net-next. Another case where if we had more time to do a proper review we could come up with a small fix, and not create additional technical debt to fix in the next release cycle. Hope's spring is eternal, oh and I just came back from France, so I guess I am full of complaints, too :)

You should have visit Disneyland! ;)