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:

quoted

On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:

quoted

On Thu, Sep 04, 2025 at 06:44:01PM +0300, Vladimir Oltean wrote:

quoted

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?

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy