Some phylink and phylib based systems might want to operate on backplane
media types ("K" in the name), and thus, picking a phy_interface_t for
them becomes a challenge.
phy_interface_t is a description of the connection between the MAC and
the PHY, or if a MAC-side PCS is present, the connection between that
and the next link segment (which can be remote).
A MAC-side PCS is so far considered to be a PCS handling link modes with
optional C37 autoneg. But C73 autoneg (for backplanes and SFP28 modules)
is not at the same level in the OSI layering, so that existing model may
or may not apply.
(a) If we say that the PCS is MAC-side for C73 modes as well, the
implication seems to be that the phy-mode should be one of
PHY_INTERFACE_MODE_10GBASEKR, PHY_INTERFACE_MODE_1000BASEKX, etc.
Similar to PHY_INTERFACE_MODE_1000BASEX which imitates the link mode
ETHTOOL_LINK_MODE_1000baseX_Full_BIT.
(b) If we say that the PCS is not MAC-side, but rather that the
phylink_pcs represents an entire non-phylib backplane PHY which may
negotiate one of many link modes (like a copper phylib PHY), then
the phy-mode should probably be one of PHY_INTERFACE_MODE_XGMII,
XLGMII etc. Or rather, because there is no MII pinout per se and the
backplane PHY / phylink_pcs is internal, we can also use
PHY_INTERFACE_MODE_INTERNAL.
The trouble with (a), in my opinion, is that if we let the phy_interface_t
follow the link mode like in the case of Base-X fiber modes, we have to
consider the fact that C73 PHYs can advertise multiple link modes, so
the phy_interface_t selection will be arbitrary, and any phy_interface_t
selection will have to leave in the "supported" and "advertised" masks
of link modes all the other backplane modes. This may be hard to justify.
That is the reasoning based on which I selected this phy-mode to
describe the setup in Layerscape SoCs which have integrated backplane
autoneg support. The changes in phylink permit the managed =
"in-band-status" fwnode property to be extended for C73 autoneg, which
is then controllable through ethtool. With phy-mode = "internal" in an
in-band autoneg mode, we advertise all backplane link modes. The list is
not exhaustive and may be extended in the future.
Link: https://lore.kernel.org/netdev/ZOXlpkbcAZ4okric@shell.armlinux.org.uk/
Link: https://lore.kernel.org/netdev/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new
drivers/net/phy/phylink.c | 19 ++++++++++++++++++-
include/linux/phylink.h | 1 +
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 548130d77302..88ace7e203c3 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -972,6 +972,21 @@ static int phylink_parse_mode(struct phylink *pl,
phylink_set(pl->supported, 100000baseDR2_Full);
break;
+ case PHY_INTERFACE_MODE_INTERNAL:
+ phylink_set(pl->supported, 1000baseKX_Full);
+ phylink_set(pl->supported, 10000baseKX4_Full);
+ phylink_set(pl->supported, 10000baseKR_Full);
+ phylink_set(pl->supported, 25000baseCR_Full);
+ phylink_set(pl->supported, 25000baseKR_Full);
+ phylink_set(pl->supported, 25000baseCR_S_Full);
+ phylink_set(pl->supported, 25000baseKR_S_Full);
+ phylink_set(pl->supported, 40000baseKR4_Full);
+ phylink_set(pl->supported, 50000baseKR2_Full);
+ phylink_set(pl->supported, 50000baseKR_Full);
+ phylink_set(pl->supported, 100000baseKR4_Full);
+ phylink_set(pl->supported, 100000baseKR2_Full);
+ break;
+
default:
phylink_err(pl,
"incorrect link mode %s for in-band status\n",
@@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
{
- return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
+ return (phy_interface_mode_is_8023z(iface) ||
+ iface == PHY_INTERFACE_MODE_INTERNAL) &&
+ phylink_autoneg_inband(mode);
}
static void phylink_pcs_an_restart(struct phylink *pl)
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 2b886ea654bb..7e8e26001587 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
case PHY_INTERFACE_MODE_1000BASEX:
case PHY_INTERFACE_MODE_2500BASEX:
+ case PHY_INTERFACE_MODE_INTERNAL:
/* 1000base-X is designed for use media-side for Fibre
* connections, and thus the Autoneg bit needs to be
* taken into account. We also do this for 2500base-X
--
2.34.1Hi Russell,
On Sat, Sep 23, 2023 at 04:48:57PM +0300, Vladimir Oltean wrote:
> Some phylink and phylib based systems might want to operate on backplane
> media types ("K" in the name), and thus, picking a phy_interface_t for
> them becomes a challenge.
>
> phy_interface_t is a description of the connection between the MAC and
> the PHY, or if a MAC-side PCS is present, the connection between that
> and the next link segment (which can be remote).
>
> A MAC-side PCS is so far considered to be a PCS handling link modes with
> optional C37 autoneg. But C73 autoneg (for backplanes and SFP28 modules)
> is not at the same level in the OSI layering, so that existing model may
> or may not apply.
>
> (a) If we say that the PCS is MAC-side for C73 modes as well, the
> implication seems to be that the phy-mode should be one of
> PHY_INTERFACE_MODE_10GBASEKR, PHY_INTERFACE_MODE_1000BASEKX, etc.
> Similar to PHY_INTERFACE_MODE_1000BASEX which imitates the link mode
> ETHTOOL_LINK_MODE_1000baseX_Full_BIT.
>
> (b) If we say that the PCS is not MAC-side, but rather that the
> phylink_pcs represents an entire non-phylib backplane PHY which may
> negotiate one of many link modes (like a copper phylib PHY), then
> the phy-mode should probably be one of PHY_INTERFACE_MODE_XGMII,
> XLGMII etc. Or rather, because there is no MII pinout per se and the
> backplane PHY / phylink_pcs is internal, we can also use
> PHY_INTERFACE_MODE_INTERNAL.
>
> The trouble with (a), in my opinion, is that if we let the phy_interface_t
> follow the link mode like in the case of Base-X fiber modes, we have to
> consider the fact that C73 PHYs can advertise multiple link modes, so
> the phy_interface_t selection will be arbitrary, and any phy_interface_t
> selection will have to leave in the "supported" and "advertised" masks
> of link modes all the other backplane modes. This may be hard to justify.
>
> That is the reasoning based on which I selected this phy-mode to
> describe the setup in Layerscape SoCs which have integrated backplane
> autoneg support. The changes in phylink permit the managed =
> "in-band-status" fwnode property to be extended for C73 autoneg, which
> is then controllable through ethtool. With phy-mode = "internal" in an
> in-band autoneg mode, we advertise all backplane link modes. The list is
> not exhaustive and may be extended in the future.
>
> Link: https://lore.kernel.org/netdev/ZOXlpkbcAZ4okric@shell.armlinux.org.uk/
> Link: https://lore.kernel.org/netdev/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: patch is new
>
> drivers/net/phy/phylink.c | 19 ++++++++++++++++++-
> include/linux/phylink.h | 1 +
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 548130d77302..88ace7e203c3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -972,6 +972,21 @@ static int phylink_parse_mode(struct phylink *pl,
> phylink_set(pl->supported, 100000baseDR2_Full);
> break;
>
> + case PHY_INTERFACE_MODE_INTERNAL:
> + phylink_set(pl->supported, 1000baseKX_Full);
> + phylink_set(pl->supported, 10000baseKX4_Full);
> + phylink_set(pl->supported, 10000baseKR_Full);
> + phylink_set(pl->supported, 25000baseCR_Full);
> + phylink_set(pl->supported, 25000baseKR_Full);
> + phylink_set(pl->supported, 25000baseCR_S_Full);
> + phylink_set(pl->supported, 25000baseKR_S_Full);
> + phylink_set(pl->supported, 40000baseKR4_Full);
> + phylink_set(pl->supported, 50000baseKR2_Full);
> + phylink_set(pl->supported, 50000baseKR_Full);
> + phylink_set(pl->supported, 100000baseKR4_Full);
> + phylink_set(pl->supported, 100000baseKR2_Full);
> + break;
> +
> default:
> phylink_err(pl,
> "incorrect link mode %s for in-band status\n",
> @@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
>
> static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
> {
> - return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
> + return (phy_interface_mode_is_8023z(iface) ||
> + iface == PHY_INTERFACE_MODE_INTERNAL) &&
> + phylink_autoneg_inband(mode);
> }
>
> static void phylink_pcs_an_restart(struct phylink *pl)
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 2b886ea654bb..7e8e26001587 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
>
> case PHY_INTERFACE_MODE_1000BASEX:
> case PHY_INTERFACE_MODE_2500BASEX:
> + case PHY_INTERFACE_MODE_INTERNAL:
> /* 1000base-X is designed for use media-side for Fibre
> * connections, and thus the Autoneg bit needs to be
> * taken into account. We also do this for 2500base-X
> --
> 2.34.1
>
What do you think about this change?
Hi Vladimir,
On Mon, Oct 02, 2023 at 05:17:43PM +0300, Vladimir Oltean wrote:
> Hi Russell,
>
> On Sat, Sep 23, 2023 at 04:48:57PM +0300, Vladimir Oltean wrote:
> > Some phylink and phylib based systems might want to operate on backplane
> > media types ("K" in the name), and thus, picking a phy_interface_t for
> > them becomes a challenge.
> >
> > phy_interface_t is a description of the connection between the MAC and
> > the PHY, or if a MAC-side PCS is present, the connection between that
> > and the next link segment (which can be remote).
> >
> > A MAC-side PCS is so far considered to be a PCS handling link modes with
> > optional C37 autoneg. But C73 autoneg (for backplanes and SFP28 modules)
> > is not at the same level in the OSI layering, so that existing model may
> > or may not apply.
> >
> > (a) If we say that the PCS is MAC-side for C73 modes as well, the
> > implication seems to be that the phy-mode should be one of
> > PHY_INTERFACE_MODE_10GBASEKR, PHY_INTERFACE_MODE_1000BASEKX, etc.
> > Similar to PHY_INTERFACE_MODE_1000BASEX which imitates the link mode
> > ETHTOOL_LINK_MODE_1000baseX_Full_BIT.
> >
> > (b) If we say that the PCS is not MAC-side, but rather that the
> > phylink_pcs represents an entire non-phylib backplane PHY which may
> > negotiate one of many link modes (like a copper phylib PHY), then
> > the phy-mode should probably be one of PHY_INTERFACE_MODE_XGMII,
> > XLGMII etc. Or rather, because there is no MII pinout per se and the
> > backplane PHY / phylink_pcs is internal, we can also use
> > PHY_INTERFACE_MODE_INTERNAL.
> >
> > The trouble with (a), in my opinion, is that if we let the phy_interface_t
> > follow the link mode like in the case of Base-X fiber modes, we have to
> > consider the fact that C73 PHYs can advertise multiple link modes, so
> > the phy_interface_t selection will be arbitrary, and any phy_interface_t
> > selection will have to leave in the "supported" and "advertised" masks
> > of link modes all the other backplane modes. This may be hard to justify.
> >
> > That is the reasoning based on which I selected this phy-mode to
> > describe the setup in Layerscape SoCs which have integrated backplane
> > autoneg support. The changes in phylink permit the managed =
> > "in-band-status" fwnode property to be extended for C73 autoneg, which
> > is then controllable through ethtool. With phy-mode = "internal" in an
> > in-band autoneg mode, we advertise all backplane link modes. The list is
> > not exhaustive and may be extended in the future.
> >
> > Link: https://lore.kernel.org/netdev/ZOXlpkbcAZ4okric@shell.armlinux.org.uk/
> > Link: https://lore.kernel.org/netdev/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > v1->v2: patch is new
> >
> > drivers/net/phy/phylink.c | 19 ++++++++++++++++++-
> > include/linux/phylink.h | 1 +
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 548130d77302..88ace7e203c3 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -972,6 +972,21 @@ static int phylink_parse_mode(struct phylink *pl,
> > phylink_set(pl->supported, 100000baseDR2_Full);
> > break;
> >
> > + case PHY_INTERFACE_MODE_INTERNAL:
> > + phylink_set(pl->supported, 1000baseKX_Full);
> > + phylink_set(pl->supported, 10000baseKX4_Full);
> > + phylink_set(pl->supported, 10000baseKR_Full);
> > + phylink_set(pl->supported, 25000baseCR_Full);
> > + phylink_set(pl->supported, 25000baseKR_Full);
> > + phylink_set(pl->supported, 25000baseCR_S_Full);
> > + phylink_set(pl->supported, 25000baseKR_S_Full);
> > + phylink_set(pl->supported, 40000baseKR4_Full);
> > + phylink_set(pl->supported, 50000baseKR2_Full);
> > + phylink_set(pl->supported, 50000baseKR_Full);
> > + phylink_set(pl->supported, 100000baseKR4_Full);
> > + phylink_set(pl->supported, 100000baseKR2_Full);
> > + break;
I wonder whether this should just set all link modes, much like
phylink_get_capabilities() allows.
I'm also wondering whether the contents of this switch() statement
should now just do:
case PHY_INTERFACE_... (for each supported mode):
cals = ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
caps = phylink_get_capabilities(interface, caps,
RATE_MATCH_NONE);
phylink_caps_to_linkmodes(pl->supported, caps);
break;
rather than duplicating the logic.
That said, 10GBASER and 10GKR are treated slightly differently because
of the problem with PHYs like 88x3310, and I think it's now difficult
to undo that bit of history.
> > +
> > default:
> > phylink_err(pl,
> > "incorrect link mode %s for in-band status\n",
> > @@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
> >
> > static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
> > {
> > - return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
> > + return (phy_interface_mode_is_8023z(iface) ||
> > + iface == PHY_INTERFACE_MODE_INTERNAL) &&
> > + phylink_autoneg_inband(mode);
Is this true also for DSA devices that use "internal" mode? I'm
wondering whether this will cause the PHY to be ignored/remain
unattached in DSA switches because of the changes in patch 7.
> > }
> >
> > static void phylink_pcs_an_restart(struct phylink *pl)
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 2b886ea654bb..7e8e26001587 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
> >
> > case PHY_INTERFACE_MODE_1000BASEX:
> > case PHY_INTERFACE_MODE_2500BASEX:
> > + case PHY_INTERFACE_MODE_INTERNAL:
> > /* 1000base-X is designed for use media-side for Fibre
> > * connections, and thus the Autoneg bit needs to be
> > * taken into account. We also do this for 2500base-X
Thinking about DSA cases, I don't think this change would be an issue
because where DSA uses "internal" there isn't a PCS, so this won't
matter.
Note that as there is now no need for anything outside phylink.c to
reference this function, I have plans at some point to move it into
the .c file rather than keeping it as an inline in the header file.
It was temporarily necessary while introducing it to be in the
header.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell,
On Tue, Oct 03, 2023 at 12:06:18PM +0100, Russell King (Oracle) wrote:
> I wonder whether this should just set all link modes, much like
> phylink_get_capabilities() allows.
>
> I'm also wondering whether the contents of this switch() statement
> should now just do:
>
> case PHY_INTERFACE_... (for each supported mode):
> cals = ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> caps = phylink_get_capabilities(interface, caps,
> RATE_MATCH_NONE);
> phylink_caps_to_linkmodes(pl->supported, caps);
> break;
>
> rather than duplicating the logic.
You mean something like this?
[PATCH] net: phylink: reimplement population of pl->supported for in-band
phylink_parse_mode() populates all possible supported link modes for a
given phy_interface_t, for the case where a phylib phy may be absent and
we can't retrieve the supported link modes from that.
Russell points out that since the introduction of the generic validation
helpers phylink_get_capabilities() and phylink_caps_to_linkmodes(), we
can rewrite this procedure to populate the pl->supported mask, so that
instead of spelling out the link modes, we derive an intermediary
mac_capabilities bit field, and we convert that to the equivalent link
modes.
Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/phy/phylink.c | 71 +++------------------------------------
1 file changed, 5 insertions(+), 66 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 548130d77302..88a4b726a9fc 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -864,6 +864,7 @@ static int phylink_parse_mode(struct phylink *pl,
{
struct fwnode_handle *dn;
const char *managed;
+ unsigned long caps;
dn = fwnode_get_named_child_node(fwnode, "fixed-link");
if (dn || fwnode_property_present(fwnode, "fixed-link"))
@@ -896,80 +897,18 @@ static int phylink_parse_mode(struct phylink *pl,
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_RTBI:
- phylink_set(pl->supported, 10baseT_Half);
- phylink_set(pl->supported, 10baseT_Full);
- phylink_set(pl->supported, 100baseT_Half);
- phylink_set(pl->supported, 100baseT_Full);
- phylink_set(pl->supported, 1000baseT_Half);
- phylink_set(pl->supported, 1000baseT_Full);
- break;
-
case PHY_INTERFACE_MODE_1000BASEX:
- phylink_set(pl->supported, 1000baseX_Full);
- break;
-
case PHY_INTERFACE_MODE_2500BASEX:
- phylink_set(pl->supported, 2500baseX_Full);
- break;
-
case PHY_INTERFACE_MODE_5GBASER:
- phylink_set(pl->supported, 5000baseT_Full);
- break;
-
case PHY_INTERFACE_MODE_25GBASER:
- phylink_set(pl->supported, 25000baseCR_Full);
- phylink_set(pl->supported, 25000baseKR_Full);
- phylink_set(pl->supported, 25000baseSR_Full);
- fallthrough;
case PHY_INTERFACE_MODE_USXGMII:
case PHY_INTERFACE_MODE_10GKR:
case PHY_INTERFACE_MODE_10GBASER:
- phylink_set(pl->supported, 10baseT_Half);
- phylink_set(pl->supported, 10baseT_Full);
- phylink_set(pl->supported, 100baseT_Half);
- phylink_set(pl->supported, 100baseT_Full);
- phylink_set(pl->supported, 1000baseT_Half);
- phylink_set(pl->supported, 1000baseT_Full);
- phylink_set(pl->supported, 1000baseX_Full);
- phylink_set(pl->supported, 1000baseKX_Full);
- phylink_set(pl->supported, 2500baseT_Full);
- phylink_set(pl->supported, 2500baseX_Full);
- phylink_set(pl->supported, 5000baseT_Full);
- phylink_set(pl->supported, 10000baseT_Full);
- phylink_set(pl->supported, 10000baseKR_Full);
- phylink_set(pl->supported, 10000baseKX4_Full);
- phylink_set(pl->supported, 10000baseCR_Full);
- phylink_set(pl->supported, 10000baseSR_Full);
- phylink_set(pl->supported, 10000baseLR_Full);
- phylink_set(pl->supported, 10000baseLRM_Full);
- phylink_set(pl->supported, 10000baseER_Full);
- break;
-
case PHY_INTERFACE_MODE_XLGMII:
- phylink_set(pl->supported, 25000baseCR_Full);
- phylink_set(pl->supported, 25000baseKR_Full);
- phylink_set(pl->supported, 25000baseSR_Full);
- phylink_set(pl->supported, 40000baseKR4_Full);
- phylink_set(pl->supported, 40000baseCR4_Full);
- phylink_set(pl->supported, 40000baseSR4_Full);
- phylink_set(pl->supported, 40000baseLR4_Full);
- phylink_set(pl->supported, 50000baseCR2_Full);
- phylink_set(pl->supported, 50000baseKR2_Full);
- phylink_set(pl->supported, 50000baseSR2_Full);
- phylink_set(pl->supported, 50000baseKR_Full);
- phylink_set(pl->supported, 50000baseSR_Full);
- phylink_set(pl->supported, 50000baseCR_Full);
- phylink_set(pl->supported, 50000baseLR_ER_FR_Full);
- phylink_set(pl->supported, 50000baseDR_Full);
- phylink_set(pl->supported, 100000baseKR4_Full);
- phylink_set(pl->supported, 100000baseSR4_Full);
- phylink_set(pl->supported, 100000baseCR4_Full);
- phylink_set(pl->supported, 100000baseLR4_ER4_Full);
- phylink_set(pl->supported, 100000baseKR2_Full);
- phylink_set(pl->supported, 100000baseSR2_Full);
- phylink_set(pl->supported, 100000baseCR2_Full);
- phylink_set(pl->supported, 100000baseLR2_ER2_FR2_Full);
- phylink_set(pl->supported, 100000baseDR2_Full);
+ caps = ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
+ caps = phylink_get_capabilities(pl->link_config.interface, caps,
+ RATE_MATCH_NONE);
+ phylink_caps_to_linkmodes(pl->supported, caps);
break;
default:
I've superficially tested it on 2 boards (LS1028A-RDB and Turris MOX)
and I didn't notice regressions.
But...
> That said, 10GBASER and 10GKR are treated slightly differently because
> of the problem with PHYs like 88x3310, and I think it's now difficult
> to undo that bit of history.
I don't really understand this. Can you please show me where and how
PHY_INTERFACE_MODE_10GBASER and PHY_INTERFACE_MODE_10GKR are treated
differently in the kernel?
> > > +
> > > default:
> > > phylink_err(pl,
> > > "incorrect link mode %s for in-band status\n",
> > > @@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
> > >
> > > static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
> > > {
> > > - return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
> > > + return (phy_interface_mode_is_8023z(iface) ||
> > > + iface == PHY_INTERFACE_MODE_INTERNAL) &&
> > > + phylink_autoneg_inband(mode);
>
> Is this true also for DSA devices that use "internal" mode? I'm
> wondering whether this will cause the PHY to be ignored/remain
> unattached in DSA switches because of the changes in patch 7.
phylink_pcs_handles_an() shouldn't return true for those, no. For that
to happen, phylink_autoneg_inband() should also return true for that link.
Which I believe it never does, because prior to this change, phylink_parse_mode()
would have errored out with -EINVAL and the "incorrect link mode %s for in-band status\n"
text for phy-mode = "internal".
> > > }
> > >
> > > static void phylink_pcs_an_restart(struct phylink *pl)
> > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > > index 2b886ea654bb..7e8e26001587 100644
> > > --- a/include/linux/phylink.h
> > > +++ b/include/linux/phylink.h
> > > @@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
> > >
> > > case PHY_INTERFACE_MODE_1000BASEX:
> > > case PHY_INTERFACE_MODE_2500BASEX:
> > > + case PHY_INTERFACE_MODE_INTERNAL:
> > > /* 1000base-X is designed for use media-side for Fibre
> > > * connections, and thus the Autoneg bit needs to be
> > > * taken into account. We also do this for 2500base-X
>
> Thinking about DSA cases, I don't think this change would be an issue
> because where DSA uses "internal" there isn't a PCS, so this won't
> matter.
I agree, and also see the justification above.
> Note that as there is now no need for anything outside phylink.c to
> reference this function, I have plans at some point to move it into
> the .c file rather than keeping it as an inline in the header file.
> It was temporarily necessary while introducing it to be in the
> header.
Ok. Do you want me to move phylink_pcs_neg_mode() to phylink.c as part
of this series?
© 2016 - 2026 Red Hat, Inc.