Am 05.09.25 um 17:29 schrieb Vladimir Oltean:

On Fri, Sep 05, 2025 at 02:44:33PM +0000, Josua Mayer wrote:

quoted

quoted

quoted

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)

quoted

    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.

quoted

quoted

quoted

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.

quoted

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.