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 software. So distinguish them by compatible strings.
At the same time, keep "fsl,lynx-28g" as backup.
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>
---
.../devicetree/bindings/phy/fsl,lynx-28g.yaml | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
index ff9f9ca0f19c..55d773c8d0e4 100644
--- a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
+++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
@@ -11,8 +11,17 @@ maintainers:
properties:
compatible:
- enum:
- - fsl,lynx-28g
+ 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
reg:
maxItems: 1
@@ -33,7 +42,7 @@ examples:
#address-cells = <2>;
#size-cells = <2>;
serdes_1: phy@1ea0000 {
- compatible = "fsl,lynx-28g";
+ compatible = "fsl,lx2160a-serdes1", "fsl,lynx-28g";
reg = <0x0 0x1ea0000 0x0 0x1e30>;
#phy-cells = <1>;
};
--
2.34.1On Thu, Sep 04, 2025 at 06:44:01PM +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 software. So distinguish them by compatible strings. > At the same time, keep "fsl,lynx-28g" as backup. > > 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> > --- > .../devicetree/bindings/phy/fsl,lynx-28g.yaml | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > index ff9f9ca0f19c..55d773c8d0e4 100644 > --- a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > @@ -11,8 +11,17 @@ maintainers: > > properties: > compatible: > - enum: > - - fsl,lynx-28g > + oneOf: > + - items: > + - const: fsl,lynx-28g Don't change that part. Previous enum was correct. You want oneOf and enum. > + - items: > + - enum: > + - fsl,lx2160a-serdes1 > + - fsl,lx2160a-serdes2 > + - fsl,lx2160a-serdes3 What are the differences? number of lanes? For this you can take num-lanes property. > + - fsl,lx2162a-serdes1 > + - fsl,lx2162a-serdes2 > + - const: fsl,lynx-28g Best regards, Krzysztof
On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
> > properties:
> > compatible:
> > - enum:
> > - - fsl,lynx-28g
> > + oneOf:
> > + - items:
> > + - const: fsl,lynx-28g
>
> Don't change that part. Previous enum was correct. You want oneOf and
> enum.
Combining the feedback from Conor and Josua, I should only be permitting
the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
or standalone. The description below achieves just that. Does it look ok
to you?
properties:
compatible:
oneOf:
- enum:
- fsl,lx2160a-serdes1
- fsl,lx2160a-serdes2
- fsl,lx2160a-serdes3
- fsl,lx2162a-serdes1
- fsl,lx2162a-serdes2
- const: fsl,lynx-28g
deprecated: true
- items:
- const: fsl,lx2160a-serdes1
- const: fsl,lynx-28g
deprecated: true
- items:
- const: fsl,lx2160a-serdes2
- const: fsl,lynx-28g
deprecated: true
- items:
- const: fsl,lx2162a-serdes1
- const: fsl,lynx-28g
deprecated: true
- items:
- const: fsl,lx2162a-serdes2
- const: fsl,lynx-28g
deprecated: true
On Fri, Sep 05, 2025 at 06:41:50PM +0300, Vladimir Oltean wrote:
> On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
> > > properties:
> > > compatible:
> > > - enum:
> > > - - fsl,lynx-28g
> > > + oneOf:
> > > + - items:
> > > + - const: fsl,lynx-28g
> >
> > Don't change that part. Previous enum was correct. You want oneOf and
> > enum.
>
> Combining the feedback from Conor and Josua, I should only be permitting
> the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
> or standalone. The description below achieves just that. Does it look ok
> to you?
>
> properties:
> compatible:
> oneOf:
> - enum:
> - fsl,lx2160a-serdes1
> - fsl,lx2160a-serdes2
> - fsl,lx2160a-serdes3
> - fsl,lx2162a-serdes1
> - fsl,lx2162a-serdes2
> - const: fsl,lynx-28g
> deprecated: true
> - items:
> - const: fsl,lx2160a-serdes1
> - const: fsl,lynx-28g
> deprecated: true
> - items:
> - const: fsl,lx2160a-serdes2
> - const: fsl,lynx-28g
> deprecated: true
> - items:
> - const: fsl,lx2162a-serdes1
> - const: fsl,lynx-28g
> deprecated: true
> - items:
> - const: fsl,lx2162a-serdes2
> - const: fsl,lynx-28g
> deprecated: true
This doesn't really make sense, none of these are currently in use
right? Everything is just using fsl,lynx-28g right?
Adding new stuff and immediately marking it deprecated is a
contradiction, just don't add it at all if you don't want people using
it. Any users of it would be something you're going to retrofit in now,
so you may as well just retrofit to use what you want people to use
going forward, which has no fallbacks.
I didn't read the back and forth with Josua (sorry!) but is the fallback
even valid? Do those devices have a common minimum set of features that
they share?
On Fri, Sep 05, 2025 at 08:02:59PM +0100, Conor Dooley wrote:
> On Fri, Sep 05, 2025 at 06:41:50PM +0300, Vladimir Oltean wrote:
> > On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
> > > > properties:
> > > > compatible:
> > > > - enum:
> > > > - - fsl,lynx-28g
> > > > + oneOf:
> > > > + - items:
> > > > + - const: fsl,lynx-28g
> > >
> > > Don't change that part. Previous enum was correct. You want oneOf and
> > > enum.
> >
> > Combining the feedback from Conor and Josua, I should only be permitting
> > the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
> > or standalone. The description below achieves just that. Does it look ok
> > to you?
> >
> > properties:
> > compatible:
> > oneOf:
> > - enum:
> > - fsl,lx2160a-serdes1
> > - fsl,lx2160a-serdes2
> > - fsl,lx2160a-serdes3
> > - fsl,lx2162a-serdes1
> > - fsl,lx2162a-serdes2
> > - const: fsl,lynx-28g
> > deprecated: true
>
> > - items:
> > - const: fsl,lx2160a-serdes1
> > - const: fsl,lynx-28g
> > deprecated: true
> > - items:
> > - const: fsl,lx2160a-serdes2
> > - const: fsl,lynx-28g
> > deprecated: true
> > - items:
> > - const: fsl,lx2162a-serdes1
> > - const: fsl,lynx-28g
> > deprecated: true
> > - items:
> > - const: fsl,lx2162a-serdes2
> > - const: fsl,lynx-28g
> > deprecated: true
>
> This doesn't really make sense, none of these are currently in use
> right? Everything is just using fsl,lynx-28g right?
> Adding new stuff and immediately marking it deprecated is a
> contradiction, just don't add it at all if you don't want people using
> it. Any users of it would be something you're going to retrofit in now,
> so you may as well just retrofit to use what you want people to use
> going forward, which has no fallbacks.
You're right that it doesn't make sense to deprecate a newly introduced
compatible string pair - my mistake for misunderstanding "deprecated".
> I didn't read the back and forth with Josua (sorry!) but is the fallback
> even valid? Do those devices have a common minimum set of features that
> they share?
I'll try to make an argument based on the facts presented below.
The current Linux driver, which recognizes only "fsl,lynx-28g", supports
only 1GbE and 10GbE. There are other SerDes protocols supported by the
SerDes, but they are irrelevant for the purpose of discussing
compatibility. Also, LX2160A SerDes #3 is also irrelevant, because it is
not currently described in the device tree.
1GbE compatibility table
SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
LX2160A SerDes #1 y y y y y y y y
LX2160A SerDes #2 y y y y y y y y
LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
LX2162A SerDes #2 y y y y y y y y LX2162A currently uses lx2160a.dtsi
10GbE compatibility table
SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
LX2160A SerDes #1 y y y y y y y y
LX2160A SerDes #2 n n n n n n y y
LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
LX2162A SerDes #2 n n n n n n y y LX2162A currently uses lx2160a.dtsi
As LX2160A SerDes #2 is treated like #1, the device tree is telling the
driver that all lanes support 10GbE (which is false for lanes 0-5).
If one of the SerDes PLLs happens to be provisioned for the 10GbE clock
net frequency, as for example with the RCW[SRDS_PRTCL_S2]=6 setting,
this will make the driver think that it can reconfigure lanes 0-5 as
10GbE.
This will directly affect upper layers (phylink), which will advertise
10GbE modes to its link partner on ports which support only 1GbE, and
the non-functional link mode might be resolved through negotiation, when
a lower speed but functional link could have been established.
You mention a common minimum feature set. That would be supporting 10GbE
only on lanes 6-7, which would be disadvantageous to existing uses of
10GbE on lanes 0-5 of SerDes #1. In some cases, the change might also be
breaking - there might be a PHY attached to these lanes whose firmware
is hardcoded to expect 10GbE, so there won't be a graceful degradation
to 1GbE in all cases.
With Josua's permission, I would consider commit 2f2900176b44 ("arm64:
dts: lx2160a: describe the SerDes block #2") as broken, for being an
incorrect description of hardware - it is presented as identical to
another device, which has a different supported feature set. I will not
try to keep SerDes #2 compatible with "fsl,lynx-28g". This will remain
synonymous only with SerDes #1. The users of the fsl-lx2162a-clearfog.dts
will need updating if the "undetected lack of support for 10GbE" becomes
an issue.
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:
- fsl,lx2160a-serdes2
- fsl,lx2160a-serdes3
- fsl,lx2162a-serdes1
- fsl,lx2162a-serdes2
Also, I will limit the driver support for the "fsl,lynx-28g" compatible
to just 1GbE and 10GbE. The 25GbE feature introduced by this series will
require a more precise compatible string.
Am 08.09.25 um 11:37 schrieb Vladimir Oltean:
> On Fri, Sep 05, 2025 at 08:02:59PM +0100, Conor Dooley wrote:
>> On Fri, Sep 05, 2025 at 06:41:50PM +0300, Vladimir Oltean wrote:
>>> On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
>>>>> properties:
>>>>> compatible:
>>>>> - enum:
>>>>> - - fsl,lynx-28g
>>>>> + oneOf:
>>>>> + - items:
>>>>> + - const: fsl,lynx-28g
>>>> Don't change that part. Previous enum was correct. You want oneOf and
>>>> enum.
>>> Combining the feedback from Conor and Josua, I should only be permitting
>>> the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
>>> or standalone. The description below achieves just that. Does it look ok
>>> to you?
>>>
>>> properties:
>>> compatible:
>>> oneOf:
>>> - enum:
>>> - fsl,lx2160a-serdes1
>>> - fsl,lx2160a-serdes2
>>> - fsl,lx2160a-serdes3
>>> - fsl,lx2162a-serdes1
>>> - fsl,lx2162a-serdes2
>>> - const: fsl,lynx-28g
>>> deprecated: true
>>> - items:
>>> - const: fsl,lx2160a-serdes1
>>> - const: fsl,lynx-28g
>>> deprecated: true
>>> - items:
>>> - const: fsl,lx2160a-serdes2
>>> - const: fsl,lynx-28g
>>> deprecated: true
>>> - items:
>>> - const: fsl,lx2162a-serdes1
>>> - const: fsl,lynx-28g
>>> deprecated: true
>>> - items:
>>> - const: fsl,lx2162a-serdes2
>>> - const: fsl,lynx-28g
>>> deprecated: true
>> This doesn't really make sense, none of these are currently in use
>> right? Everything is just using fsl,lynx-28g right?
>> Adding new stuff and immediately marking it deprecated is a
>> contradiction, just don't add it at all if you don't want people using
>> it. Any users of it would be something you're going to retrofit in now,
>> so you may as well just retrofit to use what you want people to use
>> going forward, which has no fallbacks.
> You're right that it doesn't make sense to deprecate a newly introduced
> compatible string pair - my mistake for misunderstanding "deprecated".
>
>> I didn't read the back and forth with Josua (sorry!) but is the fallback
>> even valid? Do those devices have a common minimum set of features that
>> they share?
> I'll try to make an argument based on the facts presented below.
>
> The current Linux driver, which recognizes only "fsl,lynx-28g", supports
> only 1GbE and 10GbE. There are other SerDes protocols supported by the
> SerDes, but they are irrelevant for the purpose of discussing
> compatibility. Also, LX2160A SerDes #3 is also irrelevant, because it is
> not currently described in the device tree.
>
> 1GbE compatibility table
>
> SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
> LX2160A SerDes #1 y y y y y y y y
> LX2160A SerDes #2 y y y y y y y y
> LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
> LX2162A SerDes #2 y y y y y y y y LX2162A currently uses lx2160a.dtsi
>
> 10GbE compatibility table
>
> SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
> LX2160A SerDes #1 y y y y y y y y
> LX2160A SerDes #2 n n n n n n y y
> LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
> LX2162A SerDes #2 n n n n n n y y LX2162A currently uses lx2160a.dtsi
>
> As LX2160A SerDes #2 is treated like #1, the device tree is telling the
> driver that all lanes support 10GbE (which is false for lanes 0-5).
>
> If one of the SerDes PLLs happens to be provisioned for the 10GbE clock
> net frequency, as for example with the RCW[SRDS_PRTCL_S2]=6 setting,
> this will make the driver think that it can reconfigure lanes 0-5 as
> 10GbE.
>
> This will directly affect upper layers (phylink), which will advertise
> 10GbE modes to its link partner on ports which support only 1GbE, and
> the non-functional link mode might be resolved through negotiation, when
> a lower speed but functional link could have been established.
>
> You mention a common minimum feature set. That would be supporting 10GbE
> only on lanes 6-7, which would be disadvantageous to existing uses of
> 10GbE on lanes 0-5 of SerDes #1. In some cases, the change might also be
> breaking - there might be a PHY attached to these lanes whose firmware
> is hardcoded to expect 10GbE, so there won't be a graceful degradation
> to 1GbE in all cases.
>
> With Josua's permission, I would consider commit 2f2900176b44 ("arm64:
> dts: lx2160a: describe the SerDes block #2") as broken, for being an
> incorrect description of hardware - it is presented as identical to
> another device, which has a different supported feature set. I will not
> try to keep SerDes #2 compatible with "fsl,lynx-28g". This will remain
> synonymous only with SerDes #1. The users of the fsl-lx2162a-clearfog.dts
> will need updating if the "undetected lack of support for 10GbE" becomes
> an issue.
>
> 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:
> - 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.
The driver can still probe on fsl,lynx-28g alone for backwards compatibility,
and you can limit the feature-set as you see fit in such case.
Main argument for always specifying lynx-28g is that the serdes blocks
do share a common programming model and register definitions.
>
> Also, I will limit the driver support for the "fsl,lynx-28g" compatible
> to just 1GbE and 10GbE. The 25GbE feature introduced by this series will
> require a more precise compatible string.
Okay
On Mon, Sep 08, 2025 at 02:02:35PM +0000, Josua Mayer wrote:
>
> Am 08.09.25 um 11:37 schrieb Vladimir Oltean:
> > On Fri, Sep 05, 2025 at 08:02:59PM +0100, Conor Dooley wrote:
> >> On Fri, Sep 05, 2025 at 06:41:50PM +0300, Vladimir Oltean wrote:
> >>> On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote:
> >>>>> properties:
> >>>>> compatible:
> >>>>> - enum:
> >>>>> - - fsl,lynx-28g
> >>>>> + oneOf:
> >>>>> + - items:
> >>>>> + - const: fsl,lynx-28g
> >>>> Don't change that part. Previous enum was correct. You want oneOf and
> >>>> enum.
> >>> Combining the feedback from Conor and Josua, I should only be permitting
> >>> the use of "fsl,lynx-28g" as a fallback to "fsl,lx216{0,2}a-serdes{1,2}",
> >>> or standalone. The description below achieves just that. Does it look ok
> >>> to you?
> >>>
> >>> properties:
> >>> compatible:
> >>> oneOf:
> >>> - enum:
> >>> - fsl,lx2160a-serdes1
> >>> - fsl,lx2160a-serdes2
> >>> - fsl,lx2160a-serdes3
> >>> - fsl,lx2162a-serdes1
> >>> - fsl,lx2162a-serdes2
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >>> - items:
> >>> - const: fsl,lx2160a-serdes1
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >>> - items:
> >>> - const: fsl,lx2160a-serdes2
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >>> - items:
> >>> - const: fsl,lx2162a-serdes1
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >>> - items:
> >>> - const: fsl,lx2162a-serdes2
> >>> - const: fsl,lynx-28g
> >>> deprecated: true
> >> This doesn't really make sense, none of these are currently in use
> >> right? Everything is just using fsl,lynx-28g right?
> >> Adding new stuff and immediately marking it deprecated is a
> >> contradiction, just don't add it at all if you don't want people using
> >> it. Any users of it would be something you're going to retrofit in now,
> >> so you may as well just retrofit to use what you want people to use
> >> going forward, which has no fallbacks.
> > You're right that it doesn't make sense to deprecate a newly introduced
> > compatible string pair - my mistake for misunderstanding "deprecated".
> >
> >> I didn't read the back and forth with Josua (sorry!) but is the fallback
> >> even valid? Do those devices have a common minimum set of features that
> >> they share?
> > I'll try to make an argument based on the facts presented below.
> >
> > The current Linux driver, which recognizes only "fsl,lynx-28g", supports
> > only 1GbE and 10GbE. There are other SerDes protocols supported by the
> > SerDes, but they are irrelevant for the purpose of discussing
> > compatibility. Also, LX2160A SerDes #3 is also irrelevant, because it is
> > not currently described in the device tree.
> >
> > 1GbE compatibility table
> >
> > SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
> > LX2160A SerDes #1 y y y y y y y y
> > LX2160A SerDes #2 y y y y y y y y
> > LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
> > LX2162A SerDes #2 y y y y y y y y LX2162A currently uses lx2160a.dtsi
> >
> > 10GbE compatibility table
> >
> > SerDes Lane 0 Lane 1 Lane 2 Lane 3 Lane 4 Lane 5 Lane 6 Lane 7 Comments
> > LX2160A SerDes #1 y y y y y y y y
> > LX2160A SerDes #2 n n n n n n y y
> > LX2162A SerDes #1 n/a n/a n/a n/a y y y y LX2162A currently uses lx2160a.dtsi
> > LX2162A SerDes #2 n n n n n n y y LX2162A currently uses lx2160a.dtsi
> >
> > As LX2160A SerDes #2 is treated like #1, the device tree is telling the
> > driver that all lanes support 10GbE (which is false for lanes 0-5).
> >
> > If one of the SerDes PLLs happens to be provisioned for the 10GbE clock
> > net frequency, as for example with the RCW[SRDS_PRTCL_S2]=6 setting,
> > this will make the driver think that it can reconfigure lanes 0-5 as
> > 10GbE.
> >
> > This will directly affect upper layers (phylink), which will advertise
> > 10GbE modes to its link partner on ports which support only 1GbE, and
> > the non-functional link mode might be resolved through negotiation, when
> > a lower speed but functional link could have been established.
> >
> > You mention a common minimum feature set. That would be supporting 10GbE
> > only on lanes 6-7, which would be disadvantageous to existing uses of
> > 10GbE on lanes 0-5 of SerDes #1. In some cases, the change might also be
> > breaking - there might be a PHY attached to these lanes whose firmware
> > is hardcoded to expect 10GbE, so there won't be a graceful degradation
> > to 1GbE in all cases.
> >
> > With Josua's permission, I would consider commit 2f2900176b44 ("arm64:
> > dts: lx2160a: describe the SerDes block #2") as broken, for being an
> > incorrect description of hardware - it is presented as identical to
> > another device, which has a different supported feature set. I will not
> > try to keep SerDes #2 compatible with "fsl,lynx-28g". This will remain
> > synonymous only with SerDes #1. The users of the fsl-lx2162a-clearfog.dts
> > will need updating if the "undetected lack of support for 10GbE" becomes
> > an issue.
> >
> > 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:
> > - 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.
>
> The driver can still probe on fsl,lynx-28g alone for backwards compatibility,
> and you can limit the feature-set as you see fit in such case.
>
> Main argument for always specifying lynx-28g is that the serdes blocks
> do share a common programming model and register definitions.
FWIW, I'd accept both of what's been proposed here with the
justifications being provided. Up to you guys that understand the
hardware to decide what's more suitable.
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:
> > - 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.
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:
- 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.
I realize I've CCed you late on the patches. They are here:
https://lore.kernel.org/lkml/20250904154402.300032-1-vladimir.oltean@nxp.com/
One of Conor's objections was that keeping "fsl,lynx-28g" as a fallback
compatible string may not make sense, and indeed I tried to highlight in
my previous reply that it can lead to incorrect behaviour if SerDes #2
is described in this way.
Further trying to argue that SerDes #2 should have "fsl,lynx-28g" as a
fallback without directly addressing the fact it results in incorrect
behaviour is... strange.
Also, SerDes #3 is not described at all, it's not necessary to introduce
a fallback when it can be described precisely from the start.
> The driver can still probe on fsl,lynx-28g alone for backwards compatibility,
> and you can limit the feature-set as you see fit in such case.
>
> Main argument for always specifying lynx-28g is that the serdes blocks
> do share a common programming model and register definitions.
I think this is the sticking point. The blocks do share a common
programming model, but that model does not give us a way to identify the
supported protocols. You can try to enable a protocol converter that
doesn't exist, and read back the enablement status, and you'll find the
hardware reports it to be enabled (for example PCCC[SXGMIIA_CFG]).
The snippet below is something you can try out and see for yourself (it
will need adaptation depending on kernel revision).
static void lynx_28g_lane_probe_supported(struct lynx_28g_lane *lane)
{
enum lynx_lane_mode lane_mode;
unsigned long supported = 0;
int err;
for (lane_mode = LANE_MODE_UNKNOWN + 1; lane_mode < LANE_MODE_MAX; lane_mode++) {
u32 orig_val, val;
err = lynx_pccr_read(lane, lane_mode, &orig_val);
if (err)
continue;
val = orig_val;
switch (lane_mode) {
case LANE_MODE_1000BASEKX:
val |= PCC8_SGMIIa_KX;
fallthrough;
case LANE_MODE_1000BASEX_SGMII:
val |= PCC8_SGMIIa_CFG;
break;
case LANE_MODE_10GBASER:
case LANE_MODE_10GBASEKR:
val |= PCCC_SXGMIIn_XFI;
fallthrough;
case LANE_MODE_USXGMII:
val |= PCCC_SXGMIIn_CFG;
break;
case LANE_MODE_25GBASER:
case LANE_MODE_25GBASEKR:
val |= PCCD_E25Gn_CFG;
break;
case LANE_MODE_40GBASER_XLAUI:
case LANE_MODE_40GBASEKR4:
val |= PCCE_E40Gn_CFG;
break;
default:
break;
}
err = lynx_pccr_write(lane, lane_mode, val);
if (err)
continue;
err = lynx_pccr_read(lane, lane_mode, &val);
if (err)
continue;
dev_info(&lane->phy->dev, "Protocol %d: PCCR was 0x%x, is 0x%x\n",
lane_mode, orig_val, val);
switch (lane_mode) {
case LANE_MODE_1000BASEKX:
if (val & PCC8_SGMIIa_KX)
supported |= BIT(lane_mode);
fallthrough;
case LANE_MODE_1000BASEX_SGMII:
if (val & PCC8_SGMIIa_CFG)
supported |= BIT(lane_mode);
break;
case LANE_MODE_10GBASER:
case LANE_MODE_10GBASEKR:
if (val & PCCC_SXGMIIn_XFI)
supported |= BIT(lane_mode);
fallthrough;
case LANE_MODE_USXGMII:
if (val & PCCC_SXGMIIn_CFG)
supported |= BIT(lane_mode);
break;
case LANE_MODE_25GBASER:
case LANE_MODE_25GBASEKR:
if (val & PCCD_E25Gn_CFG)
supported |= BIT(lane_mode);
break;
case LANE_MODE_40GBASER_XLAUI:
case LANE_MODE_40GBASEKR4:
if (val & PCCE_E40Gn_CFG)
supported |= BIT(lane_mode);
break;
default:
break;
}
WARN_ON(lynx_pccr_write(lane, lane_mode, orig_val));
}
dev_info(&lane->phy->dev, "Lane supported modes: 0x%lx\n", supported);
}
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. (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.
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
>>> - 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.
// 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.
>
> I realize I've CCed you late on the patches. They are here:
> https://lore.kernel.org/lkml/20250904154402.300032-1-vladimir.oltean@nxp.com/
>
> One of Conor's objections was that keeping "fsl,lynx-28g" as a fallback
> compatible string may not make sense, and indeed I tried to highlight in
> my previous reply that it can lead to incorrect behaviour if SerDes #2
> is described in this way.
>
> Further trying to argue that SerDes #2 should have "fsl,lynx-28g" as a
> fallback without directly addressing the fact it results in incorrect
> behaviour is... strange.
>
> Also, SerDes #3 is not described at all, it's not necessary to introduce
> a fallback when it can be described precisely from the start.
>
>> The driver can still probe on fsl,lynx-28g alone for backwards compatibility,
>> and you can limit the feature-set as you see fit in such case.
>>
>> Main argument for always specifying lynx-28g is that the serdes blocks
>> do share a common programming model and register definitions.
> I think this is the sticking point. The blocks do share a common
> programming model, but that model does not give us a way to identify the
> supported protocols. You can try to enable a protocol converter that
> doesn't exist, and read back the enablement status, and you'll find the
> hardware reports it to be enabled (for example PCCC[SXGMIIA_CFG]).
Indeed.
>
> The snippet below is something you can try out and see for yourself (it
> will need adaptation depending on kernel revision).
>
> static void lynx_28g_lane_probe_supported(struct lynx_28g_lane *lane)
> {
> enum lynx_lane_mode lane_mode;
> unsigned long supported = 0;
> int err;
>
> for (lane_mode = LANE_MODE_UNKNOWN + 1; lane_mode < LANE_MODE_MAX; lane_mode++) {
> u32 orig_val, val;
>
> err = lynx_pccr_read(lane, lane_mode, &orig_val);
> if (err)
> continue;
>
> val = orig_val;
>
> switch (lane_mode) {
> case LANE_MODE_1000BASEKX:
> val |= PCC8_SGMIIa_KX;
> fallthrough;
> case LANE_MODE_1000BASEX_SGMII:
> val |= PCC8_SGMIIa_CFG;
> break;
> case LANE_MODE_10GBASER:
> case LANE_MODE_10GBASEKR:
> val |= PCCC_SXGMIIn_XFI;
> fallthrough;
> case LANE_MODE_USXGMII:
> val |= PCCC_SXGMIIn_CFG;
> break;
> case LANE_MODE_25GBASER:
> case LANE_MODE_25GBASEKR:
> val |= PCCD_E25Gn_CFG;
> break;
> case LANE_MODE_40GBASER_XLAUI:
> case LANE_MODE_40GBASEKR4:
> val |= PCCE_E40Gn_CFG;
> break;
> default:
> break;
> }
>
> err = lynx_pccr_write(lane, lane_mode, val);
> if (err)
> continue;
>
> err = lynx_pccr_read(lane, lane_mode, &val);
> if (err)
> continue;
>
> dev_info(&lane->phy->dev, "Protocol %d: PCCR was 0x%x, is 0x%x\n",
> lane_mode, orig_val, val);
>
> switch (lane_mode) {
> case LANE_MODE_1000BASEKX:
> if (val & PCC8_SGMIIa_KX)
> supported |= BIT(lane_mode);
> fallthrough;
> case LANE_MODE_1000BASEX_SGMII:
> if (val & PCC8_SGMIIa_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_10GBASER:
> case LANE_MODE_10GBASEKR:
> if (val & PCCC_SXGMIIn_XFI)
> supported |= BIT(lane_mode);
> fallthrough;
> case LANE_MODE_USXGMII:
> if (val & PCCC_SXGMIIn_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_25GBASER:
> case LANE_MODE_25GBASEKR:
> if (val & PCCD_E25Gn_CFG)
> supported |= BIT(lane_mode);
> break;
> case LANE_MODE_40GBASER_XLAUI:
> case LANE_MODE_40GBASEKR4:
> if (val & PCCE_E40Gn_CFG)
> supported |= BIT(lane_mode);
> break;
> default:
> break;
> }
>
> WARN_ON(lynx_pccr_write(lane, lane_mode, orig_val));
> }
>
> dev_info(&lane->phy->dev, "Lane supported modes: 0x%lx\n", supported);
> }
I did play with those in u-boot trying to derive dt dpmac node status from
protocol converter registers ... ... ...
> 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.
You can however use this argument to drop driver support for the
lone fsl,lynx-28g compatible string.
> (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.
Presence indicates whether or not a lane is available,
while each node can specify the modes it supports.
Obviously this allows the device-tree author to describe invalid configurations.
But it avoids describing each combination in the driver.
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.
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.
Hi Josua, On Tue, Sep 09, 2025 at 02:35:43PM +0300, Vladimir Oltean wrote: > 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. ... > 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. Do you have any further feedback? I would like to keep the ball rolling and submit a new version. One thing I could do is I could require #phy-cells to be 0 when using one of the new compatibles like "fsl,lx2160a-serdes1", and 1 when "fsl,lynx-28g" is found.
On Tue, Sep 09, 2025 at 02:35:43PM +0300, Vladimir Oltean wrote: > 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". I think Josua was pointing out their own omission here. > > >>> - fsl,lx2160a-serdes2 > > >>> - fsl,lx2160a-serdes3 > > >>> - fsl,lx2162a-serdes1 > > >>> - fsl,lx2162a-serdes2
On Tue, Sep 09, 2025 at 07:35:43PM +0100, Conor Dooley wrote: > On Tue, Sep 09, 2025 at 02:35:43PM +0300, Vladimir Oltean wrote: > > 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". > > I think Josua was pointing out their own omission here. Maybe, but the quoted text was my proposal.
On Fri, Sep 05, 2025 at 10:29:33AM +0200, Krzysztof Kozlowski wrote: > > diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > > index ff9f9ca0f19c..55d773c8d0e4 100644 > > --- a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > > +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > > @@ -11,8 +11,17 @@ maintainers: > > > > properties: > > compatible: > > - enum: > > - - fsl,lynx-28g > > + oneOf: > > + - items: > > + - const: fsl,lynx-28g > > Don't change that part. Previous enum was correct. You want oneOf and > enum. > > > + - items: > > + - enum: > > + - fsl,lx2160a-serdes1 > > + - fsl,lx2160a-serdes2 > > + - fsl,lx2160a-serdes3 > > What are the differences? number of lanes? For this you can take > num-lanes property. Number of lanes, but on LX2162A it's a bit weird. It supports 4 lanes but they are numbered 4, 5, 6, 7. Also supported protocols for each lane. Just one example: lane 0 of fsl,lx2160a-serdes1 supports PCIe, SGMII, USXGMII, 40GbE, whereas lane 0 of fsl,lx2160a-serdes2 supports PCIe, SGMII.
On Thu, Sep 04, 2025 at 06:44:01PM +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 software. So distinguish them by compatible strings.
> At the same time, keep "fsl,lynx-28g" as backup.
Why keep the backup? Doesn't sound like you can use it for anything,
unless there's some minimum set of capabilities that all devices
support. If that's not the case, should it not just be marked deprecated
or removed entirely?
>
> 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>
> ---
> .../devicetree/bindings/phy/fsl,lynx-28g.yaml | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> index ff9f9ca0f19c..55d773c8d0e4 100644
> --- a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml
> @@ -11,8 +11,17 @@ maintainers:
>
> properties:
> compatible:
> - enum:
> - - fsl,lynx-28g
> + 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
>
> reg:
> maxItems: 1
> @@ -33,7 +42,7 @@ examples:
> #address-cells = <2>;
> #size-cells = <2>;
> serdes_1: phy@1ea0000 {
> - compatible = "fsl,lynx-28g";
> + compatible = "fsl,lx2160a-serdes1", "fsl,lynx-28g";
> reg = <0x0 0x1ea0000 0x0 0x1e30>;
> #phy-cells = <1>;
> };
> --
> 2.34.1
>
On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
> On Thu, Sep 04, 2025 at 06:44:01PM +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 software. So distinguish them by compatible strings.
> > At the same time, keep "fsl,lynx-28g" as backup.
>
> Why keep the backup? Doesn't sound like you can use it for anything,
> unless there's some minimum set of capabilities that all devices
> support. If that's not the case, should it not just be marked deprecated
> or removed entirely?
To be honest, I could use some guidance on the best way to handle this.
When I had written this patch downstream, lx2160a.dtsi only had serdes_1
defined, as "fsl,lynx-28g", and this patch made more sense. Keep
"fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
device trees still work with old kernels (as is sometimes needed during
'git bisect', etc), for some definition of the word "work" (more often
than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
consumer driver if the PHY driver doesn't exist, but the 'phys' property
exists in the device tree).
Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
the SerDes block #2") came and defined the second SerDes also with
"fsl,lynx-28g".
The second SerDes is less capable than the first one, so the same
developer then started battling with the fact that the driver doesn't
know that serdes_2 doesn't support some protocols, and wrote some
patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
calling lynx_28g_pll_get"), which in all likelihood could have been
avoided using a specific compatible string. The lynx_info ::
lane_supports_mode() method from patch 14/14 is supposed to say what is
supported per SerDes and what not.
In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
compatible string mean, compared to removing it entirely? Would there be
any remaining driver support for it? Should I compute the common set of
capabilities between SerDes #1 and #2, and only support that? What
impact would this have upon old device trees? Is it acceptable to just
remove support for them?
On Fri, Sep 05, 2025 at 01:49:21PM +0300, Vladimir Oltean wrote:
> On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
> > On Thu, Sep 04, 2025 at 06:44:01PM +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 software. So distinguish them by compatible strings.
> > > At the same time, keep "fsl,lynx-28g" as backup.
> >
> > Why keep the backup? Doesn't sound like you can use it for anything,
> > unless there's some minimum set of capabilities that all devices
> > support. If that's not the case, should it not just be marked deprecated
> > or removed entirely?
>
> To be honest, I could use some guidance on the best way to handle this.
>
> When I had written this patch downstream, lx2160a.dtsi only had serdes_1
> defined, as "fsl,lynx-28g", and this patch made more sense. Keep
> "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
> device trees still work with old kernels (as is sometimes needed during
> 'git bisect', etc), for some definition of the word "work" (more often
> than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
> consumer driver if the PHY driver doesn't exist, but the 'phys' property
> exists in the device tree).
>
> Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
> the SerDes block #2") came and defined the second SerDes also with
> "fsl,lynx-28g".
>
> The second SerDes is less capable than the first one, so the same
> developer then started battling with the fact that the driver doesn't
> know that serdes_2 doesn't support some protocols, and wrote some
> patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
> calling lynx_28g_pll_get"), which in all likelihood could have been
> avoided using a specific compatible string. The lynx_info ::
> lane_supports_mode() method from patch 14/14 is supposed to say what is
> supported per SerDes and what not.
> In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
> compatible string mean, compared to removing it entirely? Would there be
> any remaining driver support for it?
Really it does nothing much. The difference is that removing it entirely
from the binding will cause existing dts users to create warnings
whereas marking it deprecated is more of an attempt to stop
proliferation since it doesn't generate any warnings at the moment but
people using the binding will see that it's not ideal. I personally use
deprecated when using the old binding is only ill-advised because
there's missing features etc and I opt for removal when the old binding
is wrong and actively harmful. In both cases, I'd keep the old
compatible in the driver for compatibility reasons.
> Should I compute the common set of
> capabilities between SerDes #1 and #2, and only support that? What
> impact would this have upon old device trees? Is it acceptable to just
> remove support for them?
Up to you really, if there is a common set between the two, that's
probably the ideal thing to do for the generic compatible. If there
isn't, and shit just ain't working properly at all for either then yeah,
it might be for the better to remove support for it entirely from the
driver too. Just make sure that you're clear about the fact that it just
cannot work at all, and that's why you're axing it. Breaking
compatibility is allowed, when there's justification for doing so, it's
not a complete no-no.
Hi Vladimir,
Thanks for adding me in CC!
Am 05.09.25 um 12:49 schrieb Vladimir Oltean:
> On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
>> On Thu, Sep 04, 2025 at 06:44:01PM +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 software.
If the serdes node had a phy sub-node for each lane, then software
could describe each lane individually / omit 4 lanes on lx2162 sd1 e.g..
This comes with added benefit that perhaps in the future we can use
them to describe board-specific equalization parameters.
The current driver uses hardcoded defaults that may be appropriate
for some nxp evaluation boards only.
>>> So distinguish them by compatible strings.
>>> At the same time, keep "fsl,lynx-28g" as backup.
>> Why keep the backup? Doesn't sound like you can use it for anything,
>> unless there's some minimum set of capabilities that all devices
>> support. If that's not the case, should it not just be marked deprecated
>> or removed entirely?
> To be honest, I could use some guidance on the best way to handle this.
>
> When I had written this patch downstream, lx2160a.dtsi only had serdes_1
> defined, as "fsl,lynx-28g", and this patch made more sense. Keep
> "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
> device trees still work with old kernels (as is sometimes needed during
> 'git bisect', etc), for some definition of the word "work" (more often
> than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
> consumer driver if the PHY driver doesn't exist, but the 'phys' property
> exists in the device tree).
>
> Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
> the SerDes block #2") came and defined the second SerDes also with
> "fsl,lynx-28g".
>
> The second SerDes is less capable than the first one, so the same
> developer then started battling with the fact that the driver doesn't
> know that serdes_2 doesn't support some protocols, and wrote some
> patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
> calling lynx_28g_pll_get"), which in all likelihood could have been
> avoided using a specific compatible string.
> The lynx_info ::
In upstream my patch fixes nothing, it added a return value check
for a function call that can indeed return NULL.
My battle was a different one, unrelated to varying serdes block features
(I claim it can also happen with same phy on serdes block 1):
I found that the combination of Marvell 10G phy driver, pcs, serdes and dpaa2
did not strictly adhere to phy-connection-type set in device-tree, or the initial
mode negoitated between phy and mac.
Once phy negotiated a 2.5Gbps, the kernel would then try switching
all 3 drivers to a 2.5g mode, when it should have stuck with 10gbase-r,
or reported an error knowing that the serdes did not advertise support for 2.5g.
Due to massive downstream refactoring in the vendor kernel serdes driver,
there existed a code-path leading to null pointer dereference.
But that was also a consequence of other mistakes.
> lane_supports_mode() method from patch 14/14 is supposed to say what is
> supported per SerDes and what not.
Indeed, and upstream properly gates all reconfiguration attempts using it.
>
> In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
> compatible string mean, compared to removing it entirely? Would there be
> any remaining driver support for it?Should I compute the common set of
> capabilities between SerDes #1 and #2, and only support that? What
> impact would this have upon old device trees? Is it acceptable to just
> remove support for them?
When you remove the old compatible string, the driver should still keep
supporting old DTBs.
I personally believe it correct to keep dual compatible strings,
reflecting that the serdes blocks share a common programming model.
sincerely
Josua Mayer
On Fri, Sep 05, 2025 at 11:10:53AM +0000, Josua Mayer wrote:
> Hi Vladimir,
>
> Thanks for adding me in CC!
>
> Am 05.09.25 um 12:49 schrieb Vladimir Oltean:
> > On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
> >> On Thu, Sep 04, 2025 at 06:44:01PM +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 software.
> If the serdes node had a phy sub-node for each lane, then software
> could describe each lane individually / omit 4 lanes on lx2162 sd1 e.g..
>
> This comes with added benefit that perhaps in the future we can use
> them to describe board-specific equalization parameters.
>
> The current driver uses hardcoded defaults that may be appropriate
> for some nxp evaluation boards only.
Yeah.
Given the fact that the SerDes can change between protocols, I suspect
you want to go one level further, and describe default equalization
parameters for each protocol. The equalization for 10G won't be good for
25G and vice versa.
Do you have a specific format in mind?
> >>> So distinguish them by compatible strings.
> >>> At the same time, keep "fsl,lynx-28g" as backup.
> >> Why keep the backup? Doesn't sound like you can use it for anything,
> >> unless there's some minimum set of capabilities that all devices
> >> support. If that's not the case, should it not just be marked deprecated
> >> or removed entirely?
> > To be honest, I could use some guidance on the best way to handle this.
> >
> > When I had written this patch downstream, lx2160a.dtsi only had serdes_1
> > defined, as "fsl,lynx-28g", and this patch made more sense. Keep
> > "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
> > device trees still work with old kernels (as is sometimes needed during
> > 'git bisect', etc), for some definition of the word "work" (more often
> > than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
> > consumer driver if the PHY driver doesn't exist, but the 'phys' property
> > exists in the device tree).
> >
> > Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
> > the SerDes block #2") came and defined the second SerDes also with
> > "fsl,lynx-28g".
> >
> > The second SerDes is less capable than the first one, so the same
> > developer then started battling with the fact that the driver doesn't
> > know that serdes_2 doesn't support some protocols, and wrote some
> > patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
> > calling lynx_28g_pll_get"), which in all likelihood could have been
> > avoided using a specific compatible string.
> > The lynx_info ::
> In upstream my patch fixes nothing, it added a return value check
> for a function call that can indeed return NULL.
>
> My battle was a different one, unrelated to varying serdes block features
> (I claim it can also happen with same phy on serdes block 1):
>
> I found that the combination of Marvell 10G phy driver, pcs, serdes and dpaa2
> did not strictly adhere to phy-connection-type set in device-tree, or the initial
> mode negoitated between phy and mac.
Yes, it doesn't have to.
> Once phy negotiated a 2.5Gbps, the kernel would then try switching
> all 3 drivers to a 2.5g mode, when it should have stuck with 10gbase-r,
> or reported an error knowing that the serdes did not advertise support for 2.5g.
>
> Due to massive downstream refactoring in the vendor kernel serdes driver,
> there existed a code-path leading to null pointer dereference.
> But that was also a consequence of other mistakes.
Sorry, I interpreted your patch in the only way that could have made any
sense.
In the circumstance you describe, isn't your fix just "code after return"?
How would have lynx_28g_set_mode(PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_2500BASEX)
gotten past the lynx_28g_supports_interface() test without being rejected?
The driver would have needed to suffer some pretty serious modifications
to allow this to happen, and I'm not happy with the fact that it's changed
to handle incorrect downstream changes, without at least a complete
description.
> > lane_supports_mode() method from patch 14/14 is supposed to say what is
> > supported per SerDes and what not.
> Indeed, and upstream properly gates all reconfiguration attempts using it.
> >
> > In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
> > compatible string mean, compared to removing it entirely? Would there be
> > any remaining driver support for it?Should I compute the common set of
> > capabilities between SerDes #1 and #2, and only support that? What
> > impact would this have upon old device trees? Is it acceptable to just
> > remove support for them?
> When you remove the old compatible string, the driver should still keep
> supporting old DTBs.
Thus thinking SerDes #2 supports all features of SerDes #1?
> I personally believe it correct to keep dual compatible strings,
> reflecting that the serdes blocks share a common programming model.
Thanks for the input.
Am 05.09.25 um 13:37 schrieb Vladimir Oltean:
> On Fri, Sep 05, 2025 at 11:10:53AM +0000, Josua Mayer wrote:
>> Hi Vladimir,
>>
>> Thanks for adding me in CC!
>>
>> Am 05.09.25 um 12:49 schrieb Vladimir Oltean:
>>> On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
>>>> On Thu, Sep 04, 2025 at 06:44:01PM +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 software.
>> If the serdes node had a phy sub-node for each lane, then software
>> could describe each lane individually / omit 4 lanes on lx2162 sd1 e.g..
>>
>> This comes with added benefit that perhaps in the future we can use
>> them to describe board-specific equalization parameters.
>>
>> The current driver uses hardcoded defaults that may be appropriate
>> for some nxp evaluation boards only.
> Yeah.
>
> Given the fact that the SerDes can change between protocols, I suspect
> you want to go one level further, and describe default equalization
> parameters for each protocol. The equalization for 10G won't be good for
> 25G and vice versa.
>
> Do you have a specific format in mind?
I have a prototype implementation based on v5.15 using properties as below
(I am not sure this is the best format though, DT maintainers may have opinions):
serdes_1_lane_g: phy@6 {
reg = <6>;
#phy-cells = <0>;
fsl,eq-names = "10gbase-r", "25gbase-r";
fsl,eq-type = "3-tap", "3-tap";
fsl,eq-preq-sign = "positive", "positive";
fsl,eq-preq-rate = "1.33", "1.33";
fsl,eq-post1q-sign = "negative", "negative";
fsl,eq-post1q-rate = "1.26", "1.26";
fsl,eq-amp-red = "1.000", "1.000";
fsl,eq-adaptive = <32>, <32>;
};
I imagine a parameters sub-node per protocol may be more readable.
The best description would be generic enough to cover pci and sata, too.
Perhaps:
serdes_1_lane_g: phy@6 {
reg = <6>;
#phy-cells = <0>;
fsl,eq-params = <&serdes_1_lane_g_eq_10g>, <&serdes_1_lane_g_eq_sata>;
serdes_1_lane_g_eq_10g: eq-params-0 {
/* compare downstream enum lynx_28g_lane_mode */
fsl,lane-protocol = "xfi";
fsl,eq-type = "3-tap";
fsl,eq-preq-sign = "positive";
fsl,eq-preq-rate = "1.33";
fsl,eq-post1q-sign = "negative";
fsl,eq-post1q-rate = "1.26";
fsl,eq-amp-red = "1.000";
fsl,eq-adaptive = <32>;
};
serdes_1_lane_g_eq_sata: eq-1 {
/* compare downstream enum lynx_28g_lane_mode */
/* example parameters, do not use for sata */
fsl,lane-mode = "pci";
fsl,eq-type = "3-tap";
fsl,eq-preq-sign = "positive";
fsl,eq-preq-rate = "1.33";
fsl,eq-post1q-sign = "negative";
fsl,eq-post1q-rate = "1.26";
fsl,eq-amp-red = "1.000";
fsl,eq-adaptive = <32>;
};
};
>
>>>>> So distinguish them by compatible strings.
>>>>> At the same time, keep "fsl,lynx-28g" as backup.
>>>> Why keep the backup? Doesn't sound like you can use it for anything,
>>>> unless there's some minimum set of capabilities that all devices
>>>> support. If that's not the case, should it not just be marked deprecated
>>>> or removed entirely?
>>> To be honest, I could use some guidance on the best way to handle this.
>>>
>>> When I had written this patch downstream, lx2160a.dtsi only had serdes_1
>>> defined, as "fsl,lynx-28g", and this patch made more sense. Keep
>>> "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
>>> device trees still work with old kernels (as is sometimes needed during
>>> 'git bisect', etc), for some definition of the word "work" (more often
>>> than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
>>> consumer driver if the PHY driver doesn't exist, but the 'phys' property
>>> exists in the device tree).
>>>
>>> Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
>>> the SerDes block #2") came and defined the second SerDes also with
>>> "fsl,lynx-28g".
>>>
>>> The second SerDes is less capable than the first one, so the same
>>> developer then started battling with the fact that the driver doesn't
>>> know that serdes_2 doesn't support some protocols, and wrote some
>>> patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
>>> calling lynx_28g_pll_get"), which in all likelihood could have been
>>> avoided using a specific compatible string.
>>> The lynx_info ::
>> In upstream my patch fixes nothing, it added a return value check
>> for a function call that can indeed return NULL.
>>
>> My battle was a different one, unrelated to varying serdes block features
>> (I claim it can also happen with same phy on serdes block 1):
>>
>> I found that the combination of Marvell 10G phy driver, pcs, serdes and dpaa2
>> did not strictly adhere to phy-connection-type set in device-tree, or the initial
>> mode negoitated between phy and mac.
> Yes, it doesn't have to.
>
>> Once phy negotiated a 2.5Gbps, the kernel would then try switching
>> all 3 drivers to a 2.5g mode, when it should have stuck with 10gbase-r,
>> or reported an error knowing that the serdes did not advertise support for 2.5g.
>>
>> Due to massive downstream refactoring in the vendor kernel serdes driver,
>> there existed a code-path leading to null pointer dereference.
>> But that was also a consequence of other mistakes.
> Sorry, I interpreted your patch in the only way that could have made any
> sense.
>
> In the circumstance you describe, isn't your fix just "code after return"?
> How would have lynx_28g_set_mode(PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_2500BASEX)
> gotten past the lynx_28g_supports_interface() test without being rejected?
v6.6.6.52-2.2.0 release, .set_mode:
lynx_28g_set_mode->lynx_28g_set_link_mode->lynx_28g_set_lane_mode->lynx_28g_pll_get
does not check lynx_28g_supports_interface.
> The driver would have needed to suffer some pretty serious modifications
> to allow this to happen, and I'm not happy with the fact that it's changed
> to handle incorrect downstream changes, without at least a complete
> description.
Point of my submitted patch was merely to guard an unchecked pointer,
generating appropriate error with enough explanation for non-maintainers.
I debated using BUG_ON instead of warn.
>
>>> lane_supports_mode() method from patch 14/14 is supposed to say what is
>>> supported per SerDes and what not.
>> Indeed, and upstream properly gates all reconfiguration attempts using it.
>>> In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
>>> compatible string mean, compared to removing it entirely? Would there be
>>> any remaining driver support for it?Should I compute the common set of
>>> capabilities between SerDes #1 and #2, and only support that? What
>>> impact would this have upon old device trees? Is it acceptable to just
>>> remove support for them?
>> When you remove the old compatible string, the driver should still keep
>> supporting old DTBs.
> Thus thinking SerDes #2 supports all features of SerDes #1?
Yes.
This will not be a problem for those users who configure all lanes to SGMII,
or just the two special lanes for XFI.
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.
>
>> I personally believe it correct to keep dual compatible strings,
>> reflecting that the serdes blocks share a common programming model.
> Thanks for the input.
>
Am 05.09.25 um 16:23 schrieb Josua Mayer:
> Am 05.09.25 um 13:37 schrieb Vladimir Oltean:
>> On Fri, Sep 05, 2025 at 11:10:53AM +0000, Josua Mayer wrote:
>>> Hi Vladimir,
>>>
>>> Thanks for adding me in CC!
>>>
>>> Am 05.09.25 um 12:49 schrieb Vladimir Oltean:
>>>> On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
>>>>> On Thu, Sep 04, 2025 at 06:44:01PM +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 software.
>>> If the serdes node had a phy sub-node for each lane, then software
>>> could describe each lane individually / omit 4 lanes on lx2162 sd1 e.g..
>>>
>>> This comes with added benefit that perhaps in the future we can use
>>> them to describe board-specific equalization parameters.
>>>
>>> The current driver uses hardcoded defaults that may be appropriate
>>> for some nxp evaluation boards only.
>> Yeah.
>>
>> Given the fact that the SerDes can change between protocols, I suspect
>> you want to go one level further, and describe default equalization
>> parameters for each protocol. The equalization for 10G won't be good for
>> 25G and vice versa.
>>
>> Do you have a specific format in mind?
> I have a prototype implementation based on v5.15 using properties as below
> (I am not sure this is the best format though, DT maintainers may have opinions):
>
> serdes_1_lane_g: phy@6 {
> reg = <6>;
> #phy-cells = <0>;
> fsl,eq-names = "10gbase-r", "25gbase-r";
> fsl,eq-type = "3-tap", "3-tap";
> fsl,eq-preq-sign = "positive", "positive";
> fsl,eq-preq-rate = "1.33", "1.33";
> fsl,eq-post1q-sign = "negative", "negative";
> fsl,eq-post1q-rate = "1.26", "1.26";
> fsl,eq-amp-red = "1.000", "1.000";
> fsl,eq-adaptive = <32>, <32>;
> };
>
> I imagine a parameters sub-node per protocol may be more readable.
>
> The best description would be generic enough to cover pci and sata, too.
>
> Perhaps:
>
> serdes_1_lane_g: phy@6 {
> reg = <6>;
> #phy-cells = <0>;
> fsl,eq-params = <&serdes_1_lane_g_eq_10g>, <&serdes_1_lane_g_eq_sata>;
fsl,lane-modes = "xfi", "sata";
^^ Would be mroe elegant, as it can at the same time explain which modes
a specific lane supports generally.
Then eq-params is an optional list with specific parameters, some of
which can be shared between different modes (40g/10g)
> serdes_1_lane_g_eq_10g: eq-params-0 {
> /* compare downstream enum lynx_28g_lane_mode */
> fsl,lane-protocol = "xfi";
> fsl,eq-type = "3-tap";
> fsl,eq-preq-sign = "positive";
> fsl,eq-preq-rate = "1.33";
> fsl,eq-post1q-sign = "negative";
> fsl,eq-post1q-rate = "1.26";
> fsl,eq-amp-red = "1.000";
> fsl,eq-adaptive = <32>;
> };
>
> serdes_1_lane_g_eq_sata: eq-1 {
> /* compare downstream enum lynx_28g_lane_mode */
> /* example parameters, do not use for sata */
> fsl,lane-mode = "pci";
> fsl,eq-type = "3-tap";
> fsl,eq-preq-sign = "positive";
> fsl,eq-preq-rate = "1.33";
> fsl,eq-post1q-sign = "negative";
> fsl,eq-post1q-rate = "1.26";
> fsl,eq-amp-red = "1.000";
> fsl,eq-adaptive = <32>;
> };
> };
>
>>>>>> So distinguish them by compatible strings.
>>>>>> At the same time, keep "fsl,lynx-28g" as backup.
>>>>> Why keep the backup? Doesn't sound like you can use it for anything,
>>>>> unless there's some minimum set of capabilities that all devices
>>>>> support. If that's not the case, should it not just be marked deprecated
>>>>> or removed entirely?
>>>> To be honest, I could use some guidance on the best way to handle this.
>>>>
>>>> When I had written this patch downstream, lx2160a.dtsi only had serdes_1
>>>> defined, as "fsl,lynx-28g", and this patch made more sense. Keep
>>>> "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
>>>> device trees still work with old kernels (as is sometimes needed during
>>>> 'git bisect', etc), for some definition of the word "work" (more often
>>>> than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
>>>> consumer driver if the PHY driver doesn't exist, but the 'phys' property
>>>> exists in the device tree).
>>>>
>>>> Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
>>>> the SerDes block #2") came and defined the second SerDes also with
>>>> "fsl,lynx-28g".
>>>>
>>>> The second SerDes is less capable than the first one, so the same
>>>> developer then started battling with the fact that the driver doesn't
>>>> know that serdes_2 doesn't support some protocols, and wrote some
>>>> patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
>>>> calling lynx_28g_pll_get"), which in all likelihood could have been
>>>> avoided using a specific compatible string.
>>>> The lynx_info ::
>>> In upstream my patch fixes nothing, it added a return value check
>>> for a function call that can indeed return NULL.
>>>
>>> My battle was a different one, unrelated to varying serdes block features
>>> (I claim it can also happen with same phy on serdes block 1):
>>>
>>> I found that the combination of Marvell 10G phy driver, pcs, serdes and dpaa2
>>> did not strictly adhere to phy-connection-type set in device-tree, or the initial
>>> mode negoitated between phy and mac.
>> Yes, it doesn't have to.
>>
>>> Once phy negotiated a 2.5Gbps, the kernel would then try switching
>>> all 3 drivers to a 2.5g mode, when it should have stuck with 10gbase-r,
>>> or reported an error knowing that the serdes did not advertise support for 2.5g.
>>>
>>> Due to massive downstream refactoring in the vendor kernel serdes driver,
>>> there existed a code-path leading to null pointer dereference.
>>> But that was also a consequence of other mistakes.
>> Sorry, I interpreted your patch in the only way that could have made any
>> sense.
>>
>> In the circumstance you describe, isn't your fix just "code after return"?
>> How would have lynx_28g_set_mode(PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_2500BASEX)
>> gotten past the lynx_28g_supports_interface() test without being rejected?
> v6.6.6.52-2.2.0 release, .set_mode:
>
> lynx_28g_set_mode->lynx_28g_set_link_mode->lynx_28g_set_lane_mode->lynx_28g_pll_get
>
> does not check lynx_28g_supports_interface.
>
>> The driver would have needed to suffer some pretty serious modifications
>> to allow this to happen, and I'm not happy with the fact that it's changed
>> to handle incorrect downstream changes, without at least a complete
>> description.
> Point of my submitted patch was merely to guard an unchecked pointer,
> generating appropriate error with enough explanation for non-maintainers.
>
> I debated using BUG_ON instead of warn.
>
>>>> lane_supports_mode() method from patch 14/14 is supposed to say what is
>>>> supported per SerDes and what not.
>>> Indeed, and upstream properly gates all reconfiguration attempts using it.
>>>> In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
>>>> compatible string mean, compared to removing it entirely? Would there be
>>>> any remaining driver support for it?Should I compute the common set of
>>>> capabilities between SerDes #1 and #2, and only support that? What
>>>> impact would this have upon old device trees? Is it acceptable to just
>>>> remove support for them?
>>> When you remove the old compatible string, the driver should still keep
>>> supporting old DTBs.
>> Thus thinking SerDes #2 supports all features of SerDes #1?
> Yes.
> This will not be a problem for those users who configure all lanes to SGMII,
> or just the two special lanes for XFI.
>
> 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.
>
>>> I personally believe it correct to keep dual compatible strings,
>>> reflecting that the serdes blocks share a common programming model.
>> Thanks for the input.
>>
On Fri, Sep 05, 2025 at 02:44:33PM +0000, Josua Mayer wrote:
> >> Do you have a specific format in mind?
> > I have a prototype implementation based on v5.15 using properties as below
> > (I am not sure this is the best format though, DT maintainers may have opinions):
> >
> > serdes_1_lane_g: phy@6 {
> > reg = <6>;
> > #phy-cells = <0>;
> > fsl,eq-names = "10gbase-r", "25gbase-r";
> > fsl,eq-type = "3-tap", "3-tap";
> > fsl,eq-preq-sign = "positive", "positive";
> > fsl,eq-preq-rate = "1.33", "1.33";
> > fsl,eq-post1q-sign = "negative", "negative";
> > fsl,eq-post1q-rate = "1.26", "1.26";
> > fsl,eq-amp-red = "1.000", "1.000";
> > fsl,eq-adaptive = <32>, <32>;
> > };
> >
> > I imagine a parameters sub-node per protocol may be more readable.
> >
> > The best description would be generic enough to cover pci and sata, too.
> >
> > Perhaps:
> >
> > serdes_1_lane_g: phy@6 {
> > reg = <6>;
> > #phy-cells = <0>;
> > fsl,eq-params = <&serdes_1_lane_g_eq_10g>, <&serdes_1_lane_g_eq_sata>;
>
> fsl,lane-modes = "xfi", "sata";
>
> ^^ Would be mroe elegant, as it can at the same time explain which modes
> a specific lane supports generally.
>
> Then eq-params is an optional list with specific parameters, some of
> which can be shared between different modes (40g/10g)
>
> > serdes_1_lane_g_eq_10g: eq-params-0 {
> > /* compare downstream enum lynx_28g_lane_mode */
> > fsl,lane-protocol = "xfi";
> > fsl,eq-type = "3-tap";
> > fsl,eq-preq-sign = "positive";
> > fsl,eq-preq-rate = "1.33";
> > fsl,eq-post1q-sign = "negative";
> > fsl,eq-post1q-rate = "1.26";
> > fsl,eq-amp-red = "1.000";
> > fsl,eq-adaptive = <32>;
> > };
> >
> > serdes_1_lane_g_eq_sata: eq-1 {
> > /* compare downstream enum lynx_28g_lane_mode */
> > /* example parameters, do not use for sata */
> > fsl,lane-mode = "pci";
> > fsl,eq-type = "3-tap";
> > fsl,eq-preq-sign = "positive";
> > fsl,eq-preq-rate = "1.33";
> > fsl,eq-post1q-sign = "negative";
> > fsl,eq-post1q-rate = "1.26";
> > fsl,eq-amp-red = "1.000";
> > fsl,eq-adaptive = <32>;
> > };
> > };
Why stop the eq-params reuse at "per lane"? Why not make these global
nodes, like SFP cages? It's imaginable your pre-emphasis settings might
be the same across the board, for both SerDes #1 and #2 lanes.
Also, let's take what is upstream now as a starting point. Currently the
driver has #phy-cells = <1> (i.e. the "phys" phandle is to the SerDes,
not to individual lanes). Wouldn't we want to keep it that way, and make
the SerDes lane sub-nodes optional, only in case they have phandles to
custom pre-emphasis settings? If they don't, use the driver default
pre-emphasis.
> >> In the circumstance you describe, isn't your fix just "code after return"?
> >> How would have lynx_28g_set_mode(PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_2500BASEX)
> >> gotten past the lynx_28g_supports_interface() test without being rejected?
> > v6.6.6.52-2.2.0 release, .set_mode:
> >
> > lynx_28g_set_mode->lynx_28g_set_link_mode->lynx_28g_set_lane_mode->lynx_28g_pll_get
> >
> > does not check lynx_28g_supports_interface.
> >
> >> The driver would have needed to suffer some pretty serious modifications
> >> to allow this to happen, and I'm not happy with the fact that it's changed
> >> to handle incorrect downstream changes, without at least a complete
> >> description.
> > Point of my submitted patch was merely to guard an unchecked pointer,
> > generating appropriate error with enough explanation for non-maintainers.
> >
> > I debated using BUG_ON instead of warn.
Sorry for maybe being obtuse. You're saying you added code in mainline
to check for a condition that exists only in downstream lf-6.6.52-2.2.0?
Why?
Am 05.09.25 um 17:29 schrieb Vladimir Oltean:
> On Fri, Sep 05, 2025 at 02:44:33PM +0000, Josua Mayer wrote:
>>>> Do you have a specific format in mind?
>>> I have a prototype implementation based on v5.15 using properties as below
>>> (I am not sure this is the best format though, DT maintainers may have opinions):
>>>
>>> serdes_1_lane_g: phy@6 {
>>> reg = <6>;
>>> #phy-cells = <0>;
>>> fsl,eq-names = "10gbase-r", "25gbase-r";
>>> fsl,eq-type = "3-tap", "3-tap";
>>> fsl,eq-preq-sign = "positive", "positive";
>>> fsl,eq-preq-rate = "1.33", "1.33";
>>> fsl,eq-post1q-sign = "negative", "negative";
>>> fsl,eq-post1q-rate = "1.26", "1.26";
>>> fsl,eq-amp-red = "1.000", "1.000";
>>> fsl,eq-adaptive = <32>, <32>;
>>> };
>>>
>>> I imagine a parameters sub-node per protocol may be more readable.
>>>
>>> The best description would be generic enough to cover pci and sata, too.
>>>
>>> Perhaps:
>>>
>>> serdes_1_lane_g: phy@6 {
>>> reg = <6>;
>>> #phy-cells = <0>;
>>> fsl,eq-params = <&serdes_1_lane_g_eq_10g>, <&serdes_1_lane_g_eq_sata>;
>> fsl,lane-modes = "xfi", "sata";
>>
>> ^^ Would be mroe elegant, as it can at the same time explain which modes
>> a specific lane supports generally.
>>
>> Then eq-params is an optional list with specific parameters, some of
>> which can be shared between different modes (40g/10g)
>>
>>> serdes_1_lane_g_eq_10g: eq-params-0 {
>>> /* compare downstream enum lynx_28g_lane_mode */
>>> fsl,lane-protocol = "xfi";
>>> fsl,eq-type = "3-tap";
>>> fsl,eq-preq-sign = "positive";
>>> fsl,eq-preq-rate = "1.33";
>>> fsl,eq-post1q-sign = "negative";
>>> fsl,eq-post1q-rate = "1.26";
>>> fsl,eq-amp-red = "1.000";
>>> fsl,eq-adaptive = <32>;
>>> };
>>>
>>> serdes_1_lane_g_eq_sata: eq-1 {
>>> /* compare downstream enum lynx_28g_lane_mode */
>>> /* example parameters, do not use for sata */
>>> fsl,lane-mode = "pci";
>>> fsl,eq-type = "3-tap";
>>> fsl,eq-preq-sign = "positive";
>>> fsl,eq-preq-rate = "1.33";
>>> fsl,eq-post1q-sign = "negative";
>>> fsl,eq-post1q-rate = "1.26";
>>> fsl,eq-amp-red = "1.000";
>>> fsl,eq-adaptive = <32>;
>>> };
>>> };
> Why stop the eq-params reuse at "per lane"? Why not make these global
> nodes, like SFP cages? It's imaginable your pre-emphasis settings might
> be the same across the board, for both SerDes #1 and #2 lanes.
No special reason, I think you got the right idea.
>
> Also, let's take what is upstream now as a starting point. Currently the
> driver has #phy-cells = <1> (i.e. the "phys" phandle is to the SerDes,
> not to individual lanes).
Yes. And all serdes blocks are considered equal.
> Wouldn't we want to keep it that way, and make
> the SerDes lane sub-nodes optional, only in case they have phandles to
> custom pre-emphasis settings? If they don't, use the driver default
> pre-emphasis.
That depends on whether or not you want to use those sub-nodes also
to differentiate between the special characteristics of each serdes block,
and each lane.
Each lane could by itself declare which modes it supports, then the driver
wouldn't need to know about serdes-1/2/3.
Presence (or status) of a lane node can indicate whether the lane is usable.
No strong opinion, either approach can work out.
>
>>>> In the circumstance you describe, isn't your fix just "code after return"?
>>>> How would have lynx_28g_set_mode(PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_2500BASEX)
>>>> gotten past the lynx_28g_supports_interface() test without being rejected?
>>> v6.6.6.52-2.2.0 release, .set_mode:
>>>
>>> lynx_28g_set_mode->lynx_28g_set_link_mode->lynx_28g_set_lane_mode->lynx_28g_pll_get
>>>
>>> does not check lynx_28g_supports_interface.
>>>
>>>> The driver would have needed to suffer some pretty serious modifications
>>>> to allow this to happen, and I'm not happy with the fact that it's changed
>>>> to handle incorrect downstream changes, without at least a complete
>>>> description.
>>> Point of my submitted patch was merely to guard an unchecked pointer,
>>> generating appropriate error with enough explanation for non-maintainers.
>>>
>>> I debated using BUG_ON instead of warn.
> Sorry for maybe being obtuse. You're saying you added code in mainline
> to check for a condition that exists only in downstream lf-6.6.52-2.2.0?
> Why?
I consider it good practice to check the return value of functions, especially
when they can return NULL.
The downstream code merely shows that an original author's assumptions
on how a function is called do not necessarily hold true into the future.
It is not a bug-fix and I didn't intend it to be backported into older releases.
However I actually anticipated a discussion - which started late, here.
On Fri, Sep 05, 2025 at 03:50:52PM +0000, Josua Mayer wrote: > > Why stop the eq-params reuse at "per lane"? Why not make these global > > nodes, like SFP cages? It's imaginable your pre-emphasis settings might > > be the same across the board, for both SerDes #1 and #2 lanes. > No special reason, I think you got the right idea. Well, there is a reason why. You wouldn't put "fsl,blabla" vendor device tree properties in a root device tree node, where it isn't namespaced by the "fsl,lynx-28g" device. You'd only put them under the root node if they were vendor-agnostic.
© 2016 - 2025 Red Hat, Inc.