Messages in this thread | | | Subject | Re: [PATCH V4 net-next] net: marvell: prestera: add phylink support | From | Paolo Abeni <> | Date | Tue, 19 Jul 2022 10:53:05 +0200 |
| |
On Fri, 2022-07-15 at 22:34 +0300, Oleksandr Mazur wrote: > For SFP port prestera driver will use kernel > phylink infrastucture to configure port mode based on > the module that has beed inserted > > Co-developed-by: Yevhen Orlov <yevhen.orlov@plvision.eu> > Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu> > Co-developed-by: Taras Chornyi <taras.chornyi@plvision.eu> > Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu> > Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu> > --- > PATCH V4: > - move changelog under threeline separator. > - add small comment that explains reason for > empty pcs 'an_restart' function. > PATCH V3: > - force inband mode for SGMII > - fix >80 chars warning of checkpatch where possible (2/5) > - structure phylink_mac_change alongside phylink-related if-clause; > PATCH V2: > - fix mistreat of bitfield values as if they were bools. > - remove phylink_config ifdefs. > - remove obsolete phylink pcs / mac callbacks; > - rework mac (/pcs) config to not look for speed / duplex > parameters while link is not yet set up. > - remove unused functions. > - add phylink select cfg to prestera Kconfig. > > drivers/net/ethernet/marvell/prestera/Kconfig | 1 + > .../net/ethernet/marvell/prestera/prestera.h | 9 + > .../marvell/prestera/prestera_ethtool.c | 28 +- > .../marvell/prestera/prestera_ethtool.h | 3 - > .../ethernet/marvell/prestera/prestera_main.c | 353 ++++++++++++++++-- > 5 files changed, 332 insertions(+), 62 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig b/drivers/net/ethernet/marvell/prestera/Kconfig > index b6f20e2034c6..f2f7663c3d10 100644 > --- a/drivers/net/ethernet/marvell/prestera/Kconfig > +++ b/drivers/net/ethernet/marvell/prestera/Kconfig > @@ -8,6 +8,7 @@ config PRESTERA > depends on NET_SWITCHDEV && VLAN_8021Q > depends on BRIDGE || BRIDGE=n > select NET_DEVLINK > + select PHYLINK > help > This driver supports Marvell Prestera Switch ASICs family. > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera.h b/drivers/net/ethernet/marvell/prestera/prestera.h > index bff9651f0a89..832c27e0c284 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera.h > +++ b/drivers/net/ethernet/marvell/prestera/prestera.h > @@ -7,6 +7,7 @@ > #include <linux/notifier.h> > #include <linux/skbuff.h> > #include <linux/workqueue.h> > +#include <linux/phylink.h> > #include <net/devlink.h> > #include <uapi/linux/if_ether.h> > > @@ -92,6 +93,7 @@ struct prestera_lag { > struct prestera_flow_block; > > struct prestera_port_mac_state { > + bool valid; > u32 mode; > u32 speed; > bool oper; > @@ -151,6 +153,12 @@ struct prestera_port { > struct prestera_port_phy_config cfg_phy; > struct prestera_port_mac_state state_mac; > struct prestera_port_phy_state state_phy; > + > + struct phylink_config phy_config; > + struct phylink *phy_link; > + struct phylink_pcs phylink_pcs; > + > + rwlock_t state_mac_lock; > }; > > struct prestera_device { > @@ -291,6 +299,7 @@ struct prestera_switch { > u32 mtu_min; > u32 mtu_max; > u8 id; > + struct device_node *np; > struct prestera_router *router; > struct prestera_lag *lags; > struct prestera_counter *counter; > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c > index 40d5b89573bb..1da7ff889417 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c > +++ b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c > @@ -521,6 +521,9 @@ prestera_ethtool_get_link_ksettings(struct net_device *dev, > ecmd->base.speed = SPEED_UNKNOWN; > ecmd->base.duplex = DUPLEX_UNKNOWN; > > + if (port->phy_link) > + return phylink_ethtool_ksettings_get(port->phy_link, ecmd); > + > ecmd->base.autoneg = port->autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE; > > if (port->caps.type == PRESTERA_PORT_TYPE_TP) { > @@ -648,6 +651,9 @@ prestera_ethtool_set_link_ksettings(struct net_device *dev, > u8 adver_fec; > int err; > > + if (port->phy_link) > + return phylink_ethtool_ksettings_set(port->phy_link, ecmd); > + > err = prestera_port_type_set(ecmd, port); > if (err) > return err; > @@ -782,28 +788,6 @@ static int prestera_ethtool_nway_reset(struct net_device *dev) > return -EINVAL; > } > > -void prestera_ethtool_port_state_changed(struct prestera_port *port, > - struct prestera_port_event *evt) > -{ > - struct prestera_port_mac_state *smac = &port->state_mac; > - > - smac->oper = evt->data.mac.oper; > - > - if (smac->oper) { > - smac->mode = evt->data.mac.mode; > - smac->speed = evt->data.mac.speed; > - smac->duplex = evt->data.mac.duplex; > - smac->fc = evt->data.mac.fc; > - smac->fec = evt->data.mac.fec; > - } else { > - smac->mode = PRESTERA_MAC_MODE_MAX; > - smac->speed = SPEED_UNKNOWN; > - smac->duplex = DUPLEX_UNKNOWN; > - smac->fc = 0; > - smac->fec = 0; > - } > -} > - > const struct ethtool_ops prestera_ethtool_ops = { > .get_drvinfo = prestera_ethtool_get_drvinfo, > .get_link_ksettings = prestera_ethtool_get_link_ksettings, > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h > index 9eb18e99dea6..bd5600886bc6 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h > +++ b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h > @@ -11,7 +11,4 @@ struct prestera_port; > > extern const struct ethtool_ops prestera_ethtool_ops; > > -void prestera_ethtool_port_state_changed(struct prestera_port *port, > - struct prestera_port_event *evt); > - > #endif /* _PRESTERA_ETHTOOL_H_ */ > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c > index ea5bd5069826..192ab706e45e 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c > +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c > @@ -9,6 +9,7 @@ > #include <linux/of.h> > #include <linux/of_net.h> > #include <linux/if_vlan.h> > +#include <linux/phylink.h> > > #include "prestera.h" > #include "prestera_hw.h" > @@ -142,18 +143,24 @@ static int prestera_port_open(struct net_device *dev) > struct prestera_port_mac_config cfg_mac; > int err = 0; > > - if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) { > - err = prestera_port_cfg_mac_read(port, &cfg_mac); > - if (!err) { > - cfg_mac.admin = true; > - err = prestera_port_cfg_mac_write(port, &cfg_mac); > - } > + if (port->phy_link) { > + phylink_start(port->phy_link); > } else { > - port->cfg_phy.admin = true; > - err = prestera_hw_port_phy_mode_set(port, true, port->autoneg, > - port->cfg_phy.mode, > - port->adver_link_modes, > - port->cfg_phy.mdix); > + if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) { > + err = prestera_port_cfg_mac_read(port, &cfg_mac); > + if (!err) { > + cfg_mac.admin = true; > + err = prestera_port_cfg_mac_write(port, > + &cfg_mac); > + } > + } else { > + port->cfg_phy.admin = true; > + err = prestera_hw_port_phy_mode_set(port, true, > + port->autoneg, > + port->cfg_phy.mode, > + port->adver_link_modes, > + port->cfg_phy.mdix); > + } > } > > netif_start_queue(dev); > @@ -169,23 +176,260 @@ static int prestera_port_close(struct net_device *dev) > > netif_stop_queue(dev); > > - if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) { > + if (port->phy_link) { > + phylink_stop(port->phy_link); > + phylink_disconnect_phy(port->phy_link); > err = prestera_port_cfg_mac_read(port, &cfg_mac); > if (!err) { > cfg_mac.admin = false; > prestera_port_cfg_mac_write(port, &cfg_mac); > } > } else { > - port->cfg_phy.admin = false; > - err = prestera_hw_port_phy_mode_set(port, false, port->autoneg, > - port->cfg_phy.mode, > - port->adver_link_modes, > - port->cfg_phy.mdix); > + if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) { > + err = prestera_port_cfg_mac_read(port, &cfg_mac); > + if (!err) { > + cfg_mac.admin = false; > + prestera_port_cfg_mac_write(port, &cfg_mac); > + } > + } else { > + port->cfg_phy.admin = false; > + err = prestera_hw_port_phy_mode_set(port, false, port->autoneg, > + port->cfg_phy.mode, > + port->adver_link_modes, > + port->cfg_phy.mdix); > + } > } > > return err; > } > > +static void > +prestera_port_mac_state_cache_read(struct prestera_port *port, > + struct prestera_port_mac_state *state) > +{ > + read_lock(&port->state_mac_lock); > + *state = port->state_mac; > + read_unlock(&port->state_mac_lock);
read locks are subject to some non fair behavior. That can hurts when the relevant lock is accessed quite often/is under contention - likely not here, right? - but perhaps a plain spinlock would be nicer.
> +} > + > +static void > +prestera_port_mac_state_cache_write(struct prestera_port *port, > + struct prestera_port_mac_state *state) > +{ > + write_lock(&port->state_mac_lock); > + port->state_mac = *state; > + write_unlock(&port->state_mac_lock); > +} > + > +static struct prestera_port *prestera_pcs_to_port(struct phylink_pcs *pcs) > +{ > + return container_of(pcs, struct prestera_port, phylink_pcs); > +} > + > +static void prestera_mac_config(struct phylink_config *config, > + unsigned int an_mode, > + const struct phylink_link_state *state) > +{ > +} > + > +static void prestera_mac_link_down(struct phylink_config *config, > + unsigned int mode, phy_interface_t interface) > +{ > + struct net_device *ndev = to_net_dev(config->dev); > + struct prestera_port *port = netdev_priv(ndev); > + struct prestera_port_mac_state state_mac; > + > + /* Invalidate. Parameters will update on next link event. */ > + memset(&state_mac, 0, sizeof(state_mac)); > + state_mac.valid = false; > + prestera_port_mac_state_cache_write(port, &state_mac); > +} > + > +static void prestera_mac_link_up(struct phylink_config *config, > + struct phy_device *phy, > + unsigned int mode, phy_interface_t interface, > + int speed, int duplex, > + bool tx_pause, bool rx_pause) > +{ > +} > + > +static struct phylink_pcs * > +prestera_mac_select_pcs(struct phylink_config *config, > + phy_interface_t interface) > +{ > + struct net_device *dev = to_net_dev(config->dev); > + struct prestera_port *port = netdev_priv(dev); > + > + return &port->phylink_pcs; > +} > + > +static void prestera_pcs_get_state(struct phylink_pcs *pcs, > + struct phylink_link_state *state) > +{ > + struct prestera_port *port = container_of(pcs, struct prestera_port, > + phylink_pcs); > + struct prestera_port_mac_state smac; > + > + prestera_port_mac_state_cache_read(port, &smac); > + > + if (smac.valid) { > + state->link = smac.oper ? 1 : 0; > + /* AN is completed, when port is up */ > + state->an_complete = (smac.oper && port->autoneg) ? 1 : 0; > + state->speed = smac.speed; > + state->duplex = smac.duplex; > + } else { > + state->link = 0; > + state->an_complete = 0; > + } > +} > + > +static int prestera_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + struct prestera_port *port = port = prestera_pcs_to_port(pcs); > + struct prestera_port_mac_config cfg_mac; > + int err; > + > + err = prestera_port_cfg_mac_read(port, &cfg_mac); > + if (err) > + return err; > + > + cfg_mac.admin = true; > + cfg_mac.fec = PRESTERA_PORT_FEC_OFF; > + > + switch (interface) { > + case PHY_INTERFACE_MODE_10GBASER: > + cfg_mac.speed = SPEED_10000; > + cfg_mac.inband = 0; > + cfg_mac.mode = PRESTERA_MAC_MODE_SR_LR; > + break; > + case PHY_INTERFACE_MODE_2500BASEX: > + cfg_mac.speed = SPEED_2500; > + cfg_mac.duplex = DUPLEX_FULL; > + cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > + advertising); > + cfg_mac.mode = PRESTERA_MAC_MODE_SGMII; > + break; > + case PHY_INTERFACE_MODE_SGMII: > + cfg_mac.inband = 1; > + cfg_mac.mode = PRESTERA_MAC_MODE_SGMII; > + break; > + case PHY_INTERFACE_MODE_1000BASEX: > + default: > + cfg_mac.speed = SPEED_1000; > + cfg_mac.duplex = DUPLEX_FULL; > + cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > + advertising); > + cfg_mac.mode = PRESTERA_MAC_MODE_1000BASE_X; > + break; > + } > + > + err = prestera_port_cfg_mac_write(port, &cfg_mac); > + if (err) > + return err; > + > + return 0; > +} > + > +static void prestera_pcs_an_restart(struct phylink_pcs *pcs) > +{ > + /* > + * TODO: add 100basex AN restart support
Possibly typo above ? s/100basex/1000basex/
Cheers,
Paolo
| |