On 07/25/2018 06:12 PM, Andrew Lunn wrote:

quoted

 LANTIQ MIPS ARCHITECTURE
 M:	John Crispin [off-list ref]

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 2b81b97e994f..f1280aa3f9bd 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig

@@ -23,6 +23,14 @@ config NET_DSA_LOOP
 	  This enables support for a fake mock-up switch chip which
 	  exercises the DSA APIs.
 
+config NET_DSA_GSWIP
+	tristate "Intel / Lantiq GSWIP"

Minor nit pick. Could you make this NET_DSA_LANTIQ_GSWIP. We generally
have some manufacture ID in the name. And change the text to Lantiq /
Intel GSWIP.

done

quoted

+static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
+	/** Receive Packet Count (only packets that are accepted and not discarded). */
+	MIB_DESC(1, 0x1F, "RxGoodPkts"),
+	/** Receive Unicast Packet Count. */
+	MIB_DESC(1, 0x23, "RxUnicastPkts"),
+	/** Receive Multicast Packet Count. */
+	MIB_DESC(1, 0x22, "RxMulticastPkts"),
+	/** Receive FCS Error Packet Count. */
+	MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
+	/** Receive Undersize Good Packet Count. */
+	MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
+	/** Receive Undersize Error Packet Count. */
+	MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
+	/** Receive Oversize Good Packet Count. */
+	MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
+	/** Receive Oversize Error Packet Count. */
+	MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
+	/** Receive Good Pause Packet Count. */
+	MIB_DESC(1, 0x20, "RxGoodPausePkts"),
+	/** Receive Align Error Packet Count. */
+	MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
+	/** Receive Size 64 Packet Count. */
+	MIB_DESC(1, 0x12, "Rx64BytePkts"),
+	/** Receive Size 65-127 Packet Count. */
+	MIB_DESC(1, 0x13, "Rx127BytePkts"),
+	/** Receive Size 128-255 Packet Count. */
+	MIB_DESC(1, 0x14, "Rx255BytePkts"),
+	/** Receive Size 256-511 Packet Count. */
+	MIB_DESC(1, 0x15, "Rx511BytePkts"),
+	/** Receive Size 512-1023 Packet Count. */
+	MIB_DESC(1, 0x16, "Rx1023BytePkts"),
+	/** Receive Size 1024-1522 (or more, if configured) Packet Count. */
+	MIB_DESC(1, 0x17, "RxMaxBytePkts"),
+	/** Receive Dropped Packet Count. */
+	MIB_DESC(1, 0x18, "RxDroppedPkts"),
+	/** Filtered Packet Count. */
+	MIB_DESC(1, 0x19, "RxFilteredPkts"),
+	/** Receive Good Byte Count (64 bit). */
+	MIB_DESC(2, 0x24, "RxGoodBytes"),
+	/** Receive Bad Byte Count (64 bit). */
+	MIB_DESC(2, 0x26, "RxBadBytes"),
+	/** Transmit Dropped Packet Count, based on Congestion Management. */
+	MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
+	/** Transmit Packet Count. */
+	MIB_DESC(1, 0x0C, "TxGoodPkts"),
+	/** Transmit Unicast Packet Count. */
+	MIB_DESC(1, 0x06, "TxUnicastPkts"),
+	/** Transmit Multicast Packet Count. */
+	MIB_DESC(1, 0x07, "TxMulticastPkts"),
+	/** Transmit Size 64 Packet Count. */
+	MIB_DESC(1, 0x00, "Tx64BytePkts"),
+	/** Transmit Size 65-127 Packet Count. */
+	MIB_DESC(1, 0x01, "Tx127BytePkts"),
+	/** Transmit Size 128-255 Packet Count. */
+	MIB_DESC(1, 0x02, "Tx255BytePkts"),
+	/** Transmit Size 256-511 Packet Count. */
+	MIB_DESC(1, 0x03, "Tx511BytePkts"),
+	/** Transmit Size 512-1023 Packet Count. */
+	MIB_DESC(1, 0x04, "Tx1023BytePkts"),
+	/** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
+	MIB_DESC(1, 0x05, "TxMaxBytePkts"),
+	/** Transmit Single Collision Count. */
+	MIB_DESC(1, 0x08, "TxSingleCollCount"),
+	/** Transmit Multiple Collision Count. */
+	MIB_DESC(1, 0x09, "TxMultCollCount"),
+	/** Transmit Late Collision Count. */
+	MIB_DESC(1, 0x0A, "TxLateCollCount"),
+	/** Transmit Excessive Collision Count. */
+	MIB_DESC(1, 0x0B, "TxExcessCollCount"),
+	/** Transmit Pause Packet Count. */
+	MIB_DESC(1, 0x0D, "TxPauseCount"),
+	/** Transmit Drop Packet Count. */
+	MIB_DESC(1, 0x10, "TxDroppedPkts"),
+	/** Transmit Good Byte Count (64 bit). */
+	MIB_DESC(2, 0x0E, "TxGoodBytes"),

Most of the comments here don't add anything useful. Maybe remove
them?

Ok I removed them. Are the names ok, or should they follow any Linux
definition?

quoted

+};
+
+static u32 gswip_switch_r(struct gswip_priv *priv, u32 offset)
+{
+	return __raw_readl(priv->gswip + (offset * 4));
+}
+
+static void gswip_switch_w(struct gswip_priv *priv, u32 val, u32 offset)
+{
+	return __raw_writel(val, priv->gswip + (offset * 4));
+}

Since this is MIPS, i assume re-ordering cannot happen, there are
barriers, etc?

As far as I know this is not a problem on this bus and no barriers are
needed here.

quoted

+static int xrx200_mdio_poll(struct gswip_priv *priv)
+{
+	int cnt = 10000;
+
+	while (likely(cnt--)) {
+		u32 ctrl = gswip_mdio_r(priv, GSWIP_MDIO_CTRL);
+
+		if ((ctrl & GSWIP_MDIO_CTRL_BUSY) == 0)
+			return 0;
+		cpu_relax();
+	}
+
+	return 1;
+}

Please return ETIMEOUT when needed. Maybe use one of the variants of
readx_poll_timeout().

I am returning ETIMEOUT now.

When I would use readx_poll_timeout() I can not use the gswip_mdio_r()
function, because it takes two arguments and would have to use readl
directly. I do not think that this would make the code much better
readable.

quoted

+
+static int xrx200_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+	struct gswip_priv *priv = bus->priv;
+
+	if (xrx200_mdio_poll(priv))
+		return -EIO;

EIO is wrong, it should be a timeout. Solved above...

OK I replaced this now with this code:

err = gswip_mdio_poll(priv);
if (err)
	return err;

quoted

+
+	gswip_mdio_w(priv, val, GSWIP_MDIO_WRITE);
+	gswip_mdio_w(priv, GSWIP_MDIO_CTRL_BUSY | GSWIP_MDIO_CTRL_WR |
+		((addr & GSWIP_MDIO_CTRL_PHYAD_MASK) << GSWIP_MDIO_CTRL_PHYAD_SHIFT) |
+		(reg & GSWIP_MDIO_CTRL_REGAD_MASK),
+		GSWIP_MDIO_CTRL);
+
+	return 0;
+}
+

quoted

+static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
+{
+	struct dsa_switch *ds = priv->ds;
+
+	ds->slave_mii_bus = devm_mdiobus_alloc(priv->dev);
+	if (!ds->slave_mii_bus)
+		return -ENOMEM;
+
+	ds->slave_mii_bus->priv = priv;
+	ds->slave_mii_bus->read = xrx200_mdio_rd;
+	ds->slave_mii_bus->write = xrx200_mdio_wr;
+	ds->slave_mii_bus->name = "lantiq,xrx200-mdio";
+	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%x", 0);

You should be able to do better than that.

I replaced this now with this code:
snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
dev_name(priv->dev));

quoted

+	ds->slave_mii_bus->parent = priv->dev;
+	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
+
+	return of_mdiobus_register(ds->slave_mii_bus, mdio_np);
+}
+
+static void gswip_wait_pce_tbl_ready(struct gswip_priv *priv)
+{
+	while (gswip_switch_r(priv, GSWIP_PCE_TBL_CTRL) & GSWIP_PCE_TBL_CTRL_BAS)
+		cond_resched();

Please add some form of timeout.

I introduced a new function gswip_switch_r_timeout() which uses
readx_poll_timeout().

quoted

+}
+
+static int gswip_port_enable(struct dsa_switch *ds, int port,
+			     struct phy_device *phy)
+{
+	struct gswip_priv *priv = (struct gswip_priv *)ds->priv;

Casts like this are not needed.

removed

quoted

+	/* RMON Counter Enable for port */
+	gswip_switch_w(priv, GSWIP_BM_PCFG_CNTEN, GSWIP_BM_PCFGp(port));
+
+	/* enable port fetch/store dma & VLAN Modification */
+	gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_EN |
+				   GSWIP_FDMA_PCTRL_VLANMOD_BOTH,
+			 GSWIP_FDMA_PCTRLp(port));
+	gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
+			  GSWIP_SDMA_PCTRLp(port));
+	gswip_switch_mask(priv, 0, GSWIP_PCE_PCTRL_0_INGRESS,
+			  GSWIP_PCE_PCTRL_0p(port));
+
+	return 0;
+}
+

quoted

+static int gswip_setup(struct dsa_switch *ds)
+{
+	struct gswip_priv *priv = ds->priv;
+	int i;
+
+	gswip_switch_w(priv, GSWIP_ETHSW_SWRES_R0, GSWIP_ETHSW_SWRES);
+	usleep_range(5000, 10000);
+	gswip_switch_w(priv, 0, GSWIP_ETHSW_SWRES);
+
+	/* disable port fetch/store dma, assume CPU port is last port */

Since this comes from device tree, you should really verify that and
return EINVAL if not, in the probe() function.

I defined a struct hw_info which describes the details of this switch,
max port numbers and CPU port, as this is hard coded and this is then
compared to the device tree settings.

quoted

+	for (i = 0; i <= priv->cpu_port; i++)
+		gswip_port_disable(ds, i, NULL);
+
+	/* enable Switch */
+	gswip_mdio_mask(priv, 0, GSWIP_MDIO_GLOB_ENABLE, GSWIP_MDIO_GLOB);
+
+	xrx200_pci_microcode(priv);
+
+	/* Default unknown Broadcast/Multicast/Unicast port maps */
+	gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP1);
+	gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP2);
+	gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP3);
+
+	/* disable auto polling */
+	gswip_mdio_w(priv, 0x0, GSWIP_MDIO_MDC_CFG0);
+
+	/* enable special tag insertion on cpu port */
+	gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN,
+			  GSWIP_FDMA_PCTRLp(priv->cpu_port));
+
+	gswip_switch_mask(priv, 0, GSWIP_MAC_CTRL_2_MLEN,
+			  GSWIP_MAC_CTRL_2p(priv->cpu_port));
+	gswip_switch_w(priv, VLAN_ETH_FRAME_LEN + 8, GSWIP_MAC_FLEN);
+	gswip_switch_mask(priv, 0, GSWIP_BM_QUEUE_GCTRL_GL_MOD,
+			  GSWIP_BM_QUEUE_GCTRL);
+
+	/* VLAN aware Switching */
+	gswip_switch_mask(priv, 0, GSWIP_PCE_GCTRL_0_VLAN, GSWIP_PCE_GCTRL_0);
+
+	/* Mac Address Table Lock */
+	gswip_switch_mask(priv, 0, GSWIP_PCE_GCTRL_1_MAC_GLOCK |
+				   GSWIP_PCE_GCTRL_1_MAC_GLOCK_MOD,
+			  GSWIP_PCE_GCTRL_1);
+
+	gswip_port_enable(ds, priv->cpu_port, NULL);
+	return 0;
+}
+
+static void gswip_adjust_link(struct dsa_switch *ds, int port,
+			      struct phy_device *phydev)
+{
+	struct gswip_priv *priv = (struct gswip_priv *)ds->priv;
+	u16 phyaddr = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
+	u16 miirate = 0;
+	u16 miimode;
+	u16 lcl_adv = 0, rmt_adv = 0;
+	u8 flowctrl;
+
+	/* do not run this for the CPU port 6 */
+	if (port >= priv->cpu_port)

Please use  dsa_is_cpu_port(ds, port)

done

quoted

+		return;
+
+	miimode = gswip_mdio_r(priv, GSWIP_MII_CFGp(port));
+	miimode &= GSWIP_MII_CFG_MODE_MASK;
+
+	switch (phydev->speed) {
+	case SPEED_1000:
+		phyaddr |= GSWIP_MDIO_PHY_SPEED_G1;
+		miirate = GSWIP_MII_CFG_RATE_M125;
+		break;
+
+	case SPEED_100:
+		phyaddr |= GSWIP_MDIO_PHY_SPEED_M100;
+		switch (miimode) {
+		case GSWIP_MII_CFG_MODE_RMIIM:
+		case GSWIP_MII_CFG_MODE_RMIIP:
+			miirate = GSWIP_MII_CFG_RATE_M50;
+			break;
+		default:
+			miirate = GSWIP_MII_CFG_RATE_M25;
+			break;
+		}
+		break;
+
+	default:
+		phyaddr |= GSWIP_MDIO_PHY_SPEED_M10;
+		miirate = GSWIP_MII_CFG_RATE_M2P5;
+		break;
+	}
+
+	if (phydev->link)
+		phyaddr |= GSWIP_MDIO_PHY_LINK_UP;
+	else
+		phyaddr |= GSWIP_MDIO_PHY_LINK_DOWN;
+
+	if (phydev->duplex == DUPLEX_FULL)
+		phyaddr |= GSWIP_MDIO_PHY_FDUP_EN;
+	else
+		phyaddr |= GSWIP_MDIO_PHY_FDUP_DIS;
+
+	if (phydev->pause)
+		rmt_adv = LPA_PAUSE_CAP;
+	if (phydev->asym_pause)
+		rmt_adv |= LPA_PAUSE_ASYM;
+
+	if (phydev->advertising & ADVERTISED_Pause)
+		lcl_adv |= ADVERTISE_PAUSE_CAP;
+	if (phydev->advertising & ADVERTISED_Asym_Pause)
+		lcl_adv |= ADVERTISE_PAUSE_ASYM;
+
+	flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
+
+	if (flowctrl & FLOW_CTRL_TX)
+		phyaddr |= GSWIP_MDIO_PHY_FCONTX_EN;
+	else
+		phyaddr |= GSWIP_MDIO_PHY_FCONTX_DIS;
+	if (flowctrl & FLOW_CTRL_RX)
+		phyaddr |= GSWIP_MDIO_PHY_FCONRX_EN;
+	else
+		phyaddr |= GSWIP_MDIO_PHY_FCONRX_DIS;
+
+	gswip_mdio_mask(priv, GSWIP_MDIO_PHY_MASK, phyaddr,
+			GSWIP_MDIO_PHYp(port));

The names make this unclear. The callback is used to configure the MAC
layer when something happens at the PHY layer. phyaddr does not appear
to be an address, not should it be doing anything to a PHY.

I renamed this to phyconf, as this contains multiple configuration
values. This tells the mac what settings the phy wants to use.

quoted

+	gswip_mii_mask(priv, GSWIP_MII_CFG_RATE_MASK, miirate,
+		       GSWIP_MII_CFGp(port));
+}
+
+static u32 gswip_bcm_ram_entry_read(struct gswip_priv *priv, u32 table,
+				    u32 index)
+{
+	u32 result;
+
+	gswip_switch_w(priv, index, GSWIP_BM_RAM_ADDR);
+	gswip_switch_mask(priv, GSWIP_BM_RAM_CTRL_ADDR_MASK |
+				GSWIP_BM_RAM_CTRL_OPMOD,
+			      table | GSWIP_BM_RAM_CTRL_BAS,
+			      GSWIP_BM_RAM_CTRL);
+
+	while (gswip_switch_r(priv, GSWIP_BM_RAM_CTRL) & GSWIP_BM_RAM_CTRL_BAS)
+		cond_resched();

Please add a timeout.

I introduced a new funtion gswip_switch_r_timeout() which uses
readx_poll_timeout().

quoted

+
+	result = gswip_switch_r(priv, GSWIP_BM_RAM_VAL(0));
+	result |= gswip_switch_r(priv, GSWIP_BM_RAM_VAL(1)) << 16;
+
+	return result;
+}
+

quoted

+static int gswip_probe(struct platform_device *pdev)
+{
+	struct gswip_priv *priv;
+	struct resource *gswip_res, *mdio_res, *mii_res;
+	struct device_node *mdio_np;
+	struct device *dev = &pdev->dev;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	gswip_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->gswip = devm_ioremap_resource(dev, gswip_res);
+	if (!priv->gswip)
+		return -ENOMEM;
+
+	mdio_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->mdio = devm_ioremap_resource(dev, mdio_res);
+	if (!priv->mdio)
+		return -ENOMEM;
+
+	mii_res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	priv->mii = devm_ioremap_resource(dev, mii_res);
+	if (!priv->mii)
+		return -ENOMEM;
+
+	priv->ds = dsa_switch_alloc(dev, DSA_MAX_PORTS);

If you know how many ports there are, it is better to pass it.

done

quoted

+	if (!priv->ds)
+		return -ENOMEM;
+
+	priv->ds->priv = priv;
+	priv->ds->ops = &gswip_switch_ops;
+	priv->dev = dev;
+	priv->cpu_port = 6;
+
+	/* bring up the mdio bus */
+	mdio_np = of_find_compatible_node(pdev->dev.of_node, NULL,
+					  "lantiq,xrx200-mdio");
+	if (mdio_np) {
+		err = gswip_mdio(priv, mdio_np);
+		if (err) {
+			dev_err(dev, "mdio probe failed\n");
+			return err;
+		}
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	err = dsa_register_switch(priv->ds);
+	if (err) {
+		dev_err(dev, "dsa switch register failed: %i\n", err);
+		if (mdio_np)
+			mdiobus_unregister(priv->ds->slave_mii_bus);
+	}
+
+	return err;
+}

quoted

+static const struct of_device_id gswip_of_match[] = {
+	{ .compatible = "lantiq,xrx200-gswip" },
+	{},
+};

Please add device tree documentation.

Will do that.

quoted

+MODULE_DEVICE_TABLE(of, xrx200_match);
+
+static struct platform_driver gswip_driver = {
+	.probe = gswip_probe,
+	.remove = gswip_remove,
+	.driver = {
+		.name = "gswip",
+		.of_match_table = gswip_of_match,
+	},
+};
+#define MC_ENTRY(val, msk, ns, out, len, type, flags, ipv4_len) \
+	{ val, msk, (ns << 10 | out << 4 | len >> 1),\
+		(len & 1) << 15 | type << 13 | flags << 9 | ipv4_len << 8 }
+static const struct gswip_pce_microcode gswip_pce_microcode[] = {

How big is this table? I'm wondering if it should be loaded from
/lib/firmware. Or can it be marked __initdata?

This is sort of a firmware, but it is also in the GPL driver.
Currently the probe function is not marked __init so we can not make
this easily __initdata.
It has 64 entries of 8 bytes each so, 512 bytes, I think we can put this
into the code.

Hauke