Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x
From: Florian Fainelli <f.fainelli@gmail.com>
Date: 2021-09-05 21:16:06
On 9/5/2021 10:23 AM, Rafał Miłecki wrote:
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,
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.
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. 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 hunk ↗ jump to hunk
--- 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
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
.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 hunk ↗ jump to hunk
.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 :) -- Florian