Message-ID: <VI1PR0402MB280076275F4B8343B4E8BC2EE0EE0@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Fri, 14 Jun 2019 16:34:59 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
Subject: RE: [PATCH RFC 4/6] dpaa2-mac: add initial driver


> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> 
> On Fri, Jun 14, 2019 at 02:55:51AM +0300, Ioana Ciornei wrote:
> > The dpaa2-mac driver binds to DPAA2 DPMAC objects, dynamically
> > discovered on the fsl-mc bus. It acts as a proxy between the PHY
> > management layer and the MC firmware, delivering any configuration
> > changes to the firmware and also setting any new configuration
> > requested though PHYLINK.
> >
> > A in-depth view of the software architecture and the implementation
> > can be found in
> > 'Documentation/networking/device_drivers/freescale/dpaa2/dpmac-
> driver.rst'.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > ---
> >  MAINTAINERS                                      |   7 +
> >  drivers/net/ethernet/freescale/dpaa2/Kconfig     |  13 +
> >  drivers/net/ethernet/freescale/dpaa2/Makefile    |   2 +
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 541
> > +++++++++++++++++++++++
> >  4 files changed, 563 insertions(+)
> >  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > dd247a059889..a024ab2b2548 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4929,6 +4929,13 @@ S:	Maintained
> >  F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp*
> >  F:	drivers/net/ethernet/freescale/dpaa2/dprtc*
> >
> > +DPAA2 MAC DRIVER
> > +M:	Ioana Ciornei <ioana.ciornei@....com>
> > +L:	netdev@...r.kernel.org
> > +S:	Maintained
> > +F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-mac*
> > +F:	drivers/net/ethernet/freescale/dpaa2/dpmac*
> > +
> >  DPT_I2O SCSI RAID DRIVER
> >  M:	Adaptec OEM Raid Solutions <aacraid@...rosemi.com>
> >  L:	linux-scsi@...r.kernel.org
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > index 8bd384720f80..4ffa666c0a43 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > @@ -16,3 +16,16 @@ config FSL_DPAA2_PTP_CLOCK
> >  	help
> >  	  This driver adds support for using the DPAA2 1588 timer module
> >  	  as a PTP clock.
> > +
> > +config FSL_DPAA2_MAC
> > +	tristate "DPAA2 MAC / PHY proxy interface"
> > +	depends on FSL_MC_BUS
> > +	select MDIO_BUS_MUX_MMIOREG
> > +	select FSL_XGMAC_MDIO
> > +	select PHYLINK
> > +	help
> > +	  Prototype driver for DPAA2 MAC / PHY interface object.
> > +	  This driver works as a proxy between PHYLINK including phy drivers and
> > +	  the MC firmware.  It receives updates on link state changes from
> PHYLINK
> > +	  and forwards them to MC and receives interrupt from MC whenever a
> > +	  request is made to change the link state or configuration.
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/Makefile
> > b/drivers/net/ethernet/freescale/dpaa2/Makefile
> > index d1e78cdd512f..e96386ab23ea 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/Makefile
> > +++ b/drivers/net/ethernet/freescale/dpaa2/Makefile
> > @@ -5,10 +5,12 @@
> >
> >  obj-$(CONFIG_FSL_DPAA2_ETH)		+= fsl-dpaa2-eth.o
> >  obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK)	+= fsl-dpaa2-ptp.o
> > +obj-$(CONFIG_FSL_DPAA2_MAC)		+= fsl-dpaa2-mac.o
> >
> >  fsl-dpaa2-eth-objs	:= dpaa2-eth.o dpaa2-ethtool.o dpni.o
> >  fsl-dpaa2-eth-${CONFIG_DEBUG_FS} += dpaa2-eth-debugfs.o
> >  fsl-dpaa2-ptp-objs	:= dpaa2-ptp.o dprtc.o
> > +fsl-dpaa2-mac-objs	:= dpaa2-mac.o dpmac.o
> >
> >  # Needed by the tracing framework
> >  CFLAGS_dpaa2-eth.o := -I$(src)
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > new file mode 100644
> > index 000000000000..145ab4771788
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > @@ -0,0 +1,541 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2015 Freescale Semiconductor Inc.
> > + * Copyright 2018-2019 NXP
> > + */
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/msi.h>
> > +#include <linux/rtnetlink.h>
> > +#include <linux/if_vlan.h>
> > +
> > +#include <net/netlink.h>
> > +#include <uapi/linux/if_bridge.h>
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/of_net.h>
> > +#include <linux/phylink.h>
> > +#include <linux/notifier.h>
> > +
> > +#include <linux/fsl/mc.h>
> > +
> > +#include "dpmac.h"
> > +#include "dpmac-cmd.h"
> > +
> > +#define to_dpaa2_mac_priv(phylink_config) \
> > +	container_of(config, struct dpaa2_mac_priv, phylink_config)
> > +
> > +struct dpaa2_mac_priv {
> > +	struct fsl_mc_device *mc_dev;
> > +	struct dpmac_attr attr;
> > +	struct dpmac_link_state state;
> > +	u16 dpmac_ver_major;
> > +	u16 dpmac_ver_minor;
> > +
> > +	struct phylink *phylink;
> > +	struct phylink_config phylink_config;
> > +	struct ethtool_link_ksettings kset;
> > +};
> > +
> > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > +	switch (eth_if) {
> > +	case DPMAC_ETH_IF_RGMII:
> > +		return PHY_INTERFACE_MODE_RGMII;
> > +	case DPMAC_ETH_IF_XFI:
> > +		return PHY_INTERFACE_MODE_10GKR;
> > +	case DPMAC_ETH_IF_USXGMII:
> > +		return PHY_INTERFACE_MODE_USXGMII;
> 
> No support for SGMII nor the 802.3z modes?

The Serdes has support for both of them but the driver does not at the moment.

> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int cmp_dpmac_ver(struct dpaa2_mac_priv *priv,
> > +			 u16 ver_major, u16 ver_minor)
> > +{
> > +	if (priv->dpmac_ver_major == ver_major)
> > +		return priv->dpmac_ver_minor - ver_minor;
> > +	return priv->dpmac_ver_major - ver_major; }
> > +
> > +struct dpaa2_mac_link_mode_map {
> > +	u64 dpmac_lm;
> > +	enum ethtool_link_mode_bit_indices ethtool_lm; };
> > +
> > +static const struct dpaa2_mac_link_mode_map dpaa2_mac_lm_map[] = {
> > +	{DPMAC_ADVERTISED_10BASET_FULL,
> ETHTOOL_LINK_MODE_10baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_100BASET_FULL,
> ETHTOOL_LINK_MODE_100baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_1000BASET_FULL,
> ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_10000BASET_FULL,
> > +ETHTOOL_LINK_MODE_10000baseT_Full_BIT},
> 
> No half-duplex support?

Yes, no half-duplex.

> 
> > +	{DPMAC_ADVERTISED_AUTONEG,
> ETHTOOL_LINK_MODE_Autoneg_BIT}, };
> > +
> > +static void link_mode_phydev2dpmac(unsigned long *phydev_lm,
> > +				   u64 *dpmac_lm)
> > +{
> > +	enum ethtool_link_mode_bit_indices link_mode;
> > +	int i;
> > +
> > +	*dpmac_lm = 0;
> > +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> > +		link_mode = dpaa2_mac_lm_map[i].ethtool_lm;
> > +		if (linkmode_test_bit(link_mode, phydev_lm))
> > +			*dpmac_lm |= dpaa2_mac_lm_map[i].dpmac_lm;
> > +	}
> > +}
> > +
> > +static void dpaa2_mac_ksettings_change(struct dpaa2_mac_priv *priv) {
> > +	struct fsl_mc_device *mc_dev = priv->mc_dev;
> > +	struct dpmac_link_cfg link_cfg = { 0 };
> > +	int err, i;
> > +
> > +	err = dpmac_get_link_cfg(mc_dev->mc_io, 0,
> > +				 mc_dev->mc_handle,
> > +				 &link_cfg);
> > +
> > +	if (err) {
> > +		dev_err(&mc_dev->dev, "dpmac_get_link_cfg() = %d\n", err);
> > +		return;
> > +	}
> > +
> > +	phylink_ethtool_ksettings_get(priv->phylink, &priv->kset);
> > +
> > +	priv->kset.base.speed = link_cfg.rate;
> > +	priv->kset.base.duplex = !!(link_cfg.options &
> > +DPMAC_LINK_OPT_HALF_DUPLEX);
> 
> What's the point of setting duplex to anything other than true here - everything
> I've read in this driver apart from the above indicates that there is no support for
> half duplex.

Yep, that's another mishap on my part. That should have been removed.

> 
> > +
> > +	ethtool_link_ksettings_zero_link_mode(&priv->kset, advertising);
> > +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> > +		if (link_cfg.advertising & dpaa2_mac_lm_map[i].dpmac_lm)
> > +			__set_bit(dpaa2_mac_lm_map[i].ethtool_lm,
> > +				  priv->kset.link_modes.advertising);
> > +	}
> > +
> > +	if (link_cfg.options & DPMAC_LINK_OPT_AUTONEG) {
> > +		priv->kset.base.autoneg = AUTONEG_ENABLE;
> > +		__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +			  priv->kset.link_modes.advertising);
> > +	} else {
> > +		priv->kset.base.autoneg = AUTONEG_DISABLE;
> > +		__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +			    priv->kset.link_modes.advertising);
> > +	}
> > +
> > +	phylink_ethtool_ksettings_set(priv->phylink, &priv->kset);
> 
> What if this returns an error?  There seems to be no way to communicate failure
> back through the firmware.

That's right, we do not have a way of signaling back the failure to the firmware.

> 
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set_port_modes(mask);
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_10GKR:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> 
> Consider using the newer linkmode_set_bit() etc interfaces here.

Sure.

> 
> > +		break;
> > +	default:
> > +		goto empty_set;
> > +	}
> > +
> > +	bitmap_and(supported, supported, mask,
> __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +	bitmap_and(state->advertising, state->advertising, mask,
> > +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +
> > +	link_mode_phydev2dpmac(supported, &dpmac_state->supported);
> > +	link_mode_phydev2dpmac(state->advertising,
> > +&dpmac_state->advertising);
> 
> This is not correct.  phylink will make calls to this function to enquire whether
> something is supported or not, it isn't strictly used to say "this is what we are
> going to use", so storing these does not reflect the current state.

We can update these only on a mac_config.
> 
> > +
> > +	return;
> > +
> > +empty_set:
> > +	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); }
> > +
> > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int
> mode,
> > +			     const struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	if (state->speed == SPEED_UNKNOWN && state->duplex ==
> DUPLEX_UNKNOWN)
> > +		return;
> > +
> > +	dpmac_state->up = !!state->link;
> > +	if (dpmac_state->up) {
> > +		dpmac_state->rate = state->speed;
> > +
> > +		if (!state->duplex)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_HALF_DUPLEX;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > +
> > +		if (state->an_enabled)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_AUTONEG;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_AUTONEG;
> > +	}
> 
> Apart from my comments for the above in reply to Andrew, you can store the
> "advertising" mask here.
> 
> However, what is the point of the "dpmac_state->up = !!state->link"
> stuff (despite it being wrong as previously described) when you set dpmac_state-
> >up in the mac_link_up/mac_link_down functions below.
> This makes no sense to me.

Ok, so keep the last known value of the link until a .mac_link_{up/down} is called and only them update it.
I'll change that.

> 
> > +
> > +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> > +				   priv->mc_dev->mc_handle, dpmac_state);
> > +	if (err)
> > +		dev_err(dev, "dpmac_set_link_state() = %d\n", err); }
> > +
> > +static void dpaa2_mac_link_up(struct phylink_config *config, unsigned int
> mode,
> > +			      phy_interface_t interface, struct phy_device *phy) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	dpmac_state->up = 1;
> > +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> > +				   priv->mc_dev->mc_handle, dpmac_state);
> > +	if (err)
> > +		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
> 
> This is also very suspect - have you read the phylink documentation?
> The documentation details that there are some behavioural differences here
> depending on the negotiation mode, but your code doesn't even look at those.
> 
> Given that you're not handling those, I don't see how you expect SFP support to
> work.  In fact, given that the validate callback doesn't make any mention of
> SGMII, 1000BASEX, or 2500BASEX phy modes, I don't see how you expect this to
> work with SFP.  Given that, I really question why you want to use phylink rather
> than talking to phylib directly.

I am not handling the XFP/SFP+ support in this version of the driver since, as you noticed, I am not accepting BASEX modes.
Nonetheless, I am not ruling out adding support for SFP on top of this. This is one reason why we're using phylink. 

Also, using phylib is not straight-forward either. The dpaa2-mac should fabricate a net_device for each phy that it wants to connect to.
These net_device should be kept unregistered with the network core so that we don't see "MAC interfaces" in ifconfig.

> 
> I get the overall impression from what I've seen so far that phylink is entirely
> unsuited to the structure of this implementation.
> 
> phylinks purpose is to support hotpluggable PHYs on SFP modules where the
> MAC may be connected _directly_ to the SFP cage without an intervening PHY,
> or if there is an intervening PHY, the PHY is completely transparent.  For that to
> work, the interface modes that SFP modules support must be supported by the
> MAC.
> 
> I can't see a reason at the moment for you to use phylink.
> 

Well, a phylib solution isn't cleaner either. 

--
Ioana

Powered by blists - more mailing lists