Date:   Tue, 22 Jun 2021 17:03:25 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Joakim Zhang <qiangqing.zhang@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mark Einon <mark.einon@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-kernel@...ts.infradead.org
Subject: Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP
 L2 switch

> +static void write_atable(struct mtipl2sw_priv *priv, int index,
> +	unsigned long write_lo, unsigned long write_hi)
> +{
> +	unsigned long atable_base = (unsigned long)priv->hwentry;
> +
> +	writel(write_lo, (volatile void *)atable_base + (index << 3));
> +	writel(write_hi, (volatile void *)atable_base + (index << 3) + 4);

Using volatile is generally wrong. Why do you need it?

 > +}
> +
> +/*
> + * Clear complete MAC Look Up Table
> + */
> +static void esw_clear_atable(struct mtipl2sw_priv *priv)
> +{
> +	int index;
> +	for (index = 0; index < 2048; index++)
> +		write_atable(priv, index, 0, 0);
> +}
> +
> +static int mtipl2_sw_enable(struct mtipl2sw_priv *priv)
> +{
> +	/*
> +	 * L2 switch - reset
> +	 */
> +	writel(MCF_ESW_MODE_SW_RST, &priv->fecp->ESW_MODE);
> +	udelay(10);
> +
> +	/* Configure switch*/
> +	writel(MCF_ESW_MODE_STATRST, &priv->fecp->ESW_MODE);
> +	writel(MCF_ESW_MODE_SW_EN, &priv->fecp->ESW_MODE);
> +
> +	/* Management port configuration, make port 0 as management port */
> +	writel(0, &priv->fecp->ESW_BMPC);
> +
> +	/*
> +	 * Set backpressure threshold to minimize discarded frames
> +	 * during due to congestion.
> +	 */
> +	writel(P0BC_THRESHOLD, &priv->fecp->ESW_P0BCT);
> +
> +	/* Set the max rx buffer size */
> +	writel(L2SW_PKT_MAXBLR_SIZE, priv->hwpsw + MCF_ESW_R_BUFF_SIZE);
> +	/* Enable broadcast on all ports */
> +	writel(0x7, &priv->fecp->ESW_DBCR);
> +
> +	/* Enable multicast on all ports */
> +	writel(0x7, &priv->fecp->ESW_DMCR);
> +
> +	esw_clear_atable(priv);
> +
> +	/* Clear all pending interrupts */
> +	writel(0xffffffff, priv->hwpsw + FEC_IEVENT);
> +
> +	/* Enable interrupts we wish to service */
> +	writel(FEC_MTIP_DEFAULT_IMASK, priv->hwpsw + FEC_IMASK);
> +	priv->l2sw_on = true;
> +
> +	return 0;
> +}
> +
> +static void mtipl2_sw_disable(struct mtipl2sw_priv *priv)
> +{
> +	writel(0, &priv->fecp->ESW_MODE);
> +}
> +
> +static int mtipl2_port_enable (struct mtipl2sw_priv *priv, int port)
> +{
> +	u32 l2_ports_en;
> +
> +	pr_err("%s: PORT ENABLE %d\n", __func__, port);
> +
> +	/* Enable tx/rx on L2 switch ports */
> +	l2_ports_en = readl(&priv->fecp->ESW_PER);
> +	if (!(l2_ports_en & MCF_ESW_ENA_PORT_0))
> +		l2_ports_en = MCF_ESW_ENA_PORT_0;
> +
> +	if (port == 0 && !(l2_ports_en & MCF_ESW_ENA_PORT_1))
> +		l2_ports_en |= MCF_ESW_ENA_PORT_1;
> +
> +	if (port == 1 && !(l2_ports_en & MCF_ESW_ENA_PORT_2))
> +		l2_ports_en |= MCF_ESW_ENA_PORT_2;
> +
> +	writel(l2_ports_en, &priv->fecp->ESW_PER);
> +
> +	/*
> +	 * Check if MAC IP block is already enabled (after switch initializtion)
> +	 * or if we need to enable it after mtipl2_port_disable was called.
> +	 */
> +
> +	return 0;
> +}
> +
> +static void mtipl2_port_disable (struct mtipl2sw_priv *priv, int port)
> +{
> +	u32 l2_ports_en;
> +
> +	pr_err(" %s: PORT DISABLE %d\n", __func__, port);

Please clean up debug code this this.

> +
> +	l2_ports_en = readl(&priv->fecp->ESW_PER);
> +	if (port == 0)
> +		l2_ports_en &= ~MCF_ESW_ENA_PORT_1;
> +
> +	if (port == 1)
> +		l2_ports_en &= ~MCF_ESW_ENA_PORT_2;
> +
> +	/* Finally disable tx/rx on port 0 */
> +	if (!(l2_ports_en & MCF_ESW_ENA_PORT_1) &&
> +	    !(l2_ports_en & MCF_ESW_ENA_PORT_2))
> +		l2_ports_en &= ~MCF_ESW_ENA_PORT_0;
> +
> +	writel(l2_ports_en, &priv->fecp->ESW_PER);
> +}
> +
> +irqreturn_t
> +mtip_l2sw_interrupt_handler(int irq, void *dev_id)
> +{
> +	struct mtipl2sw_priv *priv = dev_id;
> +	struct fec_enet_private *fep = priv->fep[0];
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 int_events, int_imask;

Reverse christmas tree.

> +
> +	int_events = readl(fec_hwp(fep) + FEC_IEVENT);
> +	writel(int_events, fec_hwp(fep) + FEC_IEVENT);
> +
> +	if ((int_events & FEC_MTIP_DEFAULT_IMASK) && fep->link) {
> +		ret = IRQ_HANDLED;
> +
> +		if (napi_schedule_prep(&fep->napi)) {
> +			/* Disable RX interrupts */
> +			int_imask = readl(fec_hwp(fep) + FEC_IMASK);
> +			int_imask &= ~FEC_MTIP_ENET_RXF;
> +			writel(int_imask, fec_hwp(fep) + FEC_IMASK);
> +			__napi_schedule(&fep->napi);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int mtipl2_parse_of(struct mtipl2sw_priv *priv, struct device_node *np)
> +{
> +	struct device_node *port, *p, *fep_np;
> +	struct platform_device *pdev;
> +	struct net_device *ndev;
> +	unsigned int port_num;
> +
> +	p = of_find_node_by_name(np, "ports");
> +
> +	for_each_available_child_of_node(p, port) {
> +		if (of_property_read_u32(port, "reg", &port_num))
> +			continue;
> +
> +		priv->n_ports = port_num;
> +
> +		fep_np = of_parse_phandle(port, "phy-handle", 0);

As i said, phy-handle points to a phy. It minimum, you need to call
this mac-handle. But that then makes this switch driver very different
to every other switch driver.

> +		pdev = of_find_device_by_node(fep_np);
> +		ndev = platform_get_drvdata(pdev);
> +		priv->fep[port_num - 1] = netdev_priv(ndev);

What happens when somebody puts reg=<42>; in DT?

I would say, your basic structure needs to change, to make it more
like other switchdev drivers. You need to replace the two FEC device
instances with one switchdev driver. The switchdev driver will then
instantiate the two netdevs for the two external MACs. You can
hopefully reuse some of the FEC code for that, but i expect you are
going to have to refactor the FEC driver and turn part of it into a
library, which the switchdev driver can then use.

	 Andrew

Powered by blists - more mailing lists