Message-ID: <27f64133-729b-448c-96ce-771716485786@solid-run.com>
Date: Fri, 5 Sep 2025 14:44:33 +0000
From: Josua Mayer <josua@...id-run.com>
To: Vladimir Oltean <vladimir.oltean@....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


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.
>>

Powered by blists - more mailing lists