This patch replaces the adjust_link api with the phylink apis that provide
equivalent functionality.
The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_up api.
Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_up
Signed-off-by: Jerry Ray <[email protected]>
---
v5->v6:
- Moved to using port number to identify xMII port for the LAN9303.
v4->v5:
- Added various prep patches to better show the movement of the logic.
v3->v4:
- Reworked the implementation to preserve the adjust_link functionality
by including it in the phylink_mac_link_up api.
v2->v3:
Added back in disabling Turbo Mode on the CPU MII interface.
Removed the unnecessary clearing of the phy supported interfaces.
---
drivers/net/dsa/lan9303-core.c | 101 ++++++++++++++++++++++-----------
1 file changed, 69 insertions(+), 32 deletions(-)
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 7be4c491e5d9..e514fff81af6 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
return chip->ops->phy_write(chip, phy, regnum, val);
}
-static void lan9303_adjust_link(struct dsa_switch *ds, int port,
- struct phy_device *phydev)
-{
- int ctl;
-
- /* On this device, we are only interested in doing something here if
- * this is an xMII port. All other ports are 10/100 phys using MDIO
- * to control there link settings.
- */
- if (port != 0)
- return;
-
- ctl = lan9303_phy_read(ds, port, MII_BMCR);
-
- ctl &= ~BMCR_ANENABLE;
-
- if (phydev->speed == SPEED_100)
- ctl |= BMCR_SPEED100;
- else if (phydev->speed == SPEED_10)
- ctl &= ~BMCR_SPEED100;
- else
- dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
-
- if (phydev->duplex == DUPLEX_FULL)
- ctl |= BMCR_FULLDPLX;
- else
- ctl &= ~BMCR_FULLDPLX;
-
- lan9303_phy_write(ds, port, MII_BMCR, ctl);
-}
-
static int lan9303_port_enable(struct dsa_switch *ds, int port,
struct phy_device *phy)
{
@@ -1285,13 +1254,81 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
return 0;
}
+static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
+ struct phylink_config *config)
+{
+ struct lan9303 *chip = ds->priv;
+
+ dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
+
+ config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
+ MAC_SYM_PAUSE;
+
+ if (port == 0) {
+ __set_bit(PHY_INTERFACE_MODE_RMII,
+ config->supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_MII,
+ config->supported_interfaces);
+ } else {
+ __set_bit(PHY_INTERFACE_MODE_INTERNAL,
+ config->supported_interfaces);
+ /* Compatibility for phylib's default interface type when the
+ * phy-mode property is absent
+ */
+ __set_bit(PHY_INTERFACE_MODE_GMII,
+ config->supported_interfaces);
+ }
+
+ /* This driver does not make use of the speed, duplex, pause or the
+ * advertisement in its mac_config, so it is safe to mark this driver
+ * as non-legacy.
+ */
+ config->legacy_pre_march2020 = false;
+}
+
+static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev, int speed,
+ int duplex, bool tx_pause,
+ bool rx_pause)
+{
+ u32 ctl;
+
+ /* On this device, we are only interested in doing something here if
+ * this is the xMII port. All other ports are 10/100 phys using MDIO
+ * to control there link settings.
+ */
+ if (port != 0)
+ return;
+
+ ctl = lan9303_phy_read(ds, port, MII_BMCR);
+
+ ctl &= ~BMCR_ANENABLE;
+
+ if (speed == SPEED_100)
+ ctl |= BMCR_SPEED100;
+ else if (speed == SPEED_10)
+ ctl &= ~BMCR_SPEED100;
+ else
+ dev_err(ds->dev, "unsupported speed: %d\n", speed);
+
+ if (duplex == DUPLEX_FULL)
+ ctl |= BMCR_FULLDPLX;
+ else
+ ctl &= ~BMCR_FULLDPLX;
+
+ lan9303_phy_write(ds, port, MII_BMCR, ctl);
+}
+
static const struct dsa_switch_ops lan9303_switch_ops = {
.get_tag_protocol = lan9303_get_tag_protocol,
.setup = lan9303_setup,
.get_strings = lan9303_get_strings,
.phy_read = lan9303_phy_read,
.phy_write = lan9303_phy_write,
- .adjust_link = lan9303_adjust_link,
+ .phylink_get_caps = lan9303_phylink_get_caps,
+ .phylink_mac_link_up = lan9303_phylink_mac_link_up,
.get_ethtool_stats = lan9303_get_ethtool_stats,
.get_sset_count = lan9303_get_sset_count,
.port_enable = lan9303_port_enable,
--
2.17.1
Hi Jerry,
On Mon, 2023-01-09 at 15:18 -0600, Jerry Ray wrote:
>
> diff --git a/drivers/net/dsa/lan9303-core.c
> b/drivers/net/dsa/lan9303-core.c
> index 7be4c491e5d9..e514fff81af6 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch
> *ds, int phy, int regnum,
> return chip->ops->phy_write(chip, phy, regnum, val);
> }
>
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int
> port,
> + struct phylink_config *config)
> +{
> + struct lan9303 *chip = ds->priv;
> +
> + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> + MAC_SYM_PAUSE;
> +
> + if (port == 0) {
> + __set_bit(PHY_INTERFACE_MODE_RMII,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_MII,
> + config->supported_interfaces);
> + } else {
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + config->supported_interfaces);
> + /* Compatibility for phylib's default interface type
> when the
> + * phy-mode property is absent
> + */
> + __set_bit(PHY_INTERFACE_MODE_GMII,
> + config->supported_interfaces);
> + }
> +
> + /* This driver does not make use of the speed, duplex, pause or
> the
> + * advertisement in its mac_config, so it is safe to mark this
> driver
> + * as non-legacy.
> + */
> + config->legacy_pre_march2020 = false;
> +}
> +
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int
> port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev, int
> speed,
> + int duplex, bool tx_pause,
> + bool rx_pause)
> +{
> + u32 ctl;
> +
> + /* On this device, we are only interested in doing something
> here if
> + * this is the xMII port. All other ports are 10/100 phys using
> MDIO
> + * to control there link settings.
> + */
> + if (port != 0)
> + return;
> +
> + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> + ctl &= ~BMCR_ANENABLE;
> +
> + if (speed == SPEED_100)
> + ctl |= BMCR_SPEED100;
> + else if (speed == SPEED_10)
> + ctl &= ~BMCR_SPEED100;
> + else
> + dev_err(ds->dev, "unsupported speed: %d\n", speed);
I think, We will not reach in the error part, since in the
phylink_get_caps we specified only 10 and 100 Mbps speed. Phylink layer
will validate and if the value is beyond the speed supported, it will
return back.
> +
> + if (duplex == DUPLEX_FULL)
> + ctl |= BMCR_FULLDPLX;
> + else
> + ctl &= ~BMCR_FULLDPLX;
> +
> + lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +}
> +
>
On Mon, Jan 09, 2023 at 03:18:49PM -0600, Jerry Ray wrote:
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> + struct phylink_config *config)
> +{
> + struct lan9303 *chip = ds->priv;
> +
> + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> + MAC_SYM_PAUSE;
You indicate that pause modes are supported, but...
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev, int speed,
> + int duplex, bool tx_pause,
> + bool rx_pause)
> +{
> + u32 ctl;
> +
> + /* On this device, we are only interested in doing something here if
> + * this is the xMII port. All other ports are 10/100 phys using MDIO
> + * to control there link settings.
> + */
> + if (port != 0)
> + return;
> +
> + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> + ctl &= ~BMCR_ANENABLE;
> +
> + if (speed == SPEED_100)
> + ctl |= BMCR_SPEED100;
> + else if (speed == SPEED_10)
> + ctl &= ~BMCR_SPEED100;
> + else
> + dev_err(ds->dev, "unsupported speed: %d\n", speed);
> +
> + if (duplex == DUPLEX_FULL)
> + ctl |= BMCR_FULLDPLX;
> + else
> + ctl &= ~BMCR_FULLDPLX;
> +
> + lan9303_phy_write(ds, port, MII_BMCR, ctl);
There is no code here to program the resolved pause modes. Is it handled
internally within the switch? (Please add a comment to this effect
either in get_caps or here.)
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > + struct phylink_config *config)
> > +{
> > + struct lan9303 *chip = ds->priv;
> > +
> > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > +
> > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > + MAC_SYM_PAUSE;
>
> You indicate that pause modes are supported, but...
>
> > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > + unsigned int mode,
> > + phy_interface_t interface,
> > + struct phy_device *phydev, int speed,
> > + int duplex, bool tx_pause,
> > + bool rx_pause)
> > +{
> > + u32 ctl;
> > +
> > + /* On this device, we are only interested in doing something here if
> > + * this is the xMII port. All other ports are 10/100 phys using MDIO
> > + * to control there link settings.
> > + */
> > + if (port != 0)
> > + return;
> > +
> > + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > +
> > + ctl &= ~BMCR_ANENABLE;
> > +
> > + if (speed == SPEED_100)
> > + ctl |= BMCR_SPEED100;
> > + else if (speed == SPEED_10)
> > + ctl &= ~BMCR_SPEED100;
> > + else
> > + dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > +
> > + if (duplex == DUPLEX_FULL)
> > + ctl |= BMCR_FULLDPLX;
> > + else
> > + ctl &= ~BMCR_FULLDPLX;
> > +
> > + lan9303_phy_write(ds, port, MII_BMCR, ctl);
>
> There is no code here to program the resolved pause modes. Is it handled
> internally within the switch? (Please add a comment to this effect
> either in get_caps or here.)
>
> Thanks.
>
As I look into this, the part does have control flags for advertising
Symmetric and Asymmetric pause toward the link partner. The default is set
by a configuration strap on power-up. I am having trouble mapping the rx and
tx pause parameters into symmetric and asymmetric controls. Where can I find
the proper definitions and mappings?
ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
if(tx_pause)
ctl |= ADVERTISE_PAUSE_CAP;
if(rx_pause)
ctl |= ADVERTISE_PAUSE_AYM;
If I can pause my transmissions (receive pause requests), then advertise
symmetric whether I ever sent pause requests or not.
If I can send pause requests (using flow control on my receive side), then make
sure asymmetric support is also advertised as rx_pause might have been clear.
Not that if both rx and tx pause is supported, we can support either symmetric
or asymmetric operating modes.
If I receive a pause request, it affects my transmit data flow. So one could
argue the rx_pause flag controls my ability to receive pause requests. I tend
to overthink and almost always get these 50/50 propositions wrong.
Regards,
Jerry
> > diff --git a/drivers/net/dsa/lan9303-core.c
> > b/drivers/net/dsa/lan9303-core.c
> > index 7be4c491e5d9..e514fff81af6 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch
> > *ds, int phy, int regnum,
> > return chip->ops->phy_write(chip, phy, regnum, val);
> > }
> >
> > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int
> > port,
> > + struct phylink_config *config)
> > +{
> > + struct lan9303 *chip = ds->priv;
> > +
> > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > +
> > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > + MAC_SYM_PAUSE;
> > +
> > + if (port == 0) {
> > + __set_bit(PHY_INTERFACE_MODE_RMII,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_MII,
> > + config->supported_interfaces);
> > + } else {
> > + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > + config->supported_interfaces);
> > + /* Compatibility for phylib's default interface type
> > when the
> > + * phy-mode property is absent
> > + */
> > + __set_bit(PHY_INTERFACE_MODE_GMII,
> > + config->supported_interfaces);
> > + }
> > +
> > + /* This driver does not make use of the speed, duplex, pause or
> > the
> > + * advertisement in its mac_config, so it is safe to mark this
> > driver
> > + * as non-legacy.
> > + */
> > + config->legacy_pre_march2020 = false;
> > +}
> > +
> > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int
> > port,
> > + unsigned int mode,
> > + phy_interface_t interface,
> > + struct phy_device *phydev, int
> > speed,
> > + int duplex, bool tx_pause,
> > + bool rx_pause)
> > +{
> > + u32 ctl;
> > +
> > + /* On this device, we are only interested in doing something
> > here if
> > + * this is the xMII port. All other ports are 10/100 phys using
> > MDIO
> > + * to control there link settings.
> > + */
> > + if (port != 0)
> > + return;
> > +
> > + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > +
> > + ctl &= ~BMCR_ANENABLE;
> > +
> > + if (speed == SPEED_100)
> > + ctl |= BMCR_SPEED100;
> > + else if (speed == SPEED_10)
> > + ctl &= ~BMCR_SPEED100;
> > + else
> > + dev_err(ds->dev, "unsupported speed: %d\n", speed);
>
> I think, We will not reach in the error part, since in the
> phylink_get_caps we specified only 10 and 100 Mbps speed. Phylink layer
> will validate and if the value is beyond the speed supported, it will
> return back.
>
I will remove the error check and will go with either 10 or 100.
ctl &= ~BMCR_SPEED100;
if ((speed == SPEED_100)
ctl |= BMCR_SPEED100;
Regards,
Jerry.
> > +
> > + if (duplex == DUPLEX_FULL)
> > + ctl |= BMCR_FULLDPLX;
> > + else
> > + ctl &= ~BMCR_FULLDPLX;
> > +
> > + lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > +}
> > +
On Mon, Jan 16, 2023 at 05:22:05PM +0000, [email protected] wrote:
> > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > + struct phylink_config *config)
> > > +{
> > > + struct lan9303 *chip = ds->priv;
> > > +
> > > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > +
> > > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > + MAC_SYM_PAUSE;
> >
> > You indicate that pause modes are supported, but...
> >
> > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > + unsigned int mode,
> > > + phy_interface_t interface,
> > > + struct phy_device *phydev, int speed,
> > > + int duplex, bool tx_pause,
> > > + bool rx_pause)
> > > +{
> > > + u32 ctl;
> > > +
> > > + /* On this device, we are only interested in doing something here if
> > > + * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > + * to control there link settings.
> > > + */
> > > + if (port != 0)
> > > + return;
> > > +
> > > + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > +
> > > + ctl &= ~BMCR_ANENABLE;
> > > +
> > > + if (speed == SPEED_100)
> > > + ctl |= BMCR_SPEED100;
> > > + else if (speed == SPEED_10)
> > > + ctl &= ~BMCR_SPEED100;
> > > + else
> > > + dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > +
> > > + if (duplex == DUPLEX_FULL)
> > > + ctl |= BMCR_FULLDPLX;
> > > + else
> > > + ctl &= ~BMCR_FULLDPLX;
> > > +
> > > + lan9303_phy_write(ds, port, MII_BMCR, ctl);
> >
> > There is no code here to program the resolved pause modes. Is it handled
> > internally within the switch? (Please add a comment to this effect
> > either in get_caps or here.)
> >
> > Thanks.
> >
>
> As I look into this, the part does have control flags for advertising
> Symmetric and Asymmetric pause toward the link partner. The default is set
> by a configuration strap on power-up. I am having trouble mapping the rx and
> tx pause parameters into symmetric and asymmetric controls. Where can I find
> the proper definitions and mappings?
>
> ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
> if(tx_pause)
> ctl |= ADVERTISE_PAUSE_CAP;
> if(rx_pause)
> ctl |= ADVERTISE_PAUSE_AYM;
lan9303_phylink_mac_link_up() has nothing to do with what might be
advertised to the link partner - this is called when the link has been
negotiated and established, and it's purpose is to program the results
of the resolution into the MAC.
That means programming the MAC to operate at the negotiated speed and
duplex, and also permitting the MAC to generate pause frames when its
receive side becomes full (tx_pause) and whether to act on pause frames
received over the network (rx_pause).
If there's nowhere to program the MAC to accept and/or generate pause
frames, how are they controlled? Does the MAC always accept and/or
generate them? Or does the MAC always ignore them and never generates
them?
If the latter, then that suggests pause frames are not supported, and
thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
method.
This leads me on to another question - in the above quoted code, what
device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
it a PCS? If it is, please use the phylink_pcs support, as the
pcs_config() method gives you what is necessary to program the PCS
advertisement.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > > + struct phylink_config *config)
> > > > +{
> > > > + struct lan9303 *chip = ds->priv;
> > > > +
> > > > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > > +
> > > > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > > + MAC_SYM_PAUSE;
> > >
> > > You indicate that pause modes are supported, but...
> > >
> > > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > > + unsigned int mode,
> > > > + phy_interface_t interface,
> > > > + struct phy_device *phydev, int speed,
> > > > + int duplex, bool tx_pause,
> > > > + bool rx_pause)
> > > > +{
> > > > + u32 ctl;
> > > > +
> > > > + /* On this device, we are only interested in doing something here if
> > > > + * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > > + * to control there link settings.
> > > > + */
> > > > + if (port != 0)
> > > > + return;
> > > > +
> > > > + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > > +
> > > > + ctl &= ~BMCR_ANENABLE;
> > > > +
> > > > + if (speed == SPEED_100)
> > > > + ctl |= BMCR_SPEED100;
> > > > + else if (speed == SPEED_10)
> > > > + ctl &= ~BMCR_SPEED100;
> > > > + else
> > > > + dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > > +
> > > > + if (duplex == DUPLEX_FULL)
> > > > + ctl |= BMCR_FULLDPLX;
> > > > + else
> > > > + ctl &= ~BMCR_FULLDPLX;
> > > > +
> > > > + lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > >
> > > There is no code here to program the resolved pause modes. Is it handled
> > > internally within the switch? (Please add a comment to this effect
> > > either in get_caps or here.)
> > >
> > > Thanks.
> > >
> >
> > As I look into this, the part does have control flags for advertising
> > Symmetric and Asymmetric pause toward the link partner. The default is set
> > by a configuration strap on power-up. I am having trouble mapping the rx and
> > tx pause parameters into symmetric and asymmetric controls. Where can I find
> > the proper definitions and mappings?
> >
> > ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
> > if(tx_pause)
> > ctl |= ADVERTISE_PAUSE_CAP;
> > if(rx_pause)
> > ctl |= ADVERTISE_PAUSE_AYM;
>
> lan9303_phylink_mac_link_up() has nothing to do with what might be
> advertised to the link partner - this is called when the link has been
> negotiated and established, and it's purpose is to program the results
> of the resolution into the MAC.
>
> That means programming the MAC to operate at the negotiated speed and
> duplex, and also permitting the MAC to generate pause frames when its
> receive side becomes full (tx_pause) and whether to act on pause frames
> received over the network (rx_pause).
>
> If there's nowhere to program the MAC to accept and/or generate pause
> frames, how are they controlled? Does the MAC always accept and/or
> generate them? Or does the MAC always ignore them and never generates
> them?
>
> If the latter, then that suggests pause frames are not supported, and
> thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
> method.
>
> This leads me on to another question - in the above quoted code, what
> device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
> it a PCS? If it is, please use the phylink_pcs support, as the
> pcs_config() method gives you what is necessary to program the PCS
> advertisement.
>
> Thanks.
>
> --
On this device, the XMII connection is the rev-xmii port connected to the CPU.
This is the DSA port. This device 'emulates' a phy (virtual phy) allowing the
CPU to use standard phy registers to set things up.
Let me back up for a moment.
The device supports half-duplex BackPressure as well as full-duplex Flow
Control.
The device has bootstrapping options that will configure the Settings for
BP and FC. On port 0, these strapping options also affect the Virtual Phys
Auto-Negotiation Link Partner Base Page Ability Register.
If auto-negotiation is enabled, the flow control is enabled/disabled based
on the Sym/Asym settings of the Advertised and Link Partner's capabilities
registers.
If Manual Flow Control is enabled, then flow control is programmed into the
Manual_FC_0 register directly and the auto-neg registers are ignored. The
device can be strapped to use (default to) the Manual FC register.
So this is why I'm trying to reflect the flow control settings as provided in
the mac_link_up hook api into the emulated phy's aneg settings.
Question: In the get capabilities API, should I report the device's
flow control capabilities independent of how the device is bootstrapped or
should I reflect the bootstrapped settings? I consider the bootstrap setting
to affect the register default rather than limit what the device is physically
capable of supporting.
Thanks for helping me get this right.
Regards,
Jerry.
On Mon, Jan 16, 2023 at 10:48:47PM +0000, [email protected] wrote:
> > > > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > > > + struct phylink_config *config)
> > > > > +{
> > > > > + struct lan9303 *chip = ds->priv;
> > > > > +
> > > > > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > > > +
> > > > > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > > > + MAC_SYM_PAUSE;
> > > >
> > > > You indicate that pause modes are supported, but...
> > > >
> > > > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > > > + unsigned int mode,
> > > > > + phy_interface_t interface,
> > > > > + struct phy_device *phydev, int speed,
> > > > > + int duplex, bool tx_pause,
> > > > > + bool rx_pause)
> > > > > +{
> > > > > + u32 ctl;
> > > > > +
> > > > > + /* On this device, we are only interested in doing something here if
> > > > > + * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > > > + * to control there link settings.
> > > > > + */
> > > > > + if (port != 0)
> > > > > + return;
> > > > > +
> > > > > + ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > > > +
> > > > > + ctl &= ~BMCR_ANENABLE;
> > > > > +
> > > > > + if (speed == SPEED_100)
> > > > > + ctl |= BMCR_SPEED100;
> > > > > + else if (speed == SPEED_10)
> > > > > + ctl &= ~BMCR_SPEED100;
> > > > > + else
> > > > > + dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > > > +
> > > > > + if (duplex == DUPLEX_FULL)
> > > > > + ctl |= BMCR_FULLDPLX;
> > > > > + else
> > > > > + ctl &= ~BMCR_FULLDPLX;
> > > > > +
> > > > > + lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > > >
> > > > There is no code here to program the resolved pause modes. Is it handled
> > > > internally within the switch? (Please add a comment to this effect
> > > > either in get_caps or here.)
> > > >
> > > > Thanks.
> > > >
> > >
> > > As I look into this, the part does have control flags for advertising
> > > Symmetric and Asymmetric pause toward the link partner. The default is set
> > > by a configuration strap on power-up. I am having trouble mapping the rx and
> > > tx pause parameters into symmetric and asymmetric controls. Where can I find
> > > the proper definitions and mappings?
> > >
> > > ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
> > > if(tx_pause)
> > > ctl |= ADVERTISE_PAUSE_CAP;
> > > if(rx_pause)
> > > ctl |= ADVERTISE_PAUSE_AYM;
> >
> > lan9303_phylink_mac_link_up() has nothing to do with what might be
> > advertised to the link partner - this is called when the link has been
> > negotiated and established, and it's purpose is to program the results
> > of the resolution into the MAC.
> >
> > That means programming the MAC to operate at the negotiated speed and
> > duplex, and also permitting the MAC to generate pause frames when its
> > receive side becomes full (tx_pause) and whether to act on pause frames
> > received over the network (rx_pause).
> >
> > If there's nowhere to program the MAC to accept and/or generate pause
> > frames, how are they controlled? Does the MAC always accept and/or
> > generate them? Or does the MAC always ignore them and never generates
> > them?
> >
> > If the latter, then that suggests pause frames are not supported, and
> > thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
> > method.
> >
> > This leads me on to another question - in the above quoted code, what
> > device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
> > it a PCS? If it is, please use the phylink_pcs support, as the
> > pcs_config() method gives you what is necessary to program the PCS
> > advertisement.
> >
> > Thanks.
> >
> > --
>
> On this device, the XMII connection is the rev-xmii port connected to the CPU.
> This is the DSA port. This device 'emulates' a phy (virtual phy) allowing the
> CPU to use standard phy registers to set things up.
>
> Let me back up for a moment.
> The device supports half-duplex BackPressure as well as full-duplex Flow
> Control.
> The device has bootstrapping options that will configure the Settings for
> BP and FC. On port 0, these strapping options also affect the Virtual Phys
> Auto-Negotiation Link Partner Base Page Ability Register.
> If auto-negotiation is enabled, the flow control is enabled/disabled based
> on the Sym/Asym settings of the Advertised and Link Partner's capabilities
> registers.
> If Manual Flow Control is enabled, then flow control is programmed into the
> Manual_FC_0 register directly and the auto-neg registers are ignored. The
> device can be strapped to use (default to) the Manual FC register.
>
> So this is why I'm trying to reflect the flow control settings as provided in
> the mac_link_up hook api into the emulated phy's aneg settings.
But it's wrong to be trying to do that.
The advertisement (in other words _our_ capabilities) should be
configured at one of the _config() stages - which includes the speed,
duplex and pause capabilities.
When the link partner wants to connect, the advertisement is exchanged
between each ends, and these advertisements are then used to determine
the final properties of the link. At this point, the link comes up,
and the link_up() methods will be called.
If, in the link_up() method, you want to change the advertisement, then
you would need to program that, and _then_ trigger a renegotiation of
the link, which would cause the link to go down. The above process would
be repeated, and ultimately link_up() would be called again. You'd then
reprogram the advertisement and trigger another renegotiation, and the
link would go down, up, down, up, down, up indefinitely.
> Question: In the get capabilities API, should I report the device's
> flow control capabilities independent of how the device is bootstrapped or
> should I reflect the bootstrapped settings? I consider the bootstrap setting
> to affect the register default rather than limit what the device is physically
> capable of supporting.
I would suggest that the bootstrapping should in this case be reflected
in the MAC_*_PAUSE settings passed to phylink, so phylink knows how that
is configured and should end up with the same resolution as the
hardware. Things can go wrong if ethtool is then used to force manual
pause settings, but in such a case, you will be provided with the new
advertisement using the standard algorithm for determining the ASYM and
SYM bits that the kernel uses (which is not perfect, since it's
ambiguous.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!