Re: [PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible strings per SerDes and instantiation

[Josua Mayer <josua@solid-run.com>]

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