Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <660684000d6820524c61d733fb076225202dad8e.camel@redhat.com>
Subject: Re: [PATCH V4 net-next] net: marvell: prestera: add phylink support
From:   Paolo Abeni <pabeni@redhat.com>
To:     Oleksandr Mazur <oleksandr.mazur@plvision.eu>,
        netdev@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, yevhen.orlov@plvision.eu,
        taras.chornyi@plvision.eu, linux@armlinux.org.uk, andrew@lunn.ch
Date:   Tue, 19 Jul 2022 10:53:05 +0200
In-Reply-To: <20220715193454.7627-1-oleksandr.mazur@plvision.eu>
References: <20220715193454.7627-1-oleksandr.mazur@plvision.eu>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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