This moves all PCS-related settings to the pcs callbacks. This makes it
easy to support external PCSs. In addition, support for 1000BASE-X is
added (since we need it to set the SGMII mux).
pcs_config should set all registers necessary to bring the link up. In
addition, it needs to keep track of whether it modified anything so that
phylink can restart autonegotiation. config is also the right time to
veto configuration parameters, such as using the wrong interface. This
catches someone trying to use a slower speed (which could be supported
otherwise) after using a faster speed. We can't support this because
phylink doesn't support detaching PCSs.
pcs_link_up is necessary IFF the mode is not in-band and the speed and
duplex are different from what was set in config. However, because the
PCS supports only fixed speed (SPEED_1000 or SPEED_10000) and full
duplex, there is nothing to configure. Therefore, link_up has been
removed.
Now that the autonegotiation is done in pcs_config, we no longer need to
do it in macb_mac_config.
Signed-off-by: Sean Anderson <[email protected]>
---
drivers/net/ethernet/cadence/macb_main.c | 214 +++++++++++++++--------
1 file changed, 138 insertions(+), 76 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db7acce42a27..08dcccd94f87 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -543,6 +543,7 @@ static void macb_validate(struct phylink_config *config,
goto none;
fallthrough;
case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_1000BASEX:
if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
phylink_set(mask, 1000baseT_Full);
phylink_set(mask, 1000baseX_Full);
@@ -571,25 +572,100 @@ static void macb_validate(struct phylink_config *config,
__ETHTOOL_LINK_MODE_MASK_NBITS);
}
-static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
- phy_interface_t interface, int speed,
- int duplex)
-{
- struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
- u32 config;
-
- config = gem_readl(bp, USX_CONTROL);
- config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
- config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config);
- config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
- config |= GEM_BIT(TX_EN);
- gem_writel(bp, USX_CONTROL, config);
+static inline struct macb *pcs_to_macb(struct phylink_pcs *pcs)
+{
+ return container_of(pcs, struct macb, phylink_pcs);
+}
+
+static void macb_pcs_get_state(struct phylink_pcs *pcs,
+ struct phylink_link_state *state)
+{
+ struct macb *bp = pcs_to_macb(pcs);
+
+ if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
+ state->interface = PHY_INTERFACE_MODE_SGMII;
+ else
+ state->interface = PHY_INTERFACE_MODE_1000BASEX;
+
+ phylink_mii_c22_pcs_decode_state(state, gem_readl(bp, PCSSTS),
+ gem_readl(bp, PCSANLPBASE));
+}
+
+/**
+ * macb_pcs_config_an() - Configure autonegotiation settings for PCSs
+ * @bp - The macb to operate on
+ * @mode - The autonegotiation mode
+ * @interface - The interface to use
+ * @advertising - The advertisement mask
+ *
+ * This provides common configuration for PCS autonegotiation.
+ *
+ * Context: Call with @bp->lock held.
+ * Return: 1 if any registers were changed; 0 otherwise
+ */
+static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising)
+{
+ bool changed = false;
+ u16 old, new;
+
+ old = gem_readl(bp, PCSANADV);
+ new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
+ old);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, PCSANADV, new);
+ }
+
+ old = new = gem_readl(bp, PCSCNTRL);
+ if (mode == MLO_AN_INBAND)
+ new |= BMCR_ANENABLE;
+ else
+ new &= ~BMCR_ANENABLE;
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, PCSCNTRL, new);
+ }
+ return changed;
+}
+
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit_pause_to_mac)
+{
+ bool changed = false;
+ struct macb *bp = pcs_to_macb(pcs);
+ u16 old, new;
+ unsigned long flags;
+
+ spin_lock_irqsave(&bp->lock, flags);
+ old = new = gem_readl(bp, NCFGR);
+ if (interface == PHY_INTERFACE_MODE_SGMII) {
+ new |= GEM_BIT(SGMIIEN);
+ } else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
+ new &= ~GEM_BIT(SGMIIEN);
+ } else {
+ spin_lock_irqsave(&bp->lock, flags);
+ return -EOPNOTSUPP;
+ }
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, NCFGR, new);
+ }
+
+ if (macb_pcs_config_an(bp, mode, interface, advertising))
+ changed = true;
+
+ spin_unlock_irqrestore(&bp->lock, flags);
+ return changed;
}
static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
struct phylink_link_state *state)
{
- struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+ struct macb *bp = pcs_to_macb(pcs);
u32 val;
state->speed = SPEED_10000;
@@ -609,70 +685,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
const unsigned long *advertising,
bool permit_pause_to_mac)
{
- struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+ bool changed;
+ struct macb *bp = pcs_to_macb(pcs);
+ u16 old, new;
+ unsigned long flags;
- gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
- GEM_BIT(SIGNAL_OK));
+ if (interface != PHY_INTERFACE_MODE_10GBASER)
+ return -EOPNOTSUPP;
- return 0;
-}
+ spin_lock_irqsave(&bp->lock, flags);
+ old = new = gem_readl(bp, NCR);
+ new |= GEM_BIT(ENABLE_HS_MAC);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, NCFGR, new);
+ }
-static void macb_pcs_get_state(struct phylink_pcs *pcs,
- struct phylink_link_state *state)
-{
- state->link = 0;
-}
+ if (macb_pcs_config_an(bp, mode, interface, advertising))
+ changed = true;
-static void macb_pcs_an_restart(struct phylink_pcs *pcs)
-{
- /* Not supported */
-}
+ old = new = gem_readl(bp, USX_CONTROL);
+ new |= GEM_BIT(SIGNAL_OK);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, USX_CONTROL, new);
+ }
-static int macb_pcs_config(struct phylink_pcs *pcs,
- unsigned int mode,
- phy_interface_t interface,
- const unsigned long *advertising,
- bool permit_pause_to_mac)
-{
- return 0;
+ old = new = gem_readl(bp, USX_CONTROL);
+ new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
+ new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
+ new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
+ new |= GEM_BIT(TX_EN);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, USX_CONTROL, new);
+ }
+
+ spin_unlock_irqrestore(&bp->lock, flags);
+ return changed;
}
static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
.pcs_get_state = macb_usx_pcs_get_state,
.pcs_config = macb_usx_pcs_config,
- .pcs_link_up = macb_usx_pcs_link_up,
};
static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
.pcs_get_state = macb_pcs_get_state,
- .pcs_an_restart = macb_pcs_an_restart,
.pcs_config = macb_pcs_config,
};
static void macb_mac_config(struct phylink_config *config, unsigned int mode,
const struct phylink_link_state *state)
{
- unsigned long flags;
-
- spin_lock_irqsave(&bp->lock, flags);
-
- /* Disable AN for SGMII fixed link configuration, enable otherwise.
- * Must be written after PCSSEL is set in NCFGR,
- * otherwise writes will not take effect.
- */
- if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
- u32 pcsctrl, old_pcsctrl;
-
- old_pcsctrl = gem_readl(bp, PCSCNTRL);
- if (mode == MLO_AN_FIXED)
- pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
- else
- pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
- if (old_pcsctrl != pcsctrl)
- gem_writel(bp, PCSCNTRL, pcsctrl);
- }
-
- spin_unlock_irqrestore(&bp->lock, flags);
+ /* Nothing to do */
}
static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
@@ -763,20 +829,23 @@ static void macb_mac_link_up(struct phylink_config *config,
static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
phy_interface_t interface)
{
+ int set_pcs = 0;
struct net_device *ndev = to_net_dev(config->dev);
struct macb *bp = netdev_priv(ndev);
unsigned long flags;
u32 old_ctrl, ctrl;
u32 old_ncr, ncr;
- if (interface == PHY_INTERFACE_MODE_10GBASER)
+ if (interface == PHY_INTERFACE_MODE_10GBASER) {
bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
- else if (interface == PHY_INTERFACE_MODE_SGMII)
+ set_pcs = 1;
+ } else if (interface == PHY_INTERFACE_MODE_SGMII ||
+ interface == PHY_INTERFACE_MODE_1000BASEX) {
bp->phylink_pcs.ops = &macb_phylink_pcs_ops;
- else
- bp->phylink_pcs.ops = NULL;
+ set_pcs = 1;
+ }
- if (bp->phylink_pcs.ops)
+ if (set_pcs)
phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
spin_lock_irqsave(&bp->lock, flags);
@@ -787,21 +856,14 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
if (bp->caps & MACB_CAPS_MACB_IS_EMAC) {
if (interface == PHY_INTERFACE_MODE_RMII)
ctrl |= MACB_BIT(RM9200_RMII);
- } else if (macb_is_gem(bp)) {
- ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
- ncr &= ~GEM_BIT(ENABLE_HS_MAC);
-
- if (state->interface == PHY_INTERFACE_MODE_SGMII) {
- ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
- } else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
- ctrl |= GEM_BIT(PCSSEL);
- ncr |= GEM_BIT(ENABLE_HS_MAC);
- } else if (bp->caps & MACB_CAPS_MIIONRGMII &&
- bp->phy_interface == PHY_INTERFACE_MODE_MII) {
- ncr |= MACB_BIT(MIIONRGMII);
- }
+ } else if (bp->caps & MACB_CAPS_MIIONRGMII &&
+ bp->phy_interface == PHY_INTERFACE_MODE_MII) {
+ ncr |= MACB_BIT(MIIONRGMII);
}
+ if (macb_is_gem(bp) && set_pcs)
+ ctrl |= GEM_BIT(PCSSEL);
+
/* Apply the new configuration, if any */
if (old_ctrl ^ ctrl)
macb_or_gem_writel(bp, NCFGR, ctrl);
--
2.25.1
On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> + struct phylink_link_state *state)
> +{
> + struct macb *bp = pcs_to_macb(pcs);
> +
> + if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> + state->interface = PHY_INTERFACE_MODE_SGMII;
> + else
> + state->interface = PHY_INTERFACE_MODE_1000BASEX;
There is no requirement to set state->interface here. Phylink doesn't
cater for interface changes when reading the state. As documented,
phylink will set state->interface already before calling this function
to indicate what interface mode it is currently expecting from the
hardware.
> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising)
> +{
> + bool changed = false;
> + u16 old, new;
> +
> + old = gem_readl(bp, PCSANADV);
> + new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
> + old);
> + if (old != new) {
> + changed = true;
> + gem_writel(bp, PCSANADV, new);
> + }
> +
> + old = new = gem_readl(bp, PCSCNTRL);
> + if (mode == MLO_AN_INBAND)
Please use phylink_autoneg_inband(mode) here.
> + new |= BMCR_ANENABLE;
> + else
> + new &= ~BMCR_ANENABLE;
> + if (old != new) {
> + changed = true;
> + gem_writel(bp, PCSCNTRL, new);
> + }
There has been the suggestion that we should allow in-band AN to be
disabled in 1000base-X if we're in in-band mode according to the
ethtool state. I have a patch that adds that.
> + return changed;
> +}
> +
> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + bool changed = false;
> + struct macb *bp = pcs_to_macb(pcs);
> + u16 old, new;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&bp->lock, flags);
> + old = new = gem_readl(bp, NCFGR);
> + if (interface == PHY_INTERFACE_MODE_SGMII) {
> + new |= GEM_BIT(SGMIIEN);
> + } else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> + new &= ~GEM_BIT(SGMIIEN);
> + } else {
> + spin_lock_irqsave(&bp->lock, flags);
> + return -EOPNOTSUPP;
You can't actually abort at this point - phylink will print the error
and carry on regardless. The checking is all done via the validate()
callback and if that indicates the interface mode is acceptable, then
it should be accepted.
> static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
> .pcs_get_state = macb_usx_pcs_get_state,
> .pcs_config = macb_usx_pcs_config,
> - .pcs_link_up = macb_usx_pcs_link_up,
> };
>
> static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
> .pcs_get_state = macb_pcs_get_state,
> - .pcs_an_restart = macb_pcs_an_restart,
You don't want to remove this. When operating in 1000BASE-X mode, it
will be called if a restart is required (e.g. macb_pcs_config()
returning positive, or an ethtool request.) You need to keep the empty
function. That may also help the diff algorithm to produce a cleaner
patch too.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell,
On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> + struct phylink_link_state *state)
>> +{
>> + struct macb *bp = pcs_to_macb(pcs);
>> +
>> + if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> + state->interface = PHY_INTERFACE_MODE_SGMII;
>> + else
>> + state->interface = PHY_INTERFACE_MODE_1000BASEX;
>
> There is no requirement to set state->interface here. Phylink doesn't
> cater for interface changes when reading the state. As documented,
> phylink will set state->interface already before calling this function
> to indicate what interface mode it is currently expecting from the
> hardware.
Ok, so instead I should be doing something like
if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
interface = PHY_INTERFACE_MODE_SGMII;
else
interface = PHY_INTERFACE_MODE_1000BASEX;
if (interface != state->interface) {
state->link = 0;
return;
}
?
>> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
>> + phy_interface_t interface,
>> + const unsigned long *advertising)
>> +{
>> + bool changed = false;
>> + u16 old, new;
>> +
>> + old = gem_readl(bp, PCSANADV);
>> + new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
>> + old);
>> + if (old != new) {
>> + changed = true;
>> + gem_writel(bp, PCSANADV, new);
>> + }
>> +
>> + old = new = gem_readl(bp, PCSCNTRL);
>> + if (mode == MLO_AN_INBAND)
>
> Please use phylink_autoneg_inband(mode) here.
Ok.
>
>> + new |= BMCR_ANENABLE;
>> + else
>> + new &= ~BMCR_ANENABLE;
>> + if (old != new) {
>> + changed = true;
>> + gem_writel(bp, PCSCNTRL, new);
>> + }
>
> There has been the suggestion that we should allow in-band AN to be
> disabled in 1000base-X if we're in in-band mode according to the
> ethtool state.
This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.
> I have a patch that adds that.
Have you posted it?
>> + return changed;
>> +}
>> +
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> + phy_interface_t interface,
>> + const unsigned long *advertising,
>> + bool permit_pause_to_mac)
>> +{
>> + bool changed = false;
>> + struct macb *bp = pcs_to_macb(pcs);
>> + u16 old, new;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&bp->lock, flags);
>> + old = new = gem_readl(bp, NCFGR);
>> + if (interface == PHY_INTERFACE_MODE_SGMII) {
>> + new |= GEM_BIT(SGMIIEN);
>> + } else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
>> + new &= ~GEM_BIT(SGMIIEN);
>> + } else {
>> + spin_lock_irqsave(&bp->lock, flags);
>> + return -EOPNOTSUPP;
>
> You can't actually abort at this point - phylink will print the error
> and carry on regardless. The checking is all done via the validate()
> callback and if that indicates the interface mode is acceptable, then
> it should be accepted.
Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.
>> static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>> .pcs_get_state = macb_usx_pcs_get_state,
>> .pcs_config = macb_usx_pcs_config,
>> - .pcs_link_up = macb_usx_pcs_link_up,
>> };
>>
>> static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>> .pcs_get_state = macb_pcs_get_state,
>> - .pcs_an_restart = macb_pcs_an_restart,
>
> You don't want to remove this.
Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.
> When operating in 1000BASE-X mode, it
> will be called if a restart is required (e.g. macb_pcs_config()
> returning positive, or an ethtool request.) You need to keep the empty
> function.
Ok, perhaps I can add some sanity checks for this in pcs_register.
--Sean
> That may also help the diff algorithm to produce a cleaner
> patch too.
On Tue, Oct 05, 2021 at 12:03:50PM -0400, Sean Anderson wrote:
> Hi Russell,
>
> On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> > On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> > > +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> > > + struct phylink_link_state *state)
> > > +{
> > > + struct macb *bp = pcs_to_macb(pcs);
> > > +
> > > + if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> > > + state->interface = PHY_INTERFACE_MODE_SGMII;
> > > + else
> > > + state->interface = PHY_INTERFACE_MODE_1000BASEX;
> >
> > There is no requirement to set state->interface here. Phylink doesn't
> > cater for interface changes when reading the state. As documented,
> > phylink will set state->interface already before calling this function
> > to indicate what interface mode it is currently expecting from the
> > hardware.
>
> Ok, so instead I should be doing something like
>
> if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> interface = PHY_INTERFACE_MODE_SGMII;
> else
> interface = PHY_INTERFACE_MODE_1000BASEX;
>
> if (interface != state->interface) {
> state->link = 0;
> return;
> }
Why would it be different? If we've called the pcs_config method to
set the interface to one mode, why would it change?
> > There has been the suggestion that we should allow in-band AN to be
> > disabled in 1000base-X if we're in in-band mode according to the
> > ethtool state.
>
> This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
> another _encode variant? I hadn't done this here because the logic was
> only one if statement.
>
> > I have a patch that adds that.
>
> Have you posted it?
I haven't - it is a patch from Robert Hancock, "net: phylink: Support
disabling autonegotiation for PCS". I've had it in my tree for a while,
but I do want to make some changes to it before re-posting.
> > You can't actually abort at this point - phylink will print the error
> > and carry on regardless. The checking is all done via the validate()
> > callback and if that indicates the interface mode is acceptable, then
> > it should be accepted.
>
> Ok, so where can the PCS NAK an interface? This is the only callback
> which has a return code, so I assumed this was the correct place to say
> "no, we don't support this." This is what lynx_pcs_config does as well.
At the moment, the PCS doesn't get to NAK an inappropriate interface.
That's currently the job of the MAC's validate callback with the
assumtion that the MAC knows what interfaces are supportable.
Trying to do it later, once the configuration has been worked out can
_only_ lead to a failure of some kind - in many paths, there is no way
to report the problem except by printing a message into the kernel log.
For example, by the time we reach pcs_config(), we've already prepared
the MAC for a change to the interface, we've told the MAC to configure
for that interface. Now the PCS rejects it - we have no record of the
old configuration to restore. Even if we had a way to restore it, then
we could return an error to the user - but the user doesn't get to
control the interface themselves. If it was the result of a PHY changing
its interface, then what - we can only log an error to the kernel log.
If it's the result of a SFP being plugged in, we have no way to
renegotiate.
pcs_config() is too late to be making decisions about whether the
requested configuration is acceptable or not. It needs to be done as
part of the validation step.
However, the validation step is not purely just validation, but it's
negotiation too for SFPs to be able to work out what interface mode
they should use in combination with the support that the MAC/PCS
offers.
I do feel that the implementation around the validation/selection of
interface for SFP etc is starting to creak, and I've some patches that
introduce a bitmap of interface types that are supported by the various
components. I haven't had the motivation to finish that off as my last
attempt at making a phylink API change was not pleasant in terms of
either help updating network drivers or getting patches tested. So I
now try to avoid phylink API changes at all cost.
People have whinged that phylink's API changes too quickly... I'm
guessing we're now going to get other people arguing that it needs
change to fix issues like this...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 10/5/21 2:53 PM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:03:50PM -0400, Sean Anderson wrote:
>> Hi Russell,
>>
>> On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
>> > On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> > > +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> > > + struct phylink_link_state *state)
>> > > +{
>> > > + struct macb *bp = pcs_to_macb(pcs);
>> > > +
>> > > + if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> > > + state->interface = PHY_INTERFACE_MODE_SGMII;
>> > > + else
>> > > + state->interface = PHY_INTERFACE_MODE_1000BASEX;
>> >
>> > There is no requirement to set state->interface here. Phylink doesn't
>> > cater for interface changes when reading the state. As documented,
>> > phylink will set state->interface already before calling this function
>> > to indicate what interface mode it is currently expecting from the
>> > hardware.
>>
>> Ok, so instead I should be doing something like
>>
>> if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> interface = PHY_INTERFACE_MODE_SGMII;
>> else
>> interface = PHY_INTERFACE_MODE_1000BASEX;
>>
>> if (interface != state->interface) {
>> state->link = 0;
>> return;
>> }
>
> Why would it be different? If we've called the pcs_config method to
> set the interface to one mode, why would it change?
config() does not always come before get_state due to (e.g.)
phylink_ethtool_ksettings_get. Though in that instance, state->interface
is not read. And of course this ordering isn't documented.
That being said, I will just do
if (interface != PHY_INTERFACE_MODE_SGMII ||
interface != PHY_INTERFACE_MODE_1000BASEX) {
state->link = 0;
return;
}
for next time.
>> > There has been the suggestion that we should allow in-band AN to be
>> > disabled in 1000base-X if we're in in-band mode according to the
>> > ethtool state.
>>
>> This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
>> another _encode variant? I hadn't done this here because the logic was
>> only one if statement.
>>
>> > I have a patch that adds that.
>>
>> Have you posted it?
>
> I haven't - it is a patch from Robert Hancock, "net: phylink: Support
> disabling autonegotiation for PCS". I've had it in my tree for a while,
> but I do want to make some changes to it before re-posting.
(for those following along this is [1])
OK. I'll add an _encode variant for this function in the next revision then.
[1] https://lore.kernel.org/netdev/[email protected]/
>> > You can't actually abort at this point - phylink will print the error
>> > and carry on regardless. The checking is all done via the validate()
>> > callback and if that indicates the interface mode is acceptable, then
>> > it should be accepted.
>>
>> Ok, so where can the PCS NAK an interface? This is the only callback
>> which has a return code, so I assumed this was the correct place to say
>> "no, we don't support this." This is what lynx_pcs_config does as well.
>
> At the moment, the PCS doesn't get to NAK an inappropriate interface.
> That's currently the job of the MAC's validate callback with the
> assumtion that the MAC knows what interfaces are supportable.
Which is a rather silly assumption because then you have to update the
MAC's validate function every time you add a new PCS. And this gets
messy rather fast. For example, you might want to connect your SFP
module to a MAC which only has an RGMII interface. So you put a DP83869
on your board connected like
MAC <--RGMII--> DP83869 <--SGMII--> SFP
For the moment, I think you have to just pretend that the DP83869 is the
only PHY in the system and hope that you don't need to talk to the SFP's
PHY. But if you want to use the DP83869 as a PCS, then you need to
update the MAC's validate() to allow SGMII, even though the MAC doesn't
support that without an external converter.
In an ideal world, the MAC would select its interface based on the PCS
(or lack of one), and the PCS would validate the interface mode. But of
course, there may be multiple PCSs available, so it is not so easy.
(Selecting between multiple PCSs (or no PCS at all) seems to be similar
in spirit to the PORT_XXX settings)
> Trying to do it later, once the configuration has been worked out can
> _only_ lead to a failure of some kind - in many paths, there is no way
> to report the problem except by printing a message into the kernel log.
>
> For example, by the time we reach pcs_config(), we've already prepared
> the MAC for a change to the interface, we've told the MAC to configure
> for that interface. Now the PCS rejects it - we have no record of the
> old configuration to restore. Even if we had a way to restore it, then
> we could return an error to the user - but the user doesn't get to
> control the interface themselves. If it was the result of a PHY changing
> its interface, then what - we can only log an error to the kernel log.
> If it's the result of a SFP being plugged in, we have no way to
> renegotiate.
>
> pcs_config() is too late to be making decisions about whether the
> requested configuration is acceptable or not. It needs to be done as
> part of the validation step.
Well, if these are the constraints, then IMO the PCS must have its own
validate() callback. Otherwise there is no way to tell a MAC that (for
example) supports both SGMII and 1000BASE-X that the PCS only supports
1000BASE-X. As another example, the MAC could support half duplex, but
the PCS might only suppport full duplex.
> However, the validation step is not purely just validation, but it's
> negotiation too for SFPs to be able to work out what interface mode
> they should use in combination with the support that the MAC/PCS
> offers.
>
> I do feel that the implementation around the validation/selection of
> interface for SFP etc is starting to creak, and I've some patches that
> introduce a bitmap of interface types that are supported by the various
> components. I haven't had the motivation to finish that off as my last
> attempt at making a phylink API change was not pleasant in terms of
> either help updating network drivers or getting patches tested. So I
> now try to avoid phylink API changes at all cost.
>
> People have whinged that phylink's API changes too quickly... I'm
> guessing we're now going to get other people arguing that it needs
> change to fix issues like this...
I think it should change, and I can help test things out on my own
setup, for whatever that's worth.
At the very least, it should be clearer what things are allowed to fail
for what reasons. Several callbacks are void when things can fail under
the hood (e.g. link_up or an_restart). And the API seems to have been
primarily designed around PCSs which are tightly-coupled to their MACs.
--Sean
On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> At the very least, it should be clearer what things are allowed to fail
> for what reasons. Several callbacks are void when things can fail under
> the hood (e.g. link_up or an_restart). And the API seems to have been
> primarily designed around PCSs which are tightly-coupled to their MACs.
It has indeed been designed around that, because that's where the
technology that has been available to me has been. It is only in
the recent few years that we are starting to see designs where
the PCS is separate.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Oct 05, 2021 at 11:19:45PM +0100, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> > At the very least, it should be clearer what things are allowed to fail
> > for what reasons. Several callbacks are void when things can fail under
> > the hood (e.g. link_up or an_restart). And the API seems to have been
> > primarily designed around PCSs which are tightly-coupled to their MACs.
I think what I'd like to see is rather than a validate() callback for
the PCS, a bitmap of phy_interface_t modes that the PCS supports,
which is the direction I was wanting to take phylink and SFP support.
We can then use that information to inform the interface selection in
phylink and avoid interface modes that the PCS doesn't support.
However, things get tricky when we have several PCS that we can switch
between, and the switching is done in mac_prepare(). The current PCS
(if there is even a PCS attached) may not represent the abilities
that are actually available.
It sounds easy - just throw in an extra validation step when calling
phylink_validate(), but it isn't that simple. To avoid breaking
existing setups, phylink would need to know of every PCS that _could_
be attached, and the decisions that the MAC makes to select which PCS
is going to be used for any configuration.
We could possibly introduce a .mac_select_pcs(interface) method
which the MAC could return the PCS it wishes to use for the interface
mode with the guarantee that the PCS it returns is suitable - and if
it returns NULL that means the interface mode is unsupported. That,
along with the bitmask would probably work.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Oct 07, 2021 at 11:34:06AM +0100, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 11:19:45PM +0100, Russell King (Oracle) wrote:
> > On Tue, Oct 05, 2021 at 05:44:11PM -0400, Sean Anderson wrote:
> > > At the very least, it should be clearer what things are allowed to fail
> > > for what reasons. Several callbacks are void when things can fail under
> > > the hood (e.g. link_up or an_restart). And the API seems to have been
> > > primarily designed around PCSs which are tightly-coupled to their MACs.
>
> I think what I'd like to see is rather than a validate() callback for
> the PCS, a bitmap of phy_interface_t modes that the PCS supports,
> which is the direction I was wanting to take phylink and SFP support.
>
> We can then use that information to inform the interface selection in
> phylink and avoid interface modes that the PCS doesn't support.
>
> However, things get tricky when we have several PCS that we can switch
> between, and the switching is done in mac_prepare(). The current PCS
> (if there is even a PCS attached) may not represent the abilities
> that are actually available.
>
> It sounds easy - just throw in an extra validation step when calling
> phylink_validate(), but it isn't that simple. To avoid breaking
> existing setups, phylink would need to know of every PCS that _could_
> be attached, and the decisions that the MAC makes to select which PCS
> is going to be used for any configuration.
>
> We could possibly introduce a .mac_select_pcs(interface) method
> which the MAC could return the PCS it wishes to use for the interface
> mode with the guarantee that the PCS it returns is suitable - and if
> it returns NULL that means the interface mode is unsupported. That,
> along with the bitmask would probably work.
Here's a patch which illustrates roughly what I'm thinking at the
moment - only build tested.
mac_select_pcs() should not ever fail in phylink_major_config() - that
would be a bug. I've hooked mac_select_pcs() also into the validate
function so we can catch problems there, but we will need to involve
the PCS in the interface selection for SFPs etc.
Note that mac_select_pcs() must be inconsequential - it's asking the
MAC which PCS it wishes to use for the interface mode.
I am still very much undecided whether we wish phylink to parse the
pcs-handle property and if present, override the MAC - I feel that
logic depends in the MAC driver, since a single PCS can be very
restrictive in terms of what interface modes are supportable. If the
MAC wishes pcs-handle to override its internal ones, then it can
always do:
if (port->external_pcs)
return port->external_pcs;
in its mac_select_pcs() implementation. This gives us a bit of future
flexibility.
If we parse pcs-handle in phylink, then if we end up with multiple PCS
to choose from, we then need to work out how to either allow the MAC
driver to tell phylink not to parse pcs-handle, or we need some way for
phylink to ask the MAC "these are the PCS I have, which one should I
use" which is yet another interface.
What I don't like about the patch is the need to query the PCS based on
interface - when we have a SFP plugged in, it may support multiple
interfaces. I think we still need the MAC to restrict what it returns
in its validate() method according to the group of PCS that it has
available for the SFP interface selection to work properly. Things in
this regard should become easier _if_ I can switch phylink over to
selecting interface based on phy_interface_t bitmaps rather than the
current bodge using ethtool link modes, but that needs changes to phylib
and all MAC drivers, otherwise we have to support two entirely separate
ways to select the interface mode.
My argument against that is... I'll end up converting the network
interfaces that I use to the new implementation, and the old version
will start to rot. I've already stopped testing phylink without a PCS
attached for this very reason. The more legacy code we keep, the worse
this problem becomes.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index cf8acabb90ac..ad73a488fc5f 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -1239,7 +1239,8 @@ struct mvpp2_port {
phy_interface_t phy_interface;
struct phylink *phylink;
struct phylink_config phylink_config;
- struct phylink_pcs phylink_pcs;
+ struct phylink_pcs pcs_gmac;
+ struct phylink_pcs pcs_xlg;
struct phy *comphy;
struct mvpp2_bm_pool *pool_long;
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 3197526455d9..509ff0e68e2a 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6105,15 +6105,20 @@ static struct mvpp2_port *mvpp2_phylink_to_port(struct phylink_config *config)
return container_of(config, struct mvpp2_port, phylink_config);
}
-static struct mvpp2_port *mvpp2_pcs_to_port(struct phylink_pcs *pcs)
+static struct mvpp2_port *mvpp2_pcs_xlg_to_port(struct phylink_pcs *pcs)
{
- return container_of(pcs, struct mvpp2_port, phylink_pcs);
+ return container_of(pcs, struct mvpp2_port, pcs_xlg);
+}
+
+static struct mvpp2_port *mvpp2_pcs_gmac_to_port(struct phylink_pcs *pcs)
+{
+ return container_of(pcs, struct mvpp2_port, pcs_gmac);
}
static void mvpp2_xlg_pcs_get_state(struct phylink_pcs *pcs,
struct phylink_link_state *state)
{
- struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+ struct mvpp2_port *port = mvpp2_pcs_xlg_to_port(pcs);
u32 val;
state->speed = SPEED_10000;
@@ -6148,7 +6153,7 @@ static const struct phylink_pcs_ops mvpp2_phylink_xlg_pcs_ops = {
static void mvpp2_gmac_pcs_get_state(struct phylink_pcs *pcs,
struct phylink_link_state *state)
{
- struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+ struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
u32 val;
val = readl(port->base + MVPP2_GMAC_STATUS0);
@@ -6185,7 +6190,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
const unsigned long *advertising,
bool permit_pause_to_mac)
{
- struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+ struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
u32 mask, val, an, old_an, changed;
mask = MVPP2_GMAC_IN_BAND_AUTONEG_BYPASS |
@@ -6239,7 +6244,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
static void mvpp2_gmac_pcs_an_restart(struct phylink_pcs *pcs)
{
- struct mvpp2_port *port = mvpp2_pcs_to_port(pcs);
+ struct mvpp2_port *port = mvpp2_pcs_gmac_to_port(pcs);
u32 val = readl(port->base + MVPP2_GMAC_AUTONEG_CONFIG);
writel(val | MVPP2_GMAC_IN_BAND_RESTART_AN,
@@ -6428,8 +6433,23 @@ static void mvpp2_gmac_config(struct mvpp2_port *port, unsigned int mode,
writel(ctrl4, port->base + MVPP22_GMAC_CTRL_4_REG);
}
-static int mvpp2__mac_prepare(struct phylink_config *config, unsigned int mode,
- phy_interface_t interface)
+static struct phylink_pcs *mvpp2_select_pcs(struct phylink_config *config,
+ phy_interface_t interface)
+{
+ struct mvpp2_port *port = mvpp2_phylink_to_port(config);
+
+ /* Select the appropriate PCS operations depending on the
+ * configured interface mode. We will only switch to a mode
+ * that the validate() checks have already passed.
+ */
+ if (mvpp2_is_xlg(interface))
+ return &port->pcs_xlg;
+ else
+ return &port->pcs_gmac;
+}
+
+static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
+ phy_interface_t interface)
{
struct mvpp2_port *port = mvpp2_phylink_to_port(config);
@@ -6475,31 +6495,9 @@ static int mvpp2__mac_prepare(struct phylink_config *config, unsigned int mode,
}
}
- /* Select the appropriate PCS operations depending on the
- * configured interface mode. We will only switch to a mode
- * that the validate() checks have already passed.
- */
- if (mvpp2_is_xlg(interface))
- port->phylink_pcs.ops = &mvpp2_phylink_xlg_pcs_ops;
- else
- port->phylink_pcs.ops = &mvpp2_phylink_gmac_pcs_ops;
-
return 0;
}
-static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
- phy_interface_t interface)
-{
- struct mvpp2_port *port = mvpp2_phylink_to_port(config);
- int ret;
-
- ret = mvpp2__mac_prepare(config, mode, interface);
- if (ret == 0)
- phylink_set_pcs(port->phylink, &port->phylink_pcs);
-
- return ret;
-}
-
static void mvpp2_mac_config(struct phylink_config *config, unsigned int mode,
const struct phylink_link_state *state)
{
@@ -6691,12 +6689,15 @@ static void mvpp2_acpi_start(struct mvpp2_port *port)
struct phylink_link_state state = {
.interface = port->phy_interface,
};
- mvpp2__mac_prepare(&port->phylink_config, MLO_AN_INBAND,
- port->phy_interface);
+ struct phylink_pcs *pcs;
+
+ pcs = mvpp2_select_pcs(&port->phylink_config, port->phy_interface);
+
+ mvpp2_mac_prepare(&port->phylink_config, MLO_AN_INBAND,
+ port->phy_interface);
mvpp2_mac_config(&port->phylink_config, MLO_AN_INBAND, &state);
- port->phylink_pcs.ops->pcs_config(&port->phylink_pcs, MLO_AN_INBAND,
- port->phy_interface,
- state.advertising, false);
+ pcs->ops->pcs_config(pcs, MLO_AN_INBAND, port->phy_interface,
+ state.advertising, false);
mvpp2_mac_finish(&port->phylink_config, MLO_AN_INBAND,
port->phy_interface);
mvpp2_mac_link_up(&port->phylink_config, NULL,
@@ -6945,6 +6946,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
goto err_free_port_pcpu;
}
port->phylink = phylink;
+ port->pcs_gmac.ops = &mvpp2_phylink_gmac_pcs_ops;
+ port->pcs_xlg.ops = &mvpp2_phylink_xlg_pcs_ops;
} else {
dev_warn(&pdev->dev, "Use link irqs for port#%d. FW update required\n", port->id);
port->phylink = NULL;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ab651f6197cc..89c7df0595be 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -170,8 +170,16 @@ static const char *phylink_an_mode_str(unsigned int mode)
static int phylink_validate(struct phylink *pl, unsigned long *supported,
struct phylink_link_state *state)
{
+ struct phylink_pcs *pcs;
+
pl->mac_ops->validate(pl->config, supported, state);
+ if (pl->mac_ops->mac_select_pcs) {
+ pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
+ if (!pcs)
+ return -EINVAL;
+ }
+
return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
}
@@ -462,6 +470,7 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
static void phylink_major_config(struct phylink *pl, bool restart,
const struct phylink_link_state *state)
{
+ struct phylink_pcs *pcs;
int err;
phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
@@ -476,6 +485,14 @@ static void phylink_major_config(struct phylink *pl, bool restart,
}
}
+ if (pl->mac_ops->mac_select_pcs) {
+ pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
+ if (!pcs)
+ phylink_err(pl, "mac_select_pcs unexpectedly failed\n");
+ else
+ phylink_set_pcs(pl, pcs);
+ }
+
phylink_mac_config(pl, state);
if (pl->pcs_ops) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index f7b5ed06a815..a70c28079f4f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -81,6 +81,7 @@ struct phylink_config {
/**
* struct phylink_mac_ops - MAC operations structure.
* @validate: Validate and update the link configuration.
+ * @mac_select_pcs: Select a PCS for the interface mode.
* @mac_pcs_get_state: Read the current link state from the hardware.
* @mac_prepare: prepare for a major reconfiguration of the interface.
* @mac_config: configure the MAC for the selected mode and state.
@@ -95,6 +96,8 @@ struct phylink_mac_ops {
void (*validate)(struct phylink_config *config,
unsigned long *supported,
struct phylink_link_state *state);
+ struct phylink_pcs *(*mac_select_pcs)(struct phylink_config *config,
+ phy_interface_t iface);
void (*mac_pcs_get_state)(struct phylink_config *config,
struct phylink_link_state *state);
int (*mac_prepare)(struct phylink_config *config, unsigned int mode,
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:
> Here's a patch which illustrates roughly what I'm thinking at the
> moment - only build tested.
>
> mac_select_pcs() should not ever fail in phylink_major_config() - that
> would be a bug. I've hooked mac_select_pcs() also into the validate
> function so we can catch problems there, but we will need to involve
> the PCS in the interface selection for SFPs etc.
>
> Note that mac_select_pcs() must be inconsequential - it's asking the
> MAC which PCS it wishes to use for the interface mode.
>
> I am still very much undecided whether we wish phylink to parse the
> pcs-handle property and if present, override the MAC - I feel that
> logic depends in the MAC driver, since a single PCS can be very
> restrictive in terms of what interface modes are supportable. If the
> MAC wishes pcs-handle to override its internal ones, then it can
> always do:
>
> if (port->external_pcs)
> return port->external_pcs;
>
> in its mac_select_pcs() implementation. This gives us a bit of future
> flexibility.
>
> If we parse pcs-handle in phylink, then if we end up with multiple PCS
> to choose from, we then need to work out how to either allow the MAC
> driver to tell phylink not to parse pcs-handle, or we need some way for
> phylink to ask the MAC "these are the PCS I have, which one should I
> use" which is yet another interface.
>
> What I don't like about the patch is the need to query the PCS based on
> interface - when we have a SFP plugged in, it may support multiple
> interfaces. I think we still need the MAC to restrict what it returns
> in its validate() method according to the group of PCS that it has
> available for the SFP interface selection to work properly. Things in
> this regard should become easier _if_ I can switch phylink over to
> selecting interface based on phy_interface_t bitmaps rather than the
> current bodge using ethtool link modes, but that needs changes to phylib
> and all MAC drivers, otherwise we have to support two entirely separate
> ways to select the interface mode.
>
> My argument against that is... I'll end up converting the network
> interfaces that I use to the new implementation, and the old version
> will start to rot. I've already stopped testing phylink without a PCS
> attached for this very reason. The more legacy code we keep, the worse
> this problem becomes.
Having finished off the SFP side of the phy_interface_t bitmap
(http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
and I think the mac_select_pcs() approach will work.
See commit
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
where we have a set of phy_interface_t to choose one from, and if
we add PCS selection into that logic, the loop becomes:
static phy_interface_t phylink_select_interface(struct phylink *pl,
const unsigned long *intf
const char *intf_name)
{
phy_interface_t interface, intf;
...
interface = PHY_INTERFACE_MODE_NA;
for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
intf = phylink_sfp_interface_preference[i];
if (!test_bit(intf, u))
continue;
pcs = pl->pcs;
if (pl->mac_ops->mac_select_pcs) {
pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
if (!pcs)
continue;
}
if (pcs && !test_bit(intf, pcs->supported_interfaces))
continue;
interface = intf;
break;
}
...
}
The alternative would be to move some of that logic into
phylink_sfp_config_nophy(), and will mean knocking out bits from
the mask supplied to phylink_select_interface() each time we select
an interface mode that the PCS doesn't support... which sounds rather
more yucky to me.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 10/7/21 12:23 PM, Russell King (Oracle) wrote:
> On Thu, Oct 07, 2021 at 12:29:00PM +0100, Russell King (Oracle) wrote:
>> Here's a patch which illustrates roughly what I'm thinking at the
>> moment - only build tested.
>>
>> mac_select_pcs() should not ever fail in phylink_major_config() - that
>> would be a bug. I've hooked mac_select_pcs() also into the validate
>> function so we can catch problems there, but we will need to involve
>> the PCS in the interface selection for SFPs etc.
>>
>> Note that mac_select_pcs() must be inconsequential - it's asking the
>> MAC which PCS it wishes to use for the interface mode.
>>
>> I am still very much undecided whether we wish phylink to parse the
>> pcs-handle property and if present, override the MAC - I feel that
>> logic depends in the MAC driver, since a single PCS can be very
>> restrictive in terms of what interface modes are supportable. If the
>> MAC wishes pcs-handle to override its internal ones, then it can
>> always do:
>>
>> if (port->external_pcs)
>> return port->external_pcs;
>>
>> in its mac_select_pcs() implementation. This gives us a bit of future
>> flexibility.
>>
>> If we parse pcs-handle in phylink, then if we end up with multiple PCS
>> to choose from, we then need to work out how to either allow the MAC
>> driver to tell phylink not to parse pcs-handle, or we need some way for
>> phylink to ask the MAC "these are the PCS I have, which one should I
>> use" which is yet another interface.
>>
>> What I don't like about the patch is the need to query the PCS based on
>> interface - when we have a SFP plugged in, it may support multiple
>> interfaces. I think we still need the MAC to restrict what it returns
>> in its validate() method according to the group of PCS that it has
>> available for the SFP interface selection to work properly. Things in
>> this regard should become easier _if_ I can switch phylink over to
>> selecting interface based on phy_interface_t bitmaps rather than the
>> current bodge using ethtool link modes, but that needs changes to phylib
>> and all MAC drivers, otherwise we have to support two entirely separate
>> ways to select the interface mode.
>>
>> My argument against that is... I'll end up converting the network
>> interfaces that I use to the new implementation, and the old version
>> will start to rot. I've already stopped testing phylink without a PCS
>> attached for this very reason. The more legacy code we keep, the worse
>> this problem becomes.
>
> Having finished off the SFP side of the phy_interface_t bitmap
> (http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue)
> and I think the mac_select_pcs() approach will work.
>
> See commit
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=3e0d51c361f5191111af206e3ed024d4367fce78
> where we have a set of phy_interface_t to choose one from, and if
> we add PCS selection into that logic, the loop becomes:
>
> static phy_interface_t phylink_select_interface(struct phylink *pl,
> const unsigned long *intf
> const char *intf_name)
> {
> phy_interface_t interface, intf;
>
> ...
>
> interface = PHY_INTERFACE_MODE_NA;
> for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
> intf = phylink_sfp_interface_preference[i];
>
> if (!test_bit(intf, u))
> continue;
>
> pcs = pl->pcs;
> if (pl->mac_ops->mac_select_pcs) {
> pcs = pl->mac_ops->mac_select_pcs(pl->config, intf);
> if (!pcs)
> continue;
> }
>
> if (pcs && !test_bit(intf, pcs->supported_interfaces))
> continue;
>
> interface = intf;
> break;
> }
> ...
> }
>
> The alternative would be to move some of that logic into
> phylink_sfp_config_nophy(), and will mean knocking out bits from
> the mask supplied to phylink_select_interface() each time we select
> an interface mode that the PCS doesn't support... which sounds rather
> more yucky to me.
>
With appropriate helper functions, I don't think we would need to have a
separate mac_select_pcs callback:
probe()
{
priv->pl = phylink_create();
priv->pcs1 = my_internal_pcs;
priv->pcs2 = phylink_find_pcs();
}
validate()
{
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_GMII:
phylink_set(mask, 1000baseT_Full);
phylink_set(mask, 1000baseX_Full);
if (one)
break;
fallthrough;
default:
matched = phylink_set_pcs_modes(mask, priv->pcs1, state);
matched ||= phylink_set_pcs_modes(mask, priv->pcs2, state);
if (matched)
break;
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
}
bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
}
prepare()
{
switch (state->interface) {
case PHY_INTERFACE_GMII:
enable_gmii();
break;
default:
if (phylink_attach_matching_pcs(priv->pl, priv->pcs1, state->interface)) {
enable_internal_pcs();
break;
}
if (phylink_attach_matching_pcs(priv->pl, priv->pcs2, state->interface)) {
enable_external_pcs();
break;
}
BUG();
}
}
int phylink_set_interface_mode(mask, iface)
{
/* switch statement from phylink_parse_mode here */
}
bool phylink_set_pcs_modes(mask, pcs, state)
{
if (state->interface != PHY_INTERFACE_MODE_NA) {
if (!test_bit(state->interface, pcs->supported_interfaces))
return false;
phylink_set_interface_mode(mask, state->interface);
return true;
}
for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (test_bit(i, pcs->supported_interfaces))
phylink_set_interface_mode(mask, state->interface);
}
return true;
}
bool phylink_attach_matching_pcs(pl, pcs, iface)
{
if (!test_bit(iface, pcs->supported_interfaces))
return false;
phylink_set_pcs(pl, pcs);
return true;
}
I think this has some advantages over a separate mac_select_pcs():
- In validate() you get all the available interfaces.
- The MAC can set the priority of its PCSs however it likes, which is
probably good enough for almost every case.
- The MAC doesn't care about what interfaces the PCSs actually support.
- phylink can do whatever logic it wants for selection under the hood.
- From phylink's POV, drivers behave the same way no matter whether they
use these helpers or whether they just say "If we use SGMII, then use
our internal PCS)".
--Sean