Convention regarding SGMII in-band autonegotiation

Convention regarding SGMII in-band autonegotiation

[Daniel Golle]

Hi!

I've been dealing with several SGMII TP PHYs, some of them with support
for 2500Base-T, by switching to 2500Base-X interface mode (or by using
rate-adaptation to 2500Base-X or proprietary HiSGMII).

Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
rate-adaptation which is worth avoiding imho) I've noticed that the
current behavior of PHY and MAC drivers in the kernel is not as
consistent as I assumed it would be.

Background:
From Russell's comments and the experiments carried out together with
Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
understood that in general in-band autonegotiation should always be
switched off unless phylink_autoneg_inband(mode) returns true, ie.
mostly in case 'managed = "in-band-status";' is set in device tree,
which is generally the case for SFP cages or PHYs which are not
accessible via MDIO.

As of today this is what pcs-mtk-lynxi is now doing as this behavior
was inherited from the implementation previously found at
drivers/net/ethernet/mediatek/mtk_sgmii.c.

Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
in-band autonegotiation. It is not needed as we have out-of-band status
using MDIO and maybe even an interrupt to communicate the link status
between the two. Correct so far?

I've also previously worked around this using Vladimir Oltean's patch
series introducing sync'ing and validation of in-band-an modes between
MAC and PHY -- however, this turns out to be overkill in case the
above is true and given there is a way to always switch off in-band-an
on both, the MAC and the PHY.

Or should PHY drivers setup in-band AN according to
pl->config->ovr_an_inband...?

Also note that the current behavior of PHY drivers is that consistent:

 * drivers/net/phy/mxl-gpy.c
   This goes through great lengths to switch on inband-an when initially
   coming up in SGMII mode, then switches is off when switching to
   2500Base-X mode and after that **never switches it on again**. This
   is obviously not correct and the driver can be greatly reduced if
   dropping all that (non-)broken logic.
   Would a patch like [1] this be acceptable?

 * drivers/net/phy/realtek.c
   The driver simply doesn't do anything about in-band-an and hence looks
   innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
   by default in SGMII mode after reset. As many vendors use rate-adapter-
   mode, this only surfaces if not using the rate-adapter and having the
   MAC follow the PHY mode according to speed, as we do using [2] and [3].

   SGMII in-band AN can be switched off using a magic sequence carried
   out on undocumented registers [5].

   Would patches [2], [3], [4], [5] be acceptable?


Thank you for your advise!


Daniel

[1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
[2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
[3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
[4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
[5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD

Re: Convention regarding SGMII in-band autonegotiation

[Heiner Kallweit]

On 04.04.2023 02:29, Daniel Golle wrote:
> Hi!
> 
> I've been dealing with several SGMII TP PHYs, some of them with support
> for 2500Base-T, by switching to 2500Base-X interface mode (or by using
> rate-adaptation to 2500Base-X or proprietary HiSGMII).
> 
> Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
> rate-adaptation which is worth avoiding imho) I've noticed that the
> current behavior of PHY and MAC drivers in the kernel is not as
> consistent as I assumed it would be.
> 
> Background:
>>From Russell's comments and the experiments carried out together with
> Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
> understood that in general in-band autonegotiation should always be
> switched off unless phylink_autoneg_inband(mode) returns true, ie.
> mostly in case 'managed = "in-band-status";' is set in device tree,
> which is generally the case for SFP cages or PHYs which are not
> accessible via MDIO.
> 
> As of today this is what pcs-mtk-lynxi is now doing as this behavior
> was inherited from the implementation previously found at
> drivers/net/ethernet/mediatek/mtk_sgmii.c.
> 
> Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
> in-band autonegotiation. It is not needed as we have out-of-band status
> using MDIO and maybe even an interrupt to communicate the link status
> between the two. Correct so far?
> 
> I've also previously worked around this using Vladimir Oltean's patch
> series introducing sync'ing and validation of in-band-an modes between
> MAC and PHY -- however, this turns out to be overkill in case the
> above is true and given there is a way to always switch off in-band-an
> on both, the MAC and the PHY.
> 
> Or should PHY drivers setup in-band AN according to
> pl->config->ovr_an_inband...?
> 
> Also note that the current behavior of PHY drivers is that consistent:
> 
>  * drivers/net/phy/mxl-gpy.c
>    This goes through great lengths to switch on inband-an when initially
>    coming up in SGMII mode, then switches is off when switching to
>    2500Base-X mode and after that **never switches it on again**. This
>    is obviously not correct and the driver can be greatly reduced if
>    dropping all that (non-)broken logic.
>    Would a patch like [1] this be acceptable?
> 
>  * drivers/net/phy/realtek.c
>    The driver simply doesn't do anything about in-band-an and hence looks
>    innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
>    by default in SGMII mode after reset. As many vendors use rate-adapter-
>    mode, this only surfaces if not using the rate-adapter and having the
>    MAC follow the PHY mode according to speed, as we do using [2] and [3].
> 
These PHY's are supported as internal PHY's in RTL8125 MAC/PHY chips
where the MAC/PHY communication is handled chip-internally.
Other use cases are not officially supported (yet), also due to lack of
public datasheets.

>    SGMII in-band AN can be switched off using a magic sequence carried
>    out on undocumented registers [5].
> 
>    Would patches [2], [3], [4], [5] be acceptable?
> 
Ideas from the patches can be re-used. Some patches itself are not ready
for mainline (replace magic numbers with proper constants (as far as
documented by Realtek), inappropriate use of phy_modify_mmd_changed,
read_status() being wrong place for updating interface mode).

> 
> Thank you for your advise!
> 
> 
> Daniel
> 
> [1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
> [2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
> [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
> [4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
> [5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD

Re: Convention regarding SGMII in-band autonegotiation

[Daniel Golle]

On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
> On 04.04.2023 02:29, Daniel Golle wrote:
> > Hi!
> > 
> > I've been dealing with several SGMII TP PHYs, some of them with support
> > for 2500Base-T, by switching to 2500Base-X interface mode (or by using
> > rate-adaptation to 2500Base-X or proprietary HiSGMII).
> > 
> > Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
> > rate-adaptation which is worth avoiding imho) I've noticed that the
> > current behavior of PHY and MAC drivers in the kernel is not as
> > consistent as I assumed it would be.
> > 
> > Background:
> >>From Russell's comments and the experiments carried out together with
> > Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
> > understood that in general in-band autonegotiation should always be
> > switched off unless phylink_autoneg_inband(mode) returns true, ie.
> > mostly in case 'managed = "in-band-status";' is set in device tree,
> > which is generally the case for SFP cages or PHYs which are not
> > accessible via MDIO.
> > 
> > As of today this is what pcs-mtk-lynxi is now doing as this behavior
> > was inherited from the implementation previously found at
> > drivers/net/ethernet/mediatek/mtk_sgmii.c.
> > 
> > Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
> > in-band autonegotiation. It is not needed as we have out-of-band status
> > using MDIO and maybe even an interrupt to communicate the link status
> > between the two. Correct so far?
> > 
> > I've also previously worked around this using Vladimir Oltean's patch
> > series introducing sync'ing and validation of in-band-an modes between
> > MAC and PHY -- however, this turns out to be overkill in case the
> > above is true and given there is a way to always switch off in-band-an
> > on both, the MAC and the PHY.
> > 
> > Or should PHY drivers setup in-band AN according to
> > pl->config->ovr_an_inband...?
> > 
> > Also note that the current behavior of PHY drivers is that consistent:
> > 
> >  * drivers/net/phy/mxl-gpy.c
> >    This goes through great lengths to switch on inband-an when initially
> >    coming up in SGMII mode, then switches is off when switching to
> >    2500Base-X mode and after that **never switches it on again**. This
> >    is obviously not correct and the driver can be greatly reduced if
> >    dropping all that (non-)broken logic.
> >    Would a patch like [1] this be acceptable?
> > 
> >  * drivers/net/phy/realtek.c
> >    The driver simply doesn't do anything about in-band-an and hence looks
> >    innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
> >    by default in SGMII mode after reset. As many vendors use rate-adapter-
> >    mode, this only surfaces if not using the rate-adapter and having the
> >    MAC follow the PHY mode according to speed, as we do using [2] and [3].
> > 
> These PHY's are supported as internal PHY's in RTL8125 MAC/PHY chips
> where the MAC/PHY communication is handled chip-internally.
> Other use cases are not officially supported (yet), also due to lack of
> public datasheets.

The PHYs I've been encountering in the wild are those which were added by
74d155be2677a ("net: phy: realtek: Add support for RTL8221B-CG series")

They are independently packaged ICs. The relevant datasheets are
not public, but do provide documentation of some but not all registers
of those PHYs.

> 
> >    SGMII in-band AN can be switched off using a magic sequence carried
> >    out on undocumented registers [5].
> > 
> >    Would patches [2], [3], [4], [5] be acceptable?
> > 
> Ideas from the patches can be re-used. Some patches itself are not ready
> for mainline (replace magic numbers with proper constants (as far as
> documented by Realtek), inappropriate use of phy_modify_mmd_changed,
> read_status() being wrong place for updating interface mode).

Unfortunately the registers used to switch off rate-adapter-mode and
also to switch off (Hi)SGMII in-band autonegotation are entirely
undocumented even in the non-public datasheet.
They read/write/read-poll sequences supposedly appear in a document
called "SERDES Mode Setting Flow Application Note" which also doesn't
explain the meaning of the registers and their bits.

Where is updating the interface mode supposed to happen?

I was looking at drivers/net/phy/mxl-gpy.c which calls its
gpy_update_interface() function also from gpy_read_status(). If that is
wrong it should probably be fixed in mxl-gpy.c as well...

> 
> > 
> > Thank you for your advise!
> > 
> > 
> > Daniel
> > 
> > [1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
> > [2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
> > [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
> > [4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
> > [5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD
> 

Re: Convention regarding SGMII in-band autonegotiation

[Russell King (Oracle)]

On Tue, Apr 04, 2023 at 10:13:59AM +0100, Daniel Golle wrote:
> On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
> > Ideas from the patches can be re-used. Some patches itself are not ready
> > for mainline (replace magic numbers with proper constants (as far as
> > documented by Realtek), inappropriate use of phy_modify_mmd_changed,
> > read_status() being wrong place for updating interface mode).
> 
> Where is updating the interface mode supposed to happen?

I think Heiner may be confused.

The interface mode update can _only_ happen in read_status(). It's just
another parameter that the PHY changes as a result of media
negotiation, just like "speed" and "duplex" etc.

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

Re: Convention regarding SGMII in-band autonegotiation

[Heiner Kallweit]

On 04.04.2023 11:13, Daniel Golle wrote:
> On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
>> On 04.04.2023 02:29, Daniel Golle wrote:
>>> Hi!
>>>
>>> I've been dealing with several SGMII TP PHYs, some of them with support
>>> for 2500Base-T, by switching to 2500Base-X interface mode (or by using
>>> rate-adaptation to 2500Base-X or proprietary HiSGMII).
>>>
>>> Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
>>> rate-adaptation which is worth avoiding imho) I've noticed that the
>>> current behavior of PHY and MAC drivers in the kernel is not as
>>> consistent as I assumed it would be.
>>>
>>> Background:
>>> >From Russell's comments and the experiments carried out together with
>>> Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
>>> understood that in general in-band autonegotiation should always be
>>> switched off unless phylink_autoneg_inband(mode) returns true, ie.
>>> mostly in case 'managed = "in-band-status";' is set in device tree,
>>> which is generally the case for SFP cages or PHYs which are not
>>> accessible via MDIO.
>>>
>>> As of today this is what pcs-mtk-lynxi is now doing as this behavior
>>> was inherited from the implementation previously found at
>>> drivers/net/ethernet/mediatek/mtk_sgmii.c.
>>>
>>> Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
>>> in-band autonegotiation. It is not needed as we have out-of-band status
>>> using MDIO and maybe even an interrupt to communicate the link status
>>> between the two. Correct so far?
>>>
>>> I've also previously worked around this using Vladimir Oltean's patch
>>> series introducing sync'ing and validation of in-band-an modes between
>>> MAC and PHY -- however, this turns out to be overkill in case the
>>> above is true and given there is a way to always switch off in-band-an
>>> on both, the MAC and the PHY.
>>>
>>> Or should PHY drivers setup in-band AN according to
>>> pl->config->ovr_an_inband...?
>>>
>>> Also note that the current behavior of PHY drivers is that consistent:
>>>
>>>  * drivers/net/phy/mxl-gpy.c
>>>    This goes through great lengths to switch on inband-an when initially
>>>    coming up in SGMII mode, then switches is off when switching to
>>>    2500Base-X mode and after that **never switches it on again**. This
>>>    is obviously not correct and the driver can be greatly reduced if
>>>    dropping all that (non-)broken logic.
>>>    Would a patch like [1] this be acceptable?
>>>
>>>  * drivers/net/phy/realtek.c
>>>    The driver simply doesn't do anything about in-band-an and hence looks
>>>    innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
>>>    by default in SGMII mode after reset. As many vendors use rate-adapter-
>>>    mode, this only surfaces if not using the rate-adapter and having the
>>>    MAC follow the PHY mode according to speed, as we do using [2] and [3].
>>>
>> These PHY's are supported as internal PHY's in RTL8125 MAC/PHY chips
>> where the MAC/PHY communication is handled chip-internally.
>> Other use cases are not officially supported (yet), also due to lack of
>> public datasheets.
> 
> The PHYs I've been encountering in the wild are those which were added by
> 74d155be2677a ("net: phy: realtek: Add support for RTL8221B-CG series")
> 
> They are independently packaged ICs. The relevant datasheets are
> not public, but do provide documentation of some but not all registers
> of those PHYs.
> 
>>
>>>    SGMII in-band AN can be switched off using a magic sequence carried
>>>    out on undocumented registers [5].
>>>
>>>    Would patches [2], [3], [4], [5] be acceptable?
>>>
>> Ideas from the patches can be re-used. Some patches itself are not ready
>> for mainline (replace magic numbers with proper constants (as far as
>> documented by Realtek), inappropriate use of phy_modify_mmd_changed,
>> read_status() being wrong place for updating interface mode).
> 
> Unfortunately the registers used to switch off rate-adapter-mode and
> also to switch off (Hi)SGMII in-band autonegotation are entirely
> undocumented even in the non-public datasheet.
> They read/write/read-poll sequences supposedly appear in a document
> called "SERDES Mode Setting Flow Application Note" which also doesn't
> explain the meaning of the registers and their bits.
> 
> Where is updating the interface mode supposed to happen?
> 
> I was looking at drivers/net/phy/mxl-gpy.c which calls its
> gpy_update_interface() function also from gpy_read_status(). If that is
> wrong it should probably be fixed in mxl-gpy.c as well...
> 

Right, several drivers use the read_status() callback for this, I think
the link_change_notify() callback would be sufficient.
In interrupt mode this doesn't really make a difference, however in
polling mode we can avoid polling the register with the interface mode
information (if it's a register that isn't used for other purposes
in read_status() too).

>>
>>>
>>> Thank you for your advise!
>>>
>>>
>>> Daniel
>>>
>>> [1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
>>> [2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
>>> [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
>>> [4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
>>> [5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD
>>

Re: Convention regarding SGMII in-band autonegotiation

[Russell King (Oracle)]

On Tue, Apr 04, 2023 at 11:56:45AM +0200, Heiner Kallweit wrote:
> On 04.04.2023 11:13, Daniel Golle wrote:
> > On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
> >> Ideas from the patches can be re-used. Some patches itself are not ready
> >> for mainline (replace magic numbers with proper constants (as far as
> >> documented by Realtek), inappropriate use of phy_modify_mmd_changed,
> >> read_status() being wrong place for updating interface mode).
> > 
> > Where is updating the interface mode supposed to happen?
> > 
> > I was looking at drivers/net/phy/mxl-gpy.c which calls its
> > gpy_update_interface() function also from gpy_read_status(). If that is
> > wrong it should probably be fixed in mxl-gpy.c as well...
> > 
> 
> Right, several drivers use the read_status() callback for this, I think
> the link_change_notify() callback would be sufficient.

Sorry, but that's too late.

The problem is that phy_check_link_status() reads the link status, and
then immediately acts on it calling phy_link_up() or phy_link_down()
as appropriate. While the phy state changes at the same time, we're
still in the state machine switch() here, and it's only after that
switch() that we then call link_change_notify() - and that will be
_after_ phylink or MAC driver's adjust_link callback has happened.

So, using link_change_notify() would mean that the phylib user will
be informed of the new media parameters except for the interface mode,
_then_ link_change_notify() will be called, and only then would
phydev->interface change - and there's no callback to the phylib
user to inform them that something changed.

In any case, we do _not_ want two callbacks into the phylib user for
the state change, especially not one where the first is "link is up"
and the second is "oh by the way the interface changed".

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

Re: Convention regarding SGMII in-band autonegotiation

[Heiner Kallweit]

On 04.04.2023 12:16, Russell King (Oracle) wrote:
> On Tue, Apr 04, 2023 at 11:56:45AM +0200, Heiner Kallweit wrote:
>> On 04.04.2023 11:13, Daniel Golle wrote:
>>> On Tue, Apr 04, 2023 at 08:31:12AM +0200, Heiner Kallweit wrote:
>>>> Ideas from the patches can be re-used. Some patches itself are not ready
>>>> for mainline (replace magic numbers with proper constants (as far as
>>>> documented by Realtek), inappropriate use of phy_modify_mmd_changed,
>>>> read_status() being wrong place for updating interface mode).
>>>
>>> Where is updating the interface mode supposed to happen?
>>>
>>> I was looking at drivers/net/phy/mxl-gpy.c which calls its
>>> gpy_update_interface() function also from gpy_read_status(). If that is
>>> wrong it should probably be fixed in mxl-gpy.c as well...
>>>
>>
>> Right, several drivers use the read_status() callback for this, I think
>> the link_change_notify() callback would be sufficient.
> 
> Sorry, but that's too late.
> 
Indeed, my bad. It's been some time ago that I last looked deeper
into this corner of phylib.

> The problem is that phy_check_link_status() reads the link status, and
> then immediately acts on it calling phy_link_up() or phy_link_down()
> as appropriate. While the phy state changes at the same time, we're
> still in the state machine switch() here, and it's only after that
> switch() that we then call link_change_notify() - and that will be
> _after_ phylink or MAC driver's adjust_link callback has happened.
> 
> So, using link_change_notify() would mean that the phylib user will
> be informed of the new media parameters except for the interface mode,
> _then_ link_change_notify() will be called, and only then would
> phydev->interface change - and there's no callback to the phylib
> user to inform them that something changed.
> 
> In any case, we do _not_ want two callbacks into the phylib user for
> the state change, especially not one where the first is "link is up"
> and the second is "oh by the way the interface changed".
> 

Re: Convention regarding SGMII in-band autonegotiation

[Russell King (Oracle)]

Hi Daniel,

First thing I'll say is that bear in mind that historically Linux
didn't have any standard for using (or not) the in-band messages in
SGMII and 1000base-X. That was a big mistake in my opinion, and
leads to some of the problems today, where we're trying to have
things work consistently for the sake of SFP support.

In order to combat this, I try to ensure that phylink-using
implementations are consistent. Hence why I was insistent that
mtk_sgmii behaves in a particular way w.r.t enabling in-band mode.
Even if it doesn't solve everything, it is consistent with many of
the other implementations, which means if we want to change it in
the future, we can do so across all implementations.

As previously stated, one of the things I want to do is lift the
decision about whether in-band mode should be enabled in the PCS up out
of the PCS driver into phylink so the PCS driver doesn't need to be
making those decisions. I have some patches for that but they aren't
going anywhere until the MV88E6XXX DSA driver gets sorted out and
converted to phylink_pcs. This is the _last_ phylink based driver that
isn't using phylink_pcs for SGMII/1000base-X protocols. For that
conversion to happen, I need to get the default-to-fastest-speed
patches merged as a pre-requisit, but those have been a major problem
for a year now and whatever solution I propose there are always
objections to it. The current solution (using swnodes) was Vladimir's
suggestion, but the swnode people hate it, and I hate their
suggestions of how to make it acceptable to them (because to do it
correctly, I'm quite sure the DSA maintainers will then object
because it will have to be in the DSA core code again. I am at the
point of giving up with it, because there seems to be no way forward
that *everyone* finds acceptable. As a direct result of this, I've
more or less stopped doing much proper phylink development because
the patches will just continue to back up.

Maybe the right answer is not to do that, but to let mv88e6xxx rot
and hope that some day someone has the will power to solve the
problems - and if that means mv88e6xxx breaks, then so be it (but
that goes against the "no regressions" rule, so also can't really
be done.)

On Tue, Apr 04, 2023 at 01:29:31AM +0100, Daniel Golle wrote:
> Hi!
> 
> I've been dealing with several SGMII TP PHYs, some of them with support
> for 2500Base-T, by switching to 2500Base-X interface mode (or by using
> rate-adaptation to 2500Base-X or proprietary HiSGMII).
> 
> Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
> rate-adaptation which is worth avoiding imho) I've noticed that the
> current behavior of PHY and MAC drivers in the kernel is not as
> consistent as I assumed it would be.

Yes, rate adaption comes with it a bunch of issues such as always
having to have pause frames recognised by the MAC, or having the
requirement to increase the inter-packet gap (which no MAC driver
currently supports).

However, switching the interface mode *requires* us to know what the
PHY is doing, so the PHY must be accessible in order for this to be
even remotely possible.

> Background:
> From Russell's comments and the experiments carried out together with
> Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
> understood that in general in-band autonegotiation should always be
> switched off unless phylink_autoneg_inband(mode) returns true, ie.
> mostly in case 'managed = "in-band-status";' is set in device tree,
> which is generally the case for SFP cages or PHYs which are not
> accessible via MDIO.

Not quite, the rule for consistent behaviour is:

- When operating in *SGMII modes, then:
   - if in-band AN mode, SGMII in-band mode should be enabled.
   - otherwise SGMII in-band mode disabled.

  Let's be clear what SGMII in-band mode is. It is *not* negotiation.
  The PCS doesn't advertise anything. The PHY doesn't take action and
  change what it's doing as a result of what it receives from the PCS.
  It is status passing from the PHY to the PCS, and an acknowledgement
  by the PCS back to the PHY. Nothing more.

- When operating in an 802.3z mode, then
   - if in-band AN mode and the Autoneg bit is set, then 802.3z in-band
       mode should be enabled.
   - otherwise 802.3z in-band mode should be disabled.

The reason for the Autoneg bit with 802.3z, particularly 1000base-X, is
that these protocols are designed as the _media_ protocol, like
1000baseT, and thus they are proper negotiation between two ends of the
link. As such, the user needs to be able to turn on/off this
negotiation, and the accepted way to do that is via the Autoneg bit in
the advertising mask.

There are implementations where 1000base-X (and 2500base-X) is
documented as requiring in-band negotiation to always be enabled,
and as such they have a pcs_validate() function that rejects such a
combination.

Conversely, there are implementations where 2500base-X is documented
as not having in-band negotiation, and of course implementations where
1000base-X can have in-band enabled/disabled.

2500base-X is a total mess because it was not a standard, but
manufacturers decided to offer it and went off and did their own
thing. Many took their implementation and just increased the clock
rate to 3.125GHz from 1.25GHz, thus meaning that everything which
was offered at 1.25GHz clock rate is there for the faster rate.
Some document that AN isn't supported, but when you try it, it
works (because it's literally just 1000base-X up-clocked.)

Just like the "AN must always be enabled when not in SGMII mode" on
mvneta and mvpp2 hardware, the statement that AN isn't supported in
2500base-X in documentation is rather questionable.

> As of today this is what pcs-mtk-lynxi is now doing as this behavior
> was inherited from the implementation previously found at
> drivers/net/ethernet/mediatek/mtk_sgmii.c.
> 
> Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
> in-band autonegotiation. It is not needed as we have out-of-band status
> using MDIO and maybe even an interrupt to communicate the link status
> between the two. Correct so far?

Correct - and the reason is because, like it or not, there *are* PHYs
out there that do *not* provide any in-band status when operating in
SGMII mode, and there is *no* *way* to get them to do so... and those
PHYs do get used on SFP modules. So, we need a way for MAC/PCS to be
told to operate without in-band status.

This is exactly what MLO_AN_PHY mode is - it's a mode where we read
the status from the PHY manually, and configure the PCS and MAC
according to that read status.

> I've also previously worked around this using Vladimir Oltean's patch
> series introducing sync'ing and validation of in-band-an modes between
> MAC and PHY -- however, this turns out to be overkill in case the
> above is true and given there is a way to always switch off in-band-an
> on both, the MAC and the PHY.
> 
> Or should PHY drivers setup in-band AN according to
> pl->config->ovr_an_inband...?

That is out of reach of PHY drivers, and don't forget that phylink is
optional, many MAC drivers use phylib directly without phylink.

> Also note that the current behavior of PHY drivers is that consistent:

We definitely *need* consistency in how phylink is implemented in PCS
and MACs if we're going to stand a chance of SFPs behaving the same
no matter what they're plugged in to. More importantly for this point
is maintenance of phylink - if we have differing behaviours, then it
_will_ make future maintenance of phylink utterly impossible, as
attempting to fix or change the behaviour for one implementation could
end up breaking another if each make different decisions.

> 
>  * drivers/net/phy/mxl-gpy.c
>    This goes through great lengths to switch on inband-an when initially
>    coming up in SGMII mode, then switches is off when switching to
>    2500Base-X mode and after that **never switches it on again**. This
>    is obviously not correct and the driver can be greatly reduced if
>    dropping all that (non-)broken logic.
>    Would a patch like [1] this be acceptable?
> 
>  * drivers/net/phy/realtek.c
>    The driver simply doesn't do anything about in-band-an and hence looks
>    innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
>    by default in SGMII mode after reset. As many vendors use rate-adapter-
>    mode, this only surfaces if not using the rate-adapter and having the
>    MAC follow the PHY mode according to speed, as we do using [2] and [3].
> 
>    SGMII in-band AN can be switched off using a magic sequence carried
>    out on undocumented registers [5].
> 
>    Would patches [2], [3], [4], [5] be acceptable?

Before phylink, PHYs were free to do anything they like with in-band
modes, as long as the PHY and MAC combination that was being used
agreed. This leads to some PHYs that phylib has drivers for having
in-band enabled in SGMII mode (with or without managed = "in-band"
in DT) others may have it disabled.

The problem now is trying to get consistent behaviour can cause
regressions.

Introducing PHY interface switching to existing drivers can also be
problematical - that's only something we introduced to phylib when I
added the 88x3310 Marvell 10G PHY driver, and after we discussed the
problem. At that time, it was a new phylib driver and so there weren't
any MAC drivers, so we could make both the phylib and MAC drivers
deal with phydev->interface changing. Before this happened,
phydev->interface was constant that could be relied upon to never
change, so MAC drivers in general do not expect it to change.

Adding the ability for phydev->interface to change for existing
phylib drivers brings with it the obvious can of worms - are the MAC
drivers that are using that phylib driver setup to cope with the
interface mode changing? If they aren't, then clearly we can't
change the PHY's behaviour to switch its interface mode until the
MAC drivers have been updated.


The overall message in my reply is essentially one of caution - yes
we can make changes to how PHYs work, but we need to audit the MAC
drivers that the PHY is used with to try to cut down on unexpected
regressions.

> 
> 
> Thank you for your advise!
> 
> 
> Daniel
> 
> [1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
> [2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
> [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
> [4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
> [5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD

Eww, when clicking on those links, Firefox downloads them, and then when
I click on them, it decides to open Libreoffice Writer to view them!
Would've been nicer if Firefox could have displayed them directly.

In patch [4], isn't normal behaviour that a soft reset does not change
the programmed settings in the PHY? That is certainly true for Marvell
PHYs (which need to be frequently hit with a soft-reset after changing
settings to make them active). Do Realtek PHYs reset the register
contents on soft-reset?

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

Re: Convention regarding SGMII in-band autonegotiation

[Daniel Golle]

Hi Russell,

On Tue, Apr 04, 2023 at 10:33:21AM +0100, Russell King (Oracle) wrote:
> Hi Daniel,
> 
> First thing I'll say is that bear in mind that historically Linux
> didn't have any standard for using (or not) the in-band messages in
> SGMII and 1000base-X. That was a big mistake in my opinion, and
> leads to some of the problems today, where we're trying to have
> things work consistently for the sake of SFP support.
> 
> In order to combat this, I try to ensure that phylink-using
> implementations are consistent. Hence why I was insistent that
> mtk_sgmii behaves in a particular way w.r.t enabling in-band mode.
> Even if it doesn't solve everything, it is consistent with many of
> the other implementations, which means if we want to change it in
> the future, we can do so across all implementations.
> 
> As previously stated, one of the things I want to do is lift the
> decision about whether in-band mode should be enabled in the PCS up out
> of the PCS driver into phylink so the PCS driver doesn't need to be
> making those decisions. I have some patches for that but they aren't
> going anywhere until the MV88E6XXX DSA driver gets sorted out and
> converted to phylink_pcs. This is the _last_ phylink based driver that
> isn't using phylink_pcs for SGMII/1000base-X protocols. For that
> conversion to happen, I need to get the default-to-fastest-speed
> patches merged as a pre-requisit, but those have been a major problem
> for a year now and whatever solution I propose there are always
> objections to it. The current solution (using swnodes) was Vladimir's
> suggestion, but the swnode people hate it, and I hate their
> suggestions of how to make it acceptable to them (because to do it
> correctly, I'm quite sure the DSA maintainers will then object
> because it will have to be in the DSA core code again. I am at the
> point of giving up with it, because there seems to be no way forward
> that *everyone* finds acceptable. As a direct result of this, I've
> more or less stopped doing much proper phylink development because
> the patches will just continue to back up.
> 
> Maybe the right answer is not to do that, but to let mv88e6xxx rot
> and hope that some day someone has the will power to solve the
> problems - and if that means mv88e6xxx breaks, then so be it (but
> that goes against the "no regressions" rule, so also can't really
> be done.)

Performance of mvpp2 is now significantly worse from what it was some
years ago, OpenWrt users have reported that. And the driver in Marvell
SDK (released through several GPL drops) is a fork of what is in Linux
5.4 and the only way to get full performance (but obviously won't work
with more recent kernels, exactly because of phylink changes). At least
VLANs on mv88e6xxx are now again properly separated since Linux 5.15...

To me it feels like something must have happened at Marvell. They went
from all-in supporting Linux and coming up with DSA to swimming
face-down on the middle of the lake for the past year(s).

> 
> On Tue, Apr 04, 2023 at 01:29:31AM +0100, Daniel Golle wrote:
> > Hi!
> > 
> > I've been dealing with several SGMII TP PHYs, some of them with support
> > for 2500Base-T, by switching to 2500Base-X interface mode (or by using
> > rate-adaptation to 2500Base-X or proprietary HiSGMII).
> > 
> > Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
> > rate-adaptation which is worth avoiding imho) I've noticed that the
> > current behavior of PHY and MAC drivers in the kernel is not as
> > consistent as I assumed it would be.
> 
> Yes, rate adaption comes with it a bunch of issues such as always
> having to have pause frames recognised by the MAC, or having the
> requirement to increase the inter-packet gap (which no MAC driver
> currently supports).

Yeah, that matches my understanding of the story. Sadly, AQR PHYs are
all usually setup with rate adaption switched on, now I understand the
reason for that are Marvell's MAC drivers...

> 
> However, switching the interface mode *requires* us to know what the
> PHY is doing, so the PHY must be accessible in order for this to be
> even remotely possible.

Thank you for confirming this and spelling out the details.

> 
> > Background:
> > From Russell's comments and the experiments carried out together with
> > Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
> > understood that in general in-band autonegotiation should always be
> > switched off unless phylink_autoneg_inband(mode) returns true, ie.
> > mostly in case 'managed = "in-band-status";' is set in device tree,
> > which is generally the case for SFP cages or PHYs which are not
> > accessible via MDIO.
> 
> Not quite, the rule for consistent behaviour is:
> 
> - When operating in *SGMII modes, then:
>    - if in-band AN mode, SGMII in-band mode should be enabled.
>    - otherwise SGMII in-band mode disabled.
> 
>   Let's be clear what SGMII in-band mode is. It is *not* negotiation.
>   The PCS doesn't advertise anything. The PHY doesn't take action and
>   change what it's doing as a result of what it receives from the PCS.
>   It is status passing from the PHY to the PCS, and an acknowledgement
>   by the PCS back to the PHY. Nothing more.
> 
> - When operating in an 802.3z mode, then
>    - if in-band AN mode and the Autoneg bit is set, then 802.3z in-band
>        mode should be enabled.
>    - otherwise 802.3z in-band mode should be disabled.
> 
> The reason for the Autoneg bit with 802.3z, particularly 1000base-X, is
> that these protocols are designed as the _media_ protocol, like
> 1000baseT, and thus they are proper negotiation between two ends of the
> link. As such, the user needs to be able to turn on/off this
> negotiation, and the accepted way to do that is via the Autoneg bit in
> the advertising mask.
> 
> There are implementations where 1000base-X (and 2500base-X) is
> documented as requiring in-band negotiation to always be enabled,
> and as such they have a pcs_validate() function that rejects such a
> combination.
> 
> Conversely, there are implementations where 2500base-X is documented
> as not having in-band negotiation, and of course implementations where
> 1000base-X can have in-band enabled/disabled.
> 
> 2500base-X is a total mess because it was not a standard, but
> manufacturers decided to offer it and went off and did their own
> thing. Many took their implementation and just increased the clock
> rate to 3.125GHz from 1.25GHz, thus meaning that everything which
> was offered at 1.25GHz clock rate is there for the faster rate.
> Some document that AN isn't supported, but when you try it, it
> works (because it's literally just 1000base-X up-clocked.)
> 
> Just like the "AN must always be enabled when not in SGMII mode" on
> mvneta and mvpp2 hardware, the statement that AN isn't supported in
> 2500base-X in documentation is rather questionable.

On 1000Base-X and 2500Base-X we mean auto-negotiation as in Clause 37
of the Ethernet standard, as opposed to CISCO-style MAC-side or PHY-side
SGMII auto-negotiation in SGMII mode, right?
Ie. speed is already known and synchronization is setup, only information
about pause and duplex is transmitted.

> 
> > As of today this is what pcs-mtk-lynxi is now doing as this behavior
> > was inherited from the implementation previously found at
> > drivers/net/ethernet/mediatek/mtk_sgmii.c.
> > 
> > Hence, with MLO_AN_PHY we are expecting both MAC and PHY to *not* use
> > in-band autonegotiation. It is not needed as we have out-of-band status
> > using MDIO and maybe even an interrupt to communicate the link status
> > between the two. Correct so far?
> 
> Correct - and the reason is because, like it or not, there *are* PHYs
> out there that do *not* provide any in-band status when operating in
> SGMII mode, and there is *no* *way* to get them to do so... and those
> PHYs do get used on SFP modules. So, we need a way for MAC/PCS to be
> told to operate without in-band status.
> 
> This is exactly what MLO_AN_PHY mode is - it's a mode where we read
> the status from the PHY manually, and configure the PCS and MAC
> according to that read status.

Good, so I understood correctly. Again thank you for spelling it out,
I believe it will also help others to understand this better.

> 
> > I've also previously worked around this using Vladimir Oltean's patch
> > series introducing sync'ing and validation of in-band-an modes between
> > MAC and PHY -- however, this turns out to be overkill in case the
> > above is true and given there is a way to always switch off in-band-an
> > on both, the MAC and the PHY.
> > 
> > Or should PHY drivers setup in-band AN according to
> > pl->config->ovr_an_inband...?
> 
> That is out of reach of PHY drivers, and don't forget that phylink is
> optional, many MAC drivers use phylib directly without phylink.
> 
> > Also note that the current behavior of PHY drivers is that consistent:
> 
> We definitely *need* consistency in how phylink is implemented in PCS
> and MACs if we're going to stand a chance of SFPs behaving the same
> no matter what they're plugged in to. More importantly for this point
> is maintenance of phylink - if we have differing behaviours, then it
> _will_ make future maintenance of phylink utterly impossible, as
> attempting to fix or change the behaviour for one implementation could
> end up breaking another if each make different decisions.
> 
> > 
> >  * drivers/net/phy/mxl-gpy.c
> >    This goes through great lengths to switch on inband-an when initially
> >    coming up in SGMII mode, then switches is off when switching to
> >    2500Base-X mode and after that **never switches it on again**. This
> >    is obviously not correct and the driver can be greatly reduced if
> >    dropping all that (non-)broken logic.
> >    Would a patch like [1] this be acceptable?

Did you take a look at the current implementation in mxl-gpy.c and
patch [1]?

To me the current code looks obviously wrong and cannot work when
switching from SGMII (with in-band-status, initialized by the
bootloader) to 2500Base-X and then back to SGMII (which will then
always be without in-band-status), so that to me look like a bug, and
if something like that should work, the driver will need to remember
the previous state of in-band-status for SGMII instead of relying on an
already overwritten PHY register.

> > 
> >  * drivers/net/phy/realtek.c
> >    The driver simply doesn't do anything about in-band-an and hence looks
> >    innocent. However, all RTL8226* and RTL8221* PHYs enable in-band-an
> >    by default in SGMII mode after reset. As many vendors use rate-adapter-
> >    mode, this only surfaces if not using the rate-adapter and having the
> >    MAC follow the PHY mode according to speed, as we do using [2] and [3].
> > 
> >    SGMII in-band AN can be switched off using a magic sequence carried
> >    out on undocumented registers [5].
> > 
> >    Would patches [2], [3], [4], [5] be acceptable?
> 
> Before phylink, PHYs were free to do anything they like with in-band
> modes, as long as the PHY and MAC combination that was being used
> agreed. This leads to some PHYs that phylib has drivers for having
> in-band enabled in SGMII mode (with or without managed = "in-band"
> in DT) others may have it disabled.
> 
> The problem now is trying to get consistent behaviour can cause
> regressions.
> 
> Introducing PHY interface switching to existing drivers can also be
> problematical - that's only something we introduced to phylib when I
> added the 88x3310 Marvell 10G PHY driver, and after we discussed the
> problem. At that time, it was a new phylib driver and so there weren't
> any MAC drivers, so we could make both the phylib and MAC drivers
> deal with phydev->interface changing. Before this happened,
> phydev->interface was constant that could be relied upon to never
> change, so MAC drivers in general do not expect it to change.
> 
> Adding the ability for phydev->interface to change for existing
> phylib drivers brings with it the obvious can of worms - are the MAC
> drivers that are using that phylib driver setup to cope with the
> interface mode changing? If they aren't, then clearly we can't
> change the PHY's behaviour to switch its interface mode until the
> MAC drivers have been updated.
> 
> 
> The overall message in my reply is essentially one of caution - yes
> we can make changes to how PHYs work, but we need to audit the MAC
> drivers that the PHY is used with to try to cut down on unexpected
> regressions.

How do I even know which MAC driver is using which PHY driver as
the PHY is being probed using the PHY ID at run-time?
In git history there are some hints regarding this, but there are
probably a lot of "hidden" users of a PHY driver which we don't
even know about simply because the fact a certain PHY driver is
going to be used isn't documented anywhere.

I'm afraid we will need some kind of feature flag to indicate that a
MAC driver is known to behave according to convention with regards to
SGMII in-band-status being switched off and only in that case have the
PHY driver do the same, and otherwhise stick with the existing
codepaths and potentially unknown hardware-default behavior
(ie. another mess like the pre_march2020 situation...)

> 
> > 
> > 
> > Thank you for your advise!
> > 
> > 
> > Daniel
> > 
> > [1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/mediatek/patches-5.15/731-net-phy-hack-mxl-gpy-disable-sgmii-an.patch;hb=HEAD
> > [2]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/721-net-phy-realtek-rtl8221-allow-to-configure-SERDES-mo.patch;hb=HEAD
> > [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/722-net-phy-realtek-support-switching-between-SGMII-and-.patch;hb=HEAD
> > [4]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/724-net-phy-realtek-use-genphy_soft_reset-for-2.5G-PHYs.patch;hb=HEAD
> > [5]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob_plain;f=target/linux/generic/pending-5.15/725-net-phy-realtek-disable-SGMII-in-band-AN-for-2-5G-PHYs.patch;hb=HEAD
> 
> Eww, when clicking on those links, Firefox downloads them, and then when
> I click on them, it decides to open Libreoffice Writer to view them!
> Would've been nicer if Firefox could have displayed them directly.
> 
> In patch [4], isn't normal behaviour that a soft reset does not change
> the programmed settings in the PHY? That is certainly true for Marvell
> PHYs (which need to be frequently hit with a soft-reset after changing
> settings to make them active). Do Realtek PHYs reset the register
> contents on soft-reset?
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: Convention regarding SGMII in-band autonegotiation

[Russell King (Oracle)]

On Tue, Apr 04, 2023 at 12:56:40PM +0100, Daniel Golle wrote:
> > On Tue, Apr 04, 2023 at 01:29:31AM +0100, Daniel Golle wrote:
> > > Hi!
> > > 
> > > I've been dealing with several SGMII TP PHYs, some of them with support
> > > for 2500Base-T, by switching to 2500Base-X interface mode (or by using
> > > rate-adaptation to 2500Base-X or proprietary HiSGMII).
> > > 
> > > Dealing with such PHYs in MAC-follow-PHY-rate-mode (ie. not enabling
> > > rate-adaptation which is worth avoiding imho) I've noticed that the
> > > current behavior of PHY and MAC drivers in the kernel is not as
> > > consistent as I assumed it would be.
> > 
> > Yes, rate adaption comes with it a bunch of issues such as always
> > having to have pause frames recognised by the MAC, or having the
> > requirement to increase the inter-packet gap (which no MAC driver
> > currently supports).
> 
> Yeah, that matches my understanding of the story. Sadly, AQR PHYs are
> all usually setup with rate adaption switched on, now I understand the
> reason for that are Marvell's MAC drivers...

Not just MAC drivers, but MAC hardware. It turns out that the mvpp2 in
Armada 8040 commonly shipped with Macchiatobin just does *not* support
flow control. You wouldn't know that from reading the documentation,
but one of their engineers who submitted patches in the last few
years explained that it needs firmware support (as in a blob running
in the device) but that isn't present on earlier versions.

Wonderful.

So the only possibility for such mvpp2 is enlarging IPG, and there is
a register for that. Whether that works or not is another matter.

> > However, switching the interface mode *requires* us to know what the
> > PHY is doing, so the PHY must be accessible in order for this to be
> > even remotely possible.
> 
> Thank you for confirming this and spelling out the details.

Well, if we don't have access to the PHY:

1) we can't know what the media negotiated, so we don't know what the
   media speed is, and thus we won't know what IPG to use. All that we
   would know is the speed of the interface between MAC/PCS and PHY.

2) we can't know what interface mode it is using if it switches its
   interface mode according to the media.

Basically, for any PHY that operates with multiple different media
speeds, the only way it can sanely work is if we can access and
properly drive the PHY.

The one exception to that would be where the PHY does rate adaption
using pause frames, the MAC supports pause frames, and we know that
rate adaption is in use (so we know we need to enable RX pause
frames at the MAC.) Even with that, not having access to the PHY, we
have no idea what duplex it has negotiated (although pause frames
will slow down the MAC) and we also have no idea whether pause modes
were negotiated on the media side.

Essentially, having a PHY that is unaccessible isn't particularly
good news, and whether it works or not (which may depend on the
media side resolution) will be entirely hit and miss. I don't
think there is much we can do about that, other than maybe advising
people that that's what one gets with hardware that can't be
accessed.

> > > Background:
> > > From Russell's comments and the experiments carried out together with
> > > Frank Wunderlich for the MediaTek SGMII PCS found in mtk_eth_soc I
> > > understood that in general in-band autonegotiation should always be
> > > switched off unless phylink_autoneg_inband(mode) returns true, ie.
> > > mostly in case 'managed = "in-band-status";' is set in device tree,
> > > which is generally the case for SFP cages or PHYs which are not
> > > accessible via MDIO.
> > 
> > Not quite, the rule for consistent behaviour is:
> > 
> > - When operating in *SGMII modes, then:
> >    - if in-band AN mode, SGMII in-band mode should be enabled.
> >    - otherwise SGMII in-band mode disabled.
> > 
> >   Let's be clear what SGMII in-band mode is. It is *not* negotiation.
> >   The PCS doesn't advertise anything. The PHY doesn't take action and
> >   change what it's doing as a result of what it receives from the PCS.
> >   It is status passing from the PHY to the PCS, and an acknowledgement
> >   by the PCS back to the PHY. Nothing more.
> > 
> > - When operating in an 802.3z mode, then
> >    - if in-band AN mode and the Autoneg bit is set, then 802.3z in-band
> >        mode should be enabled.
> >    - otherwise 802.3z in-band mode should be disabled.
> > 
> > The reason for the Autoneg bit with 802.3z, particularly 1000base-X, is
> > that these protocols are designed as the _media_ protocol, like
> > 1000baseT, and thus they are proper negotiation between two ends of the
> > link. As such, the user needs to be able to turn on/off this
> > negotiation, and the accepted way to do that is via the Autoneg bit in
> > the advertising mask.
> > 
> > There are implementations where 1000base-X (and 2500base-X) is
> > documented as requiring in-band negotiation to always be enabled,
> > and as such they have a pcs_validate() function that rejects such a
> > combination.
> > 
> > Conversely, there are implementations where 2500base-X is documented
> > as not having in-band negotiation, and of course implementations where
> > 1000base-X can have in-band enabled/disabled.
> > 
> > 2500base-X is a total mess because it was not a standard, but
> > manufacturers decided to offer it and went off and did their own
> > thing. Many took their implementation and just increased the clock
> > rate to 3.125GHz from 1.25GHz, thus meaning that everything which
> > was offered at 1.25GHz clock rate is there for the faster rate.
> > Some document that AN isn't supported, but when you try it, it
> > works (because it's literally just 1000base-X up-clocked.)
> > 
> > Just like the "AN must always be enabled when not in SGMII mode" on
> > mvneta and mvpp2 hardware, the statement that AN isn't supported in
> > 2500base-X in documentation is rather questionable.
> 
> On 1000Base-X and 2500Base-X we mean auto-negotiation as in Clause 37
> of the Ethernet standard,

Definitely. I do not mix up these terms. When I talk about 1000Base-X,
I always mean the IEEE 802.3 defined protocol. When I talk about
2500Base-X, that is slightly less clear because of its history and lack
of standardisation, but I _personally_ think of it as 1000Base-X locked
at 2.5x faster.

The fact that IEEE 802.3 eventually decided to give in and put
something in the standard for 2500Base-X is welcome, but sadly was
way too late as it had already been around for years by the time they
did that... making their standardised version basically irrelevant in
the real world.

> as opposed to CISCO-style MAC-side or PHY-side
> SGMII auto-negotiation in SGMII mode, right?

You may notice in the emails I tend to send, I talk about Cisco SGMII,
particularly when I think that the recipient won't understand the
difference - there is a lot of crap in industry surrounding "SGMII"
where "SGMII" gets used for "it's a serial gigabit MII link" and then
they use it for 1000Base-X. It annoys me intensely that industry
constantly dilutes these terms in a confusing way, so we end up with
patches that talk about doing stuff with SGMII that are actually doing
stuff with 1000Base-X.

For example, recently I've seen patches that add support for a device
that can to 10GBASE-R and 1000BASE-X... and their function for
configuring 1000BASE-X was called something with "_sgmii_" in its name.

As far as I'm concerned, there's 1000Base-X which is the 802.3 protocol
and there is Cisco SGMII which is 1000Base-X taken by Cisco and
modified - and one shall not use SGMII to other than to refer to the
Cisco version, because otherwise there's way too much scope for
confusion and misunderstanding.

> > >  * drivers/net/phy/mxl-gpy.c
> > >    This goes through great lengths to switch on inband-an when initially
> > >    coming up in SGMII mode, then switches is off when switching to
> > >    2500Base-X mode and after that **never switches it on again**. This
> > >    is obviously not correct and the driver can be greatly reduced if
> > >    dropping all that (non-)broken logic.
> > >    Would a patch like [1] this be acceptable?
> 
> Did you take a look at the current implementation in mxl-gpy.c and
> patch [1]?
> 
> To me the current code looks obviously wrong and cannot work when
> switching from SGMII (with in-band-status, initialized by the
> bootloader) to 2500Base-X and then back to SGMII (which will then
> always be without in-band-status), so that to me look like a bug, and
> if something like that should work, the driver will need to remember
> the previous state of in-band-status for SGMII instead of relying on an
> already overwritten PHY register.

Why do you think it doesn't re-enable in-band AN?

gpy_update_interface() does this when called at various speeds:

if SPEED_2500, it clears VSPEC1_SGMII_CTRL_ANEN

if SPEED_1000, SPEED_100, or SPEED_10, it sets VSPEC1_SGMII_ANEN_ANRS
   and VSPEC1_SGMII_ANEN_ANRS is both the VSPEC1_SGMII_CTRL_ANEN and
   VSPEC1_SGMII_CTRL_ANRS bits.

So the situation you talk about, when switching to 2500base-X,
VSPEC1_SGMII_CTRL_ANEN will be cleared, but when switching back to
SGMII mode, VSPEC1_SGMII_CTRL_ANEN will be set again.

To be honest, when I was reviewing the patch adding this support back
in June 2021, that also got me, and I was wondering whether
VSPEC1_SGMII_CTRL_ANEN was being set afterwards... it's just the
macro naming makes it look like it doesn't. But VSPEC1_SGMII_ANEN_ANRS
contains both ANEN and ANRS bits.

> > The overall message in my reply is essentially one of caution - yes
> > we can make changes to how PHYs work, but we need to audit the MAC
> > drivers that the PHY is used with to try to cut down on unexpected
> > regressions.
> 
> How do I even know which MAC driver is using which PHY driver as
> the PHY is being probed using the PHY ID at run-time?

Sadly, that is the big problem. It's not possible to go through the
DTS files, because many don't list which PHY the board is using (as
we rely on reading it from the hardware.) So really we don't have
much to go on.

It's rather a difficult problem to solve that has crept up on the
effort to maintain code in this area.

> In git history there are some hints regarding this, but there are
> probably a lot of "hidden" users of a PHY driver which we don't
> even know about simply because the fact a certain PHY driver is
> going to be used isn't documented anywhere.

Quite.

> I'm afraid we will need some kind of feature flag to indicate that a
> MAC driver is known to behave according to convention with regards to
> SGMII in-band-status being switched off and only in that case have the
> PHY driver do the same, and otherwhise stick with the existing
> codepaths and potentially unknown hardware-default behavior
> (ie. another mess like the pre_march2020 situation...)

Yes. Thankfully, it's something that, provided the PCS implementations
are all the same, at least phylink users should be consistent and we
don't need another flag in pl->config to indicate anything. We just
tell phylib that we're phylink and be done with it.

For everything else, I think we just have to assume "let the PHY
driver do what it does today" as the safest course of action.

As for the pre_march2020 situation, we're down to just two drivers
that require that now:

1) mtk_eth_soc for its RGMII mode (which, honestly, I'm prepared to
   break at this point, because I do not believe there are *any* users
   out there - not only have my pleas for testers for that had no
   response, I believe the code in mk_eth_soc to be broken.)

   I am considering removing RGMII support there for implementations
   which have MTK_GMAC1_TRGMII but _not_ MTK_TRGMII_MT7621_CLK -
   basically the path that calls mtk_gmac0_rgmii_adjust(). I doubt
   anyone will complain because no one seems to be using it (or
   they are and they're ignoring my pleas for testers - in which
   case, being three years on, they honestly get what's coming, that
   being a regression or not.)

2) mv88e6xxx DSA support, which needs to be converted to phylink_pcs
   as previously stated.

I never thought it would take 3+ years to get drivers converted, but
sadly this shows how glacially slow progress can be in mainline, and
the more users phylink gets, the more of a problem this is likely to
become unless we have really good interfaces into code making use of
it.

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

Re: Convention regarding SGMII in-band autonegotiation

[Daniel Golle]

On Tue, Apr 04, 2023 at 03:46:41PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 04, 2023 at 12:56:40PM +0100, Daniel Golle wrote:
> [...]
> Why do you think it doesn't re-enable in-band AN?
> 
> gpy_update_interface() does this when called at various speeds:
> 
> if SPEED_2500, it clears VSPEC1_SGMII_CTRL_ANEN
> 
> if SPEED_1000, SPEED_100, or SPEED_10, it sets VSPEC1_SGMII_ANEN_ANRS
>    and VSPEC1_SGMII_ANEN_ANRS is both the VSPEC1_SGMII_CTRL_ANEN and
>    VSPEC1_SGMII_CTRL_ANRS bits.
> 
> So the situation you talk about, when switching to 2500base-X,
> VSPEC1_SGMII_CTRL_ANEN will be cleared, but when switching back to
> SGMII mode, VSPEC1_SGMII_CTRL_ANEN will be set again.
> 
> To be honest, when I was reviewing the patch adding this support back
> in June 2021, that also got me, and I was wondering whether
> VSPEC1_SGMII_CTRL_ANEN was being set afterwards... it's just the
> macro naming makes it look like it doesn't. But VSPEC1_SGMII_ANEN_ANRS
> contains both ANEN and ANRS bits.

Ok, I see it also now. So at least that works somehow.

Yet switching on Cisco SGMII in-band-status (which is what
VSPEC1_SGMII_CTRL_ANEN means) deliberately in a PHY driver which is
connected to a MAC looks wrong. As we are inside a PHY driver we
obviously have access to this PHY via MDIO and could just as well use
out-of-band status.

And it cannot work in this way with MAC drivers which are expected
to only use Cisco SGMII in-band-status in case of MLO_AN_INBAND being
set.

> [...]
> > I'm afraid we will need some kind of feature flag to indicate that a
> > MAC driver is known to behave according to convention with regards to
> > SGMII in-band-status being switched off and only in that case have the
> > PHY driver do the same, and otherwhise stick with the existing
> > codepaths and potentially unknown hardware-default behavior
> > (ie. another mess like the pre_march2020 situation...)
> 
> Yes. Thankfully, it's something that, provided the PCS implementations
> are all the same, at least phylink users should be consistent and we
> don't need another flag in pl->config to indicate anything. We just
> tell phylib that we're phylink and be done with it.

Ok, so this would work in both realtek and mxl-phy phy driver
.config_init function (pseudo-code):

if (phydev->phylink &&
    !phylink_autoneg_inband(phydev->phylink->cur_link_an_mode))
        switch_off_inband_status(phydev);

If that pattern looks good to you, I will start with patches for
mxl-gpy.c and then go on with realtek.c. In case of realtek.c we
probably should also make calling genphy_soft_reset in .soft_reset
conditional on !!phydev->phylink, and that could even become a
generic helper function, as probably many other phy drivers which
currently don't set .soft_reset to genphy_soft_reset would need
that in case phylink is being used.
Anyway, step by step...

> For everything else, I think we just have to assume "let the PHY
> driver do what it does today" as the safest course of action.
> 
> As for the pre_march2020 situation, we're down to just two drivers
> that require that now:
> 
> 1) mtk_eth_soc for its RGMII mode (which, honestly, I'm prepared to
>    break at this point, because I do not believe there are *any* users
>    out there - not only have my pleas for testers for that had no
>    response, I believe the code in mk_eth_soc to be broken.)
> 
>    I am considering removing RGMII support there for implementations
>    which have MTK_GMAC1_TRGMII but _not_ MTK_TRGMII_MT7621_CLK -
>    basically the path that calls mtk_gmac0_rgmii_adjust(). I doubt
>    anyone will complain because no one seems to be using it (or
>    they are and they're ignoring my pleas for testers - in which
>    case, being three years on, they honestly get what's coming, that
>    being a regression or not.)

So that would affect MT7623, I do have this hardware for testing, the
BPi-R2 and also Frank certainly has it flying around and would probably
be available to help testing.

I may have missed your call for help there, I can test anything you
want on the BPi-R2 board with MT7623N + MT7530:
[    2.061063] mt7530 mdio-bus:00: configuring for fixed/trgmii link mode
...
[    4.939408] mtk_soc_eth 1b100000.ethernet eth0: configuring for fixed/trgmii link mode

Is that the kind of board you'd be looking for?

For the fun of it we can of course try to also change the speed of
MT7530 CPU port to something else than 1000M...

Please point me to patches to test, I can get started right away.

> 
> 2) mv88e6xxx DSA support, which needs to be converted to phylink_pcs
>    as previously stated.
> 
> I never thought it would take 3+ years to get drivers converted, but
> sadly this shows how glacially slow progress can be in mainline, and
> the more users phylink gets, the more of a problem this is likely to
> become unless we have really good interfaces into code making use of
> it.

Re: Convention regarding SGMII in-band autonegotiation

[Daniel Golle]

Hi Russell,

On Tue, Apr 04, 2023 at 08:05:32PM +0100, Daniel Golle wrote:
> On Tue, Apr 04, 2023 at 03:46:41PM +0100, Russell King (Oracle) wrote:
> > On Tue, Apr 04, 2023 at 12:56:40PM +0100, Daniel Golle wrote:
> > [...]
> > Why do you think it doesn't re-enable in-band AN?
> > 
> > gpy_update_interface() does this when called at various speeds:
> > 
> > if SPEED_2500, it clears VSPEC1_SGMII_CTRL_ANEN
> > 
> > if SPEED_1000, SPEED_100, or SPEED_10, it sets VSPEC1_SGMII_ANEN_ANRS
> >    and VSPEC1_SGMII_ANEN_ANRS is both the VSPEC1_SGMII_CTRL_ANEN and
> >    VSPEC1_SGMII_CTRL_ANRS bits.
> > 
> > So the situation you talk about, when switching to 2500base-X,
> > VSPEC1_SGMII_CTRL_ANEN will be cleared, but when switching back to
> > SGMII mode, VSPEC1_SGMII_CTRL_ANEN will be set again.
> > 
> > To be honest, when I was reviewing the patch adding this support back
> > in June 2021, that also got me, and I was wondering whether
> > VSPEC1_SGMII_CTRL_ANEN was being set afterwards... it's just the
> > macro naming makes it look like it doesn't. But VSPEC1_SGMII_ANEN_ANRS
> > contains both ANEN and ANRS bits.
> 
> Ok, I see it also now. So at least that works somehow.
> 
> Yet switching on Cisco SGMII in-band-status (which is what
> VSPEC1_SGMII_CTRL_ANEN means) deliberately in a PHY driver which is
> connected to a MAC looks wrong. As we are inside a PHY driver we
> obviously have access to this PHY via MDIO and could just as well use
> out-of-band status.
> 
> And it cannot work in this way with MAC drivers which are expected
> to only use Cisco SGMII in-band-status in case of MLO_AN_INBAND being
> set.
> 
> > [...]
> > > I'm afraid we will need some kind of feature flag to indicate that a
> > > MAC driver is known to behave according to convention with regards to
> > > SGMII in-band-status being switched off and only in that case have the
> > > PHY driver do the same, and otherwhise stick with the existing
> > > codepaths and potentially unknown hardware-default behavior
> > > (ie. another mess like the pre_march2020 situation...)
> > 
> > Yes. Thankfully, it's something that, provided the PCS implementations
> > are all the same, at least phylink users should be consistent and we
> > don't need another flag in pl->config to indicate anything. We just
> > tell phylib that we're phylink and be done with it.
> 
> Ok, so this would work in both realtek and mxl-phy phy driver
> .config_init function (pseudo-code):
> 
> if (phydev->phylink &&
>     !phylink_autoneg_inband(phydev->phylink->cur_link_an_mode))
>         switch_off_inband_status(phydev);
> 

Ok, so obviously that was a bit naive.

phydev->phylink has not yet been populated at the time when
.config_init is called. So using !!phydev->phylink as a flag to
indicate that "we're phylink" may work later on, like in .read_status,
.config_aneg or .link_change_notify. But it won't work for .soft_reset
and .config_init because both are called before phydev->phylink has
been set.

Hence going down this path will be sufficient to allow mxl-gpy.c to
switch off SGMII in-band-status in case phylink is being used (see RFC
patch below).

However, it won't be sufficient to decide whether or not to reset an
RTL822x 2.5G PHY or if rate-adaption should be switched off in
.config_init. Unfortunately, without having called genphy_soft_reset
the current .read_status implementation won't work properly in case
vendor bootloader had previously re-programmed the RTL8221B PHY...

Please advise if the patch below would be acceptable:

From 83f55b669300ca641f5f9397f30578a7013b0d7a Mon Sep 17 00:00:00 2001
From: Daniel Golle <daniel@makrotopia.org>
Date: Thu, 6 Apr 2023 23:36:50 +0100
Subject: [PATCH RFC] net: phy: mxl-gpy: don't use SGMII AN if using phylink

MAC drivers using phylink expect SGMII in-band-status to be switched off
when attached to a PHY. Make sure this is the case also for mxl-gpy which
keeps SGMII in-band-status in case of SGMII interface mode is used.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/mxl-gpy.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 8e6bb97b5f85c..0544b0d5b0f75 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -355,8 +355,11 @@ static bool gpy_2500basex_chk(struct phy_device *phydev)
 
 	phydev->speed = SPEED_2500;
 	phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
-	phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
-		       VSPEC1_SGMII_CTRL_ANEN, 0);
+
+	if (!phydev->phylink)
+		phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+			       VSPEC1_SGMII_CTRL_ANEN, 0);
+
 	return true;
 }
 
@@ -407,6 +410,14 @@ static int gpy_config_aneg(struct phy_device *phydev)
 	u32 adv;
 	int ret;
 
+	/* Disable SGMII auto-negotiation if using phylink */
+	if (phydev->phylink) {
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
+				     VSPEC1_SGMII_CTRL_ANEN, 0);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (phydev->autoneg == AUTONEG_DISABLE) {
 		/* Configure half duplex with genphy_setup_forced,
 		 * because genphy_c45_pma_setup_forced does not support.
@@ -529,6 +540,8 @@ static int gpy_update_interface(struct phy_device *phydev)
 	switch (phydev->speed) {
 	case SPEED_2500:
 		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		if (phydev->phylink)
+			break;
 		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_SGMII_CTRL,
 				     VSPEC1_SGMII_CTRL_ANEN, 0);
 		if (ret < 0) {
@@ -542,7 +555,7 @@ static int gpy_update_interface(struct phy_device *phydev)
 	case SPEED_100:
 	case SPEED_10:
 		phydev->interface = PHY_INTERFACE_MODE_SGMII;
-		if (gpy_sgmii_aneg_en(phydev))
+		if (phydev->phylink || gpy_sgmii_aneg_en(phydev))
 			break;
 		/* Enable and restart SGMII ANEG for 10/100/1000Mbps link speed
 		 * if ANEG is disabled (in 2500-BaseX mode).
-- 
2.40.0