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

[Josua Mayer <josua@solid-run.com>]

Am 09.09.25 um 13:35 schrieb Vladimir Oltean:
> 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
No, it means they share a programming model and are compatible to a degree.
> , which they are
absolutely (by register map), and partially as implemented in LX2 SoCs.
> 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.
Correct.

As far as solidrun layerscape boards, it is currently fine to require dtb upgrade with kernel.

>
>>> (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?
That is the difference exactly.
> Generally I disagree with requiring board device tree authors to
> maintain the protocol list
>  - it should rather reside in the SoC dtsi,
agree.
> because the driver is able to automatically further restrict to the
> valid set of each board, through lynx_28g_pll_read_configuration().

correct. The Soc dtsi may however spell out the full list in dts.

> So I'm only ever going to consider this if the protocol listing is done
> only once, in the SoC dtsi.
fair.
>> 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"?
The former.
>
> 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.
You are correct, I anticipated putting the equalization parameters
as described by NXP documentation as magic (commented) numbers
in device-tree.

And the tuning process to derive those per docs I had seen involves
testing all combinations against test patterns to find the optimal combination(s).

Very often we can't create an accurate description for the electrical properties
of a board, so it is more natural to specify the compensation parameters.

>
> 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.
As Conor would accept either model, it is your choice.
I do not have any strong objection - the outcomes are workable either way.