[PATCH net-next v6 00/14] Introduce an ethernet port representation

[PATCH net-next v6 00/14] Introduce an ethernet port representation

[Maxime Chevallier]

Hi everyone,

Here's a V6 for the ongoing support for Ethernet media-side port
support.

This V6 addresses Jakub's and Simon's comments, but could use some
review on the overall approach, any comment is welcome :)

Before going into the details, a few important notes :

 - This is only a first phase. It instantiates the port, and leverage
   that to make the MAC <-> PHY <-> SFP usecase simpler.

 - Next phase will deal with controlling the port state, as well as the
   netlink uAPI for that.

 - The end-goal is to enable support for complex port MUX. This
   preliminary work focuses on PHY-driven ports, but this will be
   extended to support muxing at the MII level (Multi-phy, or compo PHY
   + SFP as found on Turris Omnia for example).

 - The naming is definitely not set in stone. I named that "phy_port",
   but this may convey the false sense that this is phylib-specific.
   Even the word "port" is not that great, as it already has several
   different meanings in the net world (switch port, devlink port,
   etc.). I used the term "connector" in the binding.

A bit of history on that work :

The end goal that I personnaly want to achieve is :

            + PHY - RJ45
            | 
 MAC - MUX -+ PHY - RJ45

After many discussions here on netdev@, but also at netdevconf[1] and
LPC[2], there appears to be several analoguous designs that exist out
there.

[1] : https://netdevconf.info/0x17/sessions/talk/improving-multi-phy-and-multi-port-interfaces.html
[2] : https://lpc.events/event/18/contributions/1964/ (video isn't the
right one)

Take the MAchiatobin, it has 2 interfaces that looks like this :

 MAC - PHY -+ RJ45
            |
	    + SFP - Whatever the module does

Now, looking at the Turris Omnia, we have :


 MAC - MUX -+ PHY - RJ45
            |
	    + SFP - Whatever the module does

We can find more example of this kind of designs, the common part is
that we expose multiple front-facing media ports. This is what this
current work aims at supporting. As of right now, it does'nt add any
support for muxing, but this will come later on.

This first phase focuses on phy-driven ports only, but there are already
quite some challenges already. For one, we can't really autodetect how
many ports are sitting behind a PHY. That's why this series introduces a
new binding. Describing ports in DT should however be a last-resort
thing when we need to clear some ambiguity about the PHY media-side.

The only use-cases that we have today for multi-port PHYs are combo PHYs
that drive both a Copper port and an SFP (the Macchiatobin case). This
in itself is challenging and this series only addresses part of this
support, by registering a phy_port for the PHY <-> SFP connection. The
SFP module should in the end be considered as a port as well, but that's
not yet the case.

However, because now PHYs can register phy_ports for every media-side
interface they have, they can register the capabilities of their ports,
which allows making the PHY-driver SFP case much more generic.

Let me know what you think, I'm all in for discussions :)

Regards,

Changes in V6:

 - Fixed kdoc on patch 3
 - Addressed a missing port-ops registration for the Marvell 88x2222
   driver
 - Addressed a warning reported by Simon on the DP83822 when building
   without CONFIG_OF_MDIO

Changes in V5 :

 - renamed the bindings to use the term "connector" instead of "port"
 - Rebased, and fixed some issues reported on the 83822 driver
 - Use phy_caps

Changes in V4 :

 - Introduced a kernel doc
 - Reworked the mediums definitions in patch 2
 - QCA807x now uses the generic SFP support
 - Fixed some implementation bugs to build the support list based on the
   interfaces supported on a port

V5: https://lore.kernel.org/netdev/20250425141511.182537-1-maxime.chevallier@bootlin.com/
V4: https://lore.kernel.org/netdev/20250213101606.1154014-1-maxime.chevallier@bootlin.com/
V3: https://lore.kernel.org/netdev/20250207223634.600218-1-maxime.chevallier@bootlin.com/
RFC V2: https://lore.kernel.org/netdev/20250122174252.82730-1-maxime.chevallier@bootlin.com/
RFC V1: https://lore.kernel.org/netdev/20241220201506.2791940-1-maxime.chevallier@bootlin.com/

Maxime



Maxime Chevallier (14):
  dt-bindings: net: Introduce the ethernet-connector description
  net: ethtool: Introduce ETHTOOL_LINK_MEDIUM_* values
  net: phy: Introduce PHY ports representation
  net: phy: dp83822: Add support for phy_port representation
  net: phy: Create a phy_port for PHY-driven SFPs
  net: phy: Introduce generic SFP handling for PHY drivers
  net: phy: marvell-88x2222: Support SFP through phy_port interface
  net: phy: marvell: Support SFP through phy_port interface
  net: phy: marvell10g: Support SFP through phy_port
  net: phy: at803x: Support SFP through phy_port interface
  net: phy: qca807x: Support SFP through phy_port interface
  net: phy: Only rely on phy_port for PHY-driven SFP
  net: phy: dp83822: Add SFP support through the phy_port interface
  Documentation: networking: Document the phy_port infrastructure

 .../bindings/net/ethernet-connector.yaml      |  47 +++
 .../devicetree/bindings/net/ethernet-phy.yaml |  18 +
 Documentation/networking/index.rst            |   1 +
 Documentation/networking/phy-port.rst         | 111 +++++++
 MAINTAINERS                                   |   3 +
 drivers/net/phy/Makefile                      |   3 +-
 drivers/net/phy/dp83822.c                     |  78 +++--
 drivers/net/phy/marvell-88x2222.c             |  98 +++---
 drivers/net/phy/marvell.c                     | 100 +++---
 drivers/net/phy/marvell10g.c                  |  37 +--
 drivers/net/phy/phy_device.c                  | 312 +++++++++++++++++-
 drivers/net/phy/phy_port.c                    | 188 +++++++++++
 drivers/net/phy/qcom/at803x.c                 |  64 +---
 drivers/net/phy/qcom/qca807x.c                |  75 ++---
 include/linux/ethtool.h                       |  33 +-
 include/linux/phy.h                           |  38 ++-
 include/linux/phy_port.h                      |  93 ++++++
 include/uapi/linux/ethtool.h                  |  20 ++
 net/ethtool/common.c                          | 265 +++++++++------
 19 files changed, 1197 insertions(+), 387 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet-connector.yaml
 create mode 100644 Documentation/networking/phy-port.rst
 create mode 100644 drivers/net/phy/phy_port.c
 create mode 100644 include/linux/phy_port.h

-- 
2.49.0

[PATCH net-next v6 01/14] dt-bindings: net: Introduce the ethernet-connector description

[Maxime Chevallier]

The ability to describe the physical ports of Ethernet devices is useful
to describe multi-port devices, as well as to remove any ambiguity with
regard to the nature of the port.

Moreover, describing ports allows for a better description of features
that are tied to connectors, such as PoE through the PSE-PD devices.

Introduce a binding to allow describing the ports, for now with 2
attributes :

 - The number of lanes, which is a quite generic property that allows
   differentating between multiple similar technologies such as BaseT1
   and "regular" BaseT (which usually means BaseT4).

 - The media that can be used on that port, such as BaseT for Twisted
   Copper, BaseC for coax copper, BaseS/L for Fiber, BaseK for backplane
   ethernet, etc. This allows defining the nature of the port, and
   therefore avoids the need for vendor-specific properties such as
   "micrel,fiber-mode" or "ti,fiber-mode".

The port description lives in its own file, as it is intended in the
future to allow describing the ports for phy-less devices.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../bindings/net/ethernet-connector.yaml      | 47 +++++++++++++++++++
 .../devicetree/bindings/net/ethernet-phy.yaml | 18 +++++++
 MAINTAINERS                                   |  1 +
 3 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet-connector.yaml

diff --git a/Documentation/devicetree/bindings/net/ethernet-connector.yaml b/Documentation/devicetree/bindings/net/ethernet-connector.yaml
new file mode 100644
index 000000000000..2aa28e6c1523
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ethernet-connector.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ethernet-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic Ethernet Connector
+
+maintainers:
+  - Maxime Chevallier <maxime.chevallier@bootlin.com>
+
+description:
+  An Ethernet Connectr represents the output of a network component such as
+  a PHY, an Ethernet controller with no PHY, or an SFP module.
+
+properties:
+
+  lanes:
+    description:
+      Defines the number of lanes on the port, that is the number of physical
+      channels used to convey the data with the link partner.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  media:
+    description:
+      The mediums, as defined in 802.3, that can be used on the port.
+    items:
+      enum:
+        - BaseT
+        - BaseK
+        - BaseS
+        - BaseC
+        - BaseL
+        - BaseD
+        - BaseE
+        - BaseF
+        - BaseV
+        - BaseMLD
+        - BaseX
+
+required:
+  - lanes
+  - media
+
+additionalProperties: true
+
+...
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 71e2cd32580f..6bf670beb66f 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -277,6 +277,17 @@ properties:
 
     additionalProperties: false
 
+  mdi:
+    type: object
+
+    patternProperties:
+      '^connector-[a-f0-9]+$':
+        $ref: /schemas/net/ethernet-connector.yaml#
+
+        unevaluatedProperties: false
+
+    additionalProperties: false
+
 required:
   - reg
 
@@ -313,5 +324,12 @@ examples:
                     default-state = "keep";
                 };
             };
+
+            mdi {
+              connector-0 {
+                lanes = <2>;
+                media = "BaseT";
+              };
+            };
         };
     };
diff --git a/MAINTAINERS b/MAINTAINERS
index 5c31814c9687..b59a9209fcb1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8777,6 +8777,7 @@ R:	Russell King <linux@armlinux.org.uk>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/ABI/testing/sysfs-class-net-phydev
+F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
 F:	Documentation/devicetree/bindings/net/ethernet-phy.yaml
 F:	Documentation/devicetree/bindings/net/mdio*
 F:	Documentation/devicetree/bindings/net/qca,ar803x.yaml
-- 
2.49.0

[PATCH net-next v6 02/14] net: ethtool: Introduce ETHTOOL_LINK_MEDIUM_* values

[Maxime Chevallier]

In an effort to have a better representation of Ethernet ports,
introduce enumeration values representing the various ethernet Mediums.

This is part of the 802.3 naming convention, for example :

1000 Base T 4
 |    |   | |
 |    |   | \_ lanes (4)
 |    |   \___ Medium (T == Twisted Copper Pairs)
 |    \_______ Baseband transmission
 \____________ Speed

 Other example :

10000 Base K X 4
           | | \_ lanes (4)
           | \___ encoding (BaseX is 8b/10b while BaseR is 66b/64b)
           \_____ Medium (K is backplane ethernet)

In the case of representing a physical port, only the medium and number
of lanes should be relevant. One exception would be 1000BaseX, which is
currently also used as a medium in what appears to be any of
1000BaseSX, 1000BaseCX and 1000BaseLX. This was reflected in the mediums
associated with the 1000BaseX linkmode.

These mediums are set in the net/ethtool/common.c lookup table that
maintains a list of all linkmodes with their number of lanes, medium,
encoding, speed and duplex.

One notable exception to this is 100M BaseT Ethernet. 100BaseTX is a
2-lanes protocol but it will also work on 4-lanes cables, so the lookup
table contains 2 sets of lane numbers, indicating the min number of lanes
for a protocol to work and the "nominal" number of lanes as well.

Another set of exceptions are linkmodes such 100000baseLR4_ER4, where
the same link mode seems to represent 100GBaseLR4 and 100GBaseER4. The
macro __DEFINE_LINK_MODE_PARAMS_MEDIUMS is here used to populate the
.mediums bitfield with all appropriate mediums.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/linux/ethtool.h      |  18 ++-
 include/uapi/linux/ethtool.h |  20 +++
 net/ethtool/common.c         | 265 +++++++++++++++++++++--------------
 3 files changed, 191 insertions(+), 112 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 117718c24814..c1d805d3e02f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -215,13 +215,25 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
 void ethtool_rxfh_context_lost(struct net_device *dev, u32 context_id);
 
 struct link_mode_info {
-	int                             speed;
-	u8                              lanes;
-	u8                              duplex;
+	int	speed;
+	u8	min_lanes;
+	u8	lanes;
+	u8	duplex;
+	u16	mediums;
 };
 
 extern const struct link_mode_info link_mode_params[];
 
+extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
+
+static inline const char *phy_mediums(enum ethtool_link_medium medium)
+{
+	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
+		return "unknown";
+
+	return ethtool_link_medium_names[medium];
+}
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 84833cca29fe..2810cbb635dd 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2582,4 +2582,24 @@ enum phy_upstream {
 	PHY_UPSTREAM_PHY,
 };
 
+enum ethtool_link_medium {
+	ETHTOOL_LINK_MEDIUM_BASET = 0,
+	ETHTOOL_LINK_MEDIUM_BASEK,
+	ETHTOOL_LINK_MEDIUM_BASES,
+	ETHTOOL_LINK_MEDIUM_BASEC,
+	ETHTOOL_LINK_MEDIUM_BASEL,
+	ETHTOOL_LINK_MEDIUM_BASED,
+	ETHTOOL_LINK_MEDIUM_BASEE,
+	ETHTOOL_LINK_MEDIUM_BASEF,
+	ETHTOOL_LINK_MEDIUM_BASEV,
+	ETHTOOL_LINK_MEDIUM_BASEMLD,
+	ETHTOOL_LINK_MEDIUM_NONE,
+
+	__ETHTOOL_LINK_MEDIUM_LAST,
+};
+
+#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
+				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
+				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
+
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 49bea6b45bd5..9f6267eb9759 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -281,12 +281,32 @@ static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 #define __LINK_MODE_LANES_DR8_2		8
 #define __LINK_MODE_LANES_T1BRR		1
 
-#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _duplex)	\
+#define __DEFINE_LINK_MODE_PARAMS_LANES(_speed, _type, _min_lanes, _lanes, _duplex, _medium) \
 	[ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = {		\
 		.speed  = SPEED_ ## _speed, \
+		.min_lanes  = _min_lanes, \
+		.lanes  = _lanes, \
+		.duplex	= __DUPLEX_ ## _duplex, \
+		.mediums = BIT(ETHTOOL_LINK_MEDIUM_BASE ## _medium) \
+	}
+
+#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _duplex, _medium)	\
+	[ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = {		\
+		.speed  = SPEED_ ## _speed, \
+		.min_lanes  = __LINK_MODE_LANES_ ## _type, \
+		.lanes  = __LINK_MODE_LANES_ ## _type, \
+		.duplex	= __DUPLEX_ ## _duplex, \
+		.mediums = BIT(ETHTOOL_LINK_MEDIUM_BASE ## _medium) \
+	}
+#define __DEFINE_LINK_MODE_PARAMS_MEDIUMS(_speed, _type, _duplex, _mediums)	\
+	[ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = {		\
+		.speed  = SPEED_ ## _speed, \
+		.min_lanes  = __LINK_MODE_LANES_ ## _type, \
 		.lanes  = __LINK_MODE_LANES_ ## _type, \
-		.duplex	= __DUPLEX_ ## _duplex \
+		.duplex	= __DUPLEX_ ## _duplex, \
+		.mediums = (_mediums) \
 	}
+#define __MED(_medium)	(BIT(ETHTOOL_LINK_MEDIUM_BASE ## _medium))
 #define __DUPLEX_Half DUPLEX_HALF
 #define __DUPLEX_Full DUPLEX_FULL
 #define __DEFINE_SPECIAL_MODE_PARAMS(_mode) \
@@ -294,138 +314,165 @@ static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 		.speed	= SPEED_UNKNOWN, \
 		.lanes	= 0, \
 		.duplex	= DUPLEX_UNKNOWN, \
+		.mediums = BIT(ETHTOOL_LINK_MEDIUM_NONE), \
 	}
 
 const struct link_mode_info link_mode_params[] = {
-	__DEFINE_LINK_MODE_PARAMS(10, T, Half),
-	__DEFINE_LINK_MODE_PARAMS(10, T, Full),
-	__DEFINE_LINK_MODE_PARAMS(100, T, Half),
-	__DEFINE_LINK_MODE_PARAMS(100, T, Full),
-	__DEFINE_LINK_MODE_PARAMS(1000, T, Half),
-	__DEFINE_LINK_MODE_PARAMS(1000, T, Full),
+	__DEFINE_LINK_MODE_PARAMS_LANES(10, T, 2, 4, Half, T),
+	__DEFINE_LINK_MODE_PARAMS_LANES(10, T, 2, 4, Full, T),
+	__DEFINE_LINK_MODE_PARAMS_LANES(100, T, 2, 4, Half, T),
+	__DEFINE_LINK_MODE_PARAMS_LANES(100, T, 2, 4, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(1000, T, Half, T),
+	__DEFINE_LINK_MODE_PARAMS(1000, T, Full, T),
 	__DEFINE_SPECIAL_MODE_PARAMS(Autoneg),
 	__DEFINE_SPECIAL_MODE_PARAMS(TP),
 	__DEFINE_SPECIAL_MODE_PARAMS(AUI),
 	__DEFINE_SPECIAL_MODE_PARAMS(MII),
 	__DEFINE_SPECIAL_MODE_PARAMS(FIBRE),
 	__DEFINE_SPECIAL_MODE_PARAMS(BNC),
-	__DEFINE_LINK_MODE_PARAMS(10000, T, Full),
+	__DEFINE_LINK_MODE_PARAMS(10000, T, Full, T),
 	__DEFINE_SPECIAL_MODE_PARAMS(Pause),
 	__DEFINE_SPECIAL_MODE_PARAMS(Asym_Pause),
-	__DEFINE_LINK_MODE_PARAMS(2500, X, Full),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(2500, X, Full,
+					  __MED(C) | __MED(S) | __MED(L)),
 	__DEFINE_SPECIAL_MODE_PARAMS(Backplane),
-	__DEFINE_LINK_MODE_PARAMS(1000, KX, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, KX4, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, KR, Full),
+	__DEFINE_LINK_MODE_PARAMS(1000, KX, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(10000, KX4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(10000, KR, Full, K),
 	[ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = {
 		.speed	= SPEED_10000,
 		.lanes	= 1,
 		.duplex = DUPLEX_FULL,
 	},
-	__DEFINE_LINK_MODE_PARAMS(20000, MLD2, Full),
-	__DEFINE_LINK_MODE_PARAMS(20000, KR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(40000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(40000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(40000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(40000, LR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(56000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(56000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(56000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(56000, LR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(25000, CR, Full),
-	__DEFINE_LINK_MODE_PARAMS(25000, KR, Full),
-	__DEFINE_LINK_MODE_PARAMS(25000, SR, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, CR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, KR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, LR4_ER4, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, SR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(1000, X, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, CR, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, SR, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, LR, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, LRM, Full),
-	__DEFINE_LINK_MODE_PARAMS(10000, ER, Full),
-	__DEFINE_LINK_MODE_PARAMS(2500, T, Full),
-	__DEFINE_LINK_MODE_PARAMS(5000, T, Full),
+	__DEFINE_LINK_MODE_PARAMS(20000, MLD2, Full, MLD),
+	__DEFINE_LINK_MODE_PARAMS(20000, KR2, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(40000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(40000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(40000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(40000, LR4, Full, L),
+	__DEFINE_LINK_MODE_PARAMS(56000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(56000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(56000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(56000, LR4, Full, L),
+	__DEFINE_LINK_MODE_PARAMS(25000, CR, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(25000, KR, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(25000, SR, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(50000, CR2, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(50000, KR2, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(100000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(100000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(100000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(100000, LR4_ER4, Full,
+					  __MED(L) | __MED(E)),
+	__DEFINE_LINK_MODE_PARAMS(50000, SR2, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(1000, X, Full,
+					  __MED(C) | __MED(S) | __MED(L)),
+	__DEFINE_LINK_MODE_PARAMS(10000, CR, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(10000, SR, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(10000, LR, Full, L),
+	__DEFINE_LINK_MODE_PARAMS(10000, LRM, Full, L),
+	__DEFINE_LINK_MODE_PARAMS(10000, ER, Full, E),
+	__DEFINE_LINK_MODE_PARAMS(2500, T, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(5000, T, Full, T),
 	__DEFINE_SPECIAL_MODE_PARAMS(FEC_NONE),
 	__DEFINE_SPECIAL_MODE_PARAMS(FEC_RS),
 	__DEFINE_SPECIAL_MODE_PARAMS(FEC_BASER),
-	__DEFINE_LINK_MODE_PARAMS(50000, KR, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, SR, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, CR, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, LR_ER_FR, Full),
-	__DEFINE_LINK_MODE_PARAMS(50000, DR, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, KR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, SR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, CR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, LR2_ER2_FR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, DR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, LR4_ER4_FR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, DR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(100, T1, Full),
-	__DEFINE_LINK_MODE_PARAMS(1000, T1, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, KR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, SR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, LR8_ER8_FR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, DR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, CR8, Full),
+	__DEFINE_LINK_MODE_PARAMS(50000, KR, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(50000, SR, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(50000, CR, Full, C),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(50000, LR_ER_FR, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(50000, DR, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(100000, KR2, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(100000, SR2, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(100000, CR2, Full, C),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(100000, LR2_ER2_FR2, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(100000, DR2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(200000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(200000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(200000, LR4_ER4_FR4, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(200000, DR4, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(200000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(100, T1, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(1000, T1, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(400000, KR8, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(400000, SR8, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(400000, LR8_ER8_FR8, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(400000, DR8, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(400000, CR8, Full, C),
 	__DEFINE_SPECIAL_MODE_PARAMS(FEC_LLRS),
-	__DEFINE_LINK_MODE_PARAMS(100000, KR, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, SR, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, LR_ER_FR, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, DR, Full),
-	__DEFINE_LINK_MODE_PARAMS(100000, CR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, KR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, SR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, LR2_ER2_FR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, DR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, CR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, LR4_ER4_FR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, DR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(100, FX, Half),
-	__DEFINE_LINK_MODE_PARAMS(100, FX, Full),
-	__DEFINE_LINK_MODE_PARAMS(10, T1L, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, CR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, KR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, DR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full),
-	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
-	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
-	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
-	__DEFINE_LINK_MODE_PARAMS(10, T1BRR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, CR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, KR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, DR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, DR_2, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, SR, Full),
-	__DEFINE_LINK_MODE_PARAMS(200000, VR, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, CR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, KR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, DR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, DR2_2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, SR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(400000, VR2, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, CR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, KR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, DR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, DR4_2, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, SR4, Full),
-	__DEFINE_LINK_MODE_PARAMS(800000, VR4, Full),
+	__DEFINE_LINK_MODE_PARAMS(100000, KR, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(100000, SR, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(100000, LR_ER_FR, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(100000, DR, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(100000, CR, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(200000, KR2, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(200000, SR2, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(200000, LR2_ER2_FR2, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(200000, DR2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(200000, CR2, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(400000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(400000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS_MEDIUMS(400000, LR4_ER4_FR4, Full,
+					  __MED(L) | __MED(E) | __MED(F)),
+	__DEFINE_LINK_MODE_PARAMS(400000, DR4, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(400000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(100, FX, Half, F),
+	__DEFINE_LINK_MODE_PARAMS(100, FX, Full, F),
+	__DEFINE_LINK_MODE_PARAMS(10, T1L, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(800000, CR8, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(800000, KR8, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(800000, DR8, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full, V),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S, Half, T),
+	__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half, T),
+	__DEFINE_LINK_MODE_PARAMS(10, T1BRR, Full, T),
+	__DEFINE_LINK_MODE_PARAMS(200000, CR, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(200000, KR, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(200000, DR, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(200000, DR_2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(200000, SR, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(200000, VR, Full, V),
+	__DEFINE_LINK_MODE_PARAMS(400000, CR2, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(400000, KR2, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(400000, DR2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(400000, DR2_2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(400000, SR2, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(400000, VR2, Full, V),
+	__DEFINE_LINK_MODE_PARAMS(800000, CR4, Full, C),
+	__DEFINE_LINK_MODE_PARAMS(800000, KR4, Full, K),
+	__DEFINE_LINK_MODE_PARAMS(800000, DR4, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(800000, DR4_2, Full, D),
+	__DEFINE_LINK_MODE_PARAMS(800000, SR4, Full, S),
+	__DEFINE_LINK_MODE_PARAMS(800000, VR4, Full, V),
 };
 static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 EXPORT_SYMBOL_GPL(link_mode_params);
 
+const char ethtool_link_medium_names[][ETH_GSTRING_LEN] = {
+	[ETHTOOL_LINK_MEDIUM_BASET] = "BaseT",
+	[ETHTOOL_LINK_MEDIUM_BASEK] = "BaseK",
+	[ETHTOOL_LINK_MEDIUM_BASES] = "BaseS",
+	[ETHTOOL_LINK_MEDIUM_BASEC] = "BaseC",
+	[ETHTOOL_LINK_MEDIUM_BASEL] = "BaseL",
+	[ETHTOOL_LINK_MEDIUM_BASED] = "BaseD",
+	[ETHTOOL_LINK_MEDIUM_BASEE] = "BaseE",
+	[ETHTOOL_LINK_MEDIUM_BASEF] = "BaseF",
+	[ETHTOOL_LINK_MEDIUM_BASEV] = "BaseV",
+	[ETHTOOL_LINK_MEDIUM_BASEMLD] = "BaseMLD",
+	[ETHTOOL_LINK_MEDIUM_NONE] = "None",
+};
+static_assert(ARRAY_SIZE(ethtool_link_medium_names) == __ETHTOOL_LINK_MEDIUM_LAST);
+EXPORT_SYMBOL_GPL(ethtool_link_medium_names);
+
 const char netif_msg_class_names[][ETH_GSTRING_LEN] = {
 	[NETIF_MSG_DRV_BIT]		= "drv",
 	[NETIF_MSG_PROBE_BIT]		= "probe",
-- 
2.49.0

[PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Maxime Chevallier]

Ethernet provides a wide variety of layer 1 protocols and standards for
data transmission. The front-facing ports of an interface have their own
complexity and configurability.

Introduce a representation of these front-facing ports. The current code
is minimalistic and only support ports controlled by PHY devices, but
the plan is to extend that to SFP as well as raw Ethernet MACs that
don't use PHY devices.

This minimal port representation allows describing the media and number
of lanes of a port. From that information, we can derive the linkmodes
usable on the port, which can be used to limit the capabilities of an
interface.

For now, the port lanes and medium is derived from devicetree, defined
by the PHY driver, or populated with default values (as we assume that
all PHYs expose at least one port).

The typical example is 100M ethernet. 100BaseT can work using only 2
lanes on a Cat 5 cables. However, in the situation where a 10/100/1000
capable PHY is wired to its RJ45 port through 2 lanes only, we have no
way of detecting that. The "max-speed" DT property can be used, but a
more accurate representation can be used :

mdi {
	connector-0 {
		media = "BaseT";
		lanes = <2>;
	};
};

From that information, we can derive the max speed reachable on the
port.

Another benefit of having that is to avoid vendor-specific DT properties
(micrel,fiber-mode or ti,fiber-mode).

This basic representation is meant to be expanded, by the introduction
of port ops, userspace listing of ports, and support for multi-port
devices.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 MAINTAINERS                  |   1 +
 drivers/net/phy/Makefile     |   3 +-
 drivers/net/phy/phy_device.c | 168 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_port.c   | 171 +++++++++++++++++++++++++++++++++++
 include/linux/ethtool.h      |  15 +++
 include/linux/phy.h          |  30 ++++++
 include/linux/phy_port.h     |  93 +++++++++++++++++++
 7 files changed, 480 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy_port.c
 create mode 100644 include/linux/phy_port.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b59a9209fcb1..b155eec69552 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8797,6 +8797,7 @@ F:	include/linux/of_net.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
 F:	include/linux/phy_link_topology.h
+F:	include/linux/phy_port.h
 F:	include/linux/phylib_stubs.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
 F:	include/linux/platform_data/mdio-gpio.h
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 631859d44451..c0c3575753d7 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -3,7 +3,8 @@
 
 libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
 				   linkmode.o phy_link_topology.o \
-				   phy_package.o phy_caps.o mdio_bus_provider.o
+				   phy_package.o phy_caps.o mdio_bus_provider.o \
+				   phy_port.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2eb735e68dd8..9e0cc6a54497 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,7 @@
 #include <linux/phylib_stubs.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/phy_link_topology.h>
+#include <linux/phy_port.h>
 #include <linux/pse-pd/pse.h>
 #include <linux/property.h>
 #include <linux/ptp_clock_kernel.h>
@@ -699,6 +700,13 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 
 	dev->state = PHY_DOWN;
 	INIT_LIST_HEAD(&dev->leds);
+	INIT_LIST_HEAD(&dev->ports);
+
+	/* The driver's probe function must change that to the real number
+	 * of ports possible on the PHY. We assume by default we are dealing
+	 * with a single-port PHY
+	 */
+	dev->max_n_ports = 1;
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
@@ -1442,6 +1450,46 @@ void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
 }
 EXPORT_SYMBOL(phy_sfp_detach);
 
+static int phy_add_port(struct phy_device *phydev, struct phy_port *port)
+{
+	int ret = 0;
+
+	if (phydev->n_ports == phydev->max_n_ports)
+		return -EBUSY;
+
+	/* We set all ports as active by default, PHY drivers may deactivate
+	 * them (when unused)
+	 */
+	port->active = true;
+
+	if (phydev->drv && phydev->drv->attach_port)
+		ret = phydev->drv->attach_port(phydev, port);
+
+	if (ret)
+		return ret;
+
+	/* The PHY driver might have added, removed or set medium/lanes info,
+	 * so update the port supported accordingly.
+	 */
+	phy_port_update_supported(port);
+
+	list_add(&port->head, &phydev->ports);
+
+	phydev->n_ports++;
+
+	return 0;
+}
+
+static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
+{
+	if (!phydev->n_ports)
+		return;
+
+	list_del(&port->head);
+
+	phydev->n_ports--;
+}
+
 /**
  * phy_sfp_probe - probe for a SFP cage attached to this PHY device
  * @phydev: Pointer to phy_device
@@ -3202,6 +3250,119 @@ static int of_phy_leds(struct phy_device *phydev)
 	return 0;
 }
 
+static void phy_cleanup_ports(struct phy_device *phydev)
+{
+	struct phy_port *tmp, *port;
+
+	list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
+		phy_del_port(phydev, port);
+		phy_port_destroy(port);
+	}
+}
+
+static int phy_default_setup_single_port(struct phy_device *phydev)
+{
+	struct phy_port *port = phy_port_alloc();
+
+	if (!port)
+		return -ENOMEM;
+
+	port->parent_type = PHY_PORT_PHY;
+	port->phy = phydev;
+	linkmode_copy(port->supported, phydev->supported);
+
+	phy_add_port(phydev, port);
+
+	/* default medium is copper */
+	if (!port->mediums)
+		port->mediums |= BIT(ETHTOOL_LINK_MEDIUM_BASET);
+
+	return 0;
+}
+
+static int of_phy_ports(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *mdi;
+	struct phy_port *port;
+	int err;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (!node)
+		return 0;
+
+	mdi = of_get_child_by_name(node, "mdi");
+	if (!mdi)
+		return 0;
+
+	for_each_available_child_of_node_scoped(mdi, port_node) {
+		port = phy_of_parse_port(port_node);
+		if (IS_ERR(port)) {
+			err = PTR_ERR(port);
+			goto out_err;
+		}
+
+		port->parent_type = PHY_PORT_PHY;
+		port->phy = phydev;
+		err = phy_add_port(phydev, port);
+		if (err)
+			goto out_err;
+	}
+	of_node_put(mdi);
+
+	return 0;
+
+out_err:
+	phy_cleanup_ports(phydev);
+	of_node_put(mdi);
+	return err;
+}
+
+static int phy_setup_ports(struct phy_device *phydev)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(ports_supported);
+	struct phy_port *port;
+	int ret;
+
+	ret = of_phy_ports(phydev);
+	if (ret)
+		return ret;
+
+	if (phydev->n_ports < phydev->max_n_ports) {
+		ret = phy_default_setup_single_port(phydev);
+		if (ret)
+			goto out;
+	}
+
+	linkmode_zero(ports_supported);
+
+	/* Aggregate the supported modes, which are made-up of :
+	 *  - What the PHY itself supports
+	 *  - What the sum of all ports support
+	 */
+	list_for_each_entry(port, &phydev->ports, head)
+		if (port->active)
+			linkmode_or(ports_supported, ports_supported,
+				    port->supported);
+
+	if (!linkmode_empty(ports_supported))
+		linkmode_and(phydev->supported, phydev->supported,
+			     ports_supported);
+
+	/* For now, the phy->port field is set as the first active port's type */
+	list_for_each_entry(port, &phydev->ports, head)
+		if (port->active)
+			phydev->port = phy_port_get_type(port);
+
+	return 0;
+
+out:
+	phy_cleanup_ports(phydev);
+	return ret;
+}
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
@@ -3339,6 +3500,11 @@ static int phy_probe(struct device *dev)
 		phydev->is_gigabit_capable = 1;
 
 	of_set_phy_supported(phydev);
+
+	err = phy_setup_ports(phydev);
+	if (err)
+		goto out;
+
 	phy_advertise_supported(phydev);
 
 	/* Get PHY default EEE advertising modes and handle them as potentially
@@ -3414,6 +3580,8 @@ static int phy_remove(struct device *dev)
 
 	phydev->state = PHY_DOWN;
 
+	phy_cleanup_ports(phydev);
+
 	sfp_bus_del_upstream(phydev->sfp_bus);
 	phydev->sfp_bus = NULL;
 
diff --git a/drivers/net/phy/phy_port.c b/drivers/net/phy/phy_port.c
new file mode 100644
index 000000000000..c69ba0d9afba
--- /dev/null
+++ b/drivers/net/phy/phy_port.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Framework to drive Ethernet ports
+ *
+ * Copyright (c) 2024 Maxime Chevallier <maxime.chevallier@bootlin.com>
+ */
+
+#include <linux/linkmode.h>
+#include <linux/of.h>
+#include <linux/phy_port.h>
+
+/**
+ * phy_port_alloc() - Allocate a new phy_port
+ *
+ * Returns: a newly allocated struct phy_port, or NULL.
+ */
+struct phy_port *phy_port_alloc(void)
+{
+	struct phy_port *port;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return NULL;
+
+	linkmode_zero(port->supported);
+	INIT_LIST_HEAD(&port->head);
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(phy_port_alloc);
+
+/**
+ * phy_port_destroy() - Free a struct phy_port
+ * @port: The port to destroy
+ */
+void phy_port_destroy(struct phy_port *port)
+{
+	kfree(port);
+}
+EXPORT_SYMBOL_GPL(phy_port_destroy);
+
+static void ethtool_medium_get_supported(unsigned long *supported,
+					 enum ethtool_link_medium medium,
+					 int lanes)
+{
+	int i;
+
+	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
+		/* Special bits such as Autoneg, Pause, Asym_pause, etc. are
+		 * set and will be masked away by the port parent.
+		 */
+		if (link_mode_params[i].mediums == BIT(ETHTOOL_LINK_MEDIUM_NONE)) {
+			linkmode_set_bit(i, supported);
+			continue;
+		}
+
+		/* For most cases, min_lanes == lanes, except for 10/100BaseT that work
+		 * on 2 lanes but are compatible with 4 lanes mediums
+		 */
+		if (link_mode_params[i].mediums & BIT(medium) &&
+		    link_mode_params[i].lanes >= lanes &&
+		    link_mode_params[i].min_lanes <= lanes) {
+			linkmode_set_bit(i, supported);
+		}
+	}
+}
+
+static enum ethtool_link_medium ethtool_str_to_medium(const char *str)
+{
+	int i;
+
+	for (i = 0; i < __ETHTOOL_LINK_MEDIUM_LAST; i++)
+		if (!strcmp(phy_mediums(i), str))
+			return i;
+
+	return ETHTOOL_LINK_MEDIUM_NONE;
+}
+
+/**
+ * phy_of_parse_port() - Create a phy_port from a firmware representation
+ * @dn: device_node representation of the port, following the
+ *	ethernet-connector.yaml binding
+ *
+ * Returns: a newly allocated and initialized phy_port pointer, or an ERR_PTR.
+ */
+struct phy_port *phy_of_parse_port(struct device_node *dn)
+{
+	struct fwnode_handle *fwnode = of_fwnode_handle(dn);
+	enum ethtool_link_medium medium;
+	struct phy_port *port;
+	struct property *prop;
+	const char *med_str;
+	u32 lanes, mediums = 0;
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "lanes", &lanes);
+	if (ret)
+		lanes = 0;
+
+	ret = fwnode_property_read_string(fwnode, "media", &med_str);
+	if (ret)
+		return ERR_PTR(ret);
+
+	of_property_for_each_string(to_of_node(fwnode), "media", prop, med_str) {
+		medium = ethtool_str_to_medium(med_str);
+		if (medium == ETHTOOL_LINK_MEDIUM_NONE)
+			return ERR_PTR(-EINVAL);
+
+		mediums |= BIT(medium);
+	}
+
+	if (!mediums)
+		return ERR_PTR(-EINVAL);
+
+	port = phy_port_alloc();
+	if (!port)
+		return ERR_PTR(-ENOMEM);
+
+	port->lanes = lanes;
+	port->mediums = mediums;
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(phy_of_parse_port);
+
+/**
+ * phy_port_update_supported() - Setup the port->supported field
+ * @port: the port to update
+ *
+ * Once the port's medium list and number of lanes has been configured based
+ * on firmware, straps and vendor-specific properties, this function may be
+ * called to update the port's supported linkmodes list.
+ *
+ * Any mode that was manually set in the port's supported list remains set.
+ */
+void phy_port_update_supported(struct phy_port *port)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+	int i, lanes = 1;
+
+	/* If there's no lanes specified, we grab the default number of
+	 * lanes as the max of the default lanes for each medium
+	 */
+	if (!port->lanes)
+		for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
+			lanes = max_t(int, lanes, phy_medium_default_lanes(i));
+
+	for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST) {
+		linkmode_zero(supported);
+		ethtool_medium_get_supported(supported, i, port->lanes);
+		linkmode_or(port->supported, port->supported, supported);
+	}
+}
+EXPORT_SYMBOL_GPL(phy_port_update_supported);
+
+/**
+ * phy_port_get_type() - get the PORT_* attribut for that port.
+ * @port: The port we want the information from
+ *
+ * Returns: A PORT_XXX value.
+ */
+int phy_port_get_type(struct phy_port *port)
+{
+	if (port->mediums & ETHTOOL_LINK_MEDIUM_BASET)
+		return PORT_TP;
+
+	if (phy_port_is_fiber(port))
+		return PORT_FIBRE;
+
+	return PORT_OTHER;
+}
+EXPORT_SYMBOL_GPL(phy_port_get_type);
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c1d805d3e02f..0d3063af5905 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -226,6 +226,10 @@ extern const struct link_mode_info link_mode_params[];
 
 extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
 
+#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
+				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
+				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
+
 static inline const char *phy_mediums(enum ethtool_link_medium medium)
 {
 	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
@@ -234,6 +238,17 @@ static inline const char *phy_mediums(enum ethtool_link_medium medium)
 	return ethtool_link_medium_names[medium];
 }
 
+static inline int phy_medium_default_lanes(enum ethtool_link_medium medium)
+{
+	/* Let's consider that the default BaseT ethernet is BaseT4, i.e.
+	 * Gigabit Ethernet.
+	 */
+	if (medium == ETHTOOL_LINK_MEDIUM_BASET)
+		return 4;
+
+	return 1;
+}
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d62d292024bc..0180f4d4fd7d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -299,6 +299,7 @@ static inline long rgmii_clock(int speed)
 struct device;
 struct kernel_hwtstamp_config;
 struct phylink;
+struct phy_port;
 struct sfp_bus;
 struct sfp_upstream_ops;
 struct sk_buff;
@@ -590,6 +591,9 @@ struct macsec_ops;
  * @master_slave_state: Current master/slave configuration
  * @mii_ts: Pointer to time stamper callbacks
  * @psec: Pointer to Power Sourcing Equipment control struct
+ * @ports: List of PHY ports structures
+ * @n_ports: Number of ports currently attached to the PHY
+ * @max_n_ports: Max number of ports this PHY can expose
  * @lock:  Mutex for serialization access to PHY
  * @state_queue: Work queue for state machine
  * @link_down_events: Number of times link was lost
@@ -724,6 +728,10 @@ struct phy_device {
 	struct mii_timestamper *mii_ts;
 	struct pse_control *psec;
 
+	struct list_head ports;
+	int n_ports;
+	int max_n_ports;
+
 	u8 mdix;
 	u8 mdix_ctrl;
 
@@ -1242,6 +1250,27 @@ struct phy_driver {
 	 * Returns the time in jiffies until the next update event.
 	 */
 	unsigned int (*get_next_update_time)(struct phy_device *dev);
+
+	/**
+	 * @attach_port: Indicates to the PHY driver that a port is detected
+	 * @dev: PHY device to notify
+	 * @port: The port being added
+	 *
+	 * Called when a port that needs to be driven by the PHY is found. The
+	 * number of time this will be called depends on phydev->max_n_ports,
+	 * which the driver can change in .probe().
+	 *
+	 * The port that is being passed may or may not be initialized. If it is
+	 * already initialized, it is by the generic port representation from
+	 * devicetree, which superseeds any strapping or vendor-specific
+	 * properties.
+	 *
+	 * If the port isn't initialized, the port->mediums and port->lanes
+	 * fields must be set, possibly according to stapping information.
+	 *
+	 * Returns 0, or an error code.
+	 */
+	int (*attach_port)(struct phy_device *dev, struct phy_port *port);
 };
 #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
@@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
+
 void phy_ethtool_ksettings_get(struct phy_device *phydev,
 			       struct ethtool_link_ksettings *cmd);
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
new file mode 100644
index 000000000000..70aa75f93096
--- /dev/null
+++ b/include/linux/phy_port.h
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __PHY_PORT_H
+#define __PHY_PORT_H
+
+#include <linux/ethtool.h>
+#include <linux/types.h>
+#include <linux/phy.h>
+
+struct phy_port;
+
+/**
+ * enum phy_port_parent - The device this port is attached to
+ *
+ * @PHY_PORT_PHY: Indicates that the port is driven by a PHY device
+ */
+enum phy_port_parent {
+	PHY_PORT_PHY,
+};
+
+struct phy_port_ops {
+	/* Sometimes, the link state can be retrieved from physical,
+	 * out-of-band channels such as the LOS signal on SFP. These
+	 * callbacks allows notifying the port about state changes
+	 */
+	void (*link_up)(struct phy_port *port);
+	void (*link_down)(struct phy_port *port);
+
+	/* If the port acts as a Media Independent Interface (Serdes port),
+	 * configures the port with the relevant state and mode. When enable is
+	 * not set, interface should be ignored
+	 */
+	int (*configure_mii)(struct phy_port *port, bool enable, phy_interface_t interface);
+};
+
+/**
+ * struct phy_port - A representation of a network device physical interface
+ *
+ * @head: Used by the port's parent to list ports
+ * @parent_type: The type of device this port is directly connected to
+ * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
+ * @ops: Callback ops implemented by the port controller
+ * @lanes: The number of lanes (diff pairs) this port has, 0 if not applicable
+ * @mediums: Bitmask of the physical mediums this port provides access to
+ * @supported: The link modes this port can expose, if this port is MDI (not MII)
+ * @interfaces: The MII interfaces this port supports, if this port is MII
+ * @active: Indicates if the port is currently part of the active link.
+ * @is_serdes: Indicates if this port is Serialised MII (Media Independent
+ *	       Interface), or an MDI (Media Dependent Interface).
+ */
+struct phy_port {
+	struct list_head head;
+	enum phy_port_parent parent_type;
+	union {
+		struct phy_device *phy;
+	};
+
+	const struct phy_port_ops *ops;
+
+	int lanes;
+	unsigned long mediums;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+
+	unsigned int active:1;
+	unsigned int is_serdes:1;
+};
+
+struct phy_port *phy_port_alloc(void);
+void phy_port_destroy(struct phy_port *port);
+
+static inline struct phy_device *port_phydev(struct phy_port *port)
+{
+	return port->phy;
+}
+
+struct phy_port *phy_of_parse_port(struct device_node *dn);
+
+static inline bool phy_port_is_copper(struct phy_port *port)
+{
+	return port->mediums == BIT(ETHTOOL_LINK_MEDIUM_BASET);
+}
+
+static inline bool phy_port_is_fiber(struct phy_port *port)
+{
+	return !!(port->mediums & ETHTOOL_MEDIUM_FIBER_BITS);
+}
+
+void phy_port_update_supported(struct phy_port *port);
+
+int phy_port_get_type(struct phy_port *port);
+
+#endif
-- 
2.49.0

[PATCH net-next v6 04/14] net: phy: dp83822: Add support for phy_port representation

[Maxime Chevallier]

With the phy_port representation intrduced, we can use .attach_port to
populate the port information based on either the straps or the
ti,fiber-mode property. This allows simplifying the probe function and
allow users to override the strapping configuration.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/dp83822.c | 71 +++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 01255dada600..a0b4ebddd484 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/phy.h>
+#include <linux/phy_port.h>
 #include <linux/netdevice.h>
 #include <linux/bitfield.h>
 
@@ -814,17 +815,6 @@ static int dp83822_of_init(struct phy_device *phydev)
 	int i, ret;
 	u32 val;
 
-	/* Signal detection for the PHY is only enabled if the FX_EN and the
-	 * SD_EN pins are strapped. Signal detection can only enabled if FX_EN
-	 * is strapped otherwise signal detection is disabled for the PHY.
-	 */
-	if (dp83822->fx_enabled && dp83822->fx_sd_enable)
-		dp83822->fx_signal_det_low = device_property_present(dev,
-								     "ti,link-loss-low");
-	if (!dp83822->fx_enabled)
-		dp83822->fx_enabled = device_property_present(dev,
-							      "ti,fiber-mode");
-
 	if (!device_property_read_string(dev, "ti,gpio2-clk-out", &of_val)) {
 		if (strcmp(of_val, "mac-if") == 0) {
 			dp83822->gpio2_clk_out = DP83822_CLK_SRC_MAC_IF;
@@ -953,6 +943,48 @@ static int dp83822_read_straps(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83822_attach_port(struct phy_device *phydev, struct phy_port *port)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	int ret;
+
+	if (port->mediums) {
+		if (phy_port_is_fiber(port))
+			dp83822->fx_enabled = true;
+	} else {
+		ret = dp83822_read_straps(phydev);
+		if (ret)
+			return ret;
+
+#ifdef CONFIG_OF_MDIO
+		if (dp83822->fx_enabled && dp83822->fx_sd_enable)
+			dp83822->fx_signal_det_low =
+				device_property_present(&phydev->mdio.dev,
+							"ti,link-loss-low");
+
+		/* ti,fiber-mode is still used for backwards compatibility, but
+		 * has been replaced with the mdi node definition, see
+		 * ethernet-port.yaml
+		 */
+		if (!dp83822->fx_enabled)
+			dp83822->fx_enabled =
+				device_property_present(&phydev->mdio.dev,
+							"ti,fiber-mode");
+#endif
+
+		if (dp83822->fx_enabled) {
+			port->lanes = 1;
+			port->mediums = BIT(ETHTOOL_LINK_MEDIUM_BASEF);
+		} else {
+			/* This PHY can only to 100BaseTX max, so on 2 lanes */
+			port->lanes = 2;
+			port->mediums = BIT(ETHTOOL_LINK_MEDIUM_BASET);
+		}
+	}
+
+	return 0;
+}
+
 static int dp8382x_probe(struct phy_device *phydev)
 {
 	struct dp83822_private *dp83822;
@@ -971,27 +1003,13 @@ static int dp8382x_probe(struct phy_device *phydev)
 
 static int dp83822_probe(struct phy_device *phydev)
 {
-	struct dp83822_private *dp83822;
 	int ret;
 
 	ret = dp8382x_probe(phydev);
 	if (ret)
 		return ret;
 
-	dp83822 = phydev->priv;
-
-	ret = dp83822_read_straps(phydev);
-	if (ret)
-		return ret;
-
-	ret = dp83822_of_init(phydev);
-	if (ret)
-		return ret;
-
-	if (dp83822->fx_enabled)
-		phydev->port = PORT_FIBRE;
-
-	return 0;
+	return dp83822_of_init(phydev);
 }
 
 static int dp83826_probe(struct phy_device *phydev)
@@ -1175,6 +1193,7 @@ static int dp83822_led_hw_control_get(struct phy_device *phydev, u8 index,
 		.led_hw_is_supported = dp83822_led_hw_is_supported,	\
 		.led_hw_control_set = dp83822_led_hw_control_set,	\
 		.led_hw_control_get = dp83822_led_hw_control_get,	\
+		.attach_port = dp83822_attach_port		\
 	}
 
 #define DP83825_PHY_DRIVER(_id, _name)				\
-- 
2.49.0

[PATCH net-next v6 05/14] net: phy: Create a phy_port for PHY-driven SFPs

[Maxime Chevallier]

Some PHY devices may be used as media-converters to drive SFP ports (for
example, to allow using SFP when the SoC can only output RGMII). This is
already supported to some extend by allowing PHY drivers to registers
themselves as being SFP upstream.

However, the logic to drive the SFP can actually be split to a per-port
control logic, allowing support for multi-port PHYs, or PHYs that can
either drive SFPs or Copper.

To that extent, create a phy_port when registering an SFP bus onto a
PHY. This port is considered a "serdes" port, in that it can feed data
to anther entity on the link. The PHY driver needs to specify the
various PHY_INTERFACE_MODE_XXX that this port supports.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
 drivers/net/phy/phy_port.c   | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9e0cc6a54497..aaf0eccbefba 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1490,6 +1490,23 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
 	phydev->n_ports--;
 }
 
+static int phy_setup_sfp_port(struct phy_device *phydev)
+{
+	struct phy_port *port = phy_port_alloc();
+
+	if (!port)
+		return -ENOMEM;
+
+	port->parent_type = PHY_PORT_PHY;
+	port->phy = phydev;
+
+	port->is_serdes = true;
+
+	phy_add_port(phydev, port);
+
+	return 0;
+}
+
 /**
  * phy_sfp_probe - probe for a SFP cage attached to this PHY device
  * @phydev: Pointer to phy_device
@@ -1511,6 +1528,10 @@ int phy_sfp_probe(struct phy_device *phydev,
 		ret = sfp_bus_add_upstream(bus, phydev, ops);
 		sfp_bus_put(bus);
 	}
+
+	if (phydev->sfp_bus)
+		ret = phy_setup_sfp_port(phydev);
+
 	return ret;
 }
 EXPORT_SYMBOL(phy_sfp_probe);
diff --git a/drivers/net/phy/phy_port.c b/drivers/net/phy/phy_port.c
index c69ba0d9afba..15c2c00f573d 100644
--- a/drivers/net/phy/phy_port.c
+++ b/drivers/net/phy/phy_port.c
@@ -8,6 +8,8 @@
 #include <linux/of.h>
 #include <linux/phy_port.h>
 
+#include "phy-caps.h"
+
 /**
  * phy_port_alloc() - Allocate a new phy_port
  *
@@ -149,6 +151,21 @@ void phy_port_update_supported(struct phy_port *port)
 		ethtool_medium_get_supported(supported, i, port->lanes);
 		linkmode_or(port->supported, port->supported, supported);
 	}
+
+	/* Serdes ports supported may through SFP may not have any medium set,
+	 * as they will output PHY_INTERFACE_MODE_XXX modes. In that case, derive
+	 * the supported list based on these interfaces
+	 */
+	if (port->is_serdes && linkmode_empty(supported)) {
+		unsigned long interface, link_caps = 0;
+
+		/* Get each interface's caps */
+		for_each_set_bit(interface, port->interfaces,
+				 PHY_INTERFACE_MODE_MAX)
+			link_caps |= phy_caps_from_interface(interface);
+
+		phy_caps_linkmodes(link_caps, port->supported);
+	}
 }
 EXPORT_SYMBOL_GPL(phy_port_update_supported);
 
-- 
2.49.0

[PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Maxime Chevallier]

There are currently 4 PHY drivers that can drive downstream SFPs:
marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
logic is boilerplate, either calling into generic phylib helpers (for
SFP PHY attach, bus attach, etc.) or performing the same tasks with a
bit of validation :
 - Getting the module's expected interface mode
 - Making sure the PHY supports it
 - Optionnaly perform some configuration to make sure the PHY outputs
   the right mode

This can be made more generic by leveraging the phy_port, and its
configure_mii() callback which allows setting a port's interfaces when
the port is a serdes.

Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
bus itself, but an SFP phandle is found in devicetree/firmware, then the
generic PHY SFP support will be used, relying on port ops.

PHY driver need to :
 - Register a .attach_port() callback
 - When a serdes port is registered to the PHY, drivers must set
   port->interfaces to the set of PHY_INTERFACE_MODE the port can output
 - If the port has limitations regarding speed, duplex and aneg, the
   port can also fine-tune the final linkmodes that can be supported
 - The port may register a set of ops, including .configure_mii(), that
   will be called at module_insert time to adjust the interface based on
   the module detected.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |   2 +
 2 files changed, 109 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index aaf0eccbefba..aca3a47cbb66 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
 }
 EXPORT_SYMBOL(phy_sfp_detach);
 
+static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id *id)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	phy_interface_t iface;
+
+	linkmode_zero(sfp_support);
+
+	if (!port)
+		return -EINVAL;
+
+	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
+
+	if (phydev->n_ports == 1)
+		phydev->port = sfp_parse_port(phydev->sfp_bus, id, sfp_support);
+
+	linkmode_and(sfp_support, port->supported, sfp_support);
+
+	if (linkmode_empty(sfp_support)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+
+	/* Check that this interface is supported */
+	if (!test_bit(iface, port->interfaces)) {
+		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	if (port->ops && port->ops->configure_mii)
+		return port->ops->configure_mii(port, true, iface);
+
+	return 0;
+}
+
+static void phy_sfp_module_remove(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->configure_mii)
+		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
+
+	if (phydev->n_ports == 1)
+		phydev->port = PORT_NONE;
+}
+
+static void phy_sfp_link_up(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->link_up)
+		port->ops->link_up(port);
+}
+
+static void phy_sfp_link_down(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+	struct phy_port *port = phy_get_sfp_port(phydev);
+
+	if (port && port->ops && port->ops->link_down)
+		port->ops->link_down(port);
+}
+
+static const struct sfp_upstream_ops sfp_phydev_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = phy_sfp_module_insert,
+	.module_remove = phy_sfp_module_remove,
+	.link_up = phy_sfp_link_up,
+	.link_down = phy_sfp_link_down,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
+};
+
 static int phy_add_port(struct phy_device *phydev, struct phy_port *port)
 {
 	int ret = 0;
@@ -3351,6 +3432,13 @@ static int phy_setup_ports(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Use generic SFP probing only if the driver didn't do so already */
+	if (!phydev->sfp_bus) {
+		ret = phy_sfp_probe(phydev, &sfp_phydev_ops);
+		if (ret)
+			goto out;
+	}
+
 	if (phydev->n_ports < phydev->max_n_ports) {
 		ret = phy_default_setup_single_port(phydev);
 		if (ret)
@@ -3384,6 +3472,25 @@ static int phy_setup_ports(struct phy_device *phydev)
 	return ret;
 }
 
+/**
+ * phy_get_sfp_port() - Returns the first valid SFP port of a PHY
+ * @phydev: pointer to the PHY device to get the SFP port from
+ *
+ * Returns: The first active SFP (serdes) port of a PHY device, NULL if none
+ * exist.
+ */
+struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
+{
+	struct phy_port *port;
+
+	list_for_each_entry(port, &phydev->ports, head)
+		if (port->active && port->is_serdes)
+			return port;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(phy_get_sfp_port);
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0180f4d4fd7d..aef13fab8882 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2091,6 +2091,8 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config,
 		       struct netlink_ext_ack *extack);
 
+struct phy_port *phy_get_sfp_port(struct phy_device *phydev);
+
 extern const struct bus_type mdio_bus_type;
 extern const struct class mdio_bus_class;
 
-- 
2.49.0

[PATCH net-next v6 07/14] net: phy: marvell-88x2222: Support SFP through phy_port interface

[Maxime Chevallier]

The 88x2222 PHY from Marvell only supports serialised modes as its
line-facing interfaces. Convert that driver to the generic phylib SFP
handling.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell-88x2222.c | 98 +++++++++++++------------------
 1 file changed, 41 insertions(+), 57 deletions(-)

diff --git a/drivers/net/phy/marvell-88x2222.c b/drivers/net/phy/marvell-88x2222.c
index fad2f54c1eac..4812c9acc32b 100644
--- a/drivers/net/phy/marvell-88x2222.c
+++ b/drivers/net/phy/marvell-88x2222.c
@@ -13,7 +13,7 @@
 #include <linux/mdio.h>
 #include <linux/marvell_phy.h>
 #include <linux/of.h>
-#include <linux/sfp.h>
+#include <linux/phy_port.h>
 #include <linux/netdevice.h>
 
 /* Port PCS Configuration */
@@ -473,90 +473,73 @@ static int mv2222_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int mv2222_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int mv2222_configure_serdes(struct phy_port *port, bool enable,
+				   phy_interface_t interface)
 {
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	struct phy_device *phydev = upstream;
-	phy_interface_t sfp_interface;
+	struct phy_device *phydev = port_phydev(port);
 	struct mv2222_data *priv;
-	struct device *dev;
-	int ret;
-
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_supported) = { 0, };
+	int ret = 0;
 
 	priv = phydev->priv;
-	dev = &phydev->mdio.dev;
-
-	sfp_parse_support(phydev->sfp_bus, id, sfp_supported, interfaces);
-	phydev->port = sfp_parse_port(phydev->sfp_bus, id, sfp_supported);
-	sfp_interface = sfp_select_interface(phydev->sfp_bus, sfp_supported);
-
-	dev_info(dev, "%s SFP module inserted\n", phy_modes(sfp_interface));
+	priv->line_interface = interface;
 
-	if (sfp_interface != PHY_INTERFACE_MODE_10GBASER &&
-	    sfp_interface != PHY_INTERFACE_MODE_1000BASEX &&
-	    sfp_interface != PHY_INTERFACE_MODE_SGMII) {
-		dev_err(dev, "Incompatible SFP module inserted\n");
+	if (enable) {
+		linkmode_and(priv->supported, phydev->supported, port->supported);
 
-		return -EINVAL;
-	}
-
-	priv->line_interface = sfp_interface;
-	linkmode_and(priv->supported, phydev->supported, sfp_supported);
+		ret = mv2222_config_line(phydev);
+		if (ret < 0)
+			return ret;
 
-	ret = mv2222_config_line(phydev);
-	if (ret < 0)
-		return ret;
+		if (mutex_trylock(&phydev->lock)) {
+			ret = mv2222_config_aneg(phydev);
+			mutex_unlock(&phydev->lock);
+		}
 
-	if (mutex_trylock(&phydev->lock)) {
-		ret = mv2222_config_aneg(phydev);
-		mutex_unlock(&phydev->lock);
+	} else {
+		linkmode_zero(priv->supported);
 	}
 
 	return ret;
 }
 
-static void mv2222_sfp_remove(void *upstream)
+static void mv2222_port_link_up(struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
-	struct mv2222_data *priv;
-
-	priv = phydev->priv;
-
-	priv->line_interface = PHY_INTERFACE_MODE_NA;
-	linkmode_zero(priv->supported);
-	phydev->port = PORT_NONE;
-}
-
-static void mv2222_sfp_link_up(void *upstream)
-{
-	struct phy_device *phydev = upstream;
+	struct phy_device *phydev = port_phydev(port);
 	struct mv2222_data *priv;
 
 	priv = phydev->priv;
 	priv->sfp_link = true;
 }
 
-static void mv2222_sfp_link_down(void *upstream)
+static void mv2222_port_link_down(struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
+	struct phy_device *phydev = port_phydev(port);
 	struct mv2222_data *priv;
 
 	priv = phydev->priv;
 	priv->sfp_link = false;
 }
 
-static const struct sfp_upstream_ops sfp_phy_ops = {
-	.module_insert = mv2222_sfp_insert,
-	.module_remove = mv2222_sfp_remove,
-	.link_up = mv2222_sfp_link_up,
-	.link_down = mv2222_sfp_link_down,
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
+static const struct phy_port_ops mv2222_port_ops = {
+	.link_up = mv2222_port_link_up,
+	.link_down = mv2222_port_link_down,
+	.configure_mii = mv2222_configure_serdes,
 };
 
+static int mv2222_attach_port(struct phy_device *phydev, struct phy_port *port)
+{
+	if (!port->is_serdes)
+		return 0;
+
+	port->ops = &mv2222_port_ops;
+
+	__set_bit(PHY_INTERFACE_MODE_10GBASER, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_SGMII, port->interfaces);
+
+	return 0;
+}
+
 static int mv2222_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -592,7 +575,7 @@ static int mv2222_probe(struct phy_device *phydev)
 	priv->line_interface = PHY_INTERFACE_MODE_NA;
 	phydev->priv = priv;
 
-	return phy_sfp_probe(phydev, &sfp_phy_ops);
+	return 0;
 }
 
 static struct phy_driver mv2222_drivers[] = {
@@ -609,6 +592,7 @@ static struct phy_driver mv2222_drivers[] = {
 		.suspend = mv2222_suspend,
 		.resume = mv2222_resume,
 		.read_status = mv2222_read_status,
+		.attach_port = mv2222_attach_port,
 	},
 };
 module_phy_driver(mv2222_drivers);
-- 
2.49.0

[PATCH net-next v6 08/14] net: phy: marvell: Support SFP through phy_port interface

[Maxime Chevallier]

Convert the Marvell driver (especially the 88e1512 driver) to use the
phy_port interface to handle SFPs. This means registering a
.attach_port() handler to detect when a serdes line interface is used
(most likely, and SFP module).

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell.c | 100 +++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 61 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 623292948fa7..54b4f7753b2a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -29,10 +29,10 @@
 #include <linux/ethtool.h>
 #include <linux/ethtool_netlink.h>
 #include <linux/phy.h>
+#include <linux/phy_port.h>
 #include <linux/marvell_phy.h>
 #include <linux/bitfield.h>
 #include <linux/of.h>
-#include <linux/sfp.h>
 
 #include <linux/io.h>
 #include <asm/irq.h>
@@ -3561,42 +3561,38 @@ static int marvell_probe(struct phy_device *phydev)
 	return marvell_hwmon_probe(phydev);
 }
 
-static int m88e1510_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int mv88e1510_port_configure_serdes(struct phy_port *port, bool enable,
+					   phy_interface_t interface)
 {
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	struct phy_device *phydev = upstream;
-	phy_interface_t interface;
+	struct phy_device *phydev = port_phydev(port);
 	struct device *dev;
 	int oldpage;
 	int ret = 0;
 	u16 mode;
 
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
-
 	dev = &phydev->mdio.dev;
 
-	sfp_parse_support(phydev->sfp_bus, id, supported, interfaces);
-	interface = sfp_select_interface(phydev->sfp_bus, supported);
-
-	dev_info(dev, "%s SFP module inserted\n", phy_modes(interface));
+	if (enable) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+			mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_1000X;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_1000BASEX:
-		mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_1000X;
+			break;
+		case PHY_INTERFACE_MODE_100BASEX:
+			mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_100FX;
 
-		break;
-	case PHY_INTERFACE_MODE_100BASEX:
-		mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_100FX;
-
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_SGMII;
+			break;
+		case PHY_INTERFACE_MODE_SGMII:
+			mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII_SGMII;
 
-		break;
-	default:
-		dev_err(dev, "Incompatible SFP module inserted\n");
+			break;
+		default:
+			dev_err(dev, "Incompatible SFP module inserted\n");
 
-		return -EINVAL;
+			return -EINVAL;
+		}
+	} else {
+		mode = MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII;
 	}
 
 	oldpage = phy_select_page(phydev, MII_MARVELL_MODE_PAGE);
@@ -3613,49 +3609,30 @@ static int m88e1510_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
 
 error:
 	return phy_restore_page(phydev, oldpage, ret);
-}
-
-static void m88e1510_sfp_remove(void *upstream)
-{
-	struct phy_device *phydev = upstream;
-	int oldpage;
-	int ret = 0;
-
-	oldpage = phy_select_page(phydev, MII_MARVELL_MODE_PAGE);
-	if (oldpage < 0)
-		goto error;
 
-	ret = __phy_modify(phydev, MII_88E1510_GEN_CTRL_REG_1,
-			   MII_88E1510_GEN_CTRL_REG_1_MODE_MASK,
-			   MII_88E1510_GEN_CTRL_REG_1_MODE_RGMII);
-	if (ret < 0)
-		goto error;
-
-	ret = __phy_set_bits(phydev, MII_88E1510_GEN_CTRL_REG_1,
-			     MII_88E1510_GEN_CTRL_REG_1_RESET);
-
-error:
-	phy_restore_page(phydev, oldpage, ret);
+	return 0;
 }
 
-static const struct sfp_upstream_ops m88e1510_sfp_ops = {
-	.module_insert = m88e1510_sfp_insert,
-	.module_remove = m88e1510_sfp_remove,
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
+static const struct phy_port_ops mv88e1510_serdes_port_ops = {
+	.configure_mii = mv88e1510_port_configure_serdes,
 };
 
-static int m88e1510_probe(struct phy_device *phydev)
+static int m88e1510_attach_port(struct phy_device *phy_device,
+				struct phy_port *port)
 {
-	int err;
+	/* For classic Copper operation, we don't have any port-specific
+	 * control to do.
+	 */
+	if (!port->is_serdes)
+		return 0;
 
-	err = marvell_probe(phydev);
-	if (err)
-		return err;
+	port->ops = &mv88e1510_serdes_port_ops;
+
+	__set_bit(PHY_INTERFACE_MODE_SGMII, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_100BASEX, port->interfaces);
 
-	return phy_sfp_probe(phydev, &m88e1510_sfp_ops);
+	return 0;
 }
 
 static struct phy_driver marvell_drivers[] = {
@@ -3915,7 +3892,7 @@ static struct phy_driver marvell_drivers[] = {
 		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
 		.features = PHY_GBIT_FIBRE_FEATURES,
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e1510_probe,
+		.probe = marvell_probe,
 		.config_init = m88e1510_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
@@ -3941,6 +3918,7 @@ static struct phy_driver marvell_drivers[] = {
 		.led_hw_is_supported = m88e1318_led_hw_is_supported,
 		.led_hw_control_set = m88e1318_led_hw_control_set,
 		.led_hw_control_get = m88e1318_led_hw_control_get,
+		.attach_port = m88e1510_attach_port,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
-- 
2.49.0

[PATCH net-next v6 09/14] net: phy: marvell10g: Support SFP through phy_port

[Maxime Chevallier]

Convert the Marvell10G driver to use the generic SFP handling, through a
dedicated .attach_port() handler to populate the port's supported
interfaces. As there's no logic to setup the interface for now (as only
10GBaseR is supported for serdes line interfaces), no extra logic is
required.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 37 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 5354c8895163..034887c6b443 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -28,7 +28,7 @@
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
-#include <linux/sfp.h>
+#include <linux/phy_port.h>
 #include <linux/netdevice.h>
 
 #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
@@ -463,36 +463,23 @@ static int mv3310_set_edpd(struct phy_device *phydev, u16 edpd)
 	return err;
 }
 
-static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int mv3310_attach_port(struct phy_device *phydev, struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	phy_interface_t iface;
+	/* Nothing special to do to handle non-serdes ports */
+	if (!port->is_serdes)
+		return 0;
 
-	sfp_parse_support(phydev->sfp_bus, id, support, interfaces);
-	iface = sfp_select_interface(phydev->sfp_bus, support);
+	__set_bit(PHY_INTERFACE_MODE_10GBASER, port->interfaces);
 
-	if (iface != PHY_INTERFACE_MODE_10GBASER) {
-		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
-		return -EINVAL;
-	}
 	return 0;
 }
 
-static const struct sfp_upstream_ops mv3310_sfp_ops = {
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
-	.module_insert = mv3310_sfp_insert,
-};
-
 static int mv3310_probe(struct phy_device *phydev)
 {
 	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
 	struct mv3310_priv *priv;
 	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
 	int ret;
 
 	if (!phydev->is_c45 ||
@@ -543,9 +530,13 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+
 	chip->init_supported_interfaces(priv->supported_interfaces);
 
-	return phy_sfp_probe(phydev, &mv3310_sfp_ops);
+	phydev->max_n_ports = 2;
+
+	return 0;
 }
 
 static void mv3310_remove(struct phy_device *phydev)
@@ -1402,6 +1393,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_loopback	= genphy_c45_loopback,
 		.get_wol	= mv3110_get_wol,
 		.set_wol	= mv3110_set_wol,
+		.attach_port	= mv3310_attach_port,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -1421,6 +1413,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_tunable	= mv3310_set_tunable,
 		.remove		= mv3310_remove,
 		.set_loopback	= genphy_c45_loopback,
+		.attach_port	= mv3310_attach_port,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
@@ -1441,6 +1434,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_loopback	= genphy_c45_loopback,
 		.get_wol	= mv3110_get_wol,
 		.set_wol	= mv3110_set_wol,
+		.attach_port	= mv3310_attach_port,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
@@ -1459,6 +1453,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_tunable	= mv3310_set_tunable,
 		.remove		= mv3310_remove,
 		.set_loopback	= genphy_c45_loopback,
+		.attach_port	= mv3310_attach_port,
 	},
 };
 
-- 
2.49.0

[PATCH net-next v6 10/14] net: phy: at803x: Support SFP through phy_port interface

[Maxime Chevallier]

Convert the at803x driver to use the generic phylib SFP handling, via a
dedicated .attach_port() callback, populating the supported interfaces.

As these devices are limited to 1000BaseX, a workaround is used to also
support, in a very limited way, copper modules. This is done by
supporting SGMII but limiting it to 1G full duplex (in which case it's
somwhat compatible with 1000BaseX).

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/qcom/at803x.c | 64 ++++++++++-------------------------
 1 file changed, 17 insertions(+), 47 deletions(-)

diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 26350b962890..87145c4b4cbd 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -19,7 +19,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
-#include <linux/sfp.h>
+#include <linux/phy_port.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
 #include "qcom.h"
@@ -722,58 +722,28 @@ static int at8031_register_regulators(struct phy_device *phydev)
 	return 0;
 }
 
-static int at8031_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int at8031_attach_port(struct phy_device *phydev, struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_support);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
-	phy_interface_t iface;
-
-	linkmode_zero(phy_support);
-	phylink_set(phy_support, 1000baseX_Full);
-	phylink_set(phy_support, 1000baseT_Full);
-	phylink_set(phy_support, Autoneg);
-	phylink_set(phy_support, Pause);
-	phylink_set(phy_support, Asym_Pause);
-
-	linkmode_zero(sfp_support);
-	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
-	/* Some modules support 10G modes as well as others we support.
-	 * Mask out non-supported modes so the correct interface is picked.
-	 */
-	linkmode_and(sfp_support, phy_support, sfp_support);
-
-	if (linkmode_empty(sfp_support)) {
-		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted\n");
-		return -EINVAL;
-	}
+	if (!port->is_serdes)
+		return 0;
 
-	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
+	linkmode_zero(port->supported);
+	phylink_set(port->supported, 1000baseX_Full);
+	phylink_set(port->supported, 1000baseT_Full);
+	phylink_set(port->supported, Autoneg);
+	phylink_set(port->supported, Pause);
+	phylink_set(port->supported, Asym_Pause);
 
-	/* Only 1000Base-X is supported by AR8031/8033 as the downstream SerDes
-	 * interface for use with SFP modules.
-	 * However, some copper modules detected as having a preferred SGMII
-	 * interface do default to and function in 1000Base-X mode, so just
-	 * print a warning and allow such modules, as they may have some chance
-	 * of working.
+	/* This device doesn't really support SGMII. However, do our best
+	 * to be compatible with copper modules (that usually require SGMII),
+	 * in a degraded mode as we only allow 1000BaseT Full
 	 */
-	if (iface == PHY_INTERFACE_MODE_SGMII)
-		dev_warn(&phydev->mdio.dev, "module may not function if 1000Base-X not supported\n");
-	else if (iface != PHY_INTERFACE_MODE_1000BASEX)
-		return -EINVAL;
+	__set_bit(PHY_INTERFACE_MODE_SGMII, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
 
 	return 0;
 }
 
-static const struct sfp_upstream_ops at8031_sfp_ops = {
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.module_insert = at8031_sfp_insert,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
-};
-
 static int at8031_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -794,8 +764,7 @@ static int at8031_parse_dt(struct phy_device *phydev)
 		return ret;
 	}
 
-	/* Only AR8031/8033 support 1000Base-X for SFP modules */
-	return phy_sfp_probe(phydev, &at8031_sfp_ops);
+	return 0;
 }
 
 static int at8031_probe(struct phy_device *phydev)
@@ -1047,6 +1016,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at8031_cable_test_start,
 	.cable_test_get_status	= at8031_cable_test_get_status,
+	.attach_port		= at8031_attach_port,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
-- 
2.49.0

[PATCH net-next v6 11/14] net: phy: qca807x: Support SFP through phy_port interface

[Maxime Chevallier]

QCA8072/8075 may be used as combo-port PHYs, with Serdes (100/1000BaseX)
 and Copper interfaces. The PHY has the ability to read the configuration
it's in.  If the configuration indicates the PHY is in combo mode, allow
registering up to 2 ports.

Register a dedicated set of port ops to handle the serdes port, and rely
on generic phylib SFP support for the SFP handling.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/qcom/qca807x.c | 75 +++++++++++++++-------------------
 1 file changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
index 1af6b5ead74b..2b1dc851214f 100644
--- a/drivers/net/phy/qcom/qca807x.c
+++ b/drivers/net/phy/qcom/qca807x.c
@@ -13,7 +13,7 @@
 #include <linux/phy.h>
 #include <linux/bitfield.h>
 #include <linux/gpio/driver.h>
-#include <linux/sfp.h>
+#include <linux/phy_port.h>
 
 #include "../phylib.h"
 #include "qcom.h"
@@ -641,67 +641,55 @@ static int qca807x_phy_package_config_init_once(struct phy_device *phydev)
 	return ret;
 }
 
-static int qca807x_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+static int qca807x_configure_serdes(struct phy_port *port, bool enable,
+				    phy_interface_t interface)
 {
-	struct phy_device *phydev = upstream;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
-	phy_interface_t iface;
+	struct phy_device *phydev = port_phydev(port);
 	int ret;
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
 
-	sfp_parse_support(phydev->sfp_bus, id, support, interfaces);
-	iface = sfp_select_interface(phydev->sfp_bus, support);
+	if (!phydev)
+		return -ENODEV;
 
-	dev_info(&phydev->mdio.dev, "%s SFP module inserted\n", phy_modes(iface));
-
-	switch (iface) {
-	case PHY_INTERFACE_MODE_1000BASEX:
-	case PHY_INTERFACE_MODE_100BASEX:
+	if (enable) {
 		/* Set PHY mode to PSGMII combo (1/4 copper + combo ports) mode */
 		ret = phy_modify(phydev,
 				 QCA807X_CHIP_CONFIGURATION,
 				 QCA807X_CHIP_CONFIGURATION_MODE_CFG_MASK,
 				 QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_FIBER);
+		if (ret)
+			return ret;
 		/* Enable fiber mode autodection (1000Base-X or 100Base-FX) */
 		ret = phy_set_bits_mmd(phydev,
 				       MDIO_MMD_AN,
 				       QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION,
 				       QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION_EN);
-		/* Select fiber page */
-		ret = phy_clear_bits(phydev,
-				     QCA807X_CHIP_CONFIGURATION,
-				     QCA807X_BT_BX_REG_SEL);
-
-		phydev->port = PORT_FIBRE;
-		break;
-	default:
-		dev_err(&phydev->mdio.dev, "Incompatible SFP module inserted\n");
-		return -EINVAL;
+		if (ret)
+			return ret;
 	}
 
-	return ret;
+	phydev->port = enable ? PORT_FIBRE : PORT_TP;
+
+	return phy_modify(phydev, QCA807X_CHIP_CONFIGURATION,
+			  QCA807X_BT_BX_REG_SEL,
+			  enable ? 0 : QCA807X_BT_BX_REG_SEL);
 }
 
-static void qca807x_sfp_remove(void *upstream)
+static const struct phy_port_ops qca807x_serdes_port_ops = {
+	.configure_mii = qca807x_configure_serdes,
+};
+
+static int qca807x_attach_port(struct phy_device *phydev, struct phy_port *port)
 {
-	struct phy_device *phydev = upstream;
+	if (!port->is_serdes)
+		return 0;
 
-	/* Select copper page */
-	phy_set_bits(phydev,
-		     QCA807X_CHIP_CONFIGURATION,
-		     QCA807X_BT_BX_REG_SEL);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, port->interfaces);
+	__set_bit(PHY_INTERFACE_MODE_100BASEX, port->interfaces);
 
-	phydev->port = PORT_TP;
-}
+	port->ops = &qca807x_serdes_port_ops;
 
-static const struct sfp_upstream_ops qca807x_sfp_ops = {
-	.attach = phy_sfp_attach,
-	.detach = phy_sfp_detach,
-	.module_insert = qca807x_sfp_insert,
-	.module_remove = qca807x_sfp_remove,
-	.connect_phy = phy_sfp_connect_phy,
-	.disconnect_phy = phy_sfp_disconnect_phy,
-};
+	return 0;
+}
 
 static int qca807x_probe(struct phy_device *phydev)
 {
@@ -743,9 +731,8 @@ static int qca807x_probe(struct phy_device *phydev)
 
 	/* Attach SFP bus on combo port*/
 	if (phy_read(phydev, QCA807X_CHIP_CONFIGURATION)) {
-		ret = phy_sfp_probe(phydev, &qca807x_sfp_ops);
-		if (ret)
-			return ret;
+		phydev->max_n_ports = 2;
+
 		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->advertising);
 	}
@@ -799,6 +786,7 @@ static struct phy_driver qca807x_drivers[] = {
 		.suspend	= genphy_suspend,
 		.cable_test_start	= qca807x_cable_test_start,
 		.cable_test_get_status	= qca808x_cable_test_get_status,
+		.attach_port	= qca807x_attach_port,
 	},
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_QCA8075),
@@ -822,6 +810,7 @@ static struct phy_driver qca807x_drivers[] = {
 		.led_hw_is_supported = qca807x_led_hw_is_supported,
 		.led_hw_control_set = qca807x_led_hw_control_set,
 		.led_hw_control_get = qca807x_led_hw_control_get,
+		.attach_port	= qca807x_attach_port,
 	},
 };
 module_phy_driver(qca807x_drivers);
-- 
2.49.0

[PATCH net-next v6 12/14] net: phy: Only rely on phy_port for PHY-driven SFP

[Maxime Chevallier]

Now that all PHY drivers that support downstream SFP have been converted
to phy_port serdes handling, we can make the generic PHY SFP handling
mandatory, thus making all phylib sfp helpers static.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 28 +++++++++-------------------
 include/linux/phy.h          |  6 ------
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index aca3a47cbb66..7f319526a7fe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1384,7 +1384,7 @@ static DEVICE_ATTR_RO(phy_standalone);
  *
  * Return: 0 on success, otherwise a negative error code.
  */
-int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
+static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phy_device *phydev = upstream;
 	struct net_device *dev = phydev->attached_dev;
@@ -1394,7 +1394,6 @@ int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
 
 	return 0;
 }
-EXPORT_SYMBOL(phy_sfp_connect_phy);
 
 /**
  * phy_sfp_disconnect_phy - Disconnect the SFP module's PHY from the upstream PHY
@@ -1406,7 +1405,7 @@ EXPORT_SYMBOL(phy_sfp_connect_phy);
  * will be destroyed, re-inserting the same module will add a new phy with a
  * new index.
  */
-void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
+static void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phy_device *phydev = upstream;
 	struct net_device *dev = phydev->attached_dev;
@@ -1414,7 +1413,6 @@ void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
 	if (dev)
 		phy_link_topo_del_phy(dev, phy);
 }
-EXPORT_SYMBOL(phy_sfp_disconnect_phy);
 
 /**
  * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
@@ -1423,7 +1421,7 @@ EXPORT_SYMBOL(phy_sfp_disconnect_phy);
  *
  * This is used to fill in the sfp_upstream_ops .attach member.
  */
-void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
+static void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
 {
 	struct phy_device *phydev = upstream;
 
@@ -1431,7 +1429,6 @@ void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
 		phydev->attached_dev->sfp_bus = bus;
 	phydev->sfp_bus_attached = true;
 }
-EXPORT_SYMBOL(phy_sfp_attach);
 
 /**
  * phy_sfp_detach - detach the SFP bus from the PHY upstream network device
@@ -1440,7 +1437,7 @@ EXPORT_SYMBOL(phy_sfp_attach);
  *
  * This is used to fill in the sfp_upstream_ops .detach member.
  */
-void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
+static void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
 {
 	struct phy_device *phydev = upstream;
 
@@ -1448,7 +1445,6 @@ void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
 		phydev->attached_dev->sfp_bus = NULL;
 	phydev->sfp_bus_attached = false;
 }
-EXPORT_SYMBOL(phy_sfp_detach);
 
 static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id *id)
 {
@@ -1591,10 +1587,8 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
 /**
  * phy_sfp_probe - probe for a SFP cage attached to this PHY device
  * @phydev: Pointer to phy_device
- * @ops: SFP's upstream operations
  */
-int phy_sfp_probe(struct phy_device *phydev,
-		  const struct sfp_upstream_ops *ops)
+static int phy_sfp_probe(struct phy_device *phydev)
 {
 	struct sfp_bus *bus;
 	int ret = 0;
@@ -1606,7 +1600,7 @@ int phy_sfp_probe(struct phy_device *phydev,
 
 		phydev->sfp_bus = bus;
 
-		ret = sfp_bus_add_upstream(bus, phydev, ops);
+		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
 		sfp_bus_put(bus);
 	}
 
@@ -1615,7 +1609,6 @@ int phy_sfp_probe(struct phy_device *phydev,
 
 	return ret;
 }
-EXPORT_SYMBOL(phy_sfp_probe);
 
 static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 {
@@ -3432,12 +3425,9 @@ static int phy_setup_ports(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	/* Use generic SFP probing only if the driver didn't do so already */
-	if (!phydev->sfp_bus) {
-		ret = phy_sfp_probe(phydev, &sfp_phydev_ops);
-		if (ret)
-			goto out;
-	}
+	ret = phy_sfp_probe(phydev);
+	if (ret)
+		goto out;
 
 	if (phydev->n_ports < phydev->max_n_ports) {
 		ret = phy_default_setup_single_port(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index aef13fab8882..4df1c951dcf2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1796,12 +1796,6 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable, int speed);
-int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
-void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
-void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
-void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
-int phy_sfp_probe(struct phy_device *phydev,
-	          const struct sfp_upstream_ops *ops);
 struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 			      phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
-- 
2.49.0

[PATCH net-next v6 13/14] net: phy: dp83822: Add SFP support through the phy_port interface

[Maxime Chevallier]

The DP83822 can support 100BaseFX. This mode was only accessible through
custom DT properties, but there also exist SFP modules that support
these modes. As this only requires setting the relevant supported
interface in the driver, expose the port capability with the new
phy_port API, allowing SFP support.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/dp83822.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index a0b4ebddd484..194de35bd064 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -982,6 +982,13 @@ static int dp83822_attach_port(struct phy_device *phydev, struct phy_port *port)
 		}
 	}
 
+	/* If attached from SFP, is_serdes is set, but not the mediums. */
+	if (port->is_serdes)
+		dp83822->fx_enabled = true;
+
+	if (dp83822->fx_enabled)
+		__set_bit(PHY_INTERFACE_MODE_100BASEX, port->interfaces);
+
 	return 0;
 }
 
-- 
2.49.0

[PATCH net-next v6 14/14] Documentation: networking: Document the phy_port infrastructure

[Maxime Chevallier]

This documentation aims at describing the main goal of the phy_port
infrastructure.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/index.rst    |   1 +
 Documentation/networking/phy-port.rst | 111 ++++++++++++++++++++++++++
 MAINTAINERS                           |   1 +
 3 files changed, 113 insertions(+)
 create mode 100644 Documentation/networking/phy-port.rst

diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
index ac90b82f3ce9..f60acc06e3f7 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst
@@ -96,6 +96,7 @@ Contents:
    packet_mmap
    phonet
    phy-link-topology
+   phy-port
    pktgen
    plip
    ppp_generic
diff --git a/Documentation/networking/phy-port.rst b/Documentation/networking/phy-port.rst
new file mode 100644
index 000000000000..6d9d46ebe438
--- /dev/null
+++ b/Documentation/networking/phy-port.rst
@@ -0,0 +1,111 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. _phy_port:
+
+=================
+Ethernet ports
+=================
+
+This document is a basic description of the phy_port infrastructure,
+introduced to represent physical interfaces of Ethernet devices.
+
+Without phy_port, we already have quite a lot of information about what the
+media-facing interface of a NIC can do and looks like, through the
+:c:type:`struct ethtool_link_ksettings <ethtool_link_ksettings>` attributes,
+which includes :
+
+ - What the NIC can do through the :c:member:`supported` field
+ - What the Link Partner advertises through :c:member:`lp_advertising`
+ - Which features we're advertising through :c:member:`advertising`
+
+We also have info about the number of lanes and the PORT type. These settings
+are built by aggregating together information reported by various devices that
+are sitting on the link :
+
+  - The NIC itself, through the :c:member:`get_link_ksettings` callback
+  - Precise information from the MAC and PCS by using phylink in the MAC driver
+  - Information reported by the PHY device
+  - Information reported by an SFP module (which can itself include a PHY)
+
+This model however starts showing its limitations when we consider devices that
+have more than one media interface. In such a case, only information about the
+actively used interface is reported, and it's not possible to know what the
+other interfaces can do. In fact, we have very few information about whether or
+not there are any other media interfaces.
+
+The goal of the phy_port representation is to provide a way of representing a
+physical interface of a NIC, regardless of what is driving the port (NIC through
+a firmware, SFP module, Ethernet PHY).
+
+Multi-port interfaces examples
+==============================
+
+Several cases of multi-interface NICs have been observed so far :
+
+Internal MII Mux::
+
+  +------------------+
+  | SoC              |
+  |          +-----+ |           +-----+
+  | +-----+  |     |-------------| PHY |
+  | | MAC |--| Mux | |   +-----+ +-----+
+  | +-----+  |     |-----| SFP |
+  |          +-----+ |   +-----+
+  +------------------+
+
+Internal Mux with internal PHY::
+
+  +------------------------+
+  | SoC                    |
+  |          +-----+ +-----+
+  | +-----+  |     |-| PHY |
+  | | MAC |--| Mux | +-----+   +-----+
+  | +-----+  |     |-----------| SFP |
+  |          +-----+       |   +-----+
+  +------------------------+
+
+External Mux::
+
+  +---------+
+  | SoC     |  +-----+  +-----+
+  |         |  |     |--| PHY |
+  | +-----+ |  |     |  +-----+
+  | | MAC |----| Mux |  +-----+
+  | +-----+ |  |     |--| PHY |
+  |         |  +-----+  +-----+
+  |         |     |
+  |    GPIO-------+
+  +---------+
+
+Double-port PHY::
+
+  +---------+
+  | SoC     | +-----+
+  |         | |     |--- RJ45
+  | +-----+ | |     |
+  | | MAC |---| PHY |   +-----+
+  | +-----+ | |     |---| SFP |
+  +---------+ +-----+   +-----+
+
+phy_port aims at providing a path to support all the above topologies, by
+representing the media interfaces in a way that's agnostic to what's driving
+the interface. the struct phy_port object has its own set of callback ops, and
+will eventually be able to report its own ksettings::
+
+             _____      +------+
+            (     )-----| Port |
+ +-----+   (       )    +------+
+ | MAC |--(   ???   )
+ +-----+   (       )    +------+
+            (_____)-----| Port |
+                        +------+
+
+Next steps
+==========
+
+As of writing this documentation, only ports controlled by PHY devices are
+supported. The next steps will be to add the Netlink API to expose these
+to userspace and add support for raw ports (controlled by some firmware, and directly
+managed by the NIC driver).
+
+Another parallel task is the introduction of a MII muxing framework to allow the
+control of non-PHY driver multi-port setups.
diff --git a/MAINTAINERS b/MAINTAINERS
index b155eec69552..211a6ba50166 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8781,6 +8781,7 @@ F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
 F:	Documentation/devicetree/bindings/net/ethernet-phy.yaml
 F:	Documentation/devicetree/bindings/net/mdio*
 F:	Documentation/devicetree/bindings/net/qca,ar803x.yaml
+F:	Documentation/networking/phy-port.rst
 F:	Documentation/networking/phy.rst
 F:	drivers/net/mdio/
 F:	drivers/net/mdio/acpi_mdio.c
-- 
2.49.0

Re: [PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Romain Gantois]

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

Hi Maxime,

On Wednesday, 7 May 2025 15:53:19 CEST Maxime Chevallier wrote:
> Ethernet provides a wide variety of layer 1 protocols and standards for
> data transmission. The front-facing ports of an interface have their own
> complexity and configurability.
> 
...
> +
> +static int phy_default_setup_single_port(struct phy_device *phydev)
> +{
> +	struct phy_port *port = phy_port_alloc();
> +
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->parent_type = PHY_PORT_PHY;
> +	port->phy = phydev;
> +	linkmode_copy(port->supported, phydev->supported);
> +
> +	phy_add_port(phydev, port);
> +
> +	/* default medium is copper */
> +	if (!port->mediums)
> +		port->mediums |= BIT(ETHTOOL_LINK_MEDIUM_BASET);

Could this be moved to phy_port_alloc() instead? That way, you'd avoid the 
extra conditional and the "default medium == baseT" rule would be enforced as 
early as possible.

> +
> +	return 0;
> +}
> +
> +static int of_phy_ports(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *mdi;
> +	struct phy_port *port;
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return 0;
> +
> +	if (!node)
> +		return 0;
> +
> +	mdi = of_get_child_by_name(node, "mdi");
> +	if (!mdi)
> +		return 0;

There are a lot of different possible failure paths here, so some specific error 
messages would be relevant IMO.

> +
> +	for_each_available_child_of_node_scoped(mdi, port_node) {
> +		port = phy_of_parse_port(port_node);
> +		if (IS_ERR(port)) {
> +			err = PTR_ERR(port);
> +			goto out_err;
> +		}
> +
> +		port->parent_type = PHY_PORT_PHY;
> +		port->phy = phydev;
> +		err = phy_add_port(phydev, port);
> +		if (err)
> +			goto out_err;
> +	}
> +	of_node_put(mdi);
> +
> +	return 0;
> +
> +out_err:
> +	phy_cleanup_ports(phydev);
> +	of_node_put(mdi);
> +	return err;
> +}
> +
> +static int phy_setup_ports(struct phy_device *phydev)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(ports_supported);
> +	struct phy_port *port;
> +	int ret;
> +
> +	ret = of_phy_ports(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->n_ports < phydev->max_n_ports) {
> +		ret = phy_default_setup_single_port(phydev);
> +		if (ret)
> +			goto out;
> +	}

Couldn't the function return at this point if phydev->n_ports is 0? That 
would eliminate the need for the linkmode_empty() check later.

> +
> +	linkmode_zero(ports_supported);
> +
> +	/* Aggregate the supported modes, which are made-up of :
> +	 *  - What the PHY itself supports
> +	 *  - What the sum of all ports support
> +	 */
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active)
> +			linkmode_or(ports_supported, ports_supported,
> +				    port->supported);
> +
> +	if (!linkmode_empty(ports_supported))
> +		linkmode_and(phydev->supported, phydev->supported,
> +			     ports_supported);
> +
> +	/* For now, the phy->port field is set as the first active port's type 
*/
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active)
> +			phydev->port = phy_port_get_type(port);
> +
> +	return 0;
> +
> +out:
> +	phy_cleanup_ports(phydev);
> +	return ret;
> +}
> +
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode
> @@ -3339,6 +3500,11 @@ static int phy_probe(struct device *dev)
>  		phydev->is_gigabit_capable = 1;
> 
>  	of_set_phy_supported(phydev);
> +
> +	err = phy_setup_ports(phydev);
> +	if (err)
> +		goto out;
> +
>  	phy_advertise_supported(phydev);
> 
>  	/* Get PHY default EEE advertising modes and handle them as 
potentially
> @@ -3414,6 +3580,8 @@ static int phy_remove(struct device *dev)
> 
>  	phydev->state = PHY_DOWN;
> 
> +	phy_cleanup_ports(phydev);
> +
>  	sfp_bus_del_upstream(phydev->sfp_bus);
>  	phydev->sfp_bus = NULL;
> 
> diff --git a/drivers/net/phy/phy_port.c b/drivers/net/phy/phy_port.c
> new file mode 100644
> index 000000000000..c69ba0d9afba
> --- /dev/null
> +++ b/drivers/net/phy/phy_port.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Framework to drive Ethernet ports
> + *
> + * Copyright (c) 2024 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/linkmode.h>
> +#include <linux/of.h>
> +#include <linux/phy_port.h>
> +
> +/**
> + * phy_port_alloc() - Allocate a new phy_port
> + *
> + * Returns: a newly allocated struct phy_port, or NULL.
> + */
> +struct phy_port *phy_port_alloc(void)
> +{
> +	struct phy_port *port;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return NULL;
> +
> +	linkmode_zero(port->supported);
> +	INIT_LIST_HEAD(&port->head);
> +
> +	return port;
> +}
> +EXPORT_SYMBOL_GPL(phy_port_alloc);
> +
> +/**
> + * phy_port_destroy() - Free a struct phy_port
> + * @port: The port to destroy
> + */
> +void phy_port_destroy(struct phy_port *port)
> +{
> +	kfree(port);
> +}
> +EXPORT_SYMBOL_GPL(phy_port_destroy);
> +
> +static void ethtool_medium_get_supported(unsigned long *supported,
> +					 enum ethtool_link_medium medium,
> +					 int lanes)
> +{
> +	int i;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
> +		/* Special bits such as Autoneg, Pause, Asym_pause, etc. are
> +		 * set and will be masked away by the port parent.
> +		 */
> +		if (link_mode_params[i].mediums == 
BIT(ETHTOOL_LINK_MEDIUM_NONE)) {
> +			linkmode_set_bit(i, supported);
> +			continue;

If you change the subsequent "if" into an "else if", you'll avoid having to 
use "continue" here, which IMO would make things a bit clearer.

> +		}
> +
> +		/* For most cases, min_lanes == lanes, except for 10/100BaseT 
that work
> +		 * on 2 lanes but are compatible with 4 lanes mediums
> +		 */
> +		if (link_mode_params[i].mediums & BIT(medium) &&
> +		    link_mode_params[i].lanes >= lanes &&
> +		    link_mode_params[i].min_lanes <= lanes) {
> +			linkmode_set_bit(i, supported);
> +		}
> +	}
> +}
> +
> +static enum ethtool_link_medium ethtool_str_to_medium(const char *str)
> +{
> +	int i;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MEDIUM_LAST; i++)
> +		if (!strcmp(phy_mediums(i), str))
> +			return i;
> +
> +	return ETHTOOL_LINK_MEDIUM_NONE;
> +}
> +
> +/**
> + * phy_of_parse_port() - Create a phy_port from a firmware representation
> + * @dn: device_node representation of the port, following the
> + *	ethernet-connector.yaml binding
> + *
> + * Returns: a newly allocated and initialized phy_port pointer, or an
> ERR_PTR. + */
> +struct phy_port *phy_of_parse_port(struct device_node *dn)
> +{
> +	struct fwnode_handle *fwnode = of_fwnode_handle(dn);
> +	enum ethtool_link_medium medium;
> +	struct phy_port *port;
> +	struct property *prop;
> +	const char *med_str;
> +	u32 lanes, mediums = 0;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "lanes", &lanes);
> +	if (ret)
> +		lanes = 0;

Checking if this property exists before calling fwnode_property_read_u32() 
would avoid masking potential error conditions where the property exists, but 
something goes wrong while reading it.

> +
> +	ret = fwnode_property_read_string(fwnode, "media", &med_str);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	of_property_for_each_string(to_of_node(fwnode), "media", prop, 
med_str) {
> +		medium = ethtool_str_to_medium(med_str);
> +		if (medium == ETHTOOL_LINK_MEDIUM_NONE)
> +			return ERR_PTR(-EINVAL);
> +
> +		mediums |= BIT(medium);
> +	}
> +
> +	if (!mediums)
> +		return ERR_PTR(-EINVAL);
> +
> +	port = phy_port_alloc();
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	port->lanes = lanes;
> +	port->mediums = mediums;
> +
> +	return port;
> +}
> +EXPORT_SYMBOL_GPL(phy_of_parse_port);
> +
> +/**
> + * phy_port_update_supported() - Setup the port->supported field
> + * @port: the port to update
> + *
> + * Once the port's medium list and number of lanes has been configured
> based + * on firmware, straps and vendor-specific properties, this function
> may be + * called to update the port's supported linkmodes list.
> + *
> + * Any mode that was manually set in the port's supported list remains set.
> + */
> +void phy_port_update_supported(struct phy_port *port)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +	int i, lanes = 1;
> +
> +	/* If there's no lanes specified, we grab the default number of
> +	 * lanes as the max of the default lanes for each medium
> +	 */
> +	if (!port->lanes)
> +		for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
> +			lanes = max_t(int, lanes, phy_medium_default_lanes(i));
> +
> +	for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST) {
> +		linkmode_zero(supported);

ethtool_medium_get_supported() can only set bits in "supported", so you could 
just do:

```
for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
	ethtool_medium_get_supported(port->supported, i, port->lanes);
```

unless you're wary of someone modifying ethtool_medium_get_supported() in a 
way that breaks this in the future?

> +		ethtool_medium_get_supported(supported, i, port->lanes);
> +		linkmode_or(port->supported, port->supported, supported);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phy_port_update_supported);
> +
> +/**
> + * phy_port_get_type() - get the PORT_* attribut for that port.
> + * @port: The port we want the information from
> + *
> + * Returns: A PORT_XXX value.
> + */
> +int phy_port_get_type(struct phy_port *port)
> +{
> +	if (port->mediums & ETHTOOL_LINK_MEDIUM_BASET)
> +		return PORT_TP;
> +
> +	if (phy_port_is_fiber(port))
> +		return PORT_FIBRE;
> +
> +	return PORT_OTHER;
> +}
> +EXPORT_SYMBOL_GPL(phy_port_get_type);
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c1d805d3e02f..0d3063af5905 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -226,6 +226,10 @@ extern const struct link_mode_info link_mode_params[];
> 
>  extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
> 
> +#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
> +				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
> +				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
> +
>  static inline const char *phy_mediums(enum ethtool_link_medium medium)
>  {
>  	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
> @@ -234,6 +238,17 @@ static inline const char *phy_mediums(enum
> ethtool_link_medium medium) return ethtool_link_medium_names[medium];
>  }
> 
> +static inline int phy_medium_default_lanes(enum ethtool_link_medium medium)
> +{
> +	/* Let's consider that the default BaseT ethernet is BaseT4, i.e.
> +	 * Gigabit Ethernet.
> +	 */
> +	if (medium == ETHTOOL_LINK_MEDIUM_BASET)
> +		return 4;
> +
> +	return 1;
> +}
> +
>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index d62d292024bc..0180f4d4fd7d 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -299,6 +299,7 @@ static inline long rgmii_clock(int speed)
>  struct device;
>  struct kernel_hwtstamp_config;
>  struct phylink;
> +struct phy_port;
>  struct sfp_bus;
>  struct sfp_upstream_ops;
>  struct sk_buff;
> @@ -590,6 +591,9 @@ struct macsec_ops;
>   * @master_slave_state: Current master/slave configuration
>   * @mii_ts: Pointer to time stamper callbacks
>   * @psec: Pointer to Power Sourcing Equipment control struct
> + * @ports: List of PHY ports structures
> + * @n_ports: Number of ports currently attached to the PHY
> + * @max_n_ports: Max number of ports this PHY can expose
>   * @lock:  Mutex for serialization access to PHY
>   * @state_queue: Work queue for state machine
>   * @link_down_events: Number of times link was lost
> @@ -724,6 +728,10 @@ struct phy_device {
>  	struct mii_timestamper *mii_ts;
>  	struct pse_control *psec;
> 
> +	struct list_head ports;
> +	int n_ports;
> +	int max_n_ports;
> +
>  	u8 mdix;
>  	u8 mdix_ctrl;
> 
> @@ -1242,6 +1250,27 @@ struct phy_driver {
>  	 * Returns the time in jiffies until the next update event.
>  	 */
>  	unsigned int (*get_next_update_time)(struct phy_device *dev);
> +
> +	/**
> +	 * @attach_port: Indicates to the PHY driver that a port is detected
> +	 * @dev: PHY device to notify
> +	 * @port: The port being added
> +	 *
> +	 * Called when a port that needs to be driven by the PHY is found. The
> +	 * number of time this will be called depends on phydev->max_n_ports,
> +	 * which the driver can change in .probe().
> +	 *
> +	 * The port that is being passed may or may not be initialized. If it 
is
> +	 * already initialized, it is by the generic port representation from
> +	 * devicetree, which superseeds any strapping or vendor-specific
> +	 * properties.
> +	 *
> +	 * If the port isn't initialized, the port->mediums and port->lanes
> +	 * fields must be set, possibly according to stapping information.
> +	 *
> +	 * Returns 0, or an error code.
> +	 */
> +	int (*attach_port)(struct phy_device *dev, struct phy_port *port);
>  };
>  #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		
\
>  				      struct phy_driver, mdiodrv)
> @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
>  void phy_mac_interrupt(struct phy_device *phydev);
>  void phy_start_machine(struct phy_device *phydev);
>  void phy_stop_machine(struct phy_device *phydev);
> +
>  void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  			       struct ethtool_link_ksettings *cmd);
>  int phy_ethtool_ksettings_set(struct phy_device *phydev,
> diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
> new file mode 100644
> index 000000000000..70aa75f93096
> --- /dev/null
> +++ b/include/linux/phy_port.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __PHY_PORT_H
> +#define __PHY_PORT_H
> +
> +#include <linux/ethtool.h>
> +#include <linux/types.h>
> +#include <linux/phy.h>
> +
> +struct phy_port;
> +
> +/**
> + * enum phy_port_parent - The device this port is attached to
> + *
> + * @PHY_PORT_PHY: Indicates that the port is driven by a PHY device
> + */
> +enum phy_port_parent {
> +	PHY_PORT_PHY,
> +};
> +
> +struct phy_port_ops {
> +	/* Sometimes, the link state can be retrieved from physical,
> +	 * out-of-band channels such as the LOS signal on SFP. These
> +	 * callbacks allows notifying the port about state changes
> +	 */
> +	void (*link_up)(struct phy_port *port);
> +	void (*link_down)(struct phy_port *port);
> +
> +	/* If the port acts as a Media Independent Interface (Serdes port),
> +	 * configures the port with the relevant state and mode. When enable is
> +	 * not set, interface should be ignored
> +	 */
> +	int (*configure_mii)(struct phy_port *port, bool enable, 
phy_interface_t
> interface); +};
> +
> +/**
> + * struct phy_port - A representation of a network device physical
> interface + *
> + * @head: Used by the port's parent to list ports
> + * @parent_type: The type of device this port is directly connected to
> + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> + * @ops: Callback ops implemented by the port controller
> + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> applicable + * @mediums: Bitmask of the physical mediums this port provides
> access to + * @supported: The link modes this port can expose, if this port
> is MDI (not MII) + * @interfaces: The MII interfaces this port supports, if
> this port is MII + * @active: Indicates if the port is currently part of
> the active link. + * @is_serdes: Indicates if this port is Serialised MII
> (Media Independent + *	       Interface), or an MDI (Media Dependent
> Interface).
> + */
> +struct phy_port {
> +	struct list_head head;
> +	enum phy_port_parent parent_type;
> +	union {
> +		struct phy_device *phy;
> +	};
> +
> +	const struct phy_port_ops *ops;
> +
> +	int lanes;
> +	unsigned long mediums;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> +
> +	unsigned int active:1;
> +	unsigned int is_serdes:1;
> +};
> +
> +struct phy_port *phy_port_alloc(void);
> +void phy_port_destroy(struct phy_port *port);
> +
> +static inline struct phy_device *port_phydev(struct phy_port *port)
> +{
> +	return port->phy;
> +}
> +
> +struct phy_port *phy_of_parse_port(struct device_node *dn);
> +
> +static inline bool phy_port_is_copper(struct phy_port *port)
> +{
> +	return port->mediums == BIT(ETHTOOL_LINK_MEDIUM_BASET);

BaseC is also "copper" right? Maybe this should be renamed to 
"phy_port_is_tp"?

> +}
> +
> +static inline bool phy_port_is_fiber(struct phy_port *port)
> +{
> +	return !!(port->mediums & ETHTOOL_MEDIUM_FIBER_BITS);
> +}
> +
> +void phy_port_update_supported(struct phy_port *port);
> +
> +int phy_port_get_type(struct phy_port *port);
> +
> +#endif

Thanks!

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois]

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

Hi Maxime,

On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:
> There are currently 4 PHY drivers that can drive downstream SFPs:
> marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> logic is boilerplate, either calling into generic phylib helpers (for
> SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> bit of validation :
>  - Getting the module's expected interface mode
>  - Making sure the PHY supports it
>  - Optionnaly perform some configuration to make sure the PHY outputs
>    the right mode
> 
> This can be made more generic by leveraging the phy_port, and its
> configure_mii() callback which allows setting a port's interfaces when
> the port is a serdes.
> 
> Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> bus itself, but an SFP phandle is found in devicetree/firmware, then the
> generic PHY SFP support will be used, relying on port ops.
> 
> PHY driver need to :
>  - Register a .attach_port() callback
>  - When a serdes port is registered to the PHY, drivers must set
>    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
>  - If the port has limitations regarding speed, duplex and aneg, the
>    port can also fine-tune the final linkmodes that can be supported
>  - The port may register a set of ops, including .configure_mii(), that
>    will be called at module_insert time to adjust the interface based on
>    the module detected.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |   2 +
>  2 files changed, 109 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index aaf0eccbefba..aca3a47cbb66 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct sfp_bus
> *bus) }
>  EXPORT_SYMBOL(phy_sfp_detach);
> 
> +static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id
> *id) +{
> +	struct phy_device *phydev = upstream;
> +	struct phy_port *port = phy_get_sfp_port(phydev);
> +

RCT

> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> +	phy_interface_t iface;
> +
> +	linkmode_zero(sfp_support);
> +
> +	if (!port)
> +		return -EINVAL;
> +
> +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> +
> +	if (phydev->n_ports == 1)
> +		phydev->port = sfp_parse_port(phydev->sfp_bus, id, 
sfp_support);

As mentionned below, this check looks a bit strange to me. Why are we only 
parsing the SFP port if the PHY device only has one registered port?

> +
> +	linkmode_and(sfp_support, port->supported, sfp_support);
> +
> +	if (linkmode_empty(sfp_support)) {
> +		dev_err(&phydev->mdio.dev, "incompatible SFP module 
inserted\n");
> +		return -EINVAL;
> +	}
> +
> +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> +
> +	/* Check that this interface is supported */
> +	if (!test_bit(iface, port->interfaces)) {
> +		dev_err(&phydev->mdio.dev, "incompatible SFP module 
inserted\n");
> +		return -EINVAL;
> +	}
> +
> +	if (port->ops && port->ops->configure_mii)
> +		return port->ops->configure_mii(port, true, iface);

The name "configure_mii()" seems a bit narrow-scoped to me, as this callback 
might have to configure something else than a MII link. For example, if a DAC 
SFP module is inserted, the downstream side of the transciever will have to be 
configured to 1000Base-X or something similar.

I'd suggest something like "post_sfp_insert()", please let me know what you 
think.

> +
> +	return 0;
> +}
> +
> +static void phy_sfp_module_remove(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct phy_port *port = phy_get_sfp_port(phydev);
> +
> +	if (port && port->ops && port->ops->configure_mii)
> +		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
> +
> +	if (phydev->n_ports == 1)
> +		phydev->port = PORT_NONE;

This check is a bit confusing to me. Could you please explain why you're only 
setting the phydev's SFP port to PORT_NONE if the PHY device only has one 
registered port? Shouldn't this be done regardless?

> +}
> +
> +static void phy_sfp_link_up(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct phy_port *port = phy_get_sfp_port(phydev);
> +
> +	if (port && port->ops && port->ops->link_up)
> +		port->ops->link_up(port);
> +}
> +
> +static void phy_sfp_link_down(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct phy_port *port = phy_get_sfp_port(phydev);
> +
> +	if (port && port->ops && port->ops->link_down)
> +		port->ops->link_down(port);
> +}
> +
> +static const struct sfp_upstream_ops sfp_phydev_ops = {
> +	.attach = phy_sfp_attach,
> +	.detach = phy_sfp_detach,
> +	.module_insert = phy_sfp_module_insert,
> +	.module_remove = phy_sfp_module_remove,
> +	.link_up = phy_sfp_link_up,
> +	.link_down = phy_sfp_link_down,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
> +};
> +
>  static int phy_add_port(struct phy_device *phydev, struct phy_port *port)
>  {
>  	int ret = 0;
> @@ -3351,6 +3432,13 @@ static int phy_setup_ports(struct phy_device *phydev)
> if (ret)
>  		return ret;
> 
> +	/* Use generic SFP probing only if the driver didn't do so already */
> +	if (!phydev->sfp_bus) {

Should the phy_sfp_probe() API be changed to something explicitely legacy? I 
feel like people writing new PHY drivers could be confused if they see some 
other drivers calling phy_sfp_probe() and others not doing anything and still 
getting SFP busses.

> +		ret = phy_sfp_probe(phydev, &sfp_phydev_ops);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	if (phydev->n_ports < phydev->max_n_ports) {
>  		ret = phy_default_setup_single_port(phydev);
>  		if (ret)
> @@ -3384,6 +3472,25 @@ static int phy_setup_ports(struct phy_device *phydev)
> return ret;
>  }
> 
> +/**
> + * phy_get_sfp_port() - Returns the first valid SFP port of a PHY
> + * @phydev: pointer to the PHY device to get the SFP port from
> + *
> + * Returns: The first active SFP (serdes) port of a PHY device, NULL if
> none + * exist.
> + */
> +struct phy_port *phy_get_sfp_port(struct phy_device *phydev)

I'd suggest "phy_get_active_sfp_port()".

> +{
> +	struct phy_port *port;
> +
> +	list_for_each_entry(port, &phydev->ports, head)
> +		if (port->active && port->is_serdes)
> +			return port;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(phy_get_sfp_port);
> +
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 0180f4d4fd7d..aef13fab8882 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -2091,6 +2091,8 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
>  		       struct kernel_hwtstamp_config *config,
>  		       struct netlink_ext_ack *extack);
> 
> +struct phy_port *phy_get_sfp_port(struct phy_device *phydev);
> +
>  extern const struct bus_type mdio_bus_type;
>  extern const struct class mdio_bus_class;

Thanks!

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 12/14] net: phy: Only rely on phy_port for PHY-driven SFP

[Romain Gantois]

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

On Wednesday, 7 May 2025 15:53:28 CEST Maxime Chevallier wrote:
> Now that all PHY drivers that support downstream SFP have been converted
> to phy_port serdes handling, we can make the generic PHY SFP handling
> mandatory, thus making all phylib sfp helpers static.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_device.c | 28 +++++++++-------------------
>  include/linux/phy.h          |  6 ------
>  2 files changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index aca3a47cbb66..7f319526a7fe 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1384,7 +1384,7 @@ static DEVICE_ATTR_RO(phy_standalone);
>   *
>   * Return: 0 on success, otherwise a negative error code.
>   */
> -int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
> +static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  {
>  	struct phy_device *phydev = upstream;
>  	struct net_device *dev = phydev->attached_dev;
> @@ -1394,7 +1394,6 @@ int phy_sfp_connect_phy(void *upstream, struct
> phy_device *phy)
> 
>  	return 0;
>  }
> -EXPORT_SYMBOL(phy_sfp_connect_phy);
> 
>  /**
>   * phy_sfp_disconnect_phy - Disconnect the SFP module's PHY from the
> upstream PHY @@ -1406,7 +1405,7 @@ EXPORT_SYMBOL(phy_sfp_connect_phy);
>   * will be destroyed, re-inserting the same module will add a new phy with
> a * new index.
>   */
> -void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
> +static void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
>  {
>  	struct phy_device *phydev = upstream;
>  	struct net_device *dev = phydev->attached_dev;
> @@ -1414,7 +1413,6 @@ void phy_sfp_disconnect_phy(void *upstream, struct
> phy_device *phy) if (dev)
>  		phy_link_topo_del_phy(dev, phy);
>  }
> -EXPORT_SYMBOL(phy_sfp_disconnect_phy);
> 
>  /**
>   * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
> @@ -1423,7 +1421,7 @@ EXPORT_SYMBOL(phy_sfp_disconnect_phy);
>   *
>   * This is used to fill in the sfp_upstream_ops .attach member.
>   */
> -void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
> +static void phy_sfp_attach(void *upstream, struct sfp_bus *bus)
>  {
>  	struct phy_device *phydev = upstream;
> 
> @@ -1431,7 +1429,6 @@ void phy_sfp_attach(void *upstream, struct sfp_bus
> *bus) phydev->attached_dev->sfp_bus = bus;
>  	phydev->sfp_bus_attached = true;
>  }
> -EXPORT_SYMBOL(phy_sfp_attach);
> 
>  /**
>   * phy_sfp_detach - detach the SFP bus from the PHY upstream network device
> @@ -1440,7 +1437,7 @@ EXPORT_SYMBOL(phy_sfp_attach);
>   *
>   * This is used to fill in the sfp_upstream_ops .detach member.
>   */
> -void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
> +static void phy_sfp_detach(void *upstream, struct sfp_bus *bus)
>  {
>  	struct phy_device *phydev = upstream;
> 
> @@ -1448,7 +1445,6 @@ void phy_sfp_detach(void *upstream, struct sfp_bus
> *bus) phydev->attached_dev->sfp_bus = NULL;
>  	phydev->sfp_bus_attached = false;
>  }
> -EXPORT_SYMBOL(phy_sfp_detach);
> 
>  static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id
> *id) {
> @@ -1591,10 +1587,8 @@ static int phy_setup_sfp_port(struct phy_device
> *phydev) /**
>   * phy_sfp_probe - probe for a SFP cage attached to this PHY device
>   * @phydev: Pointer to phy_device
> - * @ops: SFP's upstream operations
>   */
> -int phy_sfp_probe(struct phy_device *phydev,
> -		  const struct sfp_upstream_ops *ops)
> +static int phy_sfp_probe(struct phy_device *phydev)
>  {
>  	struct sfp_bus *bus;
>  	int ret = 0;
> @@ -1606,7 +1600,7 @@ int phy_sfp_probe(struct phy_device *phydev,
> 
>  		phydev->sfp_bus = bus;
> 
> -		ret = sfp_bus_add_upstream(bus, phydev, ops);
> +		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
>  		sfp_bus_put(bus);
>  	}
> 
> @@ -1615,7 +1609,6 @@ int phy_sfp_probe(struct phy_device *phydev,
> 
>  	return ret;
>  }
> -EXPORT_SYMBOL(phy_sfp_probe);
> 
>  static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  {
> @@ -3432,12 +3425,9 @@ static int phy_setup_ports(struct phy_device *phydev)
> if (ret)
>  		return ret;
> 
> -	/* Use generic SFP probing only if the driver didn't do so already */
> -	if (!phydev->sfp_bus) {

Alright, since you removed this, my earlier review comment about potentially 
making phy_sfp_probe() legacy doesn't apply.

> -		ret = phy_sfp_probe(phydev, &sfp_phydev_ops);
> -		if (ret)
> -			goto out;
> -	}
> +	ret = phy_sfp_probe(phydev);
> +	if (ret)
> +		goto out;
> 
>  	if (phydev->n_ports < phydev->max_n_ports) {
>  		ret = phy_default_setup_single_port(phydev);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index aef13fab8882..4df1c951dcf2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1796,12 +1796,6 @@ int phy_suspend(struct phy_device *phydev);
>  int phy_resume(struct phy_device *phydev);
>  int __phy_resume(struct phy_device *phydev);
>  int phy_loopback(struct phy_device *phydev, bool enable, int speed);
> -int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
> -void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
> -void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
> -void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
> -int phy_sfp_probe(struct phy_device *phydev,
> -	          const struct sfp_upstream_ops *ops);
>  struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
>  			      phy_interface_t interface);
>  struct phy_device *phy_find_first(struct mii_bus *bus);

Thanks!

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 14/14] Documentation: networking: Document the phy_port infrastructure

[Romain Gantois]

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

Hi Maxime,

On Wednesday, 7 May 2025 15:53:30 CEST Maxime Chevallier wrote:
> This documentation aims at describing the main goal of the phy_port
> infrastructure.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  Documentation/networking/index.rst    |   1 +
>  Documentation/networking/phy-port.rst | 111 ++++++++++++++++++++++++++
>  MAINTAINERS                           |   1 +
>  3 files changed, 113 insertions(+)
>  create mode 100644 Documentation/networking/phy-port.rst
> 
> diff --git a/Documentation/networking/index.rst
> b/Documentation/networking/index.rst index ac90b82f3ce9..f60acc06e3f7
> 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -96,6 +96,7 @@ Contents:
>     packet_mmap
>     phonet
>     phy-link-topology
> +   phy-port
>     pktgen
>     plip
>     ppp_generic
> diff --git a/Documentation/networking/phy-port.rst
> b/Documentation/networking/phy-port.rst new file mode 100644
> index 000000000000..6d9d46ebe438
> --- /dev/null
> +++ b/Documentation/networking/phy-port.rst
> @@ -0,0 +1,111 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _phy_port:
> +
> +=================
> +Ethernet ports
> +=================
> +
> +This document is a basic description of the phy_port infrastructure,
> +introduced to represent physical interfaces of Ethernet devices.
> +
> +Without phy_port, we already have quite a lot of information about what the
> +media-facing interface of a NIC can do and looks like, through the

I'd replace "and looks like" by "and what it looks like".

> +:c:type:`struct ethtool_link_ksettings <ethtool_link_ksettings>`
> attributes, +which includes :
> +
> + - What the NIC can do through the :c:member:`supported` field
> + - What the Link Partner advertises through :c:member:`lp_advertising`
> + - Which features we're advertising through :c:member:`advertising`
> +
> +We also have info about the number of lanes and the PORT type. These
> settings +are built by aggregating together information reported by various
> devices that +are sitting on the link :
> +
> +  - The NIC itself, through the :c:member:`get_link_ksettings` callback
> +  - Precise information from the MAC and PCS by using phylink in the MAC
> driver +  - Information reported by the PHY device
> +  - Information reported by an SFP module (which can itself include a PHY)
> +
> +This model however starts showing its limitations when we consider devices
> that +have more than one media interface. In such a case, only information
> about the +actively used interface is reported, and it's not possible to
> know what the +other interfaces can do. In fact, we have very few
> information about whether or +not there are any other media interfaces.

maybe "hints" instead of "information"?

> +
> +The goal of the phy_port representation is to provide a way of representing
> a +physical interface of a NIC, regardless of what is driving the port (NIC
> through +a firmware, SFP module, Ethernet PHY).
> +
> +Multi-port interfaces examples
> +==============================
> +
> +Several cases of multi-interface NICs have been observed so far :
> +
> +Internal MII Mux::
> +
> +  +------------------+
> +  | SoC              |
> +  |          +-----+ |           +-----+
> +  | +-----+  |     |-------------| PHY |
> +  | | MAC |--| Mux | |   +-----+ +-----+
> +  | +-----+  |     |-----| SFP |
> +  |          +-----+ |   +-----+
> +  +------------------+
> +
> +Internal Mux with internal PHY::
> +
> +  +------------------------+
> +  | SoC                    |
> +  |          +-----+ +-----+
> +  | +-----+  |     |-| PHY |
> +  | | MAC |--| Mux | +-----+   +-----+
> +  | +-----+  |     |-----------| SFP |
> +  |          +-----+       |   +-----+
> +  +------------------------+
> +
> +External Mux::
> +
> +  +---------+
> +  | SoC     |  +-----+  +-----+
> +  |         |  |     |--| PHY |
> +  | +-----+ |  |     |  +-----+
> +  | | MAC |----| Mux |  +-----+
> +  | +-----+ |  |     |--| PHY |
> +  |         |  +-----+  +-----+
> +  |         |     |
> +  |    GPIO-------+
> +  +---------+
> +
> +Double-port PHY::
> +
> +  +---------+
> +  | SoC     | +-----+
> +  |         | |     |--- RJ45
> +  | +-----+ | |     |
> +  | | MAC |---| PHY |   +-----+
> +  | +-----+ | |     |---| SFP |
> +  +---------+ +-----+   +-----+
> +
> +phy_port aims at providing a path to support all the above topologies, by
> +representing the media interfaces in a way that's agnostic to what's
> driving +the interface. the struct phy_port object has its own set of

s/the/The

> callback ops, and +will eventually be able to report its own ksettings::
> +
> +             _____      +------+
> +            (     )-----| Port |
> + +-----+   (       )    +------+
> + | MAC |--(   ???   )
> + +-----+   (       )    +------+
> +            (_____)-----| Port |
> +                        +------+
> +
> +Next steps
> +==========
> +
> +As of writing this documentation, only ports controlled by PHY devices are
> +supported. The next steps will be to add the Netlink API to expose these
> +to userspace and add support for raw ports (controlled by some firmware,
> and directly +managed by the NIC driver).
> +
> +Another parallel task is the introduction of a MII muxing framework to

I'd suggest "related" instead of "parallel".

> allow the +control of non-PHY driver multi-port setups.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b155eec69552..211a6ba50166 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8781,6 +8781,7 @@
> F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
> F:	Documentation/devicetree/bindings/net/ethernet-phy.yaml
>  F:	Documentation/devicetree/bindings/net/mdio*
>  F:	Documentation/devicetree/bindings/net/qca,ar803x.yaml
> +F:	Documentation/networking/phy-port.rst
>  F:	Documentation/networking/phy.rst
>  F:	drivers/net/mdio/
>  F:	drivers/net/mdio/acpi_mdio.c

Thanks!

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 05/14] net: phy: Create a phy_port for PHY-driven SFPs

[Romain Gantois]

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

Hi Maxime,

On Wednesday, 7 May 2025 15:53:21 CEST Maxime Chevallier wrote:
> Some PHY devices may be used as media-converters to drive SFP ports (for
> example, to allow using SFP when the SoC can only output RGMII). This is
> already supported to some extend by allowing PHY drivers to registers
> themselves as being SFP upstream.
> 
...
>   *
> @@ -149,6 +151,21 @@ void phy_port_update_supported(struct phy_port *port)
>  		ethtool_medium_get_supported(supported, i, port->lanes);
>  		linkmode_or(port->supported, port->supported, supported);
>  	}
> +
> +	/* Serdes ports supported may through SFP may not have any medium set,
> +	 * as they will output PHY_INTERFACE_MODE_XXX modes. In that case, 
derive
> +	 * the supported list based on these interfaces
> +	 */
> +	if (port->is_serdes && linkmode_empty(supported)) {

The "supported" bitmap needs to be zeroed out before this check. If the port 
has no mediums, then the bitmap won't be initialized at this point.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 01/14] dt-bindings: net: Introduce the ethernet-connector description

[Kory Maincent]

On Wed,  7 May 2025 15:53:17 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> The ability to describe the physical ports of Ethernet devices is useful
> to describe multi-port devices, as well as to remove any ambiguity with
> regard to the nature of the port.
> 
> Moreover, describing ports allows for a better description of features
> that are tied to connectors, such as PoE through the PSE-PD devices.
> 
> Introduce a binding to allow describing the ports, for now with 2
> attributes :
> 
>  - The number of lanes, which is a quite generic property that allows
>    differentating between multiple similar technologies such as BaseT1
>    and "regular" BaseT (which usually means BaseT4).
> 
>  - The media that can be used on that port, such as BaseT for Twisted
>    Copper, BaseC for coax copper, BaseS/L for Fiber, BaseK for backplane
>    ethernet, etc. This allows defining the nature of the port, and
>    therefore avoids the need for vendor-specific properties such as
>    "micrel,fiber-mode" or "ti,fiber-mode".
> 
> The port description lives in its own file, as it is intended in the
> future to allow describing the ports for phy-less devices.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> +description:
> +  An Ethernet Connectr represents the output of a network component such as

Connector

With this typo fixed:
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8777,6 +8777,7 @@ R:	Russell King <linux@armlinux.org.uk>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/ABI/testing/sysfs-class-net-phydev
> +F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
>  F:	Documentation/devicetree/bindings/net/ethernet-phy.yaml
>  F:	Documentation/devicetree/bindings/net/mdio*
>  F:	Documentation/devicetree/bindings/net/qca,ar803x.yaml

You will need Russell acked-by for that. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Kory Maincent]

On Wed,  7 May 2025 15:53:19 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Ethernet provides a wide variety of layer 1 protocols and standards for
> data transmission. The front-facing ports of an interface have their own
> complexity and configurability.
> 
> Introduce a representation of these front-facing ports. The current code
> is minimalistic and only support ports controlled by PHY devices, but
> the plan is to extend that to SFP as well as raw Ethernet MACs that
> don't use PHY devices.
> 
> This minimal port representation allows describing the media and number
> of lanes of a port. From that information, we can derive the linkmodes
> usable on the port, which can be used to limit the capabilities of an
> interface.
> 
> For now, the port lanes and medium is derived from devicetree, defined
> by the PHY driver, or populated with default values (as we assume that
> all PHYs expose at least one port).
> 
> The typical example is 100M ethernet. 100BaseT can work using only 2
> lanes on a Cat 5 cables. However, in the situation where a 10/100/1000
> capable PHY is wired to its RJ45 port through 2 lanes only, we have no
> way of detecting that. The "max-speed" DT property can be used, but a
> more accurate representation can be used :
> 
> mdi {
> 	connector-0 {
> 		media = "BaseT";
> 		lanes = <2>;
> 	};
> };
> 
> From that information, we can derive the max speed reachable on the
> port.
> 
> Another benefit of having that is to avoid vendor-specific DT properties
> (micrel,fiber-mode or ti,fiber-mode).
> 
> This basic representation is meant to be expanded, by the introduction
> of port ops, userspace listing of ports, and support for multi-port
> devices.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

...

> +	for_each_available_child_of_node_scoped(mdi, port_node) {
> +		port = phy_of_parse_port(port_node);
> +		if (IS_ERR(port)) {
> +			err = PTR_ERR(port);
> +			goto out_err;
> +		}
> +
> +		port->parent_type = PHY_PORT_PHY;
> +		port->phy = phydev;
> +		err = phy_add_port(phydev, port);
> +		if (err)
> +			goto out_err;

I think of_node_put(port_node) is missing here.

...

> @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
>  void phy_mac_interrupt(struct phy_device *phydev);
>  void phy_start_machine(struct phy_device *phydev);
>  void phy_stop_machine(struct phy_device *phydev);
> +

New empty line here?

> +/**
> + * struct phy_port - A representation of a network device physical interface
> + *
> + * @head: Used by the port's parent to list ports
> + * @parent_type: The type of device this port is directly connected to
> + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> + * @ops: Callback ops implemented by the port controller
> + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> applicable
> + * @mediums: Bitmask of the physical mediums this port provides access to
> + * @supported: The link modes this port can expose, if this port is MDI (not
> MII)
> + * @interfaces: The MII interfaces this port supports, if this port is MII
> + * @active: Indicates if the port is currently part of the active link.
> + * @is_serdes: Indicates if this port is Serialised MII (Media Independent
> + *	       Interface), or an MDI (Media Dependent Interface).
> + */
> +struct phy_port {
> +	struct list_head head;
> +	enum phy_port_parent parent_type;
> +	union {
> +		struct phy_device *phy;
> +	};

The union is useless here?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v6 04/14] net: phy: dp83822: Add support for phy_port representation

[Kory Maincent]

On Wed,  7 May 2025 15:53:20 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> With the phy_port representation intrduced, we can use .attach_port to
> populate the port information based on either the straps or the
> ti,fiber-mode property. This allows simplifying the probe function and
> allow users to override the strapping configuration.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---

...
  
> +static int dp83822_attach_port(struct phy_device *phydev, struct phy_port
> *port) +{
> +	struct dp83822_private *dp83822 = phydev->priv;
> +	int ret;
> +
> +	if (port->mediums) {
> +		if (phy_port_is_fiber(port))
> +			dp83822->fx_enabled = true;
> +	} else {
> +		ret = dp83822_read_straps(phydev);
> +		if (ret)
> +			return ret;
> +
> +#ifdef CONFIG_OF_MDIO

if IS_ENABLED(CONFIG_OF_MDIO) seems to be more used than ifdef

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Maxime Chevallier]

Hi Romain,

On Mon, 12 May 2025 10:38:52 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hi Maxime,
> 
> On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:
> > There are currently 4 PHY drivers that can drive downstream SFPs:
> > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > logic is boilerplate, either calling into generic phylib helpers (for
> > SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> > bit of validation :
> >  - Getting the module's expected interface mode
> >  - Making sure the PHY supports it
> >  - Optionnaly perform some configuration to make sure the PHY outputs
> >    the right mode
> > 
> > This can be made more generic by leveraging the phy_port, and its
> > configure_mii() callback which allows setting a port's interfaces when
> > the port is a serdes.
> > 
> > Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> > bus itself, but an SFP phandle is found in devicetree/firmware, then the
> > generic PHY SFP support will be used, relying on port ops.
> > 
> > PHY driver need to :
> >  - Register a .attach_port() callback
> >  - When a serdes port is registered to the PHY, drivers must set
> >    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
> >  - If the port has limitations regarding speed, duplex and aneg, the
> >    port can also fine-tune the final linkmodes that can be supported
> >  - The port may register a set of ops, including .configure_mii(), that
> >    will be called at module_insert time to adjust the interface based on
> >    the module detected.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
> >  include/linux/phy.h          |   2 +
> >  2 files changed, 109 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index aaf0eccbefba..aca3a47cbb66 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct sfp_bus
> > *bus) }
> >  EXPORT_SYMBOL(phy_sfp_detach);
> > 
> > +static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id
> > *id) +{
> > +	struct phy_device *phydev = upstream;
> > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > +  
> 
> RCT

Can't be done here, it won't build if in the other order...

> 
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > +	phy_interface_t iface;
> > +
> > +	linkmode_zero(sfp_support);
> > +
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> > +
> > +	if (phydev->n_ports == 1)
> > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,   
> sfp_support);
> 
> As mentionned below, this check looks a bit strange to me. Why are we only 
> parsing the SFP port if the PHY device only has one registered port?

Because phydev->port is global to the PHY. If we have another port,
then phydev->port must be handled differently so that SFP insertion /
removal doesn't overwrite what the other port is.

Handling of phydev->port is still fragile in this state of the series,
I'll try to improve on that for V7 and document it better.

> > +
> > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > +
> > +	if (linkmode_empty(sfp_support)) {
> > +		dev_err(&phydev->mdio.dev, "incompatible SFP module   
> inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > +
> > +	/* Check that this interface is supported */
> > +	if (!test_bit(iface, port->interfaces)) {
> > +		dev_err(&phydev->mdio.dev, "incompatible SFP module   
> inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (port->ops && port->ops->configure_mii)
> > +		return port->ops->configure_mii(port, true, iface);  
> 
> The name "configure_mii()" seems a bit narrow-scoped to me, as this callback 
> might have to configure something else than a MII link. For example, if a DAC 
> SFP module is inserted, the downstream side of the transciever will have to be 
> configured to 1000Base-X or something similar.

In that regard, you can consider 1000BaseX as a MII mode (we do have
PHY_INTERFACE_MODE_1000BASEX).

> I'd suggest something like "post_sfp_insert()", please let me know what you 
> think.

That's not intended to be SFP-specific though. post_sfp_insert() sounds
lke the narrow-scoped name to me :) Here we are dealing with a PHy that
has a media-side port that isn't a MDI port, but an MII interface like
a MAC would usually export. There may be an SFP here, or something else
entirely :)

One thing though is that this series uses a mix of "is_serdes" and
"configure_mii" to mean pretty-much the same thing, I'll make the names
a bit more homogenous.

> > +
> > +	return 0;
> > +}
> > +
> > +static void phy_sfp_module_remove(void *upstream)
> > +{
> > +	struct phy_device *phydev = upstream;
> > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > +
> > +	if (port && port->ops && port->ops->configure_mii)
> > +		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
> > +
> > +	if (phydev->n_ports == 1)
> > +		phydev->port = PORT_NONE;  
> 
> This check is a bit confusing to me. Could you please explain why you're only 
> setting the phydev's SFP port to PORT_NONE if the PHY device only has one 
> registered port? Shouldn't this be done regardless?

So that we don't overwrite what the other port would have set :) but,
that's a bit fragile as I said and probably not correct anyways, let me
double-check that.

Maxime

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois]

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

On Friday, 23 May 2025 14:54:57 CEST Maxime Chevallier wrote:
> Hi Romain,
> 
> On Mon, 12 May 2025 10:38:52 +0200
> 
> Romain Gantois <romain.gantois@bootlin.com> wrote:
> > Hi Maxime,
> > 
> > On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:
> > > There are currently 4 PHY drivers that can drive downstream SFPs:
> > > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > > logic is boilerplate, either calling into generic phylib helpers (for
> > > SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> > > 
> > > bit of validation :
> > >  - Getting the module's expected interface mode
> > >  - Making sure the PHY supports it
> > >  - Optionnaly perform some configuration to make sure the PHY outputs
> > >  
> > >    the right mode
> > > 
> > > This can be made more generic by leveraging the phy_port, and its
> > > configure_mii() callback which allows setting a port's interfaces when
> > > the port is a serdes.
> > > 
> > > Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> > > bus itself, but an SFP phandle is found in devicetree/firmware, then the
> > > generic PHY SFP support will be used, relying on port ops.
> > > 
> > > PHY driver need to :
> > >  - Register a .attach_port() callback
> > >  - When a serdes port is registered to the PHY, drivers must set
> > >  
> > >    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
> > >  
> > >  - If the port has limitations regarding speed, duplex and aneg, the
> > >  
> > >    port can also fine-tune the final linkmodes that can be supported
> > >  
> > >  - The port may register a set of ops, including .configure_mii(), that
> > >  
> > >    will be called at module_insert time to adjust the interface based on
> > >    the module detected.
> > > 
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > > 
> > >  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
> > >  include/linux/phy.h          |   2 +
> > >  2 files changed, 109 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index aaf0eccbefba..aca3a47cbb66 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct
> > > sfp_bus
> > > *bus) }
> > > 
> > >  EXPORT_SYMBOL(phy_sfp_detach);
> > > 
> > > +static int phy_sfp_module_insert(void *upstream, const struct
> > > sfp_eeprom_id *id) +{
> > > +	struct phy_device *phydev = upstream;
> > > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > > +
> > 
> > RCT
> 
> Can't be done here, it won't build if in the other order...
> 

You could always separate the declaration from the assignment, I've seen that 
done quite a lot to keep things in RCT.

> > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > > +	phy_interface_t iface;
> > > +
> > > +	linkmode_zero(sfp_support);
> > > +
> > > +	if (!port)
> > > +		return -EINVAL;
> > > +
> > > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> > > +
> > > +	if (phydev->n_ports == 1)
> > > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,
> > 
> > sfp_support);
> > 
> > As mentionned below, this check looks a bit strange to me. Why are we only
> > parsing the SFP port if the PHY device only has one registered port?
> 
> Because phydev->port is global to the PHY. If we have another port,
> then phydev->port must be handled differently so that SFP insertion /
> removal doesn't overwrite what the other port is.
> 

Okay, I see, thanks for explaining.

> Handling of phydev->port is still fragile in this state of the series,
> I'll try to improve on that for V7 and document it better.
> 
> > > +
> > > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > > +
> > > +	if (linkmode_empty(sfp_support)) {
> > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module
> > 
> > inserted\n");
> > 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > > +
> > > +	/* Check that this interface is supported */
> > > +	if (!test_bit(iface, port->interfaces)) {
> > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module
> > 
> > inserted\n");
> > 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (port->ops && port->ops->configure_mii)
> > > +		return port->ops->configure_mii(port, true, iface);
> > 
> > The name "configure_mii()" seems a bit narrow-scoped to me, as this
> > callback might have to configure something else than a MII link. For
> > example, if a DAC SFP module is inserted, the downstream side of the
> > transciever will have to be configured to 1000Base-X or something
> > similar.
> 
> In that regard, you can consider 1000BaseX as a MII mode (we do have
> PHY_INTERFACE_MODE_1000BASEX).
> 

Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes you're 
right.

> > I'd suggest something like "post_sfp_insert()", please let me know what
> > you
> > think.
> 
> That's not intended to be SFP-specific though. post_sfp_insert() sounds
> lke the narrow-scoped name to me :) Here we are dealing with a PHy that
> has a media-side port that isn't a MDI port, but an MII interface like
> a MAC would usually export. There may be an SFP here, or something else
> entirely :)
> 

Is that callback really not meant to be SFP-specific? It's only called from 
phy_sfp_module_insert() though.

> One thing though is that this series uses a mix of "is_serdes" and
> "configure_mii" to mean pretty-much the same thing, I'll make the names
> a bit more homogenous.
> 

Sure, sounds good.

> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void phy_sfp_module_remove(void *upstream)
> > > +{
> > > +	struct phy_device *phydev = upstream;
> > > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > > +
> > > +	if (port && port->ops && port->ops->configure_mii)
> > > +		port->ops->configure_mii(port, false, PHY_INTERFACE_MODE_NA);
> > > +
> > > +	if (phydev->n_ports == 1)
> > > +		phydev->port = PORT_NONE;
> > 
> > This check is a bit confusing to me. Could you please explain why you're
> > only setting the phydev's SFP port to PORT_NONE if the PHY device only
> > has one registered port? Shouldn't this be done regardless?
> 
> So that we don't overwrite what the other port would have set :) but,
> that's a bit fragile as I said and probably not correct anyways, let me
> double-check that.
> 

All right, that makes sense given what you've already told me.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Maxime Chevallier]

Hi Romain

On Wed, 28 May 2025 09:35:35 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> On Friday, 23 May 2025 14:54:57 CEST Maxime Chevallier wrote:
> > Hi Romain,
> > 
> > On Mon, 12 May 2025 10:38:52 +0200
> > 
> > Romain Gantois <romain.gantois@bootlin.com> wrote:  
> > > Hi Maxime,
> > > 
> > > On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:  
> > > > There are currently 4 PHY drivers that can drive downstream SFPs:
> > > > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > > > logic is boilerplate, either calling into generic phylib helpers (for
> > > > SFP PHY attach, bus attach, etc.) or performing the same tasks with a
> > > > 
> > > > bit of validation :
> > > >  - Getting the module's expected interface mode
> > > >  - Making sure the PHY supports it
> > > >  - Optionnaly perform some configuration to make sure the PHY outputs
> > > >  
> > > >    the right mode
> > > > 
> > > > This can be made more generic by leveraging the phy_port, and its
> > > > configure_mii() callback which allows setting a port's interfaces when
> > > > the port is a serdes.
> > > > 
> > > > Introduce a generic PHY SFP support. If a driver doesn't probe the SFP
> > > > bus itself, but an SFP phandle is found in devicetree/firmware, then the
> > > > generic PHY SFP support will be used, relying on port ops.
> > > > 
> > > > PHY driver need to :
> > > >  - Register a .attach_port() callback
> > > >  - When a serdes port is registered to the PHY, drivers must set
> > > >  
> > > >    port->interfaces to the set of PHY_INTERFACE_MODE the port can output
> > > >  
> > > >  - If the port has limitations regarding speed, duplex and aneg, the
> > > >  
> > > >    port can also fine-tune the final linkmodes that can be supported
> > > >  
> > > >  - The port may register a set of ops, including .configure_mii(), that
> > > >  
> > > >    will be called at module_insert time to adjust the interface based on
> > > >    the module detected.
> > > > 
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > > 
> > > >  drivers/net/phy/phy_device.c | 107 +++++++++++++++++++++++++++++++++++
> > > >  include/linux/phy.h          |   2 +
> > > >  2 files changed, 109 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index aaf0eccbefba..aca3a47cbb66 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct
> > > > sfp_bus
> > > > *bus) }
> > > > 
> > > >  EXPORT_SYMBOL(phy_sfp_detach);
> > > > 
> > > > +static int phy_sfp_module_insert(void *upstream, const struct
> > > > sfp_eeprom_id *id) +{
> > > > +	struct phy_device *phydev = upstream;
> > > > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > > > +  
> > > 
> > > RCT  
> > 
> > Can't be done here, it won't build if in the other order...
> >   
> 
> You could always separate the declaration from the assignment, I've seen that 
> done quite a lot to keep things in RCT.
> 
> > > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > > > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > > > +	phy_interface_t iface;
> > > > +
> > > > +	linkmode_zero(sfp_support);
> > > > +
> > > > +	if (!port)
> > > > +		return -EINVAL;
> > > > +
> > > > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, interfaces);
> > > > +
> > > > +	if (phydev->n_ports == 1)
> > > > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,  
> > > 
> > > sfp_support);
> > > 
> > > As mentionned below, this check looks a bit strange to me. Why are we only
> > > parsing the SFP port if the PHY device only has one registered port?  
> > 
> > Because phydev->port is global to the PHY. If we have another port,
> > then phydev->port must be handled differently so that SFP insertion /
> > removal doesn't overwrite what the other port is.
> >   
> 
> Okay, I see, thanks for explaining.
> 
> > Handling of phydev->port is still fragile in this state of the series,
> > I'll try to improve on that for V7 and document it better.
> >   
> > > > +
> > > > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > > > +
> > > > +	if (linkmode_empty(sfp_support)) {
> > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module  
> > > 
> > > inserted\n");
> > >   
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > > > +
> > > > +	/* Check that this interface is supported */
> > > > +	if (!test_bit(iface, port->interfaces)) {
> > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module  
> > > 
> > > inserted\n");
> > >   
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (port->ops && port->ops->configure_mii)
> > > > +		return port->ops->configure_mii(port, true, iface);  
> > > 
> > > The name "configure_mii()" seems a bit narrow-scoped to me, as this
> > > callback might have to configure something else than a MII link. For
> > > example, if a DAC SFP module is inserted, the downstream side of the
> > > transciever will have to be configured to 1000Base-X or something
> > > similar.  
> > 
> > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > PHY_INTERFACE_MODE_1000BASEX).
> >   
> 
> Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes you're 
> right.
> 
> > > I'd suggest something like "post_sfp_insert()", please let me know what
> > > you
> > > think.  
> > 
> > That's not intended to be SFP-specific though. post_sfp_insert() sounds
> > lke the narrow-scoped name to me :) Here we are dealing with a PHy that
> > has a media-side port that isn't a MDI port, but an MII interface like
> > a MAC would usually export. There may be an SFP here, or something else
> > entirely :)
> >   
> 
> Is that callback really not meant to be SFP-specific? It's only called from 
> phy_sfp_module_insert() though.

Well for now yeah as this is the only user now, but ports are meant to
be about more than drving SFPs.

This series as of today is quite long but doesn't cover the other
classes of use-cases which are non-phy-driven ports.

Taking the example of the Turris Omnia, we have something like that :

      +------------------+ 
      | MUX  +--------+  |
      |    +-| port 1 | --- SGMII - PHY
      |    | +--------+  |
MAC -------+             |
      |    | +--------+  |
      |    +-| port 2 | ---- SGMII/1000BaseX - SFP
      |      +--------+  |
      +------------------+

Here we have 1 mac that drives 2 ports through a MUX. So the ports here
would be driven by the mux driver (which I have a framewrok for ready to
send once this series lands). The goal would be to use the same
phy_port representation for these 2 ports, the .configure_mii()
callback will be used to switch between ports (so, perform the muxing),
then the rest of the stack takes over through the usual means (phylink,
phylib and all that). So here, the .configure_mii() ops doesn't
necessarily drives an SFP :)

Maxime

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois]

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

On Wednesday, 28 May 2025 10:14:33 CEST Maxime Chevallier wrote:
> Hi Romain
> 
> On Wed, 28 May 2025 09:35:35 +0200
> 
> Romain Gantois <romain.gantois@bootlin.com> wrote:
> > On Friday, 23 May 2025 14:54:57 CEST Maxime Chevallier wrote:
> > > Hi Romain,
> > > 
> > > On Mon, 12 May 2025 10:38:52 +0200
> > > 
> > > Romain Gantois <romain.gantois@bootlin.com> wrote:
> > > > Hi Maxime,
> > > > 
> > > > On Wednesday, 7 May 2025 15:53:22 CEST Maxime Chevallier wrote:
> > > > > There are currently 4 PHY drivers that can drive downstream SFPs:
> > > > > marvell.c, marvell10g.c, at803x.c and marvell-88x2222.c. Most of the
> > > > > logic is boilerplate, either calling into generic phylib helpers
> > > > > (for
> > > > > SFP PHY attach, bus attach, etc.) or performing the same tasks with
> > > > > a
> > > > > 
> > > > > bit of validation :
> > > > >  - Getting the module's expected interface mode
> > > > >  - Making sure the PHY supports it
> > > > >  - Optionnaly perform some configuration to make sure the PHY
> > > > >  outputs
> > > > >  
> > > > >    the right mode
> > > > > 
> > > > > This can be made more generic by leveraging the phy_port, and its
> > > > > configure_mii() callback which allows setting a port's interfaces
> > > > > when
> > > > > the port is a serdes.
> > > > > 
> > > > > Introduce a generic PHY SFP support. If a driver doesn't probe the
> > > > > SFP
> > > > > bus itself, but an SFP phandle is found in devicetree/firmware, then
> > > > > the
> > > > > generic PHY SFP support will be used, relying on port ops.
> > > > > 
> > > > > PHY driver need to :
> > > > >  - Register a .attach_port() callback
> > > > >  - When a serdes port is registered to the PHY, drivers must set
> > > > >  
> > > > >    port->interfaces to the set of PHY_INTERFACE_MODE the port can
> > > > >    output
> > > > >  
> > > > >  - If the port has limitations regarding speed, duplex and aneg, the
> > > > >  
> > > > >    port can also fine-tune the final linkmodes that can be supported
> > > > >  
> > > > >  - The port may register a set of ops, including .configure_mii(),
> > > > >  that
> > > > >  
> > > > >    will be called at module_insert time to adjust the interface
> > > > >    based on
> > > > >    the module detected.
> > > > > 
> > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > ---
> > > > > 
> > > > >  drivers/net/phy/phy_device.c | 107
> > > > >  +++++++++++++++++++++++++++++++++++
> > > > >  include/linux/phy.h          |   2 +
> > > > >  2 files changed, 109 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/phy/phy_device.c
> > > > > b/drivers/net/phy/phy_device.c
> > > > > index aaf0eccbefba..aca3a47cbb66 100644
> > > > > --- a/drivers/net/phy/phy_device.c
> > > > > +++ b/drivers/net/phy/phy_device.c
> > > > > @@ -1450,6 +1450,87 @@ void phy_sfp_detach(void *upstream, struct
> > > > > sfp_bus
> > > > > *bus) }
> > > > > 
> > > > >  EXPORT_SYMBOL(phy_sfp_detach);
> > > > > 
> > > > > +static int phy_sfp_module_insert(void *upstream, const struct
> > > > > sfp_eeprom_id *id) +{
> > > > > +	struct phy_device *phydev = upstream;
> > > > > +	struct phy_port *port = phy_get_sfp_port(phydev);
> > > > > +
> > > > 
> > > > RCT
> > > 
> > > Can't be done here, it won't build if in the other order...
> > 
> > You could always separate the declaration from the assignment, I've seen
> > that done quite a lot to keep things in RCT.
> > 
> > > > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> > > > > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > > > > +	phy_interface_t iface;
> > > > > +
> > > > > +	linkmode_zero(sfp_support);
> > > > > +
> > > > > +	if (!port)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	sfp_parse_support(phydev->sfp_bus, id, sfp_support, 
interfaces);
> > > > > +
> > > > > +	if (phydev->n_ports == 1)
> > > > > +		phydev->port = sfp_parse_port(phydev->sfp_bus, id,
> > > > 
> > > > sfp_support);
> > > > 
> > > > As mentionned below, this check looks a bit strange to me. Why are we
> > > > only
> > > > parsing the SFP port if the PHY device only has one registered port?
> > > 
> > > Because phydev->port is global to the PHY. If we have another port,
> > > then phydev->port must be handled differently so that SFP insertion /
> > > removal doesn't overwrite what the other port is.
> > 
> > Okay, I see, thanks for explaining.
> > 
> > > Handling of phydev->port is still fragile in this state of the series,
> > > I'll try to improve on that for V7 and document it better.
> > > 
> > > > > +
> > > > > +	linkmode_and(sfp_support, port->supported, sfp_support);
> > > > > +
> > > > > +	if (linkmode_empty(sfp_support)) {
> > > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module
> > > > 
> > > > inserted\n");
> > > > 
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	iface = sfp_select_interface(phydev->sfp_bus, sfp_support);
> > > > > +
> > > > > +	/* Check that this interface is supported */
> > > > > +	if (!test_bit(iface, port->interfaces)) {
> > > > > +		dev_err(&phydev->mdio.dev, "incompatible SFP module
> > > > 
> > > > inserted\n");
> > > > 
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	if (port->ops && port->ops->configure_mii)
> > > > > +		return port->ops->configure_mii(port, true, iface);
> > > > 
> > > > The name "configure_mii()" seems a bit narrow-scoped to me, as this
> > > > callback might have to configure something else than a MII link. For
> > > > example, if a DAC SFP module is inserted, the downstream side of the
> > > > transciever will have to be configured to 1000Base-X or something
> > > > similar.
> > > 
> > > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > > PHY_INTERFACE_MODE_1000BASEX).
> > 
> > Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes
> > you're
> > right.
> > 
> > > > I'd suggest something like "post_sfp_insert()", please let me know
> > > > what
> > > > you
> > > > think.
> > > 
> > > That's not intended to be SFP-specific though. post_sfp_insert() sounds
> > > lke the narrow-scoped name to me :) Here we are dealing with a PHy that
> > > has a media-side port that isn't a MDI port, but an MII interface like
> > > a MAC would usually export. There may be an SFP here, or something else
> > > entirely :)
> > 
> > Is that callback really not meant to be SFP-specific? It's only called
> > from
> > phy_sfp_module_insert() though.
> 
> Well for now yeah as this is the only user now, but ports are meant to
> be about more than drving SFPs.
> 
> This series as of today is quite long but doesn't cover the other
> classes of use-cases which are non-phy-driven ports.
> 
> Taking the example of the Turris Omnia, we have something like that :
> 
>       +------------------+
> 
>       | MUX  +--------+  |
>       | 
>       |    +-| port 1 | --- SGMII - PHY
>       |    
>       |    | +--------+  |
> 
> MAC -------+             |
> 
>       |    | +--------+  |
>       |    
>       |    +-| port 2 | ---- SGMII/1000BaseX - SFP
>       |    
>       |      +--------+  |
> 
>       +------------------+
> 
> Here we have 1 mac that drives 2 ports through a MUX. So the ports here
> would be driven by the mux driver (which I have a framewrok for ready to
> send once this series lands). The goal would be to use the same
> phy_port representation for these 2 ports, the .configure_mii()
> callback will be used to switch between ports (so, perform the muxing),
> then the rest of the stack takes over through the usual means (phylink,
> phylib and all that). So here, the .configure_mii() ops doesn't
> necessarily drives an SFP :)

I see, thanks again for explaining.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Russell King (Oracle)]

On Wed, May 28, 2025 at 09:35:35AM +0200, Romain Gantois wrote:
> > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > PHY_INTERFACE_MODE_1000BASEX).
> > 
> 
> Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes you're 
> right.

1000BASE-X is exactly what is described in IEEE 802.3. It's a PHY
interface mode because PHYs that use SerDes can connect to the host
using SGMII or 1000BASE-X over the serial link.

1000BASE-X's purpose in IEEE 802.3 is as a protocol for use over
fibre links, as the basis for 1000BASE-SX, 1000BASE-LX, 1000BASE-EX
etc where the S, L, E etc are all to do with the properties of the
medium that the electrical 1000BASE-X is sent over. It even includes
1000BASE-CX which is over copper cable.

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

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois]

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

On Thursday, 29 May 2025 15:23:22 CEST Russell King (Oracle) wrote:
> On Wed, May 28, 2025 at 09:35:35AM +0200, Romain Gantois wrote:
> > > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > > PHY_INTERFACE_MODE_1000BASEX).
> > 
> > Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes
> > you're
> > right.
> 
> 1000BASE-X is exactly what is described in IEEE 802.3. It's a PHY
> interface mode because PHYs that use SerDes can connect to the host
> using SGMII or 1000BASE-X over the serial link.
> 
> 1000BASE-X's purpose in IEEE 802.3 is as a protocol for use over
> fibre links, as the basis for 1000BASE-SX, 1000BASE-LX, 1000BASE-EX
> etc where the S, L, E etc are all to do with the properties of the
> medium that the electrical 1000BASE-X is sent over. It even includes
> 1000BASE-CX which is over copper cable.

Ah makes sense, thanks for the explanation. I guess my mistake was assuming 
that MAC/PHY interface modes were necessarily strictly at the reconciliation 
sublayer level, and didn't include PCS/PMA functions.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Russell King (Oracle)]

On Fri, May 30, 2025 at 09:28:11AM +0200, Romain Gantois wrote:
> On Thursday, 29 May 2025 15:23:22 CEST Russell King (Oracle) wrote:
> > On Wed, May 28, 2025 at 09:35:35AM +0200, Romain Gantois wrote:
> > > > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > > > PHY_INTERFACE_MODE_1000BASEX).
> > > 
> > > Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes
> > > you're
> > > right.
> > 
> > 1000BASE-X is exactly what is described in IEEE 802.3. It's a PHY
> > interface mode because PHYs that use SerDes can connect to the host
> > using SGMII or 1000BASE-X over the serial link.
> > 
> > 1000BASE-X's purpose in IEEE 802.3 is as a protocol for use over
> > fibre links, as the basis for 1000BASE-SX, 1000BASE-LX, 1000BASE-EX
> > etc where the S, L, E etc are all to do with the properties of the
> > medium that the electrical 1000BASE-X is sent over. It even includes
> > 1000BASE-CX which is over copper cable.
> 
> Ah makes sense, thanks for the explanation. I guess my mistake was assuming 
> that MAC/PHY interface modes were necessarily strictly at the reconciliation 
> sublayer level, and didn't include PCS/PMA functions.

When a serdes protocol such as SGMII, 1000BASE-X, or 10GBASE-R is being
used with a PHY, the IEEE 802.3 setup isn't followed exactly - in
effect there are more layers.

On the SoC:

	MAC
	Reconciliation (RS)
	PCS
	SerDes (part of the PMA layer)

On the PHY side of the SerDes host-to-phy link:

	SerDes
	PCS (which may or may not be exposed in the PHY register set,
	     and is normally managed by the PHY itself)
	(maybe other layers, could include MACs	back-to-back)
	PCS
	PMA
	PMD

Hope that helps explain what's going on a little more.

Another way to look at it is that with SGMII, 1000BASE-X etc between
the PHY and host, the PHY is a media converter.

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

Re: [PATCH net-next v6 06/14] net: phy: Introduce generic SFP handling for PHY drivers

[Romain Gantois]

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

On Friday, 30 May 2025 09:46:19 CEST Russell King (Oracle) wrote:
> On Fri, May 30, 2025 at 09:28:11AM +0200, Romain Gantois wrote:
> > On Thursday, 29 May 2025 15:23:22 CEST Russell King (Oracle) wrote:
> > > On Wed, May 28, 2025 at 09:35:35AM +0200, Romain Gantois wrote:
> > > > > In that regard, you can consider 1000BaseX as a MII mode (we do have
> > > > > PHY_INTERFACE_MODE_1000BASEX).
> > > > 
> > > > Ugh, the "1000BaseX" terminology never ceases to confuse me, but yes
> > > > you're
> > > > right.
> > > 
> > > 1000BASE-X is exactly what is described in IEEE 802.3. It's a PHY
> > > interface mode because PHYs that use SerDes can connect to the host
> > > using SGMII or 1000BASE-X over the serial link.
> > > 
> > > 1000BASE-X's purpose in IEEE 802.3 is as a protocol for use over
> > > fibre links, as the basis for 1000BASE-SX, 1000BASE-LX, 1000BASE-EX
> > > etc where the S, L, E etc are all to do with the properties of the
> > > medium that the electrical 1000BASE-X is sent over. It even includes
> > > 1000BASE-CX which is over copper cable.
> > 
> > Ah makes sense, thanks for the explanation. I guess my mistake was
> > assuming
> > that MAC/PHY interface modes were necessarily strictly at the
> > reconciliation sublayer level, and didn't include PCS/PMA functions.
> 
> When a serdes protocol such as SGMII, 1000BASE-X, or 10GBASE-R is being
> used with a PHY, the IEEE 802.3 setup isn't followed exactly - in
> effect there are more layers.
> 
> On the SoC:
> 
> 	MAC
> 	Reconciliation (RS)
> 	PCS
> 	SerDes (part of the PMA layer)
> 
> On the PHY side of the SerDes host-to-phy link:
> 
> 	SerDes
> 	PCS (which may or may not be exposed in the PHY register set,
> 	     and is normally managed by the PHY itself)
> 	(maybe other layers, could include MACs	back-to-back)
> 	PCS
> 	PMA
> 	PMD
> 
> Hope that helps explain what's going on a little more.

Definitely helps a lot, thanks.

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Maxime Chevallier]

Hi Romain,

On Mon, 12 May 2025 09:53:09 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hi Maxime,

I'm about to send a V7, but I realise now I didn't address these
reviews... sorry about that

> On Wednesday, 7 May 2025 15:53:19 CEST Maxime Chevallier wrote:
> > Ethernet provides a wide variety of layer 1 protocols and standards for
> > data transmission. The front-facing ports of an interface have their own
> > complexity and configurability.
> >   
> ...
> > +
> > +static int phy_default_setup_single_port(struct phy_device *phydev)
> > +{
> > +	struct phy_port *port = phy_port_alloc();
> > +
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->parent_type = PHY_PORT_PHY;
> > +	port->phy = phydev;
> > +	linkmode_copy(port->supported, phydev->supported);
> > +
> > +	phy_add_port(phydev, port);
> > +
> > +	/* default medium is copper */
> > +	if (!port->mediums)
> > +		port->mediums |= BIT(ETHTOOL_LINK_MEDIUM_BASET);  
> 
> Could this be moved to phy_port_alloc() instead? That way, you'd avoid the 
> extra conditional and the "default medium == baseT" rule would be enforced as 
> early as possible.

I'll actually remove that altogether. For a single-port PHY, we can
actually derive the mediums from the PHY's supported field.

[...]

> > +	for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST) {
> > +		linkmode_zero(supported);  
> 
> ethtool_medium_get_supported() can only set bits in "supported", so you could 
> just do:
> 
> ```
> for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
> 	ethtool_medium_get_supported(port->supported, i, port->lanes);
> ```
> 
> unless you're wary of someone modifying ethtool_medium_get_supported() in a 
> way that breaks this in the future?

Hmm yes, but at least here it makes it obvious we're accumulating the
linkmodes :/

> 
> > +		ethtool_medium_get_supported(supported, i, port->lanes);
> > +		linkmode_or(port->supported, port->supported, supported);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(phy_port_update_supported);
> > +
> > +/**
> > + * phy_port_get_type() - get the PORT_* attribut for that port.
> > + * @port: The port we want the information from
> > + *
> > + * Returns: A PORT_XXX value.
> > + */
> > +int phy_port_get_type(struct phy_port *port)
> > +{
> > +	if (port->mediums & ETHTOOL_LINK_MEDIUM_BASET)
> > +		return PORT_TP;
> > +
> > +	if (phy_port_is_fiber(port))
> > +		return PORT_FIBRE;
> > +
> > +	return PORT_OTHER;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_port_get_type);
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index c1d805d3e02f..0d3063af5905 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -226,6 +226,10 @@ extern const struct link_mode_info link_mode_params[];
> > 
> >  extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
> > 
> > +#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
> > +				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
> > +				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
> > +
> >  static inline const char *phy_mediums(enum ethtool_link_medium medium)
> >  {
> >  	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
> > @@ -234,6 +238,17 @@ static inline const char *phy_mediums(enum
> > ethtool_link_medium medium) return ethtool_link_medium_names[medium];
> >  }
> > 
> > +static inline int phy_medium_default_lanes(enum ethtool_link_medium medium)
> > +{
> > +	/* Let's consider that the default BaseT ethernet is BaseT4, i.e.
> > +	 * Gigabit Ethernet.
> > +	 */
> > +	if (medium == ETHTOOL_LINK_MEDIUM_BASET)
> > +		return 4;
> > +
> > +	return 1;
> > +}
> > +
> >  /* declare a link mode bitmap */
> >  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
> >  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index d62d292024bc..0180f4d4fd7d 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -299,6 +299,7 @@ static inline long rgmii_clock(int speed)
> >  struct device;
> >  struct kernel_hwtstamp_config;
> >  struct phylink;
> > +struct phy_port;
> >  struct sfp_bus;
> >  struct sfp_upstream_ops;
> >  struct sk_buff;
> > @@ -590,6 +591,9 @@ struct macsec_ops;
> >   * @master_slave_state: Current master/slave configuration
> >   * @mii_ts: Pointer to time stamper callbacks
> >   * @psec: Pointer to Power Sourcing Equipment control struct
> > + * @ports: List of PHY ports structures
> > + * @n_ports: Number of ports currently attached to the PHY
> > + * @max_n_ports: Max number of ports this PHY can expose
> >   * @lock:  Mutex for serialization access to PHY
> >   * @state_queue: Work queue for state machine
> >   * @link_down_events: Number of times link was lost
> > @@ -724,6 +728,10 @@ struct phy_device {
> >  	struct mii_timestamper *mii_ts;
> >  	struct pse_control *psec;
> > 
> > +	struct list_head ports;
> > +	int n_ports;
> > +	int max_n_ports;
> > +
> >  	u8 mdix;
> >  	u8 mdix_ctrl;
> > 
> > @@ -1242,6 +1250,27 @@ struct phy_driver {
> >  	 * Returns the time in jiffies until the next update event.
> >  	 */
> >  	unsigned int (*get_next_update_time)(struct phy_device *dev);
> > +
> > +	/**
> > +	 * @attach_port: Indicates to the PHY driver that a port is detected
> > +	 * @dev: PHY device to notify
> > +	 * @port: The port being added
> > +	 *
> > +	 * Called when a port that needs to be driven by the PHY is found. The
> > +	 * number of time this will be called depends on phydev->max_n_ports,
> > +	 * which the driver can change in .probe().
> > +	 *
> > +	 * The port that is being passed may or may not be initialized. If it   
> is
> > +	 * already initialized, it is by the generic port representation from
> > +	 * devicetree, which superseeds any strapping or vendor-specific
> > +	 * properties.
> > +	 *
> > +	 * If the port isn't initialized, the port->mediums and port->lanes
> > +	 * fields must be set, possibly according to stapping information.
> > +	 *
> > +	 * Returns 0, or an error code.
> > +	 */
> > +	int (*attach_port)(struct phy_device *dev, struct phy_port *port);
> >  };
> >  #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		  
> \
> >  				      struct phy_driver, mdiodrv)
> > @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
> >  void phy_mac_interrupt(struct phy_device *phydev);
> >  void phy_start_machine(struct phy_device *phydev);
> >  void phy_stop_machine(struct phy_device *phydev);
> > +
> >  void phy_ethtool_ksettings_get(struct phy_device *phydev,
> >  			       struct ethtool_link_ksettings *cmd);
> >  int phy_ethtool_ksettings_set(struct phy_device *phydev,
> > diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
> > new file mode 100644
> > index 000000000000..70aa75f93096
> > --- /dev/null
> > +++ b/include/linux/phy_port.h
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef __PHY_PORT_H
> > +#define __PHY_PORT_H
> > +
> > +#include <linux/ethtool.h>
> > +#include <linux/types.h>
> > +#include <linux/phy.h>
> > +
> > +struct phy_port;
> > +
> > +/**
> > + * enum phy_port_parent - The device this port is attached to
> > + *
> > + * @PHY_PORT_PHY: Indicates that the port is driven by a PHY device
> > + */
> > +enum phy_port_parent {
> > +	PHY_PORT_PHY,
> > +};
> > +
> > +struct phy_port_ops {
> > +	/* Sometimes, the link state can be retrieved from physical,
> > +	 * out-of-band channels such as the LOS signal on SFP. These
> > +	 * callbacks allows notifying the port about state changes
> > +	 */
> > +	void (*link_up)(struct phy_port *port);
> > +	void (*link_down)(struct phy_port *port);
> > +
> > +	/* If the port acts as a Media Independent Interface (Serdes port),
> > +	 * configures the port with the relevant state and mode. When enable is
> > +	 * not set, interface should be ignored
> > +	 */
> > +	int (*configure_mii)(struct phy_port *port, bool enable,   
> phy_interface_t
> > interface); +};
> > +
> > +/**
> > + * struct phy_port - A representation of a network device physical
> > interface + *
> > + * @head: Used by the port's parent to list ports
> > + * @parent_type: The type of device this port is directly connected to
> > + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> > + * @ops: Callback ops implemented by the port controller
> > + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> > applicable + * @mediums: Bitmask of the physical mediums this port provides
> > access to + * @supported: The link modes this port can expose, if this port
> > is MDI (not MII) + * @interfaces: The MII interfaces this port supports, if
> > this port is MII + * @active: Indicates if the port is currently part of
> > the active link. + * @is_serdes: Indicates if this port is Serialised MII
> > (Media Independent + *	       Interface), or an MDI (Media Dependent
> > Interface).
> > + */
> > +struct phy_port {
> > +	struct list_head head;
> > +	enum phy_port_parent parent_type;
> > +	union {
> > +		struct phy_device *phy;
> > +	};
> > +
> > +	const struct phy_port_ops *ops;
> > +
> > +	int lanes;
> > +	unsigned long mediums;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > +
> > +	unsigned int active:1;
> > +	unsigned int is_serdes:1;
> > +};
> > +
> > +struct phy_port *phy_port_alloc(void);
> > +void phy_port_destroy(struct phy_port *port);
> > +
> > +static inline struct phy_device *port_phydev(struct phy_port *port)
> > +{
> > +	return port->phy;
> > +}
> > +
> > +struct phy_port *phy_of_parse_port(struct device_node *dn);
> > +
> > +static inline bool phy_port_is_copper(struct phy_port *port)
> > +{
> > +	return port->mediums == BIT(ETHTOOL_LINK_MEDIUM_BASET);  
> 
> BaseC is also "copper" right? Maybe this should be renamed to 
> "phy_port_is_tp"?

Yeah indeed...

Maxime

> 

Re: [PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Maxime Chevallier]

Hi Kory,

On Tue, 13 May 2025 15:53:25 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> On Wed,  7 May 2025 15:53:19 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
> > Ethernet provides a wide variety of layer 1 protocols and standards for
> > data transmission. The front-facing ports of an interface have their own
> > complexity and configurability.
> > 
> > Introduce a representation of these front-facing ports. The current code
> > is minimalistic and only support ports controlled by PHY devices, but
> > the plan is to extend that to SFP as well as raw Ethernet MACs that
> > don't use PHY devices.
> > 
> > This minimal port representation allows describing the media and number
> > of lanes of a port. From that information, we can derive the linkmodes
> > usable on the port, which can be used to limit the capabilities of an
> > interface.
> > 
> > For now, the port lanes and medium is derived from devicetree, defined
> > by the PHY driver, or populated with default values (as we assume that
> > all PHYs expose at least one port).
> > 
> > The typical example is 100M ethernet. 100BaseT can work using only 2
> > lanes on a Cat 5 cables. However, in the situation where a 10/100/1000
> > capable PHY is wired to its RJ45 port through 2 lanes only, we have no
> > way of detecting that. The "max-speed" DT property can be used, but a
> > more accurate representation can be used :
> > 
> > mdi {
> > 	connector-0 {
> > 		media = "BaseT";
> > 		lanes = <2>;
> > 	};
> > };
> > 
> > From that information, we can derive the max speed reachable on the
> > port.
> > 
> > Another benefit of having that is to avoid vendor-specific DT properties
> > (micrel,fiber-mode or ti,fiber-mode).
> > 
> > This basic representation is meant to be expanded, by the introduction
> > of port ops, userspace listing of ports, and support for multi-port
> > devices.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
> 
> ...
> 
> > +	for_each_available_child_of_node_scoped(mdi, port_node) {
> > +		port = phy_of_parse_port(port_node);
> > +		if (IS_ERR(port)) {
> > +			err = PTR_ERR(port);
> > +			goto out_err;
> > +		}
> > +
> > +		port->parent_type = PHY_PORT_PHY;
> > +		port->phy = phydev;
> > +		err = phy_add_port(phydev, port);
> > +		if (err)
> > +			goto out_err;  
> 
> I think of_node_put(port_node) is missing here.

I don't think so, this is the _scoped variant so it takes care of
that for us.

> 
> ...
> 
> > @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
> >  void phy_mac_interrupt(struct phy_device *phydev);
> >  void phy_start_machine(struct phy_device *phydev);
> >  void phy_stop_machine(struct phy_device *phydev);
> > +  
> 
> New empty line here?

Oops, this will be removed

> > +/**
> > + * struct phy_port - A representation of a network device physical interface
> > + *
> > + * @head: Used by the port's parent to list ports
> > + * @parent_type: The type of device this port is directly connected to
> > + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> > + * @ops: Callback ops implemented by the port controller
> > + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> > applicable
> > + * @mediums: Bitmask of the physical mediums this port provides access to
> > + * @supported: The link modes this port can expose, if this port is MDI (not
> > MII)
> > + * @interfaces: The MII interfaces this port supports, if this port is MII
> > + * @active: Indicates if the port is currently part of the active link.
> > + * @is_serdes: Indicates if this port is Serialised MII (Media Independent
> > + *	       Interface), or an MDI (Media Dependent Interface).
> > + */
> > +struct phy_port {
> > +	struct list_head head;
> > +	enum phy_port_parent parent_type;
> > +	union {
> > +		struct phy_device *phy;
> > +	};  
> 
> The union is useless here?

For now yes :( But this will change when adding support for
non-phy-driver ports.

Maxime

> 
> Regards,

Re: [PATCH net-next v6 05/14] net: phy: Create a phy_port for PHY-driven SFPs

[Maxime Chevallier]

Hi Romain,

On Tue, 13 May 2025 14:25:18 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hi Maxime,
> 
> On Wednesday, 7 May 2025 15:53:21 CEST Maxime Chevallier wrote:
> > Some PHY devices may be used as media-converters to drive SFP ports (for
> > example, to allow using SFP when the SoC can only output RGMII). This is
> > already supported to some extend by allowing PHY drivers to registers
> > themselves as being SFP upstream.
> >   
> ...
> >   *
> > @@ -149,6 +151,21 @@ void phy_port_update_supported(struct phy_port *port)
> >  		ethtool_medium_get_supported(supported, i, port->lanes);
> >  		linkmode_or(port->supported, port->supported, supported);
> >  	}
> > +
> > +	/* Serdes ports supported may through SFP may not have any medium set,
> > +	 * as they will output PHY_INTERFACE_MODE_XXX modes. In that case,   
> derive
> > +	 * the supported list based on these interfaces
> > +	 */
> > +	if (port->is_serdes && linkmode_empty(supported)) {  
> 
> The "supported" bitmap needs to be zeroed out before this check. If the port 
> has no mediums, then the bitmap won't be initialized at this point.

Ah true, thanks !

Maxime