[PATCH 0/4] net: dsa: microchip: ksz8795: add Wake on LAN support

[PATCH 0/4] net: dsa: microchip: ksz8795: add Wake on LAN support

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Add WoL support for KSZ8795 family of switches. This code was tested
with a KSZ8794 chip.

Strongly based on existing KSZ9477 code but there's too many
differences, such as the indirect register access, to generalize this
code to ksz_common. Some registers names have been changed to increase
standardization between those code bases.

In addition to the device-tree addition and the actual code, there's
two more patches to correct the erratum workaround application using
the now available indirect register read and a minor correction of
existing KSZ9477 function comments.

Pieter Van Trappen (4):
  dt-bindings: net: dsa: microchip: add microchip,pme-active-high flag
  net: dsa: microchip: ksz8795: add Wake on LAN support
  net: dsa: microchip: check erratum workaround through indirect
    register read
  net: dsa: microchip: ksz9477: correct description of WoL functions

 .../bindings/net/dsa/microchip,ksz.yaml       |   5 +
 drivers/net/dsa/microchip/ksz8.h              |   5 +
 drivers/net/dsa/microchip/ksz8795.c           | 230 +++++++++++++++++-
 drivers/net/dsa/microchip/ksz8795_reg.h       |  17 +-
 drivers/net/dsa/microchip/ksz9477.c           |   8 +-
 drivers/net/dsa/microchip/ksz_common.c        |   5 +
 drivers/net/dsa/microchip/ksz_common.h        |   1 +
 7 files changed, 258 insertions(+), 13 deletions(-)


base-commit: 51835949dda3783d4639cfa74ce13a3c9829de00
-- 
2.43.0

[PATCH 1/4] dt-bindings: net: dsa: microchip: add microchip,pme-active-high flag

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Add microchip,pme-active-high property to set the PME (Power
Management Event) pin polarity for Wake on Lan interrupts.

Note that the polarity is active-low by default.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 52acc15ebcbf..c589ebc2c7be 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -51,6 +51,11 @@ properties:
       Set if the output SYNCLKO clock should be disabled. Do not mix with
       microchip,synclko-125.
 
+  microchip,pme-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates if the PME pin polarity is active-high.
+
   microchip,io-drive-strength-microamp:
     description:
       IO Pad Drive Strength
-- 
2.43.0

[PATCH 2/4] net: dsa: microchip: ksz8795: add Wake on LAN support

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Add WoL support for KSZ8795 family of switches. This code was tested
with a KSZ8794 chip.

KSZ8795 family of switches supports multiple PHY events:
- wake on Link Up
- wake on Energy Detect.
- wake on Magic Packet.
Since current UAPI can't differentiate between Link Up and Energy
Detect, map these to WAKE_PHY.

Strongly based on existing KSZ9477 code but there's too many
differences, such as the indirect register access, to generalize this
code to ksz_common. Some registers names have been changed to increase
standardization between those code bases.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 drivers/net/dsa/microchip/ksz8.h        |   5 +
 drivers/net/dsa/microchip/ksz8795.c     | 220 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz8795_reg.h |  13 +-
 drivers/net/dsa/microchip/ksz_common.c  |   5 +
 drivers/net/dsa/microchip/ksz_common.h  |   1 +
 5 files changed, 239 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ae43077e76c3..4cece61181e9 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -59,5 +59,10 @@ void ksz8_phylink_mac_link_up(struct phylink_config *config,
 			      phy_interface_t interface, int speed, int duplex,
 			      bool tx_pause, bool rx_pause);
 int ksz8_all_queues_split(struct ksz_device *dev, int queues);
+void ksz8_get_wol(struct ksz_device *dev, int port,
+		  struct ethtool_wolinfo *wol);
+int ksz8_set_wol(struct ksz_device *dev, int port,
+		 struct ethtool_wolinfo *wol);
+void ksz8_wol_pre_shutdown(struct ksz_device *dev, bool *wol_enabled);
 
 #endif
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index d27b9c36d73f..49081e9a8cb0 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -58,6 +58,26 @@ static int ksz8_ind_write8(struct ksz_device *dev, u8 table, u16 addr, u8 data)
 	return ret;
 }
 
+static int ksz8_ind_read8(struct ksz_device *dev, u8 table, u16 addr, u8 *val)
+{
+	const u16 *regs;
+	u16 ctrl_addr;
+	int ret = 0;
+
+	regs = dev->info->regs;
+
+	mutex_lock(&dev->alu_mutex);
+
+	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	if (!ret)
+		ret = ksz_read8(dev, regs[REG_IND_BYTE], val);
+
+	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
+}
+
 int ksz8_reset_switch(struct ksz_device *dev)
 {
 	if (ksz_is_ksz88x3(dev)) {
@@ -127,6 +147,200 @@ int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu)
 	return -EOPNOTSUPP;
 }
 
+/**
+ * ksz8_handle_wake_reason - Handle wake reason on a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ *
+ * This function reads the PME (Power Management Event) status register of a
+ * specified port to determine the wake reason. If there is no wake event, it
+ * returns early. Otherwise, it logs the wake reason which could be due to a
+ * "Magic Packet", "Link Up", or "Energy Detect" event. The PME status register
+ * is then cleared to acknowledge the handling of the wake event; followed by
+ * clearing the global Interrupt Status Register.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
+static int ksz8_handle_wake_reason(struct ksz_device *dev, int port)
+{
+	u8 pme_status;
+	int ret;
+
+	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, &pme_status);
+	if (ret)
+		return ret;
+
+	if (!pme_status)
+		return 0;
+
+	dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port,
+		pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "",
+		pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "",
+		pme_status & PME_WOL_ENERGY ? " \"Energy detect\"" : "");
+
+	ret = ksz8_ind_write8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, pme_status);
+	if (ret)
+		return ret;
+
+	ksz_read8(dev, REG_INT_STATUS, &pme_status);
+	return ksz_write8(dev, REG_INT_STATUS, pme_status && INT_PME);
+}
+
+/**
+ * ksz8_get_wol - Get Wake-on-LAN settings for a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ * @wol: Pointer to ethtool Wake-on-LAN settings structure.
+ *
+ * This function checks the PME 'wakeup-source' property from the
+ * device tree. If enabled, it sets the supported and active WoL
+ * flags.
+ */
+void ksz8_get_wol(struct ksz_device *dev, int port,
+		  struct ethtool_wolinfo *wol)
+{
+	u8 pme_ctrl;
+	int ret;
+
+	if (!dev->wakeup_source)
+		return;
+
+	wol->supported = WAKE_PHY;
+
+	/* Check if the current MAC address on this port can be set
+	 * as global for WAKE_MAGIC support. The result may vary
+	 * dynamically based on other ports configurations.
+	 */
+	if (ksz_is_port_mac_global_usable(dev->ds, port))
+		wol->supported |= WAKE_MAGIC;
+
+	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_CTRL, &pme_ctrl);
+	if (ret)
+		return;
+
+	if (pme_ctrl & PME_WOL_MAGICPKT)
+		wol->wolopts |= WAKE_MAGIC;
+	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
+		wol->wolopts |= WAKE_PHY;
+}
+
+/**
+ * ksz8_set_wol - Set Wake-on-LAN settings for a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ * @wol: Pointer to ethtool Wake-on-LAN settings structure.
+ *
+ * This function configures Wake-on-LAN (WoL) settings for a specified port.
+ * It validates the provided WoL options, checks if PME is enabled via the
+ * switch's device tree property, clears any previous wake reasons,
+ * and sets the Magic Packet flag in the port's PME control register if
+ * specified.
+ *
+ * Return: 0 on success, or other error codes on failure.
+ */
+int ksz8_set_wol(struct ksz_device *dev, int port,
+		 struct ethtool_wolinfo *wol)
+{
+	u8 pme_ctrl = 0, pme_ctrl_old = 0;
+	bool magic_switched_off;
+	bool magic_switched_on;
+	int ret;
+
+	if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
+		return -EINVAL;
+
+	if (!dev->wakeup_source)
+		return -EOPNOTSUPP;
+
+	ret = ksz8_handle_wake_reason(dev, port);
+	if (ret)
+		return ret;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		pme_ctrl |= PME_WOL_MAGICPKT;
+	if (wol->wolopts & WAKE_PHY)
+		pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
+
+	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_CTRL, &pme_ctrl_old);
+	if (ret)
+		return ret;
+
+	if (pme_ctrl_old == pme_ctrl)
+		return 0;
+
+	magic_switched_off = (pme_ctrl_old & PME_WOL_MAGICPKT) &&
+			    !(pme_ctrl & PME_WOL_MAGICPKT);
+	magic_switched_on = !(pme_ctrl_old & PME_WOL_MAGICPKT) &&
+			    (pme_ctrl & PME_WOL_MAGICPKT);
+
+	/* To keep reference count of MAC address, we should do this
+	 * operation only on change of WOL settings.
+	 */
+	if (magic_switched_on) {
+		ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
+		if (ret)
+			return ret;
+	} else if (magic_switched_off) {
+		ksz_switch_macaddr_put(dev->ds);
+	}
+
+	ret = ksz8_ind_write8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_CTRL, pme_ctrl);
+	if (ret) {
+		if (magic_switched_on)
+			ksz_switch_macaddr_put(dev->ds);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * ksz9477_wol_pre_shutdown - Prepares the switch device for shutdown while
+ *                            considering Wake-on-LAN (WoL) settings.
+ * @dev: The switch device structure.
+ * @wol_enabled: Pointer to a boolean which will be set to true if WoL is
+ *               enabled on any port.
+ *
+ * This function prepares the switch device for a safe shutdown while taking
+ * into account the Wake-on-LAN (WoL) settings on the user ports. It updates
+ * the wol_enabled flag accordingly to reflect whether WoL is active on any
+ * port. It also sets the PME output pin enable with the polarity specified
+ * through the device-tree.
+ */
+void ksz8_wol_pre_shutdown(struct ksz_device *dev, bool *wol_enabled)
+{
+	struct dsa_port *dp;
+	int ret;
+	u8 pme_pin_en = SW_PME_OUTPUT_ENABLE;
+
+	*wol_enabled = false;
+
+	if (!dev->wakeup_source)
+		return;
+
+	dsa_switch_for_each_user_port(dp, dev->ds) {
+		u8 pme_ctrl = 0;
+
+		ret = ksz8_ind_read8(dev, TABLE_PME_PORT(dp->index),
+				     REG_IND_PORT_PME_CTRL, &pme_ctrl);
+		if (!ret && pme_ctrl)
+			*wol_enabled = true;
+
+		/* make sure there are no pending wake events which would
+		 * prevent the device from going to sleep/shutdown.
+		 */
+		ksz8_handle_wake_reason(dev, dp->index);
+	}
+
+	/* Now we are save to enable PME pin. */
+	if (*wol_enabled) {
+		if (dev->pme_active_high)
+			pme_pin_en |= SW_PME_ACTIVE_HIGH;
+		ksz8_ind_write8(dev, TABLE_PME, REG_IND_GLOB_PME_CTRL, pme_pin_en);
+		ksz_write8(dev, REG_INT_ENABLE, INT_PME);
+	}
+}
+
 static int ksz8_port_queue_split(struct ksz_device *dev, int port, int queues)
 {
 	u8 mask_4q, mask_2q;
@@ -1829,6 +2043,12 @@ int ksz8_setup(struct dsa_switch *ds)
 	for (i = 0; i < (dev->info->num_vlans / 4); i++)
 		ksz8_r_vlan_entries(dev, i);
 
+	/* Make sure PME (WoL) is not enabled. If requested, it will be
+	 * enabled by ksz8_wol_pre_shutdown(). Otherwise, some PMICs do not
+	 * like PME events changes before shutdown.
+	 */
+	ksz_write8(dev, REG_INT_ENABLE, 0);
+
 	return ksz8_handle_global_errata(ds);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 69566a5d9cda..884e899145dd 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -337,6 +337,7 @@
 #define TABLE_EEE			(TABLE_EEE_V << TABLE_EXT_SELECT_S)
 #define TABLE_ACL			(TABLE_ACL_V << TABLE_EXT_SELECT_S)
 #define TABLE_PME			(TABLE_PME_V << TABLE_EXT_SELECT_S)
+#define TABLE_PME_PORT(port)		(TABLE_PME | (u8)((port) + 1))
 #define TABLE_LINK_MD			(TABLE_LINK_MD << TABLE_EXT_SELECT_S)
 #define TABLE_READ			BIT(4)
 #define TABLE_SELECT_S			2
@@ -359,8 +360,6 @@
 #define REG_IND_DATA_1			0x77
 #define REG_IND_DATA_0			0x78
 
-#define REG_IND_DATA_PME_EEE_ACL	0xA0
-
 #define REG_INT_STATUS			0x7C
 #define REG_INT_ENABLE			0x7D
 
@@ -589,12 +588,16 @@
 
 /* PME */
 
+#define REG_IND_GLOB_PME_CTRL		0x3
+#define REG_IND_PORT_PME_STATUS		0x3
+#define REG_IND_PORT_PME_CTRL		0x7
+
 #define SW_PME_OUTPUT_ENABLE		BIT(1)
 #define SW_PME_ACTIVE_HIGH		BIT(0)
 
-#define PORT_MAGIC_PACKET_DETECT	BIT(2)
-#define PORT_LINK_UP_DETECT		BIT(1)
-#define PORT_ENERGY_DETECT		BIT(0)
+#define PME_WOL_MAGICPKT		BIT(2)
+#define PME_WOL_LINKUP			BIT(1)
+#define PME_WOL_ENERGY			BIT(0)
 
 /* ACL */
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b074b4bb0629..61403898c1f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -307,6 +307,9 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.init = ksz8_switch_init,
 	.exit = ksz8_switch_exit,
 	.change_mtu = ksz8_change_mtu,
+	.get_wol = ksz8_get_wol,
+	.set_wol = ksz8_set_wol,
+	.wol_pre_shutdown = ksz8_wol_pre_shutdown,
 };
 
 static void ksz9477_phylink_mac_link_up(struct phylink_config *config,
@@ -4459,6 +4462,8 @@ int ksz_switch_register(struct ksz_device *dev)
 
 		dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
 							   "wakeup-source");
+		dev->pme_active_high = of_property_read_bool(dev->dev->of_node,
+							     "microchip,pme-active-high");
 	}
 
 	ret = dsa_register_switch(dev->ds);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 5f0a628b9849..1d7f2f18ee1f 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -174,6 +174,7 @@ struct ksz_device {
 	bool synclko_125;
 	bool synclko_disable;
 	bool wakeup_source;
+	bool pme_active_high;
 
 	struct vlan_table *vlan_cache;
 
-- 
2.43.0

[PATCH 3/4] net: dsa: microchip: check erratum workaround through indirect register read

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Check the erratum workaround application which ensures in addition
that indirect register write and read work as expected.

Commit b7fb7729c94f ("net: dsa: microchip: fix register write order in
ksz8_ind_write8()") would have been found faster like this.

Also fix the register naming as in the datasheet.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 drivers/net/dsa/microchip/ksz8795.c     | 10 ++++++++--
 drivers/net/dsa/microchip/ksz8795_reg.h |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 49081e9a8cb0..40851174f448 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1974,6 +1974,7 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
 	int ret = 0;
+	u8 data = 0xff;
 
 	/* KSZ87xx Errata DS80000687C.
 	 * Module 2: Link drops with some EEE link partners.
@@ -1981,8 +1982,13 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
 	 *   KSZ879x/KSZ877x/KSZ876x and some EEE link partners may result in
 	 *   the link dropping.
 	 */
-	if (dev->info->ksz87xx_eee_link_erratum)
-		ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_HI, 0);
+	if (dev->info->ksz87xx_eee_link_erratum) {
+		ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, 0);
+		if (!ret)
+			ret = ksz8_ind_read8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, &data);
+		if (!ret && data)
+			dev_err(dev->dev, "failed to disable EEE next page exchange (erratum)\n");
+	}
 
 	return ret;
 }
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 884e899145dd..8d18507c92bf 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -767,8 +767,8 @@
 #define IND_ACC_TABLE(table)		((table) << 8)
 
 /* */
-#define REG_IND_EEE_GLOB2_LO		0x34
-#define REG_IND_EEE_GLOB2_HI		0x35
+#define REG_IND_EEE_GLOB2_HI		0x34
+#define REG_IND_EEE_GLOB2_LO		0x35
 
 /**
  * MIB_COUNTER_VALUE			00-00000000-3FFFFFFF
-- 
2.43.0

[PATCH 4/4] net: dsa: microchip: ksz9477: correct description of WoL functions

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Correct KSZ9477 WoL function description, found when evaluating it for
usage at KSZ8795 family of switches.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 drivers/net/dsa/microchip/ksz9477.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 425e20daf1e9..1344b60ee2c3 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -95,9 +95,9 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
  * @port: The port number.
  * @wol: Pointer to ethtool Wake-on-LAN settings structure.
  *
- * This function checks the PME Pin Control Register to see if  PME Pin Output
- * Enable is set, indicating PME is enabled. If enabled, it sets the supported
- * and active WoL flags.
+ * This function checks the PME 'wakeup-source' property from the
+ * device tree. If enabled, it sets the supported and active WoL
+ * flags.
  */
 void ksz9477_get_wol(struct ksz_device *dev, int port,
 		     struct ethtool_wolinfo *wol)
@@ -135,7 +135,7 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
  *
  * This function configures Wake-on-LAN (WoL) settings for a specified port.
  * It validates the provided WoL options, checks if PME is enabled via the
- * switch's PME Pin Control Register, clears any previous wake reasons,
+ * switch's device tree property, clears any previous wake reasons,
  * and sets the Magic Packet flag in the port's PME control register if
  * specified.
  *
-- 
2.43.0

Re: [PATCH 4/4] net: dsa: microchip: ksz9477: correct description of WoL functions

[Oleksij Rempel]

On Wed, Jul 17, 2024 at 09:37:25PM +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Correct KSZ9477 WoL function description, found when evaluating it for
> usage at KSZ8795 family of switches.
> 
> Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 425e20daf1e9..1344b60ee2c3 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -95,9 +95,9 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
>   * @port: The port number.
>   * @wol: Pointer to ethtool Wake-on-LAN settings structure.
>   *
> - * This function checks the PME Pin Control Register to see if  PME Pin Output
> - * Enable is set, indicating PME is enabled. If enabled, it sets the supported
> - * and active WoL flags.
> + * This function checks the PME 'wakeup-source' property from the
> + * device tree. If enabled, it sets the supported and active WoL
> + * flags.

Good point, it is out dated variant of my previous implementation but i
would prefer not to mention device tree as the only source for this
flag.

>   */
>  void ksz9477_get_wol(struct ksz_device *dev, int port,
>  		     struct ethtool_wolinfo *wol)
> @@ -135,7 +135,7 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
>   *
>   * This function configures Wake-on-LAN (WoL) settings for a specified port.
>   * It validates the provided WoL options, checks if PME is enabled via the
> - * switch's PME Pin Control Register, clears any previous wake reasons,
> + * switch's device tree property, clears any previous wake reasons,

same here.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] net: dsa: microchip: check erratum workaround through indirect register read

[Andrew Lunn]

On Wed, Jul 17, 2024 at 09:37:24PM +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Check the erratum workaround application which ensures in addition
> that indirect register write and read work as expected.
> 
> Commit b7fb7729c94f ("net: dsa: microchip: fix register write order in
> ksz8_ind_write8()") would have been found faster like this.
> 
> Also fix the register naming as in the datasheet.

We are in the merge window at the moment, so net-next is closed at the
moment. Please repost in two weeks.

> 
> @@ -1974,6 +1974,7 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
>  	int ret = 0;
> +	u8 data = 0xff;

Reverse Christmas tree please.

>  
>  	/* KSZ87xx Errata DS80000687C.
>  	 * Module 2: Link drops with some EEE link partners.
> @@ -1981,8 +1982,13 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
>  	 *   KSZ879x/KSZ877x/KSZ876x and some EEE link partners may result in
>  	 *   the link dropping.
>  	 */
> -	if (dev->info->ksz87xx_eee_link_erratum)
> -		ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_HI, 0);
> +	if (dev->info->ksz87xx_eee_link_erratum) {
> +		ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, 0);
> +		if (!ret)
> +			ret = ksz8_ind_read8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, &data);
> +		if (!ret && data)
> +			dev_err(dev->dev, "failed to disable EEE next page exchange (erratum)\n");

If data is not 0, should it be considered fatal? Maybe return -EIO ?

	Andrew

Re: [PATCH 3/4] net: dsa: microchip: check erratum workaround through indirect register read

[Pieter]

On Thu, Jul 18, 2024 at 04:24, Andrew Lunn <andrew@lunn.ch> wrote :
> On Wed, Jul 17, 2024 at 09:37:24PM +0200, vtpieter@gmail.com wrote:
> > From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> >
> > Check the erratum workaround application which ensures in addition
> > that indirect register write and read work as expected.
> >
> > Commit b7fb7729c94f ("net: dsa: microchip: fix register write order in
> > ksz8_ind_write8()") would have been found faster like this.
> >
> > Also fix the register naming as in the datasheet.
>
> We are in the merge window at the moment, so net-next is closed at the
> moment. Please repost in two weeks.

Right sorry I only realize this now. I did some reading up on netdev
patches; will make sure to rebase on net-next tree as well when I
submit the v2 in 2 weeks.

> >
> >       /* KSZ87xx Errata DS80000687C.
> >        * Module 2: Link drops with some EEE link partners.
> > @@ -1981,8 +1982,13 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
> >        *   KSZ879x/KSZ877x/KSZ876x and some EEE link partners may result in
> >        *   the link dropping.
> >        */
> > -     if (dev->info->ksz87xx_eee_link_erratum)
> > -             ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_HI, 0);
> > +     if (dev->info->ksz87xx_eee_link_erratum) {
> > +             ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, 0);
> > +             if (!ret)
> > +                     ret = ksz8_ind_read8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_LO, &data);
> > +             if (!ret && data)
> > +                     dev_err(dev->dev, "failed to disable EEE next page exchange (erratum)\n");
>
> If data is not 0, should it be considered fatal? Maybe return -EIO ?

Indeed that would make more sense; will return -EIO for v2.

Thanks, Pieter

Re: [PATCH 2/4] net: dsa: microchip: ksz8795: add Wake on LAN support

[Oleksij Rempel]

Hi Pieter,

If I see it correctly, the only difference between KSZ9477 and KSZ8795
code is the register access. Even bit offsets are identical. I do not
think indirect register access is good justification for duplication
this amount of code.

On Wed, Jul 17, 2024 at 09:37:23PM +0200, vtpieter@gmail.com wrote:
>  
> +static int ksz8_ind_read8(struct ksz_device *dev, u8 table, u16 addr, u8 *val)

Will be good to add comment for this function.

....
> +/**
> + * ksz8_handle_wake_reason - Handle wake reason on a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + *
> + * This function reads the PME (Power Management Event) status register of a
> + * specified port to determine the wake reason. If there is no wake event, it
> + * returns early. Otherwise, it logs the wake reason which could be due to a
> + * "Magic Packet", "Link Up", or "Energy Detect" event. The PME status register
> + * is then cleared to acknowledge the handling of the wake event; followed by
> + * clearing the global Interrupt Status Register.
> + *
> + * Return: 0 on success, or an error code on failure.
> + */
> +static int ksz8_handle_wake_reason(struct ksz_device *dev, int port)
> +{
> +	u8 pme_status;
> +	int ret;
> +
> +	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, &pme_status);
> +	if (ret)
> +		return ret;
> +
> +	if (!pme_status)
> +		return 0;
> +
> +	dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port,
> +		pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "",
> +		pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "",
> +		pme_status & PME_WOL_ENERGY ? " \"Energy detect\"" : "");
> +
> +	ret = ksz8_ind_write8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, pme_status);
> +	if (ret)
> +		return ret;
> +
> +	ksz_read8(dev, REG_INT_STATUS, &pme_status);

Recycling a variable with use case specific name, make the code more
confusing. Use "var" far all or "int_status" for this case.

> +	return ksz_write8(dev, REG_INT_STATUS, pme_status && INT_PME);

"pme_status && INT_PME" will write BIT(0) instead of BIT(4) which should
be written in this case - So, it should be "pme_status & INT_PME".

Instead of ksz_read8(dev, REG_INT_STATUS + ksz_write8, you can use one
ksz_rmw8()

> +}
> +
> +/**
> + * ksz8_get_wol - Get Wake-on-LAN settings for a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + * @wol: Pointer to ethtool Wake-on-LAN settings structure.
> + *
> + * This function checks the PME 'wakeup-source' property from the
> + * device tree. If enabled, it sets the supported and active WoL
> + * flags.

This is a bit confusing - this function do not checks devicetree properly.
It only checks if the wakeup_source flag is set. In current code state, it is
set from devicetree properly.

> + */
> +void ksz8_get_wol(struct ksz_device *dev, int port,
> +		  struct ethtool_wolinfo *wol)
> +{
> +	u8 pme_ctrl;
> +	int ret;
> +
> +	if (!dev->wakeup_source)
> +		return;
> +
> +	wol->supported = WAKE_PHY;
> +
> +	/* Check if the current MAC address on this port can be set
> +	 * as global for WAKE_MAGIC support. The result may vary
> +	 * dynamically based on other ports configurations.
> +	 */
> +	if (ksz_is_port_mac_global_usable(dev->ds, port))
> +		wol->supported |= WAKE_MAGIC;
> +
> +	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_CTRL, &pme_ctrl);
> +	if (ret)
> +		return;
> +
> +	if (pme_ctrl & PME_WOL_MAGICPKT)
> +		wol->wolopts |= WAKE_MAGIC;
> +	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
> +		wol->wolopts |= WAKE_PHY;
> +}
> +
> +/**
> + * ksz8_set_wol - Set Wake-on-LAN settings for a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + * @wol: Pointer to ethtool Wake-on-LAN settings structure.
> + *
> + * This function configures Wake-on-LAN (WoL) settings for a specified port.
> + * It validates the provided WoL options, checks if PME is enabled via the
> + * switch's device tree property, clears any previous wake reasons,

Same here, the "device tree" related part of comment can bit rot with
time.

> + * and sets the Magic Packet flag in the port's PME control register if
> + * specified.
> + *
> + * Return: 0 on success, or other error codes on failure.
> + */
> +int ksz8_set_wol(struct ksz_device *dev, int port,
> +/**
> + * ksz9477_wol_pre_shutdown - Prepares the switch device for shutdown while

s/ksz9477_wol_pre_shutdown/ksz8_wol_pre_shutdown

> + *                            considering Wake-on-LAN (WoL) settings.
> + * @dev: The switch device structure.
> + * @wol_enabled: Pointer to a boolean which will be set to true if WoL is
> + *               enabled on any port.
> + *
> + * This function prepares the switch device for a safe shutdown while taking
> + * into account the Wake-on-LAN (WoL) settings on the user ports. It updates
> + * the wol_enabled flag accordingly to reflect whether WoL is active on any
> + * port. It also sets the PME output pin enable with the polarity specified
> + * through the device-tree.
> + */
> +void ksz8_wol_pre_shutdown(struct ksz_device *dev, bool *wol_enabled)
> +{

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index b074b4bb0629..61403898c1f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -307,6 +307,9 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>  	.init = ksz8_switch_init,
>  	.exit = ksz8_switch_exit,
>  	.change_mtu = ksz8_change_mtu,
> +	.get_wol = ksz8_get_wol,
> +	.set_wol = ksz8_set_wol,
> +	.wol_pre_shutdown = ksz8_wol_pre_shutdown,

This part will break on KSZ8830 variants. There is no WoL support.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] net: dsa: microchip: ksz8795: add Wake on LAN support

[Pieter]

Le ven. 19 juil. 2024 à 15:46, Oleksij Rempel
<o.rempel@pengutronix.de> a écrit :
>
> Hi Pieter,
>
> If I see it correctly, the only difference between KSZ9477 and KSZ8795
> code is the register access. Even bit offsets are identical. I do not
> think indirect register access is good justification for duplication
> this amount of code.

Hi Oleksij, thanks for the review! I guess you're right - I must have
gone too quickly for the easy way out. There's actually one additional
register access for KSZ8795 but it should be possible to get around
this as well. I'll work on it but it might take me a few weeks to get
this ready. Would be great if you still have a KSZ9477 family board
around to validate.

Cheers, Pieter

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip: add microchip,pme-active-high flag

[Oleksij Rempel]

On Wed, Jul 17, 2024 at 09:37:22PM +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Add microchip,pme-active-high property to set the PME (Power
> Management Event) pin polarity for Wake on Lan interrupts.
> 
> Note that the polarity is active-low by default.
> 
> Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/4] dt-bindings: net: dsa: microchip: add microchip,pme-active-high flag

[Rob Herring (Arm)]

On Wed, 17 Jul 2024 21:37:22 +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Add microchip,pme-active-high property to set the PME (Power
> Management Event) pin polarity for Wake on Lan interrupts.
> 
> Note that the polarity is active-low by default.
> 
> Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> ---
>  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>