Message-ID:
 <AM0PR06MB10396E5D3A14C7B32BFAEB264F7DEA@AM0PR06MB10396.eurprd06.prod.outlook.com>
Date: Wed, 26 Nov 2025 10:45:31 +0000
From: Holger Brunck <holger.brunck@...achienergy.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Andrew Lunn <andrew@...n.ch>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, Daniel Golle <daniel@...rotopia.org>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>, Heiner Kallweit
	<hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
	<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I
	<kishon@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Eric
 Woudstra <ericwouds@...il.com>,
	Marek Beh√∫n <kabel@...nel.org>, Lee Jones
	<lee@...nel.org>, Patrice Chotard <patrice.chotard@...s.st.com>, Maxime
 Chevallier <maxime.chevallier@...tlin.com>
Subject: RE: [PATCH net-next 1/9] dt-bindings: phy: rename
 transmit-amplitude.yaml to phy-common-props.yaml



> On Wed, Nov 26, 2025 at 09:32:30AM +0000, Holger Brunck wrote:
> > Hi Vladimir,
> >
> > > On Tue, Nov 25, 2025 at 11:33:09PM +0100, Andrew Lunn wrote:
> > > > > Yeah, although as things currently stand, I'd say that is the
> > > > > lesser of problems. The only user (mv88e6xxx) does something
> > > > > strange: it says it wants to configure the TX amplitude of
> > > > > SerDes ports, but instead follows the phy-handle and applies the
> > > > > amplitude specified in that
> > > node.
> > > > >
> > > > > I tried to mentally follow how things would work in 2 cases:
> > > > > 1. PHY referenced by phy-handle is internal, then by definition it's not
> > > > >    a SerDes port.
> > > > > 2. PHY referenced by phy-handle is external, then the mv88e6xxx driver
> > > > >    looks at what is essentially a device tree description of the PHY's
> > > > >    TX, and applies that as a mirror image to the local SerDes' TX.
> > > > >
> > > > > I think the logic is used in mv88e6xxx through case #2, i.e. we
> > > > > externalize the mv88e6xxx SerDes electrical properties to an
> > > > > unrelated OF node, the connected Ethernet PHY.
> > > >
> > > > My understanding of the code is the same, #2. Although i would
> > > > probably not say it is an unrelated node. I expect the PHY is on
> > > > the other end of the SERDES link which is having the TX amplitudes set.
> > > > This clearly will not work if there is an SFP cage on the other
> > > > end, but it does for an SGMII PHY.
> > >
> > > It is unrelated in the sense that the SGMII PHY is a different
> > > kernel object, and the mv88e6xxx is polluting its OF node with
> > > properties which it then interprets as its own, when the PHY driver
> > > may have wanted to configure its SGMII TX amplitude too, via those same
> generic properties.
> > >
> > > > I guess this code is from before the time Russell converted the
> > > > mv88e6xxx SERDES code into PCS drivers. The register being set is
> > > > within the PCS register set.  The mv88e6xxx also does not make use
> > > > of generic phys to represent the SERDES part of the PCS. So there
> > > > is no phys phandle to follow since there is no phy.
> > >
> > > In my view, the phy-common-props.yaml are supposed to be applicable
> > > to
> > > either:
> > > (1) a network PHY with SerDes host-side connection (I suppose the media
> > >     side electrical properties would be covered by Maxime's phy_port
> > >     work - Maxime, please confirm).
> > > (2) a phylink_pcs with SerDes registers within the same register set
> > > (3) a generic PHY
> > >
> > > My patch 8/9 (net: phy: air_en8811h: deprecate "airoha,pnswap-rx"
> > > and
> > > "airoha,pnswap-tx") is an example of case (1) for polarities. Also,
> > > for example, at least Aquantia Gen3 PHYs (AQR111, AQR112) have a
> > > (not very well
> > > documented) "SerDes Lane 0 Amplitude" field in the PHY XS Receive
> > > (XAUI TX) Reserved Vendor Provisioning 4 register (address 4.E413).
> > >
> > > My patch 7/9 (net: pcs: xpcs: allow lane polarity inversion) is an
> > > example of case (2).
> > >
> > > I haven't submitted an example of case (3) yet, but the Lynx PCS and
> > > Lynx SerDes would fall into that category. The PCS would be free of
> > > describing electrical properties, and those would go to the generic PHY
> (SerDes).
> > >
> > > All I'm trying to say is that we're missing an OF node to describe
> > > mv88e6xxx PCS electrical properties, because otherwise, it collides
> > > with case (1). My note regarding "phys" was just a guess that the "phy-
> handle"
> > > may have been mistaken for the port's SerDes PHY. Although there is
> > > a chance Holger knew what he was doing. In any case, I think we need
> > > to sort this one way or another, leaving the phy-handle logic a discouraged
> fallback path.
> > >
> >
> > I was checking our use case, and it is a bit special. We have the port
> > in question directly connected to a FPGA which has also have a SerDes
> > interface. We are then configuring a fixed link to the FPGA without a
> > phy in between so there is also no phy handle in our case. But in
> > general, the board in question is now in maintenance and there will be
> > no kernel update anymore in the future. Therefore, it is fine with me if you
> remove or rework the code in question completely. Hope that helps.
> >
> > Best regards
> > Holger
> 
> Thanks for the response. So given this clarification, how was commit
> 926eae604403 ("dsa: mv88e6xxx: make serdes SGMII/Fiber tx amplitude
> configurable") useful for you?

the Kirkwood based board in question was OOT. Due to the patch we were
able to use the mainline driver without patching it to configure the value we
wanted.

The DTS node looked like this:

&mdio {
        status = "okay";

        switch@10 {
                compatible = "marvell,mv88e6085";
                #address-cells = <1>;
                #size-cells = <0>;
                reg = <0x10>;
                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        port@4 {
                                reg = <4>;
                                label = "port4";
                                phy-connection-type = "sgmii";
                                tx-p2p-microvolt = <604000>;
                                fixed-link {
                                        speed = <1000>;
                                        full-duplex;
                                };
                        };
	};
};

Powered by blists - more mailing lists