[PATCH 00/14] net: dsa: Fixes and enhancements

[PATCH 00/14] net: dsa: Fixes and enhancements

[Guenter Roeck]

Patch 01/14 addresses an annoying and unhelpful log message.

Patches 02/14 and 03/14 are minor enhancements, adding support for
known switch revisions.

Patches 04/14 and 05/14 add support for MV88E6352 and MV88E6176.

Patch 06/14 adds support for hardware monitoring, specifically for
reporting the chip temperature, to the dsa subsystem.

Patches 07/14 and 08/14 implement hardware monitoring for MV88E6352,
MV88E6176, MV88E6123, MV88E6161, and MV88E6165.

Patch 09/14 adds support for EEPROM access to the DSA subsystem.

Patch 10/14 implements EEPROM access for MV88E6352 and MV88E6176.

Patch 11/14 adds support for reading switch registers to the DSA
subsystem.

Patches 12/14 amd 13/14 implement support for reading switch registers
to the drivers for MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.

Patch 14/14 adds support for reading additional RMON registers to the drivers
for  MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.

The series resides and was tested on top of v3.18-rc1 in a system
with MV88E6352. Testing in systems with 88E6131, 88E6060 and MV88E6165
was done earlier (I don't have access to those systems right now).
The series applies cleanly to net-next as of today (10/22).

----------------------------------------------------------------
Guenter Roeck (14):
      net: dsa: Don't set skb->protocol on outgoing tagged packets
      net: dsa: Report known silicon revisions for Marvell 88E6060
      net: dsa: Report known silicon revisions for Marvell 88E6131
      net: dsa: Add support for Marvell 88E6352
      net: dsa/mv88e6352: Add support for MV88E6176
      net: dsa: Add support for hardware monitoring
      net: dsa/mv88e6352: Report chip temperature
      net: dsa/mv88e6123_61_65: Report chip temperature
      net: dsa: Add support for switch EEPROM access
      net: dsa/mv88e6352: Implement EEPROM access functions
      net: dsa: Add support for reading switch registers with ethtool
      net: dsa/mv88e6123_61_65: Add support for reading switch registers
      net: dsa/mv88e6352: Add support for reading switch registers
      net: dsa: Provide additional RMON statistics

 MAINTAINERS                       |   5 +
 drivers/net/dsa/Kconfig           |   9 +
 drivers/net/dsa/Makefile          |   3 +
 drivers/net/dsa/mv88e6060.c       |   5 +-
 drivers/net/dsa/mv88e6123_61_65.c |  68 +++-
 drivers/net/dsa/mv88e6131.c       |  12 +-
 drivers/net/dsa/mv88e6352.c       | 789 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.c       |  53 ++-
 drivers/net/dsa/mv88e6xxx.h       |  15 +
 include/net/dsa.h                 |  20 +
 net/dsa/dsa.c                     | 111 ++++++
 net/dsa/slave.c                   |  60 +++
 net/dsa/tag_dsa.c                 |   2 -
 net/dsa/tag_edsa.c                |   2 -
 net/dsa/tag_trailer.c             |   2 -
 15 files changed, 1138 insertions(+), 18 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6352.c

[PATCH 01/14] net: dsa: Don't set skb->protocol on outgoing tagged packets

[Guenter Roeck]

Setting skb->protocol to a private protocol type may result in warning
messages such as
	e1000e 0000:00:19.0 em1: checksum_partial proto=dada!

This happens if the L3 protocol is IP or IPv6 and skb->ip_summed is set
to CHECKSUM_PARTIAL. Looking through the code, it appears that changing
skb->protocol for transmitted packets is not necessary and may actually
be harmful. Drop it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 net/dsa/tag_dsa.c     | 2 --
 net/dsa/tag_edsa.c    | 2 --
 net/dsa/tag_trailer.c | 2 --
 3 files changed, 6 deletions(-)

diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index ce90c8b..2dab270 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -63,8 +63,6 @@ static netdev_tx_t dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 		dsa_header[3] = 0x00;
 	}
 
-	skb->protocol = htons(ETH_P_DSA);
-
 	skb->dev = p->parent->dst->master_netdev;
 	dev_queue_xmit(skb);
 
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 94fcce7..9aeda59 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -76,8 +76,6 @@ static netdev_tx_t edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 		edsa_header[7] = 0x00;
 	}
 
-	skb->protocol = htons(ETH_P_EDSA);
-
 	skb->dev = p->parent->dst->master_netdev;
 	dev_queue_xmit(skb);
 
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 115fdca..e268f9d 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -57,8 +57,6 @@ static netdev_tx_t trailer_xmit(struct sk_buff *skb, struct net_device *dev)
 	trailer[2] = 0x10;
 	trailer[3] = 0x00;
 
-	nskb->protocol = htons(ETH_P_TRAILER);
-
 	nskb->dev = p->parent->dst->master_netdev;
 	dev_queue_xmit(nskb);
 
-- 
1.9.1

[PATCH 02/14] net: dsa: Report known silicon revisions for Marvell 88E6060

[Guenter Roeck]

Report known silicon revisions when probing Marvell 88E6060 switches.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6060.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index 05b0ca3..c29aebe 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -69,8 +69,11 @@ static char *mv88e6060_probe(struct device *host_dev, int sw_addr)
 
 	ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
 	if (ret >= 0) {
-		ret &= 0xfff0;
 		if (ret == 0x0600)
+			return "Marvell 88E6060 (A0)";
+		if (ret == 0x0601 || ret == 0x0602)
+			return "Marvell 88E6060 (B0)";
+		if ((ret & 0xfff0) == 0x0600)
 			return "Marvell 88E6060";
 	}
 
-- 
1.9.1

[PATCH 03/14] net: dsa: Report known silicon revisions for Marvell 88E6131

[Guenter Roeck]

Report known silicon revisions when probing Marvell 88E6131 switches.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6131.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 244c735..1230f52 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -21,6 +21,7 @@
 #define ID_6085		0x04a0
 #define ID_6095		0x0950
 #define ID_6131		0x1060
+#define ID_6131_B2	0x1066
 
 static char *mv88e6131_probe(struct device *host_dev, int sw_addr)
 {
@@ -32,12 +33,15 @@ static char *mv88e6131_probe(struct device *host_dev, int sw_addr)
 
 	ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
 	if (ret >= 0) {
-		ret &= 0xfff0;
-		if (ret == ID_6085)
+		int ret_masked = ret & 0xfff0;
+
+		if (ret_masked == ID_6085)
 			return "Marvell 88E6085";
-		if (ret == ID_6095)
+		if (ret_masked == ID_6095)
 			return "Marvell 88E6095/88E6095F";
-		if (ret == ID_6131)
+		if (ret == ID_6131_B2)
+			return "Marvell 88E6131 (B2)";
+		if (ret_masked == ID_6131)
 			return "Marvell 88E6131";
 	}
 
-- 
1.9.1

[PATCH 04/14] net: dsa: Add support for Marvell 88E6352

[Guenter Roeck]

Marvell 88E6352 is mostly compatible to MV88E6123/61/65,
but requires indirect phy access. Also, its configuration
registers are a bit different.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 MAINTAINERS                 |   5 +
 drivers/net/dsa/Kconfig     |   8 +
 drivers/net/dsa/Makefile    |   3 +
 drivers/net/dsa/mv88e6352.c | 464 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.c |   3 +
 drivers/net/dsa/mv88e6xxx.h |   7 +
 6 files changed, 490 insertions(+)
 create mode 100644 drivers/net/dsa/mv88e6352.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a20df9b..e48a05b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5845,6 +5845,11 @@ M:	Russell King <rmk+kernel@arm.linux.org.uk>
 S:	Maintained
 F:	drivers/gpu/drm/armada/
 
+MARVELL 88E6352 DSA support
+M:	Guenter Roeck <linux@roeck-us.net>
+S:	Maintained
+F:	drivers/net/dsa/mv88e6352.c
+
 MARVELL GIGABIT ETHERNET DRIVERS (skge/sky2)
 M:	Mirko Lindner <mlindner@marvell.com>
 M:	Stephen Hemminger <stephen@networkplumber.org>
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 9234d80..0987c33 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -45,6 +45,14 @@ config NET_DSA_MV88E6171
 	  This enables support for the Marvell 88E6171 ethernet switch
 	  chip.
 
+config NET_DSA_MV88E6352
+	tristate "Marvell 88E6352 ethernet switch chip support"
+	select NET_DSA
+	select NET_DSA_MV88E6XXX
+	select NET_DSA_TAG_EDSA
+	---help---
+	  This enables support for the Marvell 88E6352 ethernet switch chip.
+
 config NET_DSA_BCM_SF2
 	tristate "Broadcom Starfighter 2 Ethernet switch support"
 	depends on HAS_IOMEM
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 23a90de..e2d51c4 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -7,6 +7,9 @@ endif
 ifdef CONFIG_NET_DSA_MV88E6131
 mv88e6xxx_drv-y += mv88e6131.o
 endif
+ifdef CONFIG_NET_DSA_MV88E6352
+mv88e6xxx_drv-y += mv88e6352.o
+endif
 ifdef CONFIG_NET_DSA_MV88E6171
 mv88e6xxx_drv-y += mv88e6171.o
 endif
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
new file mode 100644
index 0000000..43a5826
--- /dev/null
+++ b/drivers/net/dsa/mv88e6352.c
@@ -0,0 +1,464 @@
+/*
+ * net/dsa/mv88e6352.c - Marvell 88e6352 switch chip support
+ *
+ * Copyright (c) 2014 Guenter Roeck
+ *
+ * Derived from mv88e6123_61_65.c
+ * Copyright (c) 2008-2009 Marvell Semiconductor
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/phy.h>
+#include <net/dsa.h>
+#include "mv88e6xxx.h"
+
+static int mv88e6352_phy_wait(struct dsa_switch *ds)
+{
+	unsigned long timeout = jiffies + HZ / 10;
+
+	while (time_before(jiffies, timeout)) {
+		int ret;
+
+		ret = REG_READ(REG_GLOBAL2, 0x18);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & 0x8000))
+			return 0;
+
+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;
+}
+
+static int __mv88e6352_phy_read(struct dsa_switch *ds, int addr, int regnum)
+{
+	int ret;
+
+	REG_WRITE(REG_GLOBAL2, 0x18, 0x9800 | (addr << 5) | regnum);
+
+	ret = mv88e6352_phy_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	return REG_READ(REG_GLOBAL2, 0x19);
+}
+
+static int __mv88e6352_phy_write(struct dsa_switch *ds, int addr, int regnum,
+				 u16 val)
+{
+	REG_WRITE(REG_GLOBAL2, 0x19, val);
+	REG_WRITE(REG_GLOBAL2, 0x18, 0x9400 | (addr << 5) | regnum);
+
+	return mv88e6352_phy_wait(ds);
+}
+
+static char *mv88e6352_probe(struct device *host_dev, int sw_addr)
+{
+	struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
+	int ret;
+
+	if (bus == NULL)
+		return NULL;
+
+	ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
+	if (ret >= 0) {
+		if (ret == 0x3521)
+			return "Marvell 88E6352 (A0)";
+		if (ret == 0x3522)
+			return "Marvell 88E6352 (A1)";
+		if ((ret & 0xfff0) == 0x3520)
+			return "Marvell 88E6352";
+	}
+
+	return NULL;
+}
+
+static int mv88e6352_switch_reset(struct dsa_switch *ds)
+{
+	unsigned long timeout;
+	int ret;
+	int i;
+
+	/* Set all ports to the disabled state. */
+	for (i = 0; i < 7; i++) {
+		ret = REG_READ(REG_PORT(i), 0x04);
+		REG_WRITE(REG_PORT(i), 0x04, ret & 0xfffc);
+	}
+
+	/* Wait for transmit queues to drain. */
+	usleep_range(2000, 4000);
+
+	/* Reset the switch. Keep PPU active (bit 14, undocumented).
+	 * The PPU needs to be active to support indirect phy register
+	 * accesses through global registers 0x18 and 0x19.
+	 */
+	REG_WRITE(REG_GLOBAL, 0x04, 0xc000);
+
+	/* Wait up to one second for reset to complete. */
+	timeout = jiffies + 1 * HZ;
+	while (time_before(jiffies, timeout)) {
+		ret = REG_READ(REG_GLOBAL, 0x00);
+		if ((ret & 0x8800) == 0x8800)
+			break;
+		usleep_range(1000, 2000);
+	}
+	if (time_after(jiffies, timeout))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int mv88e6352_setup_global(struct dsa_switch *ds)
+{
+	int ret;
+	int i;
+
+	/* Discard packets with excessive collisions,
+	 * mask all interrupt sources, enable PPU (bit 14, undocumented).
+	 */
+	REG_WRITE(REG_GLOBAL, 0x04, 0x6000);
+
+	/* Set the default address aging time to 5 minutes, and
+	 * enable address learn messages to be sent to all message
+	 * ports.
+	 */
+	REG_WRITE(REG_GLOBAL, 0x0a, 0x0148);
+
+	/* Configure the priority mapping registers. */
+	ret = mv88e6xxx_config_prio(ds);
+	if (ret < 0)
+		return ret;
+
+	/* Configure the upstream port, and configure the upstream
+	 * port as the port to which ingress and egress monitor frames
+	 * are to be sent.
+	 */
+	REG_WRITE(REG_GLOBAL, 0x1a, (dsa_upstream_port(ds) * 0x1110));
+
+	/* Disable remote management for now, and set the switch's
+	 * DSA device number.
+	 */
+	REG_WRITE(REG_GLOBAL, 0x1c, ds->index & 0x1f);
+
+	/* Send all frames with destination addresses matching
+	 * 01:80:c2:00:00:2x to the CPU port.
+	 */
+	REG_WRITE(REG_GLOBAL2, 0x02, 0xffff);
+
+	/* Send all frames with destination addresses matching
+	 * 01:80:c2:00:00:0x to the CPU port.
+	 */
+	REG_WRITE(REG_GLOBAL2, 0x03, 0xffff);
+
+	/* Disable the loopback filter, disable flow control
+	 * messages, disable flood broadcast override, disable
+	 * removing of provider tags, disable ATU age violation
+	 * interrupts, disable tag flow control, force flow
+	 * control priority to the highest, and send all special
+	 * multicast frames to the CPU at the highest priority.
+	 */
+	REG_WRITE(REG_GLOBAL2, 0x05, 0x00ff);
+
+	/* Program the DSA routing table. */
+	for (i = 0; i < 32; i++) {
+		int nexthop = 0x1f;
+
+		if (i != ds->index && i < ds->dst->pd->nr_chips)
+			nexthop = ds->pd->rtable[i] & 0x1f;
+
+		REG_WRITE(REG_GLOBAL2, 0x06, 0x8000 | (i << 8) | nexthop);
+	}
+
+	/* Clear all trunk masks. */
+	for (i = 0; i < 8; i++)
+		REG_WRITE(REG_GLOBAL2, 0x07, 0x8000 | (i << 12) | 0x7f);
+
+	/* Clear all trunk mappings. */
+	for (i = 0; i < 16; i++)
+		REG_WRITE(REG_GLOBAL2, 0x08, 0x8000 | (i << 11));
+
+	/* Disable ingress rate limiting by resetting all ingress
+	 * rate limit registers to their initial state.
+	 */
+	for (i = 0; i < 7; i++)
+		REG_WRITE(REG_GLOBAL2, 0x09, 0x9000 | (i << 8));
+
+	/* Initialise cross-chip port VLAN table to reset defaults. */
+	REG_WRITE(REG_GLOBAL2, 0x0b, 0x9000);
+
+	/* Clear the priority override table. */
+	for (i = 0; i < 16; i++)
+		REG_WRITE(REG_GLOBAL2, 0x0f, 0x8000 | (i << 8));
+
+	/* @@@ initialise AVB (22/23) watchdog (27) sdet (29) registers */
+
+	return 0;
+}
+
+static int mv88e6352_setup_port(struct dsa_switch *ds, int p)
+{
+	int addr = REG_PORT(p);
+	u16 val;
+
+	/* MAC Forcing register: don't force link, speed, duplex
+	 * or flow control state to any particular values on physical
+	 * ports, but force the CPU port and all DSA ports to 1000 Mb/s
+	 * full duplex.
+	 */
+	if (dsa_is_cpu_port(ds, p) || ds->dsa_port_mask & (1 << p))
+		REG_WRITE(addr, 0x01, 0x003e);
+	else
+		REG_WRITE(addr, 0x01, 0x0003);
+
+	/* Do not limit the period of time that this port can be
+	 * paused for by the remote end or the period of time that
+	 * this port can pause the remote end.
+	 */
+	REG_WRITE(addr, 0x02, 0x0000);
+
+	/* Port Control: disable Drop-on-Unlock, disable Drop-on-Lock,
+	 * disable Header mode, enable IGMP/MLD snooping, disable VLAN
+	 * tunneling, determine priority by looking at 802.1p and IP
+	 * priority fields (IP prio has precedence), and set STP state
+	 * to Forwarding.
+	 *
+	 * If this is the CPU link, use DSA or EDSA tagging depending
+	 * on which tagging mode was configured.
+	 *
+	 * If this is a link to another switch, use DSA tagging mode.
+	 *
+	 * If this is the upstream port for this switch, enable
+	 * forwarding of unknown unicasts and multicasts.
+	 */
+	val = 0x0433;
+	if (dsa_is_cpu_port(ds, p)) {
+		if (ds->dst->tag_protocol == DSA_TAG_PROTO_EDSA)
+			val |= 0x3300;
+		else
+			val |= 0x0100;
+	}
+	if (ds->dsa_port_mask & (1 << p))
+		val |= 0x0100;
+	if (p == dsa_upstream_port(ds))
+		val |= 0x000c;
+	REG_WRITE(addr, 0x04, val);
+
+	/* Port Control 1: disable trunking.  Also, if this is the
+	 * CPU port, enable learn messages to be sent to this port.
+	 */
+	REG_WRITE(addr, 0x05, dsa_is_cpu_port(ds, p) ? 0x8000 : 0x0000);
+
+	/* Port based VLAN map: give each port its own address
+	 * database, allow the CPU port to talk to each of the 'real'
+	 * ports, and allow each of the 'real' ports to only talk to
+	 * the upstream port.
+	 */
+	val = (p & 0xf) << 12;
+	if (dsa_is_cpu_port(ds, p))
+		val |= ds->phys_port_mask;
+	else
+		val |= 1 << dsa_upstream_port(ds);
+	REG_WRITE(addr, 0x06, val);
+
+	/* Default VLAN ID and priority: don't set a default VLAN
+	 * ID, and set the default packet priority to zero.
+	 */
+	REG_WRITE(addr, 0x07, 0x0000);
+
+	/* Port Control 2: don't force a good FCS, set the maximum
+	 * frame size to 10240 bytes, don't let the switch add or
+	 * strip 802.1q tags, don't discard tagged or untagged frames
+	 * on this port, do a destination address lookup on all
+	 * received packets as usual, disable ARP mirroring and don't
+	 * send a copy of all transmitted/received frames on this port
+	 * to the CPU.
+	 */
+	REG_WRITE(addr, 0x08, 0x2080);
+
+	/* Egress rate control: disable egress rate control. */
+	REG_WRITE(addr, 0x09, 0x0001);
+
+	/* Egress rate control 2: disable egress rate control. */
+	REG_WRITE(addr, 0x0a, 0x0000);
+
+	/* Port Association Vector: when learning source addresses
+	 * of packets, add the address to the address database using
+	 * a port bitmap that has only the bit for this port set and
+	 * the other bits clear.
+	 */
+	REG_WRITE(addr, 0x0b, 1 << p);
+
+	/* Port ATU control: disable limiting the number of address
+	 * database entries that this port is allowed to use.
+	 */
+	REG_WRITE(addr, 0x0c, 0x0000);
+
+	/* Priority Override: disable DA, SA and VTU priority override. */
+	REG_WRITE(addr, 0x0d, 0x0000);
+
+	/* Port Ethertype: use the Ethertype DSA Ethertype value. */
+	REG_WRITE(addr, 0x0f, ETH_P_EDSA);
+
+	/* Tag Remap: use an identity 802.1p prio -> switch prio
+	 * mapping.
+	 */
+	REG_WRITE(addr, 0x18, 0x3210);
+
+	/* Tag Remap 2: use an identity 802.1p prio -> switch prio
+	 * mapping.
+	 */
+	REG_WRITE(addr, 0x19, 0x7654);
+
+	return 0;
+}
+
+static int mv88e6352_setup(struct dsa_switch *ds)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+	int i;
+
+	mutex_init(&ps->smi_mutex);
+	mutex_init(&ps->stats_mutex);
+	mutex_init(&ps->phy_mutex);
+
+	ps->id = REG_READ(REG_PORT(0), 0x03) & 0xfff0;
+
+	ret = mv88e6352_switch_reset(ds);
+	if (ret < 0)
+		return ret;
+
+	/* @@@ initialise vtu and atu */
+
+	ret = mv88e6352_setup_global(ds);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < 7; i++) {
+		ret = mv88e6352_setup_port(ds, i);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int mv88e6352_port_to_phy_addr(int port)
+{
+	if (port >= 0 && port <= 4)
+		return port;
+	return -EINVAL;
+}
+
+static int
+mv88e6352_phy_read(struct dsa_switch *ds, int port, int regnum)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int addr = mv88e6352_port_to_phy_addr(port);
+	int ret;
+
+	if (addr < 0)
+		return addr;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = __mv88e6352_phy_read(ds, addr, regnum);
+	mutex_unlock(&ps->phy_mutex);
+
+	return ret;
+}
+
+static int
+mv88e6352_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int addr = mv88e6352_port_to_phy_addr(port);
+	int ret;
+
+	if (addr < 0)
+		return addr;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = __mv88e6352_phy_write(ds, addr, regnum, val);
+	mutex_unlock(&ps->phy_mutex);
+
+	return ret;
+}
+
+static struct mv88e6xxx_hw_stat mv88e6352_hw_stats[] = {
+	{ "in_good_octets", 8, 0x00, },
+	{ "in_bad_octets", 4, 0x02, },
+	{ "in_unicast", 4, 0x04, },
+	{ "in_broadcasts", 4, 0x06, },
+	{ "in_multicasts", 4, 0x07, },
+	{ "in_pause", 4, 0x16, },
+	{ "in_undersize", 4, 0x18, },
+	{ "in_fragments", 4, 0x19, },
+	{ "in_oversize", 4, 0x1a, },
+	{ "in_jabber", 4, 0x1b, },
+	{ "in_rx_error", 4, 0x1c, },
+	{ "in_fcs_error", 4, 0x1d, },
+	{ "out_octets", 8, 0x0e, },
+	{ "out_unicast", 4, 0x10, },
+	{ "out_broadcasts", 4, 0x13, },
+	{ "out_multicasts", 4, 0x12, },
+	{ "out_pause", 4, 0x15, },
+	{ "excessive", 4, 0x11, },
+	{ "collisions", 4, 0x1e, },
+	{ "deferred", 4, 0x05, },
+	{ "single", 4, 0x14, },
+	{ "multiple", 4, 0x17, },
+	{ "out_fcs_error", 4, 0x03, },
+	{ "late", 4, 0x1f, },
+	{ "hist_64bytes", 4, 0x08, },
+	{ "hist_65_127bytes", 4, 0x09, },
+	{ "hist_128_255bytes", 4, 0x0a, },
+	{ "hist_256_511bytes", 4, 0x0b, },
+	{ "hist_512_1023bytes", 4, 0x0c, },
+	{ "hist_1024_max_bytes", 4, 0x0d, },
+};
+
+static void
+mv88e6352_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
+{
+	mv88e6xxx_get_strings(ds, ARRAY_SIZE(mv88e6352_hw_stats),
+			      mv88e6352_hw_stats, port, data);
+}
+
+static void
+mv88e6352_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
+{
+	mv88e6xxx_get_ethtool_stats(ds, ARRAY_SIZE(mv88e6352_hw_stats),
+				    mv88e6352_hw_stats, port, data);
+}
+
+static int mv88e6352_get_sset_count(struct dsa_switch *ds)
+{
+	return ARRAY_SIZE(mv88e6352_hw_stats);
+}
+
+struct dsa_switch_driver mv88e6352_switch_driver = {
+	.tag_protocol		= DSA_TAG_PROTO_EDSA,
+	.priv_size		= sizeof(struct mv88e6xxx_priv_state),
+	.probe			= mv88e6352_probe,
+	.setup			= mv88e6352_setup,
+	.set_addr		= mv88e6xxx_set_addr_indirect,
+	.phy_read		= mv88e6352_phy_read,
+	.phy_write		= mv88e6352_phy_write,
+	.poll_link		= mv88e6xxx_poll_link,
+	.get_strings		= mv88e6352_get_strings,
+	.get_ethtool_stats	= mv88e6352_get_ethtool_stats,
+	.get_sset_count		= mv88e6352_get_sset_count,
+};
+
+MODULE_ALIAS("platform:mv88e6352");
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index a6c90cf..8e1090b 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -507,6 +507,9 @@ static int __init mv88e6xxx_init(void)
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6123_61_65)
 	register_switch_driver(&mv88e6123_61_65_switch_driver);
 #endif
+#if IS_ENABLED(CONFIG_NET_DSA_MV88E6352)
+	register_switch_driver(&mv88e6352_switch_driver);
+#endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6171)
 	register_switch_driver(&mv88e6171_switch_driver);
 #endif
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 5e5145a..77beff5 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -37,6 +37,12 @@ struct mv88e6xxx_priv_state {
 	 */
 	struct mutex	stats_mutex;
 
+	/* This mutex serializes phy access for chips with
+	 * indirect phy addressing. It is unused for chips
+	 * with direct phy access.
+	 */
+	struct mutex phy_mutex;
+
 	int		id; /* switch product id */
 };
 
@@ -70,6 +76,7 @@ void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
 
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
+extern struct dsa_switch_driver mv88e6352_switch_driver;
 extern struct dsa_switch_driver mv88e6171_switch_driver;
 
 #define REG_READ(addr, reg)						\
-- 
1.9.1

[PATCH 05/14] net: dsa/mv88e6352: Add support for MV88E6176

[Guenter Roeck]

MV88E6176 is mostly compatible to MV88E6352 and is documented
in the same functional specification. Add support for it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/Kconfig     | 5 +++--
 drivers/net/dsa/mv88e6352.c | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 0987c33..2d1a55e 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -46,12 +46,13 @@ config NET_DSA_MV88E6171
 	  chip.
 
 config NET_DSA_MV88E6352
-	tristate "Marvell 88E6352 ethernet switch chip support"
+	tristate "Marvell 88E6176/88E6352 ethernet switch chip support"
 	select NET_DSA
 	select NET_DSA_MV88E6XXX
 	select NET_DSA_TAG_EDSA
 	---help---
-	  This enables support for the Marvell 88E6352 ethernet switch chip.
+	  This enables support for the Marvell 88E6176 and 88E6352 ethernet
+	  switch chips.
 
 config NET_DSA_BCM_SF2
 	tristate "Broadcom Starfighter 2 Ethernet switch support"
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 43a5826..f17364f 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -73,6 +73,8 @@ static char *mv88e6352_probe(struct device *host_dev, int sw_addr)
 
 	ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
 	if (ret >= 0) {
+		if ((ret & 0xfff0) == 0x1760)
+			return "Marvell 88E6176";
 		if (ret == 0x3521)
 			return "Marvell 88E6352 (A0)";
 		if (ret == 0x3522)
-- 
1.9.1

[PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

Some Marvell switches provide chip temperature data.
Add support for reporting it to the dsa infrastructure.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/net/dsa.h |   6 +++
 net/dsa/dsa.c     | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b765592..d8b3057 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -242,6 +242,12 @@ struct dsa_switch_driver {
 			   struct ethtool_eee *e);
 	int	(*get_eee)(struct dsa_switch *ds, int port,
 			   struct ethtool_eee *e);
+
+	/* Hardware monitoring */
+	int	(*get_temp)(struct dsa_switch *ds, int *temp);
+	int	(*get_temp_limit)(struct dsa_switch *ds, int *temp);
+	int	(*set_temp_limit)(struct dsa_switch *ds, int temp);
+	int	(*get_temp_alarm)(struct dsa_switch *ds, bool *alarm);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 22f34cf..6567c8e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -9,6 +9,8 @@
  * (at your option) any later version.
  */
 
+#include <linux/device.h>
+#include <linux/hwmon.h>
 #include <linux/list.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -17,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_platform.h>
+#include <linux/sysfs.h>
 #include "dsa_priv.h"
 
 char dsa_driver_version[] = "0.1";
@@ -71,6 +74,104 @@ dsa_switch_probe(struct device *host_dev, int sw_addr, char **_name)
 	return ret;
 }
 
+/* hwmon support ************************************************************/
+
+#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
+
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = ds->drv->get_temp(ds, &temp);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", temp * 1000);
+}
+static DEVICE_ATTR_RO(temp1_input);
+
+static ssize_t temp1_max_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = ds->drv->get_temp_limit(ds, &temp);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", temp * 1000);
+}
+
+static ssize_t temp1_max_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	int temp, ret;
+
+	ret = kstrtoint(buf, 0, &temp);
+	if (ret < 0)
+		return ret;
+
+	ret = ds->drv->set_temp_limit(ds, DIV_ROUND_CLOSEST(temp, 1000));
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR(temp1_max, S_IRUGO, temp1_max_show, temp1_max_store);
+
+static ssize_t temp1_max_alarm_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	bool alarm;
+	int ret;
+
+	ret = ds->drv->get_temp_alarm(ds, &alarm);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", alarm);
+}
+static DEVICE_ATTR_RO(temp1_max_alarm);
+
+static struct attribute *dsa_hwmon_attrs[] = {
+	&dev_attr_temp1_input.attr,	/* 0 */
+	&dev_attr_temp1_max.attr,	/* 1 */
+	&dev_attr_temp1_max_alarm.attr,	/* 2 */
+	NULL
+};
+
+static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
+				       struct attribute *attr, int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct dsa_switch *ds = dev_get_drvdata(dev);
+	struct dsa_switch_driver *drv = ds->drv;
+	umode_t mode = attr->mode;
+
+	if (index == 1) {
+		if (!drv->get_temp_limit)
+			mode = 0;
+		else if (drv->set_temp_limit)
+			mode |= S_IWUSR;
+	} else if (index == 2 && !drv->get_temp_alarm) {
+		mode = 0;
+	}
+	return mode;
+}
+
+static const struct attribute_group dsa_hwmon_group = {
+	.attrs = dsa_hwmon_attrs,
+	.is_visible = dsa_hwmon_attrs_visible,
+};
+__ATTRIBUTE_GROUPS(dsa_hwmon);
+
+#endif /* CONFIG_HWMON */
 
 /* basic switch operations **************************************************/
 static struct dsa_switch *
@@ -225,6 +326,16 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
 		ds->ports[i] = slave_dev;
 	}
 
+#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
+	/* If the switch provides a temperature sensor,
+	 * register with hardware monitoring subsystem.
+	 * Treat registration error as non-fatal and ignore it.
+	 */
+	if (drv->get_temp)
+		devm_hwmon_device_register_with_groups(parent, "dsa", ds,
+						       dsa_hwmon_groups);
+#endif
+
 	return ds;
 
 out_free:
-- 
1.9.1

[PATCH 07/14] net: dsa/mv88e6352: Report chip temperature

[Guenter Roeck]

MV88E6352 supports reading the chip temperature from two PHY registers,
6:26 and 6:27. Report it using the more accurate register 6:27.
Also report temperature limit and alarm.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6352.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index f17364f..aff6695 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -63,6 +63,41 @@ static int __mv88e6352_phy_write(struct dsa_switch *ds, int addr, int regnum,
 	return mv88e6352_phy_wait(ds);
 }
 
+static int mv88e6352_phy_page_read(struct dsa_switch *ds,
+				   int port, int page, int reg)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = __mv88e6352_phy_write(ds, port, 0x16, page);
+	if (ret < 0)
+		goto error;
+	ret = __mv88e6352_phy_read(ds, port, reg);
+error:
+	__mv88e6352_phy_write(ds, port, 0x16, 0x0);
+	mutex_unlock(&ps->phy_mutex);
+	return ret;
+}
+
+static int mv88e6352_phy_page_write(struct dsa_switch *ds,
+				    int port, int page, int reg, int val)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = __mv88e6352_phy_write(ds, port, 0x16, page);
+	if (ret < 0)
+		goto error;
+
+	ret = __mv88e6352_phy_write(ds, port, reg, val);
+error:
+	__mv88e6352_phy_write(ds, port, 0x16, 0x0);
+	mutex_unlock(&ps->phy_mutex);
+	return ret;
+}
+
 static char *mv88e6352_probe(struct device *host_dev, int sw_addr)
 {
 	struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
@@ -325,6 +360,63 @@ static int mv88e6352_setup_port(struct dsa_switch *ds, int p)
 	return 0;
 }
 
+static int mv88e6352_get_temp(struct dsa_switch *ds, int *temp)
+{
+	int ret;
+
+	*temp = 0;
+
+	ret = mv88e6352_phy_page_read(ds, 0, 6, 27);
+	if (ret < 0)
+		return ret;
+
+	*temp = (ret & 0xff) - 25;
+
+	return 0;
+}
+
+static int mv88e6352_get_temp_limit(struct dsa_switch *ds, int *temp)
+{
+	int ret;
+
+	*temp = 0;
+
+	ret = mv88e6352_phy_page_read(ds, 0, 6, 26);
+	if (ret < 0)
+		return ret;
+
+	*temp = (((ret >> 8) & 0x1f) * 5) - 25;
+
+	return 0;
+}
+
+static int mv88e6352_set_temp_limit(struct dsa_switch *ds, int temp)
+{
+	int ret;
+
+	ret = mv88e6352_phy_page_read(ds, 0, 6, 26);
+	if (ret < 0)
+		return ret;
+	temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
+	return mv88e6352_phy_page_write(ds, 0, 6, 26,
+					(ret & 0xe0ff) | (temp << 8));
+}
+
+static int mv88e6352_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
+{
+	int ret;
+
+	*alarm = false;
+
+	ret = mv88e6352_phy_page_read(ds, 0, 6, 26);
+	if (ret < 0)
+		return ret;
+
+	*alarm = !!(ret & 0x40);
+
+	return 0;
+}
+
 static int mv88e6352_setup(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -461,6 +553,10 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.get_strings		= mv88e6352_get_strings,
 	.get_ethtool_stats	= mv88e6352_get_ethtool_stats,
 	.get_sset_count		= mv88e6352_get_sset_count,
+	.get_temp		= mv88e6352_get_temp,
+	.get_temp_limit		= mv88e6352_get_temp_limit,
+	.set_temp_limit		= mv88e6352_set_temp_limit,
+	.get_temp_alarm		= mv88e6352_get_temp_alarm,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
-- 
1.9.1

[PATCH 08/14] net: dsa/mv88e6123_61_65: Report chip temperature

[Guenter Roeck]

MV88E6123 and compatible chips support reading the chip temperature
from PHY register 6:26.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6123_61_65.c | 63 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index a332c53..17dc60e 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -291,6 +291,51 @@ static int mv88e6123_61_65_setup_port(struct dsa_switch *ds, int p)
 	return 0;
 }
 
+static int  mv88e6123_61_65_get_temp(struct dsa_switch *ds, int *temp)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+	int val;
+
+	*temp = 0;
+
+	mutex_lock(&ps->phy_mutex);
+
+	ret = mv88e6xxx_phy_write(ds, 0x0, 0x16, 0x6);
+	if (ret < 0)
+		goto error;
+
+	/* Enable temperature sensor */
+	ret = mv88e6xxx_phy_read(ds, 0x0, 0x1a);
+	if (ret < 0)
+		goto error;
+
+	ret = mv88e6xxx_phy_write(ds, 0x0, 0x1a, ret | (1 << 5));
+	if (ret < 0)
+		goto error;
+
+	/* Wait for temperature to stabilize */
+	usleep_range(10000, 12000);
+
+	val = mv88e6xxx_phy_read(ds, 0x0, 0x1a);
+	if (val < 0) {
+		ret = val;
+		goto error;
+	}
+
+	/* Disable temperature sensor */
+	ret = mv88e6xxx_phy_write(ds, 0x0, 0x1a, ret & ~(1 << 5));
+	if (ret < 0)
+		goto error;
+
+	*temp = ((val & 0x1f) - 5) * 5;
+
+error:
+	mv88e6xxx_phy_write(ds, 0x0, 0x16, 0x0);
+	mutex_unlock(&ps->phy_mutex);
+	return ret;
+}
+
 static int mv88e6123_61_65_setup(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -299,6 +344,7 @@ static int mv88e6123_61_65_setup(struct dsa_switch *ds)
 
 	mutex_init(&ps->smi_mutex);
 	mutex_init(&ps->stats_mutex);
+	mutex_init(&ps->phy_mutex);
 
 	ret = mv88e6123_61_65_switch_reset(ds);
 	if (ret < 0)
@@ -329,16 +375,28 @@ static int mv88e6123_61_65_port_to_phy_addr(int port)
 static int
 mv88e6123_61_65_phy_read(struct dsa_switch *ds, int port, int regnum)
 {
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int addr = mv88e6123_61_65_port_to_phy_addr(port);
-	return mv88e6xxx_phy_read(ds, addr, regnum);
+	int ret;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = mv88e6xxx_phy_read(ds, addr, regnum);
+	mutex_unlock(&ps->phy_mutex);
+	return ret;
 }
 
 static int
 mv88e6123_61_65_phy_write(struct dsa_switch *ds,
 			      int port, int regnum, u16 val)
 {
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int addr = mv88e6123_61_65_port_to_phy_addr(port);
-	return mv88e6xxx_phy_write(ds, addr, regnum, val);
+	int ret;
+
+	mutex_lock(&ps->phy_mutex);
+	ret = mv88e6xxx_phy_write(ds, addr, regnum, val);
+	mutex_unlock(&ps->phy_mutex);
+	return ret;
 }
 
 static struct mv88e6xxx_hw_stat mv88e6123_61_65_hw_stats[] = {
@@ -406,6 +464,7 @@ struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
 	.get_strings		= mv88e6123_61_65_get_strings,
 	.get_ethtool_stats	= mv88e6123_61_65_get_ethtool_stats,
 	.get_sset_count		= mv88e6123_61_65_get_sset_count,
+	.get_temp		= mv88e6123_61_65_get_temp,
 };
 
 MODULE_ALIAS("platform:mv88e6123");
-- 
1.9.1

[PATCH 09/14] net: dsa: Add support for switch EEPROM access

[Guenter Roeck]

On some chips it is possible to access the switch eeprom.
Add infrastructure support for it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/net/dsa.h |  7 +++++++
 net/dsa/slave.c   | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d8b3057..73146b7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -248,6 +248,13 @@ struct dsa_switch_driver {
 	int	(*get_temp_limit)(struct dsa_switch *ds, int *temp);
 	int	(*set_temp_limit)(struct dsa_switch *ds, int temp);
 	int	(*get_temp_alarm)(struct dsa_switch *ds, bool *alarm);
+
+	/* EEPROM access */
+	int	(*get_eeprom_len)(struct dsa_switch *ds);
+	int	(*get_eeprom)(struct dsa_switch *ds,
+			      struct ethtool_eeprom *eeprom, u8 *data);
+	int	(*set_eeprom)(struct dsa_switch *ds,
+			      struct ethtool_eeprom *eeprom, u8 *data);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6d18174..a54ee43 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -271,6 +271,41 @@ static u32 dsa_slave_get_link(struct net_device *dev)
 	return -EOPNOTSUPP;
 }
 
+static int dsa_slave_get_eeprom_len(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->drv->get_eeprom_len != NULL)
+		return ds->drv->get_eeprom_len(ds);
+
+	return 0;
+}
+
+static int dsa_slave_get_eeprom(struct net_device *dev,
+				struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->drv->get_eeprom != NULL)
+		return ds->drv->get_eeprom(ds, eeprom, data);
+
+	return -EOPNOTSUPP;
+}
+
+static int dsa_slave_set_eeprom(struct net_device *dev,
+				struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->drv->set_eeprom != NULL)
+		return ds->drv->set_eeprom(ds, eeprom, data);
+
+	return -EOPNOTSUPP;
+}
+
 static void dsa_slave_get_strings(struct net_device *dev,
 				  uint32_t stringset, uint8_t *data)
 {
@@ -387,6 +422,9 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_drvinfo		= dsa_slave_get_drvinfo,
 	.nway_reset		= dsa_slave_nway_reset,
 	.get_link		= dsa_slave_get_link,
+	.get_eeprom_len		= dsa_slave_get_eeprom_len,
+	.get_eeprom		= dsa_slave_get_eeprom,
+	.set_eeprom		= dsa_slave_set_eeprom,
 	.get_strings		= dsa_slave_get_strings,
 	.get_ethtool_stats	= dsa_slave_get_ethtool_stats,
 	.get_sset_count		= dsa_slave_get_sset_count,
-- 
1.9.1

[PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM access functions

[Guenter Roeck]

MV88E6352 supports read and write access to its configuration eeprom.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6352.c | 228 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx.h |   5 +
 2 files changed, 230 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index aff6695..9dddcba 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -22,18 +22,18 @@
 #include <net/dsa.h>
 #include "mv88e6xxx.h"
 
-static int mv88e6352_phy_wait(struct dsa_switch *ds)
+static int mv88e6352_wait(struct dsa_switch *ds, int reg, u16 mask)
 {
 	unsigned long timeout = jiffies + HZ / 10;
 
 	while (time_before(jiffies, timeout)) {
 		int ret;
 
-		ret = REG_READ(REG_GLOBAL2, 0x18);
+		ret = REG_READ(REG_GLOBAL2, reg);
 		if (ret < 0)
 			return ret;
 
-		if (!(ret & 0x8000))
+		if (!(ret & mask))
 			return 0;
 
 		usleep_range(1000, 2000);
@@ -41,6 +41,21 @@ static int mv88e6352_phy_wait(struct dsa_switch *ds)
 	return -ETIMEDOUT;
 }
 
+static inline int mv88e6352_phy_wait(struct dsa_switch *ds)
+{
+	return mv88e6352_wait(ds, 0x18, 0x8000);
+}
+
+static inline int mv88e6352_eeprom_load_wait(struct dsa_switch *ds)
+{
+	return mv88e6352_wait(ds, 0x14, 0x0800);
+}
+
+static inline int mv88e6352_eeprom_busy_wait(struct dsa_switch *ds)
+{
+	return mv88e6352_wait(ds, 0x14, 0x8000);
+}
+
 static int __mv88e6352_phy_read(struct dsa_switch *ds, int addr, int regnum)
 {
 	int ret;
@@ -426,6 +441,7 @@ static int mv88e6352_setup(struct dsa_switch *ds)
 	mutex_init(&ps->smi_mutex);
 	mutex_init(&ps->stats_mutex);
 	mutex_init(&ps->phy_mutex);
+	mutex_init(&ps->eeprom_mutex);
 
 	ps->id = REG_READ(REG_PORT(0), 0x03) & 0xfff0;
 
@@ -522,6 +538,209 @@ static struct mv88e6xxx_hw_stat mv88e6352_hw_stats[] = {
 	{ "hist_1024_max_bytes", 4, 0x0d, },
 };
 
+static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
+{
+	return 0x200;
+}
+
+static int mv88e6352_read_eeprom_word(struct dsa_switch *ds, int addr)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+
+	mutex_lock(&ps->eeprom_mutex);
+
+	ret = mv88e6xxx_reg_write(ds, REG_GLOBAL2, 0x14,
+				  0xc000 | (addr & 0xff));
+	if (ret < 0)
+		goto error;
+
+	ret = mv88e6352_eeprom_busy_wait(ds);
+	if (ret < 0)
+		goto error;
+
+	ret = mv88e6xxx_reg_read(ds, REG_GLOBAL2, 0x15);
+error:
+	mutex_unlock(&ps->eeprom_mutex);
+	return ret;
+}
+
+static int mv88e6352_get_eeprom(struct dsa_switch *ds,
+				struct ethtool_eeprom *eeprom, u8 *data)
+{
+	int offset;
+	int len;
+	int ret;
+
+	offset = eeprom->offset;
+	len = eeprom->len;
+	eeprom->len = 0;
+
+	eeprom->magic = 0xc3ec4951;
+
+	ret = mv88e6352_eeprom_load_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	if (offset & 1) {
+		int word;
+
+		word = mv88e6352_read_eeprom_word(ds, offset >> 1);
+		if (word < 0)
+			return word;
+
+		*data++ = (word >> 8) & 0xff;
+
+		offset++;
+		len--;
+		eeprom->len++;
+	}
+
+	while (len >= 2) {
+		int word;
+
+		word = mv88e6352_read_eeprom_word(ds, offset >> 1);
+		if (word < 0)
+			return word;
+
+		*data++ = word & 0xff;
+		*data++ = (word >> 8) & 0xff;
+
+		offset += 2;
+		len -= 2;
+		eeprom->len += 2;
+	}
+
+	if (len) {
+		int word;
+
+		word = mv88e6352_read_eeprom_word(ds, offset >> 1);
+		if (word < 0)
+			return word;
+
+		*data++ = word & 0xff;
+
+		offset++;
+		len--;
+		eeprom->len++;
+	}
+
+	return 0;
+}
+
+static int mv88e6352_eeprom_is_readonly(struct dsa_switch *ds)
+{
+	int ret;
+
+	ret = mv88e6xxx_reg_read(ds, REG_GLOBAL2, 0x14);
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & 0x0400))
+		return -EROFS;
+
+	return 0;
+}
+
+static int mv88e6352_write_eeprom_word(struct dsa_switch *ds, int addr,
+				       u16 data)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
+
+	mutex_lock(&ps->eeprom_mutex);
+
+	ret = mv88e6xxx_reg_write(ds, REG_GLOBAL2, 0x15, data);
+	if (ret < 0)
+		goto error;
+
+	ret = mv88e6xxx_reg_write(ds, REG_GLOBAL2, 0x14,
+				  0xb000 | (addr & 0xff));
+	if (ret < 0)
+		goto error;
+
+	ret = mv88e6352_eeprom_busy_wait(ds);
+error:
+	mutex_unlock(&ps->eeprom_mutex);
+	return ret;
+}
+
+static int mv88e6352_set_eeprom(struct dsa_switch *ds,
+				struct ethtool_eeprom *eeprom, u8 *data)
+{
+	int offset;
+	int ret;
+	int len;
+
+	if (eeprom->magic != 0xc3ec4951)
+		return -EINVAL;
+
+	ret = mv88e6352_eeprom_is_readonly(ds);
+	if (ret)
+		return ret;
+
+	offset = eeprom->offset;
+	len = eeprom->len;
+	eeprom->len = 0;
+
+	ret = mv88e6352_eeprom_load_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	if (offset & 1) {
+		int word;
+
+		word = mv88e6352_read_eeprom_word(ds, offset >> 1);
+		if (word < 0)
+			return word;
+
+		word = (*data++ << 8) | (word & 0xff);
+
+		ret = mv88e6352_write_eeprom_word(ds, offset >> 1, word);
+		if (ret < 0)
+			return ret;
+
+		offset++;
+		len--;
+		eeprom->len++;
+	}
+
+	while (len >= 2) {
+		int word;
+
+		word = *data++;
+		word |= *data++ << 8;
+
+		ret = mv88e6352_write_eeprom_word(ds, offset >> 1, word);
+		if (ret < 0)
+			return ret;
+
+		offset += 2;
+		len -= 2;
+		eeprom->len += 2;
+	}
+
+	if (len) {
+		int word;
+
+		word = mv88e6352_read_eeprom_word(ds, offset >> 1);
+		if (word < 0)
+			return word;
+
+		word = (word & 0xff00) | *data++;
+
+		ret = mv88e6352_write_eeprom_word(ds, offset >> 1, word);
+		if (ret < 0)
+			return ret;
+
+		offset++;
+		len--;
+		eeprom->len++;
+	}
+
+	return 0;
+}
+
 static void
 mv88e6352_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
 {
@@ -557,6 +776,9 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.get_temp_limit		= mv88e6352_get_temp_limit,
 	.set_temp_limit		= mv88e6352_set_temp_limit,
 	.get_temp_alarm		= mv88e6352_get_temp_alarm,
+	.get_eeprom_len		= mv88e6352_get_eeprom_len,
+	.get_eeprom		= mv88e6352_get_eeprom,
+	.set_eeprom		= mv88e6352_set_eeprom,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 77beff5..d4d53ae 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -43,6 +43,11 @@ struct mv88e6xxx_priv_state {
 	 */
 	struct mutex phy_mutex;
 
+	/* This mutex serializes eeprom access for chips with
+	 * eeprom support.
+	 */
+	struct mutex eeprom_mutex;
+
 	int		id; /* switch product id */
 };
 
-- 
1.9.1

[PATCH 11/14] net: dsa: Add support for reading switch registers with ethtool

[Guenter Roeck]

Add support for reading switch registers with 'ethtool -d'.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/net/dsa.h |  7 +++++++
 net/dsa/slave.c   | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 73146b7..edc5e71 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -255,6 +255,13 @@ struct dsa_switch_driver {
 			      struct ethtool_eeprom *eeprom, u8 *data);
 	int	(*set_eeprom)(struct dsa_switch *ds,
 			      struct ethtool_eeprom *eeprom, u8 *data);
+
+	/*
+	 * Register access.
+	 */
+	int	(*get_regs_len)(struct dsa_switch *ds, int port);
+	void	(*get_regs)(struct dsa_switch *ds, int port,
+			    struct ethtool_regs *regs, void *p);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a54ee43..e988d07 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -249,6 +249,26 @@ static void dsa_slave_get_drvinfo(struct net_device *dev,
 	strlcpy(drvinfo->bus_info, "platform", sizeof(drvinfo->bus_info));
 }
 
+static int dsa_slave_get_regs_len(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->drv->get_regs_len != NULL)
+		return ds->drv->get_regs_len(ds, p->port);
+
+	return -EOPNOTSUPP;
+}
+
+static void
+dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	ds->drv->get_regs(ds, p->port, regs, _p);
+}
+
 static int dsa_slave_nway_reset(struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -420,6 +440,8 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_settings		= dsa_slave_get_settings,
 	.set_settings		= dsa_slave_set_settings,
 	.get_drvinfo		= dsa_slave_get_drvinfo,
+	.get_regs_len		= dsa_slave_get_regs_len,
+	.get_regs		= dsa_slave_get_regs,
 	.nway_reset		= dsa_slave_nway_reset,
 	.get_link		= dsa_slave_get_link,
 	.get_eeprom_len		= dsa_slave_get_eeprom_len,
-- 
1.9.1

[PATCH 12/14] net: dsa/mv88e6123_61_65: Add support for reading switch registers

[Guenter Roeck]

The infrastructure can now report switch registers to ethtool.
Add support for it to the mv88e6123_61_65 driver.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6123_61_65.c |  2 ++
 drivers/net/dsa/mv88e6xxx.c       | 24 ++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h       |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index 17dc60e..a3aeba4 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -465,6 +465,8 @@ struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
 	.get_ethtool_stats	= mv88e6123_61_65_get_ethtool_stats,
 	.get_sset_count		= mv88e6123_61_65_get_sset_count,
 	.get_temp		= mv88e6123_61_65_get_temp,
+	.get_regs_len		= mv88e6xxx_get_regs_len,
+	.get_regs		= mv88e6xxx_get_regs,
 };
 
 MODULE_ALIAS("platform:mv88e6123");
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8e1090b..c071fde 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -499,6 +499,30 @@ void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
 	mutex_unlock(&ps->stats_mutex);
 }
 
+int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
+{
+	return 32 * sizeof(u16);
+}
+
+void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
+			struct ethtool_regs *regs, void *_p)
+{
+	u16 *p = _p;
+	int i;
+
+	regs->version = 0;
+
+	memset(p, 0xff, 32 * sizeof(u16));
+
+	for (i = 0; i < 32; i++) {
+		int ret;
+
+		ret = mv88e6xxx_reg_read(ds, REG_PORT(port), i);
+		if (ret >= 0)
+			p[i] = ret;
+	}
+}
+
 static int __init mv88e6xxx_init(void)
 {
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index d4d53ae..8c75702 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -78,6 +78,9 @@ void mv88e6xxx_get_strings(struct dsa_switch *ds,
 void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
 				 int nr_stats, struct mv88e6xxx_hw_stat *stats,
 				 int port, uint64_t *data);
+int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port);
+void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
+			struct ethtool_regs *regs, void *_p);
 
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
-- 
1.9.1

[PATCH 13/14] net: dsa/mv88e6352: Add support for reading switch registers

[Guenter Roeck]

Report switch register values to ethtool.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6352.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 9dddcba..2f31e28 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -779,6 +779,8 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.get_eeprom_len		= mv88e6352_get_eeprom_len,
 	.get_eeprom		= mv88e6352_get_eeprom,
 	.set_eeprom		= mv88e6352_set_eeprom,
+	.get_regs_len		= mv88e6xxx_get_regs_len,
+	.get_regs		= mv88e6xxx_get_regs,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
-- 
1.9.1

[PATCH 14/14] net: dsa: Provide additional RMON statistics

[Guenter Roeck]

Display sw_in_discards, sw_in_filtered, and sw_out_filtered for chips
supported by mv88e6123_61_65 and mv88e6352 drivers.

The variables are provided in port registers, not the normal status registers.
Mark by adding 0x100 to the register offset and add special handling code
to mv88e6xxx_get_ethtool_stats.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/dsa/mv88e6123_61_65.c |  3 +++
 drivers/net/dsa/mv88e6352.c       |  3 +++
 drivers/net/dsa/mv88e6xxx.c       | 26 +++++++++++++++++++++-----
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index a3aeba4..18c026f 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -430,6 +430,9 @@ static struct mv88e6xxx_hw_stat mv88e6123_61_65_hw_stats[] = {
 	{ "hist_256_511bytes", 4, 0x0b, },
 	{ "hist_512_1023bytes", 4, 0x0c, },
 	{ "hist_1024_max_bytes", 4, 0x0d, },
+	{ "sw_in_discards", 4, 0x110, },
+	{ "sw_in_filtered", 2, 0x112, },
+	{ "sw_out_filtered", 2, 0x113, },
 };
 
 static void
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 2f31e28..85dabd9 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -536,6 +536,9 @@ static struct mv88e6xxx_hw_stat mv88e6352_hw_stats[] = {
 	{ "hist_256_511bytes", 4, 0x0b, },
 	{ "hist_512_1023bytes", 4, 0x0c, },
 	{ "hist_1024_max_bytes", 4, 0x0d, },
+	{ "sw_in_discards", 4, 0x110, },
+	{ "sw_in_filtered", 2, 0x112, },
+	{ "sw_out_filtered", 2, 0x113, },
 };
 
 static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index c071fde..da558d8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -485,17 +485,33 @@ void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
 	for (i = 0; i < nr_stats; i++) {
 		struct mv88e6xxx_hw_stat *s = stats + i;
 		u32 low;
-		u32 high;
-
+		u32 high = 0;
+
+		if (s->reg >= 0x100) {
+			int ret;
+
+			ret = mv88e6xxx_reg_read(ds, REG_PORT(port),
+						 s->reg - 0x100);
+			if (ret < 0)
+				goto error;
+			low = ret;
+			if (s->sizeof_stat == 4) {
+				ret = mv88e6xxx_reg_read(ds, REG_PORT(port),
+							 s->reg - 0x100 + 1);
+				if (ret < 0)
+					goto error;
+				high = ret;
+			}
+			data[i] = (((u64)high) << 16) | low;
+			continue;
+		}
 		mv88e6xxx_stats_read(ds, s->reg, &low);
 		if (s->sizeof_stat == 8)
 			mv88e6xxx_stats_read(ds, s->reg + 1, &high);
-		else
-			high = 0;
 
 		data[i] = (((u64)high) << 32) | low;
 	}
-
+error:
 	mutex_unlock(&ps->stats_mutex);
 }
 
-- 
1.9.1

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Florian Fainelli]

2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
> Some Marvell switches provide chip temperature data.
> Add support for reporting it to the dsa infrastructure.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
[snip]

> +/* hwmon support ************************************************************/
> +
> +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))

IS_ENABLED(CONFIG_HWMON)?

> +
> +static ssize_t temp1_input_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct dsa_switch *ds = dev_get_drvdata(dev);
> +       int temp, ret;
> +
> +       ret = ds->drv->get_temp(ds, &temp);
> +       if (ret < 0)
> +               return ret;
> +
> +       return sprintf(buf, "%d\n", temp * 1000);
> +}
> +static DEVICE_ATTR_RO(temp1_input);

You probably want the number of temperature sensors to come from the
switch driver, and support arbitrary number of temperature sensors?
--
Florian

Re: [PATCH 11/14] net: dsa: Add support for reading switch registers with ethtool

[Florian Fainelli]

2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
> Add support for reading switch registers with 'ethtool -d'.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

[snip]

>
> +static int dsa_slave_get_regs_len(struct net_device *dev)
> +{
> +       struct dsa_slave_priv *p = netdev_priv(dev);
> +       struct dsa_switch *ds = p->parent;
> +
> +       if (ds->drv->get_regs_len != NULL)
> +               return ds->drv->get_regs_len(ds, p->port);

Most of the checks in this file are just:

if (ds->drv->callback)
     return ds->drv->callback(...)

> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static void
> +dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p)
> +{
> +       struct dsa_slave_priv *p = netdev_priv(dev);
> +       struct dsa_switch *ds = p->parent;
> +
> +       ds->drv->get_regs(ds, p->port, regs, _p);

We need to check that the driver does implement this callback here as well.
-- 
Florian

Re: [PATCH 00/14] net: dsa: Fixes and enhancements

[Florian Fainelli]

2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
> Patch 01/14 addresses an annoying and unhelpful log message.
>
> Patches 02/14 and 03/14 are minor enhancements, adding support for
> known switch revisions.
>
> Patches 04/14 and 05/14 add support for MV88E6352 and MV88E6176.
>
> Patch 06/14 adds support for hardware monitoring, specifically for
> reporting the chip temperature, to the dsa subsystem.
>
> Patches 07/14 and 08/14 implement hardware monitoring for MV88E6352,
> MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>
> Patch 09/14 adds support for EEPROM access to the DSA subsystem.
>
> Patch 10/14 implements EEPROM access for MV88E6352 and MV88E6176.
>
> Patch 11/14 adds support for reading switch registers to the DSA
> subsystem.
>
> Patches 12/14 amd 13/14 implement support for reading switch registers
> to the drivers for MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>
> Patch 14/14 adds support for reading additional RMON registers to the drivers
> for  MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>
> The series resides and was tested on top of v3.18-rc1 in a system
> with MV88E6352. Testing in systems with 88E6131, 88E6060 and MV88E6165
> was done earlier (I don't have access to those systems right now).
> The series applies cleanly to net-next as of today (10/22).

Besides the two nitpicking comments, this looks really good to me, thanks!

>
> ----------------------------------------------------------------
> Guenter Roeck (14):
>       net: dsa: Don't set skb->protocol on outgoing tagged packets
>       net: dsa: Report known silicon revisions for Marvell 88E6060
>       net: dsa: Report known silicon revisions for Marvell 88E6131
>       net: dsa: Add support for Marvell 88E6352
>       net: dsa/mv88e6352: Add support for MV88E6176
>       net: dsa: Add support for hardware monitoring
>       net: dsa/mv88e6352: Report chip temperature
>       net: dsa/mv88e6123_61_65: Report chip temperature
>       net: dsa: Add support for switch EEPROM access
>       net: dsa/mv88e6352: Implement EEPROM access functions
>       net: dsa: Add support for reading switch registers with ethtool
>       net: dsa/mv88e6123_61_65: Add support for reading switch registers
>       net: dsa/mv88e6352: Add support for reading switch registers
>       net: dsa: Provide additional RMON statistics
>
>  MAINTAINERS                       |   5 +
>  drivers/net/dsa/Kconfig           |   9 +
>  drivers/net/dsa/Makefile          |   3 +
>  drivers/net/dsa/mv88e6060.c       |   5 +-
>  drivers/net/dsa/mv88e6123_61_65.c |  68 +++-
>  drivers/net/dsa/mv88e6131.c       |  12 +-
>  drivers/net/dsa/mv88e6352.c       | 789 ++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx.c       |  53 ++-
>  drivers/net/dsa/mv88e6xxx.h       |  15 +
>  include/net/dsa.h                 |  20 +
>  net/dsa/dsa.c                     | 111 ++++++
>  net/dsa/slave.c                   |  60 +++
>  net/dsa/tag_dsa.c                 |   2 -
>  net/dsa/tag_edsa.c                |   2 -
>  net/dsa/tag_trailer.c             |   2 -
>  15 files changed, 1138 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/net/dsa/mv88e6352.c



-- 
Florian

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/22/2014 09:37 PM, Florian Fainelli wrote:
> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>> Some Marvell switches provide chip temperature data.
>> Add support for reporting it to the dsa infrastructure.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
> [snip]
>
>> +/* hwmon support ************************************************************/
>> +
>> +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
>
> IS_ENABLED(CONFIG_HWMON)?
>

Hi Florian,

unfortunately, that won't work; I had it initially and got a nice error message
from Fengguang's build test bot.

Problem is that hwmon can be built as module and the dsa subsystem can be built
into the kernel. In that case, hwmon functionality in the driver must be disabled.
The above condition covers that case. The nouveau graphics driver has the same
problem and uses the same conditional to solve it.

An alternative would be to select HWMON in the NET_DSA Kconfig entry; the
Broadcom Tigon3 driver does that. I just don't know if that is a good idea.
I am open to suggestions.

>> +
>> +static ssize_t temp1_input_show(struct device *dev,
>> +                               struct device_attribute *attr, char *buf)
>> +{
>> +       struct dsa_switch *ds = dev_get_drvdata(dev);
>> +       int temp, ret;
>> +
>> +       ret = ds->drv->get_temp(ds, &temp);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return sprintf(buf, "%d\n", temp * 1000);
>> +}
>> +static DEVICE_ATTR_RO(temp1_input);
>
> You probably want the number of temperature sensors to come from the
> switch driver, and support arbitrary number of temperature sensors?

In that case we would need the number of sensors, pass a sensor index,
and create a dynamic number of attributes. The code would get much more
complex without real benefit unless there is such a chip - and if there is,
we can still change the code at that time. I would prefer to keep it simple
for now.

Thanks,
Guenter

Re: [PATCH 11/14] net: dsa: Add support for reading switch registers with ethtool

[Guenter Roeck]

On 10/22/2014 09:40 PM, Florian Fainelli wrote:
> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>> Add support for reading switch registers with 'ethtool -d'.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>
> [snip]
>
>>
>> +static int dsa_slave_get_regs_len(struct net_device *dev)
>> +{
>> +       struct dsa_slave_priv *p = netdev_priv(dev);
>> +       struct dsa_switch *ds = p->parent;
>> +
>> +       if (ds->drv->get_regs_len != NULL)
>> +               return ds->drv->get_regs_len(ds, p->port);
>
> Most of the checks in this file are just:
>
> if (ds->drv->callback)
>       return ds->drv->callback(...)
>
Hi Florian,

To be fair, that wasn't the case when I wrote the code ;-).
No problem, I'll do the same.

>> +
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static void
>> +dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p)
>> +{
>> +       struct dsa_slave_priv *p = netdev_priv(dev);
>> +       struct dsa_switch *ds = p->parent;
>> +
>> +       ds->drv->get_regs(ds, p->port, regs, _p);
>
> We need to check that the driver does implement this callback here as well.
>

Obviously, and embarrassing ;-).

Thanks a lot for the review!
Guenter

Re: [PATCH 00/14] net: dsa: Fixes and enhancements

[Guenter Roeck]

On 10/22/2014 09:45 PM, Florian Fainelli wrote:
> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>> Patch 01/14 addresses an annoying and unhelpful log message.
>>
>> Patches 02/14 and 03/14 are minor enhancements, adding support for
>> known switch revisions.
>>
>> Patches 04/14 and 05/14 add support for MV88E6352 and MV88E6176.
>>
>> Patch 06/14 adds support for hardware monitoring, specifically for
>> reporting the chip temperature, to the dsa subsystem.
>>
>> Patches 07/14 and 08/14 implement hardware monitoring for MV88E6352,
>> MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>>
>> Patch 09/14 adds support for EEPROM access to the DSA subsystem.
>>
>> Patch 10/14 implements EEPROM access for MV88E6352 and MV88E6176.
>>
>> Patch 11/14 adds support for reading switch registers to the DSA
>> subsystem.
>>
>> Patches 12/14 amd 13/14 implement support for reading switch registers
>> to the drivers for MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>>
>> Patch 14/14 adds support for reading additional RMON registers to the drivers
>> for  MV88E6352, MV88E6176, MV88E6123, MV88E6161, and MV88E6165.
>>
>> The series resides and was tested on top of v3.18-rc1 in a system
>> with MV88E6352. Testing in systems with 88E6131, 88E6060 and MV88E6165
>> was done earlier (I don't have access to those systems right now).
>> The series applies cleanly to net-next as of today (10/22).
>
> Besides the two nitpicking comments, this looks really good to me, thanks!
>
Thanks a lot for the quick review!

I'll wait a couple of days for additional feedback before I resubmit.

Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Richard Cochran]

On Wed, Oct 22, 2014 at 10:06:41PM -0700, Guenter Roeck wrote:
> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
> >You probably want the number of temperature sensors to come from the
> >switch driver, and support arbitrary number of temperature sensors?
> 
> In that case we would need the number of sensors, pass a sensor index,
> and create a dynamic number of attributes. The code would get much more
> complex without real benefit unless there is such a chip - and if there is,
> we can still change the code at that time. I would prefer to keep it simple
> for now.

Better to do it correctly, right from the start. There *will* be such
a chip one day, and the person wanting to add it will appreciate the
solid foundation (even if that person ends up being you ;).

Thanks,
Richard

Re: [PATCH 02/14] net: dsa: Report known silicon revisions for Marvell 88E6060

[Sergei Shtylyov]

Hello.

On 10/23/2014 8:03 AM, Guenter Roeck wrote:

> Report known silicon revisions when probing Marvell 88E6060 switches.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/net/dsa/mv88e6060.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> index 05b0ca3..c29aebe 100644
> --- a/drivers/net/dsa/mv88e6060.c
> +++ b/drivers/net/dsa/mv88e6060.c
> @@ -69,8 +69,11 @@ static char *mv88e6060_probe(struct device *host_dev, int sw_addr)
>
>   	ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
>   	if (ret >= 0) {
> -		ret &= 0xfff0;
>   		if (ret == 0x0600)
> +			return "Marvell 88E6060 (A0)";
> +		if (ret == 0x0601 || ret == 0x0602)

    *else* *if*.

> +			return "Marvell 88E6060 (B0)";
> +		if ((ret & 0xfff0) == 0x0600)

    Likewise?

>   			return "Marvell 88E6060";
>   	}

WBR, Sergei

Re: [PATCH 02/14] net: dsa: Report known silicon revisions for Marvell 88E6060

[Guenter Roeck]

On 10/23/2014 05:51 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 10/23/2014 8:03 AM, Guenter Roeck wrote:
>
>> Report known silicon revisions when probing Marvell 88E6060 switches.
>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/net/dsa/mv88e6060.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>
>> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
>> index 05b0ca3..c29aebe 100644
>> --- a/drivers/net/dsa/mv88e6060.c
>> +++ b/drivers/net/dsa/mv88e6060.c
>> @@ -69,8 +69,11 @@ static char *mv88e6060_probe(struct device *host_dev, int sw_addr)
>>
>>       ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
>>       if (ret >= 0) {
>> -        ret &= 0xfff0;
>>           if (ret == 0x0600)
>> +            return "Marvell 88E6060 (A0)";
>> +        if (ret == 0x0601 || ret == 0x0602)
>
>     *else* *if*.
>
>> +            return "Marvell 88E6060 (B0)";
>> +        if ((ret & 0xfff0) == 0x0600)
>
>     Likewise?
>

The if case returns, so the else would just introduce an unnecessary
additional level of indentation. I think nowadays even checkpatch
complains about an unnecessary else after return.

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/23/2014 01:24 AM, Richard Cochran wrote:
> On Wed, Oct 22, 2014 at 10:06:41PM -0700, Guenter Roeck wrote:
>> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
>>> You probably want the number of temperature sensors to come from the
>>> switch driver, and support arbitrary number of temperature sensors?
>>
>> In that case we would need the number of sensors, pass a sensor index,
>> and create a dynamic number of attributes. The code would get much more
>> complex without real benefit unless there is such a chip - and if there is,
>> we can still change the code at that time. I would prefer to keep it simple
>> for now.
>
> Better to do it correctly, right from the start. There *will* be such
> a chip one day, and the person wanting to add it will appreciate the
> solid foundation (even if that person ends up being you ;).
>

That is really a matter of opinion; others say one should only introduce
complex infrastructure into the kernel if and when needed. I don't mind
changing the code to anticipate multiple sensors, but as I said it would
be more complex, obviously I would only have limited means to test it,
and, yes, by nature I tend to be one of the people who prefer to keep
things simple. Before I jump into doing this, I would prefer to get
guidance from the maintainer. David ?

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

> >>+static DEVICE_ATTR_RO(temp1_input);
> >
> >You probably want the number of temperature sensors to come from the
> >switch driver, and support arbitrary number of temperature sensors?
> 
> In that case we would need the number of sensors, pass a sensor index,
> and create a dynamic number of attributes. The code would get much more
> complex without real benefit unless there is such a chip 

We are two different use cases here:

One switch chip with multiple temperature sensors
Multiple switch chips, each with its own temperature sensor.

I don't know of any hardware using either of these uses cases, but
they could appear in the future.

If we are not doing the generic implementation now, how about avoiding
an ABI break in the future, by hard coding the sensors with .0.0 on
the end?

    Andrew

Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM access functions

[Andrew Lunn]

On Wed, Oct 22, 2014 at 09:03:18PM -0700, Guenter Roeck wrote:
> MV88E6352 supports read and write access to its configuration eeprom.

Hi Guenter

I don't have the datasheet for the MV88E6352. Is the EEPROM built in,
or external on an i2c bus?

> +static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
> +{
> +	return 0x200;
> +}

How do you handle the case of it being external and not populated.
Would it not be better to try to detect it here, and return 0 if it
does not exist?

      Andrew

Re: [PATCH 02/14] net: dsa: Report known silicon revisions for Marvell 88E6060

[Sergei Shtylyov]

On 10/23/2014 05:20 PM, Guenter Roeck wrote:

>>> Report known silicon revisions when probing Marvell 88E6060 switches.

>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   drivers/net/dsa/mv88e6060.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)

>>> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
>>> index 05b0ca3..c29aebe 100644
>>> --- a/drivers/net/dsa/mv88e6060.c
>>> +++ b/drivers/net/dsa/mv88e6060.c
>>> @@ -69,8 +69,11 @@ static char *mv88e6060_probe(struct device *host_dev,
>>> int sw_addr)
>>>
>>>       ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
>>>       if (ret >= 0) {
>>> -        ret &= 0xfff0;
>>>           if (ret == 0x0600)
>>> +            return "Marvell 88E6060 (A0)";
>>> +        if (ret == 0x0601 || ret == 0x0602)

>>     *else* *if*.

>>> +            return "Marvell 88E6060 (B0)";
>>> +        if ((ret & 0xfff0) == 0x0600)

>>     Likewise?

> The if case returns, so the else would just introduce an unnecessary
> additional level of indentation.

    Not really.

> I think nowadays even checkpatch
> complains about an unnecessary else after return.

    You're right about the *return* though. I should have stayed silent.

> Thanks,
> Guenter

WBR, Sergei

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote:
> > >>+static DEVICE_ATTR_RO(temp1_input);
> > >
> > >You probably want the number of temperature sensors to come from the
> > >switch driver, and support arbitrary number of temperature sensors?
> > 
> > In that case we would need the number of sensors, pass a sensor index,
> > and create a dynamic number of attributes. The code would get much more
> > complex without real benefit unless there is such a chip 
> 
> We are two different use cases here:
> 
> One switch chip with multiple temperature sensors

I understand this case. However, quite frankly, I consider this quite
unlikely to happen.

> Multiple switch chips, each with its own temperature sensor.
> 
I don't really see the problem. My understanding is that each of those
switch chips will instantiate itself, dsa_switch_setup will be called,
which will create a new hwmon device with its own sensors. That is
how all other hwmon devices do it, and it works just fine.
Why would that approach not work for switches in the dsa infrastructure ?
Am I missing something ?

If the concern or assumption is that there can not be more than one
"temp1_input" attribute, here is an example from a system with a large
number of chips with temperature sensors.

root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input
hwmon1/temp1_input   hwmon22/temp1_input  hwmon35/temp1_input
hwmon10/temp1_input  hwmon23/temp1_input  hwmon36/temp1_input
hwmon11/temp1_input  hwmon24/temp1_input  hwmon37/temp1_input
hwmon12/temp1_input  hwmon25/temp1_input  hwmon38/temp1_input
hwmon13/temp1_input  hwmon26/temp1_input  hwmon39/temp1_input
hwmon14/temp1_input  hwmon27/temp1_input  hwmon4/temp1_input
hwmon15/temp1_input  hwmon28/temp1_input  hwmon40/temp1_input
hwmon16/temp1_input  hwmon29/temp1_input  hwmon5/temp1_input
hwmon17/temp1_input  hwmon3/temp1_input   hwmon6/temp1_input
hwmon18/temp1_input  hwmon30/temp1_input  hwmon7/temp1_input
hwmon19/temp1_input  hwmon31/temp1_input  hwmon8/temp1_input
hwmon2/temp1_input   hwmon32/temp1_input  hwmon9/temp1_input
hwmon20/temp1_input  hwmon33/temp1_input
hwmon21/temp1_input  hwmon34/temp1_input

There are 6xDS1625, 11xDS1721, 1xLM95234, 4xMAX6697, and 17xLTC2978
in this system if I counted correctly. That doesn't mean that the
drivers need to do anything special for their multiple instances.
Also, the "name" of each instance does not have to be unique.
The "name" reflects the driver name, not its instance.

root@evo-xb49:/sys/class/hwmon# grep . */name | grep max
hwmon20/name:max6697
hwmon21/name:max6697
hwmon22/name:max6697
hwmon23/name:max6697

> I don't know of any hardware using either of these uses cases, but
> they could appear in the future.
> 
> If we are not doing the generic implementation now, how about avoiding
> an ABI break in the future, by hard coding the sensors with .0.0 on
> the end?

I am a little lost. What would that be for, and what would the ABI breakage
be ?

Thanks,
Guenter

Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM access functions

[Guenter Roeck]

On Thu, Oct 23, 2014 at 03:54:12PM +0200, Andrew Lunn wrote:
> On Wed, Oct 22, 2014 at 09:03:18PM -0700, Guenter Roeck wrote:
> > MV88E6352 supports read and write access to its configuration eeprom.
> 
> Hi Guenter
> 
> I don't have the datasheet for the MV88E6352. Is the EEPROM built in,
> or external on an i2c bus?
> 
External.

> > +static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
> > +{
> > +	return 0x200;
> > +}
> 
> How do you handle the case of it being external and not populated.
> Would it not be better to try to detect it here, and return 0 if it
> does not exist?
> 
Makes sense, if it is detectable that it the EEPROM not there. Browsing
through the datasheet, I don't see how I could detect it; there does not
seem to be a status bit indicating if the EEPROM is there or not. There
are sw_mode pins which determine if the eeprom should be loaded after
reset or not, but I don't see anything in the register set which would
tell me.

Chris, do you know if there is a means to detect if the EEPROM is present
on the MV88E6352 ?

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

On Thu, Oct 23, 2014 at 09:27:55AM -0700, Guenter Roeck wrote:
> On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote:
> > > >>+static DEVICE_ATTR_RO(temp1_input);
> > > >
> > > >You probably want the number of temperature sensors to come from the
> > > >switch driver, and support arbitrary number of temperature sensors?
> > > 
> > > In that case we would need the number of sensors, pass a sensor index,
> > > and create a dynamic number of attributes. The code would get much more
> > > complex without real benefit unless there is such a chip 
> > 
> > We are two different use cases here:
> > 
> > One switch chip with multiple temperature sensors
> 
> I understand this case. However, quite frankly, I consider this quite
> unlikely to happen.
> 
> > Multiple switch chips, each with its own temperature sensor.
> > 
> I don't really see the problem. My understanding is that each of those
> switch chips will instantiate itself, dsa_switch_setup will be called,
> which will create a new hwmon device with its own sensors. That is
> how all other hwmon devices do it, and it works just fine.
> Why would that approach not work for switches in the dsa infrastructure ?
> Am I missing something ?
> 
> If the concern or assumption is that there can not be more than one
> "temp1_input" attribute, here is an example from a system with a large
> number of chips with temperature sensors.
> 
> root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input
> hwmon1/temp1_input   hwmon22/temp1_input  hwmon35/temp1_input
> hwmon10/temp1_input  hwmon23/temp1_input  hwmon36/temp1_input
> hwmon11/temp1_input  hwmon24/temp1_input  hwmon37/temp1_input
> hwmon12/temp1_input  hwmon25/temp1_input  hwmon38/temp1_input
> hwmon13/temp1_input  hwmon26/temp1_input  hwmon39/temp1_input
> hwmon14/temp1_input  hwmon27/temp1_input  hwmon4/temp1_input
> hwmon15/temp1_input  hwmon28/temp1_input  hwmon40/temp1_input
> hwmon16/temp1_input  hwmon29/temp1_input  hwmon5/temp1_input
> hwmon17/temp1_input  hwmon3/temp1_input   hwmon6/temp1_input
> hwmon18/temp1_input  hwmon30/temp1_input  hwmon7/temp1_input
> hwmon19/temp1_input  hwmon31/temp1_input  hwmon8/temp1_input
> hwmon2/temp1_input   hwmon32/temp1_input  hwmon9/temp1_input
> hwmon20/temp1_input  hwmon33/temp1_input
> hwmon21/temp1_input  hwmon34/temp1_input

So are you saying it is impossible to map a hwmon device to a physical
sensor? I can know there is a hotspot somewhere in my machine, but it
is impossible to figure where that hot spot is using hwmon?

> > If we are not doing the generic implementation now, how about avoiding
> > an ABI break in the future, by hard coding the sensors with .0.0 on
> > the end?
> 
> I am a little lost. What would that be for, and what would the ABI breakage
> be ?

I assumed you would want to be able to map a temperature sensor to a
switch package. You want a unique identifier, maybe its hwman name? So
name "dsa.0.0" would be the temperature sensor 0 on switch 0. If we
don't name them like this now, whenever somebody does add support for
this will cause that name to change and we have an ABI break.


     Andrew

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On Thu, Oct 23, 2014 at 06:54:59PM +0200, Andrew Lunn wrote:
> On Thu, Oct 23, 2014 at 09:27:55AM -0700, Guenter Roeck wrote:
> > On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote:
> > > > >>+static DEVICE_ATTR_RO(temp1_input);
> > > > >
> > > > >You probably want the number of temperature sensors to come from the
> > > > >switch driver, and support arbitrary number of temperature sensors?
> > > > 
> > > > In that case we would need the number of sensors, pass a sensor index,
> > > > and create a dynamic number of attributes. The code would get much more
> > > > complex without real benefit unless there is such a chip 
> > > 
> > > We are two different use cases here:
> > > 
> > > One switch chip with multiple temperature sensors
> > 
> > I understand this case. However, quite frankly, I consider this quite
> > unlikely to happen.
> > 
> > > Multiple switch chips, each with its own temperature sensor.
> > > 
> > I don't really see the problem. My understanding is that each of those
> > switch chips will instantiate itself, dsa_switch_setup will be called,
> > which will create a new hwmon device with its own sensors. That is
> > how all other hwmon devices do it, and it works just fine.
> > Why would that approach not work for switches in the dsa infrastructure ?
> > Am I missing something ?
> > 
> > If the concern or assumption is that there can not be more than one
> > "temp1_input" attribute, here is an example from a system with a large
> > number of chips with temperature sensors.
> > 
> > root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input
> > hwmon1/temp1_input   hwmon22/temp1_input  hwmon35/temp1_input
> > hwmon10/temp1_input  hwmon23/temp1_input  hwmon36/temp1_input
> > hwmon11/temp1_input  hwmon24/temp1_input  hwmon37/temp1_input
> > hwmon12/temp1_input  hwmon25/temp1_input  hwmon38/temp1_input
> > hwmon13/temp1_input  hwmon26/temp1_input  hwmon39/temp1_input
> > hwmon14/temp1_input  hwmon27/temp1_input  hwmon4/temp1_input
> > hwmon15/temp1_input  hwmon28/temp1_input  hwmon40/temp1_input
> > hwmon16/temp1_input  hwmon29/temp1_input  hwmon5/temp1_input
> > hwmon17/temp1_input  hwmon3/temp1_input   hwmon6/temp1_input
> > hwmon18/temp1_input  hwmon30/temp1_input  hwmon7/temp1_input
> > hwmon19/temp1_input  hwmon31/temp1_input  hwmon8/temp1_input
> > hwmon2/temp1_input   hwmon32/temp1_input  hwmon9/temp1_input
> > hwmon20/temp1_input  hwmon33/temp1_input
> > hwmon21/temp1_input  hwmon34/temp1_input
> 
> So are you saying it is impossible to map a hwmon device to a physical
> sensor? I can know there is a hotspot somewhere in my machine, but it
> is impossible to figure where that hot spot is using hwmon?
> 
No, I am not saying that. The hwmon device's parent device will tell,
which is how it works for all other hwmon devices.

> > > If we are not doing the generic implementation now, how about avoiding
> > > an ABI break in the future, by hard coding the sensors with .0.0 on
> > > the end?
> > 
> > I am a little lost. What would that be for, and what would the ABI breakage
> > be ?
> 
> I assumed you would want to be able to map a temperature sensor to a
> switch package. You want a unique identifier, maybe its hwman name? So
> name "dsa.0.0" would be the temperature sensor 0 on switch 0. If we
> don't name them like this now, whenever somebody does add support for
> this will cause that name to change and we have an ABI break.
> 
Not really. Again, the parent device provides that information. libsensors,
which is the preferred way of accessing sensors information from user space,
provides the parent device instance as part of the logical sensor device
name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001,
and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001,
and so on. I don't see how this would add any value.

Also, temperature sensor 2 on switch 1 would be named temp2_input. There would
not be a sepate hwmon device for the same chip.

This is all quite similar to, say, the CPU temperature, which is reported 
by the sensors command as

coretemp-isa-0000
Adapter: ISA adapter
Physical id 0:  +28.0°C  (high = +80.0°C, crit = +100.0°C)
Core 0:         +28.0°C  (high = +80.0°C, crit = +100.0°C)
Core 1:         +24.0°C  (high = +80.0°C, crit = +100.0°C)
Core 2:         +24.0°C  (high = +80.0°C, crit = +100.0°C)
Core 3:         +26.0°C  (high = +80.0°C, crit = +100.0°C)

on my system at home.

The raw data in this case is

name:coretemp
temp1_crit:100000
temp1_crit_alarm:0
temp1_input:31000
temp1_label:Physical id 0
temp1_max:80000
temp2_crit:100000
temp2_crit_alarm:0
temp2_input:31000
temp2_label:Core 0
temp2_max:80000
temp3_crit:100000
temp3_crit_alarm:0
temp3_input:26000
temp3_label:Core 1
temp3_max:80000
temp4_crit:100000
temp4_crit_alarm:0
temp4_input:24000
temp4_label:Core 2
temp4_max:80000
temp5_crit:100000
temp5_crit_alarm:0
temp5_input:27000
temp5_label:Core 3
temp5_max:80000

In this case, the parent device symlink is

device -> ../../../coretemp.0

A second CPU in a multi-CPU server system would have pretty much
the same information in its hwmon device directory, except that
the parent device would point to coretemp.1, and the sensors command
would display "coretemp-isa-0001". The "name" attribute for that
second CPU's hwmon device node would still be "coretemp".

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

> No, I am not saying that. The hwmon device's parent device will tell,
> which is how it works for all other hwmon devices.

O.K, so parent is important.

> Not really. Again, the parent device provides that information. libsensors,
> which is the preferred way of accessing sensors information from user space,
> provides the parent device instance as part of the logical sensor device
> name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001,
> and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001,
> and so on. I don't see how this would add any value.

isa is the name of the ethernet device? Why is it not eth0? Most
Marvell SoCs used in WiFi Access Points have multiple ethernet
interfaces, so i would hope the parent actually identifies which
ethernet interface it is hanging off.

Now consider the example in

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/net/dsa/dsa.txt

We have two switches hanging off one ethernet interface. What will the
naming look like in this case?

The Marvell DSA tagging scheme allows you to have 16 switches hanging
off one ethernet interface. How is the naming going to work then,
especially if there is a mixture of switch chips, some with
temperature sensors, and some without?

What would really help is if each switch has a device in the linux
device model. The hwmon parent would then be the switch device. The
EEPROM would then hang off the switch device, not an interface on the
switch device, etc.

	    Andrew

RE: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM accessfunctions

[Chris Healy]

Hi Guenter,

I do not believe it is possible to know if an EEPROM is attached or not.

Chris
________________________________________
From: Guenter Roeck [linux@roeck-us.net]
Sent: Thursday, October 23, 2014 9:40 AM
To: Andrew Lunn
Cc: netdev@vger.kernel.org; David S. Miller; Florian Fainelli; linux-kernel@vger.kernel.org; Chris Healy
Subject: Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM         accessfunctions

On Thu, Oct 23, 2014 at 03:54:12PM +0200, Andrew Lunn wrote:
> On Wed, Oct 22, 2014 at 09:03:18PM -0700, Guenter Roeck wrote:
> > MV88E6352 supports read and write access to its configuration eeprom.
>
> Hi Guenter
>
> I don't have the datasheet for the MV88E6352. Is the EEPROM built in,
> or external on an i2c bus?
>
External.

> > +static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
> > +{
> > +   return 0x200;
> > +}
>
> How do you handle the case of it being external and not populated.
> Would it not be better to try to detect it here, and return 0 if it
> does not exist?
>
Makes sense, if it is detectable that it the EEPROM not there. Browsing
through the datasheet, I don't see how I could detect it; there does not
seem to be a status bit indicating if the EEPROM is there or not. There
are sw_mode pins which determine if the eeprom should be loaded after
reset or not, but I don't see anything in the register set which would
tell me.

Chris, do you know if there is a means to detect if the EEPROM is present
on the MV88E6352 ?

Thanks,
Guenter

________________________________


This email and any files transmitted with it are confidential & proprietary to Systems and Software Enterprises, LLC. This information is intended solely for the use of the individual or entity to which it is addressed. Access or transmittal of the information contained in this e-mail, in full or in part, to any other organization or persons is not authorized.

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote:
> > No, I am not saying that. The hwmon device's parent device will tell,
> > which is how it works for all other hwmon devices.
> 
> O.K, so parent is important.
> 
> > Not really. Again, the parent device provides that information. libsensors,
> > which is the preferred way of accessing sensors information from user space,
> > provides the parent device instance as part of the logical sensor device
> > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001,
> > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001,
> > and so on. I don't see how this would add any value.
> 
> isa is the name of the ethernet device? Why is it not eth0? Most

isa is just an internal name made up by libsensors, and pretty much just
indicates something like "neither i2c nor spi". It is mostly historic,
but nowadays almost part of the ABI. It is made up by user space,
based on the parent device type, not by the kernel.

> Marvell SoCs used in WiFi Access Points have multiple ethernet
> interfaces, so i would hope the parent actually identifies which
> ethernet interface it is hanging off.
> 
> Now consider the example in
> 
> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/net/dsa/dsa.txt
> 
> We have two switches hanging off one ethernet interface. What will the > naming look like in this case?
> 
Ah, that is a good question. You are right, the parent will be the same
for both of those switches but should be unique. Hmm ...

> The Marvell DSA tagging scheme allows you to have 16 switches hanging
> off one ethernet interface. How is the naming going to work then,
> especially if there is a mixture of switch chips, some with
> temperature sensors, and some without?
> 
The ones without sensor would not create a hwmon device, so that should
not be an issue. You are right, the other use cases are more tricky.

> What would really help is if each switch has a device in the linux
> device model. The hwmon parent would then be the switch device. The
> EEPROM would then hang off the switch device, not an interface on the
> switch device, etc.

Good point. Unfortunately that is not the case.

You have convinced me that 'dsa' as hwmon attribute name is insufficient,
so let's see what we have.

- the parent network device in dst->master_netdev
- the dsa device in 'parent'
- the host device in host_dev
- the switch index in 'index'

First question is what should be the parent device. We have three to
choose from. Should it be the parent network device or the dsa device ?
The dsa device seems like a better choice to me, but I am open to
suggestions. A problem with choosing the network device as parent
may be that this device could by itself have a temperature sensor
(some Intel and broadcom Ethernet chips have that), which could
cause confusion.

Second is what to choose for the hwmon device 'name' attribute.
'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X,
where X is the switch index ? At this point I am open to suggestions.
Note that we can not use the name returned from the switch probe
functions as it may contain spaces and other invalid characters.
Including the ethernet device name (eg as in eth0_dsa_0) may also be
problematic if it can contain '-', which is illegal for hwmon devices.

Thanks,
Guenter

Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM accessfunctions

[Florian Fainelli]

On 10/23/2014 11:41 AM, Chris Healy wrote:
> Hi Guenter,
> 
> I do not believe it is possible to know if an EEPROM is attached or not.

If we cannot do this, how about a DT/platform data set of properties
that describes the EEPROM when present?

> 
> Chris
> ________________________________________
> From: Guenter Roeck [linux@roeck-us.net]
> Sent: Thursday, October 23, 2014 9:40 AM
> To: Andrew Lunn
> Cc: netdev@vger.kernel.org; David S. Miller; Florian Fainelli; linux-kernel@vger.kernel.org; Chris Healy
> Subject: Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM         accessfunctions
> 
> On Thu, Oct 23, 2014 at 03:54:12PM +0200, Andrew Lunn wrote:
>> On Wed, Oct 22, 2014 at 09:03:18PM -0700, Guenter Roeck wrote:
>>> MV88E6352 supports read and write access to its configuration eeprom.
>>
>> Hi Guenter
>>
>> I don't have the datasheet for the MV88E6352. Is the EEPROM built in,
>> or external on an i2c bus?
>>
> External.
> 
>>> +static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
>>> +{
>>> +   return 0x200;
>>> +}
>>
>> How do you handle the case of it being external and not populated.
>> Would it not be better to try to detect it here, and return 0 if it
>> does not exist?
>>
> Makes sense, if it is detectable that it the EEPROM not there. Browsing
> through the datasheet, I don't see how I could detect it; there does not
> seem to be a status bit indicating if the EEPROM is there or not. There
> are sw_mode pins which determine if the eeprom should be loaded after
> reset or not, but I don't see anything in the register set which would
> tell me.
> 
> Chris, do you know if there is a means to detect if the EEPROM is present
> on the MV88E6352 ?
> 
> Thanks,
> Guenter
> 
> ________________________________
> 
> 
> This email and any files transmitted with it are confidential & proprietary to Systems and Software Enterprises, LLC. This information is intended solely for the use of the individual or entity to which it is addressed. Access or transmittal of the information contained in this e-mail, in full or in part, to any other organization or persons is not authorized.
> 

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

On Thu, Oct 23, 2014 at 11:43:22AM -0700, Guenter Roeck wrote:
> On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote:
> > > No, I am not saying that. The hwmon device's parent device will tell,
> > > which is how it works for all other hwmon devices.
> > 
> > O.K, so parent is important.
> > 
> > > Not really. Again, the parent device provides that information. libsensors,
> > > which is the preferred way of accessing sensors information from user space,
> > > provides the parent device instance as part of the logical sensor device
> > > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001,
> > > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001,
> > > and so on. I don't see how this would add any value.
> > 
> > isa is the name of the ethernet device? Why is it not eth0? Most
> 
> isa is just an internal name made up by libsensors, and pretty much just
> indicates something like "neither i2c nor spi". It is mostly historic,
> but nowadays almost part of the ABI. It is made up by user space,
> based on the parent device type, not by the kernel.

So for DSA, since it is not i2c or spi, the parent is actually
irrelevant, because libsensor ignores it and says "IsSomethingAlien".
So the name alone needs to identify it.

 > You have convinced me that 'dsa' as hwmon attribute name is insufficient,
> so let's see what we have.
> 
> - the parent network device in dst->master_netdev
> - the dsa device in 'parent'
> - the host device in host_dev
> - the switch index in 'index'
> 
> First question is what should be the parent device.

Does it even matter, given the observation above?  I would probably go
for dsa, since as you said, the Ethernet device may have a temperature
sensor of its own. DSA is a virtual device...

> Second is what to choose for the hwmon device 'name' attribute.
> 'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X,
> where X is the switch index ? At this point I am open to suggestions.
> Note that we can not use the name returned from the switch probe
> functions as it may contain spaces and other invalid characters.
> Including the ethernet device name (eg as in eth0_dsa_0) may also be
> problematic if it can contain '-', which is illegal for hwmon devices.

Does hwmon offer a function to sanitise a string?

The switch index definitely should be used and i would probably
combine that with a sanitised version of the ethernet device name and
"dsa".

	Andrew

Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM accessfunctions

[Guenter Roeck]

On 10/23/2014 11:55 AM, Florian Fainelli wrote:
> On 10/23/2014 11:41 AM, Chris Healy wrote:
>> Hi Guenter,
>>
>> I do not believe it is possible to know if an EEPROM is attached or not.
>
> If we cannot do this, how about a DT/platform data set of properties
> that describes the EEPROM when present?
>
Yes, I think that is a good idea.

Thanks,
Guenter

>>
>> Chris
>> ________________________________________
>> From: Guenter Roeck [linux@roeck-us.net]
>> Sent: Thursday, October 23, 2014 9:40 AM
>> To: Andrew Lunn
>> Cc: netdev@vger.kernel.org; David S. Miller; Florian Fainelli; linux-kernel@vger.kernel.org; Chris Healy
>> Subject: Re: [PATCH 10/14] net: dsa/mv88e6352: Implement EEPROM         accessfunctions
>>
>> On Thu, Oct 23, 2014 at 03:54:12PM +0200, Andrew Lunn wrote:
>>> On Wed, Oct 22, 2014 at 09:03:18PM -0700, Guenter Roeck wrote:
>>>> MV88E6352 supports read and write access to its configuration eeprom.
>>>
>>> Hi Guenter
>>>
>>> I don't have the datasheet for the MV88E6352. Is the EEPROM built in,
>>> or external on an i2c bus?
>>>
>> External.
>>
>>>> +static int mv88e6352_get_eeprom_len(struct dsa_switch *ds)
>>>> +{
>>>> +   return 0x200;
>>>> +}
>>>
>>> How do you handle the case of it being external and not populated.
>>> Would it not be better to try to detect it here, and return 0 if it
>>> does not exist?
>>>
>> Makes sense, if it is detectable that it the EEPROM not there. Browsing
>> through the datasheet, I don't see how I could detect it; there does not
>> seem to be a status bit indicating if the EEPROM is there or not. There
>> are sw_mode pins which determine if the eeprom should be loaded after
>> reset or not, but I don't see anything in the register set which would
>> tell me.
>>
>> Chris, do you know if there is a means to detect if the EEPROM is present
>> on the MV88E6352 ?
>>
>> Thanks,
>> Guenter
>>
>> ________________________________
>>
>>
>> This email and any files transmitted with it are confidential & proprietary to Systems and Software Enterprises, LLC. This information is intended solely for the use of the individual or entity to which it is addressed. Access or transmittal of the information contained in this e-mail, in full or in part, to any other organization or persons is not authorized.
>>
>
>

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 22 Oct 2014 22:06:41 -0700

> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>>> Some Marvell switches provide chip temperature data.
>>> Add support for reporting it to the dsa infrastructure.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>> [snip]
>>
>>> +/* hwmon support
>>> ************************************************************/
>>> +
>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) &&
>>> defined(CONFIG_HWMON_MODULE))
>>
>> IS_ENABLED(CONFIG_HWMON)?
>>
> 
> Hi Florian,
> 
> unfortunately, that won't work; I had it initially and got a nice
> error message
> from Fengguang's build test bot.

Then the Kconfig dependencies are broken.

Fix Kconfig to only allow legal combinations.

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/23/2014 10:03 PM, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 22 Oct 2014 22:06:41 -0700
>
>> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
>>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>>>> Some Marvell switches provide chip temperature data.
>>>> Add support for reporting it to the dsa infrastructure.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>> [snip]
>>>
>>>> +/* hwmon support
>>>> ************************************************************/
>>>> +
>>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) &&
>>>> defined(CONFIG_HWMON_MODULE))
>>>
>>> IS_ENABLED(CONFIG_HWMON)?
>>>
>>
>> Hi Florian,
>>
>> unfortunately, that won't work; I had it initially and got a nice
>> error message
>> from Fengguang's build test bot.
>
> Then the Kconfig dependencies are broken.
>
> Fix Kconfig to only allow legal combinations.
>

I see two options for that:

- Add
	select HWMON
   to the NET_DSA Kconfig entry.
   Example is Broadcom TIGON3 driver.

- Add a DSA_HWMON Kconfig entry to define the dependencies and
   to let the user select if the functionality should be enabled.
   Example is Intel IGB driver.

Any preference from your side ? If no, I'll go with the latter.

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/24/2014 05:52 AM, Florian Fainelli wrote:
> Le 23/10/2014 22:40, Guenter Roeck a écrit :
>> On 10/23/2014 10:03 PM, David Miller wrote:
>>> From: Guenter Roeck <linux@roeck-us.net>
>>> Date: Wed, 22 Oct 2014 22:06:41 -0700
>>>
>>>> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
>>>>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>>>>>> Some Marvell switches provide chip temperature data.
>>>>>> Add support for reporting it to the dsa infrastructure.
>>>>>>
>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>> ---
>>>>> [snip]
>>>>>
>>>>>> +/* hwmon support
>>>>>> ************************************************************/
>>>>>> +
>>>>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) &&
>>>>>> defined(CONFIG_HWMON_MODULE))
>>>>>
>>>>> IS_ENABLED(CONFIG_HWMON)?
>>>>>
>>>>
>>>> Hi Florian,
>>>>
>>>> unfortunately, that won't work; I had it initially and got a nice
>>>> error message
>>>> from Fengguang's build test bot.
>>>
>>> Then the Kconfig dependencies are broken.
>>>
>>> Fix Kconfig to only allow legal combinations.
>>>
>>
>> I see two options for that:
>>
>> - Add
>>      select HWMON
>>    to the NET_DSA Kconfig entry.
>>    Example is Broadcom TIGON3 driver.
>>
>> - Add a DSA_HWMON Kconfig entry to define the dependencies and
>>    to let the user select if the functionality should be enabled.
>>    Example is Intel IGB driver.
>>
>> Any preference from your side ? If no, I'll go with the latter.
>
> I would prefer DSA_HWMON personaly, though no strong feelings.

That is what I ended up implementing. NET_DSA_HWMON, actually, for consistency.

Since this is the most debated patch in this patch set, how about you drop it from your v2 and we sort this one out separately?

We can do that if there are still issues in v2.

Thanks,
Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Thu, 23 Oct 2014 22:40:59 -0700

> I see two options for that:
> 
> - Add
> 	select HWMON
>   to the NET_DSA Kconfig entry.
>   Example is Broadcom TIGON3 driver.
> 
> - Add a DSA_HWMON Kconfig entry to define the dependencies and
>   to let the user select if the functionality should be enabled.
>   Example is Intel IGB driver.
> 
> Any preference from your side ? If no, I'll go with the latter.

Probably the latter is better, select can get you into trouble.

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Florian Fainelli]

Le 23/10/2014 22:40, Guenter Roeck a écrit :
> On 10/23/2014 10:03 PM, David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Wed, 22 Oct 2014 22:06:41 -0700
>>
>>> On 10/22/2014 09:37 PM, Florian Fainelli wrote:
>>>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
>>>>> Some Marvell switches provide chip temperature data.
>>>>> Add support for reporting it to the dsa infrastructure.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>> ---
>>>> [snip]
>>>>
>>>>> +/* hwmon support
>>>>> ************************************************************/
>>>>> +
>>>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) &&
>>>>> defined(CONFIG_HWMON_MODULE))
>>>>
>>>> IS_ENABLED(CONFIG_HWMON)?
>>>>
>>>
>>> Hi Florian,
>>>
>>> unfortunately, that won't work; I had it initially and got a nice
>>> error message
>>> from Fengguang's build test bot.
>>
>> Then the Kconfig dependencies are broken.
>>
>> Fix Kconfig to only allow legal combinations.
>>
>
> I see two options for that:
>
> - Add
>      select HWMON
>    to the NET_DSA Kconfig entry.
>    Example is Broadcom TIGON3 driver.
>
> - Add a DSA_HWMON Kconfig entry to define the dependencies and
>    to let the user select if the functionality should be enabled.
>    Example is Intel IGB driver.
>
> Any preference from your side ? If no, I'll go with the latter.

I would prefer DSA_HWMON personaly, though no strong feelings. Since 
this is the most debated patch in this patch set, how about you drop it 
from your v2 and we sort this one out separately?
--
Florian

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/23/2014 12:55 PM, Andrew Lunn wrote:
[ ... ]
>
> Does hwmon offer a function to sanitise a string?
>
No, that wasn't necessary so far. The 'name' string is a constant string
provided by the driver.

> The switch index definitely should be used and i would probably
> combine that with a sanitised version of the ethernet device name and
> "dsa".
>

How is this ?

em1dsa0-isa-0000
Adapter: ISA adapter
temp1:        +38.0°C  (high = +100.0°C)

This is the sanitized network device name + 'dsa' + index.

Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On Thu, Oct 23, 2014 at 09:55:26PM +0200, Andrew Lunn wrote:
> On Thu, Oct 23, 2014 at 11:43:22AM -0700, Guenter Roeck wrote:
> > On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote:
> > > > No, I am not saying that. The hwmon device's parent device will tell,
> > > > which is how it works for all other hwmon devices.
> > > 
> > > O.K, so parent is important.
> > > 
> > > > Not really. Again, the parent device provides that information. libsensors,
> > > > which is the preferred way of accessing sensors information from user space,
> > > > provides the parent device instance as part of the logical sensor device
> > > > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001,
> > > > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001,
> > > > and so on. I don't see how this would add any value.
> > > 
> > > isa is the name of the ethernet device? Why is it not eth0? Most
> > 
> > isa is just an internal name made up by libsensors, and pretty much just
> > indicates something like "neither i2c nor spi". It is mostly historic,
> > but nowadays almost part of the ABI. It is made up by user space,
> > based on the parent device type, not by the kernel.
> 
> So for DSA, since it is not i2c or spi, the parent is actually
> irrelevant, because libsensor ignores it and says "IsSomethingAlien".
> So the name alone needs to identify it.
> 
>  > You have convinced me that 'dsa' as hwmon attribute name is insufficient,
> > so let's see what we have.
> > 
> > - the parent network device in dst->master_netdev
> > - the dsa device in 'parent'
> > - the host device in host_dev
> > - the switch index in 'index'
> > 
> > First question is what should be the parent device.
> 
> Does it even matter, given the observation above?  I would probably go
> for dsa, since as you said, the Ethernet device may have a temperature
> sensor of its own. DSA is a virtual device...
> 
> > Second is what to choose for the hwmon device 'name' attribute.
> > 'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X,
> > where X is the switch index ? At this point I am open to suggestions.
> > Note that we can not use the name returned from the switch probe
> > functions as it may contain spaces and other invalid characters.
> > Including the ethernet device name (eg as in eth0_dsa_0) may also be
> > problematic if it can contain '-', which is illegal for hwmon devices.
> 
> Does hwmon offer a function to sanitise a string?
> 
> The switch index definitely should be used and i would probably
> combine that with a sanitised version of the ethernet device name and
> "dsa".
> 
Here is another naming option:

em1dsa0-virtual-0
Adapter: Virtual device
temp1:        +52.0°C  (high = +100.0°C)

I think I'll go with that one. I get it by not specifying
a parent device when registering with the hwmon subsystem.
It is kind of similar to the thermal sensor data reported
through acpi.

acpitz-virtual-0
Adapter: Virtual device
temp1:        +52.0°C  (crit = +111.0°C)
temp2:        +52.0°C  (crit = +111.0°C)

Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

> Here is another naming option:
> 
> em1dsa0-virtual-0

I prefer this over isa.

However, i think there should be some sort of separator between the
network device name and dsa.

	Andrew

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Florian Fainelli]

On 10/25/14 07:01, Andrew Lunn wrote:
>> Here is another naming option:
>>
>> em1dsa0-virtual-0
> 
> I prefer this over isa.
> 
> However, i think there should be some sort of separator between the
> network device name and dsa.

Considering that network devices can be renamed, do we want it to be
included in the sensor name at all?
--
Florian

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Florian Fainelli]

On 10/25/14 07:01, Andrew Lunn wrote:
>> Here is another naming option:
>>
>> em1dsa0-virtual-0
> 
> I prefer this over isa.
> 
> However, i think there should be some sort of separator between the
> network device name and dsa.

Considering that network devices can be renamed, do we want it to be
included in the sensor name at all?
--
Florian

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Andrew Lunn]

On Sat, Oct 25, 2014 at 10:23:27AM -0700, Florian Fainelli wrote:
> On 10/25/14 07:01, Andrew Lunn wrote:
> >> Here is another naming option:
> >>
> >> em1dsa0-virtual-0
> > 
> > I prefer this over isa.
> > 
> > However, i think there should be some sort of separator between the
> > network device name and dsa.
> 
> Considering that network devices can be renamed, do we want it to be
> included in the sensor name at all?

Well, we need something which identifies the "DSA collection". In
that, you could have two or more collections, connected to different
network devices.

I once came across a PCI board with two Intel ethernet chipsets and
two 10 port Marvell switches. Each switch had one host cpu port, 3
ports interconnected to the other switch, and 6 going to the back
panel.  You would want to describe that as two separate DSA entities,
not one DSA with two switches. So the name needs something to indicate
the DSA collection.

	Andrew

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/25/2014 07:01 AM, Andrew Lunn wrote:
>> Here is another naming option:
>>
>> em1dsa0-virtual-0
>
> I prefer this over isa.
>
> However, i think there should be some sort of separator between the
> network device name and dsa.
>

Fine with me. Any preference ? Note that '-' is not permitted.

Guenter

Re: [PATCH 06/14] net: dsa: Add support for hardware monitoring

[Guenter Roeck]

On 10/25/2014 11:00 AM, Andrew Lunn wrote:
> On Sat, Oct 25, 2014 at 10:23:27AM -0700, Florian Fainelli wrote:
>> On 10/25/14 07:01, Andrew Lunn wrote:
>>>> Here is another naming option:
>>>>
>>>> em1dsa0-virtual-0
>>>
>>> I prefer this over isa.
>>>
>>> However, i think there should be some sort of separator between the
>>> network device name and dsa.
>>
>> Considering that network devices can be renamed, do we want it to be
>> included in the sensor name at all?
>
> Well, we need something which identifies the "DSA collection". In
> that, you could have two or more collections, connected to different
> network devices.
>
> I once came across a PCI board with two Intel ethernet chipsets and
> two 10 port Marvell switches. Each switch had one host cpu port, 3
> ports interconnected to the other switch, and 6 going to the back
> panel.  You would want to describe that as two separate DSA entities,
> not one DSA with two switches. So the name needs something to indicate
> the DSA collection.
>

I agree. I could use the dsa instance index, so we would have
names such as dsa0-isa-0000, dsa1-isa-0000, dsa0-isa-0001, dsa1-isa-0001,
and so on, but that looks awkward (I can't do the index with virtual
names because they don't have a parent and thus no index).

Using the ethernet interface name seems like a good idea to me.
Sure it can change, but the name is typically chosen before the
dsa device is instantiated, and if it is changed manually later
we just can not help it.

So, if people want a delimiter between network device name and dsa,
what should it be ? Unless I hear otherwise, I'll choose '_'.

Guenter