[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z5qmIEc6xEaeY6ys@shell.armlinux.org.uk>
Date: Wed, 29 Jan 2025 22:05:20 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>, Tristram.Ha@...rochip.com
Cc: 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
(To Tristram as well - I've added a workaround for your company mail
sewers that don't accept bounces from emails that have left your
organisation - in other words, once they have left your companies
mail servers, you have no idea whether they reached their final
recipient. You only get to know if your email servers can't send it
to the very _next_ email server.)
On Wed, Jan 29, 2025 at 11:12:26PM +0200, Vladimir Oltean 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.
Agreed. I know that the KSZ9477 information says differently, but if
the implementation is the Synopsys DesignWare XPCS, then surely the
register details in Synopsys' documentation should apply... so I'm
also interested to know why KSZ9477 seems to deviate from Synopsys'
implementation on this need.
I've been wondering whether setting DW_VR_MII_SGMII_LINK_STS and
DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII in 1000base-X mode is something
that should be done anyway, but from what Vladimir is saying, there's
nothing in Synopsys' documentation that supports it.
The next question would be whether it's something that we _could_
always do - if it has no effect for anyone else, then removing a
conditional simplifies the code.
> > (DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII has to be used together with
> > DW_VR_MII_TX_CONFIG_MASK to mean 0x08. Likewise for
> > DW_VR_MII_PCS_MODE_C37_SGMII and DW_VR_MII_PCS_MODE_MASK to mean 0x04.
> > It is a little difficult to just use those names to indicate the actual
> > value.)
> >
> > DW_VR_MII_DIG_CTRL1 is never touched. DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW
> > does not exist in KSZ9477 implementation. As setting that bit does not
> > have any effect I did not do anything about it.
>
> Never touched by whom? xpcs_config_aneg_c37_sgmii() surely tries to
> touch it... Don't you think that the absence of this bit from the
> KSZ9477 implementation might have something to do with KSZ9477's unique
> need to force the link speed when in in-band mode?
I think Tristram is talking about xpcs_config_aneg_c37_1000basex()
here, not SGMII.
Tristram, as a general note: there is a reason I utterly hate the term
"SGMII" - and the above illustrates exactly why. There is Cisco SGMII
(the modified 1000base-X protocol for use with PHYs.) Then there is the
"other" SGMII that manufacturers like to band about because they want
to describe their "Serial Gigabit Media Independent Interface" and they
use it to describe an interface that supports both 1000base-X and Cisco
SGMII.
This overloading of "SGMII" leads to nothing but confusion - please be
specific about whether you are talking about 1000base-X or Cisco SGMII,
and please please please avoid using "SGMII".
However, in the kernel code, we use "SGMII" exclusively to mean Cisco
SGMII.
> > 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.
> > KSZ9477 errata module 7 indicates the MII_ADVERTISE register needs to be
> > set 0x01A0. This is done with phylink_mii_c22_pcs_encode_advertisement()
> > but only for 1000BaseX mode. I probably need to add that code in SGMII
> > configuration.
Hang on one moment... I think we're going off to another problem.
For 1000base-X, we do use phylink_mii_c22_pcs_encode_advertisement()
which will generate the advertisement word for 1000base-X.
For Cisco SGMII, it will generate the tx_config word for a MAC-side
setup (which is basically the fixed 0x4001 value.) From what I read
in KSZ9477, this value would be unsuitable for a case where the
following register values are:
DW_VR_MII_PCS_MODE_C37_SGMII set
DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII set
DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL clear
meaning that we're generating a SGMII PHY-side word indicating the
parameters to be used from the registers rather than hardware signals.
> > The default value of this register is 0x20. This update
> > depends on SFP. So far I did not find a SGMII SFP that requires this
> > setting. This issue is more like the hardware did not set the default
> > value properly. As I said, the SGMII port works with SGMII SFP after
> > power up without programming anything.
> >
> > I am always confused by the master/slave - phy/mac nature of the SFP.
> > The hardware designers seem to think the SGMII module is acting as a
> > master then the slave is on the other side, like physically outside the
> > chip. I generally think of the slave is inside the SFP, as every board
> > is programmed that way.
I think you're getting confused by microchip terminology.
Cisco SGMII is an asymmetric protocol. Cisco invented it as a way of:
1. supporting 10M and 100M speeds over a single data pair in each
direction.
2. sending the parameters of that link from the PHY to the MAC/PCS over
that single data pair.
They took the IEEE 1000base-X specification as a basis (which is
symmetric negotiation via a 16-bit word).
The Cisco SGMII configuration word from the PHY to the PCS/MAC
contains:
bit 15 - link status
bit 14 - (reserved for AN acknowledge as per 1000base-X)
bit 13 - reserved (zero)
bit 12 - duplex mode
bit 11, 10 - speed
bits 9..1 - reserved (zero)
bit 0 - set to 1
This is "PHY" mode, or in Microchip parlence "master" mode - because
the PHY is dictating what the other end should be doing.
When the PCS/MAC receives this, the PCS/MAC is expected to respond
with a configuration word containing:
bit 15 - zero
bit 14 - set to 1 (indicating acknowledge)
bit 13..1 - zero
bit 0 - set to 1
This is MAC mode, or in Microchip parlence "slave" mode - because the
MAC is being told what it should do.
So, for a Cisco SGMII link with a SFP module which has a PHY embedded
inside, you definitely want to be using MAC mode, because the PHY on
the SFP module will be dictating the speed and duplex to the components
outside of the SFP - in other words the PCS and MAC.
> > For SFPs with label 10/100/1000Base-T
> > they start in SGMII mode. For SFPs with just 1000Base-T they start in
> > 1000BaseX mode and needs 0x18 value to work. In Linux the PHY inside the
> > SFP can switch back to SGMII mode and so the SGMII setting is used
> > because the EEPROM says SGMII mode is supported.
Ummm.... not quite. The SFP module EEPROM actually says absolutely
nothing about whether 1000base-X or Cisco SGMII should be used
with a module. The Linux SFP code (which I wrote, so I've torn my hair
out over this issue) does a best guess based on what it finds - but
ultimately, what it comes down to is... if we find a PHY that we can
access, it is a gigabit PHY, and we have a driver for it, then we
(re)program the PHY to use Cisco SGMII.
If we don't find a PHY, and it doesn't look like it's a copper module,
then we use 1000base-X (because SFPs were originally for fibre.)
We do have quirks to cope with weirdness, particularly with GPON
modules.
> > There are some SFPs
> > that will use only 1000BaseX mode. I wonder why the SFP manufacturers do
> > that. It seems the PHY access is also not reliable as some registers
> > always have 0xff value in lower part of the 16-bit value. That may be
> > the reason that there is a special Marvell PHY id just for Finisar.
I don't have any modules that have a Finisar PHY rather than a Marvell
PHY. I wonder if the problem is that the Finisar module doesn't like
multi-byte I2C accesses to the PHY.
Another thing - make sure that the I2C bus to the SFP cage is running
at 100kHz, not 400kHz.
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
View attachment "0001-net-xpcs-rearrange-register-definitions.patch" of type "text/x-diff" (2745 bytes)
View attachment "0003-net-xpcs-add-support-for-configuring-width-of-10-100.patch" of type "text/x-diff" (2689 bytes)
View attachment "0002-net-xpcs-document-SGMII-settings.patch" of type "text/x-diff" (1791 bytes)
View attachment "0004-net-xpcs-add-SGMII-mode-setting.patch" of type "text/x-diff" (4363 bytes)
Powered by blists - more mailing lists