Message-ID: <Z5tNcNINk1CMDBeo@shell.armlinux.org.uk>
Date: Thu, 30 Jan 2025 09:59:12 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Tristram.Ha@...rochip.com
Cc: olteanv@...il.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 Thu, Jan 30, 2025 at 04:50:14AM +0000, Tristram.Ha@...rochip.com wrote:
> > On Wed, Jan 29, 2025 at 12:31:09AM +0000, Tristram.Ha@...rochip.com wrote:
> > > The default value of DW_VR_MII_AN_CTRL is DW_VR_MII_PCS_MODE_C37_SGMII
> > > (0x04).  When a SGMII SFP is used the SGMII port works without any
> > > programming.  So for example network communication can be done in U-Boot
> > > through the SGMII port.  When a 1000BaseX SFP is used that register needs
> > > to be programmed (DW_VR_MII_SGMII_LINK_STS |
> > > DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII |
> > DW_VR_MII_PCS_MODE_C37_1000BASEX)
> > > (0x18) for it to work.
> > 
> > Can it be that DW_VR_MII_PCS_MODE_C37_1000BASEX is the important setting
> > when writing 0x18, and the rest is just irrelevant and bogus? If not,
> > could you please explain what is the role of DW_VR_MII_SGMII_LINK_STS |
> > DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII for 1000Base-X? The XPCS data book
> > does not suggest they would be considered for 1000Base-X operation. Are
> > you suggesting for KSZ9477 that is different? If so, please back that
> > statement up.
> 
> As I mentioned before, the IP used in KSZ9477 is old and so may not match
> the behavior in current DesignWare specifications.  I experimented with
> different settings and observed these behaviors.
> 
> DW_VR_MII_DIG_CTRL1 has default value 0x2200.  Setting MAC_AUTO_SW (9)
> has no effect as the value read back does not retain the bit.  Setting
> PHY_MODE_CTRL (0) retains the bit but does not seem to have any effect
> and it is not required for operation.  So we can ignore this register
> for KSZ9477.

So the value of 0x2200 for DIG_CTRL1 means that bits 13 and 9 are both
set, which are the EN_VSMMD1 and MAC_AUTO_SW bits. So, are you saying
that MAC_AUTO_SW can't be cleared in KSZ9477? This is key information
that we need.

PHY_MODE_CTRL only has an effect when:
	DW_VR_MII_PCS_MODE_C37_SGMII
	DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII
are both set.

are set, and it determines the source for the bits in the configuration
word to be sent to the _MAC_ (since XPCS is acting as a PHY with both
these bits set.) 

> DW_VR_MII_AN_CTRL has default value 0x0004, which means C37_SGMII is
> enabled.  Plugging in a 10/100/1000Base-T SFP will work without doing
> anything.
> 
> Setting TX_CONFIG_PHY_SIDE_SGMII (3) requires auto-negotiation to be
> disabled in MII_BMCR for the port to work.

If you have a SFP plugged in, then setting PHY-side on the XPCS is
wrong. TX_CONFIG_MAC_SIDE_SGMII should be used, and thus PHY_MODE_CTRL
is irrelevant as we are not generating a PHY-side Cisco SGMII config
word (as I detailed when I described the Cisco SGMII config words which
are asymmetric in nature.)

> SGMII_LINK_STS (4) depends on TX_CONFIG_PHY_SIDE_SGMII.  If that bit is
> not set then this bit has no effect.  The result of setting this bit is
> auto-negotiation can be enabled in MII_BMCR.

"that bit" and "this bit" makes this difficult to follow as I'm not
sure which of SGMII_LINK_STS or TX_CONFIG_PHY_SIDE_SGMII that "this"
and "that" are referring to. Please be explicit to avoid confusion.

Since in later documentation, SGMII_LINK_STS is used to populate bit
15 of the Cisco SGMII configuration word when operating in PHY mode
when these bits are set:

	TX_CONFIG_PHY_SIDE_SGMII
	PHY_MODE_CTRL

then, as PHY_MODE_CTRL is not supported by your hardware (it's marked
as reserved) I suggest that this older version of XPCS either has this
PHY_MODE_CTRL as an integration-time option, or the logic of taking
the values from registers was not implemented in that revision. Thus
why it only depends on TX_CONFIG_PHY_SIDE_SGMII in your case.

> So C37_SGMII mode can still work with TX_CONFIG_PHY_SIDE_SGMII on and
> auto-negotiation disabled.

That's because if the XPCS acts as a PHY and is talking to another PHY,
the only then that the remote PHY (in the SFP module) is looking for is
an acknowledgement (bit 14 set.) It's possible that XPCS does this
despite operating as a PHY. However, there is another (remote)
possibility.

> But the problem is when the cable is
> unplugged and plugged the port does not work as the module cannot detect
> the link.  Enabling auto-negotiation and then disabling it will cause
> the port to work again.

... because the XPCS is operating in the wrong mode, and when operating
as a PHY does not expect the other end "the MAC" to be signalling link
status to it. One end of a Cisco SGMII link must operate as a PHY and
the other end must operate as a MAC to be correct. Other configurations
may sort-of work but are incorrect to the Cisco SGMII documentation.

> Now for 1000BaseX mode C37_1000BASEX is used and when auto-negotiation
> is disabled the port works.

There's something which can complicate "works" - some implementations
have a "bypass" mode for negotiation. If they only receive idles
without any sign of negotiation, after a timeout expires, they enter
"bypass" mode and bring the data link up anyway.

> For the XPCS driver this can be done by setting neg_mode to false at
> the beginning. Problem is this flag can > only be set once at driver
> initialization.

Sigh. So you are referring to struct phylink_pcs's boolean neg_mode
member here, and fiddling with that is _wrong_. This flag is a
property of the driver code. It's "this driver code wants to see the
PHYLINK_PCS_NEG_* constants passed into its functions" when set,
as opposed to the MLO_AN_* constants that legacy drivers had. This
flag _must_ match the driver. It is _not_ to be fiddled with depending
on IP versions or other crap like that. It's purely about the code in
the driver. Do not touch this boolean. Do not change it in the XPCS
driver. It is correct. Setting it to false is incorrect. Am I clear?

> So for 1000BaseX mode TX_CONFIG_PHY_SIDE_SGMII can be turned on and then
> SGMII_LINK_STS allows auto-negotiation to be enabled all the time for
> both SGMII and 1000BaseX modes to work.
> 
> C37_SGMII working:
> 
> Auto-negotiation enabled
> 
> Auto-negotiation disabled
> TX_CONFIG_PHY_SIDE_SGMII on
> (stop working after cable is unplugged and re-plugged)
> 
> C37_1000BASEX working:
> 
> Auto-negotiation disabled
> 
> Auto-negotiation disabled
> TX_CONFIG_PHY_SIDE_SGMII on
> 
> Auto-negotiation enabled
> TX_CONFIG_PHY_SIDE_SGMII on
> SGMII_LINK_STS on
> 
> Note this behavior for 1000BaseX mode only occurs in KSZ9477, so we can
> stop finding the reasons with current specs.

Right, and its as documented in the KSZ9477 documentation. 1000base-X
mode requires TX_CONFIG_PHY_SIDE_SGMII and SGMII_LINK_STS. Likely
because of the older IP version.

Let me get back to what I said in my previous email - but word it
differently:

   Can we think that setting both TX_CONFIG_PHY_SIDE_SGMII and
   SGMII_LINK_STS unconditionally in the 1000base-X path with will
   not have any deterimental effects on newer IP versions?

> Microchip has another chip with newer IP version that does not have this
> behavior for 1000BaseX mode.  That is, it does not require
> auto-negotiation to be disabled for the port to work.  However, that chip
> has major issues when using 2.5G mode so I do not know how reliable it is
> when using 1G mode.

I think we're getting mixed up here. I think you've said that:

In Cisco SGMII mode, setting MAC mode with AN enabled works.
In 1000base-X mode, setting both TX_CONFIG_PHY_SIDE_SGMII and
SGMII_LINK_STS with AN enabled works.

To me, this seems to be the right configuration. The problem all along
that Vladimir has been trying to get to the bottom of is why you need
TX_CONFIG_PHY_SIDE_SGMII and SGMII_LINK_STS set in 1000base-X mode,
and the answer to that is: KSZ9477 documentation for an older XPCS IP
version states that this is necessary.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists