[PATCH net-next 0/4] Remove phylink_validate() from Felix DSA driver

[PATCH net-next 0/4] Remove phylink_validate() from Felix DSA driver

[Vladimir Oltean]

The Felix DSA driver still uses its own phylink_validate() procedure
rather than the (relatively newly introduced) phylink_generic_validate()
because the latter did not cater for the case where a PHY provides rate
matching between the Ethernet cable side speed and the SERDES side
speed (and does not advertise other speeds except for the SERDES speed).

This changed with Sean Anderson's generic support for rate matching PHYs
in phylib and phylink:
https://patchwork.kernel.org/project/netdevbpf/cover/20220920221235.1487501-1-sean.anderson@seco.com/

Building upon that support, this patch set makes Linux understand that
the PHYs used in combination with the Felix DSA driver (SCH-30841 riser
card with AQR412 PHY, used with SERDES protocol 0x7777 - 4x2500base-x,
plugged into LS1028A-QDS) do support PAUSE rate matching. This requires
Aquantia PHY driver support for new PHY IDs.

To activate the rate matching support in phylink, config->mac_capabilities
must be populated. Coincidentally, this also opts the Felix driver into
the generic phylink validation.

Next, code that is no longer necessary is eliminated. This includes the
Felix driver validation procedures for VSC9959 and VSC9953, the
workaround in the Ocelot switch library to leave RX flow control always
enabled, as well as DSA plumbing necessary for a custom phylink
validation procedure to be propagated to the hardware driver level.

Many thanks go to Sean Anderson for providing generic support for rate
matching.

Vladimir Oltean (4):
  net: phy: aquantia: add AQR112 and AQR412 PHY IDs
  net: dsa: felix: use phylink_generic_validate()
  net: mscc: ocelot: drop workaround for forcing RX flow control
  net: dsa: remove phylink_validate() method

 drivers/net/dsa/ocelot/felix.c           | 16 +++-------
 drivers/net/dsa/ocelot/felix.h           |  3 --
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 30 ------------------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 27 ----------------
 drivers/net/ethernet/mscc/ocelot.c       |  6 ++--
 drivers/net/phy/aquantia_main.c          | 40 ++++++++++++++++++++++++
 include/net/dsa.h                        |  3 --
 net/dsa/port.c                           | 18 +----------
 8 files changed, 47 insertions(+), 96 deletions(-)

-- 
2.34.1

[PATCH net-next 1/4] net: phy: aquantia: add AQR112 and AQR412 PHY IDs

[Vladimir Oltean]

These are Gen3 Aquantia N-BASET PHYs which support 5GBASE-T,
2.5GBASE-T, 1000BASE-T and 100BASE-TX (not 10G); also EEE, Sync-E,
PTP, PoE.

The 112 is a single PHY package, the 412 is a quad PHY package.

The system-side SERDES interface of these PHYs selects its protocol
depending on the negotiated media side link speed. That protocol can be
1000BASE-X, 2500BASE-X, 10GBASE-R, SGMII, USXGMII.

The configuration of which SERDES protocol to use for which link speed
is made by firmware; even though it could be overwritten over MDIO by
Linux, we assume that the firmware provisioning is ok for the board on
which the driver probes.

For cases when the system side runs at a fixed rate, we want phylib/phylink
to detect the PAUSE rate matching ability of these PHYs, so we need to
use the Aquantia rather than the generic C45 driver. This needs
aqr107_read_status() -> aqr107_read_rate() to set phydev->rate_matching,
as well as the aqr107_get_rate_matching() method.

I am a bit unsure about the naming convention in the driver. Since
AQR107 is a Gen2 PHY, I assume all functions prefixed with "aqr107_"
rather than "aqr_" mean Gen2+ features. So I've reused this naming
convention.

I've tested PHY "SGMII" statistics as well as the .link_change_notify
method, which prints:

Aquantia AQR412 mdio_mux-0.4:00: Link partner is Aquantia PHY, FW 4.3, fast-retrain downshift advertised, fast reframe advertised

Tested SERDES protocols are usxgmii and 2500base-x (the latter with
PAUSE rate matching). Tested link modes are 100/1000/2500 Base-T
(with Aquantia link partner and with other link partners). No notable
events observed.

The placement of these PHY IDs in the driver is right before AQR113C,
a Gen4 PHY.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/aquantia_main.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 47a76df36b74..334a6904ca5a 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -22,6 +22,8 @@
 #define PHY_ID_AQR107	0x03a1b4e0
 #define PHY_ID_AQCS109	0x03a1b5c2
 #define PHY_ID_AQR405	0x03a1b4b0
+#define PHY_ID_AQR112	0x03a1b662
+#define PHY_ID_AQR412	0x03a1b712
 #define PHY_ID_AQR113C	0x31c31c12
 
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
@@ -800,6 +802,42 @@ static struct phy_driver aqr_driver[] = {
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
+{
+	PHY_ID_MATCH_MODEL(PHY_ID_AQR112),
+	.name		= "Aquantia AQR112",
+	.probe		= aqr107_probe,
+	.config_aneg    = aqr_config_aneg,
+	.config_intr	= aqr_config_intr,
+	.handle_interrupt = aqr_handle_interrupt,
+	.get_tunable    = aqr107_get_tunable,
+	.set_tunable    = aqr107_set_tunable,
+	.suspend	= aqr107_suspend,
+	.resume		= aqr107_resume,
+	.read_status	= aqr107_read_status,
+	.get_rate_matching = aqr107_get_rate_matching,
+	.get_sset_count = aqr107_get_sset_count,
+	.get_strings	= aqr107_get_strings,
+	.get_stats	= aqr107_get_stats,
+	.link_change_notify = aqr107_link_change_notify,
+},
+{
+	PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
+	.name		= "Aquantia AQR412",
+	.probe		= aqr107_probe,
+	.config_aneg    = aqr_config_aneg,
+	.config_intr	= aqr_config_intr,
+	.handle_interrupt = aqr_handle_interrupt,
+	.get_tunable    = aqr107_get_tunable,
+	.set_tunable    = aqr107_set_tunable,
+	.suspend	= aqr107_suspend,
+	.resume		= aqr107_resume,
+	.read_status	= aqr107_read_status,
+	.get_rate_matching = aqr107_get_rate_matching,
+	.get_sset_count = aqr107_get_sset_count,
+	.get_strings	= aqr107_get_strings,
+	.get_stats	= aqr107_get_stats,
+	.link_change_notify = aqr107_link_change_notify,
+},
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
 	.name           = "Aquantia AQR113C",
@@ -831,6 +869,8 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = {
 	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR107) },
 	{ PHY_ID_MATCH_MODEL(PHY_ID_AQCS109) },
 	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR405) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR112) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR412) },
 	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) },
 	{ }
 };
-- 
2.34.1

[PATCH net-next 2/4] net: dsa: felix: use phylink_generic_validate()

[Vladimir Oltean]

Drop the custom implementation of phylink_validate() in favor of the
generic one, which requires config->mac_capabilities to be set.

This was used up until now because of the possibility of being paired
with Aquantia PHYs with support for rate matching. The phylink framework
gained generic support for these, and knows to advertise all 10/100/1000
lower speed link modes when our SERDES protocol is 2500base-x
(fixed speed).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c           | 16 ++++---------
 drivers/net/dsa/ocelot/felix.h           |  3 ---
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 30 ------------------------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 27 ---------------------
 4 files changed, 4 insertions(+), 72 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index dd3a18cc89dd..44e160f32067 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1048,21 +1048,14 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
 	 */
 	config->legacy_pre_march2020 = false;
 
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+				   MAC_10 | MAC_100 | MAC_1000FD |
+				   MAC_2500FD;
+
 	__set_bit(ocelot->ports[port]->phy_mode,
 		  config->supported_interfaces);
 }
 
-static void felix_phylink_validate(struct dsa_switch *ds, int port,
-				   unsigned long *supported,
-				   struct phylink_link_state *state)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct felix *felix = ocelot_to_felix(ocelot);
-
-	if (felix->info->phylink_validate)
-		felix->info->phylink_validate(ocelot, port, supported, state);
-}
-
 static struct phylink_pcs *felix_phylink_mac_select_pcs(struct dsa_switch *ds,
 							int port,
 							phy_interface_t iface)
@@ -2050,7 +2043,6 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.get_sset_count			= felix_get_sset_count,
 	.get_ts_info			= felix_get_ts_info,
 	.phylink_get_caps		= felix_phylink_get_caps,
-	.phylink_validate		= felix_phylink_validate,
 	.phylink_mac_select_pcs		= felix_phylink_mac_select_pcs,
 	.phylink_mac_link_down		= felix_phylink_mac_link_down,
 	.phylink_mac_link_up		= felix_phylink_mac_link_up,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index c9c29999c336..42338116eed0 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -52,9 +52,6 @@ struct felix_info {
 
 	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
-	void	(*phylink_validate)(struct ocelot *ocelot, int port,
-				    unsigned long *supported,
-				    struct phylink_link_state *state);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
 				 enum tc_setup_type type, void *type_data);
 	void	(*tas_guard_bands_update)(struct ocelot *ocelot, int port);
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 26a35ae322d1..b0ae8d6156f6 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -885,35 +885,6 @@ static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9959_phylink_validate(struct ocelot *ocelot, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
-{
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 1000baseT_Half);
-	phylink_set(mask, 1000baseT_Full);
-	phylink_set(mask, 1000baseX_Full);
-
-	if (state->interface == PHY_INTERFACE_MODE_INTERNAL ||
-	    state->interface == PHY_INTERFACE_MODE_2500BASEX ||
-	    state->interface == PHY_INTERFACE_MODE_USXGMII) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
-	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-}
-
 /* Watermark encode
  * Bit 8:   Unit; 0:1, 1:16
  * Bit 7-0: Value to be multiplied with unit
@@ -2588,7 +2559,6 @@ static const struct felix_info felix_info_vsc9959 = {
 	.ptp_caps		= &vsc9959_ptp_caps,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
-	.phylink_validate	= vsc9959_phylink_validate,
 	.port_modes		= vsc9959_port_modes,
 	.port_setup_tc		= vsc9959_port_setup_tc,
 	.port_sched_speed_set	= vsc9959_sched_speed_set,
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 7af33b2c685d..6500c1697dd6 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -840,32 +840,6 @@ static int vsc9953_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9953_phylink_validate(struct ocelot *ocelot, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
-{
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 1000baseT_Full);
-	phylink_set(mask, 1000baseX_Full);
-
-	if (state->interface == PHY_INTERFACE_MODE_INTERNAL) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
-	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-}
-
 /* Watermark encode
  * Bit 9:   Unit; 0:1, 1:16
  * Bit 8-0: Value to be multiplied with unit
@@ -1007,7 +981,6 @@ static const struct felix_info seville_info_vsc9953 = {
 	.num_tx_queues		= OCELOT_NUM_TC,
 	.mdio_bus_alloc		= vsc9953_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9953_mdio_bus_free,
-	.phylink_validate	= vsc9953_phylink_validate,
 	.port_modes		= vsc9953_port_modes,
 };
 
-- 
2.34.1

[PATCH net-next 3/4] net: mscc: ocelot: drop workaround for forcing RX flow control

[Vladimir Oltean]

As phylink gained generic support for PHYs with rate matching via PAUSE
frames, the phylink_mac_link_up() method will be called with the maximum
speed and with rx_pause=true if rate matching is in use. This means that
setups with 2500base-x as the SERDES protocol between the MAC/PCS and
the PHY now work with no need for the driver to do anything special.

Tested with fsl-ls1028a-qds-7777.dts.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 13b14110a060..da56f9bfeaf0 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -872,10 +872,8 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
 		return;
 	}
 
-	/* Handle RX pause in all cases, with 2500base-X this is used for rate
-	 * adaptation.
-	 */
-	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
+	if (rx_pause)
+		mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
 
 	if (tx_pause)
 		mac_fc_cfg |= SYS_MAC_FC_CFG_TX_FC_ENA |
-- 
2.34.1

[PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

As of now, all DSA drivers use phylink_generic_validate() and there is
no known use case remaining for a driver-specific link mode validation
procedure. As such, remove this DSA operation and let phylink determine
what is supported based on config->mac_capabilities, which all drivers
provide.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Not all DSA drivers provide config->mac_capabilities, for example
mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
those drivers on recent kernels and no one reported that they fail to
establish a link, so I'm guessing that they work (somehow). But I must
admit I don't understand why phylink_generic_validate() works when
mac_capabilities=0. Anyway, these drivers did not provide a
phylink_validate() method before and do not provide one now, so nothing
changes for them.

 include/net/dsa.h |  3 ---
 net/dsa/port.c    | 18 +-----------------
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index ee369670e20e..dde364688739 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -880,9 +880,6 @@ struct dsa_switch_ops {
 	 */
 	void	(*phylink_get_caps)(struct dsa_switch *ds, int port,
 				    struct phylink_config *config);
-	void	(*phylink_validate)(struct dsa_switch *ds, int port,
-				    unsigned long *supported,
-				    struct phylink_link_state *state);
 	struct phylink_pcs *(*phylink_mac_select_pcs)(struct dsa_switch *ds,
 						      int port,
 						      phy_interface_t iface);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 208168276995..6e417cdcdb7b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1532,22 +1532,6 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
 	return phydev;
 }
 
-static void dsa_port_phylink_validate(struct phylink_config *config,
-				      unsigned long *supported,
-				      struct phylink_link_state *state)
-{
-	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_validate) {
-		if (config->mac_capabilities)
-			phylink_generic_validate(config, supported, state);
-		return;
-	}
-
-	ds->ops->phylink_validate(ds, dp->index, supported, state);
-}
-
 static void dsa_port_phylink_mac_pcs_get_state(struct phylink_config *config,
 					       struct phylink_link_state *state)
 {
@@ -1648,7 +1632,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
 }
 
 static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
-	.validate = dsa_port_phylink_validate,
+	.validate = phylink_generic_validate,
 	.mac_select_pcs = dsa_port_phylink_mac_select_pcs,
 	.mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state,
 	.mac_config = dsa_port_phylink_mac_config,
-- 
2.34.1

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> Not all DSA drivers provide config->mac_capabilities, for example
> mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> those drivers on recent kernels and no one reported that they fail to
> establish a link, so I'm guessing that they work (somehow). But I must
> admit I don't understand why phylink_generic_validate() works when
> mac_capabilities=0. Anyway, these drivers did not provide a
> phylink_validate() method before and do not provide one now, so nothing
> changes for them.

> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 208168276995..6e417cdcdb7b 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -1532,22 +1532,6 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
>  	return phydev;
>  }
>  
> -static void dsa_port_phylink_validate(struct phylink_config *config,
> -				      unsigned long *supported,
> -				      struct phylink_link_state *state)
> -{
> -	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	if (!ds->ops->phylink_validate) {
> -		if (config->mac_capabilities)
> -			phylink_generic_validate(config, supported, state);

Ah, I think I was reading the code wrong, now I think I understand how
phylink_generic_validate() works when config->mac_capabilities=0:
it doesn't; it just wasn't called by the old code.

It will be called after my patch though, so this will break things. I
guess we'll still need a "custom" phylink_validate() which does
something approximating this?

static void dsa_port_phylink_validate(struct phylink_config *config,
				      unsigned long *supported,
				      struct phylink_link_state *state)
{
	if (config->mac_capabilities) // avoid breaking drivers which don't set this
		phylink_generic_validate(config, supported, state);
}

or what else would be preferred?

> -		return;
> -	}
> -
> -	ds->ops->phylink_validate(ds, dp->index, supported, state);
> -}

In any case, please don't merge this as-is, and sorry for realizing the
breakage after the fact.

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> Not all DSA drivers provide config->mac_capabilities, for example
> mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> those drivers on recent kernels and no one reported that they fail to
> establish a link, so I'm guessing that they work (somehow). But I must
> admit I don't understand why phylink_generic_validate() works when
> mac_capabilities=0. Anyway, these drivers did not provide a
> phylink_validate() method before and do not provide one now, so nothing
> changes for them.

There is a specific exception:

static void dsa_port_phylink_validate(struct phylink_config *config,
                                      unsigned long *supported,
                                      struct phylink_link_state *state)
{
        struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
        struct dsa_switch *ds = dp->ds;

        if (!ds->ops->phylink_validate) {
                if (config->mac_capabilities)
                        phylink_generic_validate(config, supported, state);
                return;

When config->mac_capabilities is zero, and there is no phylink_validate()
function, dsa_port_phylink_validate() becomes a no-op, and the no-op
case basically means "everything is allowed", which is how things worked
before the generic validation was added, as you will see from commit
5938bce4b6e2 ("net: dsa: support use of phylink_generic_validate()").

Changing this as you propose below will likely break these drivers.

A safer change would be to elimate ds->ops->phylink_validate, leaving
the call to phylink_generic_validate() conditional on mac_capabilities
having been filled in - which will save breaking these older drivers.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

On Tue, Nov 01, 2022 at 12:01:32PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> > Not all DSA drivers provide config->mac_capabilities, for example
> > mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> > those drivers on recent kernels and no one reported that they fail to
> > establish a link, so I'm guessing that they work (somehow). But I must
> > admit I don't understand why phylink_generic_validate() works when
> > mac_capabilities=0. Anyway, these drivers did not provide a
> > phylink_validate() method before and do not provide one now, so nothing
> > changes for them.
> 
> There is a specific exception:
> 
> When config->mac_capabilities is zero, and there is no phylink_validate()
> function, dsa_port_phylink_validate() becomes a no-op, and the no-op
> case basically means "everything is allowed", which is how things worked
> before the generic validation was added, as you will see from commit
> 5938bce4b6e2 ("net: dsa: support use of phylink_generic_validate()").
> 
> Changing this as you propose below will likely break these drivers.
> 
> A safer change would be to elimate ds->ops->phylink_validate, leaving
> the call to phylink_generic_validate() conditional on mac_capabilities
> having been filled in - which will save breaking these older drivers.

Yes, this is correct, thanks; our emails crossed.

Between keeping a no-op phylink_validate() for these drivers and filling
in mac_capabilities for them, to remove this extra code path in DSA,
what would be preferred?

The 3 drivers I mentioned could all get a blanket MAC_10 | MAC_100 |
MAC1000FD | MAC_ASYM_PAUSE | MAC_SYM_PAUSE to keep advertising what they
did, even if this may or may not be 100% correct (lan9303 and mv88e6060
are not gigabit, and I don't know if they do flow control properly), but
these issues are not new.

Re: [PATCH net-next 1/4] net: phy: aquantia: add AQR112 and AQR412 PHY IDs

[Sean Anderson]

On 11/1/22 07:48, Vladimir Oltean wrote:
> These are Gen3 Aquantia N-BASET PHYs which support 5GBASE-T,
> 2.5GBASE-T, 1000BASE-T and 100BASE-TX (not 10G); also EEE, Sync-E,
> PTP, PoE.
> 
> The 112 is a single PHY package, the 412 is a quad PHY package.
> 
> The system-side SERDES interface of these PHYs selects its protocol
> depending on the negotiated media side link speed. That protocol can be
> 1000BASE-X, 2500BASE-X, 10GBASE-R, SGMII, USXGMII.
> 
> The configuration of which SERDES protocol to use for which link speed
> is made by firmware; even though it could be overwritten over MDIO by
> Linux, we assume that the firmware provisioning is ok for the board on
> which the driver probes.
> 
> For cases when the system side runs at a fixed rate, we want phylib/phylink
> to detect the PAUSE rate matching ability of these PHYs, so we need to
> use the Aquantia rather than the generic C45 driver. This needs
> aqr107_read_status() -> aqr107_read_rate() to set phydev->rate_matching,
> as well as the aqr107_get_rate_matching() method.
> 
> I am a bit unsure about the naming convention in the driver. Since
> AQR107 is a Gen2 PHY, I assume all functions prefixed with "aqr107_"
> rather than "aqr_" mean Gen2+ features. So I've reused this naming
> convention.

In Aquantia's BSP there are references to 6 generations of phys (where
the "first" generation is the first 28nm phy; implicitly making the 40nm
phys generation zero). As far as I can tell these are completely
different from the generations of phys you refer to, which seem to me
marketing names. Unfortunately, they don't have a mapping of phys to
generations, so I'm not even sure which phys are which generations. The
datasheets for all but the latest phys seem to have gone missing...

In any case, if it works, then I think it's reasonable to use these
functions.

> I've tested PHY "SGMII" statistics as well as the .link_change_notify
> method, which prints:
> 
> Aquantia AQR412 mdio_mux-0.4:00: Link partner is Aquantia PHY, FW 4.3, fast-retrain downshift advertised, fast reframe advertised
> 
> Tested SERDES protocols are usxgmii and 2500base-x (the latter with
> PAUSE rate matching). Tested link modes are 100/1000/2500 Base-T
> (with Aquantia link partner and with other link partners). No notable
> events observed.
> 
> The placement of these PHY IDs in the driver is right before AQR113C,
> a Gen4 PHY.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/aquantia_main.c | 40 +++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
> index 47a76df36b74..334a6904ca5a 100644
> --- a/drivers/net/phy/aquantia_main.c
> +++ b/drivers/net/phy/aquantia_main.c
> @@ -22,6 +22,8 @@
>  #define PHY_ID_AQR107	0x03a1b4e0
>  #define PHY_ID_AQCS109	0x03a1b5c2
>  #define PHY_ID_AQR405	0x03a1b4b0
> +#define PHY_ID_AQR112	0x03a1b662
> +#define PHY_ID_AQR412	0x03a1b712
>  #define PHY_ID_AQR113C	0x31c31c12
>  
>  #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
> @@ -800,6 +802,42 @@ static struct phy_driver aqr_driver[] = {
>  	.handle_interrupt = aqr_handle_interrupt,
>  	.read_status	= aqr_read_status,
>  },
> +{
> +	PHY_ID_MATCH_MODEL(PHY_ID_AQR112),
> +	.name		= "Aquantia AQR112",
> +	.probe		= aqr107_probe,
> +	.config_aneg    = aqr_config_aneg,
> +	.config_intr	= aqr_config_intr,
> +	.handle_interrupt = aqr_handle_interrupt,
> +	.get_tunable    = aqr107_get_tunable,
> +	.set_tunable    = aqr107_set_tunable,
> +	.suspend	= aqr107_suspend,
> +	.resume		= aqr107_resume,
> +	.read_status	= aqr107_read_status,
> +	.get_rate_matching = aqr107_get_rate_matching,
> +	.get_sset_count = aqr107_get_sset_count,
> +	.get_strings	= aqr107_get_strings,
> +	.get_stats	= aqr107_get_stats,
> +	.link_change_notify = aqr107_link_change_notify,
> +},
> +{
> +	PHY_ID_MATCH_MODEL(PHY_ID_AQR412),
> +	.name		= "Aquantia AQR412",
> +	.probe		= aqr107_probe,
> +	.config_aneg    = aqr_config_aneg,
> +	.config_intr	= aqr_config_intr,
> +	.handle_interrupt = aqr_handle_interrupt,
> +	.get_tunable    = aqr107_get_tunable,
> +	.set_tunable    = aqr107_set_tunable,
> +	.suspend	= aqr107_suspend,
> +	.resume		= aqr107_resume,
> +	.read_status	= aqr107_read_status,
> +	.get_rate_matching = aqr107_get_rate_matching,
> +	.get_sset_count = aqr107_get_sset_count,
> +	.get_strings	= aqr107_get_strings,
> +	.get_stats	= aqr107_get_stats,
> +	.link_change_notify = aqr107_link_change_notify,
> +},
>  {
>  	PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
>  	.name           = "Aquantia AQR113C",
> @@ -831,6 +869,8 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = {
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR107) },
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_AQCS109) },
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR405) },
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR112) },
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR412) },
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) },
>  	{ }
>  };

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Tue, Nov 01, 2022 at 12:40:36PM +0000, Vladimir Oltean wrote:
> On Tue, Nov 01, 2022 at 12:01:32PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> > > Not all DSA drivers provide config->mac_capabilities, for example
> > > mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> > > those drivers on recent kernels and no one reported that they fail to
> > > establish a link, so I'm guessing that they work (somehow). But I must
> > > admit I don't understand why phylink_generic_validate() works when
> > > mac_capabilities=0. Anyway, these drivers did not provide a
> > > phylink_validate() method before and do not provide one now, so nothing
> > > changes for them.
> > 
> > There is a specific exception:
> > 
> > When config->mac_capabilities is zero, and there is no phylink_validate()
> > function, dsa_port_phylink_validate() becomes a no-op, and the no-op
> > case basically means "everything is allowed", which is how things worked
> > before the generic validation was added, as you will see from commit
> > 5938bce4b6e2 ("net: dsa: support use of phylink_generic_validate()").
> > 
> > Changing this as you propose below will likely break these drivers.
> > 
> > A safer change would be to elimate ds->ops->phylink_validate, leaving
> > the call to phylink_generic_validate() conditional on mac_capabilities
> > having been filled in - which will save breaking these older drivers.
> 
> Yes, this is correct, thanks; our emails crossed.
> 
> Between keeping a no-op phylink_validate() for these drivers and filling
> in mac_capabilities for them, to remove this extra code path in DSA,
> what would be preferred?

My stance has always been - if we don't know the answer to a question
that affects a code path in a way we want to, we can't modify that code
path. That said:

> The 3 drivers I mentioned could all get a blanket MAC_10 | MAC_100 |
> MAC1000FD | MAC_ASYM_PAUSE | MAC_SYM_PAUSE to keep advertising what they
> did, even if this may or may not be 100% correct (lan9303 and mv88e6060
> are not gigabit, and I don't know if they do flow control properly), but
> these issues are not new.

Would almost be no different from what we do today. The exception would
be the lack of 1000HD, which I think should be included even though it
isn't common - because currently it will be included.

I also think that if we're going down this route and putting code to set
those capabilities in the DSA drivers, they need to be accompanied with
a comment stating that they are a guess and may not be correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] net: phy: aquantia: add AQR112 and AQR412 PHY IDs

[Vladimir Oltean]

Hi Sean,

On Tue, Nov 01, 2022 at 11:36:23AM -0400, Sean Anderson wrote:
> On 11/1/22 07:48, Vladimir Oltean wrote:
> > These are Gen3 Aquantia N-BASET PHYs which support 5GBASE-T,
> > 2.5GBASE-T, 1000BASE-T and 100BASE-TX (not 10G); also EEE, Sync-E,
> > PTP, PoE.
> > 
> > I am a bit unsure about the naming convention in the driver. Since
> > AQR107 is a Gen2 PHY, I assume all functions prefixed with "aqr107_"
> > rather than "aqr_" mean Gen2+ features. So I've reused this naming
> > convention.
> 
> In Aquantia's BSP there are references to 6 generations of phys (where
> the "first" generation is the first 28nm phy; implicitly making the 40nm
> phys generation zero). As far as I can tell these are completely
> different from the generations of phys you refer to, which seem to me
> marketing names. Unfortunately, they don't have a mapping of phys to
> generations, so I'm not even sure which phys are which generations. The
> datasheets for all but the latest phys seem to have gone missing...
> 
> In any case, if it works, then I think it's reasonable to use these
> functions.

Sorry, I admit I don't know either the lithography process in which the
PHYs are produced, or the way in which PHYs are referenced in Aquantia
BSPs. I only have access to product datasheets confidentially licensed
to NXP (with registers, packaging and so forth), and the fact that they
are Gen2 or Gen3 is mentioned in the first statement of that document.
I also googled "aqr412" and came up with a product brief on the Marvell
website titled "Marvell® AQRATE GEN3 Ethernet PHYs" which lists the 112
and 412.

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> As of now, all DSA drivers use phylink_generic_validate() and there is
> no known use case remaining for a driver-specific link mode validation
> procedure. As such, remove this DSA operation and let phylink determine
> what is supported based on config->mac_capabilities, which all drivers
> provide.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Not all DSA drivers provide config->mac_capabilities, for example
> mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> those drivers on recent kernels and no one reported that they fail to
> establish a link, so I'm guessing that they work (somehow). But I must
> admit I don't understand why phylink_generic_validate() works when
> mac_capabilities=0. Anyway, these drivers did not provide a
> phylink_validate() method before and do not provide one now, so nothing
> changes for them.

There is one remaining issue that needs to be properly addressed,
which is the bcm_sf2 driver, which is basically buggy. The recent
kernel build bot reports reminded me of this.

I've tried talking to Florian about it, and didn't make much progress,
so I'm carrying a patch in my tree which at least makes what is
provided to phylink correct.

See
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
and all the FIXME comments in there.

This driver really needs to be fixed before we kill DSA's
phylink_validate method (although doing so doesn't change anything
in mainline, but will remove my reminder that bcm_sf2 is still
technically broken.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> > As of now, all DSA drivers use phylink_generic_validate() and there is
> > no known use case remaining for a driver-specific link mode validation
> > procedure. As such, remove this DSA operation and let phylink determine
> > what is supported based on config->mac_capabilities, which all drivers
> > provide.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > Not all DSA drivers provide config->mac_capabilities, for example
> > mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> > those drivers on recent kernels and no one reported that they fail to
> > establish a link, so I'm guessing that they work (somehow). But I must
> > admit I don't understand why phylink_generic_validate() works when
> > mac_capabilities=0. Anyway, these drivers did not provide a
> > phylink_validate() method before and do not provide one now, so nothing
> > changes for them.
> 
> There is one remaining issue that needs to be properly addressed,
> which is the bcm_sf2 driver, which is basically buggy. The recent
> kernel build bot reports reminded me of this.
> 
> I've tried talking to Florian about it, and didn't make much progress,
> so I'm carrying a patch in my tree which at least makes what is
> provided to phylink correct.
> 
> See
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> and all the FIXME comments in there.
> 
> This driver really needs to be fixed before we kill DSA's
> phylink_validate method (although doing so doesn't change anything
> in mainline, but will remove my reminder that bcm_sf2 is still
> technically broken.)

Here's the corrected patch, along with a bit more commentry about the
problems that I had kicking around in another commit.

8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: dsa: bcm_sf2: fix pause mode validation

The implementation appears not to appear to support pause modes on
anything but RGMII, RGMII_TXID, MII and REVMII interface modes. Let
phylink know that detail.

Moreover, RGMII_RXID and RGMII_ID appears to be unsupported.
(This may not be correct; particularly see the FIXMEs in this patch.)

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/bcm_sf2.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 18a3847bd82b..6676971128d1 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
 		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
 		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
 		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+
+		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
+		 * See FIXME 2 and FIXME 3 below.
+		 */
 		phy_interface_set_rgmii(interfaces);
 	}
 
@@ -734,6 +738,28 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
 		MAC_10 | MAC_100 | MAC_1000;
 }
 
+static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
+				unsigned long *supported,
+				struct phylink_link_state *state)
+{
+	u32 caps;
+
+	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
+
+	/* Pause modes are only programmed for these modes - see FIXME 3.
+	 * So, as pause modes are not configured for other modes, disable
+	 * support for them. If FIXME 3 is updated to allow the other RGMII
+	 * modes, these should be included here as well.
+	 */
+	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
+	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+	      state->interface == PHY_INTERFACE_MODE_MII ||
+	      state->interface == PHY_INTERFACE_MODE_REVMII))
+		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);
+
+	phylink_validate_mask_caps(supported, state, caps);
+}
+
 static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
 				  unsigned int mode,
 				  const struct phylink_link_state *state)
@@ -747,6 +773,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
 		return;
 
 	switch (state->interface) {
+	/* FIXME 2: Are RGMII_RXID and RGMII_ID actually supported? This
+	 * switch statement means that the RGMII control register does not
+	 * get programmed in these two modes, but surely it needs to at least
+	 * set the port mode to EXT_GPHY?
+	 */
 	case PHY_INTERFACE_MODE_RGMII:
 		id_mode_dis = 1;
 		fallthrough;
@@ -850,6 +881,10 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
 		else
 			offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
 
+		/* FIXME 3: Are RGMII_RXID and RGMII_ID actually supported?
+		 * Why are pause modes only supported for a couple of RGMII
+		 * modes? Should this be using phy_interface_mode_is_rgmii()?
+		 */
 		if (interface == PHY_INTERFACE_MODE_RGMII ||
 		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
 		    interface == PHY_INTERFACE_MODE_MII ||
@@ -1207,6 +1242,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.get_ethtool_phy_stats	= b53_get_ethtool_phy_stats,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
 	.phylink_get_caps	= bcm_sf2_sw_get_caps,
+	.phylink_validate	= bcm_sf2_sw_validate,
 	.phylink_mac_config	= bcm_sf2_sw_mac_config,
 	.phylink_mac_link_down	= bcm_sf2_sw_mac_link_down,
 	.phylink_mac_link_up	= bcm_sf2_sw_mac_link_up,
-- 
2.30.2

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

On Fri, Nov 04, 2022 at 11:35:08AM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> > There is one remaining issue that needs to be properly addressed,
> > which is the bcm_sf2 driver, which is basically buggy. The recent
> > kernel build bot reports reminded me of this.
> > 
> > I've tried talking to Florian about it, and didn't make much progress,
> > so I'm carrying a patch in my tree which at least makes what is
> > provided to phylink correct.
> > 
> > See
> > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> > and all the FIXME comments in there.
> > 
> > This driver really needs to be fixed before we kill DSA's
> > phylink_validate method (although doing so doesn't change anything
> > in mainline, but will remove my reminder that bcm_sf2 is still
> > technically broken.)
> 
> Here's the corrected patch, along with a bit more commentry about the
> problems that I had kicking around in another commit.

The inconsistencies in the sf2 driver seem valid - I don't know why/if
the hardware doesn't support flow control on MoCA, internal ports and
(some but not all?!) RGMII modes. I hope Florian can make some clarifications.

However, I don't exactly understand your choice of fixing this
inconsistency (by providing a phylink_validate method). Why don't you
simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
(and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
would know to exclude the "pause" link modes, right?

In any case, it looks like coming up with a resolution for DSA's
phylink_validate is out of scope for this patch set, and that I should
just drop patch 4/4 for now and resend the first 3.

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Fri, Nov 04, 2022 at 01:32:48PM +0000, Vladimir Oltean wrote:
> On Fri, Nov 04, 2022 at 11:35:08AM +0000, Russell King (Oracle) wrote:
> > On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> > > There is one remaining issue that needs to be properly addressed,
> > > which is the bcm_sf2 driver, which is basically buggy. The recent
> > > kernel build bot reports reminded me of this.
> > > 
> > > I've tried talking to Florian about it, and didn't make much progress,
> > > so I'm carrying a patch in my tree which at least makes what is
> > > provided to phylink correct.
> > > 
> > > See
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> > > and all the FIXME comments in there.
> > > 
> > > This driver really needs to be fixed before we kill DSA's
> > > phylink_validate method (although doing so doesn't change anything
> > > in mainline, but will remove my reminder that bcm_sf2 is still
> > > technically broken.)
> > 
> > Here's the corrected patch, along with a bit more commentry about the
> > problems that I had kicking around in another commit.
> 
> The inconsistencies in the sf2 driver seem valid - I don't know why/if
> the hardware doesn't support flow control on MoCA, internal ports and
> (some but not all?!) RGMII modes. I hope Florian can make some clarifications.
> 
> However, I don't exactly understand your choice of fixing this
> inconsistency (by providing a phylink_validate method). Why don't you
> simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
> from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
> (and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
> would know to exclude the "pause" link modes, right?

bcm_sf2_sw_get_caps() doesn't have visibility of which interface mode
will be used.

In any case, the patch is not meant for merging, it is meant to provoke
discussion about how to fix the driver, identifying the places in the
driver where it is broken and why.

I'd have fixed it if I could see a solution to the problems, but the
driver is rather self-contradictory, which makes identifying what it
actually supports rather impossible.

For example, does the driver actually work if
PHY_INTERFACE_MODE_RGMII_RXID or PHY_INTERFACE_MODE_RGMII_ID are used,
or does it fail because the port mode is set incorrectly in the RGMII
control register? Should these two interface modes be included in the
switch() to set port_mode to EXT_GPHY or should they be dropped from
the list of supported interfaces. If they should be dropped from the
list of supported interfaces, then that makes sense why we only program
pause modes for PHY_INTERFACE_MODE_RGMII and
PHY_INTERFACE_MODE_RGMII_TXID, and not the other two. Then there's
questions whether this is acting as a MAC end of the RGMII link or
the PHY end, so whether it should even be acting upon the delay
settings in the phy interface mode. If it's the MAC end then it ought
to be allowing all and dealing with the pause mode irrespective of
the RGMII mode. If it's acting as the PHY end, then maybe it only
supports the RGMII and RGMII_TXID modes, in which case the other
two don't matter.

There's too much ambiguity in the driver to derive what's actually
going on here, so the only thing I can do is raise the issues and
hope for a resolution.

What I might do is trim the patch down and submit it - mostly as a
patch adding those FIXME comments to the driver, possibly also
making the driver print a big fat warning when it's initialised in
the hope of finding someone who can at least run some tests on the
hardware.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

On Fri, Nov 04, 2022 at 02:01:15PM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 04, 2022 at 01:32:48PM +0000, Vladimir Oltean wrote:
> > On Fri, Nov 04, 2022 at 11:35:08AM +0000, Russell King (Oracle) wrote:
> > > On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> > > > There is one remaining issue that needs to be properly addressed,
> > > > which is the bcm_sf2 driver, which is basically buggy. The recent
> > > > kernel build bot reports reminded me of this.
> > > > 
> > > > I've tried talking to Florian about it, and didn't make much progress,
> > > > so I'm carrying a patch in my tree which at least makes what is
> > > > provided to phylink correct.
> > > > 
> > > > See
> > > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> > > > and all the FIXME comments in there.
> > > > 
> > > > This driver really needs to be fixed before we kill DSA's
> > > > phylink_validate method (although doing so doesn't change anything
> > > > in mainline, but will remove my reminder that bcm_sf2 is still
> > > > technically broken.)
> > > 
> > > Here's the corrected patch, along with a bit more commentry about the
> > > problems that I had kicking around in another commit.
> > 
> > The inconsistencies in the sf2 driver seem valid - I don't know why/if
> > the hardware doesn't support flow control on MoCA, internal ports and
> > (some but not all?!) RGMII modes. I hope Florian can make some clarifications.
> > 
> > However, I don't exactly understand your choice of fixing this
> > inconsistency (by providing a phylink_validate method). Why don't you
> > simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
> > from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
> > (and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
> > would know to exclude the "pause" link modes, right?
> 
> bcm_sf2_sw_get_caps() doesn't have visibility of which interface mode
> will be used.

Update your tree, commit 4d2f6dde4daa ("net: dsa: bcm_sf2: Have PHYLINK
configure CPU/IMP port(s)") has appeared in net-next and now the check
in mac_link_up() is for phy_interface_is_rgmii().

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Fri, Nov 04, 2022 at 02:25:50PM +0000, Vladimir Oltean wrote:
> On Fri, Nov 04, 2022 at 02:01:15PM +0000, Russell King (Oracle) wrote:
> > On Fri, Nov 04, 2022 at 01:32:48PM +0000, Vladimir Oltean wrote:
> > > On Fri, Nov 04, 2022 at 11:35:08AM +0000, Russell King (Oracle) wrote:
> > > > On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> > > > > There is one remaining issue that needs to be properly addressed,
> > > > > which is the bcm_sf2 driver, which is basically buggy. The recent
> > > > > kernel build bot reports reminded me of this.
> > > > > 
> > > > > I've tried talking to Florian about it, and didn't make much progress,
> > > > > so I'm carrying a patch in my tree which at least makes what is
> > > > > provided to phylink correct.
> > > > > 
> > > > > See
> > > > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> > > > > and all the FIXME comments in there.
> > > > > 
> > > > > This driver really needs to be fixed before we kill DSA's
> > > > > phylink_validate method (although doing so doesn't change anything
> > > > > in mainline, but will remove my reminder that bcm_sf2 is still
> > > > > technically broken.)
> > > > 
> > > > Here's the corrected patch, along with a bit more commentry about the
> > > > problems that I had kicking around in another commit.
> > > 
> > > The inconsistencies in the sf2 driver seem valid - I don't know why/if
> > > the hardware doesn't support flow control on MoCA, internal ports and
> > > (some but not all?!) RGMII modes. I hope Florian can make some clarifications.
> > > 
> > > However, I don't exactly understand your choice of fixing this
> > > inconsistency (by providing a phylink_validate method). Why don't you
> > > simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
> > > from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
> > > (and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
> > > would know to exclude the "pause" link modes, right?
> > 
> > bcm_sf2_sw_get_caps() doesn't have visibility of which interface mode
> > will be used.
> 
> Update your tree, commit 4d2f6dde4daa ("net: dsa: bcm_sf2: Have PHYLINK
> configure CPU/IMP port(s)") has appeared in net-next and now the check
> in mac_link_up() is for phy_interface_is_rgmii().

Great, one less fixme. Still a couple remaining open.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Vladimir Oltean]

On Fri, Nov 04, 2022 at 02:48:11PM +0000, Russell King (Oracle) wrote:
> > > > However, I don't exactly understand your choice of fixing this
> > > > inconsistency (by providing a phylink_validate method). Why don't you
> > > > simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
> > > > from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
> > > > (and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
> > > > would know to exclude the "pause" link modes, right?
> > > 
> > > bcm_sf2_sw_get_caps() doesn't have visibility of which interface mode
> > > will be used.
> > 
> > Update your tree, commit 4d2f6dde4daa ("net: dsa: bcm_sf2: Have PHYLINK
> > configure CPU/IMP port(s)") has appeared in net-next and now the check
> > in mac_link_up() is for phy_interface_is_rgmii().
> 
> Great, one less fixme. Still a couple remaining open.

True.

However, do you agree that phylink_validate() is no longer a contention
point, and that now you can rework your fixup to be localized to
bcm_sf2_sw_get_caps(), which frees up the possibility for me to remove
ds->ops->phylink_validate()?

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Florian Fainelli]

On 11/4/2022 4:35 AM, Russell King (Oracle) wrote:
> On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
>> On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
>>> As of now, all DSA drivers use phylink_generic_validate() and there is
>>> no known use case remaining for a driver-specific link mode validation
>>> procedure. As such, remove this DSA operation and let phylink determine
>>> what is supported based on config->mac_capabilities, which all drivers
>>> provide.
>>>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>>> ---
>>> Not all DSA drivers provide config->mac_capabilities, for example
>>> mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
>>> those drivers on recent kernels and no one reported that they fail to
>>> establish a link, so I'm guessing that they work (somehow). But I must
>>> admit I don't understand why phylink_generic_validate() works when
>>> mac_capabilities=0. Anyway, these drivers did not provide a
>>> phylink_validate() method before and do not provide one now, so nothing
>>> changes for them.
>>
>> There is one remaining issue that needs to be properly addressed,
>> which is the bcm_sf2 driver, which is basically buggy. The recent
>> kernel build bot reports reminded me of this.
>>
>> I've tried talking to Florian about it, and didn't make much progress,
>> so I'm carrying a patch in my tree which at least makes what is
>> provided to phylink correct.

Might be worth submitting as RFC/RFT and then just hunt me down until 
you get what you want from me?

>>
>> See
>> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
>> and all the FIXME comments in there.
>>
>> This driver really needs to be fixed before we kill DSA's
>> phylink_validate method (although doing so doesn't change anything
>> in mainline, but will remove my reminder that bcm_sf2 is still
>> technically broken.)
> 
> Here's the corrected patch, along with a bit more commentry about the
> problems that I had kicking around in another commit.
> 
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] net: dsa: bcm_sf2: fix pause mode validation
> 
> The implementation appears not to appear to support pause modes on
> anything but RGMII, RGMII_TXID, MII and REVMII interface modes. Let
> phylink know that detail.
> 
> Moreover, RGMII_RXID and RGMII_ID appears to be unsupported.
> (This may not be correct; particularly see the FIXMEs in this patch.)
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/dsa/bcm_sf2.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 18a3847bd82b..6676971128d1 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>   		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
>   		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
>   		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
> +
> +		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
> +		 * See FIXME 2 and FIXME 3 below.
> +		 */

They are supported, just not tested and still don't have hardware to 
test, I suppose you can include both modes for simplicity if they end up 
being broken, a fix would be submitted.

>   		phy_interface_set_rgmii(interfaces);
>   	}
>   
> @@ -734,6 +738,28 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>   		MAC_10 | MAC_100 | MAC_1000;
>   }
>   
> +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
> +				unsigned long *supported,
> +				struct phylink_link_state *state)
> +{
> +	u32 caps;
> +
> +	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
> +
> +	/* Pause modes are only programmed for these modes - see FIXME 3.
> +	 * So, as pause modes are not configured for other modes, disable
> +	 * support for them. If FIXME 3 is updated to allow the other RGMII
> +	 * modes, these should be included here as well.
> +	 */
> +	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
> +	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> +	      state->interface == PHY_INTERFACE_MODE_MII ||
> +	      state->interface == PHY_INTERFACE_MODE_REVMII))
> +		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);

Can be programmed on all ports.

> +
> +	phylink_validate_mask_caps(supported, state, caps);
> +}
> +
>   static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>   				  unsigned int mode,
>   				  const struct phylink_link_state *state)
> @@ -747,6 +773,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>   		return;
>   
>   	switch (state->interface) {
> +	/* FIXME 2: Are RGMII_RXID and RGMII_ID actually supported? This
> +	 * switch statement means that the RGMII control register does not
> +	 * get programmed in these two modes, but surely it needs to at least
> +	 * set the port mode to EXT_GPHY?
> +	 */
>   	case PHY_INTERFACE_MODE_RGMII:
>   		id_mode_dis = 1;
>   		fallthrough;
> @@ -850,6 +881,10 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
>   		else
>   			offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
>   
> +		/* FIXME 3: Are RGMII_RXID and RGMII_ID actually supported?
> +		 * Why are pause modes only supported for a couple of RGMII
> +		 * modes? Should this be using phy_interface_mode_is_rgmii()?
> +		 */
>   		if (interface == PHY_INTERFACE_MODE_RGMII ||
>   		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>   		    interface == PHY_INTERFACE_MODE_MII ||
> @@ -1207,6 +1242,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
>   	.get_ethtool_phy_stats	= b53_get_ethtool_phy_stats,
>   	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
>   	.phylink_get_caps	= bcm_sf2_sw_get_caps,
> +	.phylink_validate	= bcm_sf2_sw_validate,
>   	.phylink_mac_config	= bcm_sf2_sw_mac_config,
>   	.phylink_mac_link_down	= bcm_sf2_sw_mac_link_down,
>   	.phylink_mac_link_up	= bcm_sf2_sw_mac_link_up,

-- 
Florian

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Russell King (Oracle)]

On Fri, Nov 04, 2022 at 08:40:56AM -0700, Florian Fainelli wrote:
> On 11/4/2022 4:35 AM, Russell King (Oracle) wrote:
> > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> > index 18a3847bd82b..6676971128d1 100644
> > --- a/drivers/net/dsa/bcm_sf2.c
> > +++ b/drivers/net/dsa/bcm_sf2.c
> > @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
> >   		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
> >   		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
> >   		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
> > +
> > +		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
> > +		 * See FIXME 2 and FIXME 3 below.
> > +		 */
> 
> They are supported, just not tested and still don't have hardware to test, I
> suppose you can include both modes for simplicity if they end up being
> broken, a fix would be submitted.

Okay, that sounds like we can add them to the switch() in
bcm_sf2_sw_mac_config(). I assume id_mode_dis should be zero for
these other modes.

> > +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
> > +				unsigned long *supported,
> > +				struct phylink_link_state *state)
> > +{
> > +	u32 caps;
> > +
> > +	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
> > +
> > +	/* Pause modes are only programmed for these modes - see FIXME 3.
> > +	 * So, as pause modes are not configured for other modes, disable
> > +	 * support for them. If FIXME 3 is updated to allow the other RGMII
> > +	 * modes, these should be included here as well.
> > +	 */
> > +	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
> > +	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> > +	      state->interface == PHY_INTERFACE_MODE_MII ||
> > +	      state->interface == PHY_INTERFACE_MODE_REVMII))
> > +		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);
> 
> Can be programmed on all ports.

If I understand you correctly, I think you mean that this can be
programmed on all ports that support the RGMII control register:

        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 (tx_pause)
                        reg |= TX_PAUSE_EN;
                if (rx_pause)
                        reg |= RX_PAUSE_EN;

                reg_writel(priv, reg, reg_rgmii_ctrl);
        }

We seem to have several places in the code that make this decision -
bcm_sf2_sw_mac_link_set(). I'm guessing, looking at
bcm_sf2_reg_rgmii_cntrl(), that we _could_ use the device ID and port
number in bcm_sf2_sw_get_caps() to narrow down whether the pause
modes are supported - as ports that do not have the RGMII control
register can't have pause modes programmed?

So for the BCM4908, it's just port 7, and for everything else it's
ports 0-2.

That would mean something like:

static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
                                struct phylink_config *config)
{
...
	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
...
	config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000;

	if (priv->type == BCM4908_DEVICE_ID ? port == 7 :
	    (port >= 0 && port < 3))
		config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE;

may be sensible?

It brings up another question: if the port supports this register, but
we aren't using one of the rgmii, mii or revmii modes, should we be
clearing the pause bits in this register if we're telling the system
that pause isn't supported, or does the hardware not look at this RGMII
register unless its in one of those three modes?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

[Florian Fainelli]

On 11/4/2022 9:35 AM, Russell King (Oracle) wrote:
> On Fri, Nov 04, 2022 at 08:40:56AM -0700, Florian Fainelli wrote:
>> On 11/4/2022 4:35 AM, Russell King (Oracle) wrote:
>>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>>> index 18a3847bd82b..6676971128d1 100644
>>> --- a/drivers/net/dsa/bcm_sf2.c
>>> +++ b/drivers/net/dsa/bcm_sf2.c
>>> @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>>>    		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
>>>    		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
>>>    		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
>>> +
>>> +		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
>>> +		 * See FIXME 2 and FIXME 3 below.
>>> +		 */
>>
>> They are supported, just not tested and still don't have hardware to test, I
>> suppose you can include both modes for simplicity if they end up being
>> broken, a fix would be submitted.
> 
> Okay, that sounds like we can add them to the switch() in
> bcm_sf2_sw_mac_config(). I assume id_mode_dis should be zero for
> these other modes.

Yes please.

> 
>>> +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
>>> +				unsigned long *supported,
>>> +				struct phylink_link_state *state)
>>> +{
>>> +	u32 caps;
>>> +
>>> +	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
>>> +
>>> +	/* Pause modes are only programmed for these modes - see FIXME 3.
>>> +	 * So, as pause modes are not configured for other modes, disable
>>> +	 * support for them. If FIXME 3 is updated to allow the other RGMII
>>> +	 * modes, these should be included here as well.
>>> +	 */
>>> +	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
>>> +	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>> +	      state->interface == PHY_INTERFACE_MODE_MII ||
>>> +	      state->interface == PHY_INTERFACE_MODE_REVMII))
>>> +		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);
>>
>> Can be programmed on all ports.
> 
> If I understand you correctly, I think you mean that this can be
> programmed on all ports that support the RGMII control register:
> 
>          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 (tx_pause)
>                          reg |= TX_PAUSE_EN;
>                  if (rx_pause)
>                          reg |= RX_PAUSE_EN;
> 
>                  reg_writel(priv, reg, reg_rgmii_ctrl);
>          }
> 
> We seem to have several places in the code that make this decision -
> bcm_sf2_sw_mac_link_set(). I'm guessing, looking at
> bcm_sf2_reg_rgmii_cntrl(), that we _could_ use the device ID and port
> number in bcm_sf2_sw_get_caps() to narrow down whether the pause
> modes are supported - as ports that do not have the RGMII control
> register can't have pause modes programmed?
> 
> So for the BCM4908, it's just port 7, and for everything else it's
> ports 0-2.
> 
> That would mean something like:
> 
> static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>                                  struct phylink_config *config)
> {
> ...
> 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> ...
> 	config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000;
> 
> 	if (priv->type == BCM4908_DEVICE_ID ? port == 7 :
> 	    (port >= 0 && port < 3))
> 		config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
> 
> may be sensible?

Sorry, I should have been way more specific here, all ports support 
pause frames, however only a subset of the ports, those connected over 
RGMII require to program the reg_rgmii_ctrl offset with the advertised 
pause settings.

> 
> It brings up another question: if the port supports this register, but
> we aren't using one of the rgmii, mii or revmii modes, should we be
> clearing the pause bits in this register if we're telling the system
> that pause isn't supported, or does the hardware not look at this RGMII
> register unless its in one of those three modes?

It is the latter, the hardware does not look at this RGMII register 
unless it is one of the 4 possible RGMII modes, MII, or reverse MII.
-- 
Florian