netdev - Re: [PATCH RFC net-next 1/2] net: pcs: xpcs: Add special code to operate in Microchip KSZ9477 switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250129230549.kwkcxdn62mghdlx3@skbuf>
Date: Thu, 30 Jan 2025 01:05:49 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Tristram.Ha@...rochip.com, Woojung.Huh@...rochip.com, andrew@...n.ch,
	hkallweit1@...il.com, maxime.chevallier@...tlin.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, UNGLinuxDriver@...rochip.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 1/2] net: pcs: xpcs: Add special code to
 operate in Microchip KSZ9477 switch

On Wed, Jan 29, 2025 at 10:05:20PM +0000, Russell King (Oracle) wrote:
> > > It does have the intended effect of separating SGMII and 1000BaseX modes
> > > in later versions.  And DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL is used along
> > > with it.  They are mutually exclusive.  For SGMII SFP
> > > DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW is set; for 1000BaseX SFP
> > > DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL is set.
> > 
> > It's difficult for me to understand what you are trying to communicate here.
> 
> I think it makes sense - MAC_AUTO_SW is meaningless in 1000base-X mode
> because the speed is fixed at 1G, whereas in Cisco SGMII MAC mode this
> bit allows the PCS to change its speed setting according to the AN
> result.

The bit which you've just explained is the only portion that made some
sense to me. What did not make sense was:

- What is the subject of the first sentence? "it has the intended effect
  of separating SGMII and 1000BaseX modes" <- who?

- "For 1000BaseX SFP, PHY_MODE_CTRL is set"? How come, and according to whom?
  PHY_MODE_CTRL, as I've previously pasted from the XPCS data book in a
  previous table, is a field which selects, while in SGMII PHY mode,
  whether the contents of the auto-negotiation code word comes from
  wires (when set to 1) or from registers (when set to 0).

For this second reply, I even went back to triple-check this, and I am
copying this additional sentence about PHY_MODE_CTRL.

| Note: This bit should be set only when XPCS is configured as
| SGMII/QSGMII PHY i.e., TX_CONFIG=1
| In other configurations, this field is reserved and read-only.

So it is very confusing to me that Tristram would be talking about
PHY_MODE_CTRL in the context of 1000Base-X. I don't know what this
denotes, but it just makes me question whether whatever he's been
calling 1000Base-X all along is something else entirely. This
"guessing what Tristram is trying to say" game is hard.

> For Vladimir: I've added four hacky patches that build on top of the
> large RFC series I sent earlier which add probably saner configuration
> for the SGMII code, hopefully making it more understandable in light
> of Wangxun's TXGBE using PHY mode there (they were adamant that their
> hardware requires it.) These do not address Tristram's issue.

Ok, let's sidetrack Tristram's thread, sure.

Patch 2: correct but

+	/* PHY_MODE_CTRL only applies for PHY-side SGMII. When PHY_MODE_CTRL
+	 * is set, the SGMII tx_config register bits 15 (link), 12 (duplex)
+	 * and 11:10 (speed) sent is derived from hardware inputs to the XPCS.
+	 * When clear, bit 15 comes from DW_VR_MII_AN_CTRL bit 4, bit 12 from
+	 * MII_ADVERTISE bit 5, and bits 11:10 from MII_BMCR speed bits. In
+	 * the latter case, some implementation documentatoin states that
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                 integration documentation
+	 * MII_ADVERTISE must be written last.
+	 */

Patch 3: "DW_XPCS_SGMII_10_100_UNCHANGED" instead of "UNSET", maybe?
Maybe it's just me, but "unset" sounds like "set to 0"/"cleared".

Patch 4: same "documentatoin" typo.

Otherwise I think there is value in these clarification patches.