[PATCH net-next v2 0/8] Extend `phy-mode` to string array

[PATCH net-next v2 0/8] Extend `phy-mode` to string array

[Marek Behún]

Hello,

this is v2 of series that extends the `phy-connection-type` /
`phy-mode` property to be an array of strings, instead of just one
string, and makes the corresponding changes to code. It then uses
this changes to make marvell10g PHY driver choose the best MACTYPE
according to which phy-modes are supported by the MAC, the PHY and
the board.

Conventionaly the `phy-mode` means "this is the mode I want the PHY to
operate in". But we now have some PHYs that may need to change the PHY
mode during operation (marvell10g PHY driver), and so we need to know
all the supported modes. Russell King is working on MAC and PHY drivers
to inform phylink on which PHY interface modes they support, but it is
not enough, because even if a MAC/PHY driver fills all the modes
supported by the driver, still each individual board may have only some
of these modes actually wired.

This series
- changes the type of the `phy-mode` property to be an array of PHY
  interface strings,
- updated documentation of of_get_phy_mode() and related to inform that
  only first mode is returned by these functions (since this function
  is needed to still support conventional usage of the `phy-mode`
  property),
- adds fwnode_get_phy_modes() function which reads the `phy-mode` array
  and fills bitmap with mentioned modes,
- adds code to phylink to intersect the supported interfaces bitmap
  supplied by MAC driver, with interface modes defined in device-tree
  (and keeps backwards compatibility with conventional usage of the
   phy-mode property, for more information read the commit message of
   patch 4/8),
- passes supported interfaces to PHY driver so that it may configure
  a PHY to a specific mode given these interfaces,
- uses this information in marvell10g driver.

Changes since v1:
- added Reviewed-by tags
- added 10gbase-r example scenario to commit message of patch 4
- changed phylink_update_phy_modes() so that if supported_interfaces is
  empty (an unconverted driver that doesn't fill up this member), we
  leave it empty
- rewritten phylink_update_phy_modes() according to Sean Anderson's
  comment: use phy_interface_and/or() instead of several
  if (test_bit) set_bit
- added more explanation to commit message of patch 8, as per Vladimir
  Oltean's suggestion

Changes since RFC:
- update also description of the `phy-connection-type` property

Marek Behún (7):
  dt-bindings: ethernet-controller: support multiple PHY connection
    types
  net: Update documentation for *_get_phy_mode() functions
  device property: add helper function for getting phy mode bitmap
  net: phylink: update supported_interfaces with modes from fwnode
  net: phylink: pass supported PHY interface modes to phylib
  net: phy: marvell10g: Use generic macro for supported interfaces
  net: phy: marvell10g: Use tabs instead of spaces for indentation

Russell King (1):
  net: phy: marvell10g: select host interface configuration

 .../bindings/net/ethernet-controller.yaml     |  94 ++++++------
 drivers/base/property.c                       |  48 +++++-
 drivers/net/phy/marvell10g.c                  | 140 ++++++++++++++++--
 drivers/net/phy/phylink.c                     |  98 ++++++++++++
 include/linux/phy.h                           |  10 ++
 include/linux/property.h                      |   3 +
 net/core/of_net.c                             |   9 +-
 7 files changed, 332 insertions(+), 70 deletions(-)

-- 
2.32.0

[PATCH net-next v2 1/8] dt-bindings: ethernet-controller: support multiple PHY connection types

[Marek Behún]

Sometimes, an ethernet PHY may communicate with ethernet controller with
multiple different PHY connection types, and the software should be able
to choose between them.

Russell King says:
  conventionally phy-mode has meant "this is the mode we want to operate
  the PHY interface in" which was fine when PHYs didn't change their
  mode depending on the media speed
This is no longer the case, since we have PHYs that can change PHY mode.

Existing example is the Marvell 88X3310 PHY, which supports connecting
the MAC with the PHY with `xaui` and `rxaui`. The MAC may also support
both modes, but it is possible that a particular board doesn't have
these modes wired (since they use multiple SerDes lanes).

Another example is one SerDes lane capable of `1000base-x`, `2500base-x`
and `sgmii` when connecting Marvell switches with Marvell ethernet
controller. Currently we mention only one of these modes in device-tree,
and software assumes the other modes are also supported, since they use
the same SerDes lanes. But a board may be able to support `1000base-x`
and not support `2500base-x`, for example due to the higher frequency
not working correctly on a particular board.

In order for the kernel to know which modes are supported on the board,
we need to be able to specify them all in the device-tree.

Change the type of property `phy-connection-type` of an ethernet
controller to be an array of the enumerated strings, instead of just one
string. Require at least one item defined.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Cc: devicetree@vger.kernel.org
---
 .../bindings/net/ethernet-controller.yaml     | 94 ++++++++++---------
 1 file changed, 49 insertions(+), 45 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index b0933a8c295a..1fd27d45d136 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -54,51 +54,55 @@ properties:
 
   phy-connection-type:
     description:
-      Specifies interface type between the Ethernet device and a physical
-      layer (PHY) device.
-    enum:
-      # There is not a standard bus between the MAC and the PHY,
-      # something proprietary is being used to embed the PHY in the
-      # MAC.
-      - internal
-      - mii
-      - gmii
-      - sgmii
-      - qsgmii
-      - tbi
-      - rev-mii
-      - rmii
-      - rev-rmii
-
-      # RX and TX delays are added by the MAC when required
-      - rgmii
-
-      # RGMII with internal RX and TX delays provided by the PHY,
-      # the MAC should not add the RX or TX delays in this case
-      - rgmii-id
-
-      # RGMII with internal RX delay provided by the PHY, the MAC
-      # should not add an RX delay in this case
-      - rgmii-rxid
-
-      # RGMII with internal TX delay provided by the PHY, the MAC
-      # should not add an TX delay in this case
-      - rgmii-txid
-      - rtbi
-      - smii
-      - xgmii
-      - trgmii
-      - 1000base-x
-      - 2500base-x
-      - 5gbase-r
-      - rxaui
-      - xaui
-
-      # 10GBASE-KR, XFI, SFI
-      - 10gbase-kr
-      - usxgmii
-      - 10gbase-r
-      - 25gbase-r
+      Specifies interface types between the Ethernet device and a physical
+      layer (PHY) device. Since more interface types can be wired between
+      the MAC and the PHY, this property should list all that are supported
+      by the board.
+    minItems: 1
+    items:
+      enum:
+        # There is not a standard bus between the MAC and the PHY,
+        # something proprietary is being used to embed the PHY in the
+        # MAC.
+        - internal
+        - mii
+        - gmii
+        - sgmii
+        - qsgmii
+        - tbi
+        - rev-mii
+        - rmii
+        - rev-rmii
+
+        # RX and TX delays are added by the MAC when required
+        - rgmii
+
+        # RGMII with internal RX and TX delays provided by the PHY,
+        # the MAC should not add the RX or TX delays in this case
+        - rgmii-id
+
+        # RGMII with internal RX delay provided by the PHY, the MAC
+        # should not add an RX delay in this case
+        - rgmii-rxid
+
+        # RGMII with internal TX delay provided by the PHY, the MAC
+        # should not add an TX delay in this case
+        - rgmii-txid
+        - rtbi
+        - smii
+        - xgmii
+        - trgmii
+        - 1000base-x
+        - 2500base-x
+        - 5gbase-r
+        - rxaui
+        - xaui
+
+        # 10GBASE-KR, XFI, SFI
+        - 10gbase-kr
+        - usxgmii
+        - 10gbase-r
+        - 25gbase-r
 
   phy-mode:
     $ref: "#/properties/phy-connection-type"
-- 
2.32.0

[PATCH net-next v2 2/8] net: Update documentation for *_get_phy_mode() functions

[Marek Behún]

Now that the `phy-mode` DT property can be an array of strings instead
of just one string, update the documentation for of_get_phy_mode(),
fwnode_get_phy_mode() and device_get_phy_mode() saying that if multiple
strings are present, the first one is returned.

Conventionally the property was used to represent the mode we want the
PHY to operate in, but we extended this to mean the list of all
supported modes by that PHY on that particular board.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/base/property.c | 14 ++++++++------
 net/core/of_net.c       |  9 +++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f1f35b48ab8b..e12aef10f7fd 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -893,12 +893,13 @@ enum dev_dma_attr device_get_dma_attr(struct device *dev)
 EXPORT_SYMBOL_GPL(device_get_dma_attr);
 
 /**
- * fwnode_get_phy_mode - Get phy mode for given firmware node
+ * fwnode_get_phy_mode - Get first phy mode for given firmware node
  * @fwnode:	Pointer to the given node
  *
  * The function gets phy interface string from property 'phy-mode' or
- * 'phy-connection-type', and return its index in phy_modes table, or errno in
- * error case.
+ * 'phy-connection-type', and returns its index in phy_modes table, or errno in
+ * error case. If there are multiple strings in the property, the first one is
+ * used.
  */
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode)
 {
@@ -921,12 +922,13 @@ int fwnode_get_phy_mode(struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_get_phy_mode);
 
 /**
- * device_get_phy_mode - Get phy mode for given device
+ * device_get_phy_mode - Get first phy mode for given device
  * @dev:	Pointer to the given device
  *
  * The function gets phy interface string from property 'phy-mode' or
- * 'phy-connection-type', and return its index in phy_modes table, or errno in
- * error case.
+ * 'phy-connection-type', and returns its index in phy_modes table, or errno in
+ * error case. If there are multiple strings in the property, the first one is
+ * used.
  */
 int device_get_phy_mode(struct device *dev)
 {
diff --git a/net/core/of_net.c b/net/core/of_net.c
index f1a9bf7578e7..7cd10f0ef679 100644
--- a/net/core/of_net.c
+++ b/net/core/of_net.c
@@ -14,14 +14,15 @@
 #include <linux/nvmem-consumer.h>
 
 /**
- * of_get_phy_mode - Get phy mode for given device_node
+ * of_get_phy_mode - Get first phy mode for given device_node
  * @np:	Pointer to the given device_node
  * @interface: Pointer to the result
  *
  * The function gets phy interface string from property 'phy-mode' or
- * 'phy-connection-type'. The index in phy_modes table is set in
- * interface and 0 returned. In case of error interface is set to
- * PHY_INTERFACE_MODE_NA and an errno is returned, e.g. -ENODEV.
+ * 'phy-connection-type'. If there are more string in the property, the first
+ * one is used. The index in phy_modes table is set in interface and 0 returned.
+ * In case of error interface is set to PHY_INTERFACE_MODE_NA and an errno is
+ * returned, e.g. -ENODEV.
  */
 int of_get_phy_mode(struct device_node *np, phy_interface_t *interface)
 {
-- 
2.32.0

[PATCH net-next v2 3/8] device property: add helper function for getting phy mode bitmap

[Marek Behún]

Now that the 'phy-mode' property can be a string array containing more
PHY modes, add helper function fwnode_get_phy_modes() that reads this
property and fills in PHY interfaces bitmap.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/base/property.c  | 34 ++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 37 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e12aef10f7fd..9f9dbc2ae386 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -921,6 +921,40 @@ int fwnode_get_phy_mode(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_get_phy_mode);
 
+/**
+ * fwnode_get_phy_modes - Fill in phy modes bitmap for given firmware node
+ * @fwnode:	Pointer to the given node
+ * @interfaces:	Phy modes bitmask, as declared by DECLARE_PHY_INTERFACE_MASK()
+ *
+ * Reads the strings from property 'phy-mode' or 'phy-connection-type' and fills
+ * interfaces bitmask. Returns 0 on success, or errno on error.
+ */
+int fwnode_get_phy_modes(struct fwnode_handle *fwnode,
+			 unsigned long *interfaces)
+{
+	const char *modes[PHY_INTERFACE_MODE_MAX];
+	int len, i, j;
+
+	len = fwnode_property_read_string_array(fwnode, "phy-mode", modes,
+						ARRAY_SIZE(modes));
+	if (len < 0)
+		len = fwnode_property_read_string_array(fwnode,
+							"phy-connection-type",
+							modes,
+							ARRAY_SIZE(modes));
+	if (len < 0)
+		return len;
+
+	phy_interface_zero(interfaces);
+	for (i = 0; i < len; ++i)
+		for (j = 0; j < PHY_INTERFACE_MODE_MAX; j++)
+			if (!strcasecmp(modes[i], phy_modes(j)))
+				__set_bit(j, interfaces);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_modes);
+
 /**
  * device_get_phy_mode - Get first phy mode for given device
  * @dev:	Pointer to the given device
diff --git a/include/linux/property.h b/include/linux/property.h
index 88fa726a76df..99a74d524b2b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -391,6 +391,9 @@ const void *device_get_match_data(struct device *dev);
 int device_get_phy_mode(struct device *dev);
 
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
+int fwnode_get_phy_modes(struct fwnode_handle *fwnode,
+			 unsigned long *interfaces);
+
 struct fwnode_handle *fwnode_graph_get_next_endpoint(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
 struct fwnode_handle *
-- 
2.32.0

[PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Marek Behún]

Now that the 'phy-mode' property can be a string array containing more
PHY modes (all that are supported by the board), update the bitmap of
interfaces supported by the MAC with this property.

Normally this would be a simple intersection (of interfaces supported by
the current implementation of the driver and interfaces supported by the
board), but we need to keep being backwards compatible with older DTs,
which may only define one mode, since, as Russell King says,
  conventionally phy-mode has meant "this is the mode we want to operate
  the PHY interface in" which was fine when PHYs didn't change their
  mode depending on the media speed

An example is DT defining
  phy-mode = "sgmii";
but the board supporting also 1000base-x and 2500base-x.

Add the following logic to keep this backwards compatiblity:
- if more PHY modes are defined, do a simple intersection
- if one PHY mode is defined:
  - if it is sgmii, 1000base-x or 2500base-x, add all three and then do
    the intersection
  - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r,
    2500base-x, 1000base-x and sgmii, and then do the intersection

This is simple enough and should work for all boards.

Nonetheless it is possible (although extremely unlikely, in my opinion)
that a board will be found that (for example) defines
  phy-mode = "sgmii";
and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the
board DOESN'T support 2500base-x, because of electrical reasons (since
the frequency is 2.5x of sgmii).
Our code will in this case incorrectly infer also support for
2500base-x. To avoid this, the board maintainer should either change DTS
to
  phy-mode = "sgmii", "1000base-x";
and update device tree on all boards, or, if that is impossible, add a
fix into the function we are introducing in this commit.

Another example would be a board with device-tree defining
  phy-mode = "10gbase-r";
We infer from this all other modes (sgmii, 1000base-x, 2500base-x,
5gbase-r, usxgmii), and these then get filtered by those supported by
the driver. But it is possible that a driver supports all of these
modes, and yet not all are supported because the board has an older
version of the TF-A firmware, which implements changing of PHY modes via
SMC calls. For this case, the board maintainer should either provide all
supported modes in the phy-mode property, or add a fix into this
function that somehow checks for this situation. But this is a really
unprobable scenario, in my opinion.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since v1:
- added 10gbase-r example scenario to commit message
- changed phylink_update_phy_modes() so that if supported_interfaces is
  empty (an unconverted driver that doesn't fill up this member), we
  leave it empty
- rewritten phylink_update_phy_modes() according to Sean Anderson's
  comment: use phy_interface_and/or() instead of several
  if (test_bit) set_bit
---
 drivers/net/phy/phylink.c | 70 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3603c024109a..d2300a3a60ec 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -564,6 +564,74 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 	return 0;
 }
 
+static void phylink_update_phy_modes(struct phylink *pl,
+				     struct fwnode_handle *fwnode)
+{
+	unsigned long *supported = pl->config->supported_interfaces;
+	DECLARE_PHY_INTERFACE_MASK(modes);
+
+	/* FIXME: If supported is empty, leave it as it is. This happens for
+	 * unconverted drivers that don't fill up supported_interfaces. Once all
+	 * such drivers are converted, we can drop this.
+	 */
+	if (phy_interface_empty(supported))
+		return;
+
+	if (fwnode_get_phy_modes(fwnode, modes) < 0)
+		return;
+
+	/* If no modes are defined in fwnode, interpret it as all modes
+	 * supported by the MAC are supported by the board.
+	 */
+	if (phy_interface_empty(modes))
+		return;
+
+	/* We want the intersection of given supported modes with those defined
+	 * in DT.
+	 *
+	 * Some older device-trees mention only one of `sgmii`, `1000base-x` or
+	 * `2500base-x`, while supporting all three. Other mention `10gbase-r`
+	 * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`,
+	 * `2500base-x` and `5gbase-r`.
+	 * For backwards compatibility with these older DTs, make it so that if
+	 * one of these modes is mentioned in DT and MAC supports more of them,
+	 * keep all that are supported according to the logic above.
+	 *
+	 * Nonetheless it is possible that a device may support only one mode,
+	 * for example 1000base-x, due to strapping pins or some other reasons.
+	 * If a specific device supports only the mode mentioned in DT, the
+	 * exception should be made here with of_machine_is_compatible().
+	 */
+	if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) {
+		DECLARE_PHY_INTERFACE_MASK(mask);
+		bool lower = false;
+
+		if (test_bit(PHY_INTERFACE_MODE_10GBASER, modes) ||
+		    test_bit(PHY_INTERFACE_MODE_USXGMII, modes)) {
+			phy_interface_zero(mask);
+			__set_bit(PHY_INTERFACE_MODE_5GBASER, mask);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, mask);
+			__set_bit(PHY_INTERFACE_MODE_USXGMII, mask);
+			phy_interface_and(mask, supported, mask);
+			phy_interface_or(modes, modes, mask);
+			lower = true;
+		}
+
+		if (lower || (test_bit(PHY_INTERFACE_MODE_SGMII, modes) ||
+			      test_bit(PHY_INTERFACE_MODE_1000BASEX, modes) ||
+			      test_bit(PHY_INTERFACE_MODE_2500BASEX, modes))) {
+			phy_interface_zero(mask);
+			__set_bit(PHY_INTERFACE_MODE_SGMII, mask);
+			__set_bit(PHY_INTERFACE_MODE_1000BASEX, mask);
+			__set_bit(PHY_INTERFACE_MODE_2500BASEX, mask);
+			phy_interface_and(mask, supported, mask);
+			phy_interface_or(modes, modes, mask);
+		}
+	}
+
+	phy_interface_and(supported, supported, modes);
+}
+
 static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
 {
 	struct fwnode_handle *dn;
@@ -1157,6 +1225,8 @@ struct phylink *phylink_create(struct phylink_config *config,
 	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
 
+	phylink_update_phy_modes(pl, fwnode);
+
 	bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	linkmode_copy(pl->link_config.advertising, pl->supported);
 	phylink_validate(pl, pl->supported, &pl->link_config);
-- 
2.32.0

[PATCH net-next v2 5/8] net: phylink: pass supported PHY interface modes to phylib

[Marek Behún]

Pass the supported PHY interface types to phylib so that PHY drivers
can select an appropriate host configuration mode for their interface
according to the host capabilities.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 28 ++++++++++++++++++++++++++++
 include/linux/phy.h       | 10 ++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index d2300a3a60ec..0dcf94170d06 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1439,6 +1439,10 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 {
 	int ret;
 
+	/* Set the PHY's host supported interfaces */
+	phy_interface_copy(phy->host_interfaces,
+			   pl->config->supported_interfaces);
+
 	/* Use PHY device/driver interface */
 	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
 		pl->link_interface = phy->interface;
@@ -1514,6 +1518,10 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	if (!phy_dev)
 		return -ENODEV;
 
+	/* Set the PHY's host supported interfaces */
+	phy_interface_copy(phy_dev->host_interfaces,
+			   pl->config->supported_interfaces);
+
 	/* Use PHY device/driver interface */
 	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
 		pl->link_interface = phy_dev->interface;
@@ -2704,6 +2712,8 @@ static bool phylink_phy_no_inband(struct phy_device *phy)
 		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
 }
 
+static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces);
+
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phylink *pl = upstream;
@@ -2725,6 +2735,10 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	else
 		mode = MLO_AN_INBAND;
 
+	/* Set the PHY's host supported interfaces */
+	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
+			  pl->config->supported_interfaces);
+
 	/* Do the initial configuration */
 	ret = phylink_sfp_config(pl, mode, phy->supported, phy->advertising);
 	if (ret < 0)
@@ -3102,4 +3116,18 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c45_pcs_get_state);
 
+static int __init phylink_init(void)
+{
+	__set_bit(PHY_INTERFACE_MODE_USXGMII, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_10GBASER, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_5GBASER, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_2500BASEX, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_SGMII, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, phylink_sfp_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_100BASEX, phylink_sfp_interfaces);
+
+	return 0;
+}
+module_init(phylink_init);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1e57cdd95da3..11f3b5b7d7b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -169,6 +169,12 @@ static inline bool phy_interface_empty(const unsigned long *intf)
 	return bitmap_empty(intf, PHY_INTERFACE_MODE_MAX);
 }
 
+static inline void phy_interface_copy(unsigned long *dst,
+				      const unsigned long *src)
+{
+	bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX);
+}
+
 static inline void phy_interface_and(unsigned long *dst, const unsigned long *a,
 				     const unsigned long *b)
 {
@@ -563,6 +569,7 @@ struct macsec_ops;
  * @advertising: Currently advertised linkmodes
  * @adv_old: Saved advertised while power saving for WoL
  * @lp_advertising: Current link partner advertised linkmodes
+ * @host_interfaces: PHY interface modes supported by host
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
  * @autoneg: Flag autoneg being used
  * @link: Current link state
@@ -652,6 +659,9 @@ struct phy_device {
 	/* used with phy_speed_down */
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
 
+	/* host supported PHY interface types */
+	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
+
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
 
-- 
2.32.0

[PATCH net-next v2 6/8] net: phy: marvell10g: Use generic macro for supported interfaces

[Marek Behún]

Now that phy.h defines macro DECLARE_PHY_INTERFACE_MASK(), use it
instead of DECLARE_BITMAP().

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell10g.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index b6fea119fe13..d289641190db 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -148,7 +148,7 @@ struct mv3310_chip {
 };
 
 struct mv3310_priv {
-	DECLARE_BITMAP(supported_interfaces, PHY_INTERFACE_MODE_MAX);
+	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
 
 	u32 firmware_ver;
 	bool has_downshift;
-- 
2.32.0

[PATCH net-next v2 7/8] net: phy: marvell10g: Use tabs instead of spaces for indentation

[Marek Behún]

Some register definitions were defined with spaces used for indentation.
Change them to tabs.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell10g.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index d289641190db..0cb9b4ef09c7 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -117,16 +117,16 @@ enum {
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN	= 0x5,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII			= 0x7,
-	MV_V2_PORT_INTR_STS     = 0xf040,
-	MV_V2_PORT_INTR_MASK    = 0xf043,
-	MV_V2_PORT_INTR_STS_WOL_EN      = BIT(8),
-	MV_V2_MAGIC_PKT_WORD0   = 0xf06b,
-	MV_V2_MAGIC_PKT_WORD1   = 0xf06c,
-	MV_V2_MAGIC_PKT_WORD2   = 0xf06d,
+	MV_V2_PORT_INTR_STS		= 0xf040,
+	MV_V2_PORT_INTR_MASK		= 0xf043,
+	MV_V2_PORT_INTR_STS_WOL_EN	= BIT(8),
+	MV_V2_MAGIC_PKT_WORD0		= 0xf06b,
+	MV_V2_MAGIC_PKT_WORD1		= 0xf06c,
+	MV_V2_MAGIC_PKT_WORD2		= 0xf06d,
 	/* Wake on LAN registers */
-	MV_V2_WOL_CTRL          = 0xf06e,
-	MV_V2_WOL_CTRL_CLEAR_STS        = BIT(15),
-	MV_V2_WOL_CTRL_MAGIC_PKT_EN     = BIT(0),
+	MV_V2_WOL_CTRL			= 0xf06e,
+	MV_V2_WOL_CTRL_CLEAR_STS	= BIT(15),
+	MV_V2_WOL_CTRL_MAGIC_PKT_EN	= BIT(0),
 	/* Temperature control/read registers (88X3310 only) */
 	MV_V2_TEMP_CTRL		= 0xf08a,
 	MV_V2_TEMP_CTRL_MASK	= 0xc000,
-- 
2.32.0

[PATCH net-next v2 8/8] net: phy: marvell10g: select host interface configuration

[Marek Behún]

From: Russell King <rmk+kernel@armlinux.org.uk>

Select the host interface configuration according to the capabilities of
the host.

The PHY supports several configurations of host communication:
- always communicate with host in 10gbase-r, even if copper speed is
  lower (rate matching mode),
- the same as above but use xaui/rxaui instead of 10gbase-r,
- switch host SerDes mode between 10gbase-r, 5gbase-r, 2500base-x and
  sgmii according to copper speed,
- the same as above but use xaui/rxaui instead of 10gbase-r.

This mode of host communication, called MACTYPE, is by default selected
by strapping pins, but it can be changed in software.

This adds support for selecting this mode according to which modes are
supported by the host.

This allows the kernel to:
- support SFP modules with 88X33X0 or 88E21X0 inside them
- switch interface modes when the PHY is used with the mvpp2 MAC
  (e.g. on MacchiatoBIN)

Note: we use mv3310_select_mactype() for both 88X3310 and 88X3340,
although 88X3340 does not support XAUI. This is not a problem because
88X3340 does not declare XAUI in it's supported_interfaces, and so this
function will never choose that MACTYPE.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
[ rebase, updated, also added support for 88E21X0 ]
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since v1:
- added more explanation to commit message, as per Vladimir Oltean's
  suggestion
---
 drivers/net/phy/marvell10g.c | 120 +++++++++++++++++++++++++++++++++--
 1 file changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 0cb9b4ef09c7..94bea1bade6f 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -96,6 +96,11 @@ enum {
 	MV_PCS_PORT_INFO_NPORTS_MASK	= 0x0380,
 	MV_PCS_PORT_INFO_NPORTS_SHIFT	= 7,
 
+	/* SerDes reinitialization 88E21X0 */
+	MV_AN_21X0_SERDES_CTRL2	= 0x800f,
+	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
+	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
+
 	/* These registers appear at 0x800X and 0xa00X - the 0xa00X control
 	 * registers appear to set themselves to the 0x800X when AN is
 	 * restarted, but status registers appear readable from either.
@@ -140,6 +145,8 @@ struct mv3310_chip {
 	bool (*has_downshift)(struct phy_device *phydev);
 	void (*init_supported_interfaces)(unsigned long *mask);
 	int (*get_mactype)(struct phy_device *phydev);
+	int (*set_mactype)(struct phy_device *phydev, int mactype);
+	int (*select_mactype)(unsigned long *interfaces);
 	int (*init_interface)(struct phy_device *phydev, int mactype);
 
 #ifdef CONFIG_HWMON
@@ -593,6 +600,49 @@ static int mv2110_get_mactype(struct phy_device *phydev)
 	return mactype & MV_PMA_21X0_PORT_CTRL_MACTYPE_MASK;
 }
 
+static int mv2110_set_mactype(struct phy_device *phydev, int mactype)
+{
+	int err, val;
+
+	mactype &= MV_PMA_21X0_PORT_CTRL_MACTYPE_MASK;
+	err = phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_21X0_PORT_CTRL,
+			     MV_PMA_21X0_PORT_CTRL_SWRST |
+			     MV_PMA_21X0_PORT_CTRL_MACTYPE_MASK,
+			     MV_PMA_21X0_PORT_CTRL_SWRST | mactype);
+	if (err)
+		return err;
+
+	err = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MV_AN_21X0_SERDES_CTRL2,
+			       MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS |
+			       MV_AN_21X0_SERDES_CTRL2_RUN_INIT);
+	if (err)
+		return err;
+
+	err = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_AN,
+					MV_AN_21X0_SERDES_CTRL2, val,
+					!(val &
+					  MV_AN_21X0_SERDES_CTRL2_RUN_INIT),
+					5000, 100000, true);
+	if (err)
+		return err;
+
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MV_AN_21X0_SERDES_CTRL2,
+				  MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS);
+}
+
+static int mv2110_select_mactype(unsigned long *interfaces)
+{
+	if (test_bit(PHY_INTERFACE_MODE_USXGMII, interfaces))
+		return MV_PMA_21X0_PORT_CTRL_MACTYPE_USXGMII;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 !test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_PMA_21X0_PORT_CTRL_MACTYPE_5GBASER;
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_PMA_21X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH;
+	else
+		return -1;
+}
+
 static int mv3310_get_mactype(struct phy_device *phydev)
 {
 	int mactype;
@@ -604,6 +654,46 @@ static int mv3310_get_mactype(struct phy_device *phydev)
 	return mactype & MV_V2_33X0_PORT_CTRL_MACTYPE_MASK;
 }
 
+static int mv3310_set_mactype(struct phy_device *phydev, int mactype)
+{
+	int ret;
+
+	mactype &= MV_V2_33X0_PORT_CTRL_MACTYPE_MASK;
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				     MV_V2_33X0_PORT_CTRL_MACTYPE_MASK,
+				     mactype);
+	if (ret <= 0)
+		return ret;
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				MV_V2_33X0_PORT_CTRL_SWRST);
+}
+
+static int mv3310_select_mactype(unsigned long *interfaces)
+{
+	if (test_bit(PHY_INTERFACE_MODE_USXGMII, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
+		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI;
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
+		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
+	else
+		return -1;
+}
+
 static int mv2110_init_interface(struct phy_device *phydev, int mactype)
 {
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
@@ -674,10 +764,16 @@ static int mv3310_config_init(struct phy_device *phydev)
 {
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
 	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
 	int err, mactype;
 
-	/* Check that the PHY interface type is compatible */
-	if (!test_bit(phydev->interface, priv->supported_interfaces))
+	/* In case host didn't provide supported interfaces */
+	__set_bit(phydev->interface, phydev->host_interfaces);
+
+	/* Check that there is at least one compatible PHY interface type */
+	phy_interface_and(interfaces, phydev->host_interfaces,
+			  priv->supported_interfaces);
+	if (phy_interface_empty(interfaces))
 		return -ENODEV;
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
@@ -687,9 +783,15 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (err)
 		return err;
 
-	mactype = chip->get_mactype(phydev);
-	if (mactype < 0)
-		return mactype;
+	mactype = chip->select_mactype(interfaces);
+	if (mactype < 0) {
+		mactype = chip->get_mactype(phydev);
+	} else {
+		phydev_info(phydev, "Changing MACTYPE to %i\n", mactype);
+		err = chip->set_mactype(phydev, mactype);
+		if (err)
+			return err;
+	}
 
 	err = chip->init_interface(phydev, mactype);
 	if (err) {
@@ -1049,6 +1151,8 @@ static const struct mv3310_chip mv3310_type = {
 	.has_downshift = mv3310_has_downshift,
 	.init_supported_interfaces = mv3310_init_supported_interfaces,
 	.get_mactype = mv3310_get_mactype,
+	.set_mactype = mv3310_set_mactype,
+	.select_mactype = mv3310_select_mactype,
 	.init_interface = mv3310_init_interface,
 
 #ifdef CONFIG_HWMON
@@ -1060,6 +1164,8 @@ static const struct mv3310_chip mv3340_type = {
 	.has_downshift = mv3310_has_downshift,
 	.init_supported_interfaces = mv3340_init_supported_interfaces,
 	.get_mactype = mv3310_get_mactype,
+	.set_mactype = mv3310_set_mactype,
+	.select_mactype = mv3310_select_mactype,
 	.init_interface = mv3340_init_interface,
 
 #ifdef CONFIG_HWMON
@@ -1070,6 +1176,8 @@ static const struct mv3310_chip mv3340_type = {
 static const struct mv3310_chip mv2110_type = {
 	.init_supported_interfaces = mv2110_init_supported_interfaces,
 	.get_mactype = mv2110_get_mactype,
+	.set_mactype = mv2110_set_mactype,
+	.select_mactype = mv2110_select_mactype,
 	.init_interface = mv2110_init_interface,
 
 #ifdef CONFIG_HWMON
@@ -1080,6 +1188,8 @@ static const struct mv3310_chip mv2110_type = {
 static const struct mv3310_chip mv2111_type = {
 	.init_supported_interfaces = mv2111_init_supported_interfaces,
 	.get_mactype = mv2110_get_mactype,
+	.set_mactype = mv2110_set_mactype,
+	.select_mactype = mv2110_select_mactype,
 	.init_interface = mv2110_init_interface,
 
 #ifdef CONFIG_HWMON
-- 
2.32.0

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Vladimir Oltean]

On Tue, Nov 23, 2021 at 05:40:23PM +0100, Marek Behún wrote:
> Now that the 'phy-mode' property can be a string array containing more
> PHY modes (all that are supported by the board), update the bitmap of
> interfaces supported by the MAC with this property.
> 
> Normally this would be a simple intersection (of interfaces supported by
> the current implementation of the driver and interfaces supported by the
> board), but we need to keep being backwards compatible with older DTs,
> which may only define one mode, since, as Russell King says,
>   conventionally phy-mode has meant "this is the mode we want to operate
>   the PHY interface in" which was fine when PHYs didn't change their
>   mode depending on the media speed
> 
> An example is DT defining
>   phy-mode = "sgmii";
> but the board supporting also 1000base-x and 2500base-x.
> 
> Add the following logic to keep this backwards compatiblity:
> - if more PHY modes are defined, do a simple intersection
> - if one PHY mode is defined:
>   - if it is sgmii, 1000base-x or 2500base-x, add all three and then do
>     the intersection
>   - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r,
>     2500base-x, 1000base-x and sgmii, and then do the intersection
> 
> This is simple enough and should work for all boards.
> 
> Nonetheless it is possible (although extremely unlikely, in my opinion)
> that a board will be found that (for example) defines
>   phy-mode = "sgmii";
> and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the
> board DOESN'T support 2500base-x, because of electrical reasons (since
> the frequency is 2.5x of sgmii).
> Our code will in this case incorrectly infer also support for
> 2500base-x. To avoid this, the board maintainer should either change DTS
> to
>   phy-mode = "sgmii", "1000base-x";
> and update device tree on all boards, or, if that is impossible, add a
> fix into the function we are introducing in this commit.
> 
> Another example would be a board with device-tree defining
>   phy-mode = "10gbase-r";
> We infer from this all other modes (sgmii, 1000base-x, 2500base-x,
> 5gbase-r, usxgmii), and these then get filtered by those supported by
> the driver. But it is possible that a driver supports all of these
> modes, and yet not all are supported because the board has an older
> version of the TF-A firmware, which implements changing of PHY modes via
> SMC calls. For this case, the board maintainer should either provide all
> supported modes in the phy-mode property, or add a fix into this
> function that somehow checks for this situation. But this is a really
> unprobable scenario, in my opinion.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> Changes since v1:
> - added 10gbase-r example scenario to commit message
> - changed phylink_update_phy_modes() so that if supported_interfaces is
>   empty (an unconverted driver that doesn't fill up this member), we
>   leave it empty
> - rewritten phylink_update_phy_modes() according to Sean Anderson's
>   comment: use phy_interface_and/or() instead of several
>   if (test_bit) set_bit
> ---
>  drivers/net/phy/phylink.c | 70 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3603c024109a..d2300a3a60ec 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -564,6 +564,74 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>  	return 0;
>  }
>  
> +static void phylink_update_phy_modes(struct phylink *pl,
> +				     struct fwnode_handle *fwnode)
> +{
> +	unsigned long *supported = pl->config->supported_interfaces;
> +	DECLARE_PHY_INTERFACE_MASK(modes);
> +
> +	/* FIXME: If supported is empty, leave it as it is. This happens for
> +	 * unconverted drivers that don't fill up supported_interfaces. Once all
> +	 * such drivers are converted, we can drop this.
> +	 */
> +	if (phy_interface_empty(supported))
> +		return;
> +
> +	if (fwnode_get_phy_modes(fwnode, modes) < 0)
> +		return;
> +
> +	/* If no modes are defined in fwnode, interpret it as all modes
> +	 * supported by the MAC are supported by the board.
> +	 */
> +	if (phy_interface_empty(modes))
> +		return;
> +
> +	/* We want the intersection of given supported modes with those defined
> +	 * in DT.
> +	 *
> +	 * Some older device-trees mention only one of `sgmii`, `1000base-x` or
> +	 * `2500base-x`, while supporting all three. Other mention `10gbase-r`
> +	 * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`,
> +	 * `2500base-x` and `5gbase-r`.
> +	 * For backwards compatibility with these older DTs, make it so that if
> +	 * one of these modes is mentioned in DT and MAC supports more of them,
> +	 * keep all that are supported according to the logic above.
> +	 *
> +	 * Nonetheless it is possible that a device may support only one mode,
> +	 * for example 1000base-x, due to strapping pins or some other reasons.
> +	 * If a specific device supports only the mode mentioned in DT, the
> +	 * exception should be made here with of_machine_is_compatible().
> +	 */
> +	if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) {

I like the idea of extending the mask of phy_modes based on the
phylink_config.supported_interfaces bitmap that the driver populates
(assuming, of course, that it's converted correctly to this new format,
and I looked through the implementations and just found a bug). I think
it might just work with old device trees, too. In fact, it may work so
well, that I would even be tempted to ask "can we defer for a while
updating the device trees and bindings documents with an array, just
keep the phy modes as an array internally inside the kernel?"

On the Marvell boards that you're working with, do you have an example
board on which not all the PHY modes supported by the driver might be
available? Do you also know the reason why? You give an example in the
comments about 1000base-X and strapping, can you expand on that?

Because I think it's a bit strange to create a framework for fixups now,
when we don't even know what kind of stuff may be broken. The PHY modes
(effectively SERDES protocols) might not be what you actually want to restrict.
I mean, restricting the PHY modes is like saying: "the MAC supports
USXGMII and 10GBase-R, the PHY supports USXGMII and 10GBase-R, I know
how to configure both of them in each mode, but on this board, USXGMII
works and 10GBase-R doesn't".

?!

It may make more sense, when the time comes for the first fixup, to put
a cap on the maximum gross data rate attainable on that particular lane
(including stuff such as USXGMII symbol repetition), instead of having
to list out all the PHY modes that a driver might or might not support
right now. Imagine what pain it will be to update device trees each time
a driver gains software support for a SERDES protocol it didn't support
before. But if you cap the lane frequency, you can make phylink_update_phy_modes()
deduce what isn't acceptable using simple logic.

Also, if there's something to be broken by this change, why would we put
an of_machine_is_compatible() here, and why wouldn't we instead update
the phylink_config.supported_interfaces directly in the driver? I think
it's the driver's responsibility for passing a valid mask of supported
interfaces.

> +		DECLARE_PHY_INTERFACE_MASK(mask);
> +		bool lower = false;
> +
> +		if (test_bit(PHY_INTERFACE_MODE_10GBASER, modes) ||
> +		    test_bit(PHY_INTERFACE_MODE_USXGMII, modes)) {
> +			phy_interface_zero(mask);
> +			__set_bit(PHY_INTERFACE_MODE_5GBASER, mask);
> +			__set_bit(PHY_INTERFACE_MODE_10GBASER, mask);
> +			__set_bit(PHY_INTERFACE_MODE_USXGMII, mask);
> +			phy_interface_and(mask, supported, mask);
> +			phy_interface_or(modes, modes, mask);
> +			lower = true;
> +		}
> +
> +		if (lower || (test_bit(PHY_INTERFACE_MODE_SGMII, modes) ||
> +			      test_bit(PHY_INTERFACE_MODE_1000BASEX, modes) ||
> +			      test_bit(PHY_INTERFACE_MODE_2500BASEX, modes))) {
> +			phy_interface_zero(mask);
> +			__set_bit(PHY_INTERFACE_MODE_SGMII, mask);
> +			__set_bit(PHY_INTERFACE_MODE_1000BASEX, mask);
> +			__set_bit(PHY_INTERFACE_MODE_2500BASEX, mask);
> +			phy_interface_and(mask, supported, mask);
> +			phy_interface_or(modes, modes, mask);
> +		}
> +	}
> +
> +	phy_interface_and(supported, supported, modes);
> +}
> +
>  static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
>  {
>  	struct fwnode_handle *dn;
> @@ -1157,6 +1225,8 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
>  
> +	phylink_update_phy_modes(pl, fwnode);
> +
>  	bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>  	linkmode_copy(pl->link_config.advertising, pl->supported);
>  	phylink_validate(pl, pl->supported, &pl->link_config);
> -- 
> 2.32.0
> 

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Marek Behún]

Hi Vladimir,

On Tue, 23 Nov 2021 23:24:41 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> > +	/* We want the intersection of given supported modes with those defined
> > +	 * in DT.
> > +	 *
> > +	 * Some older device-trees mention only one of `sgmii`, `1000base-x` or
> > +	 * `2500base-x`, while supporting all three. Other mention `10gbase-r`
> > +	 * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`,
> > +	 * `2500base-x` and `5gbase-r`.
> > +	 * For backwards compatibility with these older DTs, make it so that if
> > +	 * one of these modes is mentioned in DT and MAC supports more of them,
> > +	 * keep all that are supported according to the logic above.
> > +	 *
> > +	 * Nonetheless it is possible that a device may support only one mode,
> > +	 * for example 1000base-x, due to strapping pins or some other reasons.
> > +	 * If a specific device supports only the mode mentioned in DT, the
> > +	 * exception should be made here with of_machine_is_compatible().
> > +	 */
> > +	if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) {  
> 
> I like the idea of extending the mask of phy_modes based on the
> phylink_config.supported_interfaces bitmap that the driver populates
> (assuming, of course, that it's converted correctly to this new format,
> and I looked through the implementations and just found a bug).

Russell is working on converting/fixing the drivers, you can look at his
changes at
 http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
look at the first two pages, search for "populate supported_interfaces"

> I think
> it might just work with old device trees, too. In fact, it may work so
> well, that I would even be tempted to ask "can we defer for a while
> updating the device trees and bindings documents with an array, just
> keep the phy modes as an array internally inside the kernel?"

We don't need to update the device-trees immediately, but I really
think the DT binding should be ready to allow multiple modes, if
someone does want to update.

> On the Marvell boards that you're working with, do you have an example
> board on which not all the PHY modes supported by the driver might be
> available? Do you also know the reason why? You give an example in the
> comments about 1000base-X and strapping, can you expand on that?

On Macchiatobin, both the mvpp2 MAC driver and the marvell10g PHY
driver support xaui mode, but that mode requires 4 SerDes lanes, and
the boards wires only one lane between the SOC and the PHY, so obviously
it cannot be used. Thinking about it, maybe I should have put this into
the commit message.

> Because I think it's a bit strange to create a framework for fixups now,
> when we don't even know what kind of stuff may be broken. The PHY modes
> (effectively SERDES protocols) might not be what you actually want to restrict.
> I mean, restricting the PHY modes is like saying: "the MAC supports
> USXGMII and 10GBase-R, the PHY supports USXGMII and 10GBase-R, I know
> how to configure both of them in each mode, but on this board, USXGMII
> works and 10GBase-R doesn't".
> 
> ?!

As I said above, the best example is with SerDeses which use multiple
lanes where not all may be wired. But this backward compatibility code
concerns only one-lane SerDeses: usxgmii, 10gbaser-r, 5gbase-r,
2500base-x, 1000base-x, sgmii. For these, I can think only of 2
possibilities why a fixup might be needed to restrict some mode when it
is supported both by MAC and PHY:
1) the mode is not supported by the board because it has too large
   frequency and the wiring on the board interferes with it.
   Example:
   - device tree defined phy-mode = "sgmii"
   - from this we infer also 1000base-x and 2500base-x
   - but 2500base-x works at 2.5x the frequency of 1000base-x/sgmii
   - the board wiring does not work at that frequency

   I don't know of any such board, but I know that such thing is
   possible, because for example the connetor on Turris MOX modules
   allows only frequencies up to 6 GHz. So it is not impossible for
   there to be boards where only 2 GHz is supported...

2) the mode is not supported by the board because the generic PHY
   driver uses SMC calls to the firmware to change the SerDes mode, but
   the board may have old version of firmware which does not support
   SMC call correctly (or at all). This was a real issue with TF-A
   firmware on Macchiatobin, where the 5gbase-r mode was not supported
   in older versions

> It may make more sense, when the time comes for the first fixup, to put
> a cap on the maximum gross data rate attainable on that particular lane
> (including stuff such as USXGMII symbol repetition), instead of having
> to list out all the PHY modes that a driver might or might not support
> right now. Imagine what pain it will be to update device trees each time
> a driver gains software support for a SERDES protocol it didn't support
> before. But if you cap the lane frequency, you can make phylink_update_phy_modes()
> deduce what isn't acceptable using simple logic.

So we would need to specify max data rate, usxgmii symbol repetition,
and number of lanes connected, and even then it could be not enough.
I think that simply specifying all the phy-modes that the HW supports
is simpler.

The devicetree does not need and should not be updated each time
software gains support for another SerDes protocol. The devicetree
should be updated only once, to specify all SerDes protocols that the
hardware supports (the SOC/MAC, the PHY, and the board wiring). The
software then takes from this list only those modes that the drivers
support. So no need to update device-tree each time SW gains support
for new SerDes mode.

> Also, if there's something to be broken by this change, why would we put
> an of_machine_is_compatible() here, and why wouldn't we instead update
> the phylink_config.supported_interfaces directly in the driver? I think
> it's the driver's responsibility for passing a valid mask of supported
> interfaces.

The whole idea of this code is to guarantee backward compatibility with
older device-trees. In my opinion (and I think Russell agrees with me),
this should be at one place, instead of putting various weird
exceptions into various MAC drivers.

Marek

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Vladimir Oltean]

On Tue, Nov 23, 2021 at 11:27:13PM +0100, Marek Behún wrote:
> Hi Vladimir,
> 
> On Tue, 23 Nov 2021 23:24:41 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > > +	/* We want the intersection of given supported modes with those defined
> > > +	 * in DT.
> > > +	 *
> > > +	 * Some older device-trees mention only one of `sgmii`, `1000base-x` or
> > > +	 * `2500base-x`, while supporting all three. Other mention `10gbase-r`
> > > +	 * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`,
> > > +	 * `2500base-x` and `5gbase-r`.
> > > +	 * For backwards compatibility with these older DTs, make it so that if
> > > +	 * one of these modes is mentioned in DT and MAC supports more of them,
> > > +	 * keep all that are supported according to the logic above.
> > > +	 *
> > > +	 * Nonetheless it is possible that a device may support only one mode,
> > > +	 * for example 1000base-x, due to strapping pins or some other reasons.
> > > +	 * If a specific device supports only the mode mentioned in DT, the
> > > +	 * exception should be made here with of_machine_is_compatible().
> > > +	 */
> > > +	if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) {  
> > 
> > I like the idea of extending the mask of phy_modes based on the
> > phylink_config.supported_interfaces bitmap that the driver populates
> > (assuming, of course, that it's converted correctly to this new format,
> > and I looked through the implementations and just found a bug).
> 
> Russell is working on converting/fixing the drivers, you can look at his
> changes at
>  http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> look at the first two pages, search for "populate supported_interfaces"

This one is a mistake in one of the existing conversions, I looked at
the git tree that you pointed to and I don't see any fixup patch on that
driver. I'm currently testing a fix, but it might take until tomorrow
until I post anything.

> > I think
> > it might just work with old device trees, too. In fact, it may work so
> > well, that I would even be tempted to ask "can we defer for a while
> > updating the device trees and bindings documents with an array, just
> > keep the phy modes as an array internally inside the kernel?"
> 
> We don't need to update the device-trees immediately, but I really
> think the DT binding should be ready to allow multiple modes, if
> someone does want to update.
> 
> > On the Marvell boards that you're working with, do you have an example
> > board on which not all the PHY modes supported by the driver might be
> > available? Do you also know the reason why? You give an example in the
> > comments about 1000base-X and strapping, can you expand on that?
> 
> On Macchiatobin, both the mvpp2 MAC driver and the marvell10g PHY
> driver support xaui mode, but that mode requires 4 SerDes lanes, and
> the boards wires only one lane between the SOC and the PHY, so obviously
> it cannot be used. Thinking about it, maybe I should have put this into
> the commit message.

Yes, so 4 lanes of 3.125 GHz or higher could support XAUI, 1 lane of
3.125 GHz or higher could support 2500base-X but not XAUI. Sounds like
simple logic.

> 
> > Because I think it's a bit strange to create a framework for fixups now,
> > when we don't even know what kind of stuff may be broken. The PHY modes
> > (effectively SERDES protocols) might not be what you actually want to restrict.
> > I mean, restricting the PHY modes is like saying: "the MAC supports
> > USXGMII and 10GBase-R, the PHY supports USXGMII and 10GBase-R, I know
> > how to configure both of them in each mode, but on this board, USXGMII
> > works and 10GBase-R doesn't".
> > 
> > ?!
> 
> As I said above, the best example is with SerDeses which use multiple
> lanes where not all may be wired. But this backward compatibility code
> concerns only one-lane SerDeses: usxgmii, 10gbaser-r, 5gbase-r,
> 2500base-x, 1000base-x, sgmii.

Right, why so? From phy-mode = "xaui", you could also infer support for
single-lane SERDES protocols up to 3.125 GHz, aren't you interested in
doing that too?

> For these, I can think only of 2
> possibilities why a fixup might be needed to restrict some mode when it
> is supported both by MAC and PHY:
> 1) the mode is not supported by the board because it has too large
>    frequency and the wiring on the board interferes with it.
>    Example:
>    - device tree defined phy-mode = "sgmii"
>    - from this we infer also 1000base-x and 2500base-x
>    - but 2500base-x works at 2.5x the frequency of 1000base-x/sgmii
>    - the board wiring does not work at that frequency
> 
>    I don't know of any such board, but I know that such thing is
>    possible, because for example the connetor on Turris MOX modules
>    allows only frequencies up to 6 GHz. So it is not impossible for
>    there to be boards where only 2 GHz is supported...

Right. We're saying the same thing, basically.

> 2) the mode is not supported by the board because the generic PHY
>    driver uses SMC calls to the firmware to change the SerDes mode, but
>    the board may have old version of firmware which does not support
>    SMC call correctly (or at all). This was a real issue with TF-A
>    firmware on Macchiatobin, where the 5gbase-r mode was not supported
>    in older versions

Ok, so in your proposal, U-Boot would have to fix up the device tree
based on a certain version of ATF, and remove "5gbase-r" from the
phy-mode array. Whereas in my proposal, the mvpp2 driver would need to
find out about the ATF version in use, and remove "5gbase-r" from the
supported interfaces.

As a user, I'd certainly prefer if Linux could figure this one out.

> > It may make more sense, when the time comes for the first fixup, to put
> > a cap on the maximum gross data rate attainable on that particular lane
> > (including stuff such as USXGMII symbol repetition), instead of having
> > to list out all the PHY modes that a driver might or might not support
> > right now. Imagine what pain it will be to update device trees each time
> > a driver gains software support for a SERDES protocol it didn't support
> > before. But if you cap the lane frequency, you can make phylink_update_phy_modes()
> > deduce what isn't acceptable using simple logic.
> 
> So we would need to specify max data rate, usxgmii symbol repetition,

No, not symbol repetition. Just data rate and number of lanes.

> and number of lanes connected, and even then it could be not enough.
> I think that simply specifying all the phy-modes that the HW supports
> is simpler.
> 
> The devicetree does not need and should not be updated each time
> software gains support for another SerDes protocol. The devicetree
> should be updated only once, to specify all SerDes protocols that the
> hardware supports (the SOC/MAC, the PHY, and the board wiring). The
> software then takes from this list only those modes that the drivers
> support. So no need to update device-tree each time SW gains support
> for new SerDes mode.

This implies that when you bring up a board and write the device tree
for it, you know that PHY mode X works without ever testing it. What if
it doesn't work when you finally add support for it? Now you already
have one DT blob in circulation. That's why I'm saying that maybe it
could be better if we could think in terms that are a bit more physical
and easy to characterize.

> > Also, if there's something to be broken by this change, why would we put
> > an of_machine_is_compatible() here, and why wouldn't we instead update
> > the phylink_config.supported_interfaces directly in the driver? I think
> > it's the driver's responsibility for passing a valid mask of supported
> > interfaces.
> 
> The whole idea of this code is to guarantee backward compatibility with
> older device-trees. In my opinion (and I think Russell agrees with me),
> this should be at one place, instead of putting various weird
> exceptions into various MAC drivers.

Yes, but they're more flexible in the driver... What if the check is not
as simple as a machine compatible (think about the ATF firmware example
you gave).

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 12:54:18AM +0200, Vladimir Oltean wrote:
> This implies that when you bring up a board and write the device tree
> for it, you know that PHY mode X works without ever testing it. What if
> it doesn't work when you finally add support for it? Now you already
> have one DT blob in circulation. That's why I'm saying that maybe it
> could be better if we could think in terms that are a bit more physical
> and easy to characterize.

However, it doesn't solve the problem. Let's take an example.

The 3310 supports a mode where it runs in XAUI/5GBASE-R/2500BASE-X/SGMII
depending on the negotiated media parameters.

XAUI is four lanes of 3.125Gbaud.
5GBASE-R is one lane of 5.15625Gbaud.

Let's say you're using this, and test the 10G speed using XAUI,
intending the other speeds to work. So you put in DT that you support
four lanes and up to 5.15625Gbaud.

Later, you discover that 5GBASE-R doesn't work because there's an
electrical issue with the board. You now have DT in circulation
which doesn't match the capabilities of the hardware.

How is this any different from the situation you describe above?
To me, it seems to be exactly the same problem.

So, I don't think one can get away from these kinds of issues - where
you create a board, do insufficient testing, publish a DT description,
and then through further testing discover you have to restrict the
hardware capabilities in DT. In fact, this is sadly an entirely normal
process - problems always get found after boards have been sent out
and initial DT has been created.

A good example is the 6th switch port on the original Clearfog boards.
This was connected to an external PHY at address 0 on the MDIO bus
behind the switch. However, the 88E6176 switch already has an internal
PHY at address 0, so the external PHY can't be accessed. Consequently,
port 6 is unreliable. That only came to light some time later, and
resulted in the DT needing to be modified.

There are always problems that need DT to be fixed - I don't think it's
possible to get away from that by changing what or how something is
described. In fact, I think trying to make that argument actually shows
a misunderstanding of the process of hardware bringup.

Just like software, hardware is buggy and takes time to be debugged,
and that process continues after the first version of DT for a board
has been produced, and there will always be changes required to DT.

I'm not saying that describing the maximum frequency and lanes doesn't
have merit, I'm merely taking issue with the basis of your argument.

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

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Marek Behún]

On Wed, 24 Nov 2021 00:54:18 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> > As I said above, the best example is with SerDeses which use multiple
> > lanes where not all may be wired. But this backward compatibility code
> > concerns only one-lane SerDeses: usxgmii, 10gbaser-r, 5gbase-r,
> > 2500base-x, 1000base-x, sgmii.  
> 
> Right, why so? From phy-mode = "xaui", you could also infer support for
> single-lane SERDES protocols up to 3.125 GHz, aren't you interested in
> doing that too?

OK, this would seem to make sense if I dropped the DT binding change
for now, and just updated the code to change the supported_interfaces
member so that marvell10g driver could select appropriate mode.

I will think about it. Thanks.

> > 2) the mode is not supported by the board because the generic PHY
> >    driver uses SMC calls to the firmware to change the SerDes mode, but
> >    the board may have old version of firmware which does not support
> >    SMC call correctly (or at all). This was a real issue with TF-A
> >    firmware on Macchiatobin, where the 5gbase-r mode was not supported
> >    in older versions  
> 
> Ok, so in your proposal, U-Boot would have to fix up the device tree
> based on a certain version of ATF, and remove "5gbase-r" from the
> phy-mode array. Whereas in my proposal, the mvpp2 driver would need to
> find out about the ATF version in use, and remove "5gbase-r" from the
> supported interfaces.
> 
> As a user, I'd certainly prefer if Linux could figure this one out.

You are right here, it would really be better for the mvpp2 driver to
do this discovering.

> This implies that when you bring up a board and write the device tree
> for it, you know that PHY mode X works without ever testing it. What if
> it doesn't work when you finally add support for it? Now you already
> have one DT blob in circulation. That's why I'm saying that maybe it
> could be better if we could think in terms that are a bit more physical
> and easy to characterize.

The thing is that this same could happen with your proposal of
max-data-rate + number-of-lanes, as Russell explained in his reply.

> > The whole idea of this code is to guarantee backward compatibility with
> > older device-trees. In my opinion (and I think Russell agrees with me),
> > this should be at one place, instead of putting various weird
> > exceptions into various MAC drivers.  
> 
> Yes, but they're more flexible in the driver... What if the check is not
> as simple as a machine compatible (think about the ATF firmware example
> you gave).

You persuaded me, it makes more sense in the driver.

Marek

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 11:04:37AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 12:54:18AM +0200, Vladimir Oltean wrote:
> > This implies that when you bring up a board and write the device tree
> > for it, you know that PHY mode X works without ever testing it. What if
> > it doesn't work when you finally add support for it? Now you already
> > have one DT blob in circulation. That's why I'm saying that maybe it
> > could be better if we could think in terms that are a bit more physical
> > and easy to characterize.
> 
> However, it doesn't solve the problem. Let's take an example.
> 
> The 3310 supports a mode where it runs in XAUI/5GBASE-R/2500BASE-X/SGMII
> depending on the negotiated media parameters.
> 
> XAUI is four lanes of 3.125Gbaud.
> 5GBASE-R is one lane of 5.15625Gbaud.
> 
> Let's say you're using this, and test the 10G speed using XAUI,
> intending the other speeds to work. So you put in DT that you support
> four lanes and up to 5.15625Gbaud.

Yes, see, the blame's on you if you do that. You effectively declared
that the lane is able of sustaining a data rate higher than you've
actually had proof it does (5.156 vs 3.125).

The reason why I'm making this suggestion is because I think it lends
itself better to the way in which hardware manufacturers work.
A hobbyist like me has no choice than to test the highest data rate when
determining what frequency to declare in the DT (it's the same thing for
spi-max-frequency and other limilar DT properties, really), but hardware
people have simulations based on IBIS-AMI models, they can do SERDES
self-tests using PRBS patterns, lots of stuff to characterize what
frequency a lane is rated for, without actually speaking any Ethernet
protocol on it. In fact there are lots of people who can do this stuff
(which I know mostly nothing about) with precision without even knowing
how to even type a simple "ls" inside a Linux shell.

> Later, you discover that 5GBASE-R doesn't work because there's an
> electrical issue with the board. You now have DT in circulation
> which doesn't match the capabilities of the hardware.
> 
> How is this any different from the situation you describe above?
> To me, it seems to be exactly the same problem.

To err is human, of course. But one thing I think we learned from the
old implementation of phylink_validate is that it gets very tiring to
keep adding PHY modes, and we always seem to miss some. When that array
will be described in DT, it could be just a tad more painful to maintain.

> So, I don't think one can get away from these kinds of issues - where
> you create a board, do insufficient testing, publish a DT description,
> and then through further testing discover you have to restrict the
> hardware capabilities in DT. In fact, this is sadly an entirely normal
> process - problems always get found after boards have been sent out
> and initial DT has been created.
> 
> A good example is the 6th switch port on the original Clearfog boards.
> This was connected to an external PHY at address 0 on the MDIO bus
> behind the switch. However, the 88E6176 switch already has an internal
> PHY at address 0, so the external PHY can't be accessed. Consequently,
> port 6 is unreliable. That only came to light some time later, and
> resulted in the DT needing to be modified.

Sure, but this is a bit unrelated.

> There are always problems that need DT to be fixed - I don't think it's
> possible to get away from that by changing what or how something is
> described. In fact, I think trying to make that argument actually shows
> a misunderstanding of the process of hardware bringup.

Oh, how so?

> Just like software, hardware is buggy and takes time to be debugged,
> and that process continues after the first version of DT for a board
> has been produced, and there will always be changes required to DT.
> 
> I'm not saying that describing the maximum frequency and lanes doesn't
> have merit, I'm merely taking issue with the basis of your argument.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Marek Behún]

On Wed, 24 Nov 2021 14:04:41 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Wed, Nov 24, 2021 at 11:04:37AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 24, 2021 at 12:54:18AM +0200, Vladimir Oltean wrote:  
> > > This implies that when you bring up a board and write the device tree
> > > for it, you know that PHY mode X works without ever testing it. What if
> > > it doesn't work when you finally add support for it? Now you already
> > > have one DT blob in circulation. That's why I'm saying that maybe it
> > > could be better if we could think in terms that are a bit more physical
> > > and easy to characterize.  
> > 
> > However, it doesn't solve the problem. Let's take an example.
> > 
> > The 3310 supports a mode where it runs in XAUI/5GBASE-R/2500BASE-X/SGMII
> > depending on the negotiated media parameters.
> > 
> > XAUI is four lanes of 3.125Gbaud.
> > 5GBASE-R is one lane of 5.15625Gbaud.
> > 
> > Let's say you're using this, and test the 10G speed using XAUI,
> > intending the other speeds to work. So you put in DT that you support
> > four lanes and up to 5.15625Gbaud.  
> 
> Yes, see, the blame's on you if you do that.You effectively declared
> that the lane is able of sustaining a data rate higher than you've
> actually had proof it does (5.156 vs 3.125).

But the blame is on the DT writer in the same way if they declare
support for a PHY mode that wasn't tested. (Or at least self-tests with
PRBS patterns at given frequency.)

> The reason why I'm making this suggestion is because I think it lends
> itself better to the way in which hardware manufacturers work.
> A hobbyist like me has no choice than to test the highest data rate when
> determining what frequency to declare in the DT (it's the same thing for
> spi-max-frequency and other limilar DT properties, really), but hardware
> people have simulations based on IBIS-AMI models, they can do SERDES
> self-tests using PRBS patterns, lots of stuff to characterize what
> frequency a lane is rated for, without actually speaking any Ethernet
> protocol on it. In fact there are lots of people who can do this stuff
> (which I know mostly nothing about) with precision without even knowing
> how to even type a simple "ls" inside a Linux shell.
>
> > Later, you discover that 5GBASE-R doesn't work because there's an
> > electrical issue with the board. You now have DT in circulation
> > which doesn't match the capabilities of the hardware.
> > 
> > How is this any different from the situation you describe above?
> > To me, it seems to be exactly the same problem.  
> 
> To err is human, of course. But one thing I think we learned from the
> old implementation of phylink_validate is that it gets very tiring to
> keep adding PHY modes, and we always seem to miss some. When that array
> will be described in DT, it could be just a tad more painful to maintain.

The thing is that we will still need the `phy-mode` property, it can't
be deprecated IMO. There are non-SerDes modes, like rgmii, which may
use different pins than SerDes modes.
There may theoretically also be a SoC or PHY where the lanes for XAUI
do not share pins with the lane of 2500base-x, and this lane may not be
wired. Tis true that I don't know of any such hardware and it probably
does not and will not exist, but we don't know that for sure and this is
a case where your proposal will fail and the phy-mode extension would
work nicely.

Maybe we need opinions from other people here.

Marek

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 01:17:03PM +0100, Marek Behún wrote:
> On Wed, 24 Nov 2021 14:04:41 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > On Wed, Nov 24, 2021 at 11:04:37AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 24, 2021 at 12:54:18AM +0200, Vladimir Oltean wrote:  
> > > > This implies that when you bring up a board and write the device tree
> > > > for it, you know that PHY mode X works without ever testing it. What if
> > > > it doesn't work when you finally add support for it? Now you already
> > > > have one DT blob in circulation. That's why I'm saying that maybe it
> > > > could be better if we could think in terms that are a bit more physical
> > > > and easy to characterize.  
> > > 
> > > However, it doesn't solve the problem. Let's take an example.
> > > 
> > > The 3310 supports a mode where it runs in XAUI/5GBASE-R/2500BASE-X/SGMII
> > > depending on the negotiated media parameters.
> > > 
> > > XAUI is four lanes of 3.125Gbaud.
> > > 5GBASE-R is one lane of 5.15625Gbaud.
> > > 
> > > Let's say you're using this, and test the 10G speed using XAUI,
> > > intending the other speeds to work. So you put in DT that you support
> > > four lanes and up to 5.15625Gbaud.  
> > 
> > Yes, see, the blame's on you if you do that.You effectively declared
> > that the lane is able of sustaining a data rate higher than you've
> > actually had proof it does (5.156 vs 3.125).
> 
> But the blame is on the DT writer in the same way if they declare
> support for a PHY mode that wasn't tested. (Or at least self-tests with
> PRBS patterns at given frequency.)

I think we're running around in circles on this one. I think there's an
overlap between the supported_interfaces and the phy-mode array in
device tree. Going back to your example with the SMC calls unsupported
by ATF, you may run into the surprise that you have a phy-mode in the
device tree which you need to mask out in Linux, via supported_interfaces.
You would have needed to mask it out anyway, even with my proposal, so
you don't gain anything except the extra burden of spelling out 5 lines
of a phy-mode array in the device tree. It's going to be a nightmare for
review, it isn't obvious to say that "this phy-mode shouldn't be here"
or "you're missing this phy-mode".

> > The reason why I'm making this suggestion is because I think it lends
> > itself better to the way in which hardware manufacturers work.
> > A hobbyist like me has no choice than to test the highest data rate when
> > determining what frequency to declare in the DT (it's the same thing for
> > spi-max-frequency and other limilar DT properties, really), but hardware
> > people have simulations based on IBIS-AMI models, they can do SERDES
> > self-tests using PRBS patterns, lots of stuff to characterize what
> > frequency a lane is rated for, without actually speaking any Ethernet
> > protocol on it. In fact there are lots of people who can do this stuff
> > (which I know mostly nothing about) with precision without even knowing
> > how to even type a simple "ls" inside a Linux shell.
> >
> > > Later, you discover that 5GBASE-R doesn't work because there's an
> > > electrical issue with the board. You now have DT in circulation
> > > which doesn't match the capabilities of the hardware.
> > > 
> > > How is this any different from the situation you describe above?
> > > To me, it seems to be exactly the same problem.  
> > 
> > To err is human, of course. But one thing I think we learned from the
> > old implementation of phylink_validate is that it gets very tiring to
> > keep adding PHY modes, and we always seem to miss some. When that array
> > will be described in DT, it could be just a tad more painful to maintain.
> 
> The thing is that we will still need the `phy-mode` property, it can't
> be deprecated IMO.

Wait a minute, who said anything about deprecating it? I just said
"let's not make it an array, in the actual device tree". The phy-mode
was, and will remain, the initial MII-side protocol, which can or cannot
be changed at runtime.

> There are non-SerDes modes, like rgmii, which may use different pins
> than SerDes modes.

And, as discussed, there is no use case for dynamic switchover between
RGMII and a SERDES lane.

> There may theoretically also be a SoC or PHY where the lanes for XAUI
> do not share pins with the lane of 2500base-x, and this lane may not be
> wired. Tis true that I don't know of any such hardware and it probably
> does not and will not exist, but we don't know that for sure and this is
> a case where your proposal will fail and the phy-mode extension would
> work nicely.

We haven't gotten to discussing any of the details yet, but my proposal
of course would have been to describe the number of SERDES lanes and max
frequency in the COMPHY nodes, and have the COMPHY tell you what is
supported. At least with Layerscape, that would be the model. So when
you have pinmuxing, you just get the capabilities from the lane in use.

> 
> Maybe we need opinions from other people here.

Other opinions are welcome of course.

Re: [PATCH net-next v2 1/8] dt-bindings: ethernet-controller: support multiple PHY connection types

[Rob Herring]

On Tue, 23 Nov 2021 17:40:20 +0100, Marek Behún wrote:
> Sometimes, an ethernet PHY may communicate with ethernet controller with
> multiple different PHY connection types, and the software should be able
> to choose between them.
> 
> Russell King says:
>   conventionally phy-mode has meant "this is the mode we want to operate
>   the PHY interface in" which was fine when PHYs didn't change their
>   mode depending on the media speed
> This is no longer the case, since we have PHYs that can change PHY mode.
> 
> Existing example is the Marvell 88X3310 PHY, which supports connecting
> the MAC with the PHY with `xaui` and `rxaui`. The MAC may also support
> both modes, but it is possible that a particular board doesn't have
> these modes wired (since they use multiple SerDes lanes).
> 
> Another example is one SerDes lane capable of `1000base-x`, `2500base-x`
> and `sgmii` when connecting Marvell switches with Marvell ethernet
> controller. Currently we mention only one of these modes in device-tree,
> and software assumes the other modes are also supported, since they use
> the same SerDes lanes. But a board may be able to support `1000base-x`
> and not support `2500base-x`, for example due to the higher frequency
> not working correctly on a particular board.
> 
> In order for the kernel to know which modes are supported on the board,
> we need to be able to specify them all in the device-tree.
> 
> Change the type of property `phy-connection-type` of an ethernet
> controller to be an array of the enumerated strings, instead of just one
> string. Require at least one item defined.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/net/ethernet-controller.yaml     | 94 ++++++++++---------
>  1 file changed, 49 insertions(+), 45 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Marek Behún]

On Wed, 24 Nov 2021 14:31:35 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> > > To err is human, of course. But one thing I think we learned from the
> > > old implementation of phylink_validate is that it gets very tiring to
> > > keep adding PHY modes, and we always seem to miss some. When that array
> > > will be described in DT, it could be just a tad more painful to maintain.  
> > 
> > The thing is that we will still need the `phy-mode` property, it can't
> > be deprecated IMO.  
> 
> Wait a minute, who said anything about deprecating it? I just said
> "let's not make it an array, in the actual device tree". The phy-mode
> was, and will remain, the initial MII-side protocol, which can or cannot
> be changed at runtime.

Hello Vladimir,

I was told multiple times that device-tree should not specify how the
software should behave (given multiple HW options). This has not been
always followed, but it is preferred.

Now the 'phy-mode' property, if interpreted as "the initial MII-side
protocol" would break this rule.

This is therefore another reason why it should either be extended to an
array, or, if we went with your proposal of 'num-lanes' + 'max-freq',
deprecated (at least for serdes modes). But it can't be deprecated
entirely, IMO (because of non serdes protocols).

I thought more about your proposal of 'num-lanes' + 'max-freq' vs
extending 'phy-mode'.

- 'num-lanes' + 'max-freq' are IMO closer to the idea of device-tree,
  since they describe exactly how the parts of the device are connected
  to each other
- otherwise I think your argument against extending 'phy-mode' because
  of people declaring support for modes that weren't properly tested and
  would later be found broken is invalid, since the same could happen
  for 'num-lanes' + 'max-freq' properties
- the 'phy-mode' property already exists. I think if we went with the
  'num-lanes' + 'max-freq' proposal, we would need to deprecate
  'phy-mode' for serdes protocols (at least for situations where
  multiple modes can be used, since then 'phy-mode' would go against
  the idea of the rule I mentioned in first paragraph)

Vladimir, Rob has now given R-B for the 'phy-mode' extension patch.

I am wondering now what to do, since other people haven't given their
opinions here. Whether to re-send the series, and maybe start discussing
there, or waiting more.

Marek

Re: [PATCH net-next v2 4/8] net: phylink: update supported_interfaces with modes from fwnode

[Vladimir Oltean]

On Thu, Dec 02, 2021 at 11:25:50PM +0100, Marek Behún wrote:
> On Wed, 24 Nov 2021 14:31:35 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
>
> > > > To err is human, of course. But one thing I think we learned from the
> > > > old implementation of phylink_validate is that it gets very tiring to
> > > > keep adding PHY modes, and we always seem to miss some. When that array
> > > > will be described in DT, it could be just a tad more painful to maintain.
> > >
> > > The thing is that we will still need the `phy-mode` property, it can't
> > > be deprecated IMO.
> >
> > Wait a minute, who said anything about deprecating it? I just said
> > "let's not make it an array, in the actual device tree". The phy-mode
> > was, and will remain, the initial MII-side protocol, which can or cannot
> > be changed at runtime.
>
> Hello Vladimir,
>
> I was told multiple times that device-tree should not specify how the
> software should behave (given multiple HW options). This has not been
> always followed, but it is preferred.
>
> Now the 'phy-mode' property, if interpreted as "the initial MII-side
> protocol" would break this rule.

Isn't the rule already broken? Like it or not, this is exactly what the
phy-mode property is today. Even the compatibility mode added by your
patch right here _depends_ on that exact interpretation.

> This is therefore another reason why it should either be extended to an
> array,

The reason being what, exactly? To me it is still non sequitur.
I think you are saying that by listing all supported PHY modes, it
becomes less of a "hardware configuration" and more of a "hardware
description". But my point is that "all PHY modes" is a pretty moving
target, and it shouldn't be put in the device tree because that should
be rather stable.

> or, if we went with your proposal of 'num-lanes' + 'max-freq',
> deprecated (at least for serdes modes). But it can't be deprecated
> entirely, IMO (because of non serdes protocols).

You haven't exactly formulated why you think the implication is that
phy-mode could be deprecated, if kept "not an array". Because it can't.
If I understand you correctly, you are basically implying that in an
ideal world where all MAC drivers and all PHY drivers could list out all
their capabilities, a given MAC + PHY pair would just work out the
highest common SERDES protocol that can be electrically supported. But
as long as the Generic PHY driver is going to be a thing, this will be
technically impossible, because there isn't any IEEE standardized way of
determining SERDES protocols. So no, the phy-mode will continue to have
its role of specifying the initial MII-side protocol, which can or
cannot be changed at runtime.

Also, as a second point, my guess is that there will always be SERDES
blocks where changing the protocol at runtime is so complicated, or has
so many restrictions, that "wont't change" is indistinguishable from
"cant't change". And the network drivers that use these SERDES blocks
will therefore use the device tree to specify the phy-mode that the
SERDES lane is preconfigured for, which is not the same as describing
the capability, technically speaking, but is nonetheless virtually
indistinguishable. So I don't see that the phy-mode should become
deprecated either way.

>
> I thought more about your proposal of 'num-lanes' + 'max-freq' vs
> extending 'phy-mode'.
>
> - 'num-lanes' + 'max-freq' are IMO closer to the idea of device-tree,
>   since they describe exactly how the parts of the device are connected
>   to each other
> - otherwise I think your argument against extending 'phy-mode' because
>   of people declaring support for modes that weren't properly tested and
>   would later be found broken is invalid, since the same could happen
>   for 'num-lanes' + 'max-freq' properties

To reiterate my points:

- The worst cases of the phy-mode array vs the physical properties of
  the lanes are about just as bad, if you're set out to misuse them.
  I.e. you can first declare that a lane is capable of 1.125 GHz, then
  "oh, you know what, now I tested 3.125 GHz and that works too", then
  "as a matter of fact, 10.3125 GHz works too", then "hey, there are
  actually 4 lanes, not just one!". I'm sorry, nobody can help you if
  you do that.

- The best cases seem to be a bit more disproportionate on the other
  hand (i.e. when things are done "right" by device tree authors):
  With the phy-mode array, if you don't want to declare things that
  haven't been tested, it gets clunky: you'd have an ever increasing
  array of gibberish, and multiple versions of it in flight.
  On the other hand, doing the 'num-lanes' + 'max-freq' thing right
  would IMO mean that you validate the electrical parameters of the
  SERDES lane at the maximum intended and testable data rate and width.
  I truly believe this can be done orthogonally to actually testing an
  Ethernet protocol, which is why I'm actually suggesting it. This
  description can't bit rot no matter what you do, if it was done
  correctly (see above). It also appears easier to look at physical
  properties and say "yes, this looks about right", than looking at a
  potentially large phy-mode array where some modes are oddly missing
  and you don't really know why.

- If you discover that some phy-modes can't be supported despite the
  electrical parameters allowing it (your SMC firmware example), it
  becomes "not a device tree problem" if you don't put the phy-mode
  array there in the first place.

So my proposal simply comes from trying to optimize for the "doing
things right" case (hopefully this would also be the common case).
If we look at ways in which people will find ways to screw things up,
I'm sure that the sky is the limit.

> - the 'phy-mode' property already exists. I think if we went with the
>   'num-lanes' + 'max-freq' proposal, we would need to deprecate
>   'phy-mode' for serdes protocols (at least for situations where
>   multiple modes can be used, since then 'phy-mode' would go against
>   the idea of the rule I mentioned in first paragraph)

I don't understand this argument, I'm sorry. I only suggested that you
don't introduce a phy-mode array until there is a proven need for it.
You've said that your current use cases are fine with just the
"compatibility" mode where you extend the single phy-mode from the
device tree based on supported_interfaces. I don't think you _have_ to
introduce my suggested electrical parameters right now, either. It is
just an alternative solution to an unspecified problem.

But one other aspect that maybe should be considered is the fact that
your compatibility mode would introduce some weirdness: if the phy-mode
array in the device tree has exactly one element, then you say "oh, this
is an old device tree, let me fix this up based on supported_interfaces
and potentially change towards protocols that aren't present in the
device tree". But if the phy-mode array has more than one element, you
don't allow changing towards protocols outside of that array. That is
strange. I think _you_ are changing the meaning of the phy-mode property.
If you restrict the array of phy-modes via some other mechanism (like
dynamically through code that populates supported_interfaces based on
stuff like SMC firmware queries, lane electrical parameters), then
transforming the phy-mode property into an array has literally no benefits,
and there won't be any backwards compatibility problem to handle.

> Vladimir, Rob has now given R-B for the 'phy-mode' extension patch.
>
> I am wondering now what to do, since other people haven't given their
> opinions here. Whether to re-send the series, and maybe start discussing
> there, or waiting more.

Go with the approach you feel the most comfortable justifying, I have no
stake here. In fact I would be glad too if other people shared their opinion
on how to move things forward.