Message-ID: <8bbbf34e-ff9a-481e-a45e-26cde7d8f6bc@solid-run.com>
Date: Fri, 5 Sep 2025 11:10:53 +0000
From: Josua Mayer <josua@...id-run.com>
To: Vladimir Oltean <vladimir.oltean@....com>, Conor Dooley <conor@...nel.org>
CC: "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

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

Powered by blists - more mailing lists