Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

Lukasz Majewski
2025-04-09T15:02:19Z

[-- Attachment #1: Type: text/plain, Size: 21583 bytes --]

Hi Simon,

> On Mon, Apr 07, 2025 at 04:51:56PM +0200, Lukasz Majewski wrote:
> > This patch series provides support for More Than IP L2 switch
> > embedded in the imx287 SoC.
> >
> > This is a two port switch (placed between uDMA[01] and MAC-NET[01]),
> > which can be used for offloading the network traffic.
> >
> > It can be used interchangeably with current FEC driver - to be more
> > specific: one can use either of it, depending on the requirements.
> >
> > The biggest difference is the usage of DMA - when FEC is used,
> > separate DMAs are available for each ENET-MAC block.
> > However, with switch enabled - only the DMA0 is used to
> > send/receive data to/form switch (and then switch sends them to
> > respecitive ports).
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
>
> Hi Lukasz,
>
> This is not a complete review, but I did spend a bit of time
> looking over this and have provided some feedback on
> things I noticed below.
>

Thanks for your feedback.

> ...
>
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > b/drivers/net/ethernet/freescale/mtipsw/Kconfig new file mode 100644
> > index 000000000000..450ff734a321
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config FEC_MTIP_L2SW
> > + tristate "MoreThanIP L2 switch support to FEC driver"
> > + depends on OF
> > + depends on NET_SWITCHDEV
> > + depends on BRIDGE
> > + depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST
> > + help
> > + This enables support for the MoreThan IP L2 switch on
> > i.MX
> > + SoCs (e.g. iMX28, vf610). It offloads bridging to this
> > IP block's
> > + hardware and allows switch management with standard
> > Linux tools.
> > + This switch driver can be used interchangeable with the
> > already
> > + available FEC driver, depending on the use case's
> > requirments.
>
> nit: requirements
>
> Flagged by checkpatch.pl --codespell
>

Ok.

> ...
>
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
>
> ...
>
> > +static void mtip_enet_init(struct switch_enet_private *fep, int
> > port) +{
> > + void __iomem *enet_addr = fep->enet_addr;
> > + u32 mii_speed, holdtime, tmp;
>
> I think it would be best to avoid variable names like tmp which have
> little meaning. Although still rather generic, perhaps reg would be
> more appropriate. Or better still something relating to the name the
> register, say rcr.

Ok, I will use reg/rcr instead of tmp.

>
> > +
> > + if (port == 2)
> > + enet_addr += MCF_ESW_ENET_PORT_OFFSET;
> > +
> > + tmp = MCF_FEC_RCR_PROM | MCF_FEC_RCR_MII_MODE |
> > + MCF_FEC_RCR_MAX_FL(1522);
> > +
> > + if (fep->phy_interface[port - 1] ==
> > PHY_INTERFACE_MODE_RMII)
> > + tmp |= MCF_FEC_RCR_RMII_MODE;
> > +
> > + writel(tmp, enet_addr + MCF_FEC_RCR);
> > +
> > + /* TCR */
> > + writel(MCF_FEC_TCR_FDEN, enet_addr + MCF_FEC_TCR);
> > +
> > + /* ECR */
> > + writel(MCF_FEC_ECR_ETHER_EN, enet_addr + MCF_FEC_ECR);
> > +
> > + /* Set MII speed to 2.5 MHz
> > + */
> > + mii_speed = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg),
> > 5000000);
> > + mii_speed--;
> > +
> > + /* The i.MX28 and i.MX6 types have another filed in the
> > MSCR (aka
> > + * MII_SPEED) register that defines the MDIO output hold
> > time. Earlier
> > + * versions are RAZ there, so just ignore the difference
> > and write the
> > + * register always.
> > + * The minimal hold time according to IEE802.3 (clause 22)
> > is 10 ns.
> > + * HOLDTIME + 1 is the number of clk cycles the fec is
> > holding the
> > + * output.
> > + * The HOLDTIME bitfield takes values between 0 and 7
> > (inclusive).
> > + * Given that ceil(clkrate / 5000000) <= 64, the
> > calculation for
> > + * holdtime cannot result in a value greater than 3.
> > + */
> > + holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg),
> > 100000000) - 1; +
> > + fep->phy_speed = mii_speed << 1 | holdtime << 8;
> > +
> > + writel(fep->phy_speed, enet_addr + MCF_FEC_MSCR);
> > +}
> > +
> > +static int mtip_setup_mac(struct net_device *dev)
> > +{
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + struct switch_enet_private *fep = priv->fep;
> > + unsigned char *iap, tmpaddr[ETH_ALEN];
>
> Maybe mac_addr instead of tmpaddr.

Ok.

>
> > +
> > + /* Use MAC address from DTS */
> > + iap = &fep->mac[priv->portnum - 1][0];
> > +
> > + /* Use MAC address set by bootloader */
> > + if (!is_valid_ether_addr(iap)) {
> > + *((unsigned long *)&tmpaddr[0]) =
> > + be32_to_cpu(readl(fep->enet_addr +
> > MCF_FEC_PALR));
> > + *((unsigned short *)&tmpaddr[4]) =
> > + be16_to_cpu(readl(fep->enet_addr +
> > + MCF_FEC_PAUR) >> 16);
>
> * Above, and elsewhere in this patch unsigned long seems to be
> used for 32 bit values. But unsigned long can be 64 bits wide.
>

As fair as I know - from the outset - this driver had some implicit
assumption to be used on 32 bit SoCs (imx28, vf610).

As a result the unsigned long would be 32 bits.

On the other hand - the documentation clearly says that registers in
this IP block implementation are 32 bit wide.

> I would suggest using u32, u16, and friends throughout this
> patch where an integer has a specific number of bits.
>

I think that values, which directly readl()/writel() values from the
registers shall be u32. However, more generic code could use int/
unsigned int though.

> * readl returns a 32-bit value in host byte order.
> But the above assumes it returns a big endian value.
>
> This does not seem correct.
>

Please consult similar implementation from fec_main.c:
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/freescale/fec_main.c#L2044

I would use the same approach as in the above link.

> * The point immediately above aside, the assignment of
> host byte order values to the byte-array tmpaddr
> seems to assume an endianness (little endian?).
>
> It should work on either endian.
>

I guess that implementation from above link is the correct one.

> > + iap = &tmpaddr[0];
> > + }
> > +
> > + /* Use random MAC address */
> > + if (!is_valid_ether_addr(iap)) {
> > + eth_hw_addr_random(dev);
> > + dev_info(&fep->pdev->dev, "Using random MAC
> > address: %pM\n",
> > + dev->dev_addr);
> > + iap = (unsigned char *)dev->dev_addr;
> > + }
> > +
> > + /* Adjust MAC if using macaddr (and increment if needed) */
> > + eth_hw_addr_gen(dev, iap, priv->portnum - 1);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * crc8_calc - calculate CRC for MAC storage
> > + *
> > + * @pmacaddress: A 6-byte array with the MAC address. The first
> > byte is
> > + * the first byte transmitted.
> > + *
> > + * Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> > + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> > + * (getting the remainder).
> > + *
> > + * Examples (hexadecimal values):
> > + * 10-11-12-13-14-15 => CRC=0xc2
> > + * 10-11-cc-dd-ee-00 => CRC=0xe6
> > + *
> > + * Return: The 8-bit CRC in bits 7:0
> > + */
> > +static int crc8_calc(unsigned char *pmacaddress)
>
> Can lib/crc8.c:crc8() be used here?

This seems a bit problematic, as the above code is taken from the
vendor driver.

The documentation states - that the hash uses CRC-8 with following
polynomial:

x^8 + x^2 + x + 1 (0x07), which shall correspond to available in Linux:

static unsigned char mac1[] = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15};
crc8_populate_msb(mtipl2sw_crc8_table, 0x07);
crc8(mtipl2sw_crc8_table, mac1, sizeof(mac1), CRC8_INIT_VALUE);

However, those results don't match (either with 0xFF and 0x00 as the
initial value).

I've also searched the Internet to compare different CRC
implementations used in field:
https://crccalc.com/?crc=0x10 0x11 0x12 0x13 0x14
0x15&method=CRC-8&datatype=hex&outtype=hex

they also doesn't match results from this code.

Hence, the question - which implementation is right?

Vendor developer could have known a bit more than we do, so I would
prefer to keep this code as is - especially that switch internally may
use this crc8 calculation to find proper "slot/index" in the atable.

>
> > +{
> > + int byt; /* byte index */
> > + int bit; /* bit index */
> > + int crc = 0x12;
> > + int inval;
> > +
> > + for (byt = 0; byt < 6; byt++) {
> > + inval = (((int)pmacaddress[byt]) & 0xff);
> > + /* shift bit 0 to bit 8 so all our bits
> > + * travel through bit 8
> > + * (simplifies below calc)
> > + */
> > + inval <<= 8;
> > +
> > + for (bit = 0; bit < 8; bit++) {
> > + /* next input bit comes into d7 after
> > shift */
> > + crc |= inval & 0x100;
> > + if (crc & 0x01)
> > + /* before shift */
> > + crc ^= 0x1c0;
> > +
> > + crc >>= 1;
> > + inval >>= 1;
> > + }
> > + }
> > + /* upper bits are clean as we shifted in zeroes! */
> > + return crc;
> > +}
> > +
> > +static void mtip_read_atable(struct switch_enet_private *fep, int
> > index,
> > + unsigned long *read_lo, unsigned long
> > *read_hi) +{
> > + unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > + *read_lo = readl((const void *)atable_base + (index << 3));
> > + *read_hi = readl((const void *)atable_base + (index << 3)
> > + 4); +}
>
> It is unclear why hwentry, which is a pointer, is being cast to an
> integer and then back to a pointer. I see pointer arithmetic, but
> that can operate on pointers just as well as integers, without making
> assumptions about how wide pointers are with respect to longs.
>
> And in any case, can't the types be used to directly access the
> offsets needed like this?
>
> atable = fep->hwentry.mtip_table64b_entry;
>
> *read_lo = readl(&atable[index].lo);
> *read_hi = readl(&atable[index].hi);
>

The code as is seems to be OK.

The (atable) memory structure is as follows:

1. You can store 2048 MAC addresses (2x32 bit each).

2. Memory from point 1 is addressed as follows:
2.1 -> from MAC address the CRC8 is calculated (0x00 - 0xFF).
This is the 'index' in the original code.
2.2 -> as it may happen that for two different MAC address the
same CRC8 is calculated (i.e. 'index' is the same), each
'index' can store 8 entries for MAC addresses (and it is
searched in a linear way if needed).

IMHO, the index above shall be multiplied by 8.

> Also, and perhaps more importantly, readl expects to be passed
> a pointer to __iomem. But the appropriate annotations seem
> to be missing (forcing them with a cast is not advisable here IMHO).
>

I think that the code below:
unsigned long atable_base = (unsigned long)fep->hwentry;

could be replaced with
void __iomem *atable_base = fep->hwentry;

and the (index << 3) with (index * ATABLE_ENTRY_PER_SLOT)

> Please do run sparse over your patches to iron out __iomem
> (and endian) issues.
>

Ok, I will run the make C=1 when compiling.

> > +
> > +static void mtip_write_atable(struct switch_enet_private *fep, int
> > index,
> > + unsigned long write_lo, unsigned
> > long write_hi) +{
> > + unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > + writel(write_lo, (void *)atable_base + (index << 3));
> > + writel(write_hi, (void *)atable_base + (index << 3) + 4);
>
> Likewise here.

Please see the above comment.

>
> > +}
>
> ...
>
> > +/* Clear complete MAC Look Up Table */
> > +void mtip_clear_atable(struct switch_enet_private *fep)
> > +{
> > + int index;
> > +
> > + for (index = 0; index < 2048; index++)
> > + mtip_write_atable(fep, index, 0, 0);
> > +}
> > +
> > +/**
> > + * mtip_update_atable_static - Update switch static address table
> > + *
> > + * @mac_addr: Pointer to the array containing MAC address to
> > + * be put as static entry
> > + * @port: Port bitmask numbers to be added in static entry,
> > + * valid values are 1-7
> > + * @priority: The priority for the static entry in table
>
> @fep should also be documented here.
>

OK.

> Flagged by ./scripts/kernel-doc -none
> and W=1 builds.
>
> > + *
> > + * Updates MAC address lookup table with a static entry.
> > + *
> > + * Searches if the MAC address is already there in the block and
> > replaces
> > + * the older entry with the new one. If MAC address is not there
> > then puts
> > + * a new entry in the first empty slot available in the block.
> > + *
> > + * Return: 0 for a successful update else -ENOSPC when no slot
> > available
> > + */
> > +static int mtip_update_atable_static(unsigned char *mac_addr,
> > unsigned int port,
> > + unsigned int priority,
> > + struct switch_enet_private
> > *fep)
>
> ...
>
> > +/* During a receive, the cur_rx points to the current incoming
> > buffer.
> > + * When we update through the ring, if the next incoming buffer has
> > + * not been given to the system, we just set the empty indicator,
> > + * effectively tossing the packet.
> > + */
> > +static int mtip_switch_rx(struct net_device *dev, int budget, int
> > *port) +{
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + struct switch_enet_private *fep = priv->fep;
> > + struct switch_t *fecp = fep->hwp;
> > + unsigned short status, pkt_len;
> > + struct net_device *pndev;
> > + u8 *data, rx_port = 0xFF;
> > + struct ethhdr *eth_hdr;
> > + int pkt_received = 0;
> > + struct sk_buff *skb;
> > + unsigned long flags;
> > + struct cbd_t *bdp;
> > +
> > + spin_lock_irqsave(&fep->hw_lock, flags);
> > +
> > + /* First, grab all of the stats for the incoming packet.
> > + * These get messed up if we get called due to a busy
> > condition.
> > + */
> > + bdp = fep->cur_rx;
> > +
> > + while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {
>
> ...
>
> > + } /* while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY))
> > */ +
> > + writel(bdp, &fep->cur_rx);
>
> I'm confused buy this.
>
> At the top of this function, bdp is assigned using:
>
> bdp = fep->cur_rx;
>
> But here writel() is used to assign bdp to &fep->cur_rx.
> Which assumes that bdp is a 32-bit little endian value.
> But it is a pointer in host byte order which may be wide than 32-bits.
>
> On x86_64 int is 32-bits while pointers are 64 bits.

I will cross compile with make W=1 and C=1 to find all problematic
places.

> W=1 builds with gcc 14.2.0 flag this problem like this:
>
>
> .../mtipl2sw.c:1108:9: error: incompatible pointer to integer
> conversion passing 'struct cbd_t *' to parameter of type 'unsigned
> int' [-Wint-conversion] 1108 | writel(bdp, &fep->cur_rx); |
> ^~~
>
>
> This also assumes that &fep->cur_rx is a pointer to __iomem,
> but that does not seem to be the case.

The writel(bdp, &fep->cur_rx); shall be just replaced with fep->cur_rx
= bdp.

Thanks for spotting.

>
> > + spin_unlock_irqrestore(&fep->hw_lock, flags);
> > +
> > + return pkt_received;
> > +}
>
> ...
>
> > +static int __init mtip_switch_dma_init(struct switch_enet_private
> > *fep) +{
> > + struct cbd_t *bdp, *cbd_base;
> > + int ret, i;
> > +
> > + /* Check mask of the streaming and coherent API */
> > + ret = dma_set_mask_and_coherent(&fep->pdev->dev,
> > DMA_BIT_MASK(32));
> > + if (ret < 0) {
> > + dev_err(&fep->pdev->dev, "No suitable DMA
> > available\n");
> > + return ret;
> > + }
> > +
> > + /* Allocate memory for buffer descriptors */
> > + cbd_base = dma_alloc_coherent(&fep->pdev->dev, PAGE_SIZE,
> > &fep->bd_dma,
> > + GFP_KERNEL);
> > + if (!cbd_base)
> > + return -ENOMEM;
> > +
> > + /* Set receive and transmit descriptor base */
> > + fep->rx_bd_base = cbd_base;
> > + fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> > +
> > + /* Initialize the receive buffer descriptors */
> > + bdp = fep->rx_bd_base;
> > + for (i = 0; i < RX_RING_SIZE; i++) {
> > + bdp->cbd_sc = 0;
> > + bdp++;
> > + }
> > +
> > + /* Set the last buffer to wrap */
> > + bdp--;
> > + bdp->cbd_sc |= BD_SC_WRAP;
> > +
> > + /* ...and the same for transmmit */
>
> nit: transmit

Ok.

>
> > + bdp = fep->tx_bd_base;
> > + for (i = 0; i < TX_RING_SIZE; i++) {
> > + /* Initialize the BD for every fragment in the
> > page */
> > + bdp->cbd_sc = 0;
> > + bdp->cbd_bufaddr = 0;
> > + bdp++;
> > + }
> > +
> > + /* Set the last buffer to wrap */
> > + bdp--;
> > + bdp->cbd_sc |= BD_SC_WRAP;
> > +
> > + return 0;
> > +}
> > +
> > +static void mtip_ndev_cleanup(struct switch_enet_private *fep)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > + if (fep->ndev[i]) {
> > + unregister_netdev(fep->ndev[i]);
> > + free_netdev(fep->ndev[i]);
> > + }
> > + }
> > +}
>
> ...
>
> > +static const struct of_device_id mtipl2_of_match[] = {
> > + { .compatible = "nxp,imx28-mtip-switch",
> > + .data = &mtip_imx28_l2switch_info},
> > + { /* sentinel */ }
> > +}
>
> There should be a trailing ';' on the line above.
>

+1

> ...
>
> > +struct addr_table64b_entry {
>
> One space after struct is enough.
>
> > + unsigned int lo; /* lower 32 bits */
> > + unsigned int hi; /* upper 32 bits */
> > +};
> > +
> > +struct mtip_addr_table_t {
>
> I think you can drop the '_t' from the name of this struct.
> We know it is a type :)

Ok.

>
> > + struct addr_table64b_entry mtip_table64b_entry[2048];
>
> One space is enough after addr_table64b_entry.
> And in general, unless the aim is to align field names
> (not here because there is only one field :)
>
> > +};
> > +
> > +#define MCF_ESW_LOOKUP_MEM_OFFSET 0x4000
> > +#define MCF_ESW_ENET_PORT_OFFSET 0x4000
> > +#define ENET_SWI_PHYS_ADDR_OFFSET 0x8000
>
> Ditto.
>

+1

> ...
>
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
>
> > new file mode 100644
> > index 000000000000..0b76a60858a5
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
> > @@ -0,0 +1,122 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * L2 switch Controller driver for MTIP block - bridge network
> > interface
> > + *
> > + * Copyright (C) 2025 DENX Software Engineering GmbH
> > + * Lukasz Majewski <lukma@denx.de>
> > + */
> > +
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "mtipl2sw.h"
>
> Blank line here please.
>

Ok.

> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > + struct net_device *br_ndev,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct mtip_ndev_priv *priv = netdev_priv(ndev),
> > *other_priv;
> > + struct switch_enet_private *fep = priv->fep;
> > + struct net_device *other_ndev;
> > +
> > + /* Check if one port of MTIP switch is already bridged */
> > + if (fep->br_members && !fep->br_offload) {
> > + /* Get the second bridge ndev */
> > + other_ndev = fep->ndev[fep->br_members - 1];
> > + other_priv = netdev_priv(other_ndev);
> > + if (other_priv->master_dev != br_ndev) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "L2 offloading only
> > possible for the same bridge!");
> > + return notifier_from_errno(-EOPNOTSUPP);
> > + }
> > +
> > + fep->br_offload = 1;
> > + mtip_switch_dis_port_separation(fep);
> > + mtip_clear_atable(fep);
> > + }
> > +
> > + if (!priv->master_dev)
> > + priv->master_dev = br_ndev;
> > +
> > + fep->br_members |= BIT(priv->portnum - 1);
> > +
> > + dev_dbg(&ndev->dev,
> > + "%s: ndev: %s br: %s fep: 0x%x members: 0x%x
> > offload: %d\n",
> > + __func__, ndev->name, br_ndev->name, (unsigned
> > int)fep,
>
> Perhaps it would be best to use %p as the format specifier for fep
> and not cast it.
>
> On x86_64 int is 32-bits while pointers are 64 bits.
> W=1 builds with gcc 14.2.0 flag this problem like this:
>
> .../mtipl2sw_br.c:45:55: warning: cast from pointer to integer of
> different size [-Wpointer-to-int-cast] 45 | __func__,
> ndev->name, br_ndev->name, (unsigned int)fep, |
> ^

Ok.

>
> > + fep->br_members, fep->br_offload);
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
> > +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> > +{
> > + struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > + struct switch_enet_private *fep = priv->fep;
> > +
> > + dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n",
> > __func__,
> > + ndev->name, fep->br_members);
> > +
> > + fep->br_members &= ~BIT(priv->portnum - 1);
> > + priv->master_dev = NULL;
> > +
> > + if (!fep->br_members) {
> > + fep->br_offload = 0;
> > + mtip_switch_en_port_separation(fep);
> > + mtip_clear_atable(fep);
> > + }
> > +}
>
> ...
>

Best regards,

Lukasz Majewski

--

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]