Message-ID: <3ad23735-38be-16ea-3bd0-c77eccc22f16@gmail.com>
Date:   Thu, 14 Jun 2018 09:51:34 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Cc:     netdev@...r.kernel.org, openwrt-devel@...ts.openwrt.org,
        LEDE Development List <lede-dev@...ts.infradead.org>,
        Gabor Juhos <juhosg@...nwrt.org>
Subject: Re: [PATCH 3/3] net: dsa: Add Vitesse VSC73xx DSA router driver



On 06/14/2018 05:35 AM, Linus Walleij wrote:
> This adds a DSA driver for:
> 
> Vitesse VSC7385 SparX-G5 5-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
> 
> These switches have a built-in 8051 CPU and can download and execute
> firmware in this CPU. They can also be configured to use an external
> CPU handling the switch in a memory-mapped manner by connecting to
> that external CPU's memory bus.
> 
> This driver (currently) only takes control of the switch chip over
> SPI and configures it to route packages around when connected to a
> CPU port. The chip has embedded PHYs and VLAN support so we model it
> using DSA as a best fit so we can easily add VLAN support and maybe
> later also exploit the internal frame header to get more direct
> control over the switch.

Yes having the internal frame header working would be really great,
DSA_TAG_PROTO_NONE is really difficult to use without knowing all the
DSA details which reminds that we should have the following action items:

- document how DSA_TAG_PROTO_NONE behave differently with respect to
bridges/VLAN configuration and the DSA master device

- possibly introduce DSA_TAG_PROTO_8021Q which would automatically
partition ports by allocating one VLAN ID per-port (e.g: from top to
bottom) that would effectively offer the same features/paradigms as what
a proper header would offer (Port separation, if nothing else) and it
could be made seemingly automatic from within DSA

- get rid of b53's DSA_TAG_PROTO_NONE

> 
> The four built-in GPIO lines are exposed using a standard GPIO chip.

What are those typically used for out of curiosity? Is this to connect
to an EEPROM?

> 
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  drivers/net/dsa/Kconfig           |   12 +
>  drivers/net/dsa/Makefile          |    1 +
>  drivers/net/dsa/vitesse-vsc73xx.c | 1362 +++++++++++++++++++++++++++++
>  3 files changed, 1375 insertions(+)
>  create mode 100644 drivers/net/dsa/vitesse-vsc73xx.c
> 
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 2b81b97e994f..2f6207b969e3 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -76,4 +76,16 @@ config NET_DSA_SMSC_LAN9303_MDIO
>  	  Enable access functions if the SMSC/Microchip LAN9303 is configured
>  	  for MDIO managed mode.
>  
> +config NET_DSA_VITESSE_VSC73XX
> +	tristate "Vitesse VSC7385/7388/7395/7398 support"
> +	depends on OF && SPI
> +	depends on NET_DSA
> +	select FIXED_PHY
> +	select VITESSE_PHY
> +	select NET_DSA_TAG_TRAILER

You advertise DSA_TAG_PROTO_NONE, so that appears to be unnecessary right?

[snip]
> +/**
> + * struct vsc73xx - VSC73xx state container
> + */
> +struct vsc73xx {
> +	struct device		*dev;
> +	struct gpio_desc	*reset;
> +	struct spi_device	*spi;
> +	struct dsa_switch	*ds;
> +	struct gpio_chip	gc;
> +	u16			chipid;
> +	bool			is_vsc7385;
> +	bool			is_vsc7388;
> +	bool			is_vsc7395;
> +	bool			is_vsc7398;

How about having an u16/u32 chip_id instead?

> +	u8			addr[ETH_ALEN];
> +	struct mutex		lock; /* Protects SPI traffic */
> +};

[snip]

> +static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
> +						      int port)
> +{
> +	/* The switch internally uses a 8 byte header with length,
> +	 * source port, tag, LPA and priority. This is supposedly
> +	 * only accessible when operating the switch using the internal
> +	 * CPU or with an external CPU mapping the device in, but not
> +	 * when operating the switch over SPI and putting frames in/out
> +	 * on port 6 (the CPU port). So far we must assume that we
> +	 * cannot access the tag. (See "Internal frame header" section
> +	 * 3.9.1 in the manual.)

I would be really good if we could get this to work even in SPI with the
CPU controlling the switch, I cannot really think of why the 8051 would
have to be involved, because having the 8051 means either the switch is
entirely standalone and runs off an EEPROM (which is additional cost on
your BOM), or the host, through SPI can entirely take over.

Is the datasheet public somehow?

> +	 */
> +	return DSA_TAG_PROTO_NONE;
> +}

[snip]

> +static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
> +				struct phy_device *phydev)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u32 val;

No is_pseudo_fixed_link() check, you really have to do all of this for
each front-panel port? That is really bad if that is the case... most
switches with front-panel built-in PHYs are at the very least capable of
re-configuring their internal MAC accordingly.

> +
> +	/* Special handling of the CPU-facing port */
> +	if (port == CPU_PORT) {
> +		/* Other ports are already initialized but not this one */
> +		vsc73xx_init_port(vsc, CPU_PORT);
> +		/* Select the external port for this interface (EXT_PORT)
> +		 * Enable the GMII GTX external clock
> +		 * Use double data rate (DDR mode)
> +		 */
> +		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
> +			      CPU_PORT,
> +			      VSC73XX_ADVPORTM,
> +			      VSC73XX_ADVPORTM_EXT_PORT |
> +			      VSC73XX_ADVPORTM_ENA_GTX |
> +			      VSC73XX_ADVPORTM_DDR_MODE);
> +	}
> +
> +	/* This is the MAC confiuration that always need to happen
> +	 * after a PHY or the CPU port comes up or down.
> +	 */
> +	val = phy_read(phydev, 1);

MII_BMSR

> +	if ((val & 0x0024) != 0x0024) {

BMSR_ANEGCOMPLETE | BMSR_LSTATUS


> +		dev_info(vsc->dev, "port %d: went down\n",
> +			 port);

That would duplicate what PHYLIB already prints, please drop it or
demote it dev_dbg().

> +
> +		/* Disable RX on this port */
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_MAC_CFG,
> +				    VSC73XX_MAC_CFG_RX_EN, 0);
> +
> +		/* Discard packets */
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> +				    VSC73XX_ARBDISC, BIT(port), BIT(port));
> +
> +		/* Wait until queue is empty */
> +		vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> +			     VSC73XX_ARBEMPTY, &val);
> +		while (!(val & BIT(port))) {
> +			msleep(1);
> +			vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> +				     VSC73XX_ARBEMPTY, &val);
> +		}

Possibly unbounded loop (if the HW behave incorrectly).

> +
> +		/* Put this port into reset */
> +		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> +			      VSC73XX_MAC_CFG_RESET);
> +
> +		/* Accept packets again */
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> +				    VSC73XX_ARBDISC, BIT(port), 0);
> +
> +		/* Allow backward dropping of frames from this port */
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> +				    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
> +
> +		/* Receive mask (disable forwarding) */
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), 0);
> +
> +		return;
> +	}
> +
> +	/* Figure out what speed was negotiated */
> +	val = phy_read(phydev, 0x0a);

MII_STAT1000

> +	if (val & 0x0c00) {

LPA_1000FULL | LPA_1000HALF

> +		dev_info(vsc->dev, "port %d: 1000 Mbit mode full duplex\n",
> +			 port);

Likewise, duplicates PHYLIB messages.

> +
> +		/* Set up default for internal or external RGMII */
> +		if (port == CPU_PORT)
> +			val = VSC73XX_MAC_CFG_1000M_F_RGMII;

You need to look at the CPU port's phy_device->interface for that (which
should be a fixed-link).

> +		else
> +			val = VSC73XX_MAC_CFG_1000M_F_PHY;
> +		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> +	} else {
> +		val = phy_read(phydev, 0x05);

MII_LPA

> +		val &= 0x05e0;
> +		val >>= 5;
> +		if (val & 0x0c) {
> +			if (val & 0x08) {
> +				val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> +				dev_info(vsc->dev,
> +					 "port %d: 100 Mbit full duplex mode\n",
> +					 port);
> +			} else {
> +				val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> +				dev_info(vsc->dev,
> +					 "port %d: 100 Mbit half duplex mode\n",
> +					 port);
> +			}
> +			vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> +		} else if (val & 0x03) {
> +			if (val & 0x02) {
> +				val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> +				dev_info(vsc->dev,
> +					 "port %d: 10 Mbit full duplex mode\n",
> +					 port);
> +			} else {
> +				val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> +				dev_info(vsc->dev,
> +					 "port %d: 10 Mbit half duplex mode\n",
> +					 port);
> +			}
> +			vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> +		} else {
> +			dev_err(vsc->dev,
> +				"could not adjust link: unknown speed\n");
> +		}
> +	}

A lot of what you are doing here has been dong by genphy_read_status()
and you should be able to extract the link speed/duplex/lpa from the
phy_device itself, and not perform additional MDIO reads.

> +
> +	/* Enable port (forwarding) in the receieve mask */
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +			    VSC73XX_RECVMASK, BIT(port), BIT(port));
> +}
> +
> +static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
> +			       struct phy_device *phy)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	dev_info(vsc->dev, "enable port %d\n", port);
> +
> +	/* VSC7385 and VSC7395 have ports 0..4 accessible */
> +	if ((vsc->is_vsc7385 || vsc->is_vsc7395) && port > 4)
> +		return -ENODEV;
> +	if ((vsc->is_vsc7388 || vsc->is_vsc7398) && port > 7)
> +		return -ENODEV;

Humm no, you would not even get there if you told DSA how many ports you
have, see below.

> +
> +	vsc73xx_init_port(vsc, port);
> +
> +	return 0;
> +}
> +
> +static void vsc73xx_port_disable(struct dsa_switch *ds, int port,
> +				 struct phy_device *phy)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	/* VSC7385 and VSC7395 have ports 0..4 accessible */
> +	if ((vsc->is_vsc7385 || vsc->is_vsc7395) && port > 4)
> +		return;
> +	if ((vsc->is_vsc7388 || vsc->is_vsc7398) && port > 7)
> +		return;

Likewise.

> +
> +	/* Just put the port into reset */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
> +		      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
> +}
> +
> +const struct vsc73xx_counter *vsc73xx_find_counter(struct vsc73xx *vsc,
> +						   u8 counter,
> +						   bool tx)

Missing static?

[snip]

> +	vsc->gc.label = devm_kasprintf(dev, GFP_KERNEL, "VSC%04x",
> +				       vsc->chipid);
> +	vsc->gc.ngpio = 4;
> +	vsc->gc.owner = THIS_MODULE;
> +	vsc->gc.parent = dev;
> +	vsc->gc.of_node = dev->of_node;
> +	vsc->gc.base = -1;
> +	vsc->gc.get = vsc73xx_gpio_get;
> +	vsc->gc.set = vsc73xx_gpio_set;
> +	vsc->gc.direction_input = vsc73xx_gpio_direction_input;
> +	vsc->gc.direction_output = vsc73xx_gpio_direction_output;
> +	vsc->gc.get_direction = vsc73xx_gpio_get_direction;
> +	vsc->gc.can_sleep = true;
> +	ret = devm_gpiochip_add_data(dev, &vsc->gc, vsc);
> +	if (ret) {
> +		dev_err(dev, "unable to register GPIO chip\n");
> +		dsa_unregister_switch(vsc->ds);
> +		return ret;
> +	}

Would you consider putting this in a separate function so this can be
optionally disabled?

> +
> +	return 0;
> +}
> +
> +static int vsc73xx_remove(struct spi_device *spi)
> +{
> +	struct vsc73xx *vsc = spi_get_drvdata(spi);
> +
> +	dsa_unregister_switch(vsc->ds);
> +	gpiod_set_value(vsc->reset, 1);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vsc73xx_of_match[] = {
> +	{
> +		.compatible = "vitesse,vsc7385",

Would not you want to pass additional data here, like the possible port
layout/chip id to be reading?
-- 
Florian

Powered by blists - more mailing lists