Message-ID: <20250908-turbulent-unhappy-378144402000@spud>
Date: Mon, 8 Sep 2025 19:39:26 +0100
From: Conor Dooley <conor@...nel.org>
To: Josua Mayer <josua@...id-run.com>
Cc: Vladimir Oltean <vladimir.oltean@....com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible
 strings per SerDes and instantiation

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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists