[net-next PATCH v4 00/11] net: pcs: Introduce support for fwnode PCS

[net-next PATCH v4 00/11] net: pcs: Introduce support for fwnode PCS

[Christian Marangi]

This series introduce a most awaited feature that is correctly
provide PCS with fwnode without having to use specific export symbol
and additional handling of PCS in phylink.

There is currently an equivalent series for this feature where
a wrapper implementation is proposed. I honestly don't really like
introducing layer of layer of wrapping to workaround stuff, so
this is my attempt at giving a more direct approach to this.

---
First the PCS fwnode:

The concept is to implement a producer-consumer API similar to other
subsystem like clock or PHY.

That seems to be the best solution to the problem as PCS driver needs
to be detached from phylink and implement a simple way to provide a
PCS while maintaining support for probe defer or driver removal.

To keep the implementation simple, the PCS driver devs needs some
collaboration to correctly implement this. This is O.K. as helper
to correctly implement this are provided hence it's really a matter
of following a pattern to correct follow removal of a PCS driver.

A PCS provider have to implement and call fwnode_pcs_add_provider() in
probe function and define an xlate function to define how the PCS
should be provided based on the requested interface and phandle spec
defined in fwnode (based on the #pcs-cells)

fwnode_pcs_get() is provided to provide a specific PCS declared in
fwnode at index.

A simple xlate function is provided for simple single PCS
implementation, fwnode_pcs_simple_get.

A PCS provider on driver removal should first call
fwnode_pcs_del_provider() to delete itself as a provider and then
release the PCS from phylink with phylink_release_pcs() under rtnl
lock.

---
Second PCS handling in phylink:

We have the PCS problem for the only reason that in initial
implementation, we permitted way too much flexibility to MAC driver
and things started to deviate. At times we couldn't think SoC
would start to put PCS outside the MAC hence it was OK to assume
they would live in the same driver. With the introduction of
10g in more consumer devices, we are observing a rapid growth
of this pattern with multiple PCS external to MAC.

To put a stop on this, the only solution is to give back to phylink
control on PCS handling and enforce more robust supported interface
definition from both MAC and PCS side.

It's suggested to read patch 0003 of this series for more info, here
a brief explaination of the idea:

This series introduce handling of PCS in phylink and try to deprecate
.mac_select_pcs.

Phylink now might contain a linked list of available PCS and
those will be used for PCS selection on phylink_major_config.

MAC driver needs to define pcs_interfaces mask in phylink_config
for every interface that needs a dedicated PCS.

These PCS needs to be provided to phylink at phylink_create time
by setting the available_pcs and num_available_pcs in phylink_config.
A helper to parse PCS from fwnode is provided
fwnode_phylink_pcs_parse() that will fill a preallocated array of
PCS. (the same function can be used to get the number of PCS
defined in DT, more info in patch 0005)

phylink_create() will fill the internal PCS list with the passed
array of PCS. phylink_major_config and other user of .mac_select_pcs
are adapted to make use of this new PCS list.

The supported interface value is also moved internally to phylink
struct. This is to handle late removal and addition of PCS.
(the bonus effect to this is giving phylink a clear idea of what
is actually supported by the MAC and his constraint with PCS)

The supported interface mask in phylink is done by OR the
supported_interfaces in phylink_config with every PCS in PCS list.

PCS removal is supported by forcing a mac_config, refresh the
supported interfaces and run a phy_resolve().

PCS late addition is supported by introducing a global notifier
for PCS provider. If a phylink have the pcs_interfaces mask not
zero, it's registered to this notifier.

PCS provider will emit a global PCS add event to signal any
interface that a new PCS might be avialable.

The function will then check if the PCS is related to the MAC
fwnode and add it accordingly.

A user for this new implementation is provided as an Airoha PCS
driver. This was also tested downstream with the IPQ95xx QCOM SoC
and with the help of Daniel also on the various Mediatek MT7988
SoC with both SFP cage implementation and DSA attached.

Lots of tests were done with driver unbind/bind and with interface
up/down also by adding print to make sure major_config_fail gets
correctly triggered and reset once the PCS comes back.

The dedicated commits have longer description on the implementation
so it's suggested to also check there for additional info.

---

Changes v4:
- Move patch 0002 phy_interface_copy to 0002 (fix bisectability
  problem)
- Address review from Lorenzo for Airoha ethernet driver
- Fix kdoc error with missing Return (actually missing : before Return)
- Fix UNMET dependency reported error for CONFIG_FWNODE_PCS
- Revert to pcs.c instead of core.c (due to name conflict with other kmod)
- Fix clang compilation error for Airoha PCS driver
- Add missing inline function to pcs.h function
Changes v3:
- Out of RFC
- Fix various spelling mistake
- Drop circular dependency patch
- Complete Airoha Ethernet phylink integration
- Introduce .pcs_link_down PCS OP
Changes v2:
- Switch to fwnode
- Implement PCS provider notifier
- Better split changes
- Move supported_interfaces to phylink
- Add circular dependency patch
- Rework handling with indirect addition/removal and
  trigger of phylink_resolve()

Christian Marangi (11):
  net: phy: introduce phy_interface_copy helper
  net: phylink: keep and use MAC supported_interfaces in phylink struct
  net: phylink: introduce internal phylink PCS handling
  net: phylink: add phylink_release_pcs() to externally release a PCS
  net: pcs: implement Firmware node support for PCS driver
  net: phylink: support late PCS provider attach
  dt-bindings: net: ethernet-controller: permit to define multiple PCS
  net: phylink: add .pcs_link_down PCS OP
  dt-bindings: net: pcs: Document support for Airoha Ethernet PCS
  net: pcs: airoha: add PCS driver for Airoha SoC
  net: airoha: add phylink support for GDM2/3/4

 .../bindings/net/ethernet-controller.yaml     |    2 -
 .../bindings/net/pcs/airoha,pcs.yaml          |  112 +
 drivers/net/ethernet/airoha/airoha_eth.c      |  155 +-
 drivers/net/ethernet/airoha/airoha_eth.h      |    3 +
 drivers/net/ethernet/airoha/airoha_regs.h     |   12 +
 drivers/net/pcs/Kconfig                       |   13 +
 drivers/net/pcs/Makefile                      |    2 +
 drivers/net/pcs/pcs-airoha.c                  | 2921 +++++++++++++++++
 drivers/net/pcs/pcs.c                         |  241 ++
 drivers/net/phy/phylink.c                     |  287 +-
 include/linux/pcs/pcs-provider.h              |   41 +
 include/linux/pcs/pcs.h                       |  104 +
 include/linux/phy.h                           |    5 +
 include/linux/phylink.h                       |   14 +
 14 files changed, 3883 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
 create mode 100644 drivers/net/pcs/pcs-airoha.c
 create mode 100644 drivers/net/pcs/pcs.c
 create mode 100644 include/linux/pcs/pcs-provider.h
 create mode 100644 include/linux/pcs/pcs.h

-- 
2.48.1

[net-next PATCH v4 01/11] net: phy: introduce phy_interface_copy helper

[Christian Marangi]

Introduce phy_interface_copy helper as a shorthand to copy the PHY
interface bitmap to a different location.

This is useful if a PHY interface bitmap needs to be stored in a
different variable and needs to be reset to an original value saved in a
different bitmap.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/phy.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index d62d292024bc..9f0e5fb30d63 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -173,6 +173,11 @@ static inline void phy_interface_or(unsigned long *dst, const unsigned long *a,
 	bitmap_or(dst, a, b, PHY_INTERFACE_MODE_MAX);
 }
 
+static inline void phy_interface_copy(unsigned long *dst, const unsigned long *src)
+{
+	bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX);
+}
+
 static inline void phy_interface_set_rgmii(unsigned long *intf)
 {
 	__set_bit(PHY_INTERFACE_MODE_RGMII, intf);
-- 
2.48.1

[net-next PATCH v4 02/11] net: phylink: keep and use MAC supported_interfaces in phylink struct

[Christian Marangi]

Add in phylink struct a copy of supported_interfaces from phylink_config
and make use of that instead of relying on phylink_config value.

This in preparation for support of PCS handling internally to phylink
where a PCS can be removed or added after the phylink is created and we
need both a reference of the supported_interfaces value from
phylink_config and an internal value that can be updated with the new
PCS info.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phylink.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0faa3d97e06b..ec42fd278604 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -59,6 +59,11 @@ struct phylink {
 	/* The link configuration settings */
 	struct phylink_link_state link_config;
 
+	/* What interface are supported by the current link.
+	 * Can change on removal or addition of new PCS.
+	 */
+	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+
 	/* The current settings */
 	phy_interface_t cur_interface;
 
@@ -610,7 +615,7 @@ static int phylink_validate_mask(struct phylink *pl, struct phy_device *phy,
 static int phylink_validate(struct phylink *pl, unsigned long *supported,
 			    struct phylink_link_state *state)
 {
-	const unsigned long *interfaces = pl->config->supported_interfaces;
+	const unsigned long *interfaces = pl->supported_interfaces;
 
 	if (state->interface == PHY_INTERFACE_MODE_NA)
 		return phylink_validate_mask(pl, NULL, supported, state,
@@ -1809,6 +1814,9 @@ struct phylink *phylink_create(struct phylink_config *config,
 	mutex_init(&pl->state_mutex);
 	INIT_WORK(&pl->resolve, phylink_resolve);
 
+	phy_interface_copy(pl->supported_interfaces,
+			   config->supported_interfaces);
+
 	pl->config = config;
 	if (config->type == PHYLINK_NETDEV) {
 		pl->netdev = to_net_dev(config->dev);
@@ -1967,7 +1975,7 @@ static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
 		 * those which the host supports.
 		 */
 		phy_interface_and(interfaces, phy->possible_interfaces,
-				  pl->config->supported_interfaces);
+				  pl->supported_interfaces);
 
 		if (phy_interface_empty(interfaces)) {
 			phylink_err(pl, "PHY has no common interfaces\n");
@@ -2684,12 +2692,12 @@ static phy_interface_t phylink_sfp_select_interface(struct phylink *pl,
 		return interface;
 	}
 
-	if (!test_bit(interface, pl->config->supported_interfaces)) {
+	if (!test_bit(interface, pl->supported_interfaces)) {
 		phylink_err(pl,
 			    "selection of interface failed, SFP selected %s (%u) but MAC supports %*pbl\n",
 			    phy_modes(interface), interface,
 			    (int)PHY_INTERFACE_MODE_MAX,
-			    pl->config->supported_interfaces);
+			    pl->supported_interfaces);
 		return PHY_INTERFACE_MODE_NA;
 	}
 
@@ -3576,14 +3584,14 @@ static int phylink_sfp_config_optical(struct phylink *pl)
 
 	phylink_dbg(pl, "optical SFP: interfaces=[mac=%*pbl, sfp=%*pbl]\n",
 		    (int)PHY_INTERFACE_MODE_MAX,
-		    pl->config->supported_interfaces,
+		    pl->supported_interfaces,
 		    (int)PHY_INTERFACE_MODE_MAX,
 		    pl->sfp_interfaces);
 
 	/* Find the union of the supported interfaces by the PCS/MAC and
 	 * the SFP module.
 	 */
-	phy_interface_and(interfaces, pl->config->supported_interfaces,
+	phy_interface_and(interfaces, pl->supported_interfaces,
 			  pl->sfp_interfaces);
 	if (phy_interface_empty(interfaces)) {
 		phylink_err(pl, "unsupported SFP module: no common interface modes\n");
@@ -3729,7 +3737,7 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 
 	/* Set the PHY's host supported interfaces */
 	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
-			  pl->config->supported_interfaces);
+			  pl->supported_interfaces);
 
 	/* Do the initial configuration */
 	return phylink_sfp_config_phy(pl, phy);
-- 
2.48.1

[net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Christian Marangi]

Introduce internal handling of PCS for phylink. This is an alternative
to .mac_select_pcs that moves the selection logic of the PCS entirely to
phylink with the usage of the supported_interface value in the PCS
struct.

MAC should now provide an array of available PCS in phylink_config in
.available_pcs and fill the .num_available_pcs with the number of
elements in the array. MAC should also define a new bitmap,
pcs_interfaces, in phylink_config to define for what interface mode a
dedicated PCS is required.

On phylink_create() this array is parsed and a linked list of PCS is
created based on the PCS passed in phylink_config.
Also the supported_interface value in phylink struct is updated with the
new supported_interface from the provided PCS.

On phylink_start() every PCS in phylink PCS list gets attached to the
phylink instance. This is done by setting the phylink value in
phylink_pcs struct to the phylink instance.

On phylink_stop(), every PCS in phylink PCS list is detached from the
phylink instance. This is done by setting the phylink value in
phylink_pcs struct to NULL.

phylink_validate_mac_and_pcs(), phylink_major_config() and
phylink_inband_caps() are updated to support this new implementation
with the PCS list stored in phylink.

They will make use of phylink_validate_pcs_interface() that will loop
for every PCS in the phylink PCS available list and find one that supports
the passed interface.

phylink_validate_pcs_interface() applies the same logic of .mac_select_pcs
where if a supported_interface value is not set for the PCS struct, then
it's assumed every interface is supported.

A MAC is required to implement either a .mac_select_pcs or make use of
the PCS list implementation. Implementing both will result in a fail
on MAC/PCS validation.

phylink value in phylink_pcs struct with this implementation is used to
track from PCS side when it's attached to a phylink instance. PCS driver
will make use of this information to correctly detach from a phylink
instance if needed.

The .mac_select_pcs implementation is not changed but it's expected that
every MAC driver migrates to the new implementation to later deprecate
and remove .mac_select_pcs.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phylink.c | 147 +++++++++++++++++++++++++++++++++-----
 include/linux/phylink.h   |  10 +++
 2 files changed, 139 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ec42fd278604..95d7e06dee56 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -59,6 +59,9 @@ struct phylink {
 	/* The link configuration settings */
 	struct phylink_link_state link_config;
 
+	/* List of available PCS */
+	struct list_head pcs_list;
+
 	/* What interface are supported by the current link.
 	 * Can change on removal or addition of new PCS.
 	 */
@@ -144,6 +147,8 @@ static const phy_interface_t phylink_sfp_interface_preference[] = {
 
 static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
 
+static void phylink_run_resolve(struct phylink *pl);
+
 /**
  * phylink_set_port_modes() - set the port type modes in the ethtool mask
  * @mask: ethtool link mode mask
@@ -499,22 +504,59 @@ static void phylink_validate_mask_caps(unsigned long *supported,
 	linkmode_and(state->advertising, state->advertising, mask);
 }
 
+static int phylink_validate_pcs_interface(struct phylink_pcs *pcs,
+					  phy_interface_t interface)
+{
+	/* If PCS define an empty supported_interfaces value, assume
+	 * all interface are supported.
+	 */
+	if (phy_interface_empty(pcs->supported_interfaces))
+		return 0;
+
+	/* Ensure that this PCS supports the interface mode */
+	if (!test_bit(interface, pcs->supported_interfaces))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int phylink_validate_mac_and_pcs(struct phylink *pl,
 					unsigned long *supported,
 					struct phylink_link_state *state)
 {
-	struct phylink_pcs *pcs = NULL;
 	unsigned long capabilities;
+	struct phylink_pcs *pcs;
+	bool pcs_found = false;
 	int ret;
 
 	/* Get the PCS for this interface mode */
 	if (pl->mac_ops->mac_select_pcs) {
+		/* Make sure either PCS internal validation or .mac_select_pcs
+		 * is used. Return error if both are defined.
+		 */
+		if (!list_empty(&pl->pcs_list)) {
+			phylink_err(pl, "either phylink_pcs_add() or .mac_select_pcs must be used\n");
+			return -EINVAL;
+		}
+
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs))
 			return PTR_ERR(pcs);
+
+		pcs_found = !!pcs;
+	} else {
+		/* Check every assigned PCS and search for one that supports
+		 * the interface.
+		 */
+		list_for_each_entry(pcs, &pl->pcs_list, list) {
+			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
+				pcs_found = true;
+				break;
+			}
+		}
 	}
 
-	if (pcs) {
+	if (pcs_found) {
 		/* The PCS, if present, must be setup before phylink_create()
 		 * has been called. If the ops is not initialised, print an
 		 * error and backtrace rather than oopsing the kernel.
@@ -526,13 +568,10 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 			return -EINVAL;
 		}
 
-		/* Ensure that this PCS supports the interface which the MAC
-		 * returned it for. It is an error for the MAC to return a PCS
-		 * that does not support the interface mode.
-		 */
-		if (!phy_interface_empty(pcs->supported_interfaces) &&
-		    !test_bit(state->interface, pcs->supported_interfaces)) {
-			phylink_err(pl, "MAC returned PCS which does not support %s\n",
+		/* Recheck PCS to handle legacy way for .mac_select_pcs */
+		ret = phylink_validate_pcs_interface(pcs, state->interface);
+		if (ret) {
+			phylink_err(pl, "selected PCS does not support %s\n",
 				    phy_modes(state->interface));
 			return -EINVAL;
 		}
@@ -937,12 +976,22 @@ static unsigned int phylink_inband_caps(struct phylink *pl,
 					 phy_interface_t interface)
 {
 	struct phylink_pcs *pcs;
+	bool pcs_found = false;
 
-	if (!pl->mac_ops->mac_select_pcs)
-		return 0;
+	if (pl->mac_ops->mac_select_pcs) {
+		pcs = pl->mac_ops->mac_select_pcs(pl->config,
+						  interface);
+		pcs_found = !!pcs;
+	} else {
+		list_for_each_entry(pcs, &pl->pcs_list, list) {
+			if (!phylink_validate_pcs_interface(pcs, interface)) {
+				pcs_found = true;
+				break;
+			}
+		}
+	}
 
-	pcs = pl->mac_ops->mac_select_pcs(pl->config, interface);
-	if (!pcs)
+	if (!pcs_found)
 		return 0;
 
 	return phylink_pcs_inband_caps(pcs, interface);
@@ -1228,10 +1277,36 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 			pl->major_config_failed = true;
 			return;
 		}
+	/* Find a PCS in available PCS list for the requested interface.
+	 * This doesn't overwrite the previous .mac_select_pcs as either
+	 * .mac_select_pcs or PCS list implementation are permitted.
+	 *
+	 * Skip searching if the MAC doesn't require a dedicaed PCS for
+	 * the requested interface.
+	 */
+	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
+		bool pcs_found = false;
+
+		list_for_each_entry(pcs, &pl->pcs_list, list) {
+			if (!phylink_validate_pcs_interface(pcs,
+							    state->interface)) {
+				pcs_found = true;
+				break;
+			}
+		}
+
+		if (!pcs_found) {
+			phylink_err(pl,
+				    "couldn't find a PCS for %s\n",
+				    phy_modes(state->interface));
 
-		pcs_changed = pl->pcs != pcs;
+			pl->major_config_failed = true;
+			return;
+		}
 	}
 
+	pcs_changed = pl->pcs != pcs;
+
 	phylink_pcs_neg_mode(pl, pcs, state->interface, state->advertising);
 
 	phylink_dbg(pl, "major config, active %s/%s/%s\n",
@@ -1258,10 +1333,12 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 	if (pcs_changed) {
 		phylink_pcs_disable(pl->pcs);
 
-		if (pl->pcs)
-			pl->pcs->phylink = NULL;
+		if (pl->mac_ops->mac_select_pcs) {
+			if (pl->pcs)
+				pl->pcs->phylink = NULL;
 
-		pcs->phylink = pl;
+			pcs->phylink = pl;
+		}
 
 		pl->pcs = pcs;
 	}
@@ -1797,8 +1874,9 @@ struct phylink *phylink_create(struct phylink_config *config,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *mac_ops)
 {
+	struct phylink_pcs *pcs;
 	struct phylink *pl;
-	int ret;
+	int i, ret;
 
 	/* Validate the supplied configuration */
 	if (phy_interface_empty(config->supported_interfaces)) {
@@ -1813,9 +1891,21 @@ struct phylink *phylink_create(struct phylink_config *config,
 
 	mutex_init(&pl->state_mutex);
 	INIT_WORK(&pl->resolve, phylink_resolve);
+	INIT_LIST_HEAD(&pl->pcs_list);
+
+	/* Fill the PCS list with available PCS from phylink config */
+	for (i = 0; i < config->num_available_pcs; i++) {
+		pcs = config->available_pcs[i];
+
+		list_add(&pcs->list, &pl->pcs_list);
+	}
 
 	phy_interface_copy(pl->supported_interfaces,
 			   config->supported_interfaces);
+	list_for_each_entry(pcs, &pl->pcs_list, list)
+		phy_interface_or(pl->supported_interfaces,
+				 pl->supported_interfaces,
+				 pcs->supported_interfaces);
 
 	pl->config = config;
 	if (config->type == PHYLINK_NETDEV) {
@@ -1894,10 +1984,16 @@ EXPORT_SYMBOL_GPL(phylink_create);
  */
 void phylink_destroy(struct phylink *pl)
 {
+	struct phylink_pcs *pcs, *tmp;
+
 	sfp_bus_del_upstream(pl->sfp_bus);
 	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
 
+	/* Remove every PCS from phylink PCS list */
+	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
+		list_del(&pcs->list);
+
 	cancel_work_sync(&pl->resolve);
 	kfree(pl);
 }
@@ -2374,6 +2470,7 @@ static irqreturn_t phylink_link_handler(int irq, void *data)
  */
 void phylink_start(struct phylink *pl)
 {
+	struct phylink_pcs *pcs;
 	bool poll = false;
 
 	ASSERT_RTNL();
@@ -2400,6 +2497,10 @@ void phylink_start(struct phylink *pl)
 
 	pl->pcs_state = PCS_STATE_STARTED;
 
+	/* link available PCS to phylink struct */
+	list_for_each_entry(pcs, &pl->pcs_list, list)
+		pcs->phylink = pl;
+
 	phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED);
 
 	if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
@@ -2444,6 +2545,8 @@ EXPORT_SYMBOL_GPL(phylink_start);
  */
 void phylink_stop(struct phylink *pl)
 {
+	struct phylink_pcs *pcs;
+
 	ASSERT_RTNL();
 
 	if (pl->sfp_bus)
@@ -2461,6 +2564,14 @@ void phylink_stop(struct phylink *pl)
 	pl->pcs_state = PCS_STATE_DOWN;
 
 	phylink_pcs_disable(pl->pcs);
+
+	/* Drop link between phylink and PCS */
+	list_for_each_entry(pcs, &pl->pcs_list, list)
+		pcs->phylink = NULL;
+
+	/* Restore original supported interfaces */
+	phy_interface_copy(pl->supported_interfaces,
+			   pl->config->supported_interfaces);
 }
 EXPORT_SYMBOL_GPL(phylink_stop);
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 30659b615fca..ef0b5a0729c8 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -150,12 +150,16 @@ enum phylink_op_type {
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
  *                        are supported by the MAC/PCS.
+ * @pcs_interfaces: bitmap describing for which PHY_INTERFACE_MODE_xxx a
+ *		    dedicated PCS is required.
  * @lpi_interfaces: bitmap describing which PHY interface modes can support
  *		    LPI signalling.
  * @mac_capabilities: MAC pause/speed/duplex capabilities.
  * @lpi_capabilities: MAC speeds which can support LPI signalling
  * @lpi_timer_default: Default EEE LPI timer setting.
  * @eee_enabled_default: If set, EEE will be enabled by phylink at creation time
+ * @available_pcs: array of available phylink_pcs PCS
+ * @num_available_pcs: num of available phylink_pcs PCS
  */
 struct phylink_config {
 	struct device *dev;
@@ -168,11 +172,14 @@ struct phylink_config {
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+	DECLARE_PHY_INTERFACE_MASK(pcs_interfaces);
 	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
 	unsigned long mac_capabilities;
 	unsigned long lpi_capabilities;
 	u32 lpi_timer_default;
 	bool eee_enabled_default;
+	struct phylink_pcs **available_pcs;
+	unsigned int num_available_pcs;
 };
 
 void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
@@ -469,6 +476,9 @@ struct phylink_pcs {
 	struct phylink *phylink;
 	bool poll;
 	bool rxc_always_on;
+
+	/* private: */
+	struct list_head list;
 };
 
 /**
-- 
2.48.1

[net-next PATCH v4 04/11] net: phylink: add phylink_release_pcs() to externally release a PCS

[Christian Marangi]

Add phylink_release_pcs() to externally release a PCS from a phylink
instance. This can be used to handle case when a single PCS needs to be
removed and the phylink instance needs to be refreshed.

On calling phylink_release_pcs(), the PCS will be removed from the
phylink internal PCS list and the phylink supported_interfaces value is
reparsed with the remaining PCS interfaces.

Also a phylink resolve is triggered to handle the PCS removal.

A flag to make phylink resolve reconfigure the interface (even if it
didn't change) is also added. This is needed to handle the special
case when the current PCS used by phylink is removed and a major_config
is needed to propagae the configuration change. With this option
enabled we also force mac_config even if the PHY link is not up for
the in-band case.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phylink.c | 58 ++++++++++++++++++++++++++++++++++++++-
 include/linux/phylink.h   |  2 ++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 95d7e06dee56..2f28c4c83062 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -84,6 +84,7 @@ struct phylink {
 	bool link_failed;
 	bool suspend_link_up;
 	bool major_config_failed;
+	bool reconfig_interface;
 	bool mac_supports_eee_ops;
 	bool mac_supports_eee;
 	bool phy_enable_tx_lpi;
@@ -895,6 +896,55 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
 	}
 }
 
+/**
+ * phylink_release_pcs - Removes a PCS from the phylink PCS available list
+ * @pcs: a pointer to the phylink_pcs struct to be released
+ *
+ * This function release a PCS from the phylink PCS available list if
+ * actually in use. It also refreshes the supported interfaces of the
+ * phylink instance by copying the supported interfaces from the phylink
+ * conf and merging the supported interfaces of the remaining available PCS
+ * in the list and trigger a resolve.
+ */
+void phylink_release_pcs(struct phylink_pcs *pcs)
+{
+	struct phylink *pl;
+
+	ASSERT_RTNL();
+
+	pl = pcs->phylink;
+	if (!pl)
+		return;
+
+	list_del(&pcs->list);
+	pcs->phylink = NULL;
+
+	/* Check if we are removing the PCS currently
+	 * in use by phylink. If this is the case,
+	 * force phylink resolve to reconfigure the interface
+	 * mode and set the phylink PCS to NULL.
+	 */
+	if (pl->pcs == pcs) {
+		mutex_lock(&pl->state_mutex);
+
+		pl->reconfig_interface = true;
+		pl->pcs = NULL;
+
+		mutex_unlock(&pl->state_mutex);
+	}
+
+	/* Refresh supported interfaces */
+	phy_interface_copy(pl->supported_interfaces,
+			   pl->config->supported_interfaces);
+	list_for_each_entry(pcs, &pl->pcs_list, list)
+		phy_interface_or(pl->supported_interfaces,
+				 pl->supported_interfaces,
+				 pcs->supported_interfaces);
+
+	phylink_run_resolve(pl);
+}
+EXPORT_SYMBOL_GPL(phylink_release_pcs);
+
 static unsigned int phylink_pcs_inband_caps(struct phylink_pcs *pcs,
 				    phy_interface_t interface)
 {
@@ -1688,6 +1738,10 @@ static void phylink_resolve(struct work_struct *w)
 		if (pl->phydev)
 			link_state.link &= pl->phy_state.link;
 
+		/* Force mac_config if we need to reconfig the interface */
+		if (pl->reconfig_interface)
+			mac_config = true;
+
 		/* Only update if the PHY link is up */
 		if (pl->phydev && pl->phy_state.link) {
 			/* If the interface has changed, force a link down
@@ -1722,7 +1776,8 @@ static void phylink_resolve(struct work_struct *w)
 		phylink_apply_manual_flow(pl, &link_state);
 
 	if (mac_config) {
-		if (link_state.interface != pl->link_config.interface) {
+		if (link_state.interface != pl->link_config.interface ||
+		    pl->reconfig_interface) {
 			/* The interface has changed, force the link down and
 			 * then reconfigure.
 			 */
@@ -1732,6 +1787,7 @@ static void phylink_resolve(struct work_struct *w)
 			}
 			phylink_major_config(pl, false, &link_state);
 			pl->link_config.interface = link_state.interface;
+			pl->reconfig_interface = false;
 		}
 	}
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index ef0b5a0729c8..c5496c063b6a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -717,6 +717,8 @@ void phylink_disconnect_phy(struct phylink *);
 int phylink_set_fixed_link(struct phylink *,
 			   const struct phylink_link_state *);
 
+void phylink_release_pcs(struct phylink_pcs *pcs);
+
 void phylink_mac_change(struct phylink *, bool up);
 void phylink_pcs_change(struct phylink_pcs *, bool up);
 
-- 
2.48.1

[net-next PATCH v4 05/11] net: pcs: implement Firmware node support for PCS driver

[Christian Marangi]

Implement the foundation of Firmware node support for PCS driver.

To support this, implement a simple Provider API where a PCS driver can
expose multiple PCS with an xlate .get function.

PCS driver will have to call fwnode_pcs_add_provider() and pass the
firmware node pointer and a xlate function to return the correct PCS for
the passed #pcs-cells.

This will register the PCS in a global list of providers so that
consumer can access it.

The consumer will then use fwnode_pcs_get() to get the actual PCS by
passing the firmware node pointer and the index for #pcs-cells.

For a simple implementation where #pcs-cells is 0 and the PCS driver
expose a single PCS, the xlate function fwnode_pcs_simple_get() is
provided.

For an advanced implementation a custom xlate function is required.

One removal the PCS driver should first delete itself from the provider
list using fwnode_pcs_del_provider() and then call phylink_release_pcs()
on every PCS the driver provides.

A generic function fwnode_phylink_pcs_parse() is provided for MAC driver
that will declare PCS in DT (or ACPI).
This function will parse "pcs-handle" property and fill the passed array
with the parsed PCS in availabel_pcs up to the passed num_pcs value.
It's also possible to pass NULL as array to only parse the PCS and
update the num_pcs value with the count of scanned PCS.

Co-developed-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/pcs/Kconfig          |   6 +
 drivers/net/pcs/Makefile         |   1 +
 drivers/net/pcs/pcs.c            | 201 +++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-provider.h |  41 +++++++
 include/linux/pcs/pcs.h          |  56 +++++++++
 5 files changed, 305 insertions(+)
 create mode 100644 drivers/net/pcs/pcs.c
 create mode 100644 include/linux/pcs/pcs-provider.h
 create mode 100644 include/linux/pcs/pcs.h

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index f6aa437473de..0d54bea1f663 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -5,6 +5,12 @@
 
 menu "PCS device drivers"
 
+config FWNODE_PCS
+	tristate
+	depends on (ACPI || OF)
+	help
+		Firmware node PCS accessors
+
 config PCS_XPCS
 	tristate "Synopsys DesignWare Ethernet XPCS"
 	select PHYLINK
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..3005cdd89ab7 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers
 
+obj-$(CONFIG_FWNODE_PCS)	+= pcs.o
 pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
 				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
 
diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
new file mode 100644
index 000000000000..26d07a2edfce
--- /dev/null
+++ b/drivers/net/pcs/pcs.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/phylink.h>
+#include <linux/pcs/pcs.h>
+#include <linux/pcs/pcs-provider.h>
+
+MODULE_DESCRIPTION("PCS library");
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_LICENSE("GPL");
+
+struct fwnode_pcs_provider {
+	struct list_head link;
+
+	struct fwnode_handle *fwnode;
+	struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
+				   void *data);
+
+	void *data;
+};
+
+static LIST_HEAD(fwnode_pcs_providers);
+static DEFINE_MUTEX(fwnode_pcs_mutex);
+
+struct phylink_pcs *fwnode_pcs_simple_get(struct fwnode_reference_args *pcsspec,
+					  void *data)
+{
+	return data;
+}
+EXPORT_SYMBOL_GPL(fwnode_pcs_simple_get);
+
+int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
+			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
+						       void *data),
+			    void *data)
+{
+	struct fwnode_pcs_provider *pp;
+
+	if (!fwnode)
+		return 0;
+
+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
+	if (!pp)
+		return -ENOMEM;
+
+	pp->fwnode = fwnode_handle_get(fwnode);
+	pp->data = data;
+	pp->get = get;
+
+	mutex_lock(&fwnode_pcs_mutex);
+	list_add(&pp->link, &fwnode_pcs_providers);
+	mutex_unlock(&fwnode_pcs_mutex);
+	pr_debug("Added pcs provider from %pfwf\n", fwnode);
+
+	fwnode_dev_initialized(fwnode, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_pcs_add_provider);
+
+void fwnode_pcs_del_provider(struct fwnode_handle *fwnode)
+{
+	struct fwnode_pcs_provider *pp;
+
+	if (!fwnode)
+		return;
+
+	mutex_lock(&fwnode_pcs_mutex);
+	list_for_each_entry(pp, &fwnode_pcs_providers, link) {
+		if (pp->fwnode == fwnode) {
+			list_del(&pp->link);
+			fwnode_dev_initialized(pp->fwnode, false);
+			fwnode_handle_put(pp->fwnode);
+			kfree(pp);
+			break;
+		}
+	}
+	mutex_unlock(&fwnode_pcs_mutex);
+}
+EXPORT_SYMBOL_GPL(fwnode_pcs_del_provider);
+
+static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode, int index,
+				const char *name,
+				struct fwnode_reference_args *out_args)
+{
+	int ret;
+
+	if (!fwnode)
+		return -ENOENT;
+
+	if (name)
+		index = fwnode_property_match_string(fwnode, "pcs-names",
+						     name);
+
+	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
+						 "#pcs-cells",
+						 -1, index, out_args);
+	if (ret || (name && index < 0))
+		return ret;
+
+	return 0;
+}
+
+static struct phylink_pcs *
+fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
+{
+	struct fwnode_pcs_provider *provider;
+	struct phylink_pcs *pcs = ERR_PTR(-EPROBE_DEFER);
+
+	if (!pcsspec)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&fwnode_pcs_mutex);
+	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
+		if (provider->fwnode == pcsspec->fwnode) {
+			pcs = provider->get(pcsspec, provider->data);
+			if (!IS_ERR(pcs))
+				break;
+		}
+	}
+	mutex_unlock(&fwnode_pcs_mutex);
+
+	return pcs;
+}
+
+static struct phylink_pcs *__fwnode_pcs_get(struct fwnode_handle *fwnode,
+					    int index, const char *con_id)
+{
+	struct fwnode_reference_args pcsspec;
+	struct phylink_pcs *pcs;
+	int ret;
+
+	ret = fwnode_parse_pcsspec(fwnode, index, con_id, &pcsspec);
+	if (ret)
+		return ERR_PTR(ret);
+
+	pcs = fwnode_pcs_get_from_pcsspec(&pcsspec);
+	fwnode_handle_put(pcsspec.fwnode);
+
+	return pcs;
+}
+
+struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode, int index)
+{
+	return __fwnode_pcs_get(fwnode, index, NULL);
+}
+EXPORT_SYMBOL_GPL(fwnode_pcs_get);
+
+static int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode,
+				    unsigned int *num_pcs)
+{
+	struct fwnode_reference_args out_args;
+	int index = 0;
+	int ret;
+
+	while (true) {
+		ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
+							 "#pcs-cells",
+							 -1, index, &out_args);
+		/* We expect to reach an -ENOENT error while counting */
+		if (ret)
+			break;
+
+		fwnode_handle_put(out_args.fwnode);
+		index++;
+	}
+
+	/* Update num_pcs with parsed PCS */
+	*num_pcs = index;
+
+	/* Return error if we didn't found any PCS */
+	return index > 0 ? 0 : -ENOENT;
+}
+
+int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
+			     struct phylink_pcs **available_pcs,
+			     unsigned int *num_pcs)
+{
+	int i;
+
+	if (!fwnode_property_present(fwnode, "pcs-handle"))
+		return -ENODEV;
+
+	/* With available_pcs NULL, only count the PCS */
+	if (!available_pcs)
+		return fwnode_phylink_pcs_count(fwnode, num_pcs);
+
+	for (i = 0; i < *num_pcs; i++) {
+		struct phylink_pcs *pcs;
+
+		pcs = fwnode_pcs_get(fwnode, i);
+		if (IS_ERR(pcs))
+			return PTR_ERR(pcs);
+
+		available_pcs[i] = pcs;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);
diff --git a/include/linux/pcs/pcs-provider.h b/include/linux/pcs/pcs-provider.h
new file mode 100644
index 000000000000..ae51c108147e
--- /dev/null
+++ b/include/linux/pcs/pcs-provider.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __LINUX_PCS_PROVIDER_H
+#define __LINUX_PCS_PROVIDER_H
+
+/**
+ * fwnode_pcs_simple_get - Simple xlate function to retrieve PCS
+ * @pcsspec: reference arguments
+ * @data: Context data (assumed assigned to the single PCS)
+ *
+ * Returns: the PCS pointed by data.
+ */
+struct phylink_pcs *fwnode_pcs_simple_get(struct fwnode_reference_args *pcsspec,
+					  void *data);
+
+/**
+ * fwnode_pcs_add_provider - Registers a new PCS provider
+ * @fwnode: Firmware node
+ * @get: xlate function to retrieve the PCS
+ * @data: Context data
+ *
+ * Register and add a new PCS to the global providers list
+ * for the firmware node. A function to get the PCS from
+ * firmware node with the use fwnode reference arguments.
+ * To the get function is also passed the interface type
+ * requested for the PHY. PCS driver will use the passed
+ * interface to understand if the PCS can support it or not.
+ *
+ * Returns: 0 on success or -ENOMEM on allocation failure.
+ */
+int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
+			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
+						       void *data),
+			    void *data);
+
+/**
+ * fwnode_pcs_del_provider - Removes a PCS provider
+ * @fwnode: Firmware node
+ */
+void fwnode_pcs_del_provider(struct fwnode_handle *fwnode);
+
+#endif /* __LINUX_PCS_PROVIDER_H */
diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
new file mode 100644
index 000000000000..33244e3a442b
--- /dev/null
+++ b/include/linux/pcs/pcs.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __LINUX_PCS_H
+#define __LINUX_PCS_H
+
+#include <linux/phylink.h>
+
+#if IS_ENABLED(CONFIG_FWNODE_PCS)
+/**
+ * fwnode_pcs_get - Retrieves a PCS from a firmware node
+ * @fwnode: firmware node
+ * @index: index fwnode PCS handle in firmware node
+ *
+ * Get a PCS from the firmware node at index.
+ *
+ * Returns: a pointer to the phylink_pcs or a negative
+ * error pointer. Can return -EPROBE_DEFER if the PCS is not
+ * present in global providers list (either due to driver
+ * still needs to be probed or it failed to probe/removed)
+ */
+struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
+				   int index);
+
+/**
+ * fwnode_phylink_pcs_parse - generic PCS parse for fwnode PCS provider
+ * @fwnode: firmware node
+ * @available_pcs: pointer to preallocated array of PCS
+ * @num_pcs: where to store count of parsed PCS
+ *
+ * Generic helper function to fill available_pcs array with PCS parsed
+ * from a "pcs-handle" fwnode property defined in firmware node up to
+ * passed num_pcs.
+ *
+ * If available_pcs is NULL, num_pcs is updated with the count of the
+ * parsed PCS.
+ *
+ * Returns: 0 or a negative error.
+ */
+int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
+			     struct phylink_pcs **available_pcs,
+			     unsigned int *num_pcs);
+#else
+static inline struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
+						 int index)
+{
+	return ERR_PTR(-ENOENT);
+}
+
+static inline int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
+					   struct phylink_pcs **available_pcs,
+					   unsigned int *num_pcs)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+#endif /* __LINUX_PCS_H */
-- 
2.48.1

[net-next PATCH v4 06/11] net: phylink: support late PCS provider attach

[Christian Marangi]

Add support for late PCS provider attachment to a phylink instance.
This works by creating a global notifier for the PCS provider and
making each phylink instance that makes use of fwnode subscribe to
this notifier.

The PCS notifier will emit the event FWNODE_PCS_PROVIDER_ADD every time
a new PCS provider is added.

phylink will then react to this event and will call the new function
fwnode_phylink_pcs_get_from_fwnode() that will check if the PCS fwnode
provided by the event is present in the phy-handle property of the
phylink instance.

If a related PCS is found, then such PCS is added to the phylink
instance PCS list.

Then we link the PCS to the phylink instance if it's not disable and we
refresh the supported interfaces of the phylink instance.

Finally we check if we are in a major_config_failed scenario and trigger
an interface reconfiguration in the next phylink resolve.

In the example scenario where the link was previously torn down due to
removal of PCS, the link will be established again as the PCS came back
and is now available to phylink.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/pcs/pcs.c     | 40 ++++++++++++++++++++++++++++++
 drivers/net/phy/phylink.c | 52 +++++++++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs.h   | 48 ++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)

diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
index 26d07a2edfce..409d06658167 100644
--- a/drivers/net/pcs/pcs.c
+++ b/drivers/net/pcs/pcs.c
@@ -22,6 +22,13 @@ struct fwnode_pcs_provider {
 
 static LIST_HEAD(fwnode_pcs_providers);
 static DEFINE_MUTEX(fwnode_pcs_mutex);
+static BLOCKING_NOTIFIER_HEAD(fwnode_pcs_notify_list);
+
+int register_fwnode_pcs_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&fwnode_pcs_notify_list, nb);
+}
+EXPORT_SYMBOL_GPL(register_fwnode_pcs_notifier);
 
 struct phylink_pcs *fwnode_pcs_simple_get(struct fwnode_reference_args *pcsspec,
 					  void *data)
@@ -55,6 +62,10 @@ int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
 
 	fwnode_dev_initialized(fwnode, true);
 
+	blocking_notifier_call_chain(&fwnode_pcs_notify_list,
+				     FWNODE_PCS_PROVIDER_ADD,
+				     fwnode);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fwnode_pcs_add_provider);
@@ -147,6 +158,35 @@ struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode, int index)
 }
 EXPORT_SYMBOL_GPL(fwnode_pcs_get);
 
+struct phylink_pcs *
+fwnode_phylink_pcs_get_from_fwnode(struct fwnode_handle *fwnode,
+				   struct fwnode_handle *pcs_fwnode)
+{
+	struct fwnode_reference_args pcsspec;
+	int index = 0;
+	int ret;
+
+	/* Loop until we find a matching PCS node or
+	 * fwnode_parse_pcsspec() returns error
+	 * if we don't have any other PCS reference to check.
+	 */
+	while (true) {
+		ret = fwnode_parse_pcsspec(fwnode, index, NULL, &pcsspec);
+		if (ret)
+			return ERR_PTR(ret);
+
+		/* Exit loop if we found the matching PCS node */
+		if (pcsspec.fwnode == pcs_fwnode)
+			break;
+
+		/* Check the next PCS reference */
+		index++;
+	}
+
+	return fwnode_pcs_get(fwnode, index);
+}
+EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_get_from_fwnode);
+
 static int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode,
 				    unsigned int *num_pcs)
 {
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 2f28c4c83062..1a4df0d24aa2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -12,6 +12,7 @@
 #include <linux/netdevice.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
+#include <linux/pcs/pcs.h>
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
 #include <linux/phylink.h>
@@ -61,6 +62,7 @@ struct phylink {
 
 	/* List of available PCS */
 	struct list_head pcs_list;
+	struct notifier_block fwnode_pcs_nb;
 
 	/* What interface are supported by the current link.
 	 * Can change on removal or addition of new PCS.
@@ -1909,6 +1911,51 @@ int phylink_set_fixed_link(struct phylink *pl,
 }
 EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
 
+static int pcs_provider_notify(struct notifier_block *self,
+			       unsigned long val, void *data)
+{
+	struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);
+	struct fwnode_handle *pcs_fwnode = data;
+	struct phylink_pcs *pcs;
+
+	/* Check if the just added PCS provider is
+	 * in the phylink instance phy-handle property
+	 */
+	pcs = fwnode_phylink_pcs_get_from_fwnode(dev_fwnode(pl->config->dev),
+						 pcs_fwnode);
+	if (IS_ERR(pcs))
+		return NOTIFY_DONE;
+
+	/* Add the PCS */
+	rtnl_lock();
+
+	list_add(&pcs->list, &pl->pcs_list);
+
+	/* Link phylink if we are started */
+	if (!pl->phylink_disable_state)
+		pcs->phylink = pl;
+
+	/* Refresh supported interfaces */
+	phy_interface_copy(pl->supported_interfaces,
+			   pl->config->supported_interfaces);
+	list_for_each_entry(pcs, &pl->pcs_list, list)
+		phy_interface_or(pl->supported_interfaces,
+				 pl->supported_interfaces,
+				 pcs->supported_interfaces);
+
+	mutex_lock(&pl->state_mutex);
+	/* Force an interface reconfig if major config fail */
+	if (pl->major_config_failed)
+		pl->reconfig_interface = true;
+	mutex_unlock(&pl->state_mutex);
+
+	rtnl_unlock();
+
+	phylink_run_resolve(pl);
+
+	return NOTIFY_OK;
+}
+
 /**
  * phylink_create() - create a phylink instance
  * @config: a pointer to the target &struct phylink_config
@@ -1963,6 +2010,11 @@ struct phylink *phylink_create(struct phylink_config *config,
 				 pl->supported_interfaces,
 				 pcs->supported_interfaces);
 
+	if (!phy_interface_empty(config->pcs_interfaces)) {
+		pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
+		register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
+	}
+
 	pl->config = config;
 	if (config->type == PHYLINK_NETDEV) {
 		pl->netdev = to_net_dev(config->dev);
diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
index 33244e3a442b..dfd3dc0f86f6 100644
--- a/include/linux/pcs/pcs.h
+++ b/include/linux/pcs/pcs.h
@@ -4,7 +4,24 @@
 
 #include <linux/phylink.h>
 
+enum fwnode_pcs_notify_event {
+	FWNODE_PCS_PROVIDER_ADD,
+};
+
 #if IS_ENABLED(CONFIG_FWNODE_PCS)
+/**
+ * register_fwnode_pcs_notifier - Register a notifier block for fwnode
+ *				  PCS events
+ * @nb: pointer to the notifier block
+ *
+ * Registers a notifier block to the fwnode_pcs_notify_list blocking
+ * notifier chain. This allows phylink instance to subscribe for
+ * PCS provider events.
+ *
+ * Returns: 0 or a negative error.
+ */
+int register_fwnode_pcs_notifier(struct notifier_block *nb);
+
 /**
  * fwnode_pcs_get - Retrieves a PCS from a firmware node
  * @fwnode: firmware node
@@ -20,6 +37,25 @@
 struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
 				   int index);
 
+/**
+ * fwnode_phylink_pcs_get_from_fwnode - Retrieves the PCS provided
+ *					by the firmware node from a
+ *					firmware node
+ * @fwnode: firmware node
+ * @pcs_fwnode: PCS firmware node
+ *
+ * Parse 'pcs-handle' in 'fwnode' and get the PCS that match
+ * 'pcs_fwnode' firmware node.
+ *
+ * Returns: a pointer to the phylink_pcs or a negative
+ * error pointer. Can return -EPROBE_DEFER if the PCS is not
+ * present in global providers list (either due to driver
+ * still needs to be probed or it failed to probe/removed)
+ */
+struct phylink_pcs *
+fwnode_phylink_pcs_get_from_fwnode(struct fwnode_handle *fwnode,
+				   struct fwnode_handle *pcs_fwnode);
+
 /**
  * fwnode_phylink_pcs_parse - generic PCS parse for fwnode PCS provider
  * @fwnode: firmware node
@@ -39,12 +75,24 @@ int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
 			     struct phylink_pcs **available_pcs,
 			     unsigned int *num_pcs);
 #else
+static inline int register_fwnode_pcs_notifier(struct notifier_block *nb)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
 						 int index)
 {
 	return ERR_PTR(-ENOENT);
 }
 
+static inline struct phylink_pcs *
+fwnode_phylink_pcs_get_from_fwnode(struct fwnode_handle *fwnode,
+				   struct fwnode_handle *pcs_fwnode)
+{
+	return ERR_PTR(-ENOENT);
+}
+
 static inline int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
 					   struct phylink_pcs **available_pcs,
 					   unsigned int *num_pcs)
-- 
2.48.1

[net-next PATCH v4 07/11] dt-bindings: net: ethernet-controller: permit to define multiple PCS

[Christian Marangi]

Drop the limitation of a single PCS in pcs-handle property. Multiple PCS
can be defined for an ethrnet-controller node to support various PHY
interface mode type.

It's very common for SoCs to have a 2 or more dedicated PCS for Base-X
(for example SGMII, 1000base-x, 2500base-x, ...) and Base-R (for example
USXGMII,10base-r, ...) with the MAC selecting one of the other based on
the attached PHY.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/net/ethernet-controller.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 7cbf11bbe99c..60605b34d242 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -84,8 +84,6 @@ properties:
 
   pcs-handle:
     $ref: /schemas/types.yaml#/definitions/phandle-array
-    items:
-      maxItems: 1
     description:
       Specifies a reference to a node representing a PCS PHY device on a MDIO
       bus to link with an external PHY (phy-handle) if exists.
-- 
2.48.1

[net-next PATCH v4 08/11] net: phylink: add .pcs_link_down PCS OP

[Christian Marangi]

Permit for PCS driver to define specific operation to torn down the link
between the MAC and the PCS.

This might be needed for some PCS that reset counter or require special
reset to correctly work if the link needs to be restored later.

On phylink_link_down() call, the additional phylink_pcs_link_down() will
be called before .mac_link_down to torn down the link.

PCS driver will need to define .pcs_link_down to make use of this.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phylink.c | 8 ++++++++
 include/linux/phylink.h   | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1a4df0d24aa2..39cd15e30598 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1009,6 +1009,12 @@ static void phylink_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 		pcs->ops->pcs_link_up(pcs, neg_mode, interface, speed, duplex);
 }
 
+static void phylink_pcs_link_down(struct phylink_pcs *pcs)
+{
+	if (pcs && pcs->ops->pcs_link_down)
+		pcs->ops->pcs_link_down(pcs);
+}
+
 static void phylink_pcs_disable_eee(struct phylink_pcs *pcs)
 {
 	if (pcs && pcs->ops->pcs_disable_eee)
@@ -1686,6 +1692,8 @@ static void phylink_link_down(struct phylink *pl)
 
 	phylink_deactivate_lpi(pl);
 
+	phylink_pcs_link_down(pl->pcs);
+
 	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
 				   pl->cur_interface);
 	phylink_info(pl, "Link is Down\n");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c5496c063b6a..8b3d1dfb83a1 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -494,6 +494,7 @@ struct phylink_pcs {
  * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
  * @pcs_link_up: program the PCS for the resolved link configuration
  *               (where necessary).
+ * @pcs_link_down: torn down link between MAC and PCS.
  * @pcs_disable_eee: optional notification to PCS that EEE has been disabled
  *		     at the MAC.
  * @pcs_enable_eee: optional notification to PCS that EEE will be enabled at
@@ -521,6 +522,7 @@ struct phylink_pcs_ops {
 	void (*pcs_an_restart)(struct phylink_pcs *pcs);
 	void (*pcs_link_up)(struct phylink_pcs *pcs, unsigned int neg_mode,
 			    phy_interface_t interface, int speed, int duplex);
+	void (*pcs_link_down)(struct phylink_pcs *pcs);
 	void (*pcs_disable_eee)(struct phylink_pcs *pcs);
 	void (*pcs_enable_eee)(struct phylink_pcs *pcs);
 	int (*pcs_pre_init)(struct phylink_pcs *pcs);
-- 
2.48.1

[net-next PATCH v4 09/11] dt-bindings: net: pcs: Document support for Airoha Ethernet PCS

[Christian Marangi]

Document support for Airoha Ethernet PCS for AN7581 SoC.

Airoha AN7581 SoC expose multiple Physical Coding Sublayer (PCS) for
the various Serdes port supporting different Media Independent Interface
(10BASE-R, USXGMII, 2500BASE-X, 1000BASE-X, SGMII).

This follow the new PCS provider with the use of #pcs-cells property.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/net/pcs/airoha,pcs.yaml          | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml

diff --git a/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml b/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
new file mode 100644
index 000000000000..8bcf7757c728
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/airoha,pcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha Ethernet PCS and Serdes
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+
+description:
+  Airoha AN7581 SoC expose multiple Physical Coding Sublayer (PCS) for
+  the various Serdes port supporting different Media Independent Interface
+  (10BASE-R, USXGMII, 2500BASE-X, 1000BASE-X, SGMII).
+
+properties:
+  compatible:
+    enum:
+      - airoha,an7581-pcs-eth
+      - airoha,an7581-pcs-pon
+
+  reg:
+    items:
+      - description: XFI MAC reg
+      - description: HSGMII AN reg
+      - description: HSGMII PCS reg
+      - description: MULTI SGMII reg
+      - description: USXGMII reg
+      - description: HSGMII rate adaption reg
+      - description: XFI Analog register
+      - description: XFI PMA (Physical Medium Attachment) register
+
+  reg-names:
+    items:
+      - const: xfi_mac
+      - const: hsgmii_an
+      - const: hsgmii_pcs
+      - const: multi_sgmii
+      - const: usxgmii
+      - const: hsgmii_rate_adp
+      - const: xfi_ana
+      - const: xfi_pma
+
+  resets:
+    items:
+      - description: MAC reset
+      - description: PHY reset
+
+  reset-names:
+    items:
+      - const: mac
+      - const: phy
+
+  "#pcs-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - resets
+  - reset-names
+  - "#pcs-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/airoha,en7581-reset.h>
+
+    pcs@1fa08000 {
+        compatible = "airoha,an7581-pcs-pon";
+        reg = <0x1fa08000 0x1000>,
+              <0x1fa80000 0x60>,
+              <0x1fa80a00 0x164>,
+              <0x1fa84000 0x450>,
+              <0x1fa85900 0x338>,
+              <0x1fa86000 0x300>,
+              <0x1fa8a000 0x1000>,
+              <0x1fa8b000 0x1000>;
+        reg-names = "xfi_mac", "hsgmii_an", "hsgmii_pcs",
+                    "multi_sgmii", "usxgmii",
+                    "hsgmii_rate_adp", "xfi_ana", "xfi_pma";
+
+        resets = <&scuclk EN7581_XPON_MAC_RST>,
+                 <&scuclk EN7581_XPON_PHY_RST>;
+        reset-names = "mac", "phy";
+
+        #pcs-cells = <0>;
+    };
+
+    pcs@1fa09000 {
+        compatible = "airoha,an7581-pcs-eth";
+        reg = <0x1fa09000 0x1000>,
+              <0x1fa70000 0x60>,
+              <0x1fa70a00 0x164>,
+              <0x1fa74000 0x450>,
+              <0x1fa75900 0x338>,
+              <0x1fa76000 0x300>,
+              <0x1fa7a000 0x1000>,
+              <0x1fa7b000 0x1000>;
+        reg-names = "xfi_mac", "hsgmii_an", "hsgmii_pcs",
+                    "multi_sgmii", "usxgmii",
+                    "hsgmii_rate_adp", "xfi_ana", "xfi_pma";
+
+        resets = <&scuclk EN7581_XSI_MAC_RST>,
+                 <&scuclk EN7581_XSI_PHY_RST>;
+        reset-names = "mac", "phy";
+
+        #pcs-cells = <0>;
+    };
-- 
2.48.1

[net-next PATCH v4 11/11] net: airoha: add phylink support for GDM2/3/4

[Christian Marangi]

Add phylink support for GDM2/3/4 port that require configuration of the
PCS to make the external PHY or attached SFP cage work.

These needs to be defined in the GDM port node using the pcs-handle
property.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c  | 155 +++++++++++++++++++++-
 drivers/net/ethernet/airoha/airoha_eth.h  |   3 +
 drivers/net/ethernet/airoha/airoha_regs.h |  12 ++
 3 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 16c7896f931f..3be1ae077a76 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -7,6 +7,7 @@
 #include <linux/of_net.h>
 #include <linux/platform_device.h>
 #include <linux/tcp.h>
+#include <linux/pcs/pcs.h>
 #include <linux/u64_stats_sync.h>
 #include <net/dst_metadata.h>
 #include <net/page_pool/helpers.h>
@@ -79,6 +80,11 @@ static bool airhoa_is_lan_gdm_port(struct airoha_gdm_port *port)
 	return port->id == 1;
 }
 
+static bool airhoa_is_phy_external(struct airoha_gdm_port *port)
+{
+	return port->id != 1;
+}
+
 static void airoha_set_macaddr(struct airoha_gdm_port *port, const u8 *addr)
 {
 	struct airoha_eth *eth = port->qdma->eth;
@@ -1613,6 +1619,17 @@ static int airoha_dev_open(struct net_device *dev)
 	struct airoha_gdm_port *port = netdev_priv(dev);
 	struct airoha_qdma *qdma = port->qdma;
 
+	if (airhoa_is_phy_external(port)) {
+		err = phylink_of_phy_connect(port->phylink, dev->dev.of_node, 0);
+		if (err) {
+			netdev_err(dev, "%s: could not attach PHY: %d\n", __func__,
+				   err);
+			return err;
+		}
+
+		phylink_start(port->phylink);
+	}
+
 	netif_tx_start_all_queues(dev);
 	err = airoha_set_vip_for_gdm_port(port, true);
 	if (err)
@@ -1665,6 +1682,11 @@ static int airoha_dev_stop(struct net_device *dev)
 		}
 	}
 
+	if (airhoa_is_phy_external(port)) {
+		phylink_stop(port->phylink);
+		phylink_disconnect_phy(port->phylink);
+	}
+
 	return 0;
 }
 
@@ -2795,6 +2817,115 @@ bool airoha_is_valid_gdm_port(struct airoha_eth *eth,
 	return false;
 }
 
+static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
+			       unsigned int mode, phy_interface_t interface,
+			       int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct airoha_gdm_port *port = container_of(config, struct airoha_gdm_port,
+						    phylink_config);
+	struct airoha_qdma *qdma = port->qdma;
+	struct airoha_eth *eth = qdma->eth;
+	u32 frag_size_tx, frag_size_rx;
+
+	if (port->id != 4)
+		return;
+
+	switch (speed) {
+	case SPEED_10000:
+	case SPEED_5000:
+		frag_size_tx = 8;
+		frag_size_rx = 8;
+		break;
+	case SPEED_2500:
+		frag_size_tx = 2;
+		frag_size_rx = 1;
+		break;
+	default:
+		frag_size_tx = 1;
+		frag_size_rx = 0;
+	}
+
+	/* Configure TX/RX frag based on speed */
+	airoha_fe_rmw(eth, REG_GDMA4_TMBI_FRAG,
+		      GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
+		      FIELD_PREP(GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
+				 frag_size_tx));
+
+	airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG,
+		      GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
+		      FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
+				 frag_size_rx));
+}
+
+static const struct phylink_mac_ops airoha_phylink_ops = {
+	.mac_link_up = airoha_mac_link_up,
+};
+
+static int airoha_setup_phylink(struct net_device *dev)
+{
+	struct airoha_gdm_port *port = netdev_priv(dev);
+	struct device_node *np = dev->dev.of_node;
+	struct phylink_pcs **available_pcs;
+	phy_interface_t phy_mode;
+	struct phylink *phylink;
+	unsigned int num_pcs;
+	int err;
+
+	err = of_get_phy_mode(np, &phy_mode);
+	if (err) {
+		dev_err(&dev->dev, "incorrect phy-mode\n");
+		return err;
+	}
+
+	port->phylink_config.dev = &dev->dev;
+	port->phylink_config.type = PHYLINK_NETDEV;
+	port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+						MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD |
+						MAC_5000FD | MAC_10000FD;
+
+	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), NULL, &num_pcs);
+	if (err)
+		return err;
+
+	available_pcs = kcalloc(num_pcs, sizeof(*available_pcs), GFP_KERNEL);
+	if (!available_pcs)
+		return -ENOMEM;
+
+	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), available_pcs,
+				       &num_pcs);
+	if (err)
+		goto out;
+
+	port->phylink_config.available_pcs = available_pcs;
+	port->phylink_config.num_available_pcs = num_pcs;
+
+	__set_bit(PHY_INTERFACE_MODE_SGMII,
+		  port->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+		  port->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+		  port->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_USXGMII,
+		  port->phylink_config.supported_interfaces);
+
+	phy_interface_copy(port->phylink_config.pcs_interfaces,
+			   port->phylink_config.supported_interfaces);
+
+	phylink = phylink_create(&port->phylink_config,
+				 of_fwnode_handle(np),
+				 phy_mode, &airoha_phylink_ops);
+	if (IS_ERR(phylink)) {
+		err = PTR_ERR(phylink);
+		goto out;
+	}
+
+	port->phylink = phylink;
+out:
+	kfree(available_pcs);
+
+	return err;
+}
+
 static int airoha_alloc_gdm_port(struct airoha_eth *eth,
 				 struct device_node *np, int index)
 {
@@ -2873,7 +3004,23 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
 	if (err)
 		return err;
 
-	return register_netdev(dev);
+	if (airhoa_is_phy_external(port)) {
+		err = airoha_setup_phylink(dev);
+		if (err)
+			return err;
+	}
+
+	err = register_netdev(dev);
+	if (err)
+		goto free_phylink;
+
+	return 0;
+
+free_phylink:
+	if (airhoa_is_phy_external(port))
+		phylink_destroy(port->phylink);
+
+	return err;
 }
 
 static int airoha_probe(struct platform_device *pdev)
@@ -2967,6 +3114,9 @@ static int airoha_probe(struct platform_device *pdev)
 		struct airoha_gdm_port *port = eth->ports[i];
 
 		if (port && port->dev->reg_state == NETREG_REGISTERED) {
+			if (airhoa_is_phy_external(port))
+				phylink_destroy(port->phylink);
+
 			unregister_netdev(port->dev);
 			airoha_metadata_dst_free(port);
 		}
@@ -2994,6 +3144,9 @@ static void airoha_remove(struct platform_device *pdev)
 			continue;
 
 		airoha_dev_stop(port->dev);
+		if (airhoa_is_phy_external(port))
+			phylink_destroy(port->phylink);
+
 		unregister_netdev(port->dev);
 		airoha_metadata_dst_free(port);
 	}
diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index 53f39083a8b0..73a500474076 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -498,6 +498,9 @@ struct airoha_gdm_port {
 	struct net_device *dev;
 	int id;
 
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
+
 	struct airoha_hw_stats stats;
 
 	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
index d931530fc96f..54f7079b28b0 100644
--- a/drivers/net/ethernet/airoha/airoha_regs.h
+++ b/drivers/net/ethernet/airoha/airoha_regs.h
@@ -357,6 +357,18 @@
 #define IP_FRAGMENT_PORT_MASK		GENMASK(8, 5)
 #define IP_FRAGMENT_NBQ_MASK		GENMASK(4, 0)
 
+#define REG_GDMA4_TMBI_FRAG		0x2028
+#define GDMA4_SGMII1_TX_WEIGHT_MASK	GENMASK(31, 26)
+#define GDMA4_SGMII1_TX_FRAG_SIZE_MASK	GENMASK(25, 16)
+#define GDMA4_SGMII0_TX_WEIGHT_MASK	GENMASK(15, 10)
+#define GDMA4_SGMII0_TX_FRAG_SIZE_MASK	GENMASK(9, 0)
+
+#define REG_GDMA4_RMBI_FRAG		0x202c
+#define GDMA4_SGMII1_RX_WEIGHT_MASK	GENMASK(31, 26)
+#define GDMA4_SGMII1_RX_FRAG_SIZE_MASK	GENMASK(25, 16)
+#define GDMA4_SGMII0_RX_WEIGHT_MASK	GENMASK(15, 10)
+#define GDMA4_SGMII0_RX_FRAG_SIZE_MASK	GENMASK(9, 0)
+
 #define REG_MC_VLAN_EN			0x2100
 #define MC_VLAN_EN_MASK			BIT(0)
 
-- 
2.48.1

Re: [net-next PATCH v4 11/11] net: airoha: add phylink support for GDM2/3/4

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8998 bytes --]

> Add phylink support for GDM2/3/4 port that require configuration of the
> PCS to make the external PHY or attached SFP cage work.
> 
> These needs to be defined in the GDM port node using the pcs-handle
> property.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c  | 155 +++++++++++++++++++++-
>  drivers/net/ethernet/airoha/airoha_eth.h  |   3 +
>  drivers/net/ethernet/airoha/airoha_regs.h |  12 ++
>  3 files changed, 169 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 16c7896f931f..3be1ae077a76 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -7,6 +7,7 @@
>  #include <linux/of_net.h>
>  #include <linux/platform_device.h>
>  #include <linux/tcp.h>
> +#include <linux/pcs/pcs.h>
>  #include <linux/u64_stats_sync.h>
>  #include <net/dst_metadata.h>
>  #include <net/page_pool/helpers.h>
> @@ -79,6 +80,11 @@ static bool airhoa_is_lan_gdm_port(struct airoha_gdm_port *port)
>  	return port->id == 1;
>  }
>  
> +static bool airhoa_is_phy_external(struct airoha_gdm_port *port)
> +{
> +	return port->id != 1;
> +}
> +
>  static void airoha_set_macaddr(struct airoha_gdm_port *port, const u8 *addr)
>  {
>  	struct airoha_eth *eth = port->qdma->eth;
> @@ -1613,6 +1619,17 @@ static int airoha_dev_open(struct net_device *dev)
>  	struct airoha_gdm_port *port = netdev_priv(dev);
>  	struct airoha_qdma *qdma = port->qdma;
>  
> +	if (airhoa_is_phy_external(port)) {
> +		err = phylink_of_phy_connect(port->phylink, dev->dev.of_node, 0);

nit: even if it is not strictly required, in order to align with the rest of the
codebase, can you please stay below 79 columns?

> +		if (err) {
> +			netdev_err(dev, "%s: could not attach PHY: %d\n", __func__,
> +				   err);

same here

> +			return err;
> +		}
> +
> +		phylink_start(port->phylink);
> +	}
> +
>  	netif_tx_start_all_queues(dev);
>  	err = airoha_set_vip_for_gdm_port(port, true);
>  	if (err)
> @@ -1665,6 +1682,11 @@ static int airoha_dev_stop(struct net_device *dev)
>  		}
>  	}
>  
> +	if (airhoa_is_phy_external(port)) {
> +		phylink_stop(port->phylink);
> +		phylink_disconnect_phy(port->phylink);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2795,6 +2817,115 @@ bool airoha_is_valid_gdm_port(struct airoha_eth *eth,
>  	return false;
>  }
>  
> +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex, bool tx_pause, bool rx_pause)

ditto.

> +{
> +	struct airoha_gdm_port *port = container_of(config, struct airoha_gdm_port,
> +						    phylink_config);

ditto.

> +	struct airoha_qdma *qdma = port->qdma;
> +	struct airoha_eth *eth = qdma->eth;
> +	u32 frag_size_tx, frag_size_rx;
> +
> +	if (port->id != 4)
> +		return;
> +
> +	switch (speed) {
> +	case SPEED_10000:
> +	case SPEED_5000:
> +		frag_size_tx = 8;
> +		frag_size_rx = 8;
> +		break;
> +	case SPEED_2500:
> +		frag_size_tx = 2;
> +		frag_size_rx = 1;
> +		break;
> +	default:
> +		frag_size_tx = 1;
> +		frag_size_rx = 0;
> +	}
> +
> +	/* Configure TX/RX frag based on speed */
> +	airoha_fe_rmw(eth, REG_GDMA4_TMBI_FRAG,
> +		      GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> +		      FIELD_PREP(GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> +				 frag_size_tx));
> +
> +	airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG,
> +		      GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> +		      FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> +				 frag_size_rx));
> +}
> +
> +static const struct phylink_mac_ops airoha_phylink_ops = {
> +	.mac_link_up = airoha_mac_link_up,
> +};
> +
> +static int airoha_setup_phylink(struct net_device *dev)
> +{
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +	struct device_node *np = dev->dev.of_node;
> +	struct phylink_pcs **available_pcs;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
> +	unsigned int num_pcs;
> +	int err;
> +
> +	err = of_get_phy_mode(np, &phy_mode);
> +	if (err) {
> +		dev_err(&dev->dev, "incorrect phy-mode\n");
> +		return err;
> +	}
> +
> +	port->phylink_config.dev = &dev->dev;
> +	port->phylink_config.type = PHYLINK_NETDEV;
> +	port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +						MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD |
> +						MAC_5000FD | MAC_10000FD;

over 79 columns

> +
> +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), NULL, &num_pcs);
> +	if (err)
> +		return err;
> +
> +	available_pcs = kcalloc(num_pcs, sizeof(*available_pcs), GFP_KERNEL);

I guess you can use devm_kcalloc() and get rid of kfree() here.

> +	if (!available_pcs)
> +		return -ENOMEM;
> +
> +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), available_pcs,
> +				       &num_pcs);
> +	if (err)
> +		goto out;
> +
> +	port->phylink_config.available_pcs = available_pcs;
> +	port->phylink_config.num_available_pcs = num_pcs;
> +
> +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> +		  port->phylink_config.supported_interfaces);
> +
> +	phy_interface_copy(port->phylink_config.pcs_interfaces,
> +			   port->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&port->phylink_config,
> +				 of_fwnode_handle(np),
> +				 phy_mode, &airoha_phylink_ops);
> +	if (IS_ERR(phylink)) {
> +		err = PTR_ERR(phylink);
> +		goto out;
> +	}
> +
> +	port->phylink = phylink;
> +out:
> +	kfree(available_pcs);
> +
> +	return err;
> +}
> +
>  static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  				 struct device_node *np, int index)
>  {
> @@ -2873,7 +3004,23 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  	if (err)
>  		return err;
>  
> -	return register_netdev(dev);
> +	if (airhoa_is_phy_external(port)) {
> +		err = airoha_setup_phylink(dev);

This will re-introduce the issue reported here:
https://lore.kernel.org/netdev/5c94b9b3850f7f29ed653e2205325620df28c3ff.1746715755.git.christophe.jaillet@wanadoo.fr/

> +		if (err)
> +			return err;
> +	}
> +
> +	err = register_netdev(dev);
> +	if (err)
> +		goto free_phylink;
> +
> +	return 0;
> +
> +free_phylink:
> +	if (airhoa_is_phy_external(port))
> +		phylink_destroy(port->phylink);
> +
> +	return err;
>  }
>  
>  static int airoha_probe(struct platform_device *pdev)
> @@ -2967,6 +3114,9 @@ static int airoha_probe(struct platform_device *pdev)
>  		struct airoha_gdm_port *port = eth->ports[i];
>  
>  		if (port && port->dev->reg_state == NETREG_REGISTERED) {
> +			if (airhoa_is_phy_external(port))
> +				phylink_destroy(port->phylink);
> +
>  			unregister_netdev(port->dev);
>  			airoha_metadata_dst_free(port);
>  		}
> @@ -2994,6 +3144,9 @@ static void airoha_remove(struct platform_device *pdev)
>  			continue;
>  
>  		airoha_dev_stop(port->dev);
> +		if (airhoa_is_phy_external(port))
> +			phylink_destroy(port->phylink);
> +
>  		unregister_netdev(port->dev);
>  		airoha_metadata_dst_free(port);
>  	}
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 53f39083a8b0..73a500474076 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -498,6 +498,9 @@ struct airoha_gdm_port {
>  	struct net_device *dev;
>  	int id;
>  
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;
> +
>  	struct airoha_hw_stats stats;
>  
>  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> index d931530fc96f..54f7079b28b0 100644
> --- a/drivers/net/ethernet/airoha/airoha_regs.h
> +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> @@ -357,6 +357,18 @@
>  #define IP_FRAGMENT_PORT_MASK		GENMASK(8, 5)
>  #define IP_FRAGMENT_NBQ_MASK		GENMASK(4, 0)
>  
> +#define REG_GDMA4_TMBI_FRAG		0x2028
> +#define GDMA4_SGMII1_TX_WEIGHT_MASK	GENMASK(31, 26)
> +#define GDMA4_SGMII1_TX_FRAG_SIZE_MASK	GENMASK(25, 16)
> +#define GDMA4_SGMII0_TX_WEIGHT_MASK	GENMASK(15, 10)
> +#define GDMA4_SGMII0_TX_FRAG_SIZE_MASK	GENMASK(9, 0)
> +
> +#define REG_GDMA4_RMBI_FRAG		0x202c
> +#define GDMA4_SGMII1_RX_WEIGHT_MASK	GENMASK(31, 26)
> +#define GDMA4_SGMII1_RX_FRAG_SIZE_MASK	GENMASK(25, 16)
> +#define GDMA4_SGMII0_RX_WEIGHT_MASK	GENMASK(15, 10)
> +#define GDMA4_SGMII0_RX_FRAG_SIZE_MASK	GENMASK(9, 0)
> +
>  #define REG_MC_VLAN_EN			0x2100
>  #define MC_VLAN_EN_MASK			BIT(0)
>  
> -- 
> 2.48.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [net-next PATCH v4 11/11] net: airoha: add phylink support for GDM2/3/4

[Christian Marangi]

On Mon, May 12, 2025 at 11:21:47AM +0200, Lorenzo Bianconi wrote:
> > Add phylink support for GDM2/3/4 port that require configuration of the
> > PCS to make the external PHY or attached SFP cage work.
> > 
> > These needs to be defined in the GDM port node using the pcs-handle
> > property.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/ethernet/airoha/airoha_eth.c  | 155 +++++++++++++++++++++-
> >  drivers/net/ethernet/airoha/airoha_eth.h  |   3 +
> >  drivers/net/ethernet/airoha/airoha_regs.h |  12 ++
> >  3 files changed, 169 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 16c7896f931f..3be1ae077a76 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/of_net.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/tcp.h>
> > +#include <linux/pcs/pcs.h>
> >  #include <linux/u64_stats_sync.h>
> >  #include <net/dst_metadata.h>
> >  #include <net/page_pool/helpers.h>
> > @@ -79,6 +80,11 @@ static bool airhoa_is_lan_gdm_port(struct airoha_gdm_port *port)
> >  	return port->id == 1;
> >  }
> >  
> > +static bool airhoa_is_phy_external(struct airoha_gdm_port *port)
> > +{
> > +	return port->id != 1;
> > +}
> > +
> >  static void airoha_set_macaddr(struct airoha_gdm_port *port, const u8 *addr)
> >  {
> >  	struct airoha_eth *eth = port->qdma->eth;
> > @@ -1613,6 +1619,17 @@ static int airoha_dev_open(struct net_device *dev)
> >  	struct airoha_gdm_port *port = netdev_priv(dev);
> >  	struct airoha_qdma *qdma = port->qdma;
> >  
> > +	if (airhoa_is_phy_external(port)) {
> > +		err = phylink_of_phy_connect(port->phylink, dev->dev.of_node, 0);
> 
> nit: even if it is not strictly required, in order to align with the rest of the
> codebase, can you please stay below 79 columns?
> 
> > +		if (err) {
> > +			netdev_err(dev, "%s: could not attach PHY: %d\n", __func__,
> > +				   err);
> 
> same here
> 
> > +			return err;
> > +		}
> > +
> > +		phylink_start(port->phylink);
> > +	}
> > +
> >  	netif_tx_start_all_queues(dev);
> >  	err = airoha_set_vip_for_gdm_port(port, true);
> >  	if (err)
> > @@ -1665,6 +1682,11 @@ static int airoha_dev_stop(struct net_device *dev)
> >  		}
> >  	}
> >  
> > +	if (airhoa_is_phy_external(port)) {
> > +		phylink_stop(port->phylink);
> > +		phylink_disconnect_phy(port->phylink);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2795,6 +2817,115 @@ bool airoha_is_valid_gdm_port(struct airoha_eth *eth,
> >  	return false;
> >  }
> >  
> > +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> > +			       unsigned int mode, phy_interface_t interface,
> > +			       int speed, int duplex, bool tx_pause, bool rx_pause)
> 
> ditto.
> 
> > +{
> > +	struct airoha_gdm_port *port = container_of(config, struct airoha_gdm_port,
> > +						    phylink_config);
> 
> ditto.
> 
> > +	struct airoha_qdma *qdma = port->qdma;
> > +	struct airoha_eth *eth = qdma->eth;
> > +	u32 frag_size_tx, frag_size_rx;
> > +
> > +	if (port->id != 4)
> > +		return;
> > +
> > +	switch (speed) {
> > +	case SPEED_10000:
> > +	case SPEED_5000:
> > +		frag_size_tx = 8;
> > +		frag_size_rx = 8;
> > +		break;
> > +	case SPEED_2500:
> > +		frag_size_tx = 2;
> > +		frag_size_rx = 1;
> > +		break;
> > +	default:
> > +		frag_size_tx = 1;
> > +		frag_size_rx = 0;
> > +	}
> > +
> > +	/* Configure TX/RX frag based on speed */
> > +	airoha_fe_rmw(eth, REG_GDMA4_TMBI_FRAG,
> > +		      GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> > +		      FIELD_PREP(GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> > +				 frag_size_tx));
> > +
> > +	airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG,
> > +		      GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> > +		      FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> > +				 frag_size_rx));
> > +}
> > +
> > +static const struct phylink_mac_ops airoha_phylink_ops = {
> > +	.mac_link_up = airoha_mac_link_up,
> > +};
> > +
> > +static int airoha_setup_phylink(struct net_device *dev)
> > +{
> > +	struct airoha_gdm_port *port = netdev_priv(dev);
> > +	struct device_node *np = dev->dev.of_node;
> > +	struct phylink_pcs **available_pcs;
> > +	phy_interface_t phy_mode;
> > +	struct phylink *phylink;
> > +	unsigned int num_pcs;
> > +	int err;
> > +
> > +	err = of_get_phy_mode(np, &phy_mode);
> > +	if (err) {
> > +		dev_err(&dev->dev, "incorrect phy-mode\n");
> > +		return err;
> > +	}
> > +
> > +	port->phylink_config.dev = &dev->dev;
> > +	port->phylink_config.type = PHYLINK_NETDEV;
> > +	port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > +						MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD |
> > +						MAC_5000FD | MAC_10000FD;
> 
> over 79 columns
> 
> > +
> > +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), NULL, &num_pcs);
> > +	if (err)
> > +		return err;
> > +
> > +	available_pcs = kcalloc(num_pcs, sizeof(*available_pcs), GFP_KERNEL);
> 
> I guess you can use devm_kcalloc() and get rid of kfree() here.
>

I forgot to answer to this in the previous revision. No devm can't be
used there available_pcs is just an array allocated for phylink_config.

Phylink then copy the data in it and doesn't use it anymore hence it
just needs to be allocated here and doesn't need to stay till the driver
gets removed.

> > +	if (!available_pcs)
> > +		return -ENOMEM;
> > +
> > +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), available_pcs,
> > +				       &num_pcs);
> > +	if (err)
> > +		goto out;
> > +
> > +	port->phylink_config.available_pcs = available_pcs;
> > +	port->phylink_config.num_available_pcs = num_pcs;
> > +
> > +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +		  port->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > +		  port->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +		  port->phylink_config.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> > +		  port->phylink_config.supported_interfaces);
> > +
> > +	phy_interface_copy(port->phylink_config.pcs_interfaces,
> > +			   port->phylink_config.supported_interfaces);
> > +
> > +	phylink = phylink_create(&port->phylink_config,
> > +				 of_fwnode_handle(np),
> > +				 phy_mode, &airoha_phylink_ops);
> > +	if (IS_ERR(phylink)) {
> > +		err = PTR_ERR(phylink);
> > +		goto out;
> > +	}
> > +
> > +	port->phylink = phylink;
> > +out:
> > +	kfree(available_pcs);
> > +
> > +	return err;
> > +}
> > +
> >  static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> >  				 struct device_node *np, int index)
> >  {
> > @@ -2873,7 +3004,23 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> >  	if (err)
> >  		return err;
> >  
> > -	return register_netdev(dev);
> > +	if (airhoa_is_phy_external(port)) {
> > +		err = airoha_setup_phylink(dev);
> 
> This will re-introduce the issue reported here:
> https://lore.kernel.org/netdev/5c94b9b3850f7f29ed653e2205325620df28c3ff.1746715755.git.christophe.jaillet@wanadoo.fr/
> 

I'm confused about this. The suggestion wasn't that register_netdev
might fail and I need to destroy phylink? Or the linked patch was merged
and I need to rebase on top of net-next?

I didn't include that change to not cause conflicts but once it's merged
I will rebase and include that fix.

> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	err = register_netdev(dev);
> > +	if (err)
> > +		goto free_phylink;
> > +
> > +	return 0;
> > +
> > +free_phylink:
> > +	if (airhoa_is_phy_external(port))
> > +		phylink_destroy(port->phylink);
> > +
> > +	return err;
> >  }
> >  
> >  static int airoha_probe(struct platform_device *pdev)
> > @@ -2967,6 +3114,9 @@ static int airoha_probe(struct platform_device *pdev)
> >  		struct airoha_gdm_port *port = eth->ports[i];
> >  
> >  		if (port && port->dev->reg_state == NETREG_REGISTERED) {
> > +			if (airhoa_is_phy_external(port))
> > +				phylink_destroy(port->phylink);
> > +
> >  			unregister_netdev(port->dev);
> >  			airoha_metadata_dst_free(port);
> >  		}
> > @@ -2994,6 +3144,9 @@ static void airoha_remove(struct platform_device *pdev)
> >  			continue;
> >  
> >  		airoha_dev_stop(port->dev);
> > +		if (airhoa_is_phy_external(port))
> > +			phylink_destroy(port->phylink);
> > +
> >  		unregister_netdev(port->dev);
> >  		airoha_metadata_dst_free(port);
> >  	}
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index 53f39083a8b0..73a500474076 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -498,6 +498,9 @@ struct airoha_gdm_port {
> >  	struct net_device *dev;
> >  	int id;
> >  
> > +	struct phylink *phylink;
> > +	struct phylink_config phylink_config;
> > +
> >  	struct airoha_hw_stats stats;
> >  
> >  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> > diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> > index d931530fc96f..54f7079b28b0 100644
> > --- a/drivers/net/ethernet/airoha/airoha_regs.h
> > +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> > @@ -357,6 +357,18 @@
> >  #define IP_FRAGMENT_PORT_MASK		GENMASK(8, 5)
> >  #define IP_FRAGMENT_NBQ_MASK		GENMASK(4, 0)
> >  
> > +#define REG_GDMA4_TMBI_FRAG		0x2028
> > +#define GDMA4_SGMII1_TX_WEIGHT_MASK	GENMASK(31, 26)
> > +#define GDMA4_SGMII1_TX_FRAG_SIZE_MASK	GENMASK(25, 16)
> > +#define GDMA4_SGMII0_TX_WEIGHT_MASK	GENMASK(15, 10)
> > +#define GDMA4_SGMII0_TX_FRAG_SIZE_MASK	GENMASK(9, 0)
> > +
> > +#define REG_GDMA4_RMBI_FRAG		0x202c
> > +#define GDMA4_SGMII1_RX_WEIGHT_MASK	GENMASK(31, 26)
> > +#define GDMA4_SGMII1_RX_FRAG_SIZE_MASK	GENMASK(25, 16)
> > +#define GDMA4_SGMII0_RX_WEIGHT_MASK	GENMASK(15, 10)
> > +#define GDMA4_SGMII0_RX_FRAG_SIZE_MASK	GENMASK(9, 0)
> > +
> >  #define REG_MC_VLAN_EN			0x2100
> >  #define MC_VLAN_EN_MASK			BIT(0)
> >  
> > -- 
> > 2.48.1
> > 



-- 
	Ansuel

Re: [net-next PATCH v4 11/11] net: airoha: add phylink support for GDM2/3/4

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5881 bytes --]

[...]
> > 
> > I guess you can use devm_kcalloc() and get rid of kfree() here.
> >
> 
> I forgot to answer to this in the previous revision. No devm can't be
> used there available_pcs is just an array allocated for phylink_config.
> 
> Phylink then copy the data in it and doesn't use it anymore hence it
> just needs to be allocated here and doesn't need to stay till the driver
> gets removed.

I guess this is exactly the point. Since available_pcs is used just here and
this is not a hot-path, I think the code would be simpler, but I do not have
a strong opinion about it.

> 
> > > +	if (!available_pcs)
> > > +		return -ENOMEM;
> > > +
> > > +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), available_pcs,
> > > +				       &num_pcs);
> > > +	if (err)
> > > +		goto out;
> > > +
> > > +	port->phylink_config.available_pcs = available_pcs;
> > > +	port->phylink_config.num_available_pcs = num_pcs;
> > > +
> > > +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> > > +		  port->phylink_config.supported_interfaces);
> > > +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > > +		  port->phylink_config.supported_interfaces);
> > > +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > > +		  port->phylink_config.supported_interfaces);
> > > +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> > > +		  port->phylink_config.supported_interfaces);
> > > +
> > > +	phy_interface_copy(port->phylink_config.pcs_interfaces,
> > > +			   port->phylink_config.supported_interfaces);
> > > +
> > > +	phylink = phylink_create(&port->phylink_config,
> > > +				 of_fwnode_handle(np),
> > > +				 phy_mode, &airoha_phylink_ops);
> > > +	if (IS_ERR(phylink)) {
> > > +		err = PTR_ERR(phylink);
> > > +		goto out;
> > > +	}
> > > +
> > > +	port->phylink = phylink;
> > > +out:
> > > +	kfree(available_pcs);
> > > +
> > > +	return err;
> > > +}
> > > +
> > >  static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > >  				 struct device_node *np, int index)
> > >  {
> > > @@ -2873,7 +3004,23 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > >  	if (err)
> > >  		return err;
> > >  
> > > -	return register_netdev(dev);
> > > +	if (airhoa_is_phy_external(port)) {
> > > +		err = airoha_setup_phylink(dev);
> > 
> > This will re-introduce the issue reported here:
> > https://lore.kernel.org/netdev/5c94b9b3850f7f29ed653e2205325620df28c3ff.1746715755.git.christophe.jaillet@wanadoo.fr/
> > 
> 
> I'm confused about this. The suggestion wasn't that register_netdev
> might fail and I need to destroy phylink? Or the linked patch was merged
> and I need to rebase on top of net-next?

what I mean here is if register_netdev() or airoha_setup_phylink() fails, we
should free metadata_dst running airoha_metadata_dst_free() as pointed out by
Christophe. I think this path depends on Christophe's one.

Regards,
Lorenzo

> 
> I didn't include that change to not cause conflicts but once it's merged
> I will rebase and include that fix.
> 
> > > +		if (err)
> > > +			return err;
> > > +	}
> > > +
> > > +	err = register_netdev(dev);
> > > +	if (err)
> > > +		goto free_phylink;
> > > +
> > > +	return 0;
> > > +
> > > +free_phylink:
> > > +	if (airhoa_is_phy_external(port))
> > > +		phylink_destroy(port->phylink);
> > > +
> > > +	return err;
> > >  }
> > >  
> > >  static int airoha_probe(struct platform_device *pdev)
> > > @@ -2967,6 +3114,9 @@ static int airoha_probe(struct platform_device *pdev)
> > >  		struct airoha_gdm_port *port = eth->ports[i];
> > >  
> > >  		if (port && port->dev->reg_state == NETREG_REGISTERED) {
> > > +			if (airhoa_is_phy_external(port))
> > > +				phylink_destroy(port->phylink);
> > > +
> > >  			unregister_netdev(port->dev);
> > >  			airoha_metadata_dst_free(port);
> > >  		}
> > > @@ -2994,6 +3144,9 @@ static void airoha_remove(struct platform_device *pdev)
> > >  			continue;
> > >  
> > >  		airoha_dev_stop(port->dev);
> > > +		if (airhoa_is_phy_external(port))
> > > +			phylink_destroy(port->phylink);
> > > +
> > >  		unregister_netdev(port->dev);
> > >  		airoha_metadata_dst_free(port);
> > >  	}
> > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > > index 53f39083a8b0..73a500474076 100644
> > > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > > @@ -498,6 +498,9 @@ struct airoha_gdm_port {
> > >  	struct net_device *dev;
> > >  	int id;
> > >  
> > > +	struct phylink *phylink;
> > > +	struct phylink_config phylink_config;
> > > +
> > >  	struct airoha_hw_stats stats;
> > >  
> > >  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> > > diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> > > index d931530fc96f..54f7079b28b0 100644
> > > --- a/drivers/net/ethernet/airoha/airoha_regs.h
> > > +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> > > @@ -357,6 +357,18 @@
> > >  #define IP_FRAGMENT_PORT_MASK		GENMASK(8, 5)
> > >  #define IP_FRAGMENT_NBQ_MASK		GENMASK(4, 0)
> > >  
> > > +#define REG_GDMA4_TMBI_FRAG		0x2028
> > > +#define GDMA4_SGMII1_TX_WEIGHT_MASK	GENMASK(31, 26)
> > > +#define GDMA4_SGMII1_TX_FRAG_SIZE_MASK	GENMASK(25, 16)
> > > +#define GDMA4_SGMII0_TX_WEIGHT_MASK	GENMASK(15, 10)
> > > +#define GDMA4_SGMII0_TX_FRAG_SIZE_MASK	GENMASK(9, 0)
> > > +
> > > +#define REG_GDMA4_RMBI_FRAG		0x202c
> > > +#define GDMA4_SGMII1_RX_WEIGHT_MASK	GENMASK(31, 26)
> > > +#define GDMA4_SGMII1_RX_FRAG_SIZE_MASK	GENMASK(25, 16)
> > > +#define GDMA4_SGMII0_RX_WEIGHT_MASK	GENMASK(15, 10)
> > > +#define GDMA4_SGMII0_RX_FRAG_SIZE_MASK	GENMASK(9, 0)
> > > +
> > >  #define REG_MC_VLAN_EN			0x2100
> > >  #define MC_VLAN_EN_MASK			BIT(0)
> > >  
> > > -- 
> > > 2.48.1
> > > 
> 
> 
> 
> -- 
> 	Ansuel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [net-next PATCH v4 07/11] dt-bindings: net: ethernet-controller: permit to define multiple PCS

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Drop the limitation of a single PCS in pcs-handle property. Multiple PCS
> can be defined for an ethrnet-controller node to support various PHY
> interface mode type.
> 
> It's very common for SoCs to have a 2 or more dedicated PCS for Base-X
> (for example SGMII, 1000base-x, 2500base-x, ...) and Base-R (for example
> USXGMII,10base-r, ...) with the MAC selecting one of the other based on
> the attached PHY.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-controller.yaml | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 7cbf11bbe99c..60605b34d242 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -84,8 +84,6 @@ properties:
>  
>    pcs-handle:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
> -    items:
> -      maxItems: 1
>      description:
>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>        bus to link with an external PHY (phy-handle) if exists.

This just specifies the default. Bindings for individual macs can
override this. See fsl,fman-dtsec.yaml for an example.

--Sean

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Introduce internal handling of PCS for phylink. This is an alternative
> to .mac_select_pcs that moves the selection logic of the PCS entirely to
> phylink with the usage of the supported_interface value in the PCS
> struct.
> 
> MAC should now provide an array of available PCS in phylink_config in
> .available_pcs and fill the .num_available_pcs with the number of
> elements in the array. MAC should also define a new bitmap,
> pcs_interfaces, in phylink_config to define for what interface mode a
> dedicated PCS is required.
> 
> On phylink_create() this array is parsed and a linked list of PCS is
> created based on the PCS passed in phylink_config.
> Also the supported_interface value in phylink struct is updated with the
> new supported_interface from the provided PCS.
> 
> On phylink_start() every PCS in phylink PCS list gets attached to the
> phylink instance. This is done by setting the phylink value in
> phylink_pcs struct to the phylink instance.
> 
> On phylink_stop(), every PCS in phylink PCS list is detached from the
> phylink instance. This is done by setting the phylink value in
> phylink_pcs struct to NULL.
> 
> phylink_validate_mac_and_pcs(), phylink_major_config() and
> phylink_inband_caps() are updated to support this new implementation
> with the PCS list stored in phylink.
> 
> They will make use of phylink_validate_pcs_interface() that will loop
> for every PCS in the phylink PCS available list and find one that supports
> the passed interface.
> 
> phylink_validate_pcs_interface() applies the same logic of .mac_select_pcs
> where if a supported_interface value is not set for the PCS struct, then
> it's assumed every interface is supported.
> 
> A MAC is required to implement either a .mac_select_pcs or make use of
> the PCS list implementation. Implementing both will result in a fail
> on MAC/PCS validation.
> 
> phylink value in phylink_pcs struct with this implementation is used to
> track from PCS side when it's attached to a phylink instance. PCS driver
> will make use of this information to correctly detach from a phylink
> instance if needed.
> 
> The .mac_select_pcs implementation is not changed but it's expected that
> every MAC driver migrates to the new implementation to later deprecate
> and remove .mac_select_pcs.

This introduces a completely parallel PCS selection system used by a
single driver. I don't think we want the maintenance burden and
complexity of two systems in perpetuity. So what is your plan for
conversion of existing drivers to your new system?

DSA drivers typically have different PCSs for each port. At the moment
these are selected with mac_select_pcs, allowing the use of a single
phylink_config for each port. If you need to pass PCSs through
phylink_config then each port will needs its own config. This may prove
difficult to integrate with the existing API.

> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phylink.c | 147 +++++++++++++++++++++++++++++++++-----
>  include/linux/phylink.h   |  10 +++
>  2 files changed, 139 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index ec42fd278604..95d7e06dee56 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -59,6 +59,9 @@ struct phylink {
>  	/* The link configuration settings */
>  	struct phylink_link_state link_config;
>  
> +	/* List of available PCS */
> +	struct list_head pcs_list;
> +
>  	/* What interface are supported by the current link.
>  	 * Can change on removal or addition of new PCS.
>  	 */
> @@ -144,6 +147,8 @@ static const phy_interface_t phylink_sfp_interface_preference[] = {
>  
>  static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
>  
> +static void phylink_run_resolve(struct phylink *pl);
> +
>  /**
>   * phylink_set_port_modes() - set the port type modes in the ethtool mask
>   * @mask: ethtool link mode mask
> @@ -499,22 +504,59 @@ static void phylink_validate_mask_caps(unsigned long *supported,
>  	linkmode_and(state->advertising, state->advertising, mask);
>  }
>  
> +static int phylink_validate_pcs_interface(struct phylink_pcs *pcs,
> +					  phy_interface_t interface)
> +{
> +	/* If PCS define an empty supported_interfaces value, assume
> +	 * all interface are supported.
> +	 */
> +	if (phy_interface_empty(pcs->supported_interfaces))
> +		return 0;
> +
> +	/* Ensure that this PCS supports the interface mode */
> +	if (!test_bit(interface, pcs->supported_interfaces))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  					unsigned long *supported,
>  					struct phylink_link_state *state)
>  {
> -	struct phylink_pcs *pcs = NULL;
>  	unsigned long capabilities;
> +	struct phylink_pcs *pcs;
> +	bool pcs_found = false;
>  	int ret;
>  
>  	/* Get the PCS for this interface mode */
>  	if (pl->mac_ops->mac_select_pcs) {
> +		/* Make sure either PCS internal validation or .mac_select_pcs
> +		 * is used. Return error if both are defined.
> +		 */
> +		if (!list_empty(&pl->pcs_list)) {
> +			phylink_err(pl, "either phylink_pcs_add() or .mac_select_pcs must be used\n");
> +			return -EINVAL;
> +		}
> +

This validation should be done in phylink_create.

>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +
> +		pcs_found = !!pcs;
> +	} else {
> +		/* Check every assigned PCS and search for one that supports
> +		 * the interface.
> +		 */
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
>  	}
>  
> -	if (pcs) {
> +	if (pcs_found) {
>  		/* The PCS, if present, must be setup before phylink_create()
>  		 * has been called. If the ops is not initialised, print an
>  		 * error and backtrace rather than oopsing the kernel.
> @@ -526,13 +568,10 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  			return -EINVAL;
>  		}
>  
> -		/* Ensure that this PCS supports the interface which the MAC
> -		 * returned it for. It is an error for the MAC to return a PCS
> -		 * that does not support the interface mode.
> -		 */
> -		if (!phy_interface_empty(pcs->supported_interfaces) &&
> -		    !test_bit(state->interface, pcs->supported_interfaces)) {
> -			phylink_err(pl, "MAC returned PCS which does not support %s\n",
> +		/* Recheck PCS to handle legacy way for .mac_select_pcs */
> +		ret = phylink_validate_pcs_interface(pcs, state->interface);
> +		if (ret) {
> +			phylink_err(pl, "selected PCS does not support %s\n",
>  				    phy_modes(state->interface));
>  			return -EINVAL;
>  		}
> @@ -937,12 +976,22 @@ static unsigned int phylink_inband_caps(struct phylink *pl,
>  					 phy_interface_t interface)
>  {
>  	struct phylink_pcs *pcs;
> +	bool pcs_found = false;
>  
> -	if (!pl->mac_ops->mac_select_pcs)
> -		return 0;
> +	if (pl->mac_ops->mac_select_pcs) {
> +		pcs = pl->mac_ops->mac_select_pcs(pl->config,
> +						  interface);
> +		pcs_found = !!pcs;
> +	} else {
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
> +	}
>  
> -	pcs = pl->mac_ops->mac_select_pcs(pl->config, interface);
> -	if (!pcs)
> +	if (!pcs_found)
>  		return 0;
>  
>  	return phylink_pcs_inband_caps(pcs, interface);
> @@ -1228,10 +1277,36 @@ static void phylink_major_config(struct phylink *pl, bool restart,
>  			pl->major_config_failed = true;
>  			return;
>  		}
> +	/* Find a PCS in available PCS list for the requested interface.
> +	 * This doesn't overwrite the previous .mac_select_pcs as either
> +	 * .mac_select_pcs or PCS list implementation are permitted.
> +	 *
> +	 * Skip searching if the MAC doesn't require a dedicaed PCS for

dedicated

> +	 * the requested interface.
> +	 */
> +	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> +		bool pcs_found = false;
> +
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs,
> +							    state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!pcs_found) {
> +			phylink_err(pl,
> +				    "couldn't find a PCS for %s\n",
> +				    phy_modes(state->interface));
>  
> -		pcs_changed = pl->pcs != pcs;
> +			pl->major_config_failed = true;
> +			return;
> +		}
>  	}
>  
> +	pcs_changed = pl->pcs != pcs;
> +
>  	phylink_pcs_neg_mode(pl, pcs, state->interface, state->advertising);
>  
>  	phylink_dbg(pl, "major config, active %s/%s/%s\n",
> @@ -1258,10 +1333,12 @@ static void phylink_major_config(struct phylink *pl, bool restart,
>  	if (pcs_changed) {
>  		phylink_pcs_disable(pl->pcs);
>  
> -		if (pl->pcs)
> -			pl->pcs->phylink = NULL;
> +		if (pl->mac_ops->mac_select_pcs) {
> +			if (pl->pcs)
> +				pl->pcs->phylink = NULL;
>  
> -		pcs->phylink = pl;
> +			pcs->phylink = pl;
> +		}
>  
>  		pl->pcs = pcs;
>  	}
> @@ -1797,8 +1874,9 @@ struct phylink *phylink_create(struct phylink_config *config,
>  			       phy_interface_t iface,
>  			       const struct phylink_mac_ops *mac_ops)
>  {
> +	struct phylink_pcs *pcs;
>  	struct phylink *pl;
> -	int ret;
> +	int i, ret;
>  
>  	/* Validate the supplied configuration */
>  	if (phy_interface_empty(config->supported_interfaces)) {
> @@ -1813,9 +1891,21 @@ struct phylink *phylink_create(struct phylink_config *config,
>  
>  	mutex_init(&pl->state_mutex);
>  	INIT_WORK(&pl->resolve, phylink_resolve);
> +	INIT_LIST_HEAD(&pl->pcs_list);
> +
> +	/* Fill the PCS list with available PCS from phylink config */
> +	for (i = 0; i < config->num_available_pcs; i++) {
> +		pcs = config->available_pcs[i];
> +
> +		list_add(&pcs->list, &pl->pcs_list);
> +	}

Why do we have a separate linked list if we are getting all the PCSs
in an array at configuration time? From what I can tell there is no
dynamic configuration of PCSs.

>  
>  	phy_interface_copy(pl->supported_interfaces,
>  			   config->supported_interfaces);
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);
>  
>  	pl->config = config;
>  	if (config->type == PHYLINK_NETDEV) {
> @@ -1894,10 +1984,16 @@ EXPORT_SYMBOL_GPL(phylink_create);
>   */
>  void phylink_destroy(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs, *tmp;
> +
>  	sfp_bus_del_upstream(pl->sfp_bus);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
> +	/* Remove every PCS from phylink PCS list */
> +	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
> +		list_del(&pcs->list);
> +
>  	cancel_work_sync(&pl->resolve);
>  	kfree(pl);
>  }
> @@ -2374,6 +2470,7 @@ static irqreturn_t phylink_link_handler(int irq, void *data)
>   */
>  void phylink_start(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs;
>  	bool poll = false;
>  
>  	ASSERT_RTNL();
> @@ -2400,6 +2497,10 @@ void phylink_start(struct phylink *pl)
>  
>  	pl->pcs_state = PCS_STATE_STARTED;
>  
> +	/* link available PCS to phylink struct */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = pl;
> +
>  	phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_STOPPED);
>  
>  	if (pl->cfg_link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
> @@ -2444,6 +2545,8 @@ EXPORT_SYMBOL_GPL(phylink_start);
>   */
>  void phylink_stop(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs;
> +
>  	ASSERT_RTNL();
>  
>  	if (pl->sfp_bus)
> @@ -2461,6 +2564,14 @@ void phylink_stop(struct phylink *pl)
>  	pl->pcs_state = PCS_STATE_DOWN;
>  
>  	phylink_pcs_disable(pl->pcs);
> +
> +	/* Drop link between phylink and PCS */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = NULL;
> +
> +	/* Restore original supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
>  }
>  EXPORT_SYMBOL_GPL(phylink_stop);
>  
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 30659b615fca..ef0b5a0729c8 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -150,12 +150,16 @@ enum phylink_op_type {
>   *		     if MAC link is at %MLO_AN_FIXED mode.
>   * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
>   *                        are supported by the MAC/PCS.
> + * @pcs_interfaces: bitmap describing for which PHY_INTERFACE_MODE_xxx a
> + *		    dedicated PCS is required.
>   * @lpi_interfaces: bitmap describing which PHY interface modes can support
>   *		    LPI signalling.
>   * @mac_capabilities: MAC pause/speed/duplex capabilities.
>   * @lpi_capabilities: MAC speeds which can support LPI signalling
>   * @lpi_timer_default: Default EEE LPI timer setting.
>   * @eee_enabled_default: If set, EEE will be enabled by phylink at creation time
> + * @available_pcs: array of available phylink_pcs PCS
> + * @num_available_pcs: num of available phylink_pcs PCS
>   */
>  struct phylink_config {
>  	struct device *dev;
> @@ -168,11 +172,14 @@ struct phylink_config {
>  	void (*get_fixed_state)(struct phylink_config *config,
>  				struct phylink_link_state *state);
>  	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
> +	DECLARE_PHY_INTERFACE_MASK(pcs_interfaces);
>  	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
>  	unsigned long mac_capabilities;
>  	unsigned long lpi_capabilities;
>  	u32 lpi_timer_default;
>  	bool eee_enabled_default;
> +	struct phylink_pcs **available_pcs;
> +	unsigned int num_available_pcs;
>  };
>  
>  void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
> @@ -469,6 +476,9 @@ struct phylink_pcs {
>  	struct phylink *phylink;
>  	bool poll;
>  	bool rxc_always_on;
> +
> +	/* private: */
> +	struct list_head list;
>  };
>  
>  /**

Re: [net-next PATCH v4 01/11] net: phy: introduce phy_interface_copy helper

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Introduce phy_interface_copy helper as a shorthand to copy the PHY
> interface bitmap to a different location.
> 
> This is useful if a PHY interface bitmap needs to be stored in a
> different variable and needs to be reset to an original value saved in a
> different bitmap.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  include/linux/phy.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index d62d292024bc..9f0e5fb30d63 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -173,6 +173,11 @@ static inline void phy_interface_or(unsigned long *dst, const unsigned long *a,
>  	bitmap_or(dst, a, b, PHY_INTERFACE_MODE_MAX);
>  }
>  
> +static inline void phy_interface_copy(unsigned long *dst, const unsigned long *src)
> +{
> +	bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX);
> +}
> +
>  static inline void phy_interface_set_rgmii(unsigned long *intf)
>  {
>  	__set_bit(PHY_INTERFACE_MODE_RGMII, intf);

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>

Re: [net-next PATCH v4 09/11] dt-bindings: net: pcs: Document support for Airoha Ethernet PCS

[Sean Anderson]

Devicetree binding patches should come first in a series.

On 5/11/25 16:12, Christian Marangi wrote:
> Document support for Airoha Ethernet PCS for AN7581 SoC.
> 
> Airoha AN7581 SoC expose multiple Physical Coding Sublayer (PCS) for
> the various Serdes port supporting different Media Independent Interface
> (10BASE-R, USXGMII, 2500BASE-X, 1000BASE-X, SGMII).
> 
> This follow the new PCS provider with the use of #pcs-cells property.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/pcs/airoha,pcs.yaml          | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml b/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
> new file mode 100644
> index 000000000000..8bcf7757c728
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pcs/airoha,pcs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha Ethernet PCS and Serdes
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  Airoha AN7581 SoC expose multiple Physical Coding Sublayer (PCS) for

The Airoha AN7581 SoC exposes multiple Physical Coding Sublayers (PCSs) for

> +  the various Serdes port supporting different Media Independent Interface

ports, Interfaces

> +  (10BASE-R, USXGMII, 2500BASE-X, 1000BASE-X, SGMII).

10GBase-R

> +
> +properties:
> +  compatible:
> +    enum:
> +      - airoha,an7581-pcs-eth
> +      - airoha,an7581-pcs-pon

What's the difference between these? There's no mention of PON above.

> +  reg:
> +    items:
> +      - description: XFI MAC reg
> +      - description: HSGMII AN reg
> +      - description: HSGMII PCS reg
> +      - description: MULTI SGMII reg
> +      - description: USXGMII reg
> +      - description: HSGMII rate adaption reg
> +      - description: XFI Analog register
> +      - description: XFI PMA (Physical Medium Attachment) register
> +
> +  reg-names:
> +    items:
> +      - const: xfi_mac
> +      - const: hsgmii_an
> +      - const: hsgmii_pcs
> +      - const: multi_sgmii
> +      - const: usxgmii
> +      - const: hsgmii_rate_adp
> +      - const: xfi_ana
> +      - const: xfi_pma
> +
> +  resets:
> +    items:
> +      - description: MAC reset
> +      - description: PHY reset
> +
> +  reset-names:
> +    items:
> +      - const: mac
> +      - const: phy
> +
> +  "#pcs-cells":
> +    const: 0

From what I can tell you only have one PCS. So this is unnecessary.

--Sean

> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - resets
> +  - reset-names
> +  - "#pcs-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/reset/airoha,en7581-reset.h>
> +
> +    pcs@1fa08000 {
> +        compatible = "airoha,an7581-pcs-pon";
> +        reg = <0x1fa08000 0x1000>,
> +              <0x1fa80000 0x60>,
> +              <0x1fa80a00 0x164>,
> +              <0x1fa84000 0x450>,
> +              <0x1fa85900 0x338>,
> +              <0x1fa86000 0x300>,
> +              <0x1fa8a000 0x1000>,
> +              <0x1fa8b000 0x1000>;
> +        reg-names = "xfi_mac", "hsgmii_an", "hsgmii_pcs",
> +                    "multi_sgmii", "usxgmii",
> +                    "hsgmii_rate_adp", "xfi_ana", "xfi_pma";
> +
> +        resets = <&scuclk EN7581_XPON_MAC_RST>,
> +                 <&scuclk EN7581_XPON_PHY_RST>;
> +        reset-names = "mac", "phy";
> +
> +        #pcs-cells = <0>;
> +    };
> +
> +    pcs@1fa09000 {
> +        compatible = "airoha,an7581-pcs-eth";
> +        reg = <0x1fa09000 0x1000>,
> +              <0x1fa70000 0x60>,
> +              <0x1fa70a00 0x164>,
> +              <0x1fa74000 0x450>,
> +              <0x1fa75900 0x338>,
> +              <0x1fa76000 0x300>,
> +              <0x1fa7a000 0x1000>,
> +              <0x1fa7b000 0x1000>;
> +        reg-names = "xfi_mac", "hsgmii_an", "hsgmii_pcs",
> +                    "multi_sgmii", "usxgmii",
> +                    "hsgmii_rate_adp", "xfi_ana", "xfi_pma";
> +
> +        resets = <&scuclk EN7581_XSI_MAC_RST>,
> +                 <&scuclk EN7581_XSI_PHY_RST>;
> +        reset-names = "mac", "phy";
> +
> +        #pcs-cells = <0>;
> +    };

Re: [net-next PATCH v4 05/11] net: pcs: implement Firmware node support for PCS driver

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Implement the foundation of Firmware node support for PCS driver.
> 
> To support this, implement a simple Provider API where a PCS driver can
> expose multiple PCS with an xlate .get function.
> 
> PCS driver will have to call fwnode_pcs_add_provider() and pass the
> firmware node pointer and a xlate function to return the correct PCS for
> the passed #pcs-cells.
> 
> This will register the PCS in a global list of providers so that
> consumer can access it.
> 
> The consumer will then use fwnode_pcs_get() to get the actual PCS by
> passing the firmware node pointer and the index for #pcs-cells.
> 
> For a simple implementation where #pcs-cells is 0 and the PCS driver
> expose a single PCS, the xlate function fwnode_pcs_simple_get() is
> provided.
> 
> For an advanced implementation a custom xlate function is required.

There is no use case for this. All PCSs have a fwnode per PCS. Removing
support for pcs cells will simplify the lookup code as well as the
registration code and API.

> One removal the PCS driver should first delete itself from the provider
> list using fwnode_pcs_del_provider() and then call phylink_release_pcs()
> on every PCS the driver provides.

And things like this can be done automatically.

> A generic function fwnode_phylink_pcs_parse() is provided for MAC driver
> that will declare PCS in DT (or ACPI).
> This function will parse "pcs-handle" property and fill the passed array
> with the parsed PCS in availabel_pcs up to the passed num_pcs value.
> It's also possible to pass NULL as array to only parse the PCS and
> update the num_pcs value with the count of scanned PCS.

When is this useful?

> Co-developed-by: Daniel Golle <daniel@makrotopia.org>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/pcs/Kconfig          |   6 +
>  drivers/net/pcs/Makefile         |   1 +
>  drivers/net/pcs/pcs.c            | 201 +++++++++++++++++++++++++++++++
>  include/linux/pcs/pcs-provider.h |  41 +++++++
>  include/linux/pcs/pcs.h          |  56 +++++++++
>  5 files changed, 305 insertions(+)
>  create mode 100644 drivers/net/pcs/pcs.c
>  create mode 100644 include/linux/pcs/pcs-provider.h
>  create mode 100644 include/linux/pcs/pcs.h
> 
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index f6aa437473de..0d54bea1f663 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -5,6 +5,12 @@
>  
>  menu "PCS device drivers"
>  
> +config FWNODE_PCS
> +	tristate
> +	depends on (ACPI || OF)
> +	help
> +		Firmware node PCS accessors
> +
>  config PCS_XPCS
>  	tristate "Synopsys DesignWare Ethernet XPCS"
>  	select PHYLINK
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b90..3005cdd89ab7 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Linux PCS drivers
>  
> +obj-$(CONFIG_FWNODE_PCS)	+= pcs.o
>  pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
>  				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
>  
> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 000000000000..26d07a2edfce
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/phylink.h>
> +#include <linux/pcs/pcs.h>
> +#include <linux/pcs/pcs-provider.h>
> +
> +MODULE_DESCRIPTION("PCS library");
> +MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
> +MODULE_LICENSE("GPL");
> +
> +struct fwnode_pcs_provider {
> +	struct list_head link;
> +
> +	struct fwnode_handle *fwnode;
> +	struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +				   void *data);
> +
> +	void *data;
> +};
> +
> +static LIST_HEAD(fwnode_pcs_providers);
> +static DEFINE_MUTEX(fwnode_pcs_mutex);
> +
> +struct phylink_pcs *fwnode_pcs_simple_get(struct fwnode_reference_args *pcsspec,
> +					  void *data)
> +{
> +	return data;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_pcs_simple_get);
> +
> +int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> +			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +						       void *data),
> +			    void *data)
> +{
> +	struct fwnode_pcs_provider *pp;
> +
> +	if (!fwnode)
> +		return 0;
> +
> +	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> +	if (!pp)
> +		return -ENOMEM;
> +
> +	pp->fwnode = fwnode_handle_get(fwnode);
> +	pp->data = data;
> +	pp->get = get;
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_add(&pp->link, &fwnode_pcs_providers);
> +	mutex_unlock(&fwnode_pcs_mutex);
> +	pr_debug("Added pcs provider from %pfwf\n", fwnode);
> +
> +	fwnode_dev_initialized(fwnode, true);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_pcs_add_provider);
> +
> +void fwnode_pcs_del_provider(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_pcs_provider *pp;
> +
> +	if (!fwnode)
> +		return;
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(pp, &fwnode_pcs_providers, link) {
> +		if (pp->fwnode == fwnode) {
> +			list_del(&pp->link);
> +			fwnode_dev_initialized(pp->fwnode, false);
> +			fwnode_handle_put(pp->fwnode);
> +			kfree(pp);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_pcs_del_provider);
> +
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode, int index,
> +				const char *name,
> +				struct fwnode_reference_args *out_args)
> +{
> +	int ret;
> +
> +	if (!fwnode)
> +		return -ENOENT;
> +
> +	if (name)
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +
> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells",
> +						 -1, index, out_args);
> +	if (ret || (name && index < 0))
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		if (provider->fwnode == pcsspec->fwnode) {
> +			pcs = provider->get(pcsspec, provider->data);
> +			if (!IS_ERR(pcs))
> +				break;
> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return pcs;
> +}
> +
> +static struct phylink_pcs *__fwnode_pcs_get(struct fwnode_handle *fwnode,
> +					    int index, const char *con_id)

Many existing drivers have to support non-standard PCS handle names
(e.g. pcsphy-handle, phy-handle, etc.). Support for this is important
for converting those drivers to use this system.

--Sean

> +{
> +	struct fwnode_reference_args pcsspec;
> +	struct phylink_pcs *pcs;
> +	int ret;
> +
> +	ret = fwnode_parse_pcsspec(fwnode, index, con_id, &pcsspec);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	pcs = fwnode_pcs_get_from_pcsspec(&pcsspec);
> +	fwnode_handle_put(pcsspec.fwnode);
> +
> +	return pcs;
> +}
> +
> +struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode, int index)
> +{
> +	return __fwnode_pcs_get(fwnode, index, NULL);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_pcs_get);
> +
> +static int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode,
> +				    unsigned int *num_pcs)
> +{
> +	struct fwnode_reference_args out_args;
> +	int index = 0;
> +	int ret;
> +
> +	while (true) {
> +		ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +							 "#pcs-cells",
> +							 -1, index, &out_args);
> +		/* We expect to reach an -ENOENT error while counting */
> +		if (ret)
> +			break;
> +
> +		fwnode_handle_put(out_args.fwnode);
> +		index++;
> +	}
> +
> +	/* Update num_pcs with parsed PCS */
> +	*num_pcs = index;
> +
> +	/* Return error if we didn't found any PCS */
> +	return index > 0 ? 0 : -ENOENT;
> +}
> +
> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int *num_pcs)
> +{
> +	int i;
> +
> +	if (!fwnode_property_present(fwnode, "pcs-handle"))
> +		return -ENODEV;
> +
> +	/* With available_pcs NULL, only count the PCS */
> +	if (!available_pcs)
> +		return fwnode_phylink_pcs_count(fwnode, num_pcs);
> +
> +	for (i = 0; i < *num_pcs; i++) {
> +		struct phylink_pcs *pcs;
> +
> +		pcs = fwnode_pcs_get(fwnode, i);
> +		if (IS_ERR(pcs))
> +			return PTR_ERR(pcs);
> +
> +		available_pcs[i] = pcs;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);
> diff --git a/include/linux/pcs/pcs-provider.h b/include/linux/pcs/pcs-provider.h
> new file mode 100644
> index 000000000000..ae51c108147e
> --- /dev/null
> +++ b/include/linux/pcs/pcs-provider.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __LINUX_PCS_PROVIDER_H
> +#define __LINUX_PCS_PROVIDER_H
> +
> +/**
> + * fwnode_pcs_simple_get - Simple xlate function to retrieve PCS
> + * @pcsspec: reference arguments
> + * @data: Context data (assumed assigned to the single PCS)
> + *
> + * Returns: the PCS pointed by data.
> + */
> +struct phylink_pcs *fwnode_pcs_simple_get(struct fwnode_reference_args *pcsspec,
> +					  void *data);
> +
> +/**
> + * fwnode_pcs_add_provider - Registers a new PCS provider
> + * @fwnode: Firmware node
> + * @get: xlate function to retrieve the PCS
> + * @data: Context data
> + *
> + * Register and add a new PCS to the global providers list
> + * for the firmware node. A function to get the PCS from
> + * firmware node with the use fwnode reference arguments.
> + * To the get function is also passed the interface type
> + * requested for the PHY. PCS driver will use the passed
> + * interface to understand if the PCS can support it or not.
> + *
> + * Returns: 0 on success or -ENOMEM on allocation failure.
> + */
> +int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> +			    struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> +						       void *data),
> +			    void *data);
> +
> +/**
> + * fwnode_pcs_del_provider - Removes a PCS provider
> + * @fwnode: Firmware node
> + */
> +void fwnode_pcs_del_provider(struct fwnode_handle *fwnode);
> +
> +#endif /* __LINUX_PCS_PROVIDER_H */
> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> new file mode 100644
> index 000000000000..33244e3a442b
> --- /dev/null
> +++ b/include/linux/pcs/pcs.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __LINUX_PCS_H
> +#define __LINUX_PCS_H
> +
> +#include <linux/phylink.h>
> +
> +#if IS_ENABLED(CONFIG_FWNODE_PCS)
> +/**
> + * fwnode_pcs_get - Retrieves a PCS from a firmware node
> + * @fwnode: firmware node
> + * @index: index fwnode PCS handle in firmware node
> + *
> + * Get a PCS from the firmware node at index.
> + *
> + * Returns: a pointer to the phylink_pcs or a negative
> + * error pointer. Can return -EPROBE_DEFER if the PCS is not
> + * present in global providers list (either due to driver
> + * still needs to be probed or it failed to probe/removed)
> + */
> +struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
> +				   int index);
> +
> +/**
> + * fwnode_phylink_pcs_parse - generic PCS parse for fwnode PCS provider
> + * @fwnode: firmware node
> + * @available_pcs: pointer to preallocated array of PCS
> + * @num_pcs: where to store count of parsed PCS
> + *
> + * Generic helper function to fill available_pcs array with PCS parsed
> + * from a "pcs-handle" fwnode property defined in firmware node up to
> + * passed num_pcs.
> + *
> + * If available_pcs is NULL, num_pcs is updated with the count of the
> + * parsed PCS.
> + *
> + * Returns: 0 or a negative error.
> + */
> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int *num_pcs);
> +#else
> +static inline struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
> +						 int index)
> +{
> +	return ERR_PTR(-ENOENT);
> +}
> +
> +static inline int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +					   struct phylink_pcs **available_pcs,
> +					   unsigned int *num_pcs)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
> +
> +#endif /* __LINUX_PCS_H */

Re: [net-next PATCH v4 08/11] net: phylink: add .pcs_link_down PCS OP

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Permit for PCS driver to define specific operation to torn down the link
> between the MAC and the PCS.
> 
> This might be needed for some PCS that reset counter

Counters must be preserved across link u/down.

> or require special
> reset to correctly work if the link needs to be restored later.

Can you describe this in more detail?

> On phylink_link_down() call, the additional phylink_pcs_link_down() will
> be called before .mac_link_down to torn down the link.

tear

> PCS driver will need to define .pcs_link_down to make use of this.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phylink.c | 8 ++++++++
>  include/linux/phylink.h   | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 1a4df0d24aa2..39cd15e30598 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1009,6 +1009,12 @@ static void phylink_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>  		pcs->ops->pcs_link_up(pcs, neg_mode, interface, speed, duplex);
>  }
>  
> +static void phylink_pcs_link_down(struct phylink_pcs *pcs)
> +{
> +	if (pcs && pcs->ops->pcs_link_down)
> +		pcs->ops->pcs_link_down(pcs);
> +}
> +
>  static void phylink_pcs_disable_eee(struct phylink_pcs *pcs)
>  {
>  	if (pcs && pcs->ops->pcs_disable_eee)
> @@ -1686,6 +1692,8 @@ static void phylink_link_down(struct phylink *pl)
>  
>  	phylink_deactivate_lpi(pl);
>  
> +	phylink_pcs_link_down(pl->pcs);
> +
>  	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
>  				   pl->cur_interface);
>  	phylink_info(pl, "Link is Down\n");
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index c5496c063b6a..8b3d1dfb83a1 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -494,6 +494,7 @@ struct phylink_pcs {
>   * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
>   * @pcs_link_up: program the PCS for the resolved link configuration
>   *               (where necessary).
> + * @pcs_link_down: torn down link between MAC and PCS.

ops documentation should use imperative verbs.

You are also missing the longer documentation below.

>   * @pcs_disable_eee: optional notification to PCS that EEE has been disabled
>   *		     at the MAC.
>   * @pcs_enable_eee: optional notification to PCS that EEE will be enabled at
> @@ -521,6 +522,7 @@ struct phylink_pcs_ops {
>  	void (*pcs_an_restart)(struct phylink_pcs *pcs);
>  	void (*pcs_link_up)(struct phylink_pcs *pcs, unsigned int neg_mode,
>  			    phy_interface_t interface, int speed, int duplex);
> +	void (*pcs_link_down)(struct phylink_pcs *pcs);
>  	void (*pcs_disable_eee)(struct phylink_pcs *pcs);
>  	void (*pcs_enable_eee)(struct phylink_pcs *pcs);
>  	int (*pcs_pre_init)(struct phylink_pcs *pcs);

Re: [net-next PATCH v4 11/11] net: airoha: add phylink support for GDM2/3/4

[Sean Anderson]

On 5/11/25 16:12, Christian Marangi wrote:
> Add phylink support for GDM2/3/4 port that require configuration of the
> PCS to make the external PHY or attached SFP cage work.
> 
> These needs to be defined in the GDM port node using the pcs-handle
> property.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c  | 155 +++++++++++++++++++++-
>  drivers/net/ethernet/airoha/airoha_eth.h  |   3 +
>  drivers/net/ethernet/airoha/airoha_regs.h |  12 ++
>  3 files changed, 169 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 16c7896f931f..3be1ae077a76 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -7,6 +7,7 @@
>  #include <linux/of_net.h>
>  #include <linux/platform_device.h>
>  #include <linux/tcp.h>
> +#include <linux/pcs/pcs.h>
>  #include <linux/u64_stats_sync.h>
>  #include <net/dst_metadata.h>
>  #include <net/page_pool/helpers.h>
> @@ -79,6 +80,11 @@ static bool airhoa_is_lan_gdm_port(struct airoha_gdm_port *port)
>  	return port->id == 1;
>  }
>  
> +static bool airhoa_is_phy_external(struct airoha_gdm_port *port)
> +{
> +	return port->id != 1;
> +}
> +
>  static void airoha_set_macaddr(struct airoha_gdm_port *port, const u8 *addr)
>  {
>  	struct airoha_eth *eth = port->qdma->eth;
> @@ -1613,6 +1619,17 @@ static int airoha_dev_open(struct net_device *dev)
>  	struct airoha_gdm_port *port = netdev_priv(dev);
>  	struct airoha_qdma *qdma = port->qdma;
>  
> +	if (airhoa_is_phy_external(port)) {
> +		err = phylink_of_phy_connect(port->phylink, dev->dev.of_node, 0);
> +		if (err) {
> +			netdev_err(dev, "%s: could not attach PHY: %d\n", __func__,
> +				   err);
> +			return err;
> +		}
> +
> +		phylink_start(port->phylink);
> +	}
> +
>  	netif_tx_start_all_queues(dev);
>  	err = airoha_set_vip_for_gdm_port(port, true);
>  	if (err)
> @@ -1665,6 +1682,11 @@ static int airoha_dev_stop(struct net_device *dev)
>  		}
>  	}
>  
> +	if (airhoa_is_phy_external(port)) {
> +		phylink_stop(port->phylink);
> +		phylink_disconnect_phy(port->phylink);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2795,6 +2817,115 @@ bool airoha_is_valid_gdm_port(struct airoha_eth *eth,
>  	return false;
>  }
>  
> +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> +	struct airoha_gdm_port *port = container_of(config, struct airoha_gdm_port,
> +						    phylink_config);
> +	struct airoha_qdma *qdma = port->qdma;
> +	struct airoha_eth *eth = qdma->eth;
> +	u32 frag_size_tx, frag_size_rx;
> +
> +	if (port->id != 4)
> +		return;
> +
> +	switch (speed) {
> +	case SPEED_10000:
> +	case SPEED_5000:
> +		frag_size_tx = 8;
> +		frag_size_rx = 8;
> +		break;
> +	case SPEED_2500:
> +		frag_size_tx = 2;
> +		frag_size_rx = 1;
> +		break;
> +	default:
> +		frag_size_tx = 1;
> +		frag_size_rx = 0;
> +	}
> +
> +	/* Configure TX/RX frag based on speed */
> +	airoha_fe_rmw(eth, REG_GDMA4_TMBI_FRAG,
> +		      GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> +		      FIELD_PREP(GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> +				 frag_size_tx));
> +
> +	airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG,
> +		      GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> +		      FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> +				 frag_size_rx));
> +}
> +
> +static const struct phylink_mac_ops airoha_phylink_ops = {
> +	.mac_link_up = airoha_mac_link_up,
> +};
> +
> +static int airoha_setup_phylink(struct net_device *dev)
> +{
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +	struct device_node *np = dev->dev.of_node;
> +	struct phylink_pcs **available_pcs;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
> +	unsigned int num_pcs;
> +	int err;
> +
> +	err = of_get_phy_mode(np, &phy_mode);
> +	if (err) {
> +		dev_err(&dev->dev, "incorrect phy-mode\n");
> +		return err;
> +	}
> +
> +	port->phylink_config.dev = &dev->dev;
> +	port->phylink_config.type = PHYLINK_NETDEV;
> +	port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +						MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD |
> +						MAC_5000FD | MAC_10000FD;
> +
> +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), NULL, &num_pcs);
> +	if (err)
> +		return err;
> +
> +	available_pcs = kcalloc(num_pcs, sizeof(*available_pcs), GFP_KERNEL);
> +	if (!available_pcs)
> +		return -ENOMEM;
> +
> +	err = fwnode_phylink_pcs_parse(dev_fwnode(&dev->dev), available_pcs,
> +				       &num_pcs);
> +	if (err)
> +		goto out;

OK, so say you have PCSs A, B, and C. phylink determines that you are
going to use B. But where do you select the PCS? Don't you have to
configure a mux or something?

--Sean

> +	port->phylink_config.available_pcs = available_pcs;
> +	port->phylink_config.num_available_pcs = num_pcs;
> +
> +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +		  port->phylink_config.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_USXGMII,
> +		  port->phylink_config.supported_interfaces);
> +
> +	phy_interface_copy(port->phylink_config.pcs_interfaces,
> +			   port->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&port->phylink_config,
> +				 of_fwnode_handle(np),
> +				 phy_mode, &airoha_phylink_ops);
> +	if (IS_ERR(phylink)) {
> +		err = PTR_ERR(phylink);
> +		goto out;
> +	}
> +
> +	port->phylink = phylink;
> +out:
> +	kfree(available_pcs);
> +
> +	return err;
> +}
> +
>  static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  				 struct device_node *np, int index)
>  {
> @@ -2873,7 +3004,23 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  	if (err)
>  		return err;
>  
> -	return register_netdev(dev);
> +	if (airhoa_is_phy_external(port)) {
> +		err = airoha_setup_phylink(dev);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = register_netdev(dev);
> +	if (err)
> +		goto free_phylink;
> +
> +	return 0;
> +
> +free_phylink:
> +	if (airhoa_is_phy_external(port))
> +		phylink_destroy(port->phylink);
> +
> +	return err;
>  }
>  
>  static int airoha_probe(struct platform_device *pdev)
> @@ -2967,6 +3114,9 @@ static int airoha_probe(struct platform_device *pdev)
>  		struct airoha_gdm_port *port = eth->ports[i];
>  
>  		if (port && port->dev->reg_state == NETREG_REGISTERED) {
> +			if (airhoa_is_phy_external(port))
> +				phylink_destroy(port->phylink);
> +
>  			unregister_netdev(port->dev);
>  			airoha_metadata_dst_free(port);
>  		}
> @@ -2994,6 +3144,9 @@ static void airoha_remove(struct platform_device *pdev)
>  			continue;
>  
>  		airoha_dev_stop(port->dev);
> +		if (airhoa_is_phy_external(port))
> +			phylink_destroy(port->phylink);
> +
>  		unregister_netdev(port->dev);
>  		airoha_metadata_dst_free(port);
>  	}
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 53f39083a8b0..73a500474076 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -498,6 +498,9 @@ struct airoha_gdm_port {
>  	struct net_device *dev;
>  	int id;
>  
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;
> +
>  	struct airoha_hw_stats stats;
>  
>  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> index d931530fc96f..54f7079b28b0 100644
> --- a/drivers/net/ethernet/airoha/airoha_regs.h
> +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> @@ -357,6 +357,18 @@
>  #define IP_FRAGMENT_PORT_MASK		GENMASK(8, 5)
>  #define IP_FRAGMENT_NBQ_MASK		GENMASK(4, 0)
>  
> +#define REG_GDMA4_TMBI_FRAG		0x2028
> +#define GDMA4_SGMII1_TX_WEIGHT_MASK	GENMASK(31, 26)
> +#define GDMA4_SGMII1_TX_FRAG_SIZE_MASK	GENMASK(25, 16)
> +#define GDMA4_SGMII0_TX_WEIGHT_MASK	GENMASK(15, 10)
> +#define GDMA4_SGMII0_TX_FRAG_SIZE_MASK	GENMASK(9, 0)
> +
> +#define REG_GDMA4_RMBI_FRAG		0x202c
> +#define GDMA4_SGMII1_RX_WEIGHT_MASK	GENMASK(31, 26)
> +#define GDMA4_SGMII1_RX_FRAG_SIZE_MASK	GENMASK(25, 16)
> +#define GDMA4_SGMII0_RX_WEIGHT_MASK	GENMASK(15, 10)
> +#define GDMA4_SGMII0_RX_FRAG_SIZE_MASK	GENMASK(9, 0)
> +
>  #define REG_MC_VLAN_EN			0x2100
>  #define MC_VLAN_EN_MASK			BIT(0)
>  

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Daniel Golle]

On Tue, May 13, 2025 at 02:18:02PM -0400, Sean Anderson wrote:
> On 5/11/25 16:12, Christian Marangi wrote:
> > Introduce internal handling of PCS for phylink. This is an alternative
> > to .mac_select_pcs that moves the selection logic of the PCS entirely to
> > phylink with the usage of the supported_interface value in the PCS
> > struct.
> > 
> > MAC should now provide an array of available PCS in phylink_config in
> > .available_pcs and fill the .num_available_pcs with the number of
> > elements in the array. MAC should also define a new bitmap,
> > pcs_interfaces, in phylink_config to define for what interface mode a
> > dedicated PCS is required.
> > 
> > On phylink_create() this array is parsed and a linked list of PCS is
> > created based on the PCS passed in phylink_config.
> > Also the supported_interface value in phylink struct is updated with the
> > new supported_interface from the provided PCS.
> > 
> > On phylink_start() every PCS in phylink PCS list gets attached to the
> > phylink instance. This is done by setting the phylink value in
> > phylink_pcs struct to the phylink instance.
> > 
> > On phylink_stop(), every PCS in phylink PCS list is detached from the
> > phylink instance. This is done by setting the phylink value in
> > phylink_pcs struct to NULL.
> > 
> > phylink_validate_mac_and_pcs(), phylink_major_config() and
> > phylink_inband_caps() are updated to support this new implementation
> > with the PCS list stored in phylink.
> > 
> > They will make use of phylink_validate_pcs_interface() that will loop
> > for every PCS in the phylink PCS available list and find one that supports
> > the passed interface.
> > 
> > phylink_validate_pcs_interface() applies the same logic of .mac_select_pcs
> > where if a supported_interface value is not set for the PCS struct, then
> > it's assumed every interface is supported.
> > 
> > A MAC is required to implement either a .mac_select_pcs or make use of
> > the PCS list implementation. Implementing both will result in a fail
> > on MAC/PCS validation.
> > 
> > phylink value in phylink_pcs struct with this implementation is used to
> > track from PCS side when it's attached to a phylink instance. PCS driver
> > will make use of this information to correctly detach from a phylink
> > instance if needed.
> > 
> > The .mac_select_pcs implementation is not changed but it's expected that
> > every MAC driver migrates to the new implementation to later deprecate
> > and remove .mac_select_pcs.
> 
> This introduces a completely parallel PCS selection system used by a
> single driver. I don't think we want the maintenance burden and
> complexity of two systems in perpetuity. So what is your plan for
> conversion of existing drivers to your new system?

Moving functionality duplicated in many drivers to a common shared
implementation is nothing unsual.

While this series proposes the new mechanism for Airoha SoC, they are
immediately useful (and long awaited) also for MediaTek and Qualcomm
SoCs.

Also in the series you posted at least the macb driver (in "[net-next
PATCH v4 09/11] net: macb: Move most of mac_config to  mac_prepare")
would benefit from that shared implementation, as all it does in it's
mac_select_pcs is selecting the PCS by a given phy_interface_t, which is
what most Ethernet drivers which use more than one PCS are doing in
their implementatio of mac_select_pcs().

Also axienet_mac_select_pcs() from "[net-next PATCH v4 08/11] net:
axienet: Convert to use PCS subsystem" could obviously very easily be
mirated to use the phylink-internal handling of PCS selection.


> 
> DSA drivers typically have different PCSs for each port. At the moment
> these are selected with mac_select_pcs, allowing the use of a single
> phylink_config for each port. If you need to pass PCSs through
> phylink_config then each port will needs its own config. This may prove
> difficult to integrate with the existing API.

This might be a misunderstanding. Also here there is only a single
phylink_config for each MAC or DSA port, just instead of having many
more or less identical implementations of .mac_select_pcs, this
functionality is moved into phylink. As a nice side-effect that also
makes managing the life-cycle of the PCS more easy, so we won't need all
the wrappers for all the PCS OPs.

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Sean Anderson]

On 5/13/25 15:03, Daniel Golle wrote:
> On Tue, May 13, 2025 at 02:18:02PM -0400, Sean Anderson wrote:
>> On 5/11/25 16:12, Christian Marangi wrote:
>> > Introduce internal handling of PCS for phylink. This is an alternative
>> > to .mac_select_pcs that moves the selection logic of the PCS entirely to
>> > phylink with the usage of the supported_interface value in the PCS
>> > struct.
>> > 
>> > MAC should now provide an array of available PCS in phylink_config in
>> > .available_pcs and fill the .num_available_pcs with the number of
>> > elements in the array. MAC should also define a new bitmap,
>> > pcs_interfaces, in phylink_config to define for what interface mode a
>> > dedicated PCS is required.
>> > 
>> > On phylink_create() this array is parsed and a linked list of PCS is
>> > created based on the PCS passed in phylink_config.
>> > Also the supported_interface value in phylink struct is updated with the
>> > new supported_interface from the provided PCS.
>> > 
>> > On phylink_start() every PCS in phylink PCS list gets attached to the
>> > phylink instance. This is done by setting the phylink value in
>> > phylink_pcs struct to the phylink instance.
>> > 
>> > On phylink_stop(), every PCS in phylink PCS list is detached from the
>> > phylink instance. This is done by setting the phylink value in
>> > phylink_pcs struct to NULL.
>> > 
>> > phylink_validate_mac_and_pcs(), phylink_major_config() and
>> > phylink_inband_caps() are updated to support this new implementation
>> > with the PCS list stored in phylink.
>> > 
>> > They will make use of phylink_validate_pcs_interface() that will loop
>> > for every PCS in the phylink PCS available list and find one that supports
>> > the passed interface.
>> > 
>> > phylink_validate_pcs_interface() applies the same logic of .mac_select_pcs
>> > where if a supported_interface value is not set for the PCS struct, then
>> > it's assumed every interface is supported.
>> > 
>> > A MAC is required to implement either a .mac_select_pcs or make use of
>> > the PCS list implementation. Implementing both will result in a fail
>> > on MAC/PCS validation.
>> > 
>> > phylink value in phylink_pcs struct with this implementation is used to
>> > track from PCS side when it's attached to a phylink instance. PCS driver
>> > will make use of this information to correctly detach from a phylink
>> > instance if needed.
>> > 
>> > The .mac_select_pcs implementation is not changed but it's expected that
>> > every MAC driver migrates to the new implementation to later deprecate
>> > and remove .mac_select_pcs.
>> 
>> This introduces a completely parallel PCS selection system used by a
>> single driver. I don't think we want the maintenance burden and
>> complexity of two systems in perpetuity. So what is your plan for
>> conversion of existing drivers to your new system?
> 
> Moving functionality duplicated in many drivers to a common shared
> implementation is nothing unsual.
> 
> While this series proposes the new mechanism for Airoha SoC, they are
> immediately useful (and long awaited) also for MediaTek and Qualcomm
> SoCs.
>
> Also in the series you posted at least the macb driver (in "[net-next
> PATCH v4 09/11] net: macb: Move most of mac_config to  mac_prepare")
> would benefit from that shared implementation, as all it does in it's
> mac_select_pcs is selecting the PCS by a given phy_interface_t, which is
> what most Ethernet drivers which use more than one PCS are doing in
> their implementatio of mac_select_pcs().

I generally agree. The vast majority of select_pcs implementations just
determine the PCS based on the interface. But I don't think this is the
right approach. This touches a lot of other areas of the code and
reimplements much of the existing phylink machinery.

I would prefer something more self-contained like

phylink_generic_select_pcs(phylink, interface)
{
	for_each(pcs : phylink->available_pcss) {
		if (test_bit(pcs->supported, interface))
			return pcs;
	}

	return NULL;
}

which could be dropped into existing implementations in an incremental
manner.

I think the inclusion of PCS lifetime management in this patch
complicates things significantly.

> Also axienet_mac_select_pcs() from "[net-next PATCH v4 08/11] net:
> axienet: Convert to use PCS subsystem" could obviously very easily be
> mirated to use the phylink-internal handling of PCS selection.

But the bulk of the patch remains. We still have to add the external PCS
lookup. There still needs to be another case in mac_config to mux the
PCS correctly. And we would need to add some code to create a list of
PCSs. I don't know whether we win on the balance.

>> 
>> DSA drivers typically have different PCSs for each port. At the moment
>> these are selected with mac_select_pcs, allowing the use of a single
>> phylink_config for each port. If you need to pass PCSs through
>> phylink_config then each port will needs its own config. This may prove
>> difficult to integrate with the existing API.
> 
> This might be a misunderstanding. Also here there is only a single
> phylink_config for each MAC or DSA port,

My point is that while this is the case at the moment, it would not be
the case with a "generic" select pcs. You would need to modify the
config for each port to ensure the right PCSs are passed in.

> just instead of having many
> more or less identical implementations of .mac_select_pcs, this
> functionality is moved into phylink. As a nice side-effect that also
> makes managing the life-cycle of the PCS more easy, so we won't need all
> the wrappers for all the PCS OPs.

I think the wrapper approach is very obviously correct. This way has me
worried about exciting new concurrency bugs.

--Sean

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Sean Anderson]

On 5/13/25 15:23, Sean Anderson wrote:
> On 5/13/25 15:03, Daniel Golle wrote:
>> On Tue, May 13, 2025 at 02:18:02PM -0400, Sean Anderson wrote:
>>> DSA drivers typically have different PCSs for each port. At the moment
>>> these are selected with mac_select_pcs, allowing the use of a single
>>> phylink_config for each port. If you need to pass PCSs through
>>> phylink_config then each port will needs its own config. This may prove
>>> difficult to integrate with the existing API.
>> 
>> This might be a misunderstanding. Also here there is only a single
>> phylink_config for each MAC or DSA port,
> 
> My point is that while this is the case at the moment, it would not be
> the case with a "generic" select pcs. You would need to modify the
> config for each port to ensure the right PCSs are passed in.

I reread dsa_port_phylink_create and I think I missed the ds/dp
distinction the first time around.

--Sean

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Daniel Golle]

On Tue, May 13, 2025 at 03:23:32PM -0400, Sean Anderson wrote:
> On 5/13/25 15:03, Daniel Golle wrote:
> > just instead of having many
> > more or less identical implementations of .mac_select_pcs, this
> > functionality is moved into phylink. As a nice side-effect that also
> > makes managing the life-cycle of the PCS more easy, so we won't need all
> > the wrappers for all the PCS OPs.
> 
> I think the wrapper approach is very obviously correct. This way has me
> worried about exciting new concurrency bugs.

You may not be surprised to read that this was also our starting point 2
months ago, I had implemented support for standalone PCS very similar to
the approach you have published now, using refcnt'ed instances and
locked wrapper functions for all OPs. My approach, like yours, was to
create a new subsystem for standalone PCS drivers which is orthogonal to
phylink and only requires very few very small changes to phylink itself.
It was a draft and not as complete and well-documented like your series
now, of course.

I've then shared that implementation with Christian and some other
experienced OpenWrt developers and we concluded that having phylink handle
the PCS lifecycle and PCS selection would be the better and more elegant
approach for multiple reasons:
 - The lifetime management of the wrapper instances becomes tricky:
   We would either have to live with them being allocated by the
   MAC-driver (imagine test-case doing unbind and then bind in a loop
   for a while -- we would end up oom). Or we need some kind of garbage
   collecting mechanism which frees the wrapper once refcnt is zero --
   and as .select_pcs would 'get' the PCS (ie. bump refcnt) we'd need a
   'put' equivalent (eg. a .pcs_destroy() OP) in phylink.

   Russell repeatedly pointed me to the possibility of a PCS
   "disappearing" (and potentially "reappearing" some time later), and
   in this case it is unclear who would then ever call pcs_put(), or
   even notify the Ethernet driver or phylink about the PCS now being
   available (again). Using device_link_add(), like it is done in
   pcs-rzn1-miic.c, prevents the worst (ie. use-after-free), but also
   impacts all other netdevs exposed by the same Ethernet driver
   instance, and has a few other rather ugly implications.

 - phylink currently expects .mac_select_pcs to never fail. But we may
   need a mechanism similar to probe deferral in case the PCS is not
   yet available.
   Your series partially solves this in patch 11/11 "of: property: Add
   device link support for PCS", but also that still won't make the link
   come back in case of a PCS showing up late to the party, eg. due to
   constraints such as phy drivers (drivers/phy, not drivers/net/phy)
   waiting for nvmem providers, or PCS instances "going away" and
   "coming back" later.

 - removal of a PCS instance (eg. via sysfs unbind) would still
   require changes to phylink. there is no phylink function to
   impair the link in this case, and using dev_close() is a bit ugly,
   and also won't bring the link back up once the PCS (re-)appears.

 - phylink anyway is the only user of PCS drivers, and will very likely
   always be. So why create another subsystem?

All that being said I also see potential problems with Christians
current implementation as it doesn't prevent the Ethernet driver to
still store a pointer to struct phylink_pcs (returned eg. from
fwnode_pcs_get()).

Hence I would like to see an even more tight integration with phylink,
in the sense that pointers to 'struct phylink_pcs' should never be
exposed to the MAC driver, as only in that way we can be sure that
phylink, and only phylink, is responsible for reacting to a PCS "going
away".

Ie. instead of fwnode_phylink_pcs_parse() handing pointers to struct
phylink_pcs to the Ethernet driver, so it can use it to populate struct
phylink_config available_pcs member, this should be the responsibility
of phylink alltogether, directly populating the list of available PCS in
phylink's private structure.

Similarly, there should not be fwnode_pcs_get() but rather phylink
providing a function fwnode_phylink_pcs_register(phylink, fwnode) which
directly adds the PCS referenced to the internal list of available PCS.

I hope we can pick the best of all the suggested implementations, and
together come up with something even better.

Re: [net-next PATCH v4 07/11] dt-bindings: net: ethernet-controller: permit to define multiple PCS

[Rob Herring]

On Sun, May 11, 2025 at 10:12:33PM +0200, Christian Marangi wrote:
> Drop the limitation of a single PCS in pcs-handle property. Multiple PCS
> can be defined for an ethrnet-controller node to support various PHY

typo

> interface mode type.
> 
> It's very common for SoCs to have a 2 or more dedicated PCS for Base-X
> (for example SGMII, 1000base-x, 2500base-x, ...) and Base-R (for example
> USXGMII,10base-r, ...) with the MAC selecting one of the other based on
> the attached PHY.

I'm confused what you need. The restriction was no arg cells allowed. 
Any number of phandles was allowed. The former would need a 
"#pcs-handle-cells" type property.

> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-controller.yaml | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 7cbf11bbe99c..60605b34d242 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -84,8 +84,6 @@ properties:
>  
>    pcs-handle:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
> -    items:
> -      maxItems: 1
>      description:
>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>        bus to link with an external PHY (phy-handle) if exists.
> -- 
> 2.48.1
> 

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Sean Anderson]

On 5/13/25 23:00, Daniel Golle wrote:
> On Tue, May 13, 2025 at 03:23:32PM -0400, Sean Anderson wrote:
>> On 5/13/25 15:03, Daniel Golle wrote:
>> > just instead of having many
>> > more or less identical implementations of .mac_select_pcs, this
>> > functionality is moved into phylink. As a nice side-effect that also
>> > makes managing the life-cycle of the PCS more easy, so we won't need all
>> > the wrappers for all the PCS OPs.
>> 
>> I think the wrapper approach is very obviously correct. This way has me
>> worried about exciting new concurrency bugs.
> 
> You may not be surprised to read that this was also our starting point 2
> months ago, I had implemented support for standalone PCS very similar to
> the approach you have published now, using refcnt'ed instances and
> locked wrapper functions for all OPs. My approach, like yours, was to
> create a new subsystem for standalone PCS drivers which is orthogonal to
> phylink and only requires very few very small changes to phylink itself.
> It was a draft and not as complete and well-documented like your series
> now, of course.
> 
> I've then shared that implementation with Christian and some other
> experienced OpenWrt developers and we concluded that having phylink handle
> the PCS lifecycle and PCS selection would be the better and more elegant
> approach for multiple reasons:
>  - The lifetime management of the wrapper instances becomes tricky:
>    We would either have to live with them being allocated by the
>    MAC-driver (imagine test-case doing unbind and then bind in a loop
>    for a while -- we would end up oom). Or we need some kind of garbage
>    collecting mechanism which frees the wrapper once refcnt is zero --
>    and as .select_pcs would 'get' the PCS (ie. bump refcnt) we'd need a
>    'put' equivalent (eg. a .pcs_destroy() OP) in phylink.
> 
>    Russell repeatedly pointed me to the possibility of a PCS
>    "disappearing" (and potentially "reappearing" some time later), and
>    in this case it is unclear who would then ever call pcs_put(), or
>    even notify the Ethernet driver or phylink about the PCS now being
>    available (again). Using device_link_add(), like it is done in
>    pcs-rzn1-miic.c, prevents the worst (ie. use-after-free), but also
>    impacts all other netdevs exposed by the same Ethernet driver
>    instance, and has a few other rather ugly implications.

SRCU neatly solves the lifetime management issues. The wrapper lives as
long as anyone (provider or user) holds a reference. A PCS can disappear
at any point and everything still works (although the link goes down).
Device links are only an optimization; they cannot be relied on for
correctness.

>  - phylink currently expects .mac_select_pcs to never fail. But we may
>    need a mechanism similar to probe deferral in case the PCS is not
>    yet available.

Which is why you grab the PCS in probe. If you want to be more dynamic,
you can do it in netdev open like is done for PHYs.

>    Your series partially solves this in patch 11/11 "of: property: Add
>    device link support for PCS", but also that still won't make the link
>    come back in case of a PCS showing up late to the party, eg. due to
>    constraints such as phy drivers (drivers/phy, not drivers/net/phy)
>    waiting for nvmem providers, or PCS instances "going away" and
>    "coming back" later.

This all works correctly due to device links. The only case that doesn't
work automatically is something like

MAC built-in
  MDIO built-in
    PCS module

where the PCS module gets loaded late. In that case you have to manually
re-probe the MAC. I think the best way to address this would be to grab
the PCS in netdev open so that the MAC can probe without the PCS.

>  - removal of a PCS instance (eg. via sysfs unbind) would still
>    require changes to phylink. there is no phylink function to
>    impair the link in this case, and using dev_close() is a bit ugly,
>    and also won't bring the link back up once the PCS (re-)appears.

This works just fine. There are two cases:

- If the PCS has an IRQ, we notify phylink and then it polls the PCS
  (see below).
- If the PCS is polled, phylink will call pcs_get_state and see that the
  link is down.

Either way, the link goes down. But bringing the link back up is pretty
unusual anyway. Unlike PHYs (which theoretically can be on removable
busses) PCSs are generally permanently attached to their MACs. The only
removable scenario I can think of is if the PCS is on an FPGA and the
MAC is not.

So if the PCS goes away, the MAC is likely to follow shortly after
(since the whole thing is on a removable bus). Or someone has manually
removed the PCS, in which case I think it's reasonable to have them
manually remove the MAC as well. If you really want to support this,
then just grab the PCS in netdev open.

>  - phylink anyway is the only user of PCS drivers, and will very likely
>    always be. So why create another subsystem?

To avoid adding overhead for the majority of PCSs where the PCS is built
into the MAC and literally can't be removed. We only pay the price for
dynamicism on the drivers where it matters.

> All that being said I also see potential problems with Christians
> current implementation as it doesn't prevent the Ethernet driver to
> still store a pointer to struct phylink_pcs (returned eg. from
> fwnode_pcs_get()).
> 
> Hence I would like to see an even more tight integration with phylink,
> in the sense that pointers to 'struct phylink_pcs' should never be
> exposed to the MAC driver, as only in that way we can be sure that
> phylink, and only phylink, is responsible for reacting to a PCS "going
> away".

OK, but then how does the MAC select the PCS? If there are multiple PCSs
then ultimately someone has to configure a mux somewhere.

> Ie. instead of fwnode_phylink_pcs_parse() handing pointers to struct
> phylink_pcs to the Ethernet driver, so it can use it to populate struct
> phylink_config available_pcs member, this should be the responsibility
> of phylink alltogether, directly populating the list of available PCS in
> phylink's private structure.
> 
> Similarly, there should not be fwnode_pcs_get() but rather phylink
> providing a function fwnode_phylink_pcs_register(phylink, fwnode) which
> directly adds the PCS referenced to the internal list of available PCS.

This is difficult to work with for existing drivers. Many of them have
non-standard ways of looking up their PCS that they need to support for
backwards-compatibility. And some of them create the PCS themselves
(such as if they are PCI devices with internal MDIO busses). It's much
easier for the MAC to create or look up the PCS itself and then hand it
off to phylink.

> I hope we can pick the best of all the suggested implementations, and
> together come up with something even better.

Sure. And I think we were starting from a clean slate then this would be
the obvious way to do things. But we must support existing drivers and
provide an upgrade path for them. This is why I favor an incremental
approach.

--Sean

Re: [net-next PATCH v4 03/11] net: phylink: introduce internal phylink PCS handling

[Sean Anderson]

On 5/19/25 14:10, Sean Anderson wrote:
> On 5/13/25 23:00, Daniel Golle wrote:
>> On Tue, May 13, 2025 at 03:23:32PM -0400, Sean Anderson wrote:
>>> On 5/13/25 15:03, Daniel Golle wrote:
>>> > just instead of having many
>>> > more or less identical implementations of .mac_select_pcs, this
>>> > functionality is moved into phylink. As a nice side-effect that also
>>> > makes managing the life-cycle of the PCS more easy, so we won't need all
>>> > the wrappers for all the PCS OPs.
>>> 
>>> I think the wrapper approach is very obviously correct. This way has me
>>> worried about exciting new concurrency bugs.
>> 
>> You may not be surprised to read that this was also our starting point 2
>> months ago, I had implemented support for standalone PCS very similar to
>> the approach you have published now, using refcnt'ed instances and
>> locked wrapper functions for all OPs. My approach, like yours, was to
>> create a new subsystem for standalone PCS drivers which is orthogonal to
>> phylink and only requires very few very small changes to phylink itself.
>> It was a draft and not as complete and well-documented like your series
>> now, of course.
>> 
>> I've then shared that implementation with Christian and some other
>> experienced OpenWrt developers and we concluded that having phylink handle
>> the PCS lifecycle and PCS selection would be the better and more elegant
>> approach for multiple reasons:
>>  - The lifetime management of the wrapper instances becomes tricky:
>>    We would either have to live with them being allocated by the
>>    MAC-driver (imagine test-case doing unbind and then bind in a loop
>>    for a while -- we would end up oom). Or we need some kind of garbage
>>    collecting mechanism which frees the wrapper once refcnt is zero --
>>    and as .select_pcs would 'get' the PCS (ie. bump refcnt) we'd need a
>>    'put' equivalent (eg. a .pcs_destroy() OP) in phylink.
>> 
>>    Russell repeatedly pointed me to the possibility of a PCS
>>    "disappearing" (and potentially "reappearing" some time later), and
>>    in this case it is unclear who would then ever call pcs_put(), or
>>    even notify the Ethernet driver or phylink about the PCS now being
>>    available (again). Using device_link_add(), like it is done in
>>    pcs-rzn1-miic.c, prevents the worst (ie. use-after-free), but also
>>    impacts all other netdevs exposed by the same Ethernet driver
>>    instance, and has a few other rather ugly implications.
> 
> SRCU neatly solves the lifetime management issues. The wrapper lives as
> long as anyone (provider or user) holds a reference. A PCS can disappear
> at any point and everything still works (although the link goes down).
> Device links are only an optimization; they cannot be relied on for
> correctness.
> 
>>  - phylink currently expects .mac_select_pcs to never fail. But we may
>>    need a mechanism similar to probe deferral in case the PCS is not
>>    yet available.
> 
> Which is why you grab the PCS in probe. If you want to be more dynamic,
> you can do it in netdev open like is done for PHYs.
> 
>>    Your series partially solves this in patch 11/11 "of: property: Add
>>    device link support for PCS", but also that still won't make the link
>>    come back in case of a PCS showing up late to the party, eg. due to
>>    constraints such as phy drivers (drivers/phy, not drivers/net/phy)
>>    waiting for nvmem providers, or PCS instances "going away" and
>>    "coming back" later.
> 
> This all works correctly due to device links. The only case that doesn't
> work automatically is something like
> 
> MAC built-in
>   MDIO built-in
>     PCS module
> 
> where the PCS module gets loaded late. In that case you have to manually
> re-probe the MAC. I think the best way to address this would be to grab
> the PCS in netdev open so that the MAC can probe without the PCS.
> 
>>  - removal of a PCS instance (eg. via sysfs unbind) would still
>>    require changes to phylink. there is no phylink function to
>>    impair the link in this case, and using dev_close() is a bit ugly,
>>    and also won't bring the link back up once the PCS (re-)appears.
> 
> This works just fine. There are two cases:
> 
> - If the PCS has an IRQ, we notify phylink and then it polls the PCS
>   (see below).
> - If the PCS is polled, phylink will call pcs_get_state and see that the
>   link is down.
> 
> Either way, the link goes down. But bringing the link back up is pretty
> unusual anyway. Unlike PHYs (which theoretically can be on removable
> busses) PCSs are generally permanently attached to their MACs. The only
> removable scenario I can think of is if the PCS is on an FPGA and the
> MAC is not.
> 
> So if the PCS goes away, the MAC is likely to follow shortly after
> (since the whole thing is on a removable bus). Or someone has manually
> removed the PCS, in which case I think it's reasonable to have them
> manually remove the MAC as well. If you really want to support this,
> then just grab the PCS in netdev open.

So I had a closer look at this and unfortunately it isn't as easy as
just grabbing the PCS in ndo_open. The problem is that we need to
know the supported interfaces before phylink_create. The interfaces are
validated and are visible to userspace as soon as the netdev is
registered. And we can't just defer phylink_create to ndo_open because a
lot of the ethtool ops are implemented with phylink. So this would
probably need something like phylink_update_supported_interfaces().

But TBH I don't think this use case is very relevant. As I said above,
it only affects FPGA reconfiguration and people manually unbinding
drivers. Either way I think they are savvy enough to reprobe the netdev.

--Sean

>>  - phylink anyway is the only user of PCS drivers, and will very likely
>>    always be. So why create another subsystem?
> 
> To avoid adding overhead for the majority of PCSs where the PCS is built
> into the MAC and literally can't be removed. We only pay the price for
> dynamicism on the drivers where it matters.
> 
>> All that being said I also see potential problems with Christians
>> current implementation as it doesn't prevent the Ethernet driver to
>> still store a pointer to struct phylink_pcs (returned eg. from
>> fwnode_pcs_get()).
>> 
>> Hence I would like to see an even more tight integration with phylink,
>> in the sense that pointers to 'struct phylink_pcs' should never be
>> exposed to the MAC driver, as only in that way we can be sure that
>> phylink, and only phylink, is responsible for reacting to a PCS "going
>> away".
> 
> OK, but then how does the MAC select the PCS? If there are multiple PCSs
> then ultimately someone has to configure a mux somewhere.
> 
>> Ie. instead of fwnode_phylink_pcs_parse() handing pointers to struct
>> phylink_pcs to the Ethernet driver, so it can use it to populate struct
>> phylink_config available_pcs member, this should be the responsibility
>> of phylink alltogether, directly populating the list of available PCS in
>> phylink's private structure.
>> 
>> Similarly, there should not be fwnode_pcs_get() but rather phylink
>> providing a function fwnode_phylink_pcs_register(phylink, fwnode) which
>> directly adds the PCS referenced to the internal list of available PCS.
> 
> This is difficult to work with for existing drivers. Many of them have
> non-standard ways of looking up their PCS that they need to support for
> backwards-compatibility. And some of them create the PCS themselves
> (such as if they are PCI devices with internal MDIO busses). It's much
> easier for the MAC to create or look up the PCS itself and then hand it
> off to phylink.
> 
>> I hope we can pick the best of all the suggested implementations, and
>> together come up with something even better.
> 
> Sure. And I think we were starting from a clean slate then this would be
> the obvious way to do things. But we must support existing drivers and
> provide an upgrade path for them. This is why I favor an incremental
> approach.
> 
> --Sean