Re: [RFC PATCH 2/9] net: dsa: Add DSA driver for Hirschmann Hellcreek switches

Hi Andrew,

first off thank you for the review.

On Thu Jun 18 2020, Andrew Lunn wrote:

quoted

+static void __hellcreek_select_port(struct hellcreek *hellcreek, int port)

Hi Kurt

In general, please can you drop all these __ prefixes. They are useful
when you have for example hellcreek_select_port() takes a lock and
then calls _hellcreek_select_port(), but here, there is no need for
them.

OK, sure.

quoted

+static int hellcreek_detect(struct hellcreek *hellcreek)
+{
+	u8 tgd_maj, tgd_min;
+	u16 id, rel_low, rel_high, date_low, date_high, tgd_ver;
+	u32 rel, date;

Reverse Christmas tree please. Here and everywhere else.

OK.

quoted

+
+	id	  = hellcreek_read(hellcreek, HR_MODID_C);
+	rel_low	  = hellcreek_read(hellcreek, HR_REL_L_C);
+	rel_high  = hellcreek_read(hellcreek, HR_REL_H_C);
+	date_low  = hellcreek_read(hellcreek, HR_BLD_L_C);
+	date_high = hellcreek_read(hellcreek, HR_BLD_H_C);
+	tgd_ver   = hellcreek_read(hellcreek, TR_TGDVER);
+
+	if (id != HELLCREEK_MODULE_ID)
+		return -ENODEV;
+
+	rel	= rel_low | (rel_high << 16);
+	date	= date_low | (date_high << 16);
+	tgd_maj = (tgd_ver & TR_TGDVER_REV_MAJ_MASK) >> TR_TGDVER_REV_MAJ_SHIFT;
+	tgd_min = (tgd_ver & TR_TGDVER_REV_MIN_MASK) >> TR_TGDVER_REV_MIN_SHIFT;
+
+	dev_info(hellcreek->dev, "Module ID=%02x Release=%04x Date=%04x TGD Version=%02x.%02x\n",
+		 id, rel, date, tgd_maj, tgd_min);
+
+	return 0;
+}

quoted

+static int hellcreek_port_enable(struct dsa_switch *ds, int port,
+				 struct phy_device *phy)
+{
+	struct hellcreek *hellcreek = ds->priv;
+	struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
+	unsigned long flags;
+	u16 val;
+
+	if (port >= NUM_PORTS)
+		return -EINVAL;
+
+	dev_info(hellcreek->dev, "Enable port %d\n", port);

dev_dbg().

These are debug messages. I'll change that in the complete code.

+

quoted

+	spin_lock_irqsave(&hellcreek->reg_lock, flags);

I've not seen any interrupt handling code in the driver, and there is
no mention of an interrupt in the DT binding. Do you really need
_irqsave spin locks?

Yes, I do. The TAPRIO offloading patch later adds hrtimers and that's
why it's needed. However, in this particular patch it is not required,
but I didn't want to change all spin lock calls later.

quoted

+
+	__hellcreek_select_port(hellcreek, port);
+	val = hellcreek_port->ptcfg;
+	val |= HR_PTCFG_ADMIN_EN;
+	hellcreek_write(hellcreek, val, HR_PTCFG);
+	hellcreek_port->ptcfg = val;
+
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+
+	return 0;
+}
+
+static void hellcreek_port_disable(struct dsa_switch *ds, int port)
+{
+	struct hellcreek *hellcreek = ds->priv;
+	struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
+	unsigned long flags;
+	u16 val;
+
+	if (port >= NUM_PORTS)
+		return;

I don't think this test is actually needed, here or in any of the
other callbacks. If it does happen, it means we have a core bug we
should fix.

Agreed.

quoted

+
+	dev_info(hellcreek->dev, "Disable port %d\n", port);

dev_dbg()

quoted

+	spin_lock_irqsave(&hellcreek->reg_lock, flags);
+
+	__hellcreek_select_port(hellcreek, port);
+	val = hellcreek_port->ptcfg;
+	val &= ~HR_PTCFG_ADMIN_EN;
+	hellcreek_write(hellcreek, val, HR_PTCFG);
+	hellcreek_port->ptcfg = val;
+
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+}
+

quoted

+static void hellcreek_get_ethtool_stats(struct dsa_switch *ds, int port,
+					uint64_t *data)
+{
+	struct hellcreek *hellcreek = ds->priv;
+	struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
+	unsigned long flags;
+	int i;
+
+	if (port >= NUM_PORTS)
+		return;
+
+	spin_lock_irqsave(&hellcreek->reg_lock, flags);
+	for (i = 0; i < ARRAY_SIZE(hellcreek_counter); ++i) {
+		const struct hellcreek_counter *counter = &hellcreek_counter[i];
+		u8 offset = counter->offset + port * 64;
+		u16 high, low;
+		u64 value = 0;
+
+		__hellcreek_select_counter(hellcreek, offset);
+
+		high  = hellcreek_read(hellcreek, HR_CRDH);
+		low   = hellcreek_read(hellcreek, HR_CRDL);
+		value = (high << 16) | low;

Is there some sort of snapshot happening here? Or do you need to read
high again, to make sure it has not incremented when low was being
read?

Yes, there is. When the counter is selected via
__hellcreek_select_counter() both registers are locked. That's why
there's no need to read high again. I'll add a comment for clarity.

quoted

+
+		hellcreek_port->counter_values[i] += value;
+		data[i] = hellcreek_port->counter_values[i];
+	}
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+}
+
+static int hellcreek_vlan_prepare(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_vlan *vlan)
+{
+	struct hellcreek *hellcreek = ds->priv;
+
+	/* Nothing todo */
+	dev_info(hellcreek->dev, "VLAN prepare for port %d\n", port);

dev_dbg()

quoted

+
+	return 0;
+}
+

quoted

+static void hellcreek_vlan_add(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan)
+{
+	struct hellcreek *hellcreek = ds->priv;
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	unsigned long flags;
+	u16 vid;
+
+	if (port >= NUM_PORTS || vlan->vid_end >= 4096)

Again, if vlan->vid_end >= 4096 is true, there is a core bug which
needs fixing.

quoted

+		return;
+
+	dev_info(hellcreek->dev, "Add VLANs (%d -- %d) on port %d, %s, %s\n",
+		 vlan->vid_begin, vlan->vid_end, port,
+		 untagged ? "untagged" : "tagged",
+		 pvid ? "PVID" : "no PVID");

Please go thought the driver and consider all you dev_info()
statements. Should they be dev_dbg()?

I'll do.

Thanks,
Kurt