Message-ID: <20251110115851.ew72c7b4n6npkxkg@skbuf>
Date: Mon, 10 Nov 2025 13:58:51 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: "Rob Herring (Arm)" <robh@...nel.org>, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>
Cc: devicetree@...r.kernel.org, Josua Mayer <josua@...id-run.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Ioana Ciornei <ioana.ciornei@....com>,
	linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 phy 15/16] dt-bindings: phy: lynx-28g: add compatible
 strings per SerDes and instantiation

On Mon, Nov 10, 2025 at 04:29:27AM -0600, Rob Herring (Arm) wrote:
> 
> On Mon, 10 Nov 2025 11:22:40 +0200, Vladimir Oltean wrote:
> > The 28G Lynx SerDes is instantiated 3 times in the NXP LX2160A SoC and
> > twice in the NXP LX2162A. All these instances share the same register
> > map, but the number of lanes and the protocols supported by each lane
> > differs in a way that isn't detectable by the programming model.
> > 
> > Going by the generic "fsl,lynx-28g" compatible string and expecting all
> > SerDes instantiations to use it was a mistake that needs to be fixed.
> > 
> > The two major options considered are
> > (a) encode the SoC and the SerDes instance in the compatible string,
> >     everything else is the responsibility of the driver to derive based
> >     on this sufficient information
> > (b) add sufficient device tree properties to describe the per-lane
> >     differences, as well as the different lane count
> > 
> > Another important consideration is that any decision made here should
> > be consistent with the decisions taken for the yet-to-be-introduced
> > 10G Lynx SerDes (older generation for older SoCs), because of how
> > similar they are.
> > 
> > I've seen option (b) at play in this unmerged patch set for the 10G Lynx
> > here, and I didn't like it:
> > https://lore.kernel.org/linux-phy/20230413160607.4128315-3-sean.anderson@seco.com/
> > 
> > This is because there, we have a higher degree of variability in the
> > PCCR register values that need to be written per protocol. This makes
> > that approach more drawn-out and more prone to errors, compared to (a)
> > which is more succinct and obviously correct.
> > 
> > So I've chosen option (a) through elimination, and this 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 per-SoC compatible string is prepended to the "fsl,lynx-28g" generic
> > compatible, which is left there for compatibility with old kernels. An
> > exception would be LX2160A SerDes #3, which at the time of writing is
> > not described in fsl-lx2160a.dtsi, and is a non-networking SerDes, so
> > the existing Linux driver is useless for it. So there is no practical
> > reason to put the "fsl,lynx-28g" fallback for "fsl,lx2160a-serdes3".
> > 
> > The specific compatible strings give us the opportunity to express more
> > constraints in the schema that we weren't able to express before:
> > - We allow #phy-cells in the top-level SerDes node only for
> >   compatibility with old kernels that don't know how to translate
> >   "phys = <&serdes_1_lane_a>" to a PHY. We don't need that feature for
> >   the not-yet-introduced LX2160A SerDes #3, so make the presence of
> >   #phy-cells at the top level be dependent on the presence of the
> >   "fsl,lynx-28g" fallback compatible.
> > - The modernization of the compatible string should come together with
> >   per-lane OF nodes.
> > - LX2162A SerDes 1 has fewer lanes than the others, and trying to use
> >   lanes 0-3 would be a mistake that could be caught by the schema.
> > 
> > Cc: Rob Herring <robh@...nel.org>
> > Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>
> > Cc: Conor Dooley <conor+dt@...nel.org>
> > Cc: devicetree@...r.kernel.org
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> > v3->v4:
> > - OF nodes per lane broken out as a separate "[PATCH v4 phy 01/16]
> >   dt-bindings: phy: lynx-28g: permit lane OF PHY providers"
> > - rewritten commit message
> > - s|"^phy@[0-9a-f]+$"|"^phy@[0-7]$"|g in patternProperties
> > - define "#address-cells" and "#size-cells" as part of common
> >   properties, only leave the part which marks them required in the allOf
> >   constraints area
> > 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 | 86 +++++++++++++++++--
> >  1 file changed, 79 insertions(+), 7 deletions(-)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/fsl,lynx-28g.example.dtb: serdes@...0000 (fsl,lx2160a-serdes1): '#phy-cells' does not match any of the regexes: '^phy@[0-7]$', '^pinctrl-[0-9]+$'
> 	from schema $id: http://devicetree.org/schemas/phy/fsl,lynx-28g.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251110092241.1306838-16-vladimir.oltean@nxp.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> 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 after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

I'm terribly sorry about this. There was a testing fault that led to
this mistake remaining uncaught.
Originally I was testing with "make -j $ncpu DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=net/ dt_binding_check":
https://github.com/vladimiroltean/nipa/commit/5579a443f88ec7f8a3e33108acb3d9d529ff825a
(and because this is a patch on Documentation/dt-bindings/phy/ rather than net/,
I missed the warnings).
I had fixed this when submitting the pull request to Jakub (removed DT_SCHEMA_FILES=net/):
https://github.com/linux-netdev/nipa/pull/65/commits/b7d53ebaaeaf1a4e474d6cd85b3b73d557a669a2
but I forgot to pull in the modification back into the test automation framework that I used :-/

Anyway, regarding the warning itself: I had forgotten in the 1+ month
since v3 why I had to do this:
https://lore.kernel.org/linux-phy/20250926180505.760089-13-vladimir.oltean@nxp.com/

 properties:
-  "#phy-cells":
-    const: 1
+
+  "#phy-cells": true // <--- this
...
+allOf:
...
+      properties:
+        "#phy-cells":
+          const: 1
+      required:
+        - "#phy-cells"

so I removed that part, but I'll have to add it back.


@linux-phy maintainers, if this turns out to be the only problem after
review, could you still consider applying patches 1-14, so that I can
make less noise while resending?

Powered by blists - more mailing lists