Re: [PATCH v3 phy 12/17] dt-bindings: phy: lynx-28g: add compatible strings per SerDes and instantiation

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Sep 26, 2025 at 09:05:00PM +0300, Vladimir Oltean wrote:
> Going by the generic "fsl,lynx-28g" compatible string and expecting all
> SerDes instantiations on all SoCs to use it was a mistake.
> 
> They all share the same register map, sure, but the number of protocol
> converters and lanes which are instantiated differs in a way that isn't
> detectable by the programming interface.
> 
> Using a separate compatible string per SerDes instantiation is
> sufficient for any device driver to distinguish these features and/or
> any instance-specific quirk. It also reflects how the SoC reference
> manual provides different tables with protocol combinations for each
> SerDes. NXP clearly documents these as not identical, and refers to them
> as such (SerDes 1, 2, etc).
> 
> The other sufficient approach for Lynx 28G would be to list in the
> device tree all protocols supported by each lane. That would be
> insufficient for the very similar Lynx 10G SerDes however, for which
> there exists a higher degree of variability in the PCCR register values
> that need to be written per protocol. This attempt can be seen in this
> unmerged patch set for Lynx 10G:
> https://lore.kernel.org/linux-phy/20230413160607.4128315-3-sean.anderson@seco.com/
> 
> but that approach is more drawn-out and more prone to errors, whereas
> this one is more succinct and obviously correct.
> 
> One aspect which is different with the per-SoC compatible strings is
> that they have one PHY provider for each lane (and #phy-cells = <0> in
> lane sub-nodes), rather than "fsl,lynx-28g" which has a single PHY
> provider for all lanes (and #phy-cells = <1> in the top-level node).
> 
> This is done to fulfill Josua Mayer's request:
> https://lore.kernel.org/lkml/02270f62-9334-400c-b7b9-7e6a44dbbfc9@solid-run.com/
> to have OF nodes for each lane, so that we can further apply schemas
> such as Documentation/devicetree/bindings/phy/transmit-amplitude.yaml
> individually.
> 
> This is the easiest and most intuitive way to describe that. The above
> is not the only electrical tuning that needs to be done, but rather the
> only one currently standardized in a schema. TX equalization parameters
> are TBD, but we need to not limit ourselves to just what currently exists.
> 
> Luckily, we can overlap the modern binding format over the legacy one
> and they can coexist without interfering. Old kernels use compatible =
> "fsl,lynx-28g" and the top-level PHY provider, whereas new kernels probe
> on e.g. compatible = "fsl,lx2160a-serdes1" and use the per-lane PHY
> providers.
> 
> Overlaying modern on top of legacy is only necessary for SerDes 1 and 2.
> LX2160A SerDes #3 (a non-networking SerDes) is not yet present in any
> device trees in circulation, and will only have the device-specific
> compatible (even though it shares the Lynx 28G programming model,
> specifying the "fsl,lynx-28g" compatible string for it provides no
> benefit that I can see).
> 
> Change the expected name of the top-level node to "serdes", and update
> the example too.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v2->v3:
> - re-add "fsl,lynx-28g" as fallback compatible, and #phy-cells = <1> in
>   top-level "serdes" node
> - drop useless description texts
> - fix text formatting
> - schema is more lax to allow overlaying old and new required properties
> v1->v2:
> - drop the usage of "fsl,lynx-28g" as a fallback compatible
> - mark "fsl,lynx-28g" as deprecated
> - implement Josua's request for per-lane OF nodes for the new compatible
>   strings
> 
>  .../devicetree/bindings/phy/fsl,lynx-28g.yaml | 159 +++++++++++++++++-
>  1 file changed, 152 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> index ff9f9ca0f19c..e8b3a48b9515 100644
> --- a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> @@ -9,21 +9,123 @@ title: Freescale Lynx 28G SerDes PHY
>  maintainers:
>    - Ioana Ciornei <ioana.ciornei@nxp.com>
>  
> +description:
> +  The Lynx 28G is a multi-lane, multi-protocol SerDes (PCIe, SATA, Ethernet)
> +  present in multiple instances on NXP LX2160A and LX2162A SoCs. All instances
> +  share a common register map and programming model, however they differ in
> +  supported protocols per lane in a way that is not detectable by said
> +  programming model without prior knowledge. The distinction is made through
> +  the compatible string.
> +
>  properties:
>    compatible:
> -    enum:
> -      - fsl,lynx-28g
> +    oneOf:
> +      - const: fsl,lynx-28g
> +        deprecated: true
> +        description:
> +          Legacy compatibility string for Lynx 28G SerDes. Any assumption
> +          regarding whether a certain lane supports a certain protocol may
> +          be incorrect. Deprecated except when used as a fallback. Use
> +          device-specific strings instead.
> +      - items:
> +          - const: fsl,lx2160a-serdes1
> +          - const: fsl,lynx-28g
> +      - items:
> +          - const: fsl,lx2160a-serdes2
> +          - const: fsl,lynx-28g
> +      - items:
> +          - const: fsl,lx2162a-serdes1
> +          - const: fsl,lynx-28g
> +      - items:
> +          - const: fsl,lx2162a-serdes2
> +          - const: fsl,lynx-28g
> +      - const: fsl,lx2160a-serdes3
>  
>    reg:
>      maxItems: 1
>  
> -  "#phy-cells":
> -    const: 1
> +  "#address-cells": true
> +
> +  "#size-cells": true
> +
> +  "#phy-cells": true
> +
> +patternProperties:
> +  "^phy@[0-9a-f]+$":
> +    type: object
> +    description: Individual SerDes lane acting as PHY provider
> +
> +    properties:
> +      reg:
> +        description: Lane index as seen in register map
> +        maxItems: 1
> +
> +      "#phy-cells":
> +        const: 0
> +
> +    required:
> +      - reg
> +      - "#phy-cells"
> +
> +    additionalProperties: false
>  
>  required:
>    - compatible
>    - reg
> -  - "#phy-cells"
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,lynx-28g
> +    then:
> +      # Legacy case: parent is the PHY provider, cell encodes lane index
> +      properties:
> +        "#phy-cells":
> +          const: 1
> +      required:
> +        - "#phy-cells"
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,lx2160a-serdes1
> +              - fsl,lx2160a-serdes2
> +              - fsl,lx2160a-serdes3
> +              - fsl,lx2162a-serdes1
> +              - fsl,lx2162a-serdes2
> +    then:
> +      # Modern binding: lanes must have their own nodes
> +      properties:
> +        "#address-cells":
> +          const: 1
> +        "#size-cells":
> +          const: 0
> +      required:
> +        - "#address-cells"
> +        - "#size-cells"
> +
> +  # LX2162A SerDes 1 has fewer lanes than the others
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,lx2162a-serdes1
> +    then:
> +      patternProperties:
> +        "^phy@[0-9a-f]+$":
> +          properties:
> +            reg:
> +              enum: [4, 5, 6, 7]
> +    else:
> +      patternProperties:
> +        "^phy@[0-9a-f]+$":
> +          properties:
> +            reg:
> +              enum: [0, 1, 2, 3, 4, 5, 6, 7]
>  
>  additionalProperties: false
>  
> @@ -32,9 +134,52 @@ examples:
>      soc {
>        #address-cells = <2>;
>        #size-cells = <2>;
> -      serdes_1: phy@1ea0000 {
> -        compatible = "fsl,lynx-28g";
> +
> +      serdes_1: serdes@1ea0000 {
> +        compatible = "fsl,lx2160a-serdes1", "fsl,lynx-28g";
>          reg = <0x0 0x1ea0000 0x0 0x1e30>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
>          #phy-cells = <1>;
> +
> +        phy@0 {
> +          reg = <0>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@1 {
> +          reg = <1>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@2 {
> +          reg = <2>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@3 {
> +          reg = <3>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@4 {
> +          reg = <4>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@5 {
> +          reg = <5>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@6 {
> +          reg = <6>;
> +          #phy-cells = <0>;
> +        };
> +
> +        phy@7 {
> +          reg = <7>;
> +          #phy-cells = <0>;
> +        };
>        };
>      };
> -- 
> 2.34.1
>

I should have realized sooner when Rob/Josua requested the changes for
backwards schema compatibility in v2:
https://lore.kernel.org/lkml/20250925080317.2ocgybitliwddhcf@skbuf/
that despite our attempts to preserve compatibility with old kernels,
we actually fail to do that. Actually I should have documented my
earlier thought process better, where I already came to that conclusion,
but which I had forgotten when told that this could work...

The SerDes schema itself is technically backwards-compatible, but the
problem is with consumers, which aren't. In old device trees, they have:

	phys = <&serdes_1 0>;

and in new ones, they have:

	phys = <&serdes_1_lane_a>;

Because the consumer has a single "phys" phandle to the PHY provider, it
either has to point to one, or to the other. But on old kernels, we do
not register PHY providers per lane, so "phys = <&serdes_1_lane_a>"
results in a broken reference.

There are 2 directions to go from here:
1. Have optional per-lane "phy" OF node children, which exist solely for
   tuning electrical parameters. We need to keep the top-level SerDes as
   the only PHY provider, with #phy-cells = <1> denoting the lane.

2. Accept that keeping "fsl,lynx-28g" and overlaid properties has
   absolutely no practical benefit, and drop them (effectively returning
   to Conor's suggestion, as implemented in v2)

3. Extend the schema and the driver support for it as a backportable bug
   fix, to allow registering PHY providers for lanes with OF nodes in
   stable kernels. This avoids regressing when the device trees are
   updated, assuming the stable kernel is also updated.

It's not that I particularly like #2, but going with #1 would imply that
lane OF nodes exist, but the "phys" phandles do not point to them.

Combine that with the fact that anything we do with the 28G Lynx
bindings will have to be replicated, for uniformity's sake, with the
upcoming 10G Lynx SerDes binding (very similar hardware IP), and #1 is
suddenly not looking so pretty at all. I.e. introducing the 10G Lynx
bindings like the 28G Lynx ones would mean deviating from the widely
established norm, and introducing them like the widely established norm
would mean deviating from the 28G Lynx. I can easily see how someone
might look at them one day and think "hmm, can't we make them more
uniform?"

OTOH, the fact that device tree updates require kernel updates (as
implied by #2) is acceptably by everyone in this thread who expressed an
opinion on this topic.

As for option #3, while IMO it would be a justified "new feature as
bug fix", it sounds a bit counterintuitive and I'm afraid I won't manage
to convince all maintainers along the way that this is the way forward.

I'll wait for the merge window to close before reposting anything, but
I'd like an explicit ack from Rob and Josua in the meantime, whose
change request I'd be effectively reverting, to make sure that this
topic is closed.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy