Re: [PATCH V4 net-next] net: marvell: prestera: add phylink support

From: Paolo Abeni
Date: Tue Jul 19 2022 - 04:53:18 EST

Next message: Ulf Hansson: "Re: [PATCH v3 1/2] power: domain: handle power supplies that need interrupts"
Previous message: Ben Dooks: "[PATCH] reset: reset-simple should depends on HAS_IOMEM"
Next in thread: Oleksandr Mazur: "Re: [PATCH V4 net-next] net: marvell: prestera: add phylink support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> Signed-off-by: Yevhen Orlov <yevhen.orlov@xxxxxxxxxxx>
> Co-developed-by: Taras Chornyi <taras.chornyi@xxxxxxxxxxx>
> Signed-off-by: Taras Chornyi <taras.chornyi@xxxxxxxxxxx>
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@xxxxxxxxxxx>
> ---
> 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

Next message: Ulf Hansson: "Re: [PATCH v3 1/2] power: domain: handle power supplies that need interrupts"
Previous message: Ben Dooks: "[PATCH] reset: reset-simple should depends on HAS_IOMEM"
Next in thread: Oleksandr Mazur: "Re: [PATCH V4 net-next] net: marvell: prestera: add phylink support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]