netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers
Date: Wed, 11 Mar 2020 17:09:18 +0000	[thread overview]
Message-ID: <20200311170918.GQ25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <CA+h21ho9eWTCJp2+hD0id_e3mfVXw_KRJziACJQMDXxmCnE5xA@mail.gmail.com>

On Wed, Mar 11, 2020 at 04:06:52PM +0200, Vladimir Oltean wrote:
> On Wed, 11 Mar 2020 at 14:08, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Implement helpers for PCS accessed via the MII bus using 802.3 clause
> > 22 cycles, conforming to 802.3 clause 37 and Cisco SGMII specifications
> > for the advertisement word.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 206 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/phylink.h   |   6 ++
> >  include/uapi/linux/mii.h  |   5 +
> >  3 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 19db68d74cb4..3ef20b34f5d6 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -2035,4 +2035,210 @@ void phylink_helper_basex_speed(struct phylink_link_state *state)
> >  }
> >  EXPORT_SYMBOL_GPL(phylink_helper_basex_speed);
> >
> > +static void phylink_decode_c37_word(struct phylink_link_state *state,
> > +                                   uint16_t config_reg, int speed)
> > +{
> > +       bool tx_pause, rx_pause;
> > +       int fd_bit;
> > +
> > +       if (speed == SPEED_2500)
> > +               fd_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
> > +       else
> > +               fd_bit = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> > +
> > +       mii_lpa_mod_linkmode_x(state->lp_advertising, config_reg, fd_bit);
> > +
> > +       if (linkmode_test_bit(fd_bit, state->advertising) &&
> > +           linkmode_test_bit(fd_bit, state->lp_advertising)) {
> > +               state->speed = speed;
> > +               state->duplex = DUPLEX_FULL;
> > +       } else {
> > +               /* negotiation failure */
> > +               state->link = false;
> > +       }
> > +
> > +       linkmode_resolve_pause(state->advertising, state->lp_advertising,
> > +                              &tx_pause, &rx_pause);
> > +
> > +       if (tx_pause)
> > +               state->pause |= MLO_PAUSE_TX;
> > +       if (rx_pause)
> > +               state->pause |= MLO_PAUSE_RX;
> > +}
> > +
> > +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> > +                                     uint16_t config_reg)
> > +{
> > +       if (!(config_reg & LPA_SGMII_LINK)) {
> > +               state->link = false;
> > +               return;
> > +       }
> > +
> > +       switch (config_reg & LPA_SGMII_SPD_MASK) {
> 
> Did you consider using or adapting the mii_lpa_mod_linkmode_adv_sgmii helper?

Yes, and we _do not_ want to be touching state->lp_advertising here.
The link partner advertisement comes from the attached PHY, not from
this information.

The config_reg information is not an advertisement - it is a
_configuration word_ coming from the PHY to tell the MAC what speed
and duplex to operate at.  It conveys nothing about whether it's
1000baseT or 1000baseX, so link modes mean nothing as far as a SGMII
configuration word is concerned.

Hence, _this_ is a more correct implementation than
mii_lpa_mod_linkmode_adv_sgmii().

> > +       case LPA_SGMII_10:
> > +               state->speed = SPEED_10;
> > +               break;
> > +       case LPA_SGMII_100:
> > +               state->speed = SPEED_100;
> > +               break;
> > +       case LPA_SGMII_1000:
> > +               state->speed = SPEED_1000;
> > +               break;
> > +       default:
> > +               state->link = false;
> > +               return;
> > +       }
> > +       if (config_reg & LPA_SGMII_FULL_DUPLEX)
> > +               state->duplex = DUPLEX_FULL;
> > +       else
> > +               state->duplex = DUPLEX_HALF;
> > +}
> > +
> > +/**
> > + * phylink_mii_c22_pcs_get_state() - read the MAC PCS state
> > + * @pcs: a pointer to a &struct mdio_device.
> > + * @state: a pointer to a &struct phylink_link_state.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation and/or SGMII control.
> > + *
> > + * Read the MAC PCS state from the MII device configured in @config and
> > + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> > + * the phylink @state structure. This is suitable to be directly plugged
> > + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> > + * structure.
> > + */
> > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > +                                  struct phylink_link_state *state)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int addr = pcs->addr;
> > +       int bmsr, lpa;
> > +
> > +       bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > +       lpa = mdiobus_read(bus, addr, MII_LPA);
> > +       if (bmsr < 0 || lpa < 0) {
> > +               state->link = false;
> > +               return;
> > +       }
> > +
> > +       state->link = !!(bmsr & BMSR_LSTATUS);
> > +       state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > +       if (!state->link)
> > +               return;
> > +
> > +       switch (state->interface) {
> > +       case PHY_INTERFACE_MODE_1000BASEX:
> > +               phylink_decode_c37_word(state, lpa, SPEED_1000);
> > +               break;
> > +
> > +       case PHY_INTERFACE_MODE_2500BASEX:
> > +               phylink_decode_c37_word(state, lpa, SPEED_2500);
> > +               break;
> > +
> > +       case PHY_INTERFACE_MODE_SGMII:
> > +               phylink_decode_sgmii_word(state, lpa);
> > +               break;
> > +
> > +       default:
> > +               state->link = false;
> > +               break;
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_get_state);
> > +
> > +/**
> > + * phylink_mii_c22_pcs_set_advertisement() - configure the clause 37 PCS
> > + *     advertisement
> > + * @pcs: a pointer to a &struct mdio_device.
> > + * @state: a pointer to the state being configured.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation and/or SGMII control.
> > + *
> > + * Configure the clause 37 PCS advertisement as specified by @state. This
> > + * does not trigger a renegotiation; phylink will do that via the
> > + * mac_an_restart() method of the struct phylink_mac_ops structure.
> > + *
> > + * Returns negative error code on failure to configure the advertisement,
> > + * zero if no change has been made, or one if the advertisement has changed.
> > + */
> > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > +                                       const struct phylink_link_state *state)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int addr = pcs->addr;
> > +       int val, ret;
> > +       u16 adv;
> > +
> > +       switch (state->interface) {
> > +       case PHY_INTERFACE_MODE_1000BASEX:
> > +       case PHY_INTERFACE_MODE_2500BASEX:
> > +               adv = ADVERTISE_1000XFULL;
> > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +                                     state->advertising))
> > +                       adv |= ADVERTISE_1000XPAUSE;
> > +               if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +                                     state->advertising))
> > +                       adv |= ADVERTISE_1000XPSE_ASYM;
> > +
> > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > +               if (val < 0)
> > +                       return val;
> > +
> > +               if (val == adv)
> > +                       return 0;
> > +
> > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               return 1;
> > +
> > +       case PHY_INTERFACE_MODE_SGMII:
> > +               val = mdiobus_read(bus, addr, MII_ADVERTISE);
> > +               if (val < 0)
> > +                       return val;
> > +
> > +               if (val == 0x0001)
> > +                       return 0;
> > +
> > +               ret = mdiobus_write(bus, addr, MII_ADVERTISE, 0x0001);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               return 1;
> > +
> > +       default:
> > +               /* Nothing to do for other modes */
> > +               return 0;
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_set_advertisement);
> > +
> > +/**
> > + * phylink_mii_c22_pcs_an_restart() - restart 802.3z autonegotiation
> > + * @pcs: a pointer to a &struct mdio_device.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 clause 22 register set for
> > + * clause 37 negotiation.
> > + *
> > + * Restart the clause 37 negotiation with the link partner. This is
> > + * suitable to be directly plugged into the mac_pcs_get_state() member
> > + * of the struct phylink_mac_ops structure.
> > + */
> > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
> > +{
> > +       struct mii_bus *bus = pcs->bus;
> > +       int val, addr = pcs->addr;
> > +
> > +       val = mdiobus_read(bus, addr, MII_BMCR);
> > +       if (val >= 0) {
> > +               val |= BMCR_ANRESTART;
> > +
> > +               mdiobus_write(bus, addr, MII_BMCR, val);
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 2180eb1aa254..de591c2fb37e 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -317,4 +317,10 @@ int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
> >  void phylink_set_port_modes(unsigned long *bits);
> >  void phylink_helper_basex_speed(struct phylink_link_state *state);
> >
> > +void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
> > +                                  struct phylink_link_state *state);
> > +int phylink_mii_c22_pcs_set_advertisement(struct mdio_device *pcs,
> > +                                       const struct phylink_link_state *state);
> > +void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
> > +
> >  #endif
> > diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> > index 0b9c3beda345..90f9b4e1ba27 100644
> > --- a/include/uapi/linux/mii.h
> > +++ b/include/uapi/linux/mii.h
> > @@ -134,11 +134,16 @@
> >  /* MAC and PHY tx_config_Reg[15:0] for SGMII in-band auto-negotiation.*/
> >  #define ADVERTISE_SGMII                0x0001  /* MAC can do SGMII            */
> >  #define LPA_SGMII              0x0001  /* PHY can do SGMII            */
> > +#define LPA_SGMII_SPD_MASK     0x0c00  /* SGMII speed mask            */
> > +#define LPA_SGMII_FULL_DUPLEX  0x1000  /* SGMII full duplex           */
> >  #define LPA_SGMII_DPX_SPD_MASK 0x1C00  /* SGMII duplex and speed bits */
> > +#define LPA_SGMII_10           0x0000  /* 10Mbps                      */
> >  #define LPA_SGMII_10HALF       0x0000  /* Can do 10mbps half-duplex   */
> >  #define LPA_SGMII_10FULL       0x1000  /* Can do 10mbps full-duplex   */
> > +#define LPA_SGMII_100          0x0400  /* 100Mbps                     */
> >  #define LPA_SGMII_100HALF      0x0400  /* Can do 100mbps half-duplex  */
> >  #define LPA_SGMII_100FULL      0x1400  /* Can do 100mbps full-duplex  */
> > +#define LPA_SGMII_1000         0x0800  /* 1000Mbps                    */
> 
> These seem a bit mixed up to say the least.
> If you're not going to use the (minimal) existing infrastructure could
> you also refactor the one existing user? I think it would be better
> than just keeping adding conflicting definitions.

Sorry, but you need to explain better what you would like to see here.
The additions I'm adding are to the SGMII specification; I find your
existing definitions to be obscure because they conflate two different
bit fields together to produce something for the ethtool linkmodes
(which I think is a big mistake.)

> 
> >  #define LPA_SGMII_1000HALF     0x0800  /* Can do 1000mbps half-duplex */
> >  #define LPA_SGMII_1000FULL     0x1800  /* Can do 1000mbps full-duplex */
> >  #define LPA_SGMII_LINK         0x8000  /* PHY link with copper-side partner */
> > --
> > 2.20.1
> >
> 
> Regards,
> -Vladimir
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

  reply	other threads:[~2020-03-11 17:09 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-11 12:06 [PATCH net-next 0/5] add phylink support for PCS Russell King - ARM Linux admin
2020-03-11 12:07 ` [PATCH net-next 1/5] net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant Russell King
2020-03-11 12:07 ` [PATCH net-next 2/5] net: mii: add linkmode_adv_to_mii_adv_x() Russell King
2020-03-11 12:07 ` [PATCH net-next 3/5] net: mdiobus: add APIs for modifying a MDIO device register Russell King
2020-03-11 12:07 ` [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers Russell King
2020-03-11 14:06   ` Vladimir Oltean
2020-03-11 17:09     ` Russell King - ARM Linux admin [this message]
2020-03-11 18:44       ` Vladimir Oltean
2020-03-11 19:32         ` Russell King - ARM Linux admin
2020-03-11 19:59           ` Vladimir Oltean
2020-03-11 20:32             ` Russell King - ARM Linux admin
2020-03-12 12:54               ` Vladimir Oltean
2020-03-12 13:13                 ` Russell King - ARM Linux admin
2020-03-12 13:35                   ` Vladimir Oltean
2020-03-11 12:07 ` [PATCH net-next 5/5] net: phylink: pcs: add 802.3 clause 45 helpers Russell King
2020-03-11 12:46 ` [PATCH net-next 0/5] add phylink support for PCS Vladimir Oltean
2020-03-11 12:54   ` Russell King - ARM Linux admin
2020-03-11 13:57     ` Vladimir Oltean
2020-03-11 17:05       ` Russell King - ARM Linux admin
2020-03-11 18:16         ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200311170918.GQ25745@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --subject='Re: [PATCH net-next 4/5] net: phylink: pcs: add 802.3 clause 22 helpers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).