On 03/23/2018 01:11 PM, Alexandre Belloni wrote:

Add a driver for Microsemi Ocelot Ethernet switch support.

This makes two modules:
mscc_ocelot_common handles all the common features that doesn't depend on
how the switch is integrated in the SoC. Currently, it handles offloading
bridging to the hardware. ocelot_io.c handles register accesses. This is
unfortunately needed because the register layout is packed and then depends
on the number of ports available on the switch. The register definition
files are automatically generated.

ocelot_board handles the switch integration on the SoC and on the board.

Frame injection and extraction to/from the CPU port is currently done using
register accesses which is quite slow. DMA is possible but the port is not
able to absorb the whole switch bandwidth.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Random drive by comments because this is quite a number of lines to review!

Overall, looks quite good for a first version. Out of curiosity, is
there a particular switch test you ran this driver against? LNST?

+static int ocelot_mact_learn(struct ocelot *ocelot, int port,
+			     const unsigned char mac[ETH_ALEN],
+			     unsigned int vid,
+			     enum macaccess_entry_type type)
+{
+	u32 macl = 0, mach = 0;
+
+	/* Set the MAC address to learn and the vlan associated in a format
+	 * understood by the hardware.
+	 */
+	mach |= vid    << 16;
+	mach |= mac[0] << 8;
+	mach |= mac[1] << 0;
+	macl |= mac[2] << 24;
+	macl |= mac[3] << 16;
+	macl |= mac[4] << 8;
+	macl |= mac[5] << 0;
+
+	ocelot_write(ocelot, macl, ANA_TABLES_MACLDATA);
+	ocelot_write(ocelot, mach, ANA_TABLES_MACHDATA);

You are repeating this in the function right below, can you factor it
somehow into a common function that this one, and the one right below
could call?

[snip]

+static void ocelot_port_adjust_link(struct net_device *dev)
+{

This is fine for now, but I would suggest implementing PHYLINK to be
future proof.

[snip]

+static int ocelot_port_stop(struct net_device *dev)
+{
+	struct ocelot_port *port = netdev_priv(dev);
+
+	phy_disconnect(port->phy);
+
+	dev->phydev = NULL;

You don't have anything else to do, like disabling the port so it
possibly saves power or anything, aside from the PHY which will be
suspended here.

[snip]

+static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct ocelot_port *port = netdev_priv(dev);
+	struct ocelot *ocelot = port->ocelot;
+	u32 val, ifh[IFH_LEN];
+	struct frame_info info = {};
+	u8 grp = 0; /* Send everything on CPU group 0 */
+	int i, count, last;

unsigned int for these types.

+
+	val = ocelot_read(ocelot, QS_INJ_STATUS);
+	if (!(val & QS_INJ_STATUS_FIFO_RDY(BIT(grp))) ||
+	    (val & QS_INJ_STATUS_WMARK_REACHED(BIT(grp))))
+		return NETDEV_TX_BUSY;
+
+	ocelot_write_rix(ocelot, QS_INJ_CTRL_GAP_SIZE(1) |
+			 QS_INJ_CTRL_SOF, QS_INJ_CTRL, grp);
+
+	info.port = BIT(port->chip_port);
+	info.cpuq = 0xff;
+	ocelot_gen_ifh(ifh, &info);
+
+	for (i = 0; i < IFH_LEN; i++)
+		ocelot_write_rix(ocelot, ifh[i], QS_INJ_WR, grp);
+
+	count = (skb->len + 3) / 4;
+	last = skb->len % 4;
+	for (i = 0; i < count; i++) {
+		ocelot_write_rix(ocelot, cpu_to_le32(((u32 *)skb->data)[i]),
+				 QS_INJ_WR, grp);
+	}
+
+	/* Add padding */
+	while (i < (OCELOT_BUFFER_CELL_SZ / 4)) {
+		ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
+		i++;
+	}
+
+	/* Indicate EOF and valid bytes in last word */
+	ocelot_write_rix(ocelot, QS_INJ_CTRL_GAP_SIZE(1) |
+			 QS_INJ_CTRL_VLD_BYTES(skb->len < OCELOT_BUFFER_CELL_SZ ? 0 : last) |
+			 QS_INJ_CTRL_EOF,
+			 QS_INJ_CTRL, grp);
+
+	/* Add dummy CRC */
+	ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
+	skb_tx_timestamp(skb);
+
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+	dev_kfree_skb_any(skb);

No interrupt to indicate transmit completion?

+static int ocelot_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
+			  struct net_device *dev, const unsigned char *addr,
+			  u16 vid, u16 flags)
+{
+	struct ocelot_port *port = netdev_priv(dev);
+	struct ocelot *ocelot = port->ocelot;
+
+	if (!vid) {
+		if (!port->vlan_aware)
+			/* If the bridge is not VLAN aware and no VID was
+			 * provided, set it to 1 as bridges have a default VID
+			 * of 1. Otherwise the MAC entry wouldn't match incoming
+			 * packets as the VID would differ (0 != 1).
+			 */
+			vid = 1;
+		else
+			/* If the bridge is VLAN aware a VID must be provided as
+			 * otherwise the learnt entry wouldn't match any frame.
+			 */
+			return -EINVAL;
+	}

So if we are targeting vid = 0 we end-up with vid = 1 possibly?

[snip]

+static int ocelot_port_attr_stp_state_set(struct ocelot_port *ocelot_port,
+					  struct switchdev_trans *trans,
+					  u8 state)
+{
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	u32 port_cfg;
+	int port, i;
+
+	if (switchdev_trans_ph_prepare(trans))
+		return 0;
+
+	if (!(BIT(ocelot_port->chip_port) & ocelot->bridge_mask))
+		return 0;
+
+	port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG,
+				   ocelot_port->chip_port);
+
+	switch (state) {
+	case BR_STATE_FORWARDING:
+		ocelot->bridge_fwd_mask |= BIT(ocelot_port->chip_port);
+		/* Fallthrough */
+	case BR_STATE_LEARNING:
+		port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
+		break;
+
+	default:
+		port_cfg &= ~ANA_PORT_PORT_CFG_LEARN_ENA;
+		ocelot->bridge_fwd_mask &= ~BIT(ocelot_port->chip_port);

Missing break, even if this is the default case.

+	}
+
+	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG,
+			 ocelot_port->chip_port);
+
+	/* Apply FWD mask. The loop is needed to add/remove the current port as
+	 * a source for the other ports.
+	 */
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		if (ocelot->bridge_fwd_mask & BIT(port)) {
+			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
+
+			for (i = 0; i < ocelot->num_phys_ports; i++) {
+				unsigned long bond_mask = ocelot->lags[i];
+
+				if (!bond_mask)
+					continue;
+
+				if (bond_mask & BIT(port)) {
+					mask &= ~bond_mask;
+					break;
+				}
+			}
+
+			ocelot_write_rix(ocelot,
+					 BIT(ocelot->num_phys_ports) | mask,
+					 ANA_PGID_PGID, PGID_SRC + port);
+		} else {
+			/* Only the CPU port, this is compatible with link
+			 * aggregation.
+			 */
+			ocelot_write_rix(ocelot,
+					 BIT(ocelot->num_phys_ports),
+					 ANA_PGID_PGID, PGID_SRC + port);
+		}

All of this sounds like it should be moved into the br_join/leave, this
does not appear to be the right place to do that.

[snip]

+static int ocelot_port_attr_set(struct net_device *dev,
+				const struct switchdev_attr *attr,
+				struct switchdev_trans *trans)
+{
+	struct ocelot_port *ocelot_port = netdev_priv(dev);
+	int err = 0;

Should not this be EOPNOTSUPP by default so your cases below are
properly handled, like BRIDGE_FLAGS, MROUTER etc.

+
+	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		ocelot_port_attr_stp_state_set(ocelot_port, trans,
+					       attr->u.stp_state);
+		break;
+	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
+		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
+		ocelot_port_attr_ageing_set(ocelot_port, attr->u.ageing_time);
+		break;
+	case SWITCHDEV_ATTR_ID_PORT_MROUTER:
+		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
+		ocelot_port_attr_mc_set(ocelot_port, !attr->u.mc_disabled);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static struct ocelot_multicast *ocelot_multicast_get(struct ocelot *ocelot,
+						     const unsigned char *addr,
+						     u16 vid)
+{
+	struct ocelot_multicast *mc;
+
+	list_for_each_entry(mc, &ocelot->multicast, list) {
+		if (ether_addr_equal(mc->addr, addr) && mc->vid == vid)
+			return mc;
+	}
+
+	return NULL;
+}

+static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
+{
+	struct ocelot *ocelot = arg;
+	int i = 0, grp = 0;
+	int err = 0;
+
+	if (!(ocelot_read(ocelot, QS_XTR_DATA_PRESENT) & BIT(grp)))
+		return IRQ_NONE;
+
+	do {
+		struct sk_buff *skb;
+		struct net_device *dev;
+		u32 *buf;
+		int sz, len;
+		u32 ifh[4];
+		u32 val;
+		struct frame_info info;
+
+		for (i = 0; i < IFH_LEN; i++) {
+			err = ocelot_rx_frame_word(ocelot, grp, true, &ifh[i]);
+			if (err != 4)
+				break;
+		}

NAPI maybe?

[snip]

+	ocelot->targets[SYS] = ocelot_io_platform_init(ocelot, pdev, "sys");
+	if (IS_ERR(ocelot->targets[SYS]))
+		return PTR_ERR(ocelot->targets[SYS]);

You can clearly make this in a loop instead of repeating this section,
you just need an array of register names to be looking for.

[snip]

+	if (np) {

Please rework the indentation here, check for !np

+		for_each_child_of_node(np, portnp) {

for_each_available_child_of_node() you should be able to mark specific
ports as being disabled and skip over these accordingly.


[snip]

+int ocelot_regfields_init(struct ocelot *ocelot,
+			  const struct reg_field *const regfields)
+{
+	int i;

unsigned int i
-- 
Florian