[RFC net-next PATCH 00/16] Add support for Xilinx PCS

This series adds experimental support for Xilinx PCS devices. It is
experimental because while I believe I have the Linux side mostly
sorted, I have yet to achieve any data transfer. Adding support for this
device has required some plumbing work related to PCSs in general, and I
would appreciate feedback in that area. In general, I have not tested
these changes outside of my particular setup, though I do have the
ability to test the macb changes using the internal PCS in the future.


Sean Anderson (16):
  dt-bindings: net: Add pcs property
  dt-bindings: net: Add binding for Xilinx PCS
  net: sfp: Fix typo in state machine debug string
  net: phylink: Move phylink_set_pcs before phylink_create
  net: phylink: Automatically attach PCS devices
  net: phylink: Add function for optionally adding a PCS
  net: phylink: Add helpers for c22 registers without MDIO
  net: macb: Clean up macb_validate
  net: macb: Move most of mac_prepare to mac_config
  net: macb: Move PCS settings to PCS callbacks
  net: macb: Support restarting PCS autonegotiation
  net: macb: Support external PCSs
  net: phy: Export get_phy_c22_id
  net: mdio: Add helper functions for accessing MDIO devices
  net: pcs: Add Xilinx PCS driver
  net: sfp: Add quirk to ignore PHYs

 .../bindings/net/ethernet-controller.yaml     |   5 +
 .../devicetree/bindings/net/xilinx,pcs.yaml   |  83 ++++
 MAINTAINERS                                   |   6 +
 drivers/net/ethernet/cadence/macb_main.c      | 375 +++++++++++-------
 drivers/net/pcs/Kconfig                       |  19 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-xilinx.c                  | 326 +++++++++++++++
 drivers/net/phy/phy_device.c                  |   3 +-
 drivers/net/phy/phylink.c                     | 335 ++++++++++++----
 drivers/net/phy/sfp-bus.c                     |  12 +-
 drivers/net/phy/sfp.c                         |   5 +-
 include/linux/mdio.h                          |  17 +
 include/linux/phy.h                           |   1 +
 include/linux/phylink.h                       |  17 +-
 14 files changed, 963 insertions(+), 242 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml
 create mode 100644 drivers/net/pcs/pcs-xilinx.c

-- 
2.25.1

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

Signed-off-by: Sean Anderson <redacted>
---

 .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index b0933a8c295a..def95fa6a315 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml

@@ -116,6 +116,11 @@ properties:
     $ref: "#/properties/phy-handle"
     deprecated: true
 
+  pcs:
+    $ref: /schemas/types.yaml#definitions/phandle
+    description:
+      Specifies a reference to a node representing a PCS device.
+
   rx-fifo-depth:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

We actually already have LX2160A platforms using "pcs-handle", (see
Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml) so we're
in danger of ending up with multiple property names describing the same
thing.

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

On 10/5/21 5:39 AM, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

quoted

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

We actually already have LX2160A platforms using "pcs-handle", (see
Documentation/devicetree/bindings/net/fsl,qoriq-mc-dpmac.yaml) so we're
in danger of ending up with multiple property names describing the same
thing.

Either way is fine by me.

--Sean

On Tue, Oct 05, 2021 at 10:39:36AM +0100, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

quoted

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

On 1G and up ethernet, we have 2 PHYs. There's the external (typically) 
ethernet PHY which is what the above properties are for. Then there's 
the on-chip serdes PHY similar to SATA, PCIe, etc. which includes the 
PCS part. For this part, we should use the generic PHY binding. I think 
we already have bindings doing that.

Rob

Hi Rob,

On 10/12/21 9:16 AM, Rob Herring wrote:

On Tue, Oct 05, 2021 at 10:39:36AM +0100, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

quoted

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

On 1G and up ethernet, we have 2 PHYs. There's the external (typically)
ethernet PHY which is what the above properties are for. Then there's
the on-chip serdes PHY similar to SATA, PCIe, etc. which includes the
PCS part. For this part, we should use the generic PHY binding. I think
we already have bindings doing that.

In the 802.3 models, there are several components which convert between
the MII (from the MAC) and the MDI (the physical protocol on the wire).
These are the Physical Coding Sublayer (PCS), Physical Medium Attachment
(PMA) sublayer, and Physical Medium Dependent (PMD) sublayer. The PMD
converts between the physical layer signaling and the on-chip (or
on-board) signalling. The PMA performs clock recovery and converts the
serial data from the PMD into parallel data for the PCS. The PCS handles
autonegotiation, CSMA/CD, and conversion to the apripriate MII for
communicating with the MAC.

In the above model, generic serdes devices generally correspond to the
PMA/PMD sublayers. The PCS is generally a separate device, both
on the hardware and software level. It provides an ethernet-specific
layer on top of the more generic underlying encoding. For this reason,
the PCS should be modeled as its own device, which may then contain a
reference to the appropriate serdes.

The above model describes physical layers such as 1000BASE-X or
10GBASE-X where the PCS/PMA/PMD is the last layer before the physical
medium. In that case, the PCS could be modeled as a traditional PHY.
However, when using (e.g.) SGMII, it is common for the "MDI" to be
SGMII, and for another PHY to convert to 1000BASE-T. To model this
correctly, the PCS/PMA/PMD layer must be considered independently from
the PHY which will ultimately convert the MII to the MDI.

--Sean

On Tue, Oct 12, 2021 at 11:18 AM Sean Anderson [off-list ref] wrote:

Hi Rob,

On 10/12/21 9:16 AM, Rob Herring wrote:

quoted

On Tue, Oct 05, 2021 at 10:39:36AM +0100, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

quoted

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

On 1G and up ethernet, we have 2 PHYs. There's the external (typically)
ethernet PHY which is what the above properties are for. Then there's
the on-chip serdes PHY similar to SATA, PCIe, etc. which includes the
PCS part. For this part, we should use the generic PHY binding. I think
we already have bindings doing that.

In the 802.3 models, there are several components which convert between
the MII (from the MAC) and the MDI (the physical protocol on the wire).
These are the Physical Coding Sublayer (PCS), Physical Medium Attachment
(PMA) sublayer, and Physical Medium Dependent (PMD) sublayer. The PMD
converts between the physical layer signaling and the on-chip (or
on-board) signalling. The PMA performs clock recovery and converts the
serial data from the PMD into parallel data for the PCS. The PCS handles
autonegotiation, CSMA/CD, and conversion to the apripriate MII for
communicating with the MAC.

In the above model, generic serdes devices generally correspond to the
PMA/PMD sublayers. The PCS is generally a separate device, both
on the hardware and software level. It provides an ethernet-specific
layer on top of the more generic underlying encoding. For this reason,
the PCS should be modeled as its own device, which may then contain a
reference to the appropriate serdes.

On the h/w I've worked on, PCS was an additional block instantiated
within the PHY, so it looked like one block to s/w. But that's been
almost 10 years ago now.

If you do have 2 h/w blocks, one option is doing something like this:

phys = <&pcs_phy>, <&sgmii_phy>;

I'm okay with 'pcs-handle', but just want to make sure we're not using
it where 'phys' would work.

The above model describes physical layers such as 1000BASE-X or
10GBASE-X where the PCS/PMA/PMD is the last layer before the physical
medium. In that case, the PCS could be modeled as a traditional PHY.
However, when using (e.g.) SGMII, it is common for the "MDI" to be
SGMII, and for another PHY to convert to 1000BASE-T. To model this
correctly, the PCS/PMA/PMD layer must be considered independently from
the PHY which will ultimately convert the MII to the MDI.

On 10/12/21 12:44 PM, Rob Herring wrote:

On Tue, Oct 12, 2021 at 11:18 AM Sean Anderson [off-list ref] wrote:

quoted

Hi Rob,

On 10/12/21 9:16 AM, Rob Herring wrote:

quoted

On Tue, Oct 05, 2021 at 10:39:36AM +0100, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:12PM -0400, Sean Anderson wrote:

quoted

Add a property for associating PCS devices with ethernet controllers.
Because PCS has no generic analogue like PHY, I have left off the
-handle suffix.

For PHYs, we used to have phy and phy-device as property names, but the
modern name is "phy-handle". I think we should do the same here, so I
would suggest using "pcs-handle".

On 1G and up ethernet, we have 2 PHYs. There's the external (typically)
ethernet PHY which is what the above properties are for. Then there's
the on-chip serdes PHY similar to SATA, PCIe, etc. which includes the
PCS part. For this part, we should use the generic PHY binding. I think
we already have bindings doing that.

In the 802.3 models, there are several components which convert between
the MII (from the MAC) and the MDI (the physical protocol on the wire).
These are the Physical Coding Sublayer (PCS), Physical Medium Attachment
(PMA) sublayer, and Physical Medium Dependent (PMD) sublayer. The PMD
converts between the physical layer signaling and the on-chip (or
on-board) signalling. The PMA performs clock recovery and converts the
serial data from the PMD into parallel data for the PCS. The PCS handles
autonegotiation, CSMA/CD, and conversion to the apripriate MII for
communicating with the MAC.

In the above model, generic serdes devices generally correspond to the
PMA/PMD sublayers. The PCS is generally a separate device, both
on the hardware and software level. It provides an ethernet-specific
layer on top of the more generic underlying encoding. For this reason,
the PCS should be modeled as its own device, which may then contain a
reference to the appropriate serdes.

On the h/w I've worked on, PCS was an additional block instantiated
within the PHY, so it looked like one block to s/w. But that's been
almost 10 years ago now.

Well, perhaps the line is not as clear as I made it seem above. The
PCS/PMA/PMD separation is mostly a logical one, so different platforms
divide up the work differently. In addition, the naming may not align
with ethernet's idea of what a PCS or PMA is. For example, on the
ZynqMP, the serdes (GTH) contains its own PCS and PMD (as shown on the
datasheet). However, these blocks both correspond to the PMA layer from
ethernet's POV. The ethernet PCS is a block controlled via registers
within the MAC's address space.

If you do have 2 h/w blocks, one option is doing something like this:

phys = <&pcs_phy>, <&sgmii_phy>;

Generally, PCSs don't export the same interface as PHYs (see e.g.
drivers/net/pcs and include/linux/phylink.h). IMO it would be an abuse
of the phys property to use it for a PCS as well.

I'm okay with 'pcs-handle', but just want to make sure we're not using
it where 'phys' would work.

quoted

The above model describes physical layers such as 1000BASE-X or
10GBASE-X where the PCS/PMA/PMD is the last layer before the physical
medium. In that case, the PCS could be modeled as a traditional PHY.
However, when using (e.g.) SGMII, it is common for the "MDI" to be
SGMII, and for another PHY to convert to 1000BASE-T. To model this
correctly, the PCS/PMA/PMD layer must be considered independently from
the PHY which will ultimately convert the MII to the MDI.

This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII
LogiCORE IP. This device is a soft device typically used to adapt between
GMII and SGMII or 1000BASE-X (in combination with a suitable SERDES). The
standard property is roughly analogous to the interface property of
ethernet controllers, except that it has an additional value used to
indicate that dynamic switching is supported. Note that switching is
supported only between SGMII and 1000BASE-X, and only if the appropriate
parameter is set when the device is synthesized. The property name was
chosen to align with the terminology in the datasheet. I also considered
"mdi", but that is a bit of a misnomer in the case of SGMII.

Signed-off-by: Sean Anderson <redacted>
---

 .../devicetree/bindings/net/xilinx,pcs.yaml   | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml

diff --git a/Documentation/devicetree/bindings/net/xilinx,pcs.yaml b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml
new file mode 100644
index 000000000000..43750dcb4b11
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml

@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/xilinx,pcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP
+
+maintainers:
+  - Sean Anderson <sean.anderson@seco.com>
+
+description:
+  This is a soft device which converts between GMII and SGMII, 2.5G SGMII,
+  1000BASE-X, or 2500BASE-X. It may have an attached SERDES, or may talk
+  directly to LVDS.
+
+allOf:
+  - $ref: "ethernet-controller.yaml#"
+
+properties:
+  compatible:
+    contains:
+      const:
+        - xilinx,pcs-16.2
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    items:
+      - description: The reference clock for the PMD, which is typically a
+                     SERDES but may be a direct interface to LVDS I/Os.
+                     Depending on your setup, this may be the gtrefclk, refclk,
+                     or clk125m signal.
+
+  clock-names:
+    const: refclk
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: pcs
+
+  standard:
+    description:
+      The interface standard that the PCS supports. The sgmii/1000base-x
+      setting indicates that the PCS supports dynamically switching between
+      SGMII and 1000BASE-X.
+    enum:
+      - sgmii
+      - 1000base-x
+      - sgmii/1000base-x
+      - 2500base-x
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - standard
+
+additionalProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pcs0: ethernet-pcs@0 {
+            compatible = "xlnx,pcs-16.2";
+            reg = <0>;
+            clocks = <&si570>;
+            clock-names = "refclk";
+            resets = <&pcs_reset 1>;
+            reset-names = "pcs";
+            standard = "sgmii/1000base-x";
+        };
+    };

-- 
2.25.1

On Mon, 04 Oct 2021 15:15:13 -0400, Sean Anderson wrote:

This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII
LogiCORE IP. This device is a soft device typically used to adapt between
GMII and SGMII or 1000BASE-X (in combination with a suitable SERDES). The
standard property is roughly analogous to the interface property of
ethernet controllers, except that it has an additional value used to
indicate that dynamic switching is supported. Note that switching is
supported only between SGMII and 1000BASE-X, and only if the appropriate
parameter is set when the device is synthesized. The property name was
chosen to align with the terminology in the datasheet. I also considered
"mdi", but that is a bit of a misnomer in the case of SGMII.

Signed-off-by: Sean Anderson <redacted>
---

 .../devicetree/bindings/net/xilinx,pcs.yaml   | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/xilinx,pcs.yaml: properties:compatible:contains:const: ['xilinx,pcs-16.2'] is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/xilinx,pcs.yaml: properties:clocks: {'maxItems': 1, 'items': [{'description': 'The reference clock for the PMD, which is typically a SERDES but may be a direct interface to LVDS I/Os. Depending on your setup, this may be the gtrefclk, refclk, or clk125m signal.'}]} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/xilinx,pcs.yaml: properties:clocks: 'oneOf' conditional failed, one must be fixed:
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/xilinx,pcs.yaml: properties:clocks: 'anyOf' conditional failed, one must be fixed:
		'items' is not one of ['maxItems', 'description', 'deprecated']
			hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
		'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
		'items' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
		1 is less than the minimum of 2
			hint: Arrays must be described with a combination of minItems/maxItems/items
		hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
		from schema $id: http://devicetree.org/meta-schemas/clocks.yaml#
	'maxItems' is not one of ['type', 'description', 'dependencies', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'allOf', 'anyOf', 'oneOf', '$ref']
	'items' is not one of ['type', 'description', 'dependencies', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'allOf', 'anyOf', 'oneOf', '$ref']
	'type' is a required property
		hint: DT nodes ("object" type in schemas) can only use a subset of json-schema keywords
	from schema $id: http://devicetree.org/meta-schemas/clocks.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/xilinx,pcs.yaml: ignoring, error in schema: properties: compatible: contains: const
warning: no schema found in file: ./Documentation/devicetree/bindings/net/xilinx,pcs.yaml
Documentation/devicetree/bindings/net/xilinx,pcs.example.dt.yaml:0:0: /example-0/mdio/ethernet-pcs@0: failed to match any schema with compatible: ['xlnx,pcs-16.2']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1536331

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

The string should be "tx_disable" to match the state enum.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/sfp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 34e90216bd2c..ab77a9f439ef 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c

@@ -134,7 +134,7 @@ static const char * const sm_state_strings[] = {
 	[SFP_S_LINK_UP] = "link_up",
 	[SFP_S_TX_FAULT] = "tx_fault",
 	[SFP_S_REINIT] = "reinit",
-	[SFP_S_TX_DISABLE] = "rx_disable",
+	[SFP_S_TX_DISABLE] = "tx_disable",
 };
 
 static const char *sm_state_to_str(unsigned short sm_state)

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:14PM -0400, Sean Anderson wrote:

Hi Sean

This is a bug fix. Please separate it out and base it on net, not
net-next.

Fixes: 4005a7cb4f55 ("net: phy: sftp: print debug message with text, not numbers")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

This adds support for automatically attaching PCS devices when creating
a phylink. To do this, drivers must first register with
phylink_register_pcs. After that, new phylinks will attach the PCS
device specified by the "pcs" property.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

I believe this is mostly correct with regard to registering/
unregistering. However I am not too familiar with the guts of Linux's
device subsystem. It is possible (likely, even) that the current system
is insufficient to prevent removing PCS devices which are still in-use.
I would really appreciate any feedback, or suggestions of subsystems to
use as reference. In particular: do I need to manually create device
links? Should I instead add an entry to of_supplier_bindings? Do I need
a call to try_module_get?

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/phylink.c | 115 ++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |  11 +++-
 2 files changed, 120 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6387c40c5592..046fdac3597d 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -795,6 +795,42 @@ static int phylink_register_sfp(struct phylink *pl,
 	return ret;
 }
 
+static LIST_HEAD(pcs_devices);
+static DEFINE_MUTEX(pcs_mutex);
+
+/**
+ * phylink_register_pcs() - register a new PCS
+ * @pcs: the PCS to register
+ *
+ * Registers a new PCS which can be automatically attached to a phylink.
+ *
+ * Return: 0 on success, or -errno on error
+ */
+int phylink_register_pcs(struct phylink_pcs *pcs)
+{
+	if (!pcs->dev || !pcs->ops)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&pcs->list);
+	mutex_lock(&pcs_mutex);
+	list_add(&pcs->list, &pcs_devices);
+	mutex_unlock(&pcs_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(phylink_register_pcs);
+
+/**
+ * phylink_unregister_pcs() - unregister a PCS
+ * @pcs: a PCS previously registered with phylink_register_pcs()
+ */
+void phylink_unregister_pcs(struct phylink_pcs *pcs)
+{
+	mutex_lock(&pcs_mutex);
+	list_del(&pcs->list);
+	mutex_unlock(&pcs_mutex);
+}
+EXPORT_SYMBOL_GPL(phylink_unregister_pcs);
+
 /**
  * phylink_set_pcs() - set the current PCS for phylink to use
  * @pl: a pointer to a &struct phylink returned from phylink_create()

@@ -808,14 +844,72 @@ static int phylink_register_sfp(struct phylink *pl,
  * callback if a PCS is present (denoting a newer setup) so removing a PCS
  * is not supported, and if a PCS is going to be used, it must be registered
  * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ *
+ * Context: may sleep.
+ * Return: 0 on success or -errno on failure.
  */
-void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
+int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 {
+	if (pl->pcs && pl->pcs->dev)
+		device_link_remove(pl->dev, pl->pcs->dev);
+
+	if (pcs->dev) {
+		struct device_link *dl =
+			device_link_add(pl->dev, pcs->dev, 0);
+
+		if (IS_ERR(dl)) {
+			dev_err(pl->dev,
+				"failed to create device link to %s\n",
+				dev_name(pcs->dev));
+			return PTR_ERR(dl);
+		}
+	}
+
 	pl->pcs = pcs;
 	pl->pcs_ops = pcs->ops;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);
 
+static struct phylink_pcs *phylink_find_pcs(struct fwnode_handle *fwnode)
+{
+	struct phylink_pcs *pcs;
+
+	mutex_lock(&pcs_mutex);
+	list_for_each_entry(pcs, &pcs_devices, list) {
+		if (pcs->dev && pcs->dev->fwnode == fwnode) {
+			mutex_unlock(&pcs_mutex);
+			return pcs;
+		}
+	}
+	mutex_unlock(&pcs_mutex);
+
+	return NULL;
+}
+
+static int phylink_attach_pcs(struct phylink *pl, struct fwnode_handle *fwnode)
+{
+	int ret;
+	struct phylink_pcs *pcs;
+	struct fwnode_reference_args ref;
+
+	ret = fwnode_property_get_reference_args(fwnode, "pcs", NULL,
+						 0, 0, &ref);
+	if (ret == -ENOENT)
+		return 0;
+	else if (ret)
+		return ret;
+
+	pcs = phylink_find_pcs(ref.fwnode);
+	if (pcs)
+		ret = phylink_set_pcs(pl, pcs);
+	else
+		ret = -EPROBE_DEFER;
+
+	fwnode_handle_put(ref.fwnode);
+	return ret;
+}
+
 /**
  * phylink_create() - create a phylink instance
  * @config: a pointer to the target &struct phylink_config

@@ -893,12 +987,20 @@ struct phylink *phylink_create(struct phylink_config *config,
 	pl->cur_link_an_mode = pl->cfg_link_an_mode;
 
 	ret = phylink_register_sfp(pl, fwnode);
-	if (ret < 0) {
-		kfree(pl);
-		return ERR_PTR(ret);
-	}
+	if (ret < 0)
+		goto err_sfp;
+
+	ret = phylink_attach_pcs(pl, fwnode);
+	if (ret)
+		goto err_pcs;
 
 	return pl;
+
+err_pcs:
+	sfp_bus_del_upstream(pl->sfp_bus);
+err_sfp:
+	kfree(pl);
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(phylink_create);
 

@@ -913,6 +1015,9 @@ EXPORT_SYMBOL_GPL(phylink_create);
  */
 void phylink_destroy(struct phylink *pl)
 {
+	if (pl->pcs && pl->pcs->dev)
+		device_link_remove(pl->dev, pl->pcs->dev);
+
 	sfp_bus_del_upstream(pl->sfp_bus);
 	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 237291196ce2..d60756b36ad3 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -331,14 +331,20 @@ struct phylink_pcs_ops;
 
 /**
  * struct phylink_pcs - PHYLINK PCS instance
+ * @dev: the device associated with this PCS, or %NULL if the PCS doesn't have
+ *       a device of its own. Typically, @dev should only be %NULL for internal
+ *       PCS devices which do not need to be looked up via phandle.
  * @ops: a pointer to the &struct phylink_pcs_ops structure
+ * @list: internal list of PCS devices
  * @poll: poll the PCS for link changes
  *
  * This structure is designed to be embedded within the PCS private data,
  * and will be passed between phylink and the PCS.
  */
 struct phylink_pcs {
+	struct device *dev;
 	const struct phylink_pcs_ops *ops;
+	struct list_head list;
 	bool poll;
 };
 

@@ -433,10 +439,13 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		 phy_interface_t interface, int speed, int duplex);
 #endif
 
+int phylink_register_pcs(struct phylink_pcs *pcs);
+void phylink_unregister_pcs(struct phylink_pcs *pcs);
+int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs);
+
 struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *mac_ops);
-void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:

This adds support for automatically attaching PCS devices when creating
a phylink. To do this, drivers must first register with
phylink_register_pcs. After that, new phylinks will attach the PCS
device specified by the "pcs" property.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

I believe this is mostly correct with regard to registering/
unregistering. However I am not too familiar with the guts of Linux's
device subsystem. It is possible (likely, even) that the current system
is insufficient to prevent removing PCS devices which are still in-use.
I would really appreciate any feedback, or suggestions of subsystems to
use as reference. In particular: do I need to manually create device
links? Should I instead add an entry to of_supplier_bindings? Do I need
a call to try_module_get?

I think this is an area that needs to be thought about carefully.
Things are not trivial here.

The first mistake I see below is the use of device links. pl->dev is
the "struct device" embedded within "struct net_device". This doesn't
have a driver associated with it, and so using device links is likely
ineffectual.

Even with the right device, I think careful thought is needed - we have
network drivers where one "struct device" contains multiple network
interfaces. Should the removal of a PCS from one network interface take
out all of them?

Alternatively, could we instead use phylink to "unplug" the PCS and
mark the link down - would that be a better approach than trying to
use device links?

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

On 10/5/21 5:48 AM, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:

quoted

This adds support for automatically attaching PCS devices when creating
a phylink. To do this, drivers must first register with
phylink_register_pcs. After that, new phylinks will attach the PCS
device specified by the "pcs" property.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

I believe this is mostly correct with regard to registering/
unregistering. However I am not too familiar with the guts of Linux's
device subsystem. It is possible (likely, even) that the current system
is insufficient to prevent removing PCS devices which are still in-use.
I would really appreciate any feedback, or suggestions of subsystems to
use as reference. In particular: do I need to manually create device
links? Should I instead add an entry to of_supplier_bindings? Do I need
a call to try_module_get?

I think this is an area that needs to be thought about carefully.
Things are not trivial here.

The first mistake I see below is the use of device links. pl->dev is
the "struct device" embedded within "struct net_device". This doesn't
have a driver associated with it, and so using device links is likely
ineffectual.

So what can the device in net_device be used for?

Even with the right device, I think careful thought is needed - we have
network drivers where one "struct device" contains multiple network
interfaces. Should the removal of a PCS from one network interface take
out all of them?

Well, it's more of the other way around. We need to prevent removing the
PCS while it is still in-use.

Alternatively, could we instead use phylink to "unplug" the PCS and
mark the link down - would that be a better approach than trying to
use device links?

So here, I think the logic should be: allow phylink to "unplug" the PCS
only when the link is down.

--Sean

On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:

On 10/5/21 5:48 AM, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:

quoted

This adds support for automatically attaching PCS devices when creating
a phylink. To do this, drivers must first register with
phylink_register_pcs. After that, new phylinks will attach the PCS
device specified by the "pcs" property.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

I believe this is mostly correct with regard to registering/
unregistering. However I am not too familiar with the guts of Linux's
device subsystem. It is possible (likely, even) that the current system
is insufficient to prevent removing PCS devices which are still in-use.
I would really appreciate any feedback, or suggestions of subsystems to
use as reference. In particular: do I need to manually create device
links? Should I instead add an entry to of_supplier_bindings? Do I need
a call to try_module_get?

I think this is an area that needs to be thought about carefully.
Things are not trivial here.

The first mistake I see below is the use of device links. pl->dev is
the "struct device" embedded within "struct net_device". This doesn't
have a driver associated with it, and so using device links is likely
ineffectual.

So what can the device in net_device be used for?

That is used for the class device that is commonly found in
/sys/devices/$pathtothedevice/net/$interfacename

quoted

Even with the right device, I think careful thought is needed - we have
network drivers where one "struct device" contains multiple network
interfaces. Should the removal of a PCS from one network interface take
out all of them?

Well, it's more of the other way around. We need to prevent removing the
PCS while it is still in-use.

devlinks don't do that - if the "producer" device goes away, they force
the "consumer" device to be unbound.

As I mention above, the "consumer" device, which would be the device
providing the network interface(s) could have more than one interface
and unbinding it could have drastic consequences for the platform.

quoted

Alternatively, could we instead use phylink to "unplug" the PCS and
mark the link down - would that be a better approach than trying to
use device links?

So here, I think the logic should be: allow phylink to "unplug" the PCS
only when the link is down.

When a device is unbound from its driver, the driver has no say in
whether that goes ahead or not. Think about it as grabbing that USB
stick plugged into your computer and you yanking it out. None of the
software gets a look in to say "don't do that".

phylink (or any other subsystem) does not have the power to say
"I don't want XYZ to be removed".

Yes, it's harder to do that with PCS, but my point is that if one asks
the driver model to unbind the PCS driver from the PCS device, then
the driver model will do that whether the PCS driver wants to allow it
at that moment or not. It isn't something the PCS driver can prevent.

One can tell the driver model not to expose the bind/unbind attributes
for the driver, but that doesn't stop the unbind happening should the
struct device actually go away.

So, IMHO, it's better to design assuming that components will go away
at an inconvenient time and deal with it gracefully.

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

On 10/7/21 6:23 AM, Russell King (Oracle) wrote:

On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:

quoted

On 10/5/21 5:48 AM, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:

quoted

This adds support for automatically attaching PCS devices when creating
a phylink. To do this, drivers must first register with
phylink_register_pcs. After that, new phylinks will attach the PCS
device specified by the "pcs" property.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

I believe this is mostly correct with regard to registering/
unregistering. However I am not too familiar with the guts of Linux's
device subsystem. It is possible (likely, even) that the current system
is insufficient to prevent removing PCS devices which are still in-use.
I would really appreciate any feedback, or suggestions of subsystems to
use as reference. In particular: do I need to manually create device
links? Should I instead add an entry to of_supplier_bindings? Do I need
a call to try_module_get?

I think this is an area that needs to be thought about carefully.
Things are not trivial here.

The first mistake I see below is the use of device links. pl->dev is
the "struct device" embedded within "struct net_device". This doesn't
have a driver associated with it, and so using device links is likely
ineffectual.

Ok, so the 'real' device is actually the parent of pl->netdev->dev?

quoted

So what can the device in net_device be used for?

That is used for the class device that is commonly found in
/sys/devices/$pathtothedevice/net/$interfacename

By the way, why don't we set pl->dev = config->dev->parent in
phylink_create() when config->type == PHYLINK_NETDEV?

quoted

quoted

Even with the right device, I think careful thought is needed - we have
network drivers where one "struct device" contains multiple network
interfaces. Should the removal of a PCS from one network interface take
out all of them?

Well, it's more of the other way around. We need to prevent removing the
PCS while it is still in-use.

devlinks don't do that - if the "producer" device goes away, they force
the "consumer" device to be unbound.

Ah, I didn't realize that was the relationship being modeled.

As I mention above, the "consumer" device, which would be the device
providing the network interface(s) could have more than one interface
and unbinding it could have drastic consequences for the platform.

Well, then don't unbind the PCS ;)

After reviewing several other subsystems, I think the correct way to
approach this is to add an entry to of_supplier_bindings, which will
help out with ordering, and get the module when looking up the PCS. That
is, something like

int phylink_get_pcs(fwnode, struct phylink_pcs **pcs)
{
	int ret;
	struct fwnode_reference_args ref;

	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle", NULL,
						 0, 0, &ref);
	if (ret)
		return ret;

	*pcs = phylink_find_pcs(ref.fwnode);
	fwnode_handle_put(ref.fwnode);
	if (!*pcs)
		return -EPROBE_DEFER;

	if (!try_module_get(*pcs->owner))
		return -EBUSY;
	return 0;
}

phylink_put_pcs(pcs)
{
	module_put(pcs->owner);
}

and keep phylink_set as-is (the above should be considered along with my comments on patch 10).

Realistically, the only time a PCS is optional is if there isn't a PCS
reference in the device tree.

quoted

quoted

Alternatively, could we instead use phylink to "unplug" the PCS and
mark the link down - would that be a better approach than trying to
use device links?

So here, I think the logic should be: allow phylink to "unplug" the PCS
only when the link is down.

When a device is unbound from its driver, the driver has no say in
whether that goes ahead or not. Think about it as grabbing that USB
stick plugged into your computer and you yanking it out. None of the
software gets a look in to say "don't do that".

I suspect the vast majority of PCSs will be DEVICE_FIXED.

phylink (or any other subsystem) does not have the power to say
"I don't want XYZ to be removed".

However, we do have the power to say "I don't want XYZ's module to be
removed", which should cover most of the situations where a device is
removed after boot.

Yes, it's harder to do that with PCS, but my point is that if one asks
the driver model to unbind the PCS driver from the PCS device, then
the driver model will do that whether the PCS driver wants to allow it
at that moment or not. It isn't something the PCS driver can prevent.

One can tell the driver model not to expose the bind/unbind attributes
for the driver, but that doesn't stop the unbind happening should the
struct device actually go away.

So, IMHO, it's better to design assuming that components will go away
at an inconvenient time and deal with it gracefully.

See above, but I think it's better here to assume that components will
stick around and if they disappear at an inconvenient time then we
should just let the netdev be removed as well.

--Sean

This adds a function to set the PCS only if there is not one currently
set. The intention here is to allow MAC drivers to have a "default" PCS
(such as an internal one) which may be used when one has not been set
via the device tree. This allows for backwards compatibility for cases
where a PCS was automatically attached if necessary.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/phylink.c | 26 ++++++++++++++++++++++++++
 include/linux/phylink.h   |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 046fdac3597d..f82dc0f87f40 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -871,6 +871,32 @@ int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
 }
 EXPORT_SYMBOL_GPL(phylink_set_pcs);
 
+/**
+ * phylink_set_pcs_weak() - optionally set the current PCS for phylink to use
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @pcs: a pointer to the &struct phylink_pcs
+ *
+ * Bind the MAC PCS to phylink only if there is no currently-bound PCS. This
+ * may be called to set a "default" PCS, such as an internal PCS which was
+ * previously handled by the MAC driver directly. Otherwise, this function
+ * behaves like phylink_set_pcs();
+ *
+ * Context: may sleep.
+ * Return: 1 if the PCS was set, 0 if it was not, or -errno on failure.
+ */
+int phylink_set_pcs_weak(struct phylink *pl, struct phylink_pcs *pcs)
+{
+	int ret;
+
+	if (!pl->pcs) {
+		ret = phylink_set_pcs(pl, pcs);
+		return ret ? ret : 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(phylink_set_pcs_weak);
+
 static struct phylink_pcs *phylink_find_pcs(struct fwnode_handle *fwnode)
 {
 	struct phylink_pcs *pcs;

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d60756b36ad3..bd0ce707d098 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -442,6 +442,7 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 int phylink_register_pcs(struct phylink_pcs *pcs);
 void phylink_unregister_pcs(struct phylink_pcs *pcs);
 int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs);
+int phylink_set_pcs_weak(struct phylink *pl, struct phylink_pcs *pcs);
 
 struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 			       phy_interface_t iface,

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote:

This adds a function to set the PCS only if there is not one currently
set. The intention here is to allow MAC drivers to have a "default" PCS
(such as an internal one) which may be used when one has not been set
via the device tree. This allows for backwards compatibility for cases
where a PCS was automatically attached if necessary.

I'm not sure I entirely like this approach. Why can't the network
driver check for the pcs-handle property and avoid using its
"default" if present?

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

On Tue, Oct 05, 2021 at 10:51:38AM +0100, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote:

quoted

This adds a function to set the PCS only if there is not one currently
set. The intention here is to allow MAC drivers to have a "default" PCS
(such as an internal one) which may be used when one has not been set
via the device tree. This allows for backwards compatibility for cases
where a PCS was automatically attached if necessary.

I'm not sure I entirely like this approach. Why can't the network
driver check for the pcs-handle property and avoid using its
"default" if present?

And that would also be in line with

ethernet/freescale/dpaa2/dpaa2-mac.c:	node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);

We need a uniform meaning of pcs-handle. And dpaa2-mac.c has set the
precedent that the MAC uses it, not phylink. That can however be
changed, if it make sense, but both users should do the same.

	 Andrew

On 10/5/21 9:43 AM, Andrew Lunn wrote:

On Tue, Oct 05, 2021 at 10:51:38AM +0100, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote:

quoted

This adds a function to set the PCS only if there is not one currently
set. The intention here is to allow MAC drivers to have a "default" PCS
(such as an internal one) which may be used when one has not been set
via the device tree. This allows for backwards compatibility for cases
where a PCS was automatically attached if necessary.

I'm not sure I entirely like this approach. Why can't the network
driver check for the pcs-handle property and avoid using its
"default" if present?

And that would also be in line with

ethernet/freescale/dpaa2/dpaa2-mac.c:	node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);

We need a uniform meaning of pcs-handle. And dpaa2-mac.c has set the
precedent that the MAC uses it, not phylink. That can however be
changed, if it make sense, but both users should do the same.

The tricky bit here happens when drivers set the PCS in mac_prepare()
depending on the interface. So you have to remember during probe()
whether you are supposed to set the PCS for later. I would like to leave
more of this to phylink to ensure that the process is done in a uniform
way.

--Sean

Some devices expose memory-mapped c22-compliant PHYs. Because these
devices do not have an MDIO bus, we cannot use the existing helpers.
Refactor the existing helpers to allow supplying the values for c22
registers directly, instead of using MDIO to access them. Only get_state
and set_adversisement are converted, since they contain the most complex
logic.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/phylink.c | 154 ++++++++++++++++++++++----------------
 include/linux/phylink.h   |   5 ++
 2 files changed, 96 insertions(+), 63 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f82dc0f87f40..1b6077557e31 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -2639,6 +2639,49 @@ void phylink_decode_usxgmii_word(struct phylink_link_state *state,
 }
 EXPORT_SYMBOL_GPL(phylink_decode_usxgmii_word);
 
+/**
+ * phylink_mii_c22_pcs_decode_state() - Decode MAC PCS state from MII registers
+ * @state: a pointer to a &struct phylink_link_state.
+ * @bmsr: The value of the %MII_BMSR register
+ * @lpa: The value of the %MII_LPA register
+ *
+ * Helper for MAC PCS supporting the 802.3 clause 22 register set for
+ * clause 37 negotiation and/or SGMII control.
+ *
+ * Parse the Clause 37 or Cisco SGMII link partner negotiation word into
+ * the phylink @state structure. This is suitable to be used for implementing
+ * the mac_pcs_get_state() member of the struct phylink_mac_ops structure if
+ * accessing @bmsr and @lpa cannot be done with MDIO directly.
+ */
+void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
+				      u16 bmsr, u16 lpa)
+{
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+	if (!state->link)
+		return;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		phylink_decode_c37_word(state, lpa, SPEED_1000);
+		break;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		phylink_decode_c37_word(state, lpa, SPEED_2500);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		phylink_decode_sgmii_word(state, lpa);
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_decode_state);
+
 /**
  * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
  * @pcs: a pointer to a &struct mdio_device.

@@ -2667,32 +2710,48 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 		return;
 	}
 
-	state->link = !!(bmsr & BMSR_LSTATUS);
-	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
-	if (!state->link)
-		return;
-
-	switch (state->interface) {
-	case PHY_INTERFACE_MODE_1000BASEX:
-		phylink_decode_c37_word(state, lpa, SPEED_1000);
-		break;
-
-	case PHY_INTERFACE_MODE_2500BASEX:
-		phylink_decode_c37_word(state, lpa, SPEED_2500);
-		break;
-
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		phylink_decode_sgmii_word(state, lpa);
-		break;
-
-	default:
-		state->link = false;
-		break;
-	}
+	phylink_mii_c22_pcs_decode_state(state, bmsr, lpa);
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
 
+/**
+ * phylink_mii_c22_pcs_encode_advertisement() - configure the clause 37 PCS
+ *	advertisement
+ * @interface: the PHY interface mode being configured
+ * @advertising: the ethtool advertisement mask
+ * @adv: the value of the %MII_ADVERTISE register
+ *
+ * Helper for MAC PCS supporting the 802.3 clause 22 register set for
+ * clause 37 negotiation and/or SGMII control.
+ *
+ * Encode the clause 37 PCS advertisement as specified by @interface and
+ * @advertising. Callers should write @adv if it has been modified.
+ *
+ * Return: The new value for @adv.
+ */
+u16 phylink_mii_c22_pcs_encode_advertisement(phy_interface_t interface,
+					     const unsigned long *advertising,
+					     u16 adv)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		adv = ADVERTISE_1000XFULL;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      advertising))
+			adv |= ADVERTISE_1000XPAUSE;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				      advertising))
+			adv |= ADVERTISE_1000XPSE_ASYM;
+		return adv;
+	case PHY_INTERFACE_MODE_SGMII:
+		return 0x0001;
+	default:
+		/* Nothing to do for other modes */
+		return adv;
+	}
+}
+
 /**
  * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
  *	advertisement

@@ -2719,48 +2778,17 @@ int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
 	int val, ret;
 	u16 adv;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_1000BASEX:
-	case PHY_INTERFACE_MODE_2500BASEX:
-		adv = ADVERTISE_1000XFULL;
-		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-				      advertising))
-			adv |= ADVERTISE_1000XPAUSE;
-		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-				      advertising))
-			adv |= ADVERTISE_1000XPSE_ASYM;
+	val = mdiobus_read(bus, addr, MII_ADVERTISE);
+	if (val < 0)
+		return val;
 
-		val = mdiobus_read(bus, addr, MII_ADVERTISE);
-		if (val < 0)
-			return val;
-
-		if (val == adv)
-			return 0;
-
-		ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
-		if (ret < 0)
-			return ret;
-
-		return 1;
-
-	case PHY_INTERFACE_MODE_SGMII:
-		val = mdiobus_read(bus, addr, MII_ADVERTISE);
-		if (val < 0)
-			return val;
-
-		if (val == 0x0001)
-			return 0;
-
-		ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
-		if (ret < 0)
-			return ret;
-
-		return 1;
-
-	default:
-		/* Nothing to do for other modes */
+	adv = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
+						       adv);
+	if (adv == val)
 		return 0;
-	}
+
+	ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
+	return ret < 0 ? ret : 1;
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
 

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bd0ce707d098..b28ed8b569ee 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -496,8 +496,13 @@ int phylink_speed_up(struct phylink *pl);
 void phylink_set_port_modes(unsigned long *bits);
 void phylink_helper_basex_speed(struct phylink_link_state *state);
 
+void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
+				      u16 bmsr, u16 lpa);
 void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 				   struct phylink_link_state *state);
+u16 phylink_mii_c22_pcs_encode_advertisement(phy_interface_t interface,
+					     const unsigned long *advertising,
+					     u16 adv);
 int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
 					  phy_interface_t interface,
 					  const unsigned long *advertising);

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:18PM -0400, Sean Anderson wrote:

Some devices expose memory-mapped c22-compliant PHYs. Because these
devices do not have an MDIO bus, we cannot use the existing helpers.
Refactor the existing helpers to allow supplying the values for c22
registers directly, instead of using MDIO to access them. Only get_state
and set_adversisement are converted, since they contain the most complex
logic.

I think this patch is useful on its own, there are certainly cases
where being able to hold the MII bus lock while reading the BMSR/LPA
registers would be advantageous. Please can you update the patch
against net-next and submit it?

Thanks.

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

As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.

While we're on the subject, could someone clarify the relationship
between the various speed capabilities? What's the difference between
MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS,
and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE?
HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think
this one is a bug, because it cares later on)?

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/ethernet/cadence/macb_main.c | 99 +++++++++++-------------
 1 file changed, 45 insertions(+), 54 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e2730b3e1a57..18afa544b623 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config,
 			  unsigned long *supported,
 			  struct phylink_link_state *state)
 {
+	bool one = state->interface == PHY_INTERFACE_MODE_NA;
 	struct net_device *ndev = to_net_dev(config->dev);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct macb *bp = netdev_priv(ndev);
 
-	/* We only support MII, RMII, GMII, RGMII & SGMII. */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_RMII &&
-	    state->interface != PHY_INTERFACE_MODE_GMII &&
-	    state->interface != PHY_INTERFACE_MODE_SGMII &&
-	    state->interface != PHY_INTERFACE_MODE_10GBASER &&
-	    !phy_interface_mode_is_rgmii(state->interface)) {
-		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
-		return;
-	}
-
-	if (!macb_is_gem(bp) &&
-	    (state->interface == PHY_INTERFACE_MODE_GMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
-		return;
-	}
-
-	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
-	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
-	      bp->caps & MACB_CAPS_PCS)) {
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_10GBASER:
+		if (bp->caps & MACB_CAPS_HIGH_SPEED &&
+		    bp->caps & MACB_CAPS_PCS &&
+		    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+			phylink_set(mask, 10000baseCR_Full);
+			phylink_set(mask, 10000baseER_Full);
+			phylink_set(mask, 10000baseKR_Full);
+			phylink_set(mask, 10000baseLR_Full);
+			phylink_set(mask, 10000baseLRM_Full);
+			phylink_set(mask, 10000baseSR_Full);
+			phylink_set(mask, 10000baseT_Full);
+		} else if (one) {
+			goto none;
+		}
+		if (one)
+			break;
+		fallthrough;
+	case PHY_INTERFACE_MODE_GMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		if (!macb_is_gem(bp) && one)
+			goto none;
+		fallthrough;
+	case PHY_INTERFACE_MODE_SGMII:
+		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_Full);
+			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
+				phylink_set(mask, 1000baseT_Half);
+		}
+		fallthrough;
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_RMII:
+		phylink_set(mask, 10baseT_Half);
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Half);
+		phylink_set(mask, 100baseT_Full);
+		break;
+	none:
+	default:
 		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 		return;
 	}

@@ -543,38 +566,6 @@ static void macb_validate(struct phylink_config *config,
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Asym_Pause);
-
-	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
-	    (state->interface == PHY_INTERFACE_MODE_NA ||
-	     state->interface == PHY_INTERFACE_MODE_10GBASER)) {
-		phylink_set(mask, 10000baseCR_Full);
-		phylink_set(mask, 10000baseER_Full);
-		phylink_set(mask, 10000baseKR_Full);
-		phylink_set(mask, 10000baseLR_Full);
-		phylink_set(mask, 10000baseLRM_Full);
-		phylink_set(mask, 10000baseSR_Full);
-		phylink_set(mask, 10000baseT_Full);
-		if (state->interface != PHY_INTERFACE_MODE_NA)
-			goto out;
-	}
-
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
-	    (state->interface == PHY_INTERFACE_MODE_NA ||
-	     state->interface == PHY_INTERFACE_MODE_GMII ||
-	     state->interface == PHY_INTERFACE_MODE_SGMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseX_Full);
-
-		if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
-			phylink_set(mask, 1000baseT_Half);
-	}
-out:
 	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_and(state->advertising, state->advertising, mask,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:19PM -0400, Sean Anderson wrote:

As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.

This doesn't look right to me.

Shouldn't this be != ?

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

On 10/4/21 7:04 PM, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:19PM -0400, Sean Anderson wrote:

quoted

As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.

This doesn't look right to me.

quoted

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e2730b3e1a57..18afa544b623 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config,
 			  unsigned long *supported,
 			  struct phylink_link_state *state)
 {
+	bool one = state->interface == PHY_INTERFACE_MODE_NA;

Shouldn't this be != ?

Since PHY_INTERFACE_MODE_NA is supposed to return all capabilities.

Ah, yes it should.

--Sean

On 04/10/2021 at 21:15, Sean Anderson wrote:

While we're on the subject, could someone clarify the relationship
between the various speed capabilities? What's the difference between
MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS,
and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE?

Yes. GEM is a new revision of the IP that is capable of doing Gigabit 
mode or not. sama7g5_emac_config is typically one of those doing only 
10/100.

HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think
this one is a bug, because it cares later on)?

MACB_CAPS_HIGH_SPEED and MACB_CAPS_PCS were added by 
e4e143e26ce8f5f57c60a994bdc63d0ddce3a823 ("net: macb: add support for 
high speed interface"). In this commit it is said that "This controller 
has separate MAC's and PCS'es for low and high speed paths." Maybe it's 
a hint.

Best regards,
   Nicolas


-- 
Nicolas Ferre

Hi Nicolas,

On 10/7/21 9:22 AM, Nicolas Ferre wrote:

On 04/10/2021 at 21:15, Sean Anderson wrote:

quoted

While we're on the subject, could someone clarify the relationship
between the various speed capabilities? What's the difference between
MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS,
and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE?

Yes. GEM is a new revision of the IP that is capable of doing Gigabit
mode or not. sama7g5_emac_config is typically one of those doing only
10/100.

Thanks for pointing that out. But even that config still has
MACB_CAPS_GIGABIT_MODE_AVAILABLE. So presumably you can use it for
gigabit speed if you don't use MII-on-RGMII. I suppose that
sama7g5_emac_config is not a GEM?

quoted

HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think
this one is a bug, because it cares later on)?

MACB_CAPS_HIGH_SPEED and MACB_CAPS_PCS were added by
e4e143e26ce8f5f57c60a994bdc63d0ddce3a823 ("net: macb: add support for
high speed interface"). In this commit it is said that "This
controller has separate MAC's and PCS'es for low and high speed
paths." Maybe it's a hint.

Ah, so perhaps HIGH_SPEED only represents the capability for a
high-speed PCS. Which of course is a bit strange considering that we
check for both it, PCS, and GIGABIT_MODE when determining whether we can
do 10G.

--Sean

Sean,

On 08/10/2021 at 02:20, Sean Anderson wrote:

On 10/7/21 9:22 AM, Nicolas Ferre wrote:

quoted

On 04/10/2021 at 21:15, Sean Anderson wrote:

quoted

While we're on the subject, could someone clarify the relationship
between the various speed capabilities? What's the difference between
MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS,
and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE?

 >>

quoted

Yes. GEM is a new revision of the IP that is capable of doing Gigabit
mode or not. sama7g5_emac_config is typically one of those doing only
10/100.

 >

Thanks for pointing that out. But even that config still has
MACB_CAPS_GIGABIT_MODE_AVAILABLE. So presumably you can use it for
gigabit speed if you don't use MII-on-RGMII. I suppose that
sama7g5_emac_config is not a GEM?

There must be a confusion between sama7g5_gem_config and 
sama7g5_emac_config here. The later one doesn't have this 
MACB_CAPS_GIGABIT_MODE_AVAILABLE capability.
Both are flavors of GEM and identified as such in the driver.

Best regards,
   Nicolas

-- 
Nicolas Ferre

The new pcs-related functions in the next few commits fit better before
phylink_create, so move phylink_set_pcs in preparation. No functional
change intended.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/phylink.c | 42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5a58c77d0002..6387c40c5592 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -795,6 +795,27 @@ static int phylink_register_sfp(struct phylink *pl,
 	return ret;
 }
 
+/**
+ * phylink_set_pcs() - set the current PCS for phylink to use
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @pcs: a pointer to the &struct phylink_pcs
+ *
+ * Bind the MAC PCS to phylink.  This may be called after phylink_create(),
+ * in mac_prepare() or mac_config() methods if it is desired to dynamically
+ * change the PCS.
+ *
+ * Please note that there are behavioural changes with the mac_config()
+ * callback if a PCS is present (denoting a newer setup) so removing a PCS
+ * is not supported, and if a PCS is going to be used, it must be registered
+ * by calling phylink_set_pcs() at the latest in the first mac_config() call.
+ */
+void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
+{
+	pl->pcs = pcs;
+	pl->pcs_ops = pcs->ops;
+}
+EXPORT_SYMBOL_GPL(phylink_set_pcs);
+
 /**
  * phylink_create() - create a phylink instance
  * @config: a pointer to the target &struct phylink_config

@@ -881,27 +902,6 @@ struct phylink *phylink_create(struct phylink_config *config,
 }
 EXPORT_SYMBOL_GPL(phylink_create);
 
-/**
- * phylink_set_pcs() - set the current PCS for phylink to use
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- * @pcs: a pointer to the &struct phylink_pcs
- *
- * Bind the MAC PCS to phylink.  This may be called after phylink_create(),
- * in mac_prepare() or mac_config() methods if it is desired to dynamically
- * change the PCS.
- *
- * Please note that there are behavioural changes with the mac_config()
- * callback if a PCS is present (denoting a newer setup) so removing a PCS
- * is not supported, and if a PCS is going to be used, it must be registered
- * by calling phylink_set_pcs() at the latest in the first mac_config() call.
- */
-void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
-{
-	pl->pcs = pcs;
-	pl->pcs_ops = pcs->ops;
-}
-EXPORT_SYMBOL_GPL(phylink_set_pcs);
-
 /**
  * phylink_destroy() - cleanup and destroy the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:15PM -0400, Sean Anderson wrote:

The new pcs-related functions in the next few commits fit better before
phylink_create, so move phylink_set_pcs in preparation. No functional
change intended.

Signed-off-by: Sean Anderson <redacted>

Reviewed-by: Russell King (Oracle) <redacted>

Thanks.

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

This adds support for using an external PCS. If someone else has set the
PCS beforehand, then we will use it instead of the internal PCS.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/ethernet/cadence/macb_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b938cdf4bb59..7e9fd12c09c8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -862,7 +862,7 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 	}
 
 	if (set_pcs)
-		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
+		set_pcs = phylink_set_pcs_weak(bp->phylink, &bp->phylink_pcs);
 
 	spin_lock_irqsave(&bp->lock, flags);
 

@@ -877,8 +877,11 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 		ncr |= MACB_BIT(MIIONRGMII);
 	}
 
-	if (macb_is_gem(bp) && set_pcs)
-		ctrl |= GEM_BIT(PCSSEL);
+	if (macb_is_gem(bp)) {
+		ctrl &= ~GEM_BIT(PCSSEL);
+		if (set_pcs)
+			ctrl |= GEM_BIT(PCSSEL);
+	}
 
 	/* Apply the new configuration, if any */
 	if (old_ctrl ^ ctrl)

-- 
2.25.1

This function is useful when probing MDIO devices which present a
phy-like interface despite not using the Linux ethernet phy subsystem.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/phy/phy_device.c | 3 ++-
 include/linux/phy.h          | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ba5ad86ec826..c75b189f1a3e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -805,7 +805,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
  * valid, %-EIO on bus access error, or %-ENODEV if no device responds
  * or invalid ID.
  */
-static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
+int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 {
 	int phy_reg;
 

@@ -833,6 +833,7 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(get_phy_c22_id);
 
 /* Extract the phy ID from the compatible string of the form
  * ethernet-phy-idAAAA.BBBB.

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 736e1d1a47c4..206049c0f587 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1381,6 +1381,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
+int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id);
 int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
 struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);

-- 
2.25.1

On Mon, Oct 04, 2021 at 03:15:24PM -0400, Sean Anderson wrote:

This function is useful when probing MDIO devices which present a
phy-like interface despite not using the Linux ethernet phy subsystem.

Maybe we should consider moving this into mdio_device.c and renaming
it if it's going to become more generic? Maybe mdio_read_c22_id()?
Andrew, Heiner, any opinions?

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

mac_prepare is called every time the interface is changed, so we can do
all of our configuration there, instead of in mac_config. This will be
useful for the next patch where we will set the PCS bit based on whether
we are using our internal PCS. No functional change intended.

Signed-off-by: Sean Anderson <redacted>
---

 drivers/net/ethernet/cadence/macb_main.c | 67 +++++++++++++-----------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 18afa544b623..db7acce42a27 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -652,42 +652,10 @@ static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
 static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 			    const struct phylink_link_state *state)
 {
-	struct net_device *ndev = to_net_dev(config->dev);
-	struct macb *bp = netdev_priv(ndev);
 	unsigned long flags;
-	u32 old_ctrl, ctrl;
-	u32 old_ncr, ncr;
 
 	spin_lock_irqsave(&bp->lock, flags);
 
-	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
-	old_ncr = ncr = macb_or_gem_readl(bp, NCR);
-
-	if (bp->caps & MACB_CAPS_MACB_IS_EMAC) {
-		if (state->interface == PHY_INTERFACE_MODE_RMII)
-			ctrl |= MACB_BIT(RM9200_RMII);
-	} else if (macb_is_gem(bp)) {
-		ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
-		ncr &= ~GEM_BIT(ENABLE_HS_MAC);
-
-		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
-			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
-		} else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
-			ctrl |= GEM_BIT(PCSSEL);
-			ncr |= GEM_BIT(ENABLE_HS_MAC);
-		} else if (bp->caps & MACB_CAPS_MIIONRGMII &&
-			   bp->phy_interface == PHY_INTERFACE_MODE_MII) {
-			ncr |= MACB_BIT(MIIONRGMII);
-		}
-	}
-
-	/* Apply the new configuration, if any */
-	if (old_ctrl ^ ctrl)
-		macb_or_gem_writel(bp, NCFGR, ctrl);
-
-	if (old_ncr ^ ncr)
-		macb_or_gem_writel(bp, NCR, ncr);
-
 	/* Disable AN for SGMII fixed link configuration, enable otherwise.
 	 * Must be written after PCSSEL is set in NCFGR,
 	 * otherwise writes will not take effect.