Message-ID: <5a295e63-f2a2-4aee-9472-389eabfdef41@solid-run.com>
Date: Fri, 5 Sep 2025 15:50:52 +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 17:29 schrieb Vladimir Oltean:
> On Fri, Sep 05, 2025 at 02:44:33PM +0000, Josua Mayer wrote:
>>>> 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>;
>>>     };
>>> };
> Why stop the eq-params reuse at "per lane"? Why not make these global
> nodes, like SFP cages? It's imaginable your pre-emphasis settings might
> be the same across the board, for both SerDes #1 and #2 lanes.
No special reason, I think you got the right idea.
>
> Also, let's take what is upstream now as a starting point. Currently the
> driver has #phy-cells = <1> (i.e. the "phys" phandle is to the SerDes,
> not to individual lanes).
Yes. And all serdes blocks are considered equal.
> Wouldn't we want to keep it that way, and make
> the SerDes lane sub-nodes optional, only in case they have phandles to
> custom pre-emphasis settings? If they don't, use the driver default
> pre-emphasis.
That depends on whether or not you want to use those sub-nodes also
to differentiate between the special characteristics of each serdes block,
and each lane.

Each lane could by itself declare which modes it supports, then the driver
wouldn't need to know about serdes-1/2/3.
Presence (or status) of a lane node can indicate whether the lane is usable.

No strong opinion, either approach can work out.

>
>>>> 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.
> Sorry for maybe being obtuse. You're saying you added code in mainline
> to check for a condition that exists only in downstream lf-6.6.52-2.2.0?
> Why?
I consider it good practice to check the return value of functions, especially
when they can return NULL.

The downstream code merely shows that an original author's assumptions
on how a function is called do not necessarily hold true into the future.

It is not a bug-fix and I didn't intend it to be backported into older releases.
However I actually anticipated a discussion - which started late, here.

Powered by blists - more mailing lists