Hi Andrew,

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+static const struct net_device_ops mtip_netdev_ops;  

more forward declarations.

Ok, fixed.

quoted

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

quoted

+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

quoted

+			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()

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+	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

quoted

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

quoted

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

-ENOMEM?

Ok.

quoted

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

quoted

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

return ret?

Ok.

quoted

+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@denx.de