Re: [PATCH v3 net-next 17/24] net: dsa: sja1105: Add support for ethtool port counters

[Jiri Pirko]

Sat, Apr 13, 2019 at 03:28:15AM CEST, olteanv@xxxxxxxxx wrote:
>Signed-off-by: Vladimir Oltean <olteanv@xxxxxxxxx>
>Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>---
>Changes in v3:
>None.
>
>Changes in v2:
>None functional. Moved the IS_ET() and IS_PQRS() device identification
>macros here since they are not used in earlier patches.
>
> drivers/net/dsa/sja1105/Makefile              |   1 +
> drivers/net/dsa/sja1105/sja1105.h             |   7 +-
> drivers/net/dsa/sja1105/sja1105_ethtool.c     | 414 ++++++++++++++++++
> drivers/net/dsa/sja1105/sja1105_main.c        |   3 +
> .../net/dsa/sja1105/sja1105_static_config.h   |  21 +
> 5 files changed, 445 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/dsa/sja1105/sja1105_ethtool.c
>
>diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
>index ed00840802f4..bb4404c79eb2 100644
>--- a/drivers/net/dsa/sja1105/Makefile
>+++ b/drivers/net/dsa/sja1105/Makefile
>@@ -3,6 +3,7 @@ obj-$(CONFIG_NET_DSA_SJA1105) += sja1105.o
> sja1105-objs := \
>     sja1105_spi.o \
>     sja1105_main.o \
>+    sja1105_ethtool.o \
>     sja1105_clocking.o \
>     sja1105_static_config.o \
>     sja1105_dynamic_config.o \
>diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
>index 4c9df44a4478..80b20bdd8f9c 100644
>--- a/drivers/net/dsa/sja1105/sja1105.h
>+++ b/drivers/net/dsa/sja1105/sja1105.h
>@@ -120,8 +120,13 @@ typedef enum {
> int sja1105_clocking_setup_port(struct sja1105_private *priv, int port);
> int sja1105_clocking_setup(struct sja1105_private *priv);
> 
>-/* From sja1105_dynamic_config.c */
>+/* From sja1105_ethtool.c */
>+void sja1105_get_ethtool_stats(struct dsa_switch *ds, int port, u64 *data);
>+void sja1105_get_strings(struct dsa_switch *ds, int port,
>+			 u32 stringset, u8 *data);
>+int sja1105_get_sset_count(struct dsa_switch *ds, int port, int sset);
> 
>+/* From sja1105_dynamic_config.c */
> int sja1105_dynamic_config_read(struct sja1105_private *priv,
> 				enum sja1105_blk_idx blk_idx,
> 				int index, void *entry);
>diff --git a/drivers/net/dsa/sja1105/sja1105_ethtool.c b/drivers/net/dsa/sja1105/sja1105_ethtool.c
>new file mode 100644
>index 000000000000..c082599702bd
>--- /dev/null
>+++ b/drivers/net/dsa/sja1105/sja1105_ethtool.c
>@@ -0,0 +1,414 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/* Copyright (c) 2018-2019, Vladimir Oltean <olteanv@xxxxxxxxx>
>+ */
>+#include "sja1105.h"
>+
>+#define SIZE_MAC_AREA		(0x02 * 4)
>+#define SIZE_HL1_AREA		(0x10 * 4)
>+#define SIZE_HL2_AREA		(0x4 * 4)
>+#define SIZE_QLEVEL_AREA	(0x8 * 4) /* 0x4 to 0xB */

Please use prefixes for defines like this. For example "SIZE_MAC_AREA"
sounds way too generic.

[...]


>+static void
>+sja1105_port_status_hl1_unpack(void *buf,
>+			       struct sja1105_port_status_hl1 *status)
>+{
>+	/* Make pointer arithmetic work on 4 bytes */
>+	u32 *p = (u32 *)buf;

You don't need to cast void *. Please avoid it in the whole patchset.

[...]


>+	if (!IS_PQRS(priv->info->device_id))
>+		return;
>+
>+	memset(data + k, 0, ARRAY_SIZE(sja1105pqrs_extra_port_stats) *
>+			sizeof(u64));
>+	for (i = 0; i < 8; i++) {

Array size instead of "8"?


>+		data[k++] = status.hl2.qlevel_hwm[i];
>+		data[k++] = status.hl2.qlevel[i];
>+	}

[...]


> 
>+#define IS_PQRS(device_id) \
>+	(((device_id) == SJA1105PR_DEVICE_ID) || \
>+	 ((device_id) == SJA1105QS_DEVICE_ID))
>+#define IS_ET(device_id) \
>+	(((device_id) == SJA1105E_DEVICE_ID) || \
>+	 ((device_id) == SJA1105T_DEVICE_ID))
>+/* P and R have same Device ID, and differ by Part Number */
>+#define IS_P(device_id, part_nr) \
>+	(((device_id) == SJA1105PR_DEVICE_ID) && \
>+	 ((part_nr) == SJA1105P_PART_NR))
>+#define IS_R(device_id, part_nr) \
>+	(((device_id) == SJA1105PR_DEVICE_ID) && \
>+	 ((part_nr) == SJA1105R_PART_NR))
>+/* Same do Q and S */
>+#define IS_Q(device_id, part_nr) \
>+	(((device_id) == SJA1105QS_DEVICE_ID) && \
>+	 ((part_nr) == SJA1105Q_PART_NR))
>+#define IS_S(device_id, part_nr) \

Please have a prefix for macros like this. "IS_S" sounds way too
generic...


>+	(((device_id) == SJA1105QS_DEVICE_ID) && \
>+	 ((part_nr) == SJA1105S_PART_NR))
>+
> struct sja1105_general_params_entry {
> 	u64 vllupformat;
> 	u64 mirr_ptacu;
>-- 
>2.17.1
>