[PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible strings per SerDes and instantiation
Vladimir Oltean
vladimir.oltean at nxp.com
Tue Sep 9 04:35:43 PDT 2025
On Mon, Sep 08, 2025 at 04:04:45PM +0000, Josua Mayer wrote:
> Am 08.09.25 um 17:37 schrieb Vladimir Oltean:
> > On Mon, Sep 08, 2025 at 02:02:35PM +0000, Josua Mayer wrote:
> >>> My updated plan is to describe the schema rules for the compatible as
> >>> follows. Is that ok with everyone?
> >>>
> >>> properties:
> >>> compatible:
> >>> oneOf:
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >>> - items:
> >>> - const: fsl,lx2160a-serdes1
> >>> - const: fsl,lynx-28g
> >>> - enum:
> missed fsl,lx2160a-serdes1
I didn't miss anything. "fsl,lx2160a-serdes1" is deliberately not in
"enum" because there's no point specifying this compatible as
standalone, if for backwards compatibility reasons it will always have
to be "fsl,lx2160a-serdes1", "fsl,lynx-28g" (it was described as
"fsl,lynx-28g" before), which is already covered by "items".
> >>> - fsl,lx2160a-serdes2
> >>> - fsl,lx2160a-serdes3
> >>> - fsl,lx2162a-serdes1
> >>> - fsl,lx2162a-serdes2
> >> Weak objection, I think this is more complex than it should be.
> >> Perhaps it was discussed before to keep two compatible strings ...:
> >>
> >> properties:
> >> compatible:
> >> items:
> >> - enum:
> >> - fsl,lx2160a-serdes2
> >> - fsl,lx2160a-serdes3
> >> - fsl,lx2162a-serdes1
> >> - fsl,lx2162a-serdes2
> >> - const: fsl,lynx-28g
> >>
> >> This will cause the dtbs_check to complain about anyone in the future
> >> using it wrong.
> My proposal requires two compatible strings always, or the schema will fail
> to validate. Examples:
>
> compatible = "fsl,lynx-28g";
> // fails validation but driver can keep supporting it for backwards compatibility
>
> compatible = "fsl,lx2160a-serdes1", "fsl,lynx-28g";
> // valid per my proposal, functional with existing driver and future changes.
No, not valid per your proposal, there is no "fsl,lx2160a-serdes1" in it.
But verbally I got the point. What you propose is only different from
the patch that I submitted in that you're saying let's drop schema
support for the lone "fsl,lynx-28g".
Our proposals are fundamentally different in this aspect: to you,
"fsl,lynx-28g" means the general register layout and programming model.
To me, it specifically means LX2160A SerDes #1. We have to agree which
one is it.
So, generally I do agree that either one of:
- compatible = "fsl,lx2160a-serdes1" or
- compatible = "fsl,lynx-28g" + explicit protocol listing for each lane
are sufficient hardware descriptions, but as a matter of practicality I
prefer to keep only obviously correct information in the device tree,
which is only sufficient for the expert level details to go in the
driver, where they are much easier to fix if wrong.
The current "fsl,lynx-28g" definition and use does _not_ have explicit
protocol listing per lane, so unless it is interpreted as meaning a
synonym for one particular SerDes instance, it becomes even more
meaningless with current device trees.
> // this is how you will know it is SD #1
>
> compatible = "fsl,lx2160a-serdes2", "fsl,lynx-28g";
> // valid per my proposal, and driver can use it in the future to identify SD #2
>
> The kernel looks in compatible strings for the *first match*.
>
> > So just that we stay on track, this is what the submitted patch
> > originally proposed:
> >
> > properties:
> > compatible:
> > oneOf:
> > - items:
> > - const: fsl,lynx-28g
> > - items:
> > - enum:
> > - fsl,lx2160a-serdes1
> > - fsl,lx2160a-serdes2
> > - fsl,lx2160a-serdes3
> > - fsl,lx2162a-serdes1
> > - fsl,lx2162a-serdes2
> > - const: fsl,lynx-28g
> >
> > Your proposal is different in the following ways:
> - always require 2 compatible strings specified in combination,.
> validation fails when fsl,lynx-28g string specified alone.
> > - Just compatible = "fsl,lynx-28g" will produce a schema validation error, BUT
> >
> > - There is no compatible = "fsl,lx2160a-serdes1". I don't understand how
> > you propose to describe that SerDes.
> copy-paste failure, I intended to list them all, including sd1.
ok.
> > The fact that SerDes #2 works on the fsl-lx2162a-clearfog.dts is
> > accidental and doesn't change the fact that describing it as
> > "fsl,lynx-28g" is wrong.
>
> It works because only the SGMII modes are used on that board.
Yes, which qualifies as "accidental", considering that the SoC dtsi
describes two non-identical blocks as identical.
> You can however use this argument to drop driver support for the
> lone fsl,lynx-28g compatible string.
I'm not going to do that. There is a big difference in the two uses,
which is that "fsl,lynx-28g" is problematic for SerDes #2 but isn't so
for SerDes #1.
Accepting "fsl,lx216*a-serdes2", "fsl,lynx-28g" in the schema would mean
the two are compatible, which they are not.
Keeping driver support for the lone "fsl,lynx-28g" (treating it as
SerDes #1) would mean newer kernels would continue to work on
non-updated fsl-lx2162a-clearfog.dts, but updating the device tree would
require updating the kernel. I think you implicitly gave an ack for that
here:
https://lore.kernel.org/lkml/02270f62-9334-400c-b7b9-7e6a44dbbfc9@solid-run.com/
|I don't think you need to fix a downstream dtb via the driver.
|If downstream user can update the kernel, they can update the dtb also,
|to resolve their own bugs.
> > (of course, I stand corrected if someone finds
> > a way to determine that 10GbE is unsupported on some lanes based on just
> > the programming model, but I doubt it.)
> >
> > The only 3 ways to find the list of supported protocols, that are known
> > to me to work, are:
> > #1: list them all in the device tree (talking about tens, and the list
> > is ever-expanding as the driver gets more development). This is by
> > far the most complex and difficult to maintain solution and my least
> > preferred.
> > #2: hardcode them in the driver, based on SerDes compatible string (the
> > current patch, or variations). This is my preferred variant for
> > keeping the dt-bindings simple and the
> > #3: like #2, but distinguish between two "fsl,lynx-28g" instances based
> > on the "reg" value. This should work fine, as every SerDes block
> > index is instatiated at a fixed physical address in every SoC (block
> > #1: 0x1ea0000, #2: 0x1eb0000, #3: 0x1ec0000). It should directly
> > address your objection, but:
> > - it also requires dt-bindings maintainers buy-in.
> > - this method can distinguish features of SerDes i from j, but not
> > from SoC A vs B. There is an upcoming Lynx 10G driver where we
> > need the per-SoC capabilities as well, and it would be good to
> > have the same overall driver and dt-binding structure for both.
> #4: by listing descriptive phy sub-nodes under the serdes blocks root node.
So in which way is #4 fundamentally different than #1, other than
slightly different organization of information?
Generally I disagree with requiring board device tree authors to
maintain the protocol list - it should rather reside in the SoC dtsi,
because the driver is able to automatically further restrict to the
valid set of each board, through lynx_28g_pll_read_configuration().
So I'm only ever going to consider this if the protocol listing is done
only once, in the SoC dtsi.
> Presence indicates whether or not a lane is available,
> while each node can specify the modes it supports.
You mean "lane is available" as in "LX2162A SerDes #1 only has lanes 4-7
available as an SoC parameterisation option", or as in "this board only
uses SerDes lanes A and B"?
If the former, then yes, maybe. If the latter - I don't think this is
compatible with my idea of describing SerDes lanes in the SoC dtsi.
> Obviously this allows the device-tree author to describe invalid configurations.
> But it avoids describing each combination in the driver.
I see "describing each combination in the driver" as literally the
smallest of problems. It is just a little bit of extra code and a few
tables, located a few tens of lines above the code that implements those
same features in the same driver.
Compare that to listing protocols in the device tree, which may be
distributed through entirely different channels than the kernel, so it
involves an ABI contract to obey.
It's really a matter of humbly admitting that I don't know what I don't
know - I would very much want to avoid the logistical nightmare of
having to update device trees again when new things are discovered.
I've seen your proposal map out on the Lynx 10G SerDes, and it would
look like this:
https://lore.kernel.org/linux-phy/20230413160607.4128315-3-sean.anderson@seco.com/
You can hardly consider it "KISS" when the device tree specifies what
value should be written to what PCCR register for what protocol.
What you seem to want (customize electrical parameters per lane) doesn't
seem to need what you're asking for (listing supported protocols in the
device tree per lane).
This is mostly because we're _not_ going to add "fsl,eq-amp-red",
"fsl,eq-adaptive" etc as you seem to imply here:
https://lore.kernel.org/lkml/02270f62-9334-400c-b7b9-7e6a44dbbfc9@solid-run.com/
- i.e. shoving device tree values into hardware registers. We will
instead define a vendor-agnostic method of specifying equalizer
attenuations in decibels for each tap, and somehow translate that in the
driver into LNaTECR0/ LNaTECR1 register values. See for instance how
Documentation/devicetree/bindings/phy/transmit-amplitude.yaml defines
"tx-p2p-microvolt", and which is a starting point for programming the
AMP_RED field. That property already has "tx-p2p-microvolt-names" per
SerDes protocol (or so it should be - no idea what's with "xgmii"), so
there should be no reason to link this with custom "fsl,lane-protocol"
values or whatever.
It's completely fair to say that currently I have no idea how to
translate standard measurement units into register values, and I'd have
to ask the hardware validation team for formulas. But if you were to
imagine yourself as a user rather than as a developer, I think it's
pretty clear which option you would choose.
Anyway, I don't see why the above can be future extensions, and aren't
my main preoccupation right now. For now we just need to sort out the
lane capability detection per SerDes.
More information about the linux-phy
mailing list