Message-ID: <20250905113757.pmumjdnbd52jxw6o@skbuf>
Date: Fri, 5 Sep 2025 14:37:57 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Josua Mayer <josua@...id-run.com>
Cc: Conor Dooley <conor@...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>,
	Krzysztof Kozlowski <krzk+dt@...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 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.

Powered by blists - more mailing lists