[PATCH net-next v3 0/7] net: dsa: microchip: provide Wake on LAN support

[PATCH net-next v3 0/7] net: dsa: microchip: provide Wake on LAN support

[Oleksij Rempel]

changes v3:
- use ethernet address of DSA master instead from devicetree
- use dev_ops->wol* instead of list of supported switch
- don't shotdown the switch if WoL is enabled
- rework on top of latest HSR changes

changes v2:
- rebase against latest next

This series of patches provides Wake on LAN support for the KSZ9477
family of switches. It was tested on KSZ8565 Switch with PME pin
attached to an external PMIC.

The patch making WoL configuration persist on system shutdown will be
send separately, since it will potentially need more discussion.

Oleksij Rempel (7):
  net: dsa: microchip: Add missing MAC address register offset for
    ksz8863
  net: dsa: microchip: Set unique MAC at startup for WoL support
  net: dsa: microchip: ksz9477: add Wake on LAN support
  net: dsa: microchip: ksz9477: add Wake on PHY event support
  dt-bindings: net: dsa: microchip: add wakeup-source property
  net: dsa: microchip: use wakeup-source DT property to enable PME
    output
  net: dsa: microchip: do not shut down the switch if WoL is active

 .../bindings/net/dsa/microchip,ksz.yaml       |   2 +
 drivers/net/dsa/microchip/ksz9477.c           | 116 +++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h           |   4 +
 drivers/net/dsa/microchip/ksz9477_i2c.c       |   3 +
 drivers/net/dsa/microchip/ksz_common.c        | 117 ++++++++++++++++--
 drivers/net/dsa/microchip/ksz_common.h        |   7 ++
 drivers/net/dsa/microchip/ksz_spi.c           |   3 +
 7 files changed, 245 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH net-next v3 1/7] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Oleksij Rempel]

Add the missing offset for the global MAC address register
(REG_SW_MAC_ADDR) for the ksz8863 family of switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b800ace40ce1..02fab1adb27f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -441,6 +441,7 @@ static const u8 ksz8795_shifts[] = {
 };
 
 static const u16 ksz8863_regs[] = {
+	[REG_SW_MAC_ADDR]		= 0x70,
 	[REG_IND_CTRL_0]		= 0x79,
 	[REG_IND_DATA_8]		= 0x7B,
 	[REG_IND_DATA_CHECK]		= 0x7B,
-- 
2.39.2

Re: [PATCH net-next v3 1/7] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Andrew Lunn]

On Fri, Oct 13, 2023 at 02:23:59PM +0200, Oleksij Rempel wrote:
> Add the missing offset for the global MAC address register
> (REG_SW_MAC_ADDR) for the ksz8863 family of switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3 1/7] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Florian Fainelli]

On 10/13/2023 5:23 AM, Oleksij Rempel wrote:
> Add the missing offset for the global MAC address register
> (REG_SW_MAC_ADDR) for the ksz8863 family of switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next v3 2/7] net: dsa: microchip: Set unique MAC at startup for WoL support

[Oleksij Rempel]

Set a unique global MAC address for each switch on the network at system
startup by syncing the switch's global MAC address with the Ethernet
address of the DSA master interface. This is crucial for supporting
Wake-on-LAN (WoL) functionality, as it requires a unique address for
each switch.

Although the operation is performed only at system start and won't sync
if the master Ethernet address changes dynamically, it lays the
groundwork for WoL support by ensuring a unique MAC address for each
switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 69 +++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 02fab1adb27f..db0ef4ad181e 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2173,6 +2173,57 @@ static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
 	return ksz_irq_common_setup(dev, pirq);
 }
 
+/**
+ * ksz_cmn_write_global_mac_addr - Write global MAC address to switch.
+ * @dev: The device structure.
+ * @addr: Pointer to MAC address.
+ *
+ * This function programs the switch's MAC address register with the given
+ * MAC address. The global MAC address is used as the source address in MAC
+ * pause control frames, for HSR self-address filtering, Wake-on-LAN (WoL),
+ * and for self-address filtering purposes.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
+static int ksz_cmn_write_global_mac_addr(struct ksz_device *dev,
+					 const unsigned char *addr)
+{
+	const u16 *regs = dev->info->regs;
+	int i, ret;
+
+	for (i = 0; i < ETH_ALEN; i++) {
+		ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * ksz_cmn_set_default_switch_mac_addr - Set the switch's global MAC address
+ *                                       from master port.
+ * @dev: The device structure.
+ *
+ * This function retrieves the MAC address from the master network device
+ * associated with the CPU port and writes it to the switch's global MAC
+ * address register.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
+static int ksz_cmn_set_default_switch_mac_addr(struct ksz_device *dev)
+{
+	struct dsa_switch *ds = dev->ds;
+	struct net_device *master;
+
+	/* use CPU port to get master net device because it is guaranteed
+	 * to be a valid port
+	 */
+	master = dsa_port_to_master(dsa_to_port(ds, dev->cpu_port));
+
+	return ksz_cmn_write_global_mac_addr(dev, master->dev_addr);
+}
+
 static int ksz_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
@@ -2194,6 +2245,16 @@ static int ksz_setup(struct dsa_switch *ds)
 		return ret;
 	}
 
+	/* Set switch MAC address from master port.
+	 * In the current implementation, this operation is only
+	 * performed during system start and will not sync if the master
+	 * Ethernet address changes dynamically thereafter. The global MAC
+	 * address still can be overwritten for HSR use cases.
+	 */
+	ret = ksz_cmn_set_default_switch_mac_addr(dev);
+	if (ret)
+		return ret;
+
 	/* set broadcast storm protection 10% rate */
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   BROADCAST_STORM_RATE,
@@ -3572,8 +3633,6 @@ static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 	const unsigned char *addr = slave->dev_addr;
 	struct ksz_switch_macaddr *switch_macaddr;
 	struct ksz_device *dev = ds->priv;
-	const u16 *regs = dev->info->regs;
-	int i;
 
 	/* Make sure concurrent MAC address changes are blocked */
 	ASSERT_RTNL();
@@ -3599,11 +3658,7 @@ static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 	refcount_set(&switch_macaddr->refcount, 1);
 	dev->switch_macaddr = switch_macaddr;
 
-	/* Program the switch MAC address to hardware */
-	for (i = 0; i < ETH_ALEN; i++)
-		ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
-
-	return 0;
+	return ksz_cmn_write_global_mac_addr(dev, addr);
 }
 
 static void ksz_switch_macaddr_put(struct dsa_switch *ds)
-- 
2.39.2

Re: [PATCH net-next v3 2/7] net: dsa: microchip: Set unique MAC at startup for WoL support

[Vladimir Oltean]

On Fri, Oct 13, 2023 at 02:24:00PM +0200, Oleksij Rempel wrote:
> Set a unique global MAC address for each switch on the network at system
> startup by syncing the switch's global MAC address with the Ethernet
> address of the DSA master interface. This is crucial for supporting
> Wake-on-LAN (WoL) functionality, as it requires a unique address for
> each switch.
> 
> Although the operation is performed only at system start and won't sync
> if the master Ethernet address changes dynamically, it lays the
> groundwork for WoL support by ensuring a unique MAC address for each
> switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Why not take the MAC address of the user port at ksz9477_set_wol() time,
and use the existing ksz_switch_macaddr_get() API that was just added so
that this use case could work?

Re: [PATCH net-next v3 2/7] net: dsa: microchip: Set unique MAC at startup for WoL support

[Florian Fainelli]

On 10/13/2023 5:32 AM, Vladimir Oltean wrote:
> On Fri, Oct 13, 2023 at 02:24:00PM +0200, Oleksij Rempel wrote:
>> Set a unique global MAC address for each switch on the network at system
>> startup by syncing the switch's global MAC address with the Ethernet
>> address of the DSA master interface. This is crucial for supporting
>> Wake-on-LAN (WoL) functionality, as it requires a unique address for
>> each switch.
>>
>> Although the operation is performed only at system start and won't sync
>> if the master Ethernet address changes dynamically, it lays the
>> groundwork for WoL support by ensuring a unique MAC address for each
>> switch.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
> 
> Why not take the MAC address of the user port at ksz9477_set_wol() time,
> and use the existing ksz_switch_macaddr_get() API that was just added so
> that this use case could work?

Agreed we do that in a number of Ethernet MAC and PHY drivers FWIW 
(net_device::dev_addr).
-- 
Florian

Re: [PATCH net-next v3 2/7] net: dsa: microchip: Set unique MAC at startup for WoL support

[Vladimir Oltean]

On Sun, Oct 15, 2023 at 02:18:43PM -0700, Florian Fainelli wrote:
> 
> 
> On 10/13/2023 5:32 AM, Vladimir Oltean wrote:
> > On Fri, Oct 13, 2023 at 02:24:00PM +0200, Oleksij Rempel wrote:
> > > Set a unique global MAC address for each switch on the network at system
> > > startup by syncing the switch's global MAC address with the Ethernet
> > > address of the DSA master interface. This is crucial for supporting
> > > Wake-on-LAN (WoL) functionality, as it requires a unique address for
> > > each switch.
> > > 
> > > Although the operation is performed only at system start and won't sync
> > > if the master Ethernet address changes dynamically, it lays the
> > > groundwork for WoL support by ensuring a unique MAC address for each
> > > switch.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > 
> > Why not take the MAC address of the user port at ksz9477_set_wol() time,
> > and use the existing ksz_switch_macaddr_get() API that was just added so
> > that this use case could work?
> 
> Agreed we do that in a number of Ethernet MAC and PHY drivers FWIW
> (net_device::dev_addr).
> -- 
> Florian

To be clear (to Oleksij), the request is for WoL to use the same runtime
management of the global MAC address (ksz_switch_macaddr_get) as HSR,
and also extend ksz_port_set_mac_address() to deny address changes to a
port with WoL active. Thus, multiple user ports could have WoL enabled
as long as they share the same MAC address. MAC address changes are also
possible while WoL is not enabled. I guess wol->supported should only
get set on those user ports which have the same MAC address as the
global MAC address (if a global MAC address is configured), or on all
user ports (if there is none).

Re: [PATCH net-next v3 2/7] net: dsa: microchip: Set unique MAC at startup for WoL support

[Andrew Lunn]

On Fri, Oct 13, 2023 at 02:24:00PM +0200, Oleksij Rempel wrote:
> Set a unique global MAC address for each switch on the network at system
> startup by syncing the switch's global MAC address with the Ethernet
> address of the DSA master interface. This is crucial for supporting
> Wake-on-LAN (WoL) functionality, as it requires a unique address for
> each switch.
> 
> Although the operation is performed only at system start and won't sync
> if the master Ethernet address changes dynamically, it lays the
> groundwork for WoL support by ensuring a unique MAC address for each
> switch.

I've not been following this patchset, so sorry if i make points
others have asked on earlier versions.

Maybe it would be good to add that the hardware only supports one MAC
address for all ports for WoL, and its this address. At least that is
my assumption.

> + * ksz_cmn_set_default_switch_mac_addr - Set the switch's global MAC address
> + *                                       from master port.

Florian is doing a search replace to make use of the word `conduit`. 


> @@ -3572,8 +3633,6 @@ static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
>  	const unsigned char *addr = slave->dev_addr;

and this might need to change to user?

[PATCH net-next v3 3/7] net: dsa: microchip: ksz9477: add Wake on LAN support

[Oleksij Rempel]

Add WoL support for KSZ9477 family of switches. This code was tested on
KSZ8563 chip and supports only wake on Magic Packet for now.
Other parts needed for fully operational WoL support are in the followup
patches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 109 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |   4 +
 drivers/net/dsa/microchip/ksz_common.c |  26 ++++++
 drivers/net/dsa/microchip/ksz_common.h |   4 +
 4 files changed, 143 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index cde8ef33d029..3d9b6d6083cf 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -56,6 +56,112 @@ int ksz9477_change_mtu(struct ksz_device *dev, int port, int mtu)
 				  REG_SW_MTU_MASK, frame_size);
 }
 
+/**
+ * ksz9477_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.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
+static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
+{
+	u8 pme_status;
+	int ret;
+
+	ret = ksz_pread8(dev, port, REG_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 ? "\"Enery detect\"" : "");
+
+	return ksz_pwrite8(dev, port, REG_PORT_PME_STATUS, pme_status);
+}
+
+/**
+ * ksz9477_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 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.
+ */
+void ksz9477_get_wol(struct ksz_device *dev, int port,
+		     struct ethtool_wolinfo *wol)
+{
+	u8 pme_ctrl, pme_conf;
+	int ret;
+
+	ret = ksz_read8(dev, REG_SW_PME_CTRL, &pme_conf);
+	if (ret)
+		return;
+
+	if (!(pme_conf & PME_ENABLE))
+		return;
+
+	wol->supported = WAKE_MAGIC;
+
+	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
+	if (ret)
+		return;
+
+	if (pme_ctrl & PME_WOL_MAGICPKT)
+		wol->wolopts |= WAKE_MAGIC;
+}
+
+/**
+ * ksz9477_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 PME Pin Control Register, 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 ksz9477_set_wol(struct ksz_device *dev, int port,
+		    struct ethtool_wolinfo *wol)
+{
+	u8 pme_conf, pme_ctrl = 0;
+	int ret;
+
+	if (wol->wolopts & ~WAKE_MAGIC)
+		return -EINVAL;
+
+	ret = ksz_read8(dev, REG_SW_PME_CTRL, &pme_conf);
+	if (ret)
+		return ret;
+
+	if (!(pme_conf & PME_ENABLE))
+		return -EOPNOTSUPP;
+
+	ret = ksz9477_handle_wake_reason(dev, port);
+	if (ret)
+		return ret;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		pme_ctrl |= PME_WOL_MAGICPKT;
+
+	return ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl);
+}
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
 {
 	unsigned int val;
@@ -1006,6 +1112,9 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
 
 	ksz9477_port_acl_init(dev, port);
+
+	/* clear pending wake flags */
+	ksz9477_handle_wake_reason(dev, port);
 }
 
 void ksz9477_config_cpu_port(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index f90e2e8ebe80..fa8d0318b437 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -58,6 +58,10 @@ void ksz9477_switch_exit(struct ksz_device *dev);
 void ksz9477_port_queue_split(struct ksz_device *dev, int port);
 void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr);
 void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr);
+void ksz9477_get_wol(struct ksz_device *dev, int port,
+		     struct ethtool_wolinfo *wol);
+int ksz9477_set_wol(struct ksz_device *dev, int port,
+		    struct ethtool_wolinfo *wol);
 
 int ksz9477_port_acl_init(struct ksz_device *dev, int port);
 void ksz9477_port_acl_free(struct ksz_device *dev, int port);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index db0ef4ad181e..bef1951fe6f2 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -319,6 +319,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.mdb_del = ksz9477_mdb_del,
 	.change_mtu = ksz9477_change_mtu,
 	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
+	.wol_get = ksz9477_get_wol,
+	.wol_set = ksz9477_set_wol,
 	.config_cpu_port = ksz9477_config_cpu_port,
 	.tc_cbs_set_cinc = ksz9477_tc_cbs_set_cinc,
 	.enable_stp_addr = ksz9477_enable_stp_addr,
@@ -2935,6 +2937,28 @@ static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void ksz_get_wol(struct dsa_switch *ds, int port,
+			struct ethtool_wolinfo *wol)
+{
+	struct ksz_device *dev = ds->priv;
+
+	memset(wol, 0, sizeof(*wol));
+
+	if (dev->dev_ops->wol_get)
+		dev->dev_ops->wol_get(dev, port, wol);
+}
+
+static int ksz_set_wol(struct dsa_switch *ds, int port,
+		       struct ethtool_wolinfo *wol)
+{
+	struct ksz_device *dev = ds->priv;
+
+	if (dev->dev_ops->wol_set)
+		return dev->dev_ops->wol_set(dev, port, wol);
+
+	return -EOPNOTSUPP;
+}
+
 static void ksz_set_xmii(struct ksz_device *dev, int port,
 			 phy_interface_t interface)
 {
@@ -3782,6 +3806,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_pause_stats	= ksz_get_pause_stats,
 	.port_change_mtu	= ksz_change_mtu,
 	.port_max_mtu		= ksz_max_mtu,
+	.get_wol		= ksz_get_wol,
+	.set_wol		= ksz_set_wol,
 	.get_ts_info		= ksz_get_ts_info,
 	.port_hwtstamp_get	= ksz_hwtstamp_get,
 	.port_hwtstamp_set	= ksz_hwtstamp_set,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8842efca0871..43d0d8717eaa 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -373,6 +373,10 @@ struct ksz_dev_ops {
 				    int duplex, bool tx_pause, bool rx_pause);
 	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
 	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
+	void (*wol_get)(struct ksz_device *dev, int port,
+			struct ethtool_wolinfo *wol);
+	int (*wol_set)(struct ksz_device *dev, int port,
+		       struct ethtool_wolinfo *wol);
 	void (*config_cpu_port)(struct dsa_switch *ds);
 	int (*enable_stp_addr)(struct ksz_device *dev);
 	int (*reset)(struct ksz_device *dev);
-- 
2.39.2

Re: [PATCH net-next v3 3/7] net: dsa: microchip: ksz9477: add Wake on LAN support

[Andrew Lunn]

> + * ksz9477_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 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.

I'm missing something here: Why would the PME Pin Output Enable not be
set?

I also wounder if there should be one additional test. Is the MAC
address on the user interface the same as the one programmed in the
switch? If it is different supported should not be set.

> +void ksz9477_get_wol(struct ksz_device *dev, int port,
> +		     struct ethtool_wolinfo *wol)
> +{
> +	u8 pme_ctrl, pme_conf;
> +	int ret;
> +
> +	ret = ksz_read8(dev, REG_SW_PME_CTRL, &pme_conf);
> +	if (ret)
> +		return;
> +
> +	if (!(pme_conf & PME_ENABLE))
> +		return;
> +
> +	wol->supported = WAKE_MAGIC;
> +
> +	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
> +	if (ret)
> +		return;
> +
> +	if (pme_ctrl & PME_WOL_MAGICPKT)
> +		wol->wolopts |= WAKE_MAGIC;
> +}

> +static void ksz_get_wol(struct dsa_switch *ds, int port,
> +			struct ethtool_wolinfo *wol)
> +{
> +	struct ksz_device *dev = ds->priv;
> +
> +	memset(wol, 0, sizeof(*wol));

Drivers should not need this, i would expect the core to zero the
structure.


    Andrew

---
pw-bot: cr

Re: [PATCH net-next v3 3/7] net: dsa: microchip: ksz9477: add Wake on LAN support

[Vladimir Oltean]

On Fri, Oct 13, 2023 at 02:24:01PM +0200, Oleksij Rempel wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index db0ef4ad181e..bef1951fe6f2 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -319,6 +319,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.mdb_del = ksz9477_mdb_del,
>  	.change_mtu = ksz9477_change_mtu,
>  	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
> +	.wol_get = ksz9477_get_wol,
> +	.wol_set = ksz9477_set_wol,

Can the dev_ops function pointers also be named get_wol() and set_wol()
for consistency with everything else?

>  	.config_cpu_port = ksz9477_config_cpu_port,
>  	.tc_cbs_set_cinc = ksz9477_tc_cbs_set_cinc,
>  	.enable_stp_addr = ksz9477_enable_stp_addr,
> @@ -2935,6 +2937,28 @@ static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
>  	return 0;
>  }

[PATCH net-next v3 4/7] net: dsa: microchip: ksz9477: add Wake on PHY event support

[Oleksij Rempel]

KSZ9477 family of switches supports multiple PHY events:
- wake on Link Up
- wake on Energy Detect.
Since current UAPI can't differentiate between this PHY events, map all of them
to WAKE_PHY.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3d9b6d6083cf..8d51ec629d76 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -112,7 +112,7 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
 	if (!(pme_conf & PME_ENABLE))
 		return;
 
-	wol->supported = WAKE_MAGIC;
+	wol->supported = WAKE_PHY | WAKE_MAGIC;
 
 	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
 	if (ret)
@@ -120,6 +120,8 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
 
 	if (pme_ctrl & PME_WOL_MAGICPKT)
 		wol->wolopts |= WAKE_MAGIC;
+	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
+		wol->wolopts |= WAKE_PHY;
 }
 
 /**
@@ -142,7 +144,7 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
 	u8 pme_conf, pme_ctrl = 0;
 	int ret;
 
-	if (wol->wolopts & ~WAKE_MAGIC)
+	if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
 		return -EINVAL;
 
 	ret = ksz_read8(dev, REG_SW_PME_CTRL, &pme_conf);
@@ -158,6 +160,8 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
 
 	if (wol->wolopts & WAKE_MAGIC)
 		pme_ctrl |= PME_WOL_MAGICPKT;
+	if (wol->wolopts & WAKE_PHY)
+		pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
 
 	return ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl);
 }
-- 
2.39.2

Re: [PATCH net-next v3 4/7] net: dsa: microchip: ksz9477: add Wake on PHY event support

[Andrew Lunn]

On Fri, Oct 13, 2023 at 02:24:02PM +0200, Oleksij Rempel wrote:
> KSZ9477 family of switches supports multiple PHY events:
> - wake on Link Up
> - wake on Energy Detect.
> Since current UAPI can't differentiate between this PHY events, map all of them
> to WAKE_PHY.

I assume link up and energy detect work without doing a MAC address
comparison? So maybe these should be added first, and then handle
WAKE_MAGIC and setting the global MAC address, and verifying it
matches the user interface MAC address?

     Andrew

[PATCH net-next v3 5/7] dt-bindings: net: dsa: microchip: add wakeup-source property

[Oleksij Rempel]

Add wakeup-source property to enable Wake on Lan functionality in the
switch.

Since PME wake pin is not always attached to the SoC, use wakeup-source
instead of wakeup-gpios

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 41014f5c01c4..5751a729af33 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -72,6 +72,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  wakeup-source: true
+
 required:
   - compatible
   - reg
-- 
2.39.2

[PATCH net-next v3 6/7] net: dsa: microchip: use wakeup-source DT property to enable PME output

[Oleksij Rempel]

KSZ switches with WoL support signals wake event over PME pin. If this
pin is attached to some external PMIC or System Controller can't be
described as GPIO, the only way to describe it in the devicetree is to
use wakeup-source property. So, add support for this property and enable
PME switch output if this property is present.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 3 +++
 drivers/net/dsa/microchip/ksz_common.c | 3 +++
 drivers/net/dsa/microchip/ksz_common.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d51ec629d76..8035e14ac372 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1241,6 +1241,9 @@ int ksz9477_setup(struct dsa_switch *ds)
 	/* enable global MIB counter freeze function */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
 
+	if (dev->wakeup_source)
+		ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index bef1951fe6f2..820edda82cea 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -4240,6 +4240,9 @@ int ksz_switch_register(struct ksz_device *dev)
 			dev_err(dev->dev, "inconsistent synclko settings\n");
 			return -EINVAL;
 		}
+
+		dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
+							   "wakeup-source");
 	}
 
 	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 43d0d8717eaa..80679f38ee12 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -163,6 +163,7 @@ struct ksz_device {
 	phy_interface_t compat_interface;
 	bool synclko_125;
 	bool synclko_disable;
+	bool wakeup_source;
 
 	struct vlan_table *vlan_cache;
 
-- 
2.39.2

Re: [PATCH net-next v3 6/7] net: dsa: microchip: use wakeup-source DT property to enable PME output

[Andrew Lunn]

On Fri, Oct 13, 2023 at 02:24:04PM +0200, Oleksij Rempel wrote:
> KSZ switches with WoL support signals wake event over PME pin. If this
> pin is attached to some external PMIC or System Controller can't be
> described as GPIO, the only way to describe it in the devicetree is to
> use wakeup-source property. So, add support for this property and enable
> PME switch output if this property is present.

So i get the feeling these patches are in the wrong order. The binding
and this patch being first would answer my question about why the pin
would not be active. Or a better explanation in patch 0/X about the
whole series.

      Andrew

[PATCH net-next v3 7/7] net: dsa: microchip: do not shut down the switch if WoL is active

[Oleksij Rempel]

For Wake on Lan we should not reconfigure, reset or power down the
switch on shut down sequence.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477_i2c.c |  3 +++
 drivers/net/dsa/microchip/ksz_common.c  | 18 ++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h  |  2 ++
 drivers/net/dsa/microchip/ksz_spi.c     |  3 +++
 4 files changed, 26 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 2710afad4f3a..fe818742051c 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -66,6 +66,9 @@ static void ksz9477_i2c_shutdown(struct i2c_client *i2c)
 	if (!dev)
 		return;
 
+	if (ksz_wol_is_active(dev))
+		return;
+
 	if (dev->dev_ops->reset)
 		dev->dev_ops->reset(dev);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 820edda82cea..c3669b9cc6ce 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2959,6 +2959,24 @@ static int ksz_set_wol(struct dsa_switch *ds, int port,
 	return -EOPNOTSUPP;
 }
 
+bool ksz_wol_is_active(struct ksz_device *dev)
+{
+	struct dsa_port *dp;
+
+	if (!dev->wakeup_source)
+		return false;
+
+	dsa_switch_for_each_user_port(dp, dev->ds) {
+		struct ethtool_wolinfo wol;
+
+		ksz_get_wol(dev->ds, dp->index, &wol);
+		if (wol.wolopts)
+			return true;
+	}
+
+	return false;
+}
+
 static void ksz_set_xmii(struct ksz_device *dev, int port,
 			 phy_interface_t interface)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 80679f38ee12..84b1eed8cd2a 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -397,6 +397,8 @@ bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
 extern const struct ksz_chip_data ksz_switch_chips[];
 
+bool ksz_wol_is_active(struct ksz_device *dev);
+
 /* Common register access functions */
 static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
 {
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 279338451621..c5d9c3d86ddb 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -114,6 +114,9 @@ static void ksz_spi_shutdown(struct spi_device *spi)
 	if (!dev)
 		return;
 
+	if (ksz_wol_is_active(dev))
+		return;
+
 	if (dev->dev_ops->reset)
 		dev->dev_ops->reset(dev);
 
-- 
2.39.2

Re: [PATCH net-next v3 7/7] net: dsa: microchip: do not shut down the switch if WoL is active

[Vladimir Oltean]

On Fri, Oct 13, 2023 at 02:24:05PM +0200, Oleksij Rempel wrote:
> For Wake on Lan we should not reconfigure, reset or power down the
> switch on shut down sequence.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz9477_i2c.c |  3 +++
>  drivers/net/dsa/microchip/ksz_common.c  | 18 ++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.h  |  2 ++
>  drivers/net/dsa/microchip/ksz_spi.c     |  3 +++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 2710afad4f3a..fe818742051c 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -66,6 +66,9 @@ static void ksz9477_i2c_shutdown(struct i2c_client *i2c)
>  	if (!dev)
>  		return;
>  
> +	if (ksz_wol_is_active(dev))
> +		return;
> +
>  	if (dev->dev_ops->reset)
>  		dev->dev_ops->reset(dev);

The rest of the code used to read:

	dsa_switch_shutdown(dev->ds);

	i2c_set_clientdata(i2c, NULL);
}

and that part is non-optional. Please only skip the dev->dev_ops->reset()
call if ksz_wol_is_active().

>  static void ksz_set_xmii(struct ksz_device *dev, int port,
>  			 phy_interface_t interface)
>  {
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 80679f38ee12..84b1eed8cd2a 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -397,6 +397,8 @@ bool ksz_get_gbit(struct ksz_device *dev, int port);
>  phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
>  extern const struct ksz_chip_data ksz_switch_chips[];
>  
> +bool ksz_wol_is_active(struct ksz_device *dev);
> +
>  /* Common register access functions */
>  static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
>  {
> diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
> index 279338451621..c5d9c3d86ddb 100644
> --- a/drivers/net/dsa/microchip/ksz_spi.c
> +++ b/drivers/net/dsa/microchip/ksz_spi.c
> @@ -114,6 +114,9 @@ static void ksz_spi_shutdown(struct spi_device *spi)
>  	if (!dev)
>  		return;
>  
> +	if (ksz_wol_is_active(dev))
> +		return;
> +
>  	if (dev->dev_ops->reset)
>  		dev->dev_ops->reset(dev);

Same here. Maybe introducing a common ksz_switch_shutdown() would be
appropriate.