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

On Thu, Aug 20, 2020 at 10:11:12AM +0200, Kurt Kanzenbach wrote:

Hi Kurt

The DSA entries are sorted into alphabetic order based on what you see
in make menuconfig. As such, "Hirschmann Hellcreek TSN Switch support"
fits in between "DSA mock-up Ethernet switch chip support" and "Lantiq / Intel GSWIP"

This file is also sorted. 

+static int hellcreek_detect(struct hellcreek *hellcreek)
+{
+	u16 id, rel_low, rel_high, date_low, date_high, tgd_ver;
+	u8 tgd_maj, tgd_min;
+	u32 rel, date;
+
+	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;

Are there other Hellcreek devices? I'm just wondering if we should
have a specific compatible for 0x4c30 as well as the more generic 
"hirschmann,hellcreek".

+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;
+	unsigned long flags;
+	int i;
+
+	hellcreek_port = &hellcreek->ports[port];
+
+	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);
+
+		/* The registers are locked internally by selecting the
+		 * counter. So low and high can be read without reading high
+		 * again.
+		 */

Is there any locking/snapshot of all the counters at once? Most
devices have support for that, so you can compare counters against
each other.

+		high  = hellcreek_read(hellcreek, HR_CRDH);
+		low   = hellcreek_read(hellcreek, HR_CRDL);
+		value = (high << 16) | low;
+
+		hellcreek_port->counter_values[i] += value;
+		data[i] = hellcreek_port->counter_values[i];
+	}
+	spin_unlock_irqrestore(&hellcreek->reg_lock, flags);
+}
+

  Andrew