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

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

[Sean Anderson]

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

[RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 .../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

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Russell King (Oracle)]

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!

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Sean Anderson]

On 10/5/21 5:39 AM, Russell King (Oracle) wrote:
> 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.
> 

Either way is fine by me.

--Sean

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Rob Herring]

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:
> > 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

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Sean Anderson]

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:
>> 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".
>
> 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

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Rob Herring]

On Tue, Oct 12, 2021 at 11:18 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> 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:
> >> 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".
> >
> > 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.

Re: [RFC net-next PATCH 01/16] dt-bindings: net: Add pcs property

[Sean Anderson]

On 10/12/21 12:44 PM, Rob Herring wrote:
> On Tue, Oct 12, 2021 at 11:18 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> 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:
>> >> 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".
>> >
>> > 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.
>
>> 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.
>

[RFC net-next PATCH 02/16] dt-bindings: net: Add binding for Xilinx PCS

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 .../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

Re: [RFC net-next PATCH 02/16] dt-bindings: net: Add binding for Xilinx PCS

[Rob Herring]

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 <sean.anderson@seco.com>
> ---
> 
>  .../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.

[RFC net-next PATCH 03/16] net: sfp: Fix typo in state machine debug string

[Sean Anderson]

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

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 03/16] net: sfp: Fix typo in state machine debug string

[Andrew Lunn]

On Mon, Oct 04, 2021 at 03:15:14PM -0400, Sean Anderson wrote:
> The string should be "tx_disable" to match the state enum.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  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",
>  };

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

[RFC net-next PATCH 04/16] net: phylink: Move phylink_set_pcs before phylink_create

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 04/16] net: phylink: Move phylink_set_pcs before phylink_create

[Russell King (Oracle)]

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 <sean.anderson@seco.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks.

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

[RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Russell King (Oracle)]

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!

Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Sean Anderson]

On 10/5/21 5:48 AM, Russell King (Oracle) wrote:
> 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.

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

Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:
> 
> 
> On 10/5/21 5:48 AM, Russell King (Oracle) wrote:
> > 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.
> 
> 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

> > 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.

> > 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!

Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Sean Anderson]

On 10/7/21 6:23 AM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:
>>
>>
>> On 10/5/21 5:48 AM, Russell King (Oracle) wrote:
>> > 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.

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

>>
>> 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?

>> > 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.

>> > 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

[RFC net-next PATCH 06/16] net: phylink: Add function for optionally adding a PCS

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 06/16] net: phylink: Add function for optionally adding a PCS

[Russell King (Oracle)]

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!

Re: [RFC net-next PATCH 06/16] net: phylink: Add function for optionally adding a PCS

[Andrew Lunn]

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:
> > 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

Re: [RFC net-next PATCH 06/16] net: phylink: Add function for optionally adding a PCS

[Sean Anderson]

On 10/5/21 9:43 AM, Andrew Lunn wrote:
> 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:
>> > 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

[RFC net-next PATCH 07/16] net: phylink: Add helpers for c22 registers without MDIO

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 07/16] net: phylink: Add helpers for c22 registers without MDIO

[Russell King (Oracle)]

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!

[RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Russell King (Oracle)]

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.

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

Re: [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Sean Anderson]

On 10/4/21 7:04 PM, Russell King (Oracle) wrote:
> 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.
> 
>> 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

Re: [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Nicolas Ferre]

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

Re: [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Sean Anderson]

Hi Nicolas,

On 10/7/21 9:22 AM, Nicolas Ferre wrote:
> 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.

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?

>> 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

Re: [RFC net-next PATCH 08/16] net: macb: Clean up macb_validate

[Nicolas Ferre]

Sean,

On 08/10/2021 at 02:20, Sean Anderson wrote:
> 
> On 10/7/21 9:22 AM, Nicolas Ferre wrote:
>> 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.
 >
> 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

[RFC net-next PATCH 09/16] net: macb: Move most of mac_prepare to mac_config

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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.
@@ -797,6 +765,9 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 {
 	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;
 
 	if (interface == PHY_INTERFACE_MODE_10GBASER)
 		bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
@@ -808,6 +779,38 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 	if (bp->phylink_pcs.ops)
 		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
 
+	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 (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);
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+
 	return 0;
 }
 
-- 
2.25.1

Re: [RFC net-next PATCH 09/16] net: macb: Move most of mac_prepare to mac_config

[Russell King (Oracle)]

On Mon, Oct 04, 2021 at 03:15:20PM -0400, Sean Anderson wrote:
> 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.

The subject line appears to be the reverse of what you're actually
doing.

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

Re: [RFC net-next PATCH 09/16] net: macb: Move most of mac_prepare to mac_config

[Sean Anderson]

On 10/4/21 7:05 PM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:20PM -0400, Sean Anderson wrote:
>> 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.
> 
> The subject line appears to be the reverse of what you're actually
> doing.
> 

So it does.

--Sean

[RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Sean Anderson]

This moves all PCS-related settings to the pcs callbacks. This makes it
easy to support external PCSs. In addition, support for 1000BASE-X is
added (since we need it to set the SGMII mux).

pcs_config should set all registers necessary to bring the link up. In
addition, it needs to keep track of whether it modified anything so that
phylink can restart autonegotiation. config is also the right time to
veto configuration parameters, such as using the wrong interface. This
catches someone trying to use a slower speed (which could be supported
otherwise) after using a faster speed. We can't support this because
phylink doesn't support detaching PCSs.

pcs_link_up is necessary IFF the mode is not in-band and the speed and
duplex are different from what was set in config. However, because the
PCS supports only fixed speed (SPEED_1000 or SPEED_10000) and full
duplex, there is nothing to configure. Therefore, link_up has been
removed.

Now that the autonegotiation is done in pcs_config, we no longer need to
do it in macb_mac_config.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/ethernet/cadence/macb_main.c | 214 +++++++++++++++--------
 1 file changed, 138 insertions(+), 76 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db7acce42a27..08dcccd94f87 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -543,6 +543,7 @@ static void macb_validate(struct phylink_config *config,
 			goto none;
 		fallthrough;
 	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
 		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
 			phylink_set(mask, 1000baseT_Full);
 			phylink_set(mask, 1000baseX_Full);
@@ -571,25 +572,100 @@ static void macb_validate(struct phylink_config *config,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
-static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
-				 phy_interface_t interface, int speed,
-				 int duplex)
-{
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
-	u32 config;
-
-	config = gem_readl(bp, USX_CONTROL);
-	config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
-	config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config);
-	config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
-	config |= GEM_BIT(TX_EN);
-	gem_writel(bp, USX_CONTROL, config);
+static inline struct macb *pcs_to_macb(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct macb, phylink_pcs);
+}
+
+static void macb_pcs_get_state(struct phylink_pcs *pcs,
+			       struct phylink_link_state *state)
+{
+	struct macb *bp = pcs_to_macb(pcs);
+
+	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
+		state->interface = PHY_INTERFACE_MODE_SGMII;
+	else
+		state->interface = PHY_INTERFACE_MODE_1000BASEX;
+
+	phylink_mii_c22_pcs_decode_state(state, gem_readl(bp, PCSSTS),
+					 gem_readl(bp, PCSANLPBASE));
+}
+
+/**
+ * macb_pcs_config_an() - Configure autonegotiation settings for PCSs
+ * @bp - The macb to operate on
+ * @mode - The autonegotiation mode
+ * @interface - The interface to use
+ * @advertising - The advertisement mask
+ *
+ * This provides common configuration for PCS autonegotiation.
+ *
+ * Context: Call with @bp->lock held.
+ * Return: 1 if any registers were changed; 0 otherwise
+ */
+static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising)
+{
+	bool changed = false;
+	u16 old, new;
+
+	old = gem_readl(bp, PCSANADV);
+	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
+						       old);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSANADV, new);
+	}
+
+	old = new = gem_readl(bp, PCSCNTRL);
+	if (mode == MLO_AN_INBAND)
+		new |= BMCR_ANENABLE;
+	else
+		new &= ~BMCR_ANENABLE;
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSCNTRL, new);
+	}
+	return changed;
+}
+
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			   phy_interface_t interface,
+			   const unsigned long *advertising,
+			   bool permit_pause_to_mac)
+{
+	bool changed = false;
+	struct macb *bp = pcs_to_macb(pcs);
+	u16 old, new;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bp->lock, flags);
+	old = new = gem_readl(bp, NCFGR);
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		new |= GEM_BIT(SGMIIEN);
+	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
+		new &= ~GEM_BIT(SGMIIEN);
+	} else {
+		spin_lock_irqsave(&bp->lock, flags);
+		return -EOPNOTSUPP;
+	}
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, NCFGR, new);
+	}
+
+	if (macb_pcs_config_an(bp, mode, interface, advertising))
+		changed = true;
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+	return changed;
 }
 
 static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
 				   struct phylink_link_state *state)
 {
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	struct macb *bp = pcs_to_macb(pcs);
 	u32 val;
 
 	state->speed = SPEED_10000;
@@ -609,70 +685,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
 			       const unsigned long *advertising,
 			       bool permit_pause_to_mac)
 {
-	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	bool changed;
+	struct macb *bp = pcs_to_macb(pcs);
+	u16 old, new;
+	unsigned long flags;
 
-	gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
-		   GEM_BIT(SIGNAL_OK));
+	if (interface != PHY_INTERFACE_MODE_10GBASER)
+		return -EOPNOTSUPP;
 
-	return 0;
-}
+	spin_lock_irqsave(&bp->lock, flags);
+	old = new = gem_readl(bp, NCR);
+	new |= GEM_BIT(ENABLE_HS_MAC);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, NCFGR, new);
+	}
 
-static void macb_pcs_get_state(struct phylink_pcs *pcs,
-			       struct phylink_link_state *state)
-{
-	state->link = 0;
-}
+	if (macb_pcs_config_an(bp, mode, interface, advertising))
+		changed = true;
 
-static void macb_pcs_an_restart(struct phylink_pcs *pcs)
-{
-	/* Not supported */
-}
+	old = new = gem_readl(bp, USX_CONTROL);
+	new |= GEM_BIT(SIGNAL_OK);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, USX_CONTROL, new);
+	}
 
-static int macb_pcs_config(struct phylink_pcs *pcs,
-			   unsigned int mode,
-			   phy_interface_t interface,
-			   const unsigned long *advertising,
-			   bool permit_pause_to_mac)
-{
-	return 0;
+	old = new = gem_readl(bp, USX_CONTROL);
+	new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
+	new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
+	new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
+	new |= GEM_BIT(TX_EN);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, USX_CONTROL, new);
+	}
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+	return changed;
 }
 
 static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
 	.pcs_get_state = macb_usx_pcs_get_state,
 	.pcs_config = macb_usx_pcs_config,
-	.pcs_link_up = macb_usx_pcs_link_up,
 };
 
 static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
 	.pcs_get_state = macb_pcs_get_state,
-	.pcs_an_restart = macb_pcs_an_restart,
 	.pcs_config = macb_pcs_config,
 };
 
 static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 			    const struct phylink_link_state *state)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&bp->lock, flags);
-
-	/* Disable AN for SGMII fixed link configuration, enable otherwise.
-	 * Must be written after PCSSEL is set in NCFGR,
-	 * otherwise writes will not take effect.
-	 */
-	if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
-		u32 pcsctrl, old_pcsctrl;
-
-		old_pcsctrl = gem_readl(bp, PCSCNTRL);
-		if (mode == MLO_AN_FIXED)
-			pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
-		else
-			pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
-		if (old_pcsctrl != pcsctrl)
-			gem_writel(bp, PCSCNTRL, pcsctrl);
-	}
-
-	spin_unlock_irqrestore(&bp->lock, flags);
+	/* Nothing to do */
 }
 
 static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
@@ -763,20 +829,23 @@ static void macb_mac_link_up(struct phylink_config *config,
 static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 			    phy_interface_t interface)
 {
+	int set_pcs = 0;
 	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;
 
-	if (interface == PHY_INTERFACE_MODE_10GBASER)
+	if (interface == PHY_INTERFACE_MODE_10GBASER) {
 		bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
-	else if (interface == PHY_INTERFACE_MODE_SGMII)
+		set_pcs = 1;
+	} else if (interface == PHY_INTERFACE_MODE_SGMII ||
+		   interface == PHY_INTERFACE_MODE_1000BASEX) {
 		bp->phylink_pcs.ops = &macb_phylink_pcs_ops;
-	else
-		bp->phylink_pcs.ops = NULL;
+		set_pcs = 1;
+	}
 
-	if (bp->phylink_pcs.ops)
+	if (set_pcs)
 		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
 
 	spin_lock_irqsave(&bp->lock, flags);
@@ -787,21 +856,14 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
 	if (bp->caps & MACB_CAPS_MACB_IS_EMAC) {
 		if (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);
-		}
+	} else if (bp->caps & MACB_CAPS_MIIONRGMII &&
+		   bp->phy_interface == PHY_INTERFACE_MODE_MII) {
+		ncr |= MACB_BIT(MIIONRGMII);
 	}
 
+	if (macb_is_gem(bp) && set_pcs)
+		ctrl |= GEM_BIT(PCSSEL);
+
 	/* Apply the new configuration, if any */
 	if (old_ctrl ^ ctrl)
 		macb_or_gem_writel(bp, NCFGR, ctrl);
-- 
2.25.1

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> +			       struct phylink_link_state *state)
> +{
> +	struct macb *bp = pcs_to_macb(pcs);
> +
> +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> +		state->interface = PHY_INTERFACE_MODE_SGMII;
> +	else
> +		state->interface = PHY_INTERFACE_MODE_1000BASEX;

There is no requirement to set state->interface here. Phylink doesn't
cater for interface changes when reading the state. As documented,
phylink will set state->interface already before calling this function
to indicate what interface mode it is currently expecting from the
hardware.

> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
> +			      phy_interface_t interface,
> +			      const unsigned long *advertising)
> +{
> +	bool changed = false;
> +	u16 old, new;
> +
> +	old = gem_readl(bp, PCSANADV);
> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
> +						       old);
> +	if (old != new) {
> +		changed = true;
> +		gem_writel(bp, PCSANADV, new);
> +	}
> +
> +	old = new = gem_readl(bp, PCSCNTRL);
> +	if (mode == MLO_AN_INBAND)

Please use phylink_autoneg_inband(mode) here.

> +		new |= BMCR_ANENABLE;
> +	else
> +		new &= ~BMCR_ANENABLE;
> +	if (old != new) {
> +		changed = true;
> +		gem_writel(bp, PCSCNTRL, new);
> +	}

There has been the suggestion that we should allow in-band AN to be
disabled in 1000base-X if we're in in-band mode according to the
ethtool state. I have a patch that adds that.

> +	return changed;
> +}
> +
> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> +			   phy_interface_t interface,
> +			   const unsigned long *advertising,
> +			   bool permit_pause_to_mac)
> +{
> +	bool changed = false;
> +	struct macb *bp = pcs_to_macb(pcs);
> +	u16 old, new;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	old = new = gem_readl(bp, NCFGR);
> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		new |= GEM_BIT(SGMIIEN);
> +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> +		new &= ~GEM_BIT(SGMIIEN);
> +	} else {
> +		spin_lock_irqsave(&bp->lock, flags);
> +		return -EOPNOTSUPP;

You can't actually abort at this point - phylink will print the error
and carry on regardless. The checking is all done via the validate()
callback and if that indicates the interface mode is acceptable, then
it should be accepted.

>  static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>  	.pcs_get_state = macb_usx_pcs_get_state,
>  	.pcs_config = macb_usx_pcs_config,
> -	.pcs_link_up = macb_usx_pcs_link_up,
>  };
>  
>  static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>  	.pcs_get_state = macb_pcs_get_state,
> -	.pcs_an_restart = macb_pcs_an_restart,

You don't want to remove this. When operating in 1000BASE-X mode, it
will be called if a restart is required (e.g. macb_pcs_config()
returning positive, or an ethtool request.) You need to keep the empty
function. That may also help the diff algorithm to produce a cleaner
patch too.

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Sean Anderson]

Hi Russell,

On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> +			       struct phylink_link_state *state)
>> +{
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +
>> +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> +		state->interface = PHY_INTERFACE_MODE_SGMII;
>> +	else
>> +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
>
> There is no requirement to set state->interface here. Phylink doesn't
> cater for interface changes when reading the state. As documented,
> phylink will set state->interface already before calling this function
> to indicate what interface mode it is currently expecting from the
> hardware.

Ok, so instead I should be doing something like

if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
	interface = PHY_INTERFACE_MODE_SGMII;
else
	interface = PHY_INTERFACE_MODE_1000BASEX;

if (interface != state->interface) {
	state->link = 0;
	return;
}

?

>> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
>> +			      phy_interface_t interface,
>> +			      const unsigned long *advertising)
>> +{
>> +	bool changed = false;
>> +	u16 old, new;
>> +
>> +	old = gem_readl(bp, PCSANADV);
>> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
>> +						       old);
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSANADV, new);
>> +	}
>> +
>> +	old = new = gem_readl(bp, PCSCNTRL);
>> +	if (mode == MLO_AN_INBAND)
>
> Please use phylink_autoneg_inband(mode) here.

Ok.

>
>> +		new |= BMCR_ANENABLE;
>> +	else
>> +		new &= ~BMCR_ANENABLE;
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSCNTRL, new);
>> +	}
>
> There has been the suggestion that we should allow in-band AN to be
> disabled in 1000base-X if we're in in-band mode according to the
> ethtool state.

This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.

> I have a patch that adds that.

Have you posted it?

>> +	return changed;
>> +}
>> +
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> +			   phy_interface_t interface,
>> +			   const unsigned long *advertising,
>> +			   bool permit_pause_to_mac)
>> +{
>> +	bool changed = false;
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +	u16 old, new;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&bp->lock, flags);
>> +	old = new = gem_readl(bp, NCFGR);
>> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
>> +		new |= GEM_BIT(SGMIIEN);
>> +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
>> +		new &= ~GEM_BIT(SGMIIEN);
>> +	} else {
>> +		spin_lock_irqsave(&bp->lock, flags);
>> +		return -EOPNOTSUPP;
>
> You can't actually abort at this point - phylink will print the error
> and carry on regardless. The checking is all done via the validate()
> callback and if that indicates the interface mode is acceptable, then
> it should be accepted.

Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.

>>  static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>>  	.pcs_get_state = macb_usx_pcs_get_state,
>>  	.pcs_config = macb_usx_pcs_config,
>> -	.pcs_link_up = macb_usx_pcs_link_up,
>>  };
>>
>>  static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>>  	.pcs_get_state = macb_pcs_get_state,
>> -	.pcs_an_restart = macb_pcs_an_restart,
>
> You don't want to remove this.

Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.


> When operating in 1000BASE-X mode, it
> will be called if a restart is required (e.g. macb_pcs_config()
> returning positive, or an ethtool request.) You need to keep the empty
> function.

Ok, perhaps I can add some sanity checks for this in pcs_register.

--Sean

> That may also help the diff algorithm to produce a cleaner
> patch too.

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 12:03:50PM -0400, Sean Anderson wrote:
> Hi Russell,
> 
> On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> > On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> > > +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> > > +			       struct phylink_link_state *state)
> > > +{
> > > +	struct macb *bp = pcs_to_macb(pcs);
> > > +
> > > +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> > > +		state->interface = PHY_INTERFACE_MODE_SGMII;
> > > +	else
> > > +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
> > 
> > There is no requirement to set state->interface here. Phylink doesn't
> > cater for interface changes when reading the state. As documented,
> > phylink will set state->interface already before calling this function
> > to indicate what interface mode it is currently expecting from the
> > hardware.
> 
> Ok, so instead I should be doing something like
> 
> if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> 	interface = PHY_INTERFACE_MODE_SGMII;
> else
> 	interface = PHY_INTERFACE_MODE_1000BASEX;
> 
> if (interface != state->interface) {
> 	state->link = 0;
> 	return;
> }

Why would it be different? If we've called the pcs_config method to
set the interface to one mode, why would it change?

> > There has been the suggestion that we should allow in-band AN to be
> > disabled in 1000base-X if we're in in-band mode according to the
> > ethtool state.
> 
> This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
> another _encode variant? I hadn't done this here because the logic was
> only one if statement.
> 
> > I have a patch that adds that.
> 
> Have you posted it?

I haven't - it is a patch from Robert Hancock, "net: phylink: Support
disabling autonegotiation for PCS". I've had it in my tree for a while,
but I do want to make some changes to it before re-posting.

> > You can't actually abort at this point - phylink will print the error
> > and carry on regardless. The checking is all done via the validate()
> > callback and if that indicates the interface mode is acceptable, then
> > it should be accepted.
> 
> Ok, so where can the PCS NAK an interface? This is the only callback
> which has a return code, so I assumed this was the correct place to say
> "no, we don't support this." This is what lynx_pcs_config does as well.

At the moment, the PCS doesn't get to NAK an inappropriate interface.
That's currently the job of the MAC's validate callback with the
assumtion that the MAC knows what interfaces are supportable.

Trying to do it later, once the configuration has been worked out can
_only_ lead to a failure of some kind - in many paths, there is no way
to report the problem except by printing a message into the kernel log.

For example, by the time we reach pcs_config(), we've already prepared
the MAC for a change to the interface, we've told the MAC to configure
for that interface. Now the PCS rejects it - we have no record of the
old configuration to restore. Even if we had a way to restore it, then
we could return an error to the user - but the user doesn't get to
control the interface themselves. If it was the result of a PHY changing
its interface, then what - we can only log an error to the kernel log.
If it's the result of a SFP being plugged in, we have no way to
renegotiate.

pcs_config() is too late to be making decisions about whether the
requested configuration is acceptable or not. It needs to be done as
part of the validation step.

However, the validation step is not purely just validation, but it's
negotiation too for SFPs to be able to work out what interface mode
they should use in combination with the support that the MAC/PCS
offers.

I do feel that the implementation around the validation/selection of
interface for SFP etc is starting to creak, and I've some patches that
introduce a bitmap of interface types that are supported by the various
components. I haven't had the motivation to finish that off as my last
attempt at making a phylink API change was not pleasant in terms of
either help updating network drivers or getting patches tested. So I
now try to avoid phylink API changes at all cost.

People have whinged that phylink's API changes too quickly... I'm
guessing we're now going to get other people arguing that it needs
change to fix issues like this...

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Sean Anderson]

On 10/5/21 2:53 PM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:03:50PM -0400, Sean Anderson wrote:
>> Hi Russell,
>>
>> On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
>> > On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> > > +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> > > +			       struct phylink_link_state *state)
>> > > +{
>> > > +	struct macb *bp = pcs_to_macb(pcs);
>> > > +
>> > > +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> > > +		state->interface = PHY_INTERFACE_MODE_SGMII;
>> > > +	else
>> > > +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
>> >
>> > There is no requirement to set state->interface here. Phylink doesn't
>> > cater for interface changes when reading the state. As documented,
>> > phylink will set state->interface already before calling this function
>> > to indicate what interface mode it is currently expecting from the
>> > hardware.
>>
>> Ok, so instead I should be doing something like
>>
>> if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> 	interface = PHY_INTERFACE_MODE_SGMII;
>> else
>> 	interface = PHY_INTERFACE_MODE_1000BASEX;
>>
>> if (interface != state->interface) {
>> 	state->link = 0;
>> 	return;
>> }
>
> Why would it be different? If we've called the pcs_config method to
> set the interface to one mode, why would it change?

config() does not always come before get_state due to (e.g.)
phylink_ethtool_ksettings_get. Though in that instance, state->interface
is not read. And of course this ordering isn't documented.

That being said, I will just do

if (interface != PHY_INTERFACE_MODE_SGMII ||
     interface != PHY_INTERFACE_MODE_1000BASEX) {
	state->link = 0;
	return;
}

for next time.

>> > There has been the suggestion that we should allow in-band AN to be
>> > disabled in 1000base-X if we're in in-band mode according to the
>> > ethtool state.
>>
>> This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
>> another _encode variant? I hadn't done this here because the logic was
>> only one if statement.
>>
>> > I have a patch that adds that.
>>
>> Have you posted it?
>
> I haven't - it is a patch from Robert Hancock, "net: phylink: Support
> disabling autonegotiation for PCS". I've had it in my tree for a while,
> but I do want to make some changes to it before re-posting.

(for those following along this is [1])

OK. I'll add an _encode variant for this function in the next revision then.

[1] https://lore.kernel.org/netdev/20210630174927.1077249-1-robert.hancock@calian.com/

>> > You can't actually abort at this point - phylink will print the error
>> > and carry on regardless. The checking is all done via the validate()
>> > callback and if that indicates the interface mode is acceptable, then
>> > it should be accepted.
>>
>> Ok, so where can the PCS NAK an interface? This is the only callback
>> which has a return code, so I assumed this was the correct place to say
>> "no, we don't support this." This is what lynx_pcs_config does as well.
>
> At the moment, the PCS doesn't get to NAK an inappropriate interface.
> That's currently the job of the MAC's validate callback with the
> assumtion that the MAC knows what interfaces are supportable.

Which is a rather silly assumption because then you have to update the
MAC's validate function every time you add a new PCS. And this gets
messy rather fast. For example, you might want to connect your SFP
module to a MAC which only has an RGMII interface. So you put a DP83869
on your board connected like

MAC <--RGMII--> DP83869 <--SGMII--> SFP

For the moment, I think you have to just pretend that the DP83869 is the
only PHY in the system and hope that you don't need to talk to the SFP's
PHY. But if you want to use the DP83869 as a PCS, then you need to
update the MAC's validate() to allow SGMII, even though the MAC doesn't
support that without an external converter.

In an ideal world, the MAC would select its interface based on the PCS
(or lack of one), and the PCS would validate the interface mode. But of
course, there may be multiple PCSs available, so it is not so easy.

(Selecting between multiple PCSs (or no PCS at all) seems to be similar
in spirit to the PORT_XXX settings)

> Trying to do it later, once the configuration has been worked out can
> _only_ lead to a failure of some kind - in many paths, there is no way
> to report the problem except by printing a message into the kernel log.
>
> For example, by the time we reach pcs_config(), we've already prepared
> the MAC for a change to the interface, we've told the MAC to configure
> for that interface. Now the PCS rejects it - we have no record of the
> old configuration to restore. Even if we had a way to restore it, then
> we could return an error to the user - but the user doesn't get to
> control the interface themselves. If it was the result of a PHY changing
> its interface, then what - we can only log an error to the kernel log.
> If it's the result of a SFP being plugged in, we have no way to
> renegotiate.
>
> pcs_config() is too late to be making decisions about whether the
> requested configuration is acceptable or not. It needs to be done as
> part of the validation step.

Well, if these are the constraints, then IMO the PCS must have its own
validate() callback. Otherwise there is no way to tell a MAC that (for
example) supports both SGMII and 1000BASE-X that the PCS only supports
1000BASE-X. As another example, the MAC could support half duplex, but
the PCS might only suppport full duplex.

> However, the validation step is not purely just validation, but it's
> negotiation too for SFPs to be able to work out what interface mode
> they should use in combination with the support that the MAC/PCS
> offers.
>
> I do feel that the implementation around the validation/selection of
> interface for SFP etc is starting to creak, and I've some patches that
> introduce a bitmap of interface types that are supported by the various
> components. I haven't had the motivation to finish that off as my last
> attempt at making a phylink API change was not pleasant in terms of
> either help updating network drivers or getting patches tested. So I
> now try to avoid phylink API changes at all cost.
>
> People have whinged that phylink's API changes too quickly... I'm
> guessing we're now going to get other people arguing that it needs
> change to fix issues like this...

I think it should change, and I can help test things out on my own
setup, for whatever that's worth.

At the very least, it should be clearer what things are allowed to fail
for what reasons. Several callbacks are void when things can fail under
the hood (e.g. link_up or an_restart). And the API seems to have been
primarily designed around PCSs which are tightly-coupled to their MACs.

--Sean

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> At the very least, it should be clearer what things are allowed to fail
> for what reasons. Several callbacks are void when things can fail under
> the hood (e.g. link_up or an_restart). And the API seems to have been
> primarily designed around PCSs which are tightly-coupled to their MACs.

It has indeed been designed around that, because that's where the
technology that has been available to me has been. It is only in
the recent few years that we are starting to see designs where
the PCS is separate.

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 11:19:45PM +0100, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> > At the very least, it should be clearer what things are allowed to fail
> > for what reasons. Several callbacks are void when things can fail under
> > the hood (e.g. link_up or an_restart). And the API seems to have been
> > primarily designed around PCSs which are tightly-coupled to their MACs.

I think what I'd like to see is rather than a validate() callback for
the PCS, a bitmap of phy_interface_t modes that the PCS supports,
which is the direction I was wanting to take phylink and SFP support.

We can then use that information to inform the interface selection in
phylink and avoid interface modes that the PCS doesn't support.

However, things get tricky when we have several PCS that we can switch
between, and the switching is done in mac_prepare(). The current PCS
(if there is even a PCS attached) may not represent the abilities
that are actually available.

It sounds easy - just throw in an extra validation step when calling
phylink_validate(), but it isn't that simple. To avoid breaking
existing setups, phylink would need to know of every PCS that _could_
be attached, and the decisions that the MAC makes to select which PCS
is going to be used for any configuration.

We could possibly introduce a .mac_select_pcs(interface) method
which the MAC could return the PCS it wishes to use for the interface
mode with the guarantee that the PCS it returns is suitable - and if
it returns NULL that means the interface mode is unsupported. That,
along with the bitmask would probably work.

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Thu, Oct 07, 2021 at 11:34:06AM +0100, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 11:19:45PM +0100, Russell King (Oracle) wrote:
> > On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> > > At the very least, it should be clearer what things are allowed to fail
> > > for what reasons. Several callbacks are void when things can fail under
> > > the hood (e.g. link_up or an_restart). And the API seems to have been
> > > primarily designed around PCSs which are tightly-coupled to their MACs.
> 
> I think what I'd like to see is rather than a validate() callback for
> the PCS, a bitmap of phy_interface_t modes that the PCS supports,
> which is the direction I was wanting to take phylink and SFP support.
> 
> We can then use that information to inform the interface selection in
> phylink and avoid interface modes that the PCS doesn't support.
> 
> However, things get tricky when we have several PCS that we can switch
> between, and the switching is done in mac_prepare(). The current PCS
> (if there is even a PCS attached) may not represent the abilities
> that are actually available.
> 
> It sounds easy - just throw in an extra validation step when calling
> phylink_validate(), but it isn't that simple. To avoid breaking
> existing setups, phylink would need to know of every PCS that _could_
> be attached, and the decisions that the MAC makes to select which PCS
> is going to be used for any configuration.
> 
> We could possibly introduce a .mac_select_pcs(interface) method
> which the MAC could return the PCS it wishes to use for the interface
> mode with the guarantee that the PCS it returns is suitable - and if
> it returns NULL that means the interface mode is unsupported. That,
> along with the bitmask would probably work.

Here's a patch which illustrates roughly what I'm thinking at the
moment - only build tested.

mac_select_pcs() should not ever fail in phylink_major_config() - that
would be a bug. I've hooked mac_select_pcs() also into the validate
function so we can catch problems there, but we will need to involve
the PCS in the interface selection for SFPs etc.

Note that mac_select_pcs() must be inconsequential - it's asking the
MAC which PCS it wishes to use for the interface mode.

I am still very much undecided whether we wish phylink to parse the
pcs-handle property and if present, override the MAC - I feel that
logic depends in the MAC driver, since a single PCS can be very
restrictive in terms of what interface modes are supportable. If the
MAC wishes pcs-handle to override its internal ones, then it can
always do:

	if (port->external_pcs)
		return port->external_pcs;

in its mac_select_pcs() implementation. This gives us a bit of future
flexibility.

If we parse pcs-handle in phylink, then if we end up with multiple PCS
to choose from, we then need to work out how to either allow the MAC
driver to tell phylink not to parse pcs-handle, or we need some way for
phylink to ask the MAC "these are the PCS I have, which one should I
use" which is yet another interface.

What I don't like about the patch is the need to query the PCS based on
interface - when we have a SFP plugged in, it may support multiple
interfaces. I think we still need the MAC to restrict what it returns
in its validate() method according to the group of PCS that it has
available for the SFP interface selection to work properly. Things in
this regard should become easier _if_ I can switch phylink over to
selecting interface based on phy_interface_t bitmaps rather than the
current bodge using ethtool link modes, but that needs changes to phylib
and all MAC drivers, otherwise we have to support two entirely separate
ways to select the interface mode.

My argument against that is... I'll end up converting the network
interfaces that I use to the new implementation, and the old version
will start to rot. I've already stopped testing phylink without a PCS
attached for this very reason. The more legacy code we keep, the worse
this problem becomes.

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index cf8acabb90ac..ad73a488fc5f 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -1239,7 +1239,8 @@ struct mvpp2_port {
 	phy_interface_t phy_interface;
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
-	struct phylink_pcs phylink_pcs;
+	struct phylink_pcs pcs_gmac;
+	struct phylink_pcs pcs_xlg;
 	struct phy *comphy;
 
 	struct mvpp2_bm_pool *pool_long;
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 3197526455d9..509ff0e68e2a 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6105,15 +6105,20 @@ static struct mvpp2_port *mvpp2_phylink_to_port(struct phylink_config *config)
 	return container_of(config, struct mvpp2_port, phylink_config);
 }
 
-static struct mvpp2_port *mvpp2_pcs_to_port(struct phylink_pcs *pcs)
+static struct mvpp2_port *mvpp2_pcs_xlg_to_port(struct phylink_pcs *pcs)
 {
-	return container_of(pcs, struct mvpp2_port, phylink_pcs);
+	return container_of(pcs, struct mvpp2_port, pcs_xlg);
+}
+
+static struct mvpp2_port *mvpp2_pcs_gmac_to_port(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct mvpp2_port, pcs_gmac);
 }
 
 static void mvpp2_xlg_pcs_get_state(struct phylink_pcs *pcs,
 				    struct phylink_link_state *state)
 {
-	struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+	struct mvpp2_port *port = mvpp2_pcs_xlg_to_port(pcs);
 	u32 val;
 
 	state->speed = SPEED_10000;
@@ -6148,7 +6153,7 @@ static const struct phylink_pcs_ops mvpp2_phylink_xlg_pcs_ops = {
 static void mvpp2_gmac_pcs_get_state(struct phylink_pcs *pcs,
 				     struct phylink_link_state *state)
 {
-	struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+	struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
 	u32 val;
 
 	val = readl(port->base + MVPP2_GMAC_STATUS0);
@@ -6185,7 +6190,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 				 const unsigned long *advertising,
 				 bool permit_pause_to_mac)
 {
-	struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+	struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
 	u32 mask, val, an, old_an, changed;
 
 	mask = MVPP2_GMAC_IN_BAND_AUTONEG_BYPASS |
@@ -6239,7 +6244,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 
 static void mvpp2_gmac_pcs_an_restart(struct phylink_pcs *pcs)
 {
-	struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+	struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
 	u32 val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG);
 
 	writel(val | MVPP2_GMAC_IN_BAND_RESTART_AN,
@@ -6428,8 +6433,23 @@ static void mvpp2_gmac_config(struct mvpp2_port *port, unsigned int mode,
 		writel(ctrl4, port->base + MVPP22_GMAC_CTRL_4_REG);
 }
 
-static int mvpp2__mac_prepare(struct phylink_config *config, unsigned int mode,
-			      phy_interface_t interface)
+static struct phylink_pcs *mvpp2_select_pcs(struct phylink_config *config,
+					    phy_interface_t interface)
+{
+	struct mvpp2_port *port = mvpp2_phylink_to_port(config);
+
+	/* Select the appropriate PCS operations depending on the
+	 * configured interface mode. We will only switch to a mode
+	 * that the validate() checks have already passed.
+	 */
+	if (mvpp2_is_xlg(interface))
+		return &port->pcs_xlg;
+	else
+		return &port->pcs_gmac;
+}
+
+static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
+			     phy_interface_t interface)
 {
 	struct mvpp2_port *port = mvpp2_phylink_to_port(config);
 
@@ -6475,31 +6495,9 @@ static int mvpp2__mac_prepare(struct phylink_config *config, unsigned int mode,
 		}
 	}
 
-	/* Select the appropriate PCS operations depending on the
-	 * configured interface mode. We will only switch to a mode
-	 * that the validate() checks have already passed.
-	 */
-	if (mvpp2_is_xlg(interface))
-		port->phylink_pcs.ops = &mvpp2_phylink_xlg_pcs_ops;
-	else
-		port->phylink_pcs.ops = &mvpp2_phylink_gmac_pcs_ops;
-
 	return 0;
 }
 
-static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
-			     phy_interface_t interface)
-{
-	struct mvpp2_port *port = mvpp2_phylink_to_port(config);
-	int ret;
-
-	ret = mvpp2__mac_prepare(config, mode, interface);
-	if (ret == 0)
-		phylink_set_pcs(port->phylink, &port->phylink_pcs);
-
-	return ret;
-}
-
 static void mvpp2_mac_config(struct phylink_config *config, unsigned int mode,
 			     const struct phylink_link_state *state)
 {
@@ -6691,12 +6689,15 @@ static void mvpp2_acpi_start(struct mvpp2_port *port)
 	struct phylink_link_state state = {
 		.interface = port->phy_interface,
 	};
-	mvpp2__mac_prepare(&port->phylink_config, MLO_AN_INBAND,
-			   port->phy_interface);
+	struct phylink_pcs *pcs;
+
+	pcs = mvpp2_select_pcs(&port->phylink_config, port->phy_interface);
+
+	mvpp2_mac_prepare(&port->phylink_config, MLO_AN_INBAND,
+			  port->phy_interface);
 	mvpp2_mac_config(&port->phylink_config, MLO_AN_INBAND, &state);
-	port->phylink_pcs.ops->pcs_config(&port->phylink_pcs, MLO_AN_INBAND,
-					  port->phy_interface,
-					  state.advertising, false);
+	pcs->ops->pcs_config(pcs, MLO_AN_INBAND, port->phy_interface,
+			     state.advertising, false);
 	mvpp2_mac_finish(&port->phylink_config, MLO_AN_INBAND,
 			 port->phy_interface);
 	mvpp2_mac_link_up(&port->phylink_config, NULL,
@@ -6945,6 +6946,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 			goto err_free_port_pcpu;
 		}
 		port->phylink = phylink;
+		port->pcs_gmac.ops = &mvpp2_phylink_gmac_pcs_ops;
+		port->pcs_xlg.ops = &mvpp2_phylink_xlg_pcs_ops;
 	} else {
 		dev_warn(&pdev->dev, "Use link irqs for port#%d. FW update required\n", port->id);
 		port->phylink = NULL;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ab651f6197cc..89c7df0595be 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -170,8 +170,16 @@ static const char *phylink_an_mode_str(unsigned int mode)
 static int phylink_validate(struct phylink *pl, unsigned long *supported,
 			    struct phylink_link_state *state)
 {
+	struct phylink_pcs *pcs;
+
 	pl->mac_ops->validate(pl->config, supported, state);
 
+	if (pl->mac_ops->mac_select_pcs) {
+		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
+		if (!pcs)
+			return -EINVAL;
+	}
+
 	return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
 }
 
@@ -462,6 +470,7 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
 static void phylink_major_config(struct phylink *pl, bool restart,
 				  const struct phylink_link_state *state)
 {
+	struct phylink_pcs *pcs;
 	int err;
 
 	phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
@@ -476,6 +485,14 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 		}
 	}
 
+	if (pl->mac_ops->mac_select_pcs) {
+		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
+		if (!pcs)
+			phylink_err(pl, "mac_select_pcs unexpectedly failed\n");
+		else
+			phylink_set_pcs(pl, pcs);
+	}
+
 	phylink_mac_config(pl, state);
 
 	if (pl->pcs_ops) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index f7b5ed06a815..a70c28079f4f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -81,6 +81,7 @@ struct phylink_config {
 /**
  * struct phylink_mac_ops - MAC operations structure.
  * @validate: Validate and update the link configuration.
+ * @mac_select_pcs: Select a PCS for the interface mode.
  * @mac_pcs_get_state: Read the current link state from the hardware.
  * @mac_prepare: prepare for a major reconfiguration of the interface.
  * @mac_config: configure the MAC for the selected mode and state.
@@ -95,6 +96,8 @@ struct phylink_mac_ops {
 	void (*validate)(struct phylink_config *config,
 			 unsigned long *supported,
 			 struct phylink_link_state *state);
+	struct phylink_pcs *(*mac_select_pcs)(struct phylink_config *config,
+					      phy_interface_t iface);
 	void (*mac_pcs_get_state)(struct phylink_config *config,
 				  struct phylink_link_state *state);
 	int (*mac_prepare)(struct phylink_config *config, unsigned int mode,

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Russell King (Oracle)]

On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:
> Here's a patch which illustrates roughly what I'm thinking at the
> moment - only build tested.
> 
> mac_select_pcs() should not ever fail in phylink_major_config() - that
> would be a bug. I've hooked mac_select_pcs() also into the validate
> function so we can catch problems there, but we will need to involve
> the PCS in the interface selection for SFPs etc.
> 
> Note that mac_select_pcs() must be inconsequential - it's asking the
> MAC which PCS it wishes to use for the interface mode.
> 
> I am still very much undecided whether we wish phylink to parse the
> pcs-handle property and if present, override the MAC - I feel that
> logic depends in the MAC driver, since a single PCS can be very
> restrictive in terms of what interface modes are supportable. If the
> MAC wishes pcs-handle to override its internal ones, then it can
> always do:
> 
> 	if (port->external_pcs)
> 		return port->external_pcs;
> 
> in its mac_select_pcs() implementation. This gives us a bit of future
> flexibility.
> 
> If we parse pcs-handle in phylink, then if we end up with multiple PCS
> to choose from, we then need to work out how to either allow the MAC
> driver to tell phylink not to parse pcs-handle, or we need some way for
> phylink to ask the MAC "these are the PCS I have, which one should I
> use" which is yet another interface.
> 
> What I don't like about the patch is the need to query the PCS based on
> interface - when we have a SFP plugged in, it may support multiple
> interfaces. I think we still need the MAC to restrict what it returns
> in its validate() method according to the group of PCS that it has
> available for the SFP interface selection to work properly. Things in
> this regard should become easier _if_ I can switch phylink over to
> selecting interface based on phy_interface_t bitmaps rather than the
> current bodge using ethtool link modes, but that needs changes to phylib
> and all MAC drivers, otherwise we have to support two entirely separate
> ways to select the interface mode.
> 
> My argument against that is... I'll end up converting the network
> interfaces that I use to the new implementation, and the old version
> will start to rot. I've already stopped testing phylink without a PCS
> attached for this very reason. The more legacy code we keep, the worse
> this problem becomes.

Having finished off the SFP side of the phy_interface_t bitmap
(http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
and I think the mac_select_pcs() approach will work.

See commit
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
where we have a set of phy_interface_t to choose one from, and if
we add PCS selection into that logic, the loop becomes:

static phy_interface_t phylink_select_interface(struct phylink *pl,
						const unsigned long *intf
						const char *intf_name)
{
	phy_interface_t interface, intf;

	...

	interface = PHY_INTERFACE_MODE_NA;
	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
		intf = phylink_sfp_interface_preference[i];

		if (!test_bit(intf, u))
			continue;

		pcs = pl->pcs;
		if (pl->mac_ops->mac_select_pcs) {
			pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
			if (!pcs)
				continue;
		}

		if (pcs && !test_bit(intf, pcs->supported_interfaces))
			continue;

		interface = intf;
		break;
	}
	...
}

The alternative would be to move some of that logic into
phylink_sfp_config_nophy(), and will mean knocking out bits from
the mask supplied to phylink_select_interface() each time we select
an interface mode that the PCS doesn't support... which sounds rather
more yucky to me.

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

Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

[Sean Anderson]

On 10/7/21 12:23 PM, Russell King (Oracle) wrote:
> On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:
>> Here's a patch which illustrates roughly what I'm thinking at the
>> moment - only build tested.
>> 
>> mac_select_pcs() should not ever fail in phylink_major_config() - that
>> would be a bug. I've hooked mac_select_pcs() also into the validate
>> function so we can catch problems there, but we will need to involve
>> the PCS in the interface selection for SFPs etc.
>> 
>> Note that mac_select_pcs() must be inconsequential - it's asking the
>> MAC which PCS it wishes to use for the interface mode.
>> 
>> I am still very much undecided whether we wish phylink to parse the
>> pcs-handle property and if present, override the MAC - I feel that
>> logic depends in the MAC driver, since a single PCS can be very
>> restrictive in terms of what interface modes are supportable. If the
>> MAC wishes pcs-handle to override its internal ones, then it can
>> always do:
>> 
>> 	if (port->external_pcs)
>> 		return port->external_pcs;
>> 
>> in its mac_select_pcs() implementation. This gives us a bit of future
>> flexibility.
>> 
>> If we parse pcs-handle in phylink, then if we end up with multiple PCS
>> to choose from, we then need to work out how to either allow the MAC
>> driver to tell phylink not to parse pcs-handle, or we need some way for
>> phylink to ask the MAC "these are the PCS I have, which one should I
>> use" which is yet another interface.
>> 
>> What I don't like about the patch is the need to query the PCS based on
>> interface - when we have a SFP plugged in, it may support multiple
>> interfaces. I think we still need the MAC to restrict what it returns
>> in its validate() method according to the group of PCS that it has
>> available for the SFP interface selection to work properly. Things in
>> this regard should become easier _if_ I can switch phylink over to
>> selecting interface based on phy_interface_t bitmaps rather than the
>> current bodge using ethtool link modes, but that needs changes to phylib
>> and all MAC drivers, otherwise we have to support two entirely separate
>> ways to select the interface mode.
>> 
>> My argument against that is... I'll end up converting the network
>> interfaces that I use to the new implementation, and the old version
>> will start to rot. I've already stopped testing phylink without a PCS
>> attached for this very reason. The more legacy code we keep, the worse
>> this problem becomes.
> 
> Having finished off the SFP side of the phy_interface_t bitmap
> (http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
> and I think the mac_select_pcs() approach will work.
> 
> See commit
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
> where we have a set of phy_interface_t to choose one from, and if
> we add PCS selection into that logic, the loop becomes:
> 
> static phy_interface_t phylink_select_interface(struct phylink *pl,
> 						const unsigned long *intf
> 						const char *intf_name)
> {
> 	phy_interface_t interface, intf;
> 
> 	...
> 
> 	interface = PHY_INTERFACE_MODE_NA;
> 	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
> 		intf = phylink_sfp_interface_preference[i];
> 
> 		if (!test_bit(intf, u))
> 			continue;
> 
> 		pcs = pl->pcs;
> 		if (pl->mac_ops->mac_select_pcs) {
> 			pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
> 			if (!pcs)
> 				continue;
> 		}
> 
> 		if (pcs && !test_bit(intf, pcs->supported_interfaces))
> 			continue;
> 
> 		interface = intf;
> 		break;
> 	}
> 	...
> }
> 
> The alternative would be to move some of that logic into
> phylink_sfp_config_nophy(), and will mean knocking out bits from
> the mask supplied to phylink_select_interface() each time we select
> an interface mode that the PCS doesn't support... which sounds rather
> more yucky to me.
> 

With appropriate helper functions, I don't think we would need to have a
separate mac_select_pcs callback:

probe()
{
	priv->pl = phylink_create();
	priv->pcs1 = my_internal_pcs;
	priv->pcs2 = phylink_find_pcs();
}

validate()
{
	switch (state->interface) {
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_GMII:
		phylink_set(mask, 1000baseT_Full);
		phylink_set(mask, 1000baseX_Full);
		if (one)
			break;
		fallthrough;
	default:
		matched = phylink_set_pcs_modes(mask, priv->pcs1, state);
		matched ||= phylink_set_pcs_modes(mask, priv->pcs2, state);
		if (matched)
			break;
		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
		return;
	}
	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
}

prepare()
{
	switch (state->interface) {
	case PHY_INTERFACE_GMII:
		enable_gmii();
		break;
	default:
		if (phylink_attach_matching_pcs(priv->pl, priv->pcs1, state->interface)) {
			enable_internal_pcs();
			break;
		}
		if (phylink_attach_matching_pcs(priv->pl, priv->pcs2, state->interface)) {
			enable_external_pcs();
			break;	
		}
		BUG();
	}
}

int phylink_set_interface_mode(mask, iface)
{
	/* switch statement from phylink_parse_mode here */
}

bool phylink_set_pcs_modes(mask, pcs, state)
{
	if (state->interface != PHY_INTERFACE_MODE_NA) {
		if (!test_bit(state->interface, pcs->supported_interfaces))
			return false;
		phylink_set_interface_mode(mask, state->interface);
		return true;
	}
	
	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
		if (test_bit(i, pcs->supported_interfaces))
			phylink_set_interface_mode(mask, state->interface);
	}
	return true;
}

bool phylink_attach_matching_pcs(pl, pcs, iface)
{
	if (!test_bit(iface, pcs->supported_interfaces))
		return false;
	phylink_set_pcs(pl, pcs);
	return true;
}

I think this has some advantages over a separate mac_select_pcs():

- In validate() you get all the available interfaces.
- The MAC can set the priority of its PCSs however it likes, which is
   probably good enough for almost every case.
- The MAC doesn't care about what interfaces the PCSs actually support.
- phylink can do whatever logic it wants for selection under the hood.
- From phylink's POV, drivers behave the same way no matter whether they
   use these helpers or whether they just say "If we use SGMII, then use
   our internal PCS)".

--Sean

[RFC net-next PATCH 11/16] net: macb: Support restarting PCS autonegotiation

[Sean Anderson]

This adds support for restarting autonegotiation using the internal PCS.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/ethernet/cadence/macb_main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 08dcccd94f87..b938cdf4bb59 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -662,6 +662,21 @@ static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	return changed;
 }
 
+static void macb_pcs_an_restart(struct phylink_pcs *pcs)
+{
+	struct macb *bp = pcs_to_macb(pcs);
+	u32 bmcr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bp->lock, flags);
+
+	bmcr = gem_readl(bp, PCSCNTRL);
+	bmcr |= BMCR_ANENABLE;
+	gem_writel(bp, PCSCNTRL, bmcr);
+
+	spin_lock_irqsave(&bp->lock, flags);
+}
+
 static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
 				   struct phylink_link_state *state)
 {
@@ -733,6 +748,7 @@ static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
 static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
 	.pcs_get_state = macb_pcs_get_state,
 	.pcs_config = macb_pcs_config,
+	.pcs_an_restart = macb_pcs_an_restart,
 };
 
 static void macb_mac_config(struct phylink_config *config, unsigned int mode,
-- 
2.25.1

[RFC net-next PATCH 12/16] net: macb: Support external PCSs

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

[RFC net-next PATCH 13/16] net: phy: Export get_phy_c22_id

[Sean Anderson]

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 <sean.anderson@seco.com>
---

 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

Re: [RFC net-next PATCH 13/16] net: phy: Export get_phy_c22_id

[Russell King (Oracle)]

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!

[RFC net-next PATCH 14/16] net: mdio: Add helper functions for accessing MDIO devices

[Sean Anderson]

This adds some helpers for accessing non-phy MDIO devices. They are
analogous to phy_(read|write|modify), except that they take an mdio_device
and not a phy_device.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 include/linux/mdio.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 5e6dc38f418e..1342bbb6ef0c 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -350,6 +350,23 @@ int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
 int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
 		   u16 set);
 
+static inline int mdiodev_read(struct mdio_device *mdiodev, u32 regnum)
+{
+	return mdiobus_read(mdiodev->bus, mdiodev->addr, regnum);
+}
+
+static inline int mdiodev_write(struct mdio_device *mdiodev, u32 regnum,
+				u16 val)
+{
+	return mdiobus_write(mdiodev->bus, mdiodev->addr, regnum, val);
+}
+
+static inline int mdiodev_modify(struct mdio_device *mdiodev, u32 regnum,
+				u16 mask, u16 set)
+{
+	return mdiobus_modify(mdiodev->bus, mdiodev->addr, regnum, mask, set);
+}
+
 static inline u32 mdiobus_c45_addr(int devad, u16 regnum)
 {
 	return MII_ADDR_C45 | devad << MII_DEVADDR_C45_SHIFT | regnum;
-- 
2.25.1

[RFC net-next PATCH 15/16] net: pcs: Add Xilinx PCS driver

[Sean Anderson]

This adds support for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII
device. This is a soft device which converts between GMII and either
SGMII, 2.5G SGMII, 1000BASE-X, or 2500BASE-X. If configured correctly,
it can also switch between SGMII and 1000BASE-X at runtime. Internally,
these are referred to as the "standard" to match the terminology in the
datasheet. I am not sure how to handle the 2.5G SGMII case yet (since
AIUI Linux doesn't think it exists), and I do not have the hardware to
test it, so I have left out support.

This device has a c22-compliant PHY interface, so for the most part we
can just use the phylink helpers. Where we have to do things manually is
usually just to set the interface. We also set the speed and duplex when
the link comes up. I don't know what the permit_pause_to_mac parameter
is for in pcs_config, so I ignore it...

This device supports an interrupt which is triggered on link status
change. While according to Documentation/networking/sfp-phylink.rst the
mac should call phylink_mac_change on link state change, it is unclear
to me what the PCS should call. For now I have just set the PCS to
poll.

This device supports sharing some logic between different
implementations of the device. In this case, one device has the shared
logic, and several output (clocks, resets, etc.) are connected between
it and the consuming devices. I am not sure how to model this (create a
clock? add a devlink?) so I have left it out, but I would like to
include support for this use-case.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 MAINTAINERS                  |   6 +
 drivers/net/pcs/Kconfig      |  19 ++
 drivers/net/pcs/Makefile     |   1 +
 drivers/net/pcs/pcs-xilinx.c | 326 +++++++++++++++++++++++++++++++++++
 4 files changed, 352 insertions(+)
 create mode 100644 drivers/net/pcs/pcs-xilinx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f4615336fa5..eaee1028a7ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20591,6 +20591,12 @@ F:	Documentation/devicetree/bindings/gpio/gpio-zynq.yaml
 F:	drivers/gpio/gpio-xilinx.c
 F:	drivers/gpio/gpio-zynq.c
 
+XILINX PCS DRIVER
+M:	Sean Anderson <sean.anderso@seco.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/xilinx,pcs.yaml
+F:	drivers/net/pcs/pcs-xilinx.c
+
 XILINX SD-FEC IP CORES
 M:	Derek Kiernan <derek.kiernan@xilinx.com>
 M:	Dragan Cvetic <dragan.cvetic@xilinx.com>
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..3ee415504eeb 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -18,4 +18,23 @@ config PCS_LYNX
 	  This module provides helpers to phylink for managing the Lynx PCS
 	  which is part of the Layerscape and QorIQ Ethernet SERDES.
 
+config PCS_XILINX
+	depends on OF
+	depends on GPIOLIB
+	depends on COMMON_CLK
+	depends on PHYLINK
+	tristate "Xilinx PCS driver"
+	help
+	  PCS driver for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII device.
+	  This device can either act as a PCS+PMA for 1000BASE-X or 2500BASE-X,
+	  or as a GMII-to-SGMII bridge. It can also switch between 1000BASE-X
+	  and SGMII dynamically if configured correctly when synthesized.
+	  Typical applications use this device on an FPGA connected to a GEM or
+	  TEMAC on the GMII side. The other side is typically connected to
+	  on-device gigabit transceivers, off-device SERDES devices using TBI,
+	  or LVDS IO resources directly.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pcs-xilinx.
+
 endmenu
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 0603d469bd57..4a8580ca4134 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -5,3 +5,4 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
+obj-$(CONFIG_PCS_XILINX)	+= pcs-xilinx.o
diff --git a/drivers/net/pcs/pcs-xilinx.c b/drivers/net/pcs/pcs-xilinx.c
new file mode 100644
index 000000000000..297c9881b401
--- /dev/null
+++ b/drivers/net/pcs/pcs-xilinx.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 Sean Anderson <sean.anderson@seco.com>
+ *
+ * This is the driver for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE
+ * IP. A typical setup will look something like
+ *
+ * MAC <--GMII--> PCS+PMA <--internal/TBI--> PMD (SERDES) <--SGMII/1000BASE-X
+ *
+ * The link to the PMD is not modeled by this driver, except for refclk. It is
+ * assumed that the SERDES needs no configuration. It is also possible to go
+ * from SGMII to GMII (PHY mode), but this is not supported.
+ *
+ * This driver was written with reference to PG047:
+ * https://www.xilinx.com/support/documentation/ip_documentation/gig_ethernet_pcs_pma/v16_2/pg047-gig-eth-pcs-pma.pdf
+ */
+
+#include <linux/clk.h>
+#include <linux/mdio.h>
+#include <linux/of.h>
+#include <linux/phylink.h>
+#include <linux/reset.h>
+
+/* Vendor-specific MDIO registers */
+#define XILINX_PCS_ANICR 16 /* Auto-Negotiation Interrupt Control Register */
+#define XILINX_PCS_SSR   17 /* Standard Selection Register */
+
+#define XILINX_PCS_ANICR_IE BIT(0) /* Interrupt Enable */
+#define XILINX_PCS_ANICR_IS BIT(1) /* Interrupt Status */
+
+#define XILINX_PCS_SSR_SGMII BIT(0) /* Select SGMII standard */
+
+/**
+ * enum xilinx_pcs_standard - Support for interface standards
+ * @XILINX_PCS_STD_SGMII: SGMII for 10/100/1000BASE-T
+ * @XILINX_PCS_STD_1000BASEX: 1000BASE-X PMD Support Interface
+ * @XILINX_PCS_STD_BOTH: Support for both SGMII and 1000BASE-X
+ * @XILINX_PCS_STD_2500BASEX: 2500BASE-X PMD Support Interface
+ * @XILINX_PCS_STD_2500SGMII: 2.5G SGMII for 2.5GBASE-T
+ */
+enum xilinx_pcs_standard {
+	XILINX_PCS_STD_SGMII,
+	XILINX_PCS_STD_1000BASEX,
+	XILINX_PCS_STD_BOTH,
+	XILINX_PCS_STD_2500BASEX,
+	XILINX_PCS_STD_2500SGMII,
+};
+
+/**
+ * struct xilinx_pcs - Private data for Xilinx PCS devices
+ * @pcs: The phylink PCS
+ * @mdiodev: The mdiodevice used to access the PCS
+ * @refclk: The reference clock for the PMD
+ * @reset: The reset controller for the PCS
+ * @standard: The supported interface standard
+ */
+struct xilinx_pcs {
+	struct phylink_pcs pcs;
+	struct mdio_device *mdiodev;
+	struct clk *refclk;
+	struct reset_control *reset;
+	enum xilinx_pcs_standard standard;
+};
+
+static inline struct xilinx_pcs *pcs_to_xilinx(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct xilinx_pcs, pcs);
+}
+
+static void xilinx_pcs_get_state(struct phylink_pcs *pcs,
+				 struct phylink_link_state *state)
+{
+	struct xilinx_pcs *xp = pcs_to_xilinx(pcs);
+
+	switch (xp->standard) {
+	case XILINX_PCS_STD_SGMII:
+		state->interface = PHY_INTERFACE_MODE_SGMII;
+		break;
+	case XILINX_PCS_STD_1000BASEX:
+		state->interface = PHY_INTERFACE_MODE_1000BASEX;
+		break;
+	case XILINX_PCS_STD_BOTH: {
+		int ssr = mdiodev_read(xp->mdiodev, XILINX_PCS_SSR);
+
+		if (ssr < 0) {
+			dev_err(pcs->dev, "could not read SSR (err=%d)\n", ssr);
+			return;
+		}
+
+		if (ssr & XILINX_PCS_SSR_SGMII)
+			state->interface = PHY_INTERFACE_MODE_SGMII;
+		else
+			state->interface = PHY_INTERFACE_MODE_1000BASEX;
+		break;
+	}
+	case XILINX_PCS_STD_2500BASEX:
+		state->interface = PHY_INTERFACE_MODE_2500BASEX;
+		break;
+	default:
+		return;
+	}
+
+	phylink_mii_c22_pcs_get_state(xp->mdiodev, state);
+}
+
+static int xilinx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			     phy_interface_t interface,
+			     const unsigned long *advertising,
+			     bool permit_pause_to_mac)
+{
+	int ret;
+	bool changed = false;
+	struct xilinx_pcs *xp = pcs_to_xilinx(pcs);
+
+	switch (xp->standard) {
+	case XILINX_PCS_STD_SGMII:
+		if (interface != PHY_INTERFACE_MODE_SGMII)
+			return -EOPNOTSUPP;
+		break;
+	case XILINX_PCS_STD_1000BASEX:
+		if (interface != PHY_INTERFACE_MODE_1000BASEX)
+			return -EOPNOTSUPP;
+		break;
+	case XILINX_PCS_STD_BOTH: {
+		u16 ssr;
+
+		if (interface == PHY_INTERFACE_MODE_SGMII)
+			ssr = XILINX_PCS_SSR_SGMII;
+		else if (interface == PHY_INTERFACE_MODE_1000BASEX)
+			ssr = 0;
+		else
+			return -EOPNOTSUPP;
+
+		ret = mdiodev_read(xp->mdiodev, XILINX_PCS_SSR);
+		if (ret < 0)
+			return ret;
+
+		if (ret == ssr)
+			break;
+
+		changed = true;
+		ret = mdiodev_write(xp->mdiodev, XILINX_PCS_SSR, ssr);
+		if (ret)
+			return ret;
+		break;
+	}
+	case XILINX_PCS_STD_2500BASEX:
+		if (interface != PHY_INTERFACE_MODE_2500BASEX)
+			return -EOPNOTSUPP;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = phylink_mii_c22_pcs_config(xp->mdiodev, mode, interface,
+					 advertising);
+	if (ret)
+		return ret;
+	return changed;
+}
+
+static void xilinx_pcs_an_restart(struct phylink_pcs *pcs)
+{
+	struct xilinx_pcs *xp = pcs_to_xilinx(pcs);
+
+	phylink_mii_c22_pcs_an_restart(xp->mdiodev);
+}
+
+static void xilinx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+			       phy_interface_t interface, int speed, int duplex)
+{
+	int bmcr;
+	struct xilinx_pcs *xp = pcs_to_xilinx(pcs);
+
+	if (phylink_autoneg_inband(mode))
+		return;
+
+	bmcr = mdiodev_read(xp->mdiodev, MII_BMCR);
+	if (bmcr < 0) {
+		dev_err(pcs->dev, "could not read BMCR (err=%d)\n", bmcr);
+		return;
+	}
+
+	bmcr &= ~BMCR_FULLDPLX;
+	if (duplex == DUPLEX_FULL)
+		bmcr |= BMCR_FULLDPLX;
+	else if (duplex != DUPLEX_HALF)
+		dev_err(pcs->dev, "unknown duplex %d\n", duplex);
+
+	bmcr &= ~(BMCR_SPEED1000 | BMCR_SPEED100);
+	switch (speed) {
+	case SPEED_2500:
+	case SPEED_1000:
+		bmcr |= BMCR_SPEED1000;
+		break;
+	case SPEED_100:
+		bmcr |= BMCR_SPEED100;
+		break;
+	case SPEED_10:
+		bmcr |= BMCR_SPEED10;
+		break;
+	default:
+		dev_err(pcs->dev, "invalid speed %d\n", speed);
+	}
+
+	bmcr = mdiodev_write(xp->mdiodev, MII_BMCR, bmcr);
+	if (bmcr < 0)
+		dev_err(pcs->dev, "could not write BMCR (err=%d)\n", bmcr);
+}
+
+static const struct phylink_pcs_ops xilinx_pcs_ops = {
+	.pcs_get_state = xilinx_pcs_get_state,
+	.pcs_config = xilinx_pcs_config,
+	.pcs_an_restart = xilinx_pcs_an_restart,
+	.pcs_link_up = xilinx_pcs_link_up,
+};
+
+static int xilinx_pcs_probe(struct mdio_device *mdiodev)
+{
+	const char *standard;
+	int ret;
+	struct xilinx_pcs *xp;
+	struct device *dev = &mdiodev->dev;
+	struct device_node *np = dev->of_node;
+	u32 phy_id;
+
+	xp = devm_kzalloc(dev, sizeof(*xp), GFP_KERNEL);
+	if (!xp)
+		return -ENOMEM;
+	xp->mdiodev = mdiodev;
+	dev_set_drvdata(dev, xp);
+
+	ret = of_property_read_string(np, "standard", &standard);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not read standard\n");
+
+	if (!strncmp(standard, "1000base-x", 10))
+		xp->standard = XILINX_PCS_STD_1000BASEX;
+	else if (!strncmp(standard, "sgmii/1000base-x", 16))
+		xp->standard = XILINX_PCS_STD_BOTH;
+	else if (!strncmp(standard, "sgmii", 5))
+		xp->standard = XILINX_PCS_STD_SGMII;
+	else if (!strncmp(standard, "2500base-x", 10))
+		xp->standard = XILINX_PCS_STD_2500BASEX;
+	/* TODO: 2.5G SGMII support */
+	else
+		return dev_err_probe(dev, -EINVAL,
+				     "unknown/unsupported standard %s\n",
+				     standard);
+
+	xp->refclk = devm_clk_get(dev, "refclk");
+	if (IS_ERR(xp->refclk))
+		return dev_err_probe(dev, PTR_ERR(xp->refclk),
+				     "could not get reference clock\n");
+
+	xp->reset = devm_reset_control_get_exclusive(dev, "pcs");
+	if (IS_ERR(xp->reset))
+		return dev_err_probe(dev, PTR_ERR(xp->reset),
+				     "could not get reset\n");
+
+	ret = reset_control_assert(xp->reset);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not enter reset\n");
+
+	ret = clk_prepare_enable(xp->refclk);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "could not enable reference clock\n");
+
+	ret = reset_control_deassert(xp->reset);
+	if (ret) {
+		dev_err_probe(dev, ret, "could not exit reset\n");
+		goto err_unprepare;
+	}
+
+	/* Sanity check */
+	ret = get_phy_c22_id(mdiodev->bus, mdiodev->addr, &phy_id);
+	if (ret) {
+		dev_err_probe(dev, ret, "could not read id\n");
+		goto err_unprepare;
+	}
+	if ((phy_id & 0xfffffff0) != 0x01740c00)
+		dev_warn(dev, "unknown phy id %x\n", phy_id);
+
+	xp->pcs.dev = dev;
+	xp->pcs.ops = &xilinx_pcs_ops;
+	xp->pcs.poll = true;
+	ret = phylink_register_pcs(&xp->pcs);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not register PCS\n");
+	dev_info(dev, "probed (standard=%s)\n", standard);
+	return 0;
+
+err_unprepare:
+	clk_disable_unprepare(xp->refclk);
+	return ret;
+}
+
+static void xilinx_pcs_remove(struct mdio_device *mdiodev)
+{
+	struct xilinx_pcs *xp = dev_get_drvdata(&mdiodev->dev);
+
+	phylink_unregister_pcs(&xp->pcs);
+	reset_control_assert(xp->reset);
+	clk_disable_unprepare(xp->refclk);
+}
+
+static const struct of_device_id xilinx_pcs_of_match[] = {
+	{ .compatible = "xlnx,pcs-16.2", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
+
+static struct mdio_driver xilinx_pcs_driver = {
+	.probe = xilinx_pcs_probe,
+	.remove = xilinx_pcs_remove,
+	.mdiodrv.driver = {
+		.name = "xilinx-pcs",
+		.of_match_table = of_match_ptr(xilinx_pcs_of_match),
+	},
+};
+mdio_module_driver(xilinx_pcs_driver);
+
+MODULE_ALIAS("platform:xilinx-pcs");
+MODULE_DESCRIPTION("Xilinx PCS driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Sean Anderson]

Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
probe it, we might attach genphy anyway if addresses 2 and 3 return
something other than all 1s. To avoid this, add a quirk for these modules
so that we do not probe their PHY.

The particular module in this case is a Finisar SFP-GB-GE-T. This module is
also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
manually. However, I do not believe that it has a PHY in the first place:

$ i2cdump -y -r 0-31 $BUS 0x56 w
     0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
08: fc48 000e ff78 0000 0000 0000 0000 00f0
10: 7800 00bc 0000 401c 680c 0300 0000 0000
18: ff41 0000 0a00 8890 0000 0000 0000 0000

The first several addresses contain the same value, which should almost
never be the case for a proper phy. In addition, the "OUI" 00-7F-C3 does
not match Finisar's OUI of 00-90-65 (or any other OUI for that matter).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/phy/sfp-bus.c | 12 +++++++++++-
 drivers/net/phy/sfp.c     |  3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 7362f8c3271c..0b79893a79ea 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -14,6 +14,7 @@ struct sfp_quirk {
 	const char *vendor;
 	const char *part;
 	void (*modes)(const struct sfp_eeprom_id *id, unsigned long *modes);
+	bool ignore_phy;
 };
 
 /**
@@ -68,6 +69,12 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "ALCATELLUCENT",
 		.part = "3FE46541AA",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		// Finisar SFP-GB-GE-T has something on its I2C bus at
+		// SFP_PHY_ADDR, but it is not a (c22-compliant) phy
+		.vendor = "FS",
+		.part = "SFP-GB-GE-T",
+		.ignore_phy = true,
 	}, {
 		// Huawei MA5671A can operate at 2500base-X, but report 1.2GBd
 		// NRZ in their EEPROM
@@ -204,6 +211,9 @@ EXPORT_SYMBOL_GPL(sfp_parse_port);
  */
 bool sfp_may_have_phy(struct sfp_bus *bus, const struct sfp_eeprom_id *id)
 {
+	if (bus->sfp_quirk && bus->sfp_quirk->ignore_phy)
+		return false;
+
 	if (id->base.e1000_base_t)
 		return true;
 
@@ -370,7 +380,7 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 			phylink_set(modes, 2500baseX_Full);
 	}
 
-	if (bus->sfp_quirk)
+	if (bus->sfp_quirk && bus->sfp_quirk->modes)
 		bus->sfp_quirk->modes(id, modes);
 
 	bitmap_or(support, support, modes, __ETHTOOL_LINK_MODE_MASK_NBITS);
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index ab77a9f439ef..35c414eb1ecb 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1512,6 +1512,9 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 	struct phy_device *phy;
 	int err;
 
+	if (!sfp_may_have_phy(sfp->sfp_bus, &sfp->id))
+		return 0;
+
 	phy = get_phy_device(sfp->i2c_mii, SFP_PHY_ADDR, is_c45);
 	if (phy == ERR_PTR(-ENODEV))
 		return PTR_ERR(phy);
-- 
2.25.1

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Andrew Lunn]

On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
> Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
> probe it, we might attach genphy anyway if addresses 2 and 3 return
> something other than all 1s. To avoid this, add a quirk for these modules
> so that we do not probe their PHY.
> 
> The particular module in this case is a Finisar SFP-GB-GE-T. This module is
> also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
> manually. However, I do not believe that it has a PHY in the first place:
> 
> $ i2cdump -y -r 0-31 $BUS 0x56 w
>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
> 08: fc48 000e ff78 0000 0000 0000 0000 00f0
> 10: 7800 00bc 0000 401c 680c 0300 0000 0000
> 18: ff41 0000 0a00 8890 0000 0000 0000 0000
> 
> The first several addresses contain the same value, which should almost
> never be the case for a proper phy. In addition, the "OUI" 00-7F-C3 does
> not match Finisar's OUI of 00-90-65 (or any other OUI for that matter).
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Hi Sean

This does not really have anything to do with PCS. I would send it on
its own.

    Andrew

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Russell King (Oracle)]

On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
> Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
> probe it, we might attach genphy anyway if addresses 2 and 3 return
> something other than all 1s. To avoid this, add a quirk for these modules
> so that we do not probe their PHY.
> 
> The particular module in this case is a Finisar SFP-GB-GE-T. This module is
> also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
> manually. However, I do not believe that it has a PHY in the first place:
> 
> $ i2cdump -y -r 0-31 $BUS 0x56 w
>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
> 08: fc48 000e ff78 0000 0000 0000 0000 00f0
> 10: 7800 00bc 0000 401c 680c 0300 0000 0000
> 18: ff41 0000 0a00 8890 0000 0000 0000 0000

Actually, I think that is a PHY. It's byteswapped (which is normal using
i2cdump in this way). The real contents of the registers are:

00: 01ff 01ff 01ff 0cc2 0c01 c001 000f 2001
08: 48fc 0e00 78ff 0000 0000 0000 0000 f000
10: 0078 bc00 0000 1c40 0c68 0003 0000 0000
18: 41ff 0000 000a 9088 0000 0000 0000 0000

It's advertising pause + asym pause, 1000BASE-T FD, link partner is also
advertising 1000BASE-T FD but no pause abilities.

When comparing this with a Marvell 88e1111:

00: 1140 7949 0141 0cc2 05e1 0000 0004 2001
08: 0000 0e00 4000 0000 0000 0000 0000 f000
10: 0078 8100 0000 0040 0568 0000 0000 0000
18: 4100 0000 0002 8084 0000 0000 0000 0000

It looks remarkably similar. However, The first few reads seem to be
corrupted with 0x01ff. It may be that the module is slow to allow the
PHY to start responding - we've had similar with Champion One SFPs.

It looks like it's a Marvell 88e1111. The register at 0x11 is the
Marvell status register, and 0xbc00 indicates 1000Mbit, FD, AN
resolved, link up which agrees with what's in the various other
registers.

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

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Sean Anderson]

On 10/5/21 6:33 AM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
>> Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
>> probe it, we might attach genphy anyway if addresses 2 and 3 return
>> something other than all 1s. To avoid this, add a quirk for these modules
>> so that we do not probe their PHY.
>>
>> The particular module in this case is a Finisar SFP-GB-GE-T. This module is
>> also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
>> manually. However, I do not believe that it has a PHY in the first place:
>>
>> $ i2cdump -y -r 0-31 $BUS 0x56 w
>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
>> 08: fc48 000e ff78 0000 0000 0000 0000 00f0
>> 10: 7800 00bc 0000 401c 680c 0300 0000 0000
>> 18: ff41 0000 0a00 8890 0000 0000 0000 0000
>
> Actually, I think that is a PHY. It's byteswapped (which is normal using
> i2cdump in this way).The real contents of the registers are:
>
> 00: 01ff 01ff 01ff 0cc2 0c01 c001 000f 2001
> 08: 48fc 0e00 78ff 0000 0000 0000 0000 f000
> 10: 0078 bc00 0000 1c40 0c68 0003 0000 0000
> 18: 41ff 0000 000a 9088 0000 0000 0000 0000

Ah, thanks for catching this.

> It's advertising pause + asym pause, 1000BASE-T FD, link partner is also
> advertising 1000BASE-T FD but no pause abilities.
>
> When comparing this with a Marvell 88e1111:
>
> 00: 1140 7949 0141 0cc2 05e1 0000 0004 2001
> 08: 0000 0e00 4000 0000 0000 0000 0000 f000
> 10: 0078 8100 0000 0040 0568 0000 0000 0000
> 18: 4100 0000 0002 8084 0000 0000 0000 0000
>
> It looks remarkably similar. However, The first few reads seem to be
> corrupted with 0x01ff. It may be that the module is slow to allow the
> PHY to start responding - we've had similar with Champion One SFPs.

Do you have an an example of how to work around this? Even reading one
register at a time I still get the bogus 0x01ff. Reading bytewise, a
reasonable-looking upper byte is returned every other read, but the
lower byte is 0xff every time.

> It looks like it's a Marvell 88e1111. The register at 0x11 is the
> Marvell status register, and 0xbc00 indicates 1000Mbit, FD, AN
> resolved, link up which agrees with what's in the various other
> registers.

That matches some supplemental info on the manufacturer's website (which
was frustratingly not associated with the model number of this
particular module).

--Sean

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Sean Anderson]

On 10/5/21 12:45 PM, Sean Anderson wrote:
>
>
> On 10/5/21 6:33 AM, Russell King (Oracle) wrote:
>> On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
>>> Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
>>> probe it, we might attach genphy anyway if addresses 2 and 3 return
>>> something other than all 1s. To avoid this, add a quirk for these modules
>>> so that we do not probe their PHY.
>>>
>>> The particular module in this case is a Finisar SFP-GB-GE-T. This module is
>>> also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
>>> manually. However, I do not believe that it has a PHY in the first place:
>>>
>>> $ i2cdump -y -r 0-31 $BUS 0x56 w
>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>> 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
>>> 08: fc48 000e ff78 0000 0000 0000 0000 00f0
>>> 10: 7800 00bc 0000 401c 680c 0300 0000 0000
>>> 18: ff41 0000 0a00 8890 0000 0000 0000 0000
>>
>> Actually, I think that is a PHY. It's byteswapped (which is normal using
>> i2cdump in this way).The real contents of the registers are:
>>
>> 00: 01ff 01ff 01ff 0cc2 0c01 c001 000f 2001
>> 08: 48fc 0e00 78ff 0000 0000 0000 0000 f000
>> 10: 0078 bc00 0000 1c40 0c68 0003 0000 0000
>> 18: 41ff 0000 000a 9088 0000 0000 0000 0000
>
> Ah, thanks for catching this.
>
>> It's advertising pause + asym pause, 1000BASE-T FD, link partner is also
>> advertising 1000BASE-T FD but no pause abilities.
>>
>> When comparing this with a Marvell 88e1111:
>>
>> 00: 1140 7949 0141 0cc2 05e1 0000 0004 2001
>> 08: 0000 0e00 4000 0000 0000 0000 0000 f000
>> 10: 0078 8100 0000 0040 0568 0000 0000 0000
>> 18: 4100 0000 0002 8084 0000 0000 0000 0000
>>
>> It looks remarkably similar. However, The first few reads seem to be
>> corrupted with 0x01ff. It may be that the module is slow to allow the
>> PHY to start responding - we've had similar with Champion One SFPs.
>
> Do you have an an example of how to work around this? Even reading one
> register at a time I still get the bogus 0x01ff. Reading bytewise, a
> reasonable-looking upper byte is returned every other read, but the
> lower byte is 0xff every time.

Ok, upon further experimentation, I can read out something reasonable by using the "consecutive byte" mode

# i2cdump -y -r 0-0x3f 2 0x56 c
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 01 40 01 6d 01 41 0c c2 0c 01 c5 e1 00 0d 20 01    ?@?m?A??????.? ?
10: 59 f9 0e 00 78 00 00 00 00 00 00 00 00 00 f0 00    Y??.x.........?.
20: 00 78 ac 40 00 00 00 00 0c 68 00 00 00 00 00 00    .x?@....?h......
30: 41 00 00 00 00 0a 90 88 00 00 00 00 00 00 00 00    A....???........

I believe this is just doing i2c_smbus_write_byte+i2c_smbus_read_byte

S Addr Wr [A] Phyaddr [A] P
S Addr Rd [A] [DataHigh] NA P
S Addr Rd [A] [DataLow] NA P

as opposed to i2c_smbus_read_word_data

S Addr Wr [A] Phyaddr [A]
S Addr Rd [A] [DataHigh] A [DataLow] NA P

or i2c_smbus_read_word_data

S Addr Wr [A] Phyaddr [A]
S Addr Rd [A] [DataHigh] NA P
S Addr Wr [A] Phyaddr [A]
S Addr Rd [A] [DataLow] NA P

So now I suppose the question is whether replacing the existing i2c
logic with something like

	i2c_smbus_write_byte(i2c, i2c_mii_phy_addr(phy_id));
	i2c_smbus_read_byte(i2c);
	i2c_smbus_read_byte(i2c);

will break everyone else's SFP phys. If it does, this could be difficult
to do as a quirk because the MII-I2C bus is created before we read the
SFP EEPROM.

--Sean

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 12:45:28PM -0400, Sean Anderson wrote:
> 
> 
> On 10/5/21 6:33 AM, Russell King (Oracle) wrote:
> > On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
> > > Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
> > > probe it, we might attach genphy anyway if addresses 2 and 3 return
> > > something other than all 1s. To avoid this, add a quirk for these modules
> > > so that we do not probe their PHY.
> > > 
> > > The particular module in this case is a Finisar SFP-GB-GE-T. This module is
> > > also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
> > > manually. However, I do not believe that it has a PHY in the first place:
> > > 
> > > $ i2cdump -y -r 0-31 $BUS 0x56 w
> > >      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> > > 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
> > > 08: fc48 000e ff78 0000 0000 0000 0000 00f0
> > > 10: 7800 00bc 0000 401c 680c 0300 0000 0000
> > > 18: ff41 0000 0a00 8890 0000 0000 0000 0000
> > 
> > Actually, I think that is a PHY. It's byteswapped (which is normal using
> > i2cdump in this way).The real contents of the registers are:
> > 
> > 00: 01ff 01ff 01ff 0cc2 0c01 c001 000f 2001
> > 08: 48fc 0e00 78ff 0000 0000 0000 0000 f000
> > 10: 0078 bc00 0000 1c40 0c68 0003 0000 0000
> > 18: 41ff 0000 000a 9088 0000 0000 0000 0000
> 
> Ah, thanks for catching this.
> 
> > It's advertising pause + asym pause, 1000BASE-T FD, link partner is also
> > advertising 1000BASE-T FD but no pause abilities.
> > 
> > When comparing this with a Marvell 88e1111:
> > 
> > 00: 1140 7949 0141 0cc2 05e1 0000 0004 2001
> > 08: 0000 0e00 4000 0000 0000 0000 0000 f000
> > 10: 0078 8100 0000 0040 0568 0000 0000 0000
> > 18: 4100 0000 0002 8084 0000 0000 0000 0000
> > 
> > It looks remarkably similar. However, The first few reads seem to be
> > corrupted with 0x01ff. It may be that the module is slow to allow the
> > PHY to start responding - we've had similar with Champion One SFPs.
> 
> Do you have an an example of how to work around this? Even reading one
> register at a time I still get the bogus 0x01ff. Reading bytewise, a
> reasonable-looking upper byte is returned every other read, but the
> lower byte is 0xff every time.

I think the Champion One modules just don't respond to the I2C
transactions, so we keep retrying for a while. We try every
50ms for 12 retries, which seems to be long enough for their
modules.

> > It looks like it's a Marvell 88e1111. The register at 0x11 is the
> > Marvell status register, and 0xbc00 indicates 1000Mbit, FD, AN
> > resolved, link up which agrees with what's in the various other
> > registers.
> 
> That matches some supplemental info on the manufacturer's website
> (which was frustratingly not associated with the model number of
> this particular module).

The interesting thing is, many modules use 88e1111, which is about
the only PHY that I'm aware that supports I2C access mode natively.
So, it's really surprising that you're getting corrupted data,
unless...

There's been a history of using too strong pull-ups on the SFP I2C
lines. The SFP MSA gives a minimum value of the resistors (4.7k).
SFP+ lowers the minimum value and raises the maximum clock frequency.
Some SFP modules are unable to drive the I2C bus low against the
lower resistances resulting in corrupted data (or worse, it can
corrupt the EEPROMs.)

Other problems on some platforms have been with I2C level shifters
locking up, but that doesn't look like what's happening here - they
lockup at logic low not logic high. Even so-called "impossible to
lockup" level shifters have locked up despite their manufacturer
stating that it is impossible.

Is it always the same addresses? What if you read from a different
offset? What if you re-read after it seems to have cleared?

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

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Sean Anderson]

On 10/5/21 3:12 PM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:45:28PM -0400, Sean Anderson wrote:
>>
>>
>> On 10/5/21 6:33 AM, Russell King (Oracle) wrote:
>> > On Mon, Oct 04, 2021 at 03:15:27PM -0400, Sean Anderson wrote:
>> > > Some modules have something at SFP_PHY_ADDR which isn't a PHY. If we try to
>> > > probe it, we might attach genphy anyway if addresses 2 and 3 return
>> > > something other than all 1s. To avoid this, add a quirk for these modules
>> > > so that we do not probe their PHY.
>> > >
>> > > The particular module in this case is a Finisar SFP-GB-GE-T. This module is
>> > > also worked around in xgbe_phy_finisar_phy_quirks() by setting the support
>> > > manually. However, I do not believe that it has a PHY in the first place:
>> > >
>> > > $ i2cdump -y -r 0-31 $BUS 0x56 w
>> > >      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> > > 00: ff01 ff01 ff01 c20c 010c 01c0 0f00 0120
>> > > 08: fc48 000e ff78 0000 0000 0000 0000 00f0
>> > > 10: 7800 00bc 0000 401c 680c 0300 0000 0000
>> > > 18: ff41 0000 0a00 8890 0000 0000 0000 0000
>> >
>> > Actually, I think that is a PHY. It's byteswapped (which is normal using
>> > i2cdump in this way).The real contents of the registers are:
>> >
>> > 00: 01ff 01ff 01ff 0cc2 0c01 c001 000f 2001
>> > 08: 48fc 0e00 78ff 0000 0000 0000 0000 f000
>> > 10: 0078 bc00 0000 1c40 0c68 0003 0000 0000
>> > 18: 41ff 0000 000a 9088 0000 0000 0000 0000
>>
>> Ah, thanks for catching this.
>>
>> > It's advertising pause + asym pause, 1000BASE-T FD, link partner is also
>> > advertising 1000BASE-T FD but no pause abilities.
>> >
>> > When comparing this with a Marvell 88e1111:
>> >
>> > 00: 1140 7949 0141 0cc2 05e1 0000 0004 2001
>> > 08: 0000 0e00 4000 0000 0000 0000 0000 f000
>> > 10: 0078 8100 0000 0040 0568 0000 0000 0000
>> > 18: 4100 0000 0002 8084 0000 0000 0000 0000
>> >
>> > It looks remarkably similar. However, The first few reads seem to be
>> > corrupted with 0x01ff. It may be that the module is slow to allow the
>> > PHY to start responding - we've had similar with Champion One SFPs.
>>
>> Do you have an an example of how to work around this? Even reading one
>> register at a time I still get the bogus 0x01ff. Reading bytewise, a
>> reasonable-looking upper byte is returned every other read, but the
>> lower byte is 0xff every time.
>
> I think the Champion One modules just don't respond to the I2C
> transactions, so we keep retrying for a while. We try every
> 50ms for 12 retries, which seems to be long enough for their
> modules.
>
>> > It looks like it's a Marvell 88e1111. The register at 0x11 is the
>> > Marvell status register, and 0xbc00 indicates 1000Mbit, FD, AN
>> > resolved, link up which agrees with what's in the various other
>> > registers.
>>
>> That matches some supplemental info on the manufacturer's website
>> (which was frustratingly not associated with the model number of
>> this particular module).
>
> The interesting thing is, many modules use 88e1111, which is about
> the only PHY that I'm aware that supports I2C access mode natively.
> So, it's really surprising that you're getting corrupted data,
> unless...
>
> There's been a history of using too strong pull-ups on the SFP I2C
> lines. The SFP MSA gives a minimum value of the resistors (4.7k).
> SFP+ lowers the minimum value and raises the maximum clock frequency.
> Some SFP modules are unable to drive the I2C bus low against the
> lower resistances resulting in corrupted data (or worse, it can
> corrupt the EEPROMs.)

There is a level shifter. Between the shifter and the SoC there were
1.8k (!) pull-ups, and between the shifter and the SFP there were 10k
pull-ups. I tried replacing the pull-ups between the SoC and the shifter
with 10k pull-ups, but noticed no difference. I have also noticed no
issues accessing the EEPROM, and I have not noticed any difference
accessing other registers (see below). Additionally, this same error is
"present" already in xgbe_phy_finisar_phy_quirks(), as noted in the
commit message.

> Other problems on some platforms have been with I2C level shifters
> locking up, but that doesn't look like what's happening here - they
> lockup at logic low not logic high. Even so-called "impossible to
> lockup" level shifters have locked up despite their manufacturer
> stating that it is impossible.
>
> Is it always the same addresses?

Yes.

> What if you read from a different offset?

Same thing.

> What if you re-read after it seems to have cleared?

Here are some various transfers which hopefully will clarify the
behavior:

First, reading two bytes at a time
	$ i2ctransfer -y 2 w1@0x56 2 r2
	0x01 0xff
This behavior is repeatable
	$ i2ctransfer -y 2 w1@0x56 2 r2
	0x01 0xff
Now, reading one byte at a time
	$ i2ctransfer -y 2 w1@0x56 2 r1
	0x01
A second write/single read gets us the first byte again.
	$ i2ctransfer -y 2 w1@0x56 2 r1
	0x41
And doing it for a third time gets us the first byte again.
	$ i2ctransfer -y 2 w1@0x56 2 r1
	0x01
If we start another one-byte read without writing the address, we get
the second byte
	$ i2ctransfer -y 2 r1@0x56
	0x41
And continuing this pattern, we get the next byte.
	$ i2ctransfer -y 2 r1@0x56
	0x0c
This can be repeated indefinitely
	$ i2ctransfer -y 2 r1@0x56
	0xc2
	$ i2ctransfer -y 2 r1@0x56
	0x0c
But stopping in the "middle" of a register fails
	$ i2ctransfer -y 2 w1@0x56 2 r1
	Error: Sending messages failed: Input/output error
We don't have to immediately read a byte:
	$ i2ctransfer -y 2 w1@0x56 2
	$ i2ctransfer -y 2 r1@0x56
	0x01
	$ i2ctransfer -y 2 r1@0x56
	0x41
We can read two bytes indefinitely after "priming the pump"
	$ i2ctransfer -y 2 w1@0x56 2 r1
	0x01
	$ i2ctransfer -y 2 r1@0x56
	0x41
	$ i2ctransfer -y 2 r2@0x56
	0x0c 0xc2
	$ i2ctransfer -y 2 r2@0x56
	0x0c 0x01
	$ i2ctransfer -y 2 r2@0x56
	0x00 0x00
	$ i2ctransfer -y 2 r2@0x56
	0x00 0x04
	$ i2ctransfer -y 2 r2@0x56
	0x20 0x01
	$ i2ctransfer -y 2 r2@0x56
	0x00 0x00
But more than that "runs out"
	$ i2ctransfer -y 2 w1@0x56 2 r1
	0x01
	$ i2ctransfer -y 2 r1@0x56
	0x41
	$ i2ctransfer -y 2 r4@0x56
	0x0c 0xc2 0x0c 0x01
	$ i2ctransfer -y 2 r4@0x56
	0x00 0x00 0x00 0x04
	$ i2ctransfer -y 2 r4@0x56
	0x20 0x01 0xff 0xff
	$ i2ctransfer -y 2 r4@0x56
	0x01 0xff 0xff 0xff
However, the above multi-byte reads only works when starting at register
2 or greater.
	$ i2ctransfer -y 2 w1@0x56 0 r1
	0x01
	$ i2ctransfer -y 2 r1@0x56
	0x40
	$ i2ctransfer -y 2 r2@0x56
	0x01 0xff

Based on the above session, I believe that it may be best to treat this
phy as having an autoincrementing register address which must be read
one byte at a time, in multiples of two bytes. I think that existing SFP
phys may compatible with this, but unfortunately I do not have any on
hand to test with.

--Sean

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Russell King (Oracle)]

On Tue, Oct 05, 2021 at 04:38:23PM -0400, Sean Anderson wrote:
> There is a level shifter. Between the shifter and the SoC there were
> 1.8k (!) pull-ups, and between the shifter and the SFP there were 10k
> pull-ups. I tried replacing the pull-ups between the SoC and the shifter
> with 10k pull-ups, but noticed no difference. I have also noticed no
> issues accessing the EEPROM, and I have not noticed any difference
> accessing other registers (see below). Additionally, this same error is
> "present" already in xgbe_phy_finisar_phy_quirks(), as noted in the
> commit message.

Hmm, thanks for checking. So it's something "weird" that this module
is doing.

As I say, the 88E1111 has a native I2C mode, it sounds like they're not
using it but have their own, seemingly broken, protocol conversion from
the I2C bus to MDIO. I've opened and traced the I2C connections on this
module - they only go to an EEPROM and the 88E1111, so we know this is
a "genuine" 88E1111 in I2C mode we are talking to.

> First, reading two bytes at a time
> 	$ i2ctransfer -y 2 w1@0x56 2 r2
> 	0x01 0xff
> This behavior is repeatable
> 	$ i2ctransfer -y 2 w1@0x56 2 r2
> 	0x01 0xff
> Now, reading one byte at a time
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	0x01
> A second write/single read gets us the first byte again.
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	0x41

I think you mean you get the other half of the first word. When I try
this with a 88E1111 directly connected to the I2C bus, I get:

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r2
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r2
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
0x01
root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
0x01

So a completely different behaviour. Continuing...

> And doing it for a third time gets us the first byte again.
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	0x01
> If we start another one-byte read without writing the address, we get
> the second byte
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x41

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
0x01
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01

Again, different behaviour.

> And continuing this pattern, we get the next byte.
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x0c
> This can be repeated indefinitely
> 	$ i2ctransfer -y 2 r1@0x56
> 	0xc2
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x0c

root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x41
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01

Here we eventually start toggling between the high and low bytes of
the word.

> But stopping in the "middle" of a register fails
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	Error: Sending messages failed: Input/output error

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
0x01

No error for me.

> We don't have to immediately read a byte:
> 	$ i2ctransfer -y 2 w1@0x56 2
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x01
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x41

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01

Again, no toggling between high/low bytes of the word.

> We can read two bytes indefinitely after "priming the pump"
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	0x01
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x41
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x0c 0xc2
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x0c 0x01
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x00 0x00
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x00 0x04
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x20 0x01
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x00 0x00

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41
root@clearfog21:~# i2ctransfer -y 1 r2@0x56
0x01 0x41

No auto-increment of the register.

> But more than that "runs out"
> 	$ i2ctransfer -y 2 w1@0x56 2 r1
> 	0x01
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x41
> 	$ i2ctransfer -y 2 r4@0x56
> 	0x0c 0xc2 0x0c 0x01
> 	$ i2ctransfer -y 2 r4@0x56
> 	0x00 0x00 0x00 0x04
> 	$ i2ctransfer -y 2 r4@0x56
> 	0x20 0x01 0xff 0xff
> 	$ i2ctransfer -y 2 r4@0x56
> 	0x01 0xff 0xff 0xff

root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r1@0x56
0x01
root@clearfog21:~# i2ctransfer -y 1 r4@0x56
0x01 0x41 0x0c 0xc2
root@clearfog21:~# i2ctransfer -y 1 r4@0x56
0x01 0x41 0x0c 0xc2
root@clearfog21:~# i2ctransfer -y 1 r4@0x56
0x01 0x41 0x0c 0xc2
root@clearfog21:~# i2ctransfer -y 1 r4@0x56
0x01 0x41 0x0c 0xc2

> However, the above multi-byte reads only works when starting at register
> 2 or greater.
> 	$ i2ctransfer -y 2 w1@0x56 0 r1
> 	0x01
> 	$ i2ctransfer -y 2 r1@0x56
> 	0x40
> 	$ i2ctransfer -y 2 r2@0x56
> 	0x01 0xff
> 
> Based on the above session, I believe that it may be best to treat this
> phy as having an autoincrementing register address which must be read
> one byte at a time, in multiples of two bytes. I think that existing SFP
> phys may compatible with this, but unfortunately I do not have any on
> hand to test with.

Sadly, according to my results above, I think your module is doing
something strange with the 88E1111.

You say that it's Finisar, but I can only find this module in
Fiberstore's website: https://www.fs.com/uk/products/20057.html
Fiberstore commonly use "FS" in the vendor field.

You have me wondering what they've done to this PHY to make it respond
in the way you are seeing.

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

Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs

[Sean Anderson]

On 10/5/21 6:17 PM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 04:38:23PM -0400, Sean Anderson wrote:
>> There is a level shifter. Between the shifter and the SoC there were
>> 1.8k (!) pull-ups, and between the shifter and the SFP there were 10k
>> pull-ups. I tried replacing the pull-ups between the SoC and the shifter
>> with 10k pull-ups, but noticed no difference. I have also noticed no
>> issues accessing the EEPROM, and I have not noticed any difference
>> accessing other registers (see below). Additionally, this same error is
>> "present" already in xgbe_phy_finisar_phy_quirks(), as noted in the
>> commit message.
>
> Hmm, thanks for checking. So it's something "weird" that this module
> is doing.
>
> As I say, the 88E1111 has a native I2C mode, it sounds like they're not
> using it but have their own, seemingly broken, protocol conversion from
> the I2C bus to MDIO. I've opened and traced the I2C connections on this
> module - they only go to an EEPROM and the 88E1111, so we know this is
> a "genuine" 88E1111 in I2C mode we are talking to.

Well, I had a look inside mine and it had a "Custom Code/Die Revision"
of B2. Nothing else unusual. Just the PHY, magnetics, EEPROM, crystal,
and a regulator.

>> First, reading two bytes at a time
>> 	$ i2ctransfer -y 2 w1@0x56 2 r2
>> 	0x01 0xff
>> This behavior is repeatable
>> 	$ i2ctransfer -y 2 w1@0x56 2 r2
>> 	0x01 0xff
>> Now, reading one byte at a time
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	0x01
>> A second write/single read gets us the first byte again.
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	0x41
>
> I think you mean you get the other half of the first word.

Yes.

> When I try this with a 88E1111 directly connected to the I2C bus, I
> get:
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r2
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r2
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
> 0x01
>
> So a completely different behaviour. Continuing...
>
>> And doing it for a third time gets us the first byte again.
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	0x01
>> If we start another one-byte read without writing the address, we get
>> the second byte
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x41
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
>
> Again, different behaviour.
>
>> And continuing this pattern, we get the next byte.
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x0c
>> This can be repeated indefinitely
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0xc2
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x0c
>
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x41
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
>
> Here we eventually start toggling between the high and low bytes of
> the word.
>
>> But stopping in the "middle" of a register fails
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	Error: Sending messages failed: Input/output error
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2 r1
> 0x01
>
> No error for me.
>
>> We don't have to immediately read a byte:
>> 	$ i2ctransfer -y 2 w1@0x56 2
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x41
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
>
> Again, no toggling between high/low bytes of the word.
>
>> We can read two bytes indefinitely after "priming the pump"
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x41
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x0c 0xc2
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x0c 0x01
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x00 0x00
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x00 0x04
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x20 0x01
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x00 0x00
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
> root@clearfog21:~# i2ctransfer -y 1 r2@0x56
> 0x01 0x41
>
> No auto-increment of the register.
>
>> But more than that "runs out"
>> 	$ i2ctransfer -y 2 w1@0x56 2 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x41
>> 	$ i2ctransfer -y 2 r4@0x56
>> 	0x0c 0xc2 0x0c 0x01
>> 	$ i2ctransfer -y 2 r4@0x56
>> 	0x00 0x00 0x00 0x04
>> 	$ i2ctransfer -y 2 r4@0x56
>> 	0x20 0x01 0xff 0xff
>> 	$ i2ctransfer -y 2 r4@0x56
>> 	0x01 0xff 0xff 0xff
>
> root@clearfog21:~# i2ctransfer -y 1 w1@0x56 2
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r1@0x56
> 0x01
> root@clearfog21:~# i2ctransfer -y 1 r4@0x56
> 0x01 0x41 0x0c 0xc2
> root@clearfog21:~# i2ctransfer -y 1 r4@0x56
> 0x01 0x41 0x0c 0xc2
> root@clearfog21:~# i2ctransfer -y 1 r4@0x56
> 0x01 0x41 0x0c 0xc2
> root@clearfog21:~# i2ctransfer -y 1 r4@0x56
> 0x01 0x41 0x0c 0xc2
>
>> However, the above multi-byte reads only works when starting at register
>> 2 or greater.
>> 	$ i2ctransfer -y 2 w1@0x56 0 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@0x56
>> 	0x40
>> 	$ i2ctransfer -y 2 r2@0x56
>> 	0x01 0xff
>>
>> Based on the above session, I believe that it may be best to treat this
>> phy as having an autoincrementing register address which must be read
>> one byte at a time, in multiples of two bytes. I think that existing SFP
>> phys may compatible with this, but unfortunately I do not have any on
>> hand to test with.
>
> Sadly, according to my results above, I think your module is doing
> something strange with the 88E1111.
>
> You say that it's Finisar, but I can only find this module in
> Fiberstore's website: https://www.fs.com/uk/products/20057.html
> Fiberstore commonly use "FS" in the vendor field.

So you are correct. I had seen the xgbe fixup for the same phy_id and
assumed that FS meant the same thing in both cases. You may also notice
that nowhere on their site do they spell out their name.

> You have me wondering what they've done to this PHY to make it respond
> in the way you are seeing.

You may notice on their site that there are different "compatible"
options for e.g. "FS", "Dell", "Cisco", and around 50 other
manufacturers. I wonder if I've come across a module which supports
autoincrementing byte reads in order to be compatible with some
manufacturer's hardware. And perhaps this change has inadvertently
broken the two-byte read capability, but only for the first 3 registers.

--Sean