[PATCH net-next 0/2] Let phylink manage in-band AN for the PHY

[PATCH net-next 0/2] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This small series creates a configuration knob for PHY drivers which use
serial MII-side interfaces and support clause 37 in-band auto-negotiation
there.

Vladimir Oltean (2):
  net: phylink: explicitly configure in-band autoneg for PHYs that
    support it
  net: phy: mscc: configure in-band auto-negotiation for VSC8514

 drivers/net/phy/mscc/mscc.h      |  2 ++
 drivers/net/phy/mscc/mscc_main.c | 13 +++++++++++++
 drivers/net/phy/phy.c            | 12 ++++++++++++
 drivers/net/phy/phylink.c        |  8 ++++++++
 include/linux/phy.h              |  8 ++++++++
 5 files changed, 43 insertions(+)

-- 
2.25.1

[PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently Linux has no control over whether a MAC-to-PHY interface uses
in-band signaling or not, even though phylink has the
	managed = "in-band-status";
property which denotes that the MAC expects in-band signaling to be used.

The problem is really that if the in-band signaling is configurable in
both the PHY and the MAC, there is a risk that they are out of sync
unless phylink manages them both. Most if not all in-band autoneg state
machines follow IEEE 802.3 clause 37, which means that they will not
change the operating mode of the SERDES lane from control to data mode
unless in-band AN completed successfully. Therefore traffic will not
work.

It is particularly unpleasant that currently, we assume that PHYs which
have configurable in-band AN come pre-configured from a prior boot stage
such as U-Boot, because once the bootloader changes, all bets are off.

Let's introduce a new PHY driver method for configuring in-band autoneg,
and make phylink be its first user. The main PHY library does not call
phy_config_inband_autoneg, because it does not know what to configure it
to. Presumably, non-phylink drivers can also call phy_config_inband_autoneg
individually.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phy.c     | 12 ++++++++++++
 drivers/net/phy/phylink.c |  8 ++++++++
 include/linux/phy.h       |  8 ++++++++
 3 files changed, 28 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fdb914b5b857..d6c63c54943e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -748,6 +748,18 @@ static int phy_check_link_status(struct phy_device *phydev)
 	return 0;
 }
 
+int phy_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (!phydev->drv->config_inband_aneg)
+		return -EOPNOTSUPP;
+
+	return phydev->drv->config_inband_aneg(phydev, enabled);
+}
+EXPORT_SYMBOL(phy_config_inband_aneg);
+
 /**
  * phy_start_aneg - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 84f6e197f965..ef3e947d5019 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -978,6 +978,14 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		return ret;
 	}
 
+	ret = phy_config_inband_aneg(phy,
+				     (pl->cur_link_an_mode == MLO_AN_INBAND));
+	if (ret && ret != -EOPNOTSUPP) {
+		phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
+			     ret);
+		return ret;
+	}
+
 	phy->phylink = pl;
 	phy->phy_link_change = phylink_phy_change;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c130788306c8..2260b512ffbf 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -746,6 +746,13 @@ struct phy_driver {
 	 */
 	int (*config_aneg)(struct phy_device *phydev);
 
+	/**
+	 * @config_inband_aneg: Enable or disable in-band auto-negotiation for
+	 * the system-side interface if the PHY operates in a mode that
+	 * requires it: (Q)SGMII, USXGMII, 1000Base-X, etc.
+	 */
+	int (*config_inband_aneg)(struct phy_device *phydev, bool enabled);
+
 	/** @aneg_done: Determines the auto negotiation result */
 	int (*aneg_done)(struct phy_device *phydev);
 
@@ -1394,6 +1401,7 @@ void phy_detach(struct phy_device *phydev);
 void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
+int phy_config_inband_aneg(struct phy_device *phydev, bool enabled);
 int phy_aneg_done(struct phy_device *phydev);
 int phy_speed_down(struct phy_device *phydev, bool sync);
 int phy_speed_up(struct phy_device *phydev);
-- 
2.25.1

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Michael Walle]

Am 2021-02-12 18:23, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Currently Linux has no control over whether a MAC-to-PHY interface uses
> in-band signaling or not, even though phylink has the
> 	managed = "in-band-status";
> property which denotes that the MAC expects in-band signaling to be 
> used.
> 
> The problem is really that if the in-band signaling is configurable in
> both the PHY and the MAC, there is a risk that they are out of sync
> unless phylink manages them both. Most if not all in-band autoneg state
> machines follow IEEE 802.3 clause 37, which means that they will not
> change the operating mode of the SERDES lane from control to data mode
> unless in-band AN completed successfully. Therefore traffic will not
> work.
> 
> It is particularly unpleasant that currently, we assume that PHYs which
> have configurable in-band AN come pre-configured from a prior boot 
> stage
> such as U-Boot, because once the bootloader changes, all bets are off.

Fun fact, now it may be the other way around. If the bootloader doesn't
configure it and the PHY isn't reset by the hardware, it won't work in
the bootloader after a reboot ;)

> Let's introduce a new PHY driver method for configuring in-band 
> autoneg,
> and make phylink be its first user. The main PHY library does not call
> phy_config_inband_autoneg, because it does not know what to configure 
> it
> to. Presumably, non-phylink drivers can also call 
> phy_config_inband_autoneg
> individually.

If you disable aneg between MAC and PHY, what would be the actual speed
setting/duplex mode then? I guess it have to match the external speed?

I'm trying this on the AT8031. I've removed 'managed = 
"in-band-status";'
for the PHY. Confirmed that it won't work and then I've implemented your
new callback. That will disable the SGMII aneg (which is done via the
BMCR of fiber page if I'm not entirely mistaken); ethernet will then
work again. But only for gigabit. I presume because the speed setting
of the SGMII link is set to gigabit.

-michael

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Russell King - ARM Linux admin]

On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote:
> Fun fact, now it may be the other way around. If the bootloader doesn't
> configure it and the PHY isn't reset by the hardware, it won't work in
> the bootloader after a reboot ;)

If we start messing around with the configuration of PHYs in that
regard, we could be opening ourselves up for a world of pain...

> If you disable aneg between MAC and PHY, what would be the actual speed
> setting/duplex mode then? I guess it have to match the external speed?

That is a function of the interface mode and the PHY capabilities.

1) if the PHY supports rate adaption, and is programmed for that, then
   the PHY link normally operates at a fixed speed (e.g. 1G for SGMII)
   and the PHY converts to the appropriate speed.

   We don't actually support this per se, since the parameters we give
   to the MAC via mac_link_up() are the media side parameters, not the
   link parameters.

2) if the PHY does not support rate adaption, then the MAC to PHY link
   needs to follow the media speed and duplex. phylink will be in "PHY"
   mode, where it passes the media side negotiation results to the MAC
   just like phylib would, and the MAC should be programmed
   appropriately. In the case of a SGMII link, the link needs to be
   programmed to do the appropriate symbol repetition for 100M and 10M
   speeds. The PHY /should/ do that automatically, but if it doesn't,
   then the PHY also needs to be programmed to conform. (since if
   there's no rate adaption in the PHY, the MAC side and the media side
   must match.)

Hope that helps.

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

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Michael Walle]

Am 2021-02-13 01:18, schrieb Russell King - ARM Linux admin:
> On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote:
>> Fun fact, now it may be the other way around. If the bootloader 
>> doesn't
>> configure it and the PHY isn't reset by the hardware, it won't work in
>> the bootloader after a reboot ;)
> 
> If we start messing around with the configuration of PHYs in that
> regard, we could be opening ourselves up for a world of pain...
> 
>> If you disable aneg between MAC and PHY, what would be the actual 
>> speed
>> setting/duplex mode then? I guess it have to match the external speed?
> 
> That is a function of the interface mode and the PHY capabilities.
> 
> 1) if the PHY supports rate adaption, and is programmed for that, then
>    the PHY link normally operates at a fixed speed (e.g. 1G for SGMII)
>    and the PHY converts to the appropriate speed.
> 
>    We don't actually support this per se, since the parameters we give
>    to the MAC via mac_link_up() are the media side parameters, not the
>    link parameters.
> 
> 2) if the PHY does not support rate adaption, then the MAC to PHY link
>    needs to follow the media speed and duplex. phylink will be in "PHY"
>    mode, where it passes the media side negotiation results to the MAC
>    just like phylib would, and the MAC should be programmed
>    appropriately. In the case of a SGMII link, the link needs to be
>    programmed to do the appropriate symbol repetition for 100M and 10M
>    speeds. The PHY /should/ do that automatically, but if it doesn't,
>    then the PHY also needs to be programmed to conform. (since if
>    there's no rate adaption in the PHY, the MAC side and the media side
>    must match.)

Thanks, but I'm not sure I understand the difference between "rate
adaption" and symbol repetition. The SGMII link is always 1.25Gb,
right? If the media side is 100Mbit it will repeat the symbol 10
times or 100 times in case of 10Mbit. What is "rate adaption" then?

-- 
-michael

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Andrew Lunn]

> Thanks, but I'm not sure I understand the difference between "rate
> adaption" and symbol repetition. The SGMII link is always 1.25Gb,
> right? If the media side is 100Mbit it will repeat the symbol 10
> times or 100 times in case of 10Mbit. What is "rate adaption" then?

Hi Michael

Some multiG PHYs fix their host side interface to say 10Gbps,
independent of what the media side is doing. The PHY adapts the 10Gbps
stream it gets from the host down to 10Mbps etc as needed, dropping
frames if its internal buffers overflow.

       Andrew

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Russell King - ARM Linux admin]

On Sat, Feb 13, 2021 at 05:41:55PM +0100, Michael Walle wrote:
> Am 2021-02-13 01:18, schrieb Russell King - ARM Linux admin:
> > That is a function of the interface mode and the PHY capabilities.
> > 
> > 1) if the PHY supports rate adaption, and is programmed for that, then
> >    the PHY link normally operates at a fixed speed (e.g. 1G for SGMII)
> >    and the PHY converts to the appropriate speed.
> > 
> >    We don't actually support this per se, since the parameters we give
> >    to the MAC via mac_link_up() are the media side parameters, not the
> >    link parameters.
> > 
> > 2) if the PHY does not support rate adaption, then the MAC to PHY link
> >    needs to follow the media speed and duplex. phylink will be in "PHY"
> >    mode, where it passes the media side negotiation results to the MAC
> >    just like phylib would, and the MAC should be programmed
> >    appropriately. In the case of a SGMII link, the link needs to be
> >    programmed to do the appropriate symbol repetition for 100M and 10M
> >    speeds. The PHY /should/ do that automatically, but if it doesn't,
> >    then the PHY also needs to be programmed to conform. (since if
> >    there's no rate adaption in the PHY, the MAC side and the media side
> >    must match.)
> 
> Thanks, but I'm not sure I understand the difference between "rate
> adaption" and symbol repetition. The SGMII link is always 1.25Gb,
> right? If the media side is 100Mbit it will repeat the symbol 10
> times or 100 times in case of 10Mbit. What is "rate adaption" then?

You are correct about SGMII.

Bear in mind that there is very little difference between SGMII and
1000base-X when you don't have AN. SGMII can be forced to do the
symbol repetition.  Symbol repetition does not exist in 1000base-X.

Some PHYs, particularly 10G multi-speed PHYs, have a "rate adaption"
block that can be used when the media side is operating at slower-
than-10G speed when the media side is fixed at 10G. What this means
is the MAC side always operates at full 10G speed, but the PHY
queues packets for transmission at the media rate. Received packets
are received by the PHY and then forwarded to the MAC at 10G speed.

To add to that, some PHYs have the ability to send pause frames
when their internal buffer fills up. Others stipulate that the MAC
needs to rate-limit the transmitted packets to match the media side
speed.

Whether such a feature exists in 1G PHYs is unknown. If a PHY is
capable of 1G/100M/10M speeds, but if SGMII AN is disabled and
there is no way to force the symbol repetition, the SGMII side
will be fixed at 1G speed. The only way this can work with a
slower media speed is if the PHY performs rate adaption internally,
converting between the 1G speed on the host side and whatever speed
the media side is operating.

So, for example, if the PHY was to have rate adaption, the media
side would operate at 100M but the MAC side would continue operating
at 1G speed without symbol repetition.

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

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Vladimir Oltean]

On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote:
> Am 2021-02-12 18:23, schrieb Vladimir Oltean:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > Currently Linux has no control over whether a MAC-to-PHY interface uses
> > in-band signaling or not, even though phylink has the
> > 	managed = "in-band-status";
> > property which denotes that the MAC expects in-band signaling to be
> > used.
> > 
> > The problem is really that if the in-band signaling is configurable in
> > both the PHY and the MAC, there is a risk that they are out of sync
> > unless phylink manages them both. Most if not all in-band autoneg state
> > machines follow IEEE 802.3 clause 37, which means that they will not
> > change the operating mode of the SERDES lane from control to data mode
> > unless in-band AN completed successfully. Therefore traffic will not
> > work.
> > 
> > It is particularly unpleasant that currently, we assume that PHYs which
> > have configurable in-band AN come pre-configured from a prior boot stage
> > such as U-Boot, because once the bootloader changes, all bets are off.
> 
> Fun fact, now it may be the other way around. If the bootloader doesn't
> configure it and the PHY isn't reset by the hardware, it won't work in
> the bootloader after a reboot ;)

My understanding is that this is precisely the reason why the U-Boot
people don't want to support booting from RAM, and want to assume that
the nothing else ran between Power On Reset and the bootloader:
https://www.denx.de/wiki/view/DULG/CanUBootBeConfiguredSuchThatItCanBeStartedInRAM
[ that does make me wonder what they think about ARM TF-A ]

> > Let's introduce a new PHY driver method for configuring in-band autoneg,
> > and make phylink be its first user. The main PHY library does not call
> > phy_config_inband_autoneg, because it does not know what to configure it
> > to. Presumably, non-phylink drivers can also call
> > phy_config_inband_autoneg
> > individually.
> 
> If you disable aneg between MAC and PHY, what would be the actual speed
> setting/duplex mode then? I guess it have to match the external speed?
> 
> I'm trying this on the AT8031. I've removed 'managed = "in-band-status";'
> for the PHY. Confirmed that it won't work and then I've implemented your
> new callback. That will disable the SGMII aneg (which is done via the
> BMCR of fiber page if I'm not entirely mistaken); ethernet will then
> work again. But only for gigabit. I presume because the speed setting
> of the SGMII link is set to gigabit.

Which MAC driver are you testing on? Are you saying that it doesn't
force the link to the speed resolved over MDIO and passed to
.phylink_mac_link_up, or that the speed communicated to it is incorrect?

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Michael Walle]

Am 2021-02-13 01:36, schrieb Vladimir Oltean:
> On Fri, Feb 12, 2021 at 11:40:59PM +0100, Michael Walle wrote:
>> Am 2021-02-12 18:23, schrieb Vladimir Oltean:
>> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
>> >
>> > Currently Linux has no control over whether a MAC-to-PHY interface uses
>> > in-band signaling or not, even though phylink has the
>> > 	managed = "in-band-status";
>> > property which denotes that the MAC expects in-band signaling to be
>> > used.
>> >
>> > The problem is really that if the in-band signaling is configurable in
>> > both the PHY and the MAC, there is a risk that they are out of sync
>> > unless phylink manages them both. Most if not all in-band autoneg state
>> > machines follow IEEE 802.3 clause 37, which means that they will not
>> > change the operating mode of the SERDES lane from control to data mode
>> > unless in-band AN completed successfully. Therefore traffic will not
>> > work.
>> >
>> > It is particularly unpleasant that currently, we assume that PHYs which
>> > have configurable in-band AN come pre-configured from a prior boot stage
>> > such as U-Boot, because once the bootloader changes, all bets are off.
>> 
>> Fun fact, now it may be the other way around. If the bootloader 
>> doesn't
>> configure it and the PHY isn't reset by the hardware, it won't work in
>> the bootloader after a reboot ;)
> 
> My understanding is that this is precisely the reason why the U-Boot
> people don't want to support booting from RAM, and want to assume that
> the nothing else ran between Power On Reset and the bootloader:
> https://www.denx.de/wiki/view/DULG/CanUBootBeConfiguredSuchThatItCanBeStartedInRAM
> [ that does make me wonder what they think about ARM TF-A ]

It isn't that easy sometimes. Eg. there might be boards without a proper
reset of the peripherals, maybe the SoC will just generate an internal
reset, whatever. One might conisder that a broken board. But, for
example, on the kontron sl28, we deliberatly chose not to do a PHY
reset (well it is actually configurable) because this will also prevent
WoL by the PHY.

>> > Let's introduce a new PHY driver method for configuring in-band autoneg,
>> > and make phylink be its first user. The main PHY library does not call
>> > phy_config_inband_autoneg, because it does not know what to configure it
>> > to. Presumably, non-phylink drivers can also call
>> > phy_config_inband_autoneg
>> > individually.
>> 
>> If you disable aneg between MAC and PHY, what would be the actual 
>> speed
>> setting/duplex mode then? I guess it have to match the external speed?
>> 
>> I'm trying this on the AT8031. I've removed 'managed = 
>> "in-band-status";'
>> for the PHY. Confirmed that it won't work and then I've implemented 
>> your
>> new callback. That will disable the SGMII aneg (which is done via the
>> BMCR of fiber page if I'm not entirely mistaken); ethernet will then
>> work again. But only for gigabit. I presume because the speed setting
>> of the SGMII link is set to gigabit.
> 
> Which MAC driver are you testing on?

enetc

> Are you saying that it doesn't
> force the link to the speed resolved over MDIO and passed to
> .phylink_mac_link_up, or that the speed communicated to it is 
> incorrect?

That seem to work:

[ 5313.852406] fsl_enetc 0000:00:00.0 gbe0: phy link down 
sgmii/Unknown/Unknown
[ 5313.852414] fsl_enetc 0000:00:00.0 gbe0: Link is Down
[ 5315.900687] fsl_enetc 0000:00:00.0 gbe0: phy link up 
sgmii/100Mbps/Full
[ 5315.916816] fsl_enetc 0000:00:00.0 gbe0: Link is Up - 100Mbps/Full - 
flow control rx/tx

But the Atheros PHY seems to have a problem with the SGMII link
if there is no autoneg.
No matter what I do, I can't get any traffic though if its not
gigabit on the copper side. Unfortunately, I don't have access
to an oscilloscope right now to see whats going on on the SGMII
link.

-michael

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Michael Walle]

Am 2021-02-13 17:53, schrieb Michael Walle:
> Am 2021-02-13 01:36, schrieb Vladimir Oltean:
> But the Atheros PHY seems to have a problem with the SGMII link
> if there is no autoneg.
> No matter what I do, I can't get any traffic though if its not
> gigabit on the copper side. Unfortunately, I don't have access
> to an oscilloscope right now to see whats going on on the SGMII
> link.

Scrap that. It will work if I set the speed/duplex mode in BMCR
correctly. (I tried that before, but I shifted one bit. doh).

So that will work, but when will it be done? There is no
callback to configure the PCS side of the PHY if a link up is
detected.

-michael

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Vladimir Oltean]

On Sat, Feb 13, 2021 at 06:09:13PM +0100, Michael Walle wrote:
> Am 2021-02-13 17:53, schrieb Michael Walle:
> > Am 2021-02-13 01:36, schrieb Vladimir Oltean:
> > But the Atheros PHY seems to have a problem with the SGMII link
> > if there is no autoneg.
> > No matter what I do, I can't get any traffic though if its not
> > gigabit on the copper side. Unfortunately, I don't have access
> > to an oscilloscope right now to see whats going on on the SGMII
> > link.
> 
> Scrap that. It will work if I set the speed/duplex mode in BMCR
> correctly. (I tried that before, but I shifted one bit. doh).
> 
> So that will work, but when will it be done? There is no
> callback to configure the PCS side of the PHY if a link up is
> detected.

That's interesting/odd, on VSC8514 there is no need to force the speed
of the system side to what was negotiated on media side. I took a quick
look through the AR8033 datasheet and there isn't any mentioning the
ability to program the SGMII link according to internal state as opposed
to register settings, but it's equally possible that I'm simply not
seeing it.

On the other hand, I never meant for the inband autoneg setting to only
be configurable both ways. I expect some PHYs are not able to operate
using noinband mode, and for those I guess you should simply return
-EINVAL, allowing the system designer to know that the configuration
will not work and why.

I think you could hook into .config_aneg_done, for the autoneg=true
case, and into .config_aneg for autoneg=false (I'm talking about autoneg
on media side here), but honestly I think the PHY is pretty broken for
requiring external coordination between the clause 28 and the clause 37
PCS. So unless there is a real need to configure noinband mode, I would
probably not bother.

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Michael Walle]

Am 2021-02-13 19:56, schrieb Vladimir Oltean:
> On Sat, Feb 13, 2021 at 06:09:13PM +0100, Michael Walle wrote:
>> Am 2021-02-13 17:53, schrieb Michael Walle:
>> > Am 2021-02-13 01:36, schrieb Vladimir Oltean:
>> > But the Atheros PHY seems to have a problem with the SGMII link
>> > if there is no autoneg.
>> > No matter what I do, I can't get any traffic though if its not
>> > gigabit on the copper side. Unfortunately, I don't have access
>> > to an oscilloscope right now to see whats going on on the SGMII
>> > link.
>> 
>> Scrap that. It will work if I set the speed/duplex mode in BMCR
>> correctly. (I tried that before, but I shifted one bit. doh).
>> 
>> So that will work, but when will it be done? There is no
>> callback to configure the PCS side of the PHY if a link up is
>> detected.
> 
> That's interesting/odd, on VSC8514 there is no need to force the speed
> of the system side to what was negotiated on media side. I took a quick
> look through the AR8033 datasheet and there isn't any mentioning the
> ability to program the SGMII link according to internal state as 
> opposed
> to register settings, but it's equally possible that I'm simply not
> seeing it.

I couldn't find anything in the AR8031 datasheet either.

> On the other hand, I never meant for the inband autoneg setting to only
> be configurable both ways.

Then why is there a "bool enabled"?

> I expect some PHYs are not able to operate
> using noinband mode, and for those I guess you should simply return
> -EINVAL, allowing the system designer to know that the configuration
> will not work and why.

You mean like this:

static int at803x_config_inband_aneg(struct phy_device *phydev, bool 
enabled)
{
	if (!enabled)
		return -EINVAL;
	/* enable SGMII autoneg */
	return phy_write_paged(...);
}

But then why bother with config_inband_aneg() at all and just enable
it unconditionally in config_init(). [and maybe keep the return 
-EINVAL].
Which then begs the question, does it makes sense on (Q)SGMII links at
all?

> I think you could hook into .config_aneg_done, for the autoneg=true
> case, and into .config_aneg for autoneg=false (I'm talking about 
> autoneg
> on media side here), but honestly I think the PHY is pretty broken for
> requiring external coordination between the clause 28 and the clause 37
> PCS. So unless there is a real need to configure noinband mode, I would
> probably not bother.

Probably not.

-michael

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Vladimir Oltean]

On Sat, Feb 13, 2021 at 08:57:46PM +0100, Michael Walle wrote:
> > On the other hand, I never meant for the inband autoneg setting to only
> > be configurable both ways.
>
> Then why is there a "bool enabled"?

Let me stress the word _only_ both ways. The whole point of the "bool
enabled" is to attempt coordination with the 'managed = "in-band-status"'
property device tree property of the MAC, or to error out if it is not
possible.

> > I expect some PHYs are not able to operate
> > using noinband mode, and for those I guess you should simply return
> > -EINVAL, allowing the system designer to know that the configuration
> > will not work and why.
>
> You mean like this:
>
> static int at803x_config_inband_aneg(struct phy_device *phydev, bool
> enabled)
> {
> 	if (!enabled)
> 		return -EINVAL;
> 	/* enable SGMII autoneg */
> 	return phy_write_paged(...);
> }
>
> But then why bother with config_inband_aneg() at all and just enable
> it unconditionally in config_init(). [and maybe keep the return -EINVAL].

Because .config_init() is generic code, while .config_inband_autoneg()
is phylink-specific. Generally I don't want to make any assumption about
the state in which a PHY driver used to operate prior to this series.
If you are sure that at803x.c user relies on a prior bootloader stage
having disabled in-band AN, then sure, I suppose you can enable it
unconditionally in .config_init().

For VSC8514 I put the configuration deliberately in a phylink-specific
callback since I trust that at least the MAC-side drivers were reviewed
for proper use of MLO_AN_PHY vs MLO_AN_INBAND.

> Which then begs the question, does it makes sense on (Q)SGMII links at
> all?

See above. In the general case we need to assume a wild world where the
same PHY driver operates as inband on some platforms and noinband on
others (and most importantly, it works on both, which we'd like to
preserve at least). I would be glad if we didn't need to make that
assumption though.

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Russell King - ARM Linux admin]

On Sat, Feb 13, 2021 at 08:57:46PM +0100, Michael Walle wrote:
> But then why bother with config_inband_aneg() at all and just enable
> it unconditionally in config_init(). [and maybe keep the return -EINVAL].
> Which then begs the question, does it makes sense on (Q)SGMII links at
> all?

There are three cases. There are PHYs which operate in SGMII mode:
1) with in-band signalling enabled or disabled.
2) only with in-band signalling enabled.
3) with no in-band signalling capability.

Most Marvell PHYs can be configured and fall into class (1), although
changing that configuration is disruptive to the entire PHY. There is
at least one Broadcom PHY we support that falls into class (3) and it
appears on SFP modules. It sounds like AR803x fall into class (2).

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

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Russell King - ARM Linux admin]

On Fri, Feb 12, 2021 at 07:23:40PM +0200, Vladimir Oltean wrote:
> +	ret = phy_config_inband_aneg(phy,
> +				     (pl->cur_link_an_mode == MLO_AN_INBAND));

Please use phylink_autoneg_inband(pl->cur_link_an_mode) here.

> +	if (ret && ret != -EOPNOTSUPP) {
> +		phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
> +			     ret);

Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value.

As mentioned in this thread, we have at least one PHY which is unable
to provide the inband signalling in any mode (BCM84881). Currently,
phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
adjusts not to use inband mode. This would need to be addressed if we
are creating an alterative way to discover whether the PHY supports
inband mode or not.

Also, there needs to be consideration of PHYs that dynamically change
their interface type, and whether they support inband signalling.
For example, a PHY may support a mode where it dynamically selects
between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
mode may have inband signalling enabled or disabled. This is not a
theoretical case; we have a PHY like that supported in the kernel and
boards use it. What would the semantics of your new call be for a PHY
that performs this?

Should we also have a phydev->inband tristate, taking values "unknown,
enabled, disabled" which the PHY driver is required to update in their
read_status callback if they dynamically change their interface type?
(Although then phylink will need to figure out how to deal with that.)

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

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Vladimir Oltean]

On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote:
> > +	if (ret && ret != -EOPNOTSUPP) {
> > +		phylink_warn(pl, "failed to configure PHY in-band autoneg: %d\n",
> > +			     ret);
> 
> Please use %pe and ERR_PTR(ret) so we can get a symbolic errno value.

I didn't know that was possible, thanks for the hint.

> As mentioned in this thread, we have at least one PHY which is unable
> to provide the inband signalling in any mode (BCM84881). Currently,
> phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
> adjusts not to use inband mode. This would need to be addressed if we
> are creating an alterative way to discover whether the PHY supports
> inband mode or not.

So I haven't studied the SFP code path too deeply, but I think part of
the issue is the order in which things are done. It's almost as if there
should be a validation phase for PHY inband abilities too.

phylink_sfp_connect_phy
-> phylink_sfp_config:
   -> first this checks if phylink_phy_no_inband
   -> then this changes pl->link_config.interface and pl->cur_link_an_mode
-> phylink_bringup_phy:
   -> this is where I'm adding the new phy_config_inband_aneg function

If we were to use only my phy_config_inband_aneg function, it would need
to be moved upwards in the code path, to be precise where phylink_phy_no_inband
currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback
to MLO_AN_PHY if that fails. I think this would unnecessarily complicate
the code.

Alternatively, I could create a second PHY driver method, simply for
validation of supported inband modes. The validation can be done in
place of the current phylink_phy_noinband(), and I can still have the
phy_config_inband_aneg() where I put it now, since we shouldn't have any
surprises w.r.t. supported operating mode, and there should be no reason
to repeat the attempt as there would be with a single PHY driver method.
Thoughts?

> Also, there needs to be consideration of PHYs that dynamically change
> their interface type, and whether they support inband signalling.
> For example, a PHY may support a mode where it dynamically selects
> between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
> mode may have inband signalling enabled or disabled. This is not a
> theoretical case; we have a PHY like that supported in the kernel and
> boards use it. What would the semantics of your new call be for a PHY
> that performs this?
> 
> Should we also have a phydev->inband tristate, taking values "unknown,
> enabled, disabled" which the PHY driver is required to update in their
> read_status callback if they dynamically change their interface type?
> (Although then phylink will need to figure out how to deal with that.)

I don't have such PHY to test with, but I think the easiest way would be
to just call the validation method again, after we change the
phydev->interface value. The PHY driver can easily take phydev->interface
into consideration when answering the question "is inband aneg supported
or not". I don't think that making phydev->inband a stateful value is
going to be as useful as making it a function, since as you say, we will
be required to keep it up to date from generic PHY driver methods, but
only phylink will use it.

Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band autoneg for PHYs that support it

[Russell King - ARM Linux admin]

On Sun, Feb 14, 2021 at 01:10:14PM +0200, Vladimir Oltean wrote:
> On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote:
> > As mentioned in this thread, we have at least one PHY which is unable
> > to provide the inband signalling in any mode (BCM84881). Currently,
> > phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
> > adjusts not to use inband mode. This would need to be addressed if we
> > are creating an alterative way to discover whether the PHY supports
> > inband mode or not.
> 
> So I haven't studied the SFP code path too deeply, but I think part of
> the issue is the order in which things are done. It's almost as if there
> should be a validation phase for PHY inband abilities too.
> 
> phylink_sfp_connect_phy
> -> phylink_sfp_config:
>    -> first this checks if phylink_phy_no_inband
>    -> then this changes pl->link_config.interface and pl->cur_link_an_mode
> -> phylink_bringup_phy:
>    -> this is where I'm adding the new phy_config_inband_aneg function
> 
> If we were to use only my phy_config_inband_aneg function, it would need
> to be moved upwards in the code path, to be precise where phylink_phy_no_inband
> currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback
> to MLO_AN_PHY if that fails. I think this would unnecessarily complicate
> the code.

There's another possibility - we could postpone the actual configuration
on the MAC side to always be in phylink_sfp_module_start(), essentially
moving the phylink_mac_initial_config() call to that point while
preserving the current locations where we compute the initial interface
mode. We can then defer the AN mode selection.

> Alternatively, I could create a second PHY driver method, simply for
> validation of supported inband modes. The validation can be done in
> place of the current phylink_phy_noinband(), and I can still have the
> phy_config_inband_aneg() where I put it now, since we shouldn't have any
> surprises w.r.t. supported operating mode, and there should be no reason
> to repeat the attempt as there would be with a single PHY driver method.
> Thoughts?

That also sounds like it should work, and would probably be more
flexible.

> > Also, there needs to be consideration of PHYs that dynamically change
> > their interface type, and whether they support inband signalling.
> > For example, a PHY may support a mode where it dynamically selects
> > between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
> > mode may have inband signalling enabled or disabled. This is not a
> > theoretical case; we have a PHY like that supported in the kernel and
> > boards use it. What would the semantics of your new call be for a PHY
> > that performs this?
> > 
> > Should we also have a phydev->inband tristate, taking values "unknown,
> > enabled, disabled" which the PHY driver is required to update in their
> > read_status callback if they dynamically change their interface type?
> > (Although then phylink will need to figure out how to deal with that.)
> 
> I don't have such PHY to test with, but I think the easiest way would be
> to just call the validation method again, after we change the
> phydev->interface value.

I'm not sure I follow. It is the PHY driver that is in charge of
changing phydev->interface dynamically, since that is precisely what
the hardware is doing.

88x3310 hardware, when connected using a single lane serdes without
rate adaption will switch the MAC side interface between 10GBASE-R,
5GBASE-R, 2500BASE-X and SGMII depending on the media side speed (10G,
5G, 2.5G, 1G, 100M, 10M.)

BCM84881 does the same, switching dynamically between 10GBASE-R,
2500BASE-X and SGMII (never with inband signalling - it's not
supported in hardware) for 10G, 2.5G, 1G and 100M speeds. 10M is not
supported.

With both these PHYs, you don't get to say "I want you to operate in
_this_ single interface mode", with the exception of 88x3310 with rate
adaption, they aren't designed for that. As soon as there is link on
the media side, the PHYs automatically reconfigure their MAC side with
no intervention from MDIO.

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

[PATCH net-next 2/2] net: phy: mscc: configure in-band auto-negotiation for VSC8514

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Add the in-band configuration knob for the VSC8514 quad PHY. Tested with
QSGMII in-band AN both on and off on NXP LS1028A-RDB and T1040-RDB.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/mscc/mscc.h      |  2 ++
 drivers/net/phy/mscc/mscc_main.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 9481bce94c2e..44d1d8f28481 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -187,6 +187,8 @@ enum rgmii_clock_delay {
 #define MSCC_PHY_EXTENDED_INT_MS_EGR	  BIT(9)
 
 /* Extended Page 3 Registers */
+#define MSCC_PHY_SERDES_PCS_CTRL	  16
+#define MSCC_PHY_SERDES_ANEG		  BIT(7)
 #define MSCC_PHY_SERDES_TX_VALID_CNT	  21
 #define MSCC_PHY_SERDES_TX_CRC_ERR_CNT	  22
 #define MSCC_PHY_SERDES_RX_VALID_CNT	  28
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 2f2157e3deab..2951ed216620 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1986,6 +1986,18 @@ static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
+static int vsc8514_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	int reg_val = 0;
+
+	if (enabled)
+		reg_val = MSCC_PHY_SERDES_ANEG;
+
+	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
+				MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
+				reg_val);
+}
+
 static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
@@ -2176,6 +2188,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.phy_id_mask	= 0xfffffff0,
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc8514_config_init,
+	.config_inband_aneg = vsc8514_config_inband_aneg,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
 	.handle_interrupt = vsc85xx_handle_interrupt,
-- 
2.25.1