[PATCH v2 4/4] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

Hi Andrew,

> > +static bool bridge_offload;
> > +module_param(bridge_offload, bool, 0644); /* Allow setting by root
> > on boot */ +MODULE_PARM_DESC(bridge_offload, "L2 switch offload
> > mode enable:1, disable:0");  
> 
> Please drop. module parameters are not liked.
> 

Ok.

> In Linux, ports of a switch always starting in isolated mode, and
> userspace needs to add them to the same bridge.

Ok.

> 
> > +
> > +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb,
> > +				   struct net_device *dev);
> > +static void mtip_switch_tx(struct net_device *dev);
> > +static int mtip_switch_rx(struct net_device *dev, int budget, int
> > *port); +static void mtip_set_multicast_list(struct net_device
> > *dev); +static void mtip_switch_restart(struct net_device *dev, int
> > duplex0,
> > +				int duplex1);  
> 
> Forwards references are not like. Put the functions in the correct
> order so they are not needed.

Ok.

> 
> > +/* 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):<br>
> > + *   10-11-12-13-14-15  => CRC=0xc2
> > + *   10-11-cc-dd-ee-00  => CRC=0xe6
> > + *
> > + *   param: pmacaddress
> > + *          A 6-byte array with the MAC address.
> > + *          The first byte is the first byte transmitted
> > + *   return The 8-bit CRC in bits 7:0
> > + */
> > +static int crc8_calc(unsigned char *pmacaddress)
> > +{
> > +	/* byte index */
> > +	int byt;
> > +	/* bit index */
> > +	int bit;
> > +	int inval;
> > +	int crc;  
> 
> Reverse Christmas tree. Please look through the whole driver and fix
> it up.

Ok.

> 
> > +/* 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 new one. If MAC address is not there then
> > puts a
> > + * new entry in the first empty slot available in the block
> > + *
> > + * 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
> > + *
> > + * return 0 for a successful update else -1  when no slot
> > available  
> 
> It would be nice to turn this into proper kerneldoc. It is not too far
> away at the moment.
> 
> Also, return a proper error code not -1. ENOSPC?

Ok.

> 
> > +static int mtip_update_atable_dynamic1(unsigned long write_lo,
> > +				       unsigned long write_hi, int
> > block_index,
> > +				       unsigned int port,
> > +				       unsigned int curr_time,
> > +				       struct switch_enet_private
> > *fep)  
> 
> It would be good to document the return value, because it is not the
> usual 0 success or negative error code.

Ok.

> 
> > +static const struct net_device_ops mtip_netdev_ops;  
> 
> more forward declarations.

Ok, fixed.

> 
> > +struct switch_enet_private *mtip_netdev_get_priv(const struct
> > net_device *ndev) +{
> > +	if (ndev->netdev_ops == &mtip_netdev_ops)
> > +		return netdev_priv(ndev);
> > +
> > +	return NULL;
> > +}  
> 
> I _think_ the return value is not actually used. So maybe 0 or
> -ENODEV?

It is used at:
drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c in
mtip_port_dev_check()

to assess if network interfaces eligible for bridging are using the
same (i.e. mtipl2sw) driver.

Only when they match - bridging is performed.

> 
> > +static int esw_mac_addr_static(struct switch_enet_private *fep)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > +		if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) {
> >  
> 
> Is that possible? This is the interfaces own MAC address? If it is not
> valid, the probe should of failed.

I've double checked it - this cannot happen (i.e. that
is_valid_ether_addr(fep->ndev[i]->dev_addr) is NOT valid at this point
of execution.

I will remove this check

> 
> > +			mtip_update_atable_static((unsigned char *)
> > +
> > fep->ndev[i]->dev_addr,
> > +						  7, 7, fep);
> > +		} else {
> > +			dev_err(&fep->pdev->dev,
> > +				"Can not add mac address %pM to
> > switch!\n",
> > +				fep->ndev[i]->dev_addr);
> > +			return -EFAULT;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtip_print_link_status(struct phy_device *phydev)
> > +{
> > +	if (phydev->link)
> > +		netdev_info(phydev->attached_dev,
> > +			    "Link is Up - %s/%s - flow control
> > %s\n",
> > +			    phy_speed_to_str(phydev->speed),
> > +			    phy_duplex_to_str(phydev->duplex),
> > +			    phydev->pause ? "rx/tx" : "off");
> > +	else
> > +		netdev_info(phydev->attached_dev, "Link is
> > Down\n"); +}  
> 
> phy_print_status()

Yes, I will remove mtip_print_link_status() and replace it with
phy_print_status()

> 
> > +static void mtip_adjust_link(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct phy_device *phy_dev;
> > +	int status_change = 0, idx;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fep->hw_lock, flags);
> > +
> > +	idx = priv->portnum - 1;
> > +	phy_dev = fep->phy_dev[idx];
> > +
> > +	/* Prevent a state halted on mii error */
> > +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> > +		phy_dev->state = PHY_UP;
> > +		goto spin_unlock;
> > +	}  
> 
> A MAC driver should not be playing around with the internal state of
> phylib.

Ok, I've replaced it with PHY API calls (phy_start() and
phy_is_started()).

> 
> > +static int mtip_mii_probe(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	int port_idx = priv->portnum - 1;
> > +	struct phy_device *phy_dev = NULL;
> > +
> > +	if (fep->phy_np[port_idx]) {
> > +		phy_dev = of_phy_connect(dev,
> > fep->phy_np[port_idx],
> > +					 &mtip_adjust_link, 0,
> > +
> > fep->phy_interface[port_idx]);
> > +		if (!phy_dev) {
> > +			netdev_err(dev, "Unable to connect to
> > phy\n");
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	phy_set_max_speed(phy_dev, 100);
> > +	fep->phy_dev[port_idx] = phy_dev;
> > +	fep->link[port_idx] = 0;
> > +	fep->full_duplex[port_idx] = 0;
> > +
> > +	dev_info(&dev->dev,
> > +		 "MTIP PHY driver [%s] (mii_bus:phy_addr=%s,
> > irq=%d)\n",
> > +		 fep->phy_dev[port_idx]->drv->name,
> > +		 phydev_name(fep->phy_dev[port_idx]),
> > +		 fep->phy_dev[port_idx]->irq);  
> 
> phylib already prints something like that.

Yes, the 
"net lan0: lan0: MTIP eth L2 switch <mac addr>" 

is printed.

For the original call - I've used dev_dbg().

> 
> > +static int mtip_mdiobus_reset(struct mii_bus *bus)
> > +{
> > +	if (!bus || !bus->reset_gpiod) {
> > +		dev_err(&bus->dev, "Reset GPIO pin not
> > provided!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> > +
> > +	/* Extra time to allow:
> > +	 * 1. GPIO RESET pin go high to prevent situation where
> > its value is
> > +	 *    "LOW" as it is NOT configured.
> > +	 * 2. The ENET CLK to stabilize before GPIO RESET is
> > asserted
> > +	 */
> > +	usleep_range(200, 300);
> > +
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 0);
> > +	usleep_range(bus->reset_delay_us, bus->reset_delay_us +
> > 1000);
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> > +
> > +	if (bus->reset_post_delay_us > 0)
> > +		usleep_range(bus->reset_post_delay_us,
> > +			     bus->reset_post_delay_us + 1000);
> > +
> > +	return 0;
> > +}  
> 
> What is wrong with the core code __mdiobus_register() which does the
> bus reset.

The main problem is that the "default" mdio reset is just asserting and
deasserting the reset line.

It doesn't take into account the state of the reset gpio before
assertion (if it was high for enough time) and if clocks already
were stabilized.

> 
> > +static void mtip_get_drvinfo(struct net_device *dev,
> > +			     struct ethtool_drvinfo *info)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	strscpy(info->driver, fep->pdev->dev.driver->name,
> > +		sizeof(info->driver));
> > +	strscpy(info->version, VERSION, sizeof(info->version));  
> 
> Leave this empty, so you get the git hash of the kernel.

Ok.

> 
> > +static void mtip_ndev_setup(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +
> > +	ether_setup(dev);  
> 
> That is pretty unusual

Yes - how it has been used is described below.

> 
> > +	dev->ethtool_ops = &mtip_ethtool_ops;
> > +	dev->netdev_ops = &mtip_netdev_ops;
> > +
> > +	memset(priv, 0, sizeof(struct mtip_ndev_priv));  
> 
> priv should already be zero....

Ok, I will remove

> 
> > +static int mtip_ndev_init(struct switch_enet_private *fep)
> > +{
> > +	struct mtip_ndev_priv *priv;
> > +	int i, ret = 0;
> > +
> > +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > +		fep->ndev[i] = alloc_netdev(sizeof(struct
> > mtip_ndev_priv),
> > +					    fep->ndev_name[i],
> > NET_NAME_USER,
> > +					    mtip_ndev_setup);  
> 
> This explains the ether_setup(). It would be more normal to pass
> ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here.
> 

Yes. I will do that.

> > +		if (!fep->ndev[i]) {
> > +			ret = -1;  
> 
> -ENOMEM?

Ok.

> 
> > +			break;
> > +		}
> > +
> > +		priv = netdev_priv(fep->ndev[i]);
> > +		priv->fep = fep;
> > +		priv->portnum = i + 1;
> > +		fep->ndev[i]->irq = fep->irq;
> > +
> > +		ret = mtip_setup_mac(fep->ndev[i]);
> > +		if (ret) {
> > +			dev_err(&fep->ndev[i]->dev,
> > +				"%s: ndev %s MAC setup err: %d\n",
> > +				__func__, fep->ndev[i]->name, ret);
> > +			break;
> > +		}
> > +
> > +		ret = register_netdev(fep->ndev[i]);
> > +		if (ret) {
> > +			dev_err(&fep->ndev[i]->dev,
> > +				"%s: ndev %s register err: %d\n",
> > __func__,
> > +				fep->ndev[i]->name, ret);
> > +			break;
> > +		}
> > +		dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2
> > switch %pM\n",
> > +			 fep->ndev[i]->name,
> > fep->ndev[i]->dev_addr);  
> 
> I would drop this. A driver is normally silent unless things go wrong.

I've replaced dev_info() with dev_dbg() as this information may be
relevant during development.

> 
> > +	}
> > +
> > +	if (ret)
> > +		mtip_ndev_cleanup(fep);
> > +
> > +	return 0;  
> 
> return ret?

Ok.

> 
> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep); +
> > +	/* Check if MTIP switch is already enabled */
> > +	if (!fep->br_offload) {
> > +		if (!priv->master_dev)
> > +			priv->master_dev = br_ndev;  
> 
> It needs to be a little bit more complex than that, because the two
> ports could be assigned to two different bridges. You should only
> enable hardware bridging if they are a member of the same bridge.

This has been explained earlier.
The mtip_port_dev_check() checks in mtip_netdevice_event() if we use
ports from the mtipl2sw driver.

Only for them we start the bridge.

> 
> 	Andrew


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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20250330/2ee3d86b/attachment.sig>