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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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.

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

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