Message-ID: <20210623133704.334a84df@ktm>
Date:   Wed, 23 Jun 2021 13:37:04 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
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

Hi Andrew,

> > +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?

This was the code, which I took from the legacy driver. I will adjust
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.
> 

Ok.

> > +
> > +	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.

Ok.

> 
> > +
> > +	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.

Other drivers (DSA for example) use "ethernet" or "link" properties.
Maybe those can be reused?

> 
> > +		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 do guess that this will break the code.

However, DSA DT descriptions also rely on the exact numbering [1] (via
e.g. reg property) of the ports. I've followed this paradigm.

> 
> 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.

I've used the cpsw_new.c as the example.

> The switchdev driver will then
> instantiate the two netdevs for the two external MACs.

Then there is a question - what about eth[01], which already exists?

Shall I close them and then reuse (create as a new one?) eth0 to be
connected to switch port0 (via DMA0)?

Then, I do need two net_device ports, which would only control PHY
device and setup ENET-MAC for rmii, as the L2 switch will provide data
for transmission. Those two ports are connected to switch's port[12]
and look very similar to ports created by DSA driver (but shall not
transmit and receive data).

Maybe I've overlooked something, but the rocker switchdev driver
(rocker_main.c) sets netdev_ops (with .ndo_start_xmit) for ports which
it creates. The prestera's prestera_sdma_xmit() (in prestera_rxtx.c)
also setups the SDMA for those.

In i.MX L2 switch case - one just needs to setup DMA0 to send data to
engress ports. IMHO, those ports just need to handle PHY link (up/down
10/100 change) and statistics.

> 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.

To be honest - such driver for L2 switch already has been forward
ported by me [2] to v4.19.

It has the L2 switch enabled after the boot and there is no way to
disable it.

When you look on the code - it is a copy-paste of the FEC driver, with
some necessary adjustments. 

The FEC driver itself is large and used by almost _ALL_ i.MX SoCs.
Turning it into library will move the already working code around. I
wanted to avoid it.

The idea behind this patch series is as follows (to offload bridging to
MTIP L2 switch IP block):
-------------------------
1. When bridge is created disable eth0 and eth1 (fec_close)

2. Set a flag for fec driver, so DMA descriptors registers and ones to
initiate transfer are adjusted for DMA0 (eth0) device. Also L2 switch
IP block has different bits positions for interrupts.

3. DMA1 - which with normal setup corresponds to eth1 - is not used.

4. FEC driver also monitors PHY changes (link up/down speed change
10/100) (for both eth0, eth1).

5. When br0 is deleted - the mtip support flag is cleared and FEC
network interfaces (eth[01]) are opened again (via fec_open).

With above approach many operations are already performed - like
ENET-MAC setup, buffers allocation, etc.
To make L2 switch working with this setup we need to re-map 4 registers
and two interrupt bits in fec_main driver.

I'm just wondering if is it worth to refactor already working driver to
library, instantiate new interfaces and re-init all the already
initialized stuff ?


> 
> 	 Andrew


Links:

[1] -
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/armada-388-clearfog.dts#L93

[2] - https://github.com/lmajewski/linux-imx28-l2switch/commits/master

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists