[RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Florian Fainelli]

Hi all,

Although this should work for all devices, since most DTBs on the
platforms where bcm_sf2 is use do not populate a 'fixed-link' property
for their CPU ports, but rely on the Ethernet controller DT node doing
that, we will not be registering a 'fixed-link' instance for CPU ports.

This still works because the switch matches the configuration of the
Ethernet controller, but on BCM4908 where we want to force 2GBits/sec,
that I cannot test, not so sure.

So as of now, this series does not produce register for register
compatile changes.

Florian Fainelli (2):
  net: dsa: bcm_sf2: Introduce helper for port override offset
  net: dsa: bcm_sf2: Have PHYLINK configure CPU/IMP port(s)

 drivers/net/dsa/bcm_sf2.c | 130 ++++++++++++++++++--------------------
 1 file changed, 63 insertions(+), 67 deletions(-)

-- 
2.25.1

[RFC net-next 1/2] net: dsa: bcm_sf2: Introduce helper for port override offset

[Florian Fainelli]

Depending upon the generation of switches, we have different offsets for
configuring a given port's status override where link parameters are
applied. Introduce a helper function that we re-use throughout the code
in order to let phylink callbacks configure the IMP/CPU port(s) in
subsequent changes.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index be0edfa093d0..10de0cffa047 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -94,6 +94,24 @@ static u16 bcm_sf2_reg_led_base(struct bcm_sf2_priv *priv, int port)
 	return REG_SWITCH_STATUS;
 }
 
+static u32 bcm_sf2_port_override_offset(struct bcm_sf2_priv *priv, int port)
+{
+	switch (priv->type) {
+	case BCM4908_DEVICE_ID:
+	case BCM7445_DEVICE_ID:
+		return port == 8 ? CORE_STS_OVERRIDE_IMP :
+				   CORE_STS_OVERRIDE_GMIIP_PORT(port);
+	case BCM7278_DEVICE_ID:
+		return port == 8 ? CORE_STS_OVERRIDE_IMP2 :
+				   CORE_STS_OVERRIDE_GMIIP2_PORT(port);
+	default:
+		WARN_ONCE(1, "Unsupported device: %d\n", priv->type);
+	}
+
+	/* RO fallback register */
+	return REG_SWITCH_STATUS;
+}
+
 /* Return the number of active ports, not counting the IMP (CPU) port */
 static unsigned int bcm_sf2_num_active_ports(struct dsa_switch *ds)
 {
@@ -167,11 +185,7 @@ static void bcm_sf2_imp_setup(struct dsa_switch *ds, int port)
 	b53_brcm_hdr_setup(ds, port);
 
 	if (port == 8) {
-		if (priv->type == BCM4908_DEVICE_ID ||
-		    priv->type == BCM7445_DEVICE_ID)
-			offset = CORE_STS_OVERRIDE_IMP;
-		else
-			offset = CORE_STS_OVERRIDE_IMP2;
+		offset = bcm_sf2_port_override_offset(priv, port);
 
 		/* Force link status for IMP port */
 		reg = core_readl(priv, offset);
@@ -813,12 +827,7 @@ static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port,
 		return;
 
 	if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
-		if (priv->type == BCM4908_DEVICE_ID ||
-		    priv->type == BCM7445_DEVICE_ID)
-			offset = CORE_STS_OVERRIDE_GMIIP_PORT(port);
-		else
-			offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
-
+		offset = bcm_sf2_port_override_offset(priv, port);
 		reg = core_readl(priv, offset);
 		reg &= ~LINK_STS;
 		core_writel(priv, reg, offset);
@@ -843,11 +852,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
 		u32 reg_rgmii_ctrl = 0;
 		u32 reg, offset;
 
-		if (priv->type == BCM4908_DEVICE_ID ||
-		    priv->type == BCM7445_DEVICE_ID)
-			offset = CORE_STS_OVERRIDE_GMIIP_PORT(port);
-		else
-			offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
+		offset = bcm_sf2_port_override_offset(priv, port);
 
 		if (interface == PHY_INTERFACE_MODE_RGMII ||
 		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-- 
2.25.1

[RFC net-next 2/2] net: dsa: bcm_sf2: Have PHYLINK configure CPU/IMP port(s)

[Florian Fainelli]

Remove the artificial limitations imposed upon
bcm_sf2_sw_mac_link_{up,down} and allow us to override the link
parameters for IMP port(s) as well as regular ports by accounting for
the special differences that exist there.

Remove the code that did override the link parameters in
bcm_sf2_imp_setup().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 95 ++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 52 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 10de0cffa047..572f7450b527 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -159,7 +159,7 @@ static void bcm_sf2_imp_setup(struct dsa_switch *ds, int port)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	unsigned int i;
-	u32 reg, offset;
+	u32 reg;
 
 	/* Enable the port memories */
 	reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
@@ -185,17 +185,6 @@ static void bcm_sf2_imp_setup(struct dsa_switch *ds, int port)
 	b53_brcm_hdr_setup(ds, port);
 
 	if (port == 8) {
-		offset = bcm_sf2_port_override_offset(priv, port);
-
-		/* Force link status for IMP port */
-		reg = core_readl(priv, offset);
-		reg |= (MII_SW_OR | LINK_STS);
-		if (priv->type == BCM4908_DEVICE_ID)
-			reg |= GMII_SPEED_UP_2G;
-		else
-			reg &= ~GMII_SPEED_UP_2G;
-		core_writel(priv, reg, offset);
-
 		/* Enable Broadcast, Multicast, Unicast forwarding to IMP port */
 		reg = core_readl(priv, CORE_IMP_CTL);
 		reg |= (RX_BCST_EN | RX_MCST_EN | RX_UCST_EN);
@@ -826,12 +815,10 @@ static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port,
 	if (priv->wol_ports_mask & BIT(port))
 		return;
 
-	if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
-		offset = bcm_sf2_port_override_offset(priv, port);
-		reg = core_readl(priv, offset);
-		reg &= ~LINK_STS;
-		core_writel(priv, reg, offset);
-	}
+	offset = bcm_sf2_port_override_offset(priv, port);
+	reg = core_readl(priv, offset);
+	reg &= ~LINK_STS;
+	core_writel(priv, reg, offset);
 
 	bcm_sf2_sw_mac_link_set(ds, port, interface, false);
 }
@@ -845,51 +832,55 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	struct ethtool_eee *p = &priv->dev->ports[port].eee;
+	u32 reg_rgmii_ctrl = 0;
+	u32 reg, offset;
 
 	bcm_sf2_sw_mac_link_set(ds, port, interface, true);
 
-	if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
-		u32 reg_rgmii_ctrl = 0;
-		u32 reg, offset;
+	offset = bcm_sf2_port_override_offset(priv, port);
 
-		offset = bcm_sf2_port_override_offset(priv, port);
+	if (phy_interface_mode_is_rgmii(interface) ||
+	    interface == PHY_INTERFACE_MODE_MII ||
+	    interface == PHY_INTERFACE_MODE_REVMII) {
+		reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
+		reg = reg_readl(priv, reg_rgmii_ctrl);
+		reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);
 
-		if (interface == PHY_INTERFACE_MODE_RGMII ||
-		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-		    interface == PHY_INTERFACE_MODE_MII ||
-		    interface == PHY_INTERFACE_MODE_REVMII) {
-			reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
-			reg = reg_readl(priv, reg_rgmii_ctrl);
-			reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);
+		if (tx_pause)
+			reg |= TX_PAUSE_EN;
+		if (rx_pause)
+			reg |= RX_PAUSE_EN;
 
-			if (tx_pause)
-				reg |= TX_PAUSE_EN;
-			if (rx_pause)
-				reg |= RX_PAUSE_EN;
+		reg_writel(priv, reg, reg_rgmii_ctrl);
+	}
 
-			reg_writel(priv, reg, reg_rgmii_ctrl);
-		}
+	reg = LINK_STS;
+	if (port == 8) {
+		if (priv->type == BCM4908_DEVICE_ID)
+			reg |= GMII_SPEED_UP_2G;
+		reg |= MII_SW_OR;
+	} else {
+		reg |= SW_OVERRIDE;
+	}
 
-		reg = SW_OVERRIDE | LINK_STS;
-		switch (speed) {
-		case SPEED_1000:
-			reg |= SPDSTS_1000 << SPEED_SHIFT;
-			break;
-		case SPEED_100:
-			reg |= SPDSTS_100 << SPEED_SHIFT;
-			break;
-		}
+	switch (speed) {
+	case SPEED_1000:
+		reg |= SPDSTS_1000 << SPEED_SHIFT;
+		break;
+	case SPEED_100:
+		reg |= SPDSTS_100 << SPEED_SHIFT;
+		break;
+	}
 
-		if (duplex == DUPLEX_FULL)
-			reg |= DUPLX_MODE;
+	if (duplex == DUPLEX_FULL)
+		reg |= DUPLX_MODE;
 
-		if (tx_pause)
-			reg |= TXFLOW_CNTL;
-		if (rx_pause)
-			reg |= RXFLOW_CNTL;
+	if (tx_pause)
+		reg |= TXFLOW_CNTL;
+	if (rx_pause)
+		reg |= RXFLOW_CNTL;
 
-		core_writel(priv, reg, offset);
-	}
+	core_writel(priv, reg, offset);
 
 	if (mode == MLO_AN_PHY && phydev)
 		p->eee_enabled = b53_eee_init(ds, port, phydev);
-- 
2.25.1

Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Vladimir Oltean]

Hi Florian,

On Mon, Jul 25, 2022 at 02:49:40PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> Although this should work for all devices, since most DTBs on the
> platforms where bcm_sf2 is use do not populate a 'fixed-link' property
> for their CPU ports, but rely on the Ethernet controller DT node doing
> that, we will not be registering a 'fixed-link' instance for CPU ports.
> 
> This still works because the switch matches the configuration of the
> Ethernet controller, but on BCM4908 where we want to force 2GBits/sec,
> that I cannot test, not so sure.
> 
> So as of now, this series does not produce register for register
> compatile changes.

My understanding of this change set is that it stops overriding the link
status of IMP ports from dsa_switch_ops :: setup (unconditionally) and
it moves it to phylink_mac_link_up(). But the latter may not be called
when you lack a fixed-link, and yet the IMP port(s) still work(s).

This begs the natural question, is overriding the link status ever needed?
It was done since the initial commit for this driver, 246d7f773c13c.

Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Florian Fainelli]

On 7/26/22 04:23, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Mon, Jul 25, 2022 at 02:49:40PM -0700, Florian Fainelli wrote:
>> Hi all,
>>
>> Although this should work for all devices, since most DTBs on the
>> platforms where bcm_sf2 is use do not populate a 'fixed-link' property
>> for their CPU ports, but rely on the Ethernet controller DT node doing
>> that, we will not be registering a 'fixed-link' instance for CPU ports.
>>
>> This still works because the switch matches the configuration of the
>> Ethernet controller, but on BCM4908 where we want to force 2GBits/sec,
>> that I cannot test, not so sure.
>>
>> So as of now, this series does not produce register for register
>> compatile changes.
> 
> My understanding of this change set is that it stops overriding the link
> status of IMP ports from dsa_switch_ops :: setup (unconditionally) and
> it moves it to phylink_mac_link_up(). But the latter may not be called
> when you lack a fixed-link, and yet the IMP port(s) still work(s).
> 
> This begs the natural question, is overriding the link status ever needed?

It was until we started to unconditionally reset the switch using the "external" reset method as opposed to the "internal" reset method which turned out not to be functional:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eee87e4377a4b86dc2eea0ade162b0dc33f40576

At any rate (no pun intended), 4908 will want a 2GBit/sec IMP port to be set-up and we have no way to do that other than by forcing that setting, either through the bcm_sf2_imp_setup() method or via a hack to the mac_link_up() callback. This is kind of orthogonal in the sense that there is no "official" support for speed 2000 mbits/sec anyway in the emulated SW PHY, PHYLINK or anywhere in between but if we want to fully transition over to PHYLINK to configure all ports, which is absolutely the goal, we will need to find a solution one way or another.

I would prefer if also we sort of "transferred" the 'fixed-link' parameters from the DSA Ethernet controller attached to the CPU port onto the PHYLINK instance of the CPU port in the switch as they ought to be strictly identical otherwise it just won't work. This would ensure that we continue to force the link and it would make me sleep better a night to know that the IMP port is operating strictly the same way it was. My script compares register values before/after for the registers that are static and this was flagged as a difference.
-- 
Florian

Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Vladimir Oltean]

On Tue, Jul 26, 2022 at 09:14:17AM -0700, Florian Fainelli wrote:
> > This begs the natural question, is overriding the link status ever needed?
> 
> It was until we started to unconditionally reset the switch using the
> "external" reset method as opposed to the "internal" reset method
> which turned out not to be functional:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eee87e4377a4b86dc2eea0ade162b0dc33f40576

Ok, I see.

> At any rate (no pun intended), 4908 will want a 2GBit/sec IMP port to
> be set-up and we have no way to do that other than by forcing that
> setting, either through the bcm_sf2_imp_setup() method or via a hack
> to the mac_link_up() callback. This is kind of orthogonal in the sense
> that there is no "official" support for speed 2000 mbits/sec anyway in
> the emulated SW PHY, PHYLINK or anywhere in between but if we want to
> fully transition over to PHYLINK to configure all ports, which is
> absolutely the goal, we will need to find a solution one way or
> another.

So I made some tests with speed = <2000>; in the device tree and in a
way I'm more confused than when I started. I was expecting phylink_validate()
to somehow fail but this isn't at all what happened. Instead everything
seems to work just fine, minus some ergonomic details (some prints).

So in the case of a fixed-link, phylink_validate() is actually called
twice, once directly from phylink_create() and once almost immediately
afterwards from phylink_parse_fixedlink(). Both validations are of the
inquisitive kind rather than the confrontational kind, i.e. their return
value isn't checked, and "pl->supported"/"pl->link_config.advertising"
are initially filled by phylink with all ones, in order for the driver
to reduce this to all link mode bits that are supported.
Minor side note, this second validation done during fixed-link parsing
is redundant IMO, because nothing relevant inside the arguments that we
pass to pl->mac_ops->validate() will have changed in any way between the
calls.

Anyway, if phylink_validate() is never going to confront us about the
pl->supported link mode mask becoming zero, you might wonder why it
calls even inquisitively in the first place.

Essentially phylink_parse_fixedlink() just wants to print in case it's
using a link speed that isn't supported by the driver. To do that, it
calls phy_lookup_setting() where one of the arguments is pl->supported
itself. But in our case, there is no link mode for speed 2000, although
that shouldn't matter, since no Ethernet PHY sees or needs to advertise
this speed, so phy_lookup_setting() finds nothing. I suspect this is
largely due to historical reasons, where the link modes were the common
denominator at the level of the driver visible phylink_validate() API.
Today we may simply extend config->mac_capabilities and forgo adding
bogus link modes just for this to work.

Curiously, even if we go to the extra lengths of silencing phylink's
"fixed link not recognised" warning, nothing seems to be broken even if
we don't do that.

Immediately after pl->supported has been populated by the inquisitive
phylink_validate(), phylink clears it (which means that the pl->supported
variable used above could have very well been just a temporary on-stack
variable), and just populates some fields.
Namely the pause fields, and a *single* speed, corresponding to "s"
(what phy_lookup_setting() found).

	linkmode_zero(pl->supported);
	phylink_set(pl->supported, MII);
	phylink_set(pl->supported, Pause);
	phylink_set(pl->supported, Asym_Pause);
	phylink_set(pl->supported, Autoneg);
	if (s) {
		__set_bit(s->bit, pl->supported);
		__set_bit(s->bit, pl->link_config.lp_advertising);
	} else {
		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
			     pl->link_config.speed);
	}

Why phylink even bothers to keep the speed-related linkmode in
pl->supported, if it won't use it anywhere further, I can't answer.
I can even delete the "if (s) ... else ..." block altogether and nothing
seems to be adversely impacted.

In any case, the short version of the code walkthrough is that phylink
can apparently operate in fixed-link mode with a pl->supported and
pl->link_config.lp_advertising mask of link modes that doesn't contain
any speed, and this won't generate any error, although I'm not completely
sure it was intended either.

> I would prefer if also we sort of "transferred" the 'fixed-link'
> parameters from the DSA Ethernet controller attached to the CPU port
> onto the PHYLINK instance of the CPU port in the switch as they ought
> to be strictly identical otherwise it just won't work. This would
> ensure that we continue to force the link and it would make me sleep
> better a night to know that the IMP port is operating strictly the
> same way it was. My script compares register values before/after for
> the registers that are static and this was flagged as a difference.

There are several problems with transferring the parameters. Most
obvious derives from what we discussed about speed = <2000> just above:
the DSA master won't have it, either, because it's a non-standard speed.
Additionally, the DSA master may be missing the phy-mode too.

Second has to do with how we transfer the phy-mode assuming it isn't
missing on the master. RGMII modes are clearly problematic precisely
because we have so many driver interpretations of what they mean.
But "mii" and "rmii" aren't all that clear-cut either. Do we translate
into "mii" and "rmii" for DSA, or "rev-mii" and "rev-rmii"?
bcm_sf2 understands "rev-mii", but mv88e6xxx doesn't.

Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Florian Fainelli]

On 7/26/2022 6:29 PM, Vladimir Oltean wrote:
> On Tue, Jul 26, 2022 at 09:14:17AM -0700, Florian Fainelli wrote:
>>> This begs the natural question, is overriding the link status ever needed?
>>
>> It was until we started to unconditionally reset the switch using the
>> "external" reset method as opposed to the "internal" reset method
>> which turned out not to be functional:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eee87e4377a4b86dc2eea0ade162b0dc33f40576
> 
> Ok, I see.
> 
>> At any rate (no pun intended), 4908 will want a 2GBit/sec IMP port to
>> be set-up and we have no way to do that other than by forcing that
>> setting, either through the bcm_sf2_imp_setup() method or via a hack
>> to the mac_link_up() callback. This is kind of orthogonal in the sense
>> that there is no "official" support for speed 2000 mbits/sec anyway in
>> the emulated SW PHY, PHYLINK or anywhere in between but if we want to
>> fully transition over to PHYLINK to configure all ports, which is
>> absolutely the goal, we will need to find a solution one way or
>> another.
> 
> So I made some tests with speed = <2000>; in the device tree and in a
> way I'm more confused than when I started. I was expecting phylink_validate()
> to somehow fail but this isn't at all what happened. Instead everything
> seems to work just fine, minus some ergonomic details (some prints).
> 
> So in the case of a fixed-link, phylink_validate() is actually called
> twice, once directly from phylink_create() and once almost immediately
> afterwards from phylink_parse_fixedlink(). Both validations are of the
> inquisitive kind rather than the confrontational kind, i.e. their return
> value isn't checked, and "pl->supported"/"pl->link_config.advertising"
> are initially filled by phylink with all ones, in order for the driver
> to reduce this to all link mode bits that are supported.
> Minor side note, this second validation done during fixed-link parsing
> is redundant IMO, because nothing relevant inside the arguments that we
> pass to pl->mac_ops->validate() will have changed in any way between the
> calls.
> 
> Anyway, if phylink_validate() is never going to confront us about the
> pl->supported link mode mask becoming zero, you might wonder why it
> calls even inquisitively in the first place.
> 
> Essentially phylink_parse_fixedlink() just wants to print in case it's
> using a link speed that isn't supported by the driver. To do that, it
> calls phy_lookup_setting() where one of the arguments is pl->supported
> itself. But in our case, there is no link mode for speed 2000, although
> that shouldn't matter, since no Ethernet PHY sees or needs to advertise
> this speed, so phy_lookup_setting() finds nothing. I suspect this is
> largely due to historical reasons, where the link modes were the common
> denominator at the level of the driver visible phylink_validate() API.
> Today we may simply extend config->mac_capabilities and forgo adding
> bogus link modes just for this to work.
> 
> Curiously, even if we go to the extra lengths of silencing phylink's
> "fixed link not recognised" warning, nothing seems to be broken even if
> we don't do that.
> 
> Immediately after pl->supported has been populated by the inquisitive
> phylink_validate(), phylink clears it (which means that the pl->supported
> variable used above could have very well been just a temporary on-stack
> variable), and just populates some fields.
> Namely the pause fields, and a *single* speed, corresponding to "s"
> (what phy_lookup_setting() found).
> 
> 	linkmode_zero(pl->supported);
> 	phylink_set(pl->supported, MII);
> 	phylink_set(pl->supported, Pause);
> 	phylink_set(pl->supported, Asym_Pause);
> 	phylink_set(pl->supported, Autoneg);
> 	if (s) {
> 		__set_bit(s->bit, pl->supported);
> 		__set_bit(s->bit, pl->link_config.lp_advertising);
> 	} else {
> 		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
> 			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
> 			     pl->link_config.speed);
> 	}
> 
> Why phylink even bothers to keep the speed-related linkmode in
> pl->supported, if it won't use it anywhere further, I can't answer.
> I can even delete the "if (s) ... else ..." block altogether and nothing
> seems to be adversely impacted.
> 
> In any case, the short version of the code walkthrough is that phylink
> can apparently operate in fixed-link mode with a pl->supported and
> pl->link_config.lp_advertising mask of link modes that doesn't contain
> any speed, and this won't generate any error, although I'm not completely
> sure it was intended either.

OK, well maybe we need to syszbot the crap out of PHYLINK at some point, 
kunit anyone?

> 
>> I would prefer if also we sort of "transferred" the 'fixed-link'
>> parameters from the DSA Ethernet controller attached to the CPU port
>> onto the PHYLINK instance of the CPU port in the switch as they ought
>> to be strictly identical otherwise it just won't work. This would
>> ensure that we continue to force the link and it would make me sleep
>> better a night to know that the IMP port is operating strictly the
>> same way it was. My script compares register values before/after for
>> the registers that are static and this was flagged as a difference.
> 
> There are several problems with transferring the parameters. Most
> obvious derives from what we discussed about speed = <2000> just above:
> the DSA master won't have it, either, because it's a non-standard speed.
> Additionally, the DSA master may be missing the phy-mode too.
> 
> Second has to do with how we transfer the phy-mode assuming it isn't
> missing on the master. RGMII modes are clearly problematic precisely
> because we have so many driver interpretations of what they mean.
> But "mii" and "rmii" aren't all that clear-cut either. Do we translate
> into "mii" and "rmii" for DSA, or "rev-mii" and "rev-rmii"?
> bcm_sf2 understands "rev-mii", but mv88e6xxx doesn't.

Yep, you have me convinced. I suppose the course of action for me is to 
update the DTSes to also include a fixed-link property and phy-mode 
property in the CPU node, even if that duplicates what the Ethernet 
controller node already has, and then given a cycle or two, merge this 
patch series.
-- 
Florian

Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all ports

[Vladimir Oltean]

On Tue, Jul 26, 2022 at 07:17:24PM -0700, Florian Fainelli wrote:
> > > I would prefer if also we sort of "transferred" the 'fixed-link'
> > > parameters from the DSA Ethernet controller attached to the CPU port
> > > onto the PHYLINK instance of the CPU port in the switch as they ought
> > > to be strictly identical otherwise it just won't work. This would
> > > ensure that we continue to force the link and it would make me sleep
> > > better a night to know that the IMP port is operating strictly the
> > > same way it was. My script compares register values before/after for
> > > the registers that are static and this was flagged as a difference.
> > 
> > There are several problems with transferring the parameters. Most
> > obvious derives from what we discussed about speed = <2000> just above:
> > the DSA master won't have it, either, because it's a non-standard speed.
> > Additionally, the DSA master may be missing the phy-mode too.
> > 
> > Second has to do with how we transfer the phy-mode assuming it isn't
> > missing on the master. RGMII modes are clearly problematic precisely
> > because we have so many driver interpretations of what they mean.
> > But "mii" and "rmii" aren't all that clear-cut either. Do we translate
> > into "mii" and "rmii" for DSA, or "rev-mii" and "rev-rmii"?
> > bcm_sf2 understands "rev-mii", but mv88e6xxx doesn't.
> 
> Yep, you have me convinced. I suppose the course of action for me is to
> update the DTSes to also include a fixed-link property and phy-mode property
> in the CPU node, even if that duplicates what the Ethernet controller node
> already has, and then given a cycle or two, merge this patch series.

I don't want to say that the 'peeking at the master' technique can't be
used at all, maybe if it would only come down to internal ports, and you
could give me a guarantee that the master does have a valid DT description
for your SoCs, we could consider doing this as a transition thing for
bcm_sf2 (on non-4908, see more below).

Having said that, I looked again at my analysis for bcm_sf2:
https://patchwork.kernel.org/project/netdevbpf/patch/20220723164635.1621911-1-vladimir.oltean@nxp.com/

and I notice that I said that arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi
*does* have a valid (complete at least) OF node description for the CPU port:

	port@8 {
		reg = <8>;
		phy-mode = "internal";
		ethernet = <&enet>;

		fixed-link {
			speed = <1000>;
			full-duplex;
		};
	};

but I didn't have the information that the speed is wrong (you said you
want it to operate at 2G).

Additionally (and curiously), the "enet" node lacks any link
description:

	enet: ethernet@2000 {
		compatible = "brcm,bcm4908-enet";
		reg = <0x2000 0x1000>;

		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "rx", "tx";
	};

My question is, do you use the DTS file as seen in the mainline kernel
at all, or is it just for some sort of reference?

In case the 4908 really does have a fixed-link at 1000 provided by the
bootloader, you should be free to move the IMP setup code to
phylink_mac_link_up(), because phylink does have what it needs to run,
and just hack it to 2G for this SoC due to driver level knowledge,
despite what phylink thinks is going on. This can take place indefinitely.
In the long term, I don't know what else to suggest beyond fixing the
device tree to report the proper speed, sorry.