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

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

[Oleksij Rempel]

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 (6):
  dt-bindings: net: dsa: microchip: add wakeup-source property
  dt-bindings: net: dsa: microchip: add local-mac-address property
    support
  net: dsa: microchip: ksz9477: add Wake on LAN support
  net: dsa: microchip: ksz9477: add Wake on PHY event support
  net: dsa: microchip: use wakeup-source DT property to enable PME
    output
  net: dsa: microchip: ksz9477: make switch MAC address configurable

 .../bindings/net/dsa/microchip,ksz.yaml       |  3 +
 drivers/net/dsa/microchip/ksz9477.c           | 86 +++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h           |  4 +
 drivers/net/dsa/microchip/ksz_common.c        | 51 +++++++++++
 drivers/net/dsa/microchip/ksz_common.h        |  3 +
 5 files changed, 147 insertions(+)

-- 
2.39.2

[PATCH net-next v2 1/6] 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 e51be1ac03623..26385ba624245 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -49,6 +49,8 @@ properties:
       Set if the output SYNCLKO clock should be disabled. Do not mix with
       microchip,synclko-125.
 
+  wakeup-source: true
+
 required:
   - compatible
   - reg
-- 
2.39.2

[PATCH net-next v2 2/6] dt-bindings: net: dsa: microchip: add local-mac-address property support

[Oleksij Rempel]

All KSZ switches have configurable MAC address support which is used for
sending pause frames and for Wake on Magic Packet. So, allow local-mac-address
property in the root of the switch node.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 26385ba624245..fd9a10d0ba28c 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -50,6 +50,7 @@ properties:
       microchip,synclko-125.
 
   wakeup-source: true
+  local-mac-address: true
 
 required:
   - compatible
-- 
2.39.2

[PATCH net-next v2 3/6] 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    | 71 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |  4 ++
 drivers/net/dsa/microchip/ksz_common.c | 41 +++++++++++++++
 3 files changed, 116 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f5d48310cbe8a..d14fcc98ec1e0 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -56,6 +56,74 @@ int ksz9477_change_mtu(struct ksz_device *dev, int port, int mtu)
 				  REG_SW_MTU_MASK, frame_size);
 }
 
+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)
+		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);
+}
+
+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;
+}
+
+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;
@@ -1004,6 +1072,9 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	/* clear pending interrupts */
 	if (dev->info->internal_phy[port])
 		ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
+
+	/* 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 27171482d0556..f972b6e7f1d12 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -58,6 +58,10 @@ int ksz9477_dsa_init(struct ksz_device *dev);
 int ksz9477_switch_init(struct ksz_device *dev);
 void ksz9477_switch_exit(struct ksz_device *dev);
 void ksz9477_port_queue_split(struct ksz_device *dev, int port);
+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);
 int ksz9477_cls_flower_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 2d98c3055f7a0..6adc2c6537a31 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2779,6 +2779,45 @@ 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));
+
+	switch (dev->chip_id) {
+	case KSZ8563_CHIP_ID:
+	case KSZ9477_CHIP_ID:
+	case KSZ9563_CHIP_ID:
+	case KSZ9567_CHIP_ID:
+	case KSZ9893_CHIP_ID:
+	case KSZ9896_CHIP_ID:
+	case KSZ9897_CHIP_ID:
+		ksz9477_get_wol(dev, port, wol);
+		return;
+	}
+}
+
+static int ksz_set_wol(struct dsa_switch *ds, int port,
+		       struct ethtool_wolinfo *wol)
+{
+	struct ksz_device *dev = ds->priv;
+
+	switch (dev->chip_id) {
+	case KSZ8563_CHIP_ID:
+	case KSZ9477_CHIP_ID:
+	case KSZ9563_CHIP_ID:
+	case KSZ9567_CHIP_ID:
+	case KSZ9893_CHIP_ID:
+	case KSZ9896_CHIP_ID:
+	case KSZ9897_CHIP_ID:
+		return ksz9477_set_wol(dev, port, wol);
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static void ksz_set_xmii(struct ksz_device *dev, int port,
 			 phy_interface_t interface)
 {
@@ -3485,6 +3524,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,
-- 
2.39.2

[PATCH net-next v2 4/6] 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 d14fcc98ec1e0..e3143b95a8ec9 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -88,7 +88,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)
@@ -96,6 +96,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;
 }
 
 int ksz9477_set_wol(struct ksz_device *dev, int port,
@@ -104,7 +106,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);
@@ -120,6 +122,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

[PATCH net-next v2 5/6] 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 e3143b95a8ec9..69909241b1f44 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1208,6 +1208,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 6adc2c6537a31..a7b298838932c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3715,6 +3715,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 6dca488fbc5f2..589f8b582a415 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -158,6 +158,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

[PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Oleksij Rempel]

The switch MAC address is used for sending pause frames and for Wake on Magic
Packet. So, make use of local-mac-address property in the switch node
root and configure it in the HW.

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

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 69909241b1f44..427176c1441f7 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds)
 	if (dev->wakeup_source)
 		ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
 
+	if (is_valid_ether_addr(dev->mac_addr)) {
+		int i;
+
+		for (i = 0; i < ETH_ALEN; i++)
+			ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
+				   dev->mac_addr[i]);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index a7b298838932c..4de2456b5a349 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3684,6 +3684,8 @@ int ksz_switch_register(struct ksz_device *dev)
 	for (port_num = 0; port_num < dev->info->port_cnt; ++port_num)
 		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
+		const u8 *mac;
+
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
@@ -3718,6 +3720,11 @@ int ksz_switch_register(struct ksz_device *dev)
 
 		dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
 							   "wakeup-source");
+
+		mac = of_get_property(dev->dev->of_node, "local-mac-address",
+				      NULL);
+		if (mac)
+			memcpy(dev->mac_addr, mac, ETH_ALEN);
 	}
 
 	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 589f8b582a415..bdfcd1dc02ee9 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -171,6 +171,8 @@ struct ksz_device {
 	struct mutex lock_irq;		/* IRQ Access */
 	struct ksz_irq girq;
 	struct ksz_ptp_data ptp_data;
+
+	u8 mac_addr[ETH_ALEN];
 };
 
 /* List of supported models */
-- 
2.39.2

RE: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Tristram.Ha]

> The switch MAC address is used for sending pause frames and for Wake on Magic
> Packet. So, make use of local-mac-address property in the switch node
> root and configure it in the HW.
>

> @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds)
>         if (dev->wakeup_source)
>                 ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
> 
> +       if (is_valid_ether_addr(dev->mac_addr)) {
> +               int i;
> +
> +               for (i = 0; i < ETH_ALEN; i++)
> +                       ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> +                                  dev->mac_addr[i]);
> +       }
> +

> +
> +               mac = of_get_property(dev->dev->of_node, "local-mac-address",
> +                                     NULL);
> +               if (mac)
> +                       memcpy(dev->mac_addr, mac, ETH_ALEN);
 
So the Magic Packet uses the same MAC address for all KSZ9477 switches
if local-mac-address is not defined.  Is that practical in real operating
environment?
The DSA driver used to have an API to set the MAC address to the switch,
but it was removed because nobody used it.

Re: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Vladimir Oltean]

On Fri, Jul 21, 2023 at 10:09:57PM +0000, Tristram.Ha@microchip.com wrote:
> > The switch MAC address is used for sending pause frames and for Wake on Magic
> > Packet. So, make use of local-mac-address property in the switch node
> > root and configure it in the HW.
> >
> 
> > @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds)
> >         if (dev->wakeup_source)
> >                 ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
> > 
> > +       if (is_valid_ether_addr(dev->mac_addr)) {
> > +               int i;
> > +
> > +               for (i = 0; i < ETH_ALEN; i++)
> > +                       ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> > +                                  dev->mac_addr[i]);
> > +       }
> > +
> 
> > +
> > +               mac = of_get_property(dev->dev->of_node, "local-mac-address",
> > +                                     NULL);
> > +               if (mac)
> > +                       memcpy(dev->mac_addr, mac, ETH_ALEN);
>  
> So the Magic Packet uses the same MAC address for all KSZ9477 switches
> if local-mac-address is not defined.  Is that practical in real operating
> environment?

"same address" which is 00:00:00:00:00:00 AFAIU.

Do you mean to ask "what do we do for systems without this device tree
property, don't we want to support Magic Packet for them too?". If so,
I agree, it is a valid concern (although we need to modify it anyway,
to add "wakeup-source", apparently).

Additionally, "local-mac-address" would be overloaded with a secondary
meaning compared to what ethernet-controller.yaml says:

      Specifies the MAC address that was assigned to the network device.

...which this is not.

Since the switch hardware doesn't have a register per user port that
could be kept in sync with the MAC address of the Linux interface, it
means that the address for WOL and for flow control will always require
a special way for the user to find out about it.

I'm thinking, would it be simpler to just program the hardware with the
MAC address of the DSA master? Every DSA master has one; we can track
changes to it; no special device tree properties are needed; it also
kinda makes sense as a MAC SA for flow control as well; DSA user ports
also inherit it if they don't have a local-mac-address of their own.
If there needs to be an override for that auto-selected MAC address, a
device tree property can be added later, in the "microchip," namespace.

But "how does the user find out what MAC address to use for WOL" still
remains a concern, when it's stuffed in the device tree. Is there going
to be a BIOS customized for the KSZ9477 which always fixes up the
local-mac-address of the switch node with what's set there?

> The DSA driver used to have an API to set the MAC address to the switch,
> but it was removed because nobody used it.
> 

Which API is that? "git log -GREG_SW_MAC_ADDR_0 drivers/net/dsa/" came
up with nothing.

Re: [PATCH net-next v2 0/6] net: dsa: microchip: provide Wake on LAN support

[Jakub Kicinski]

On Fri, 21 Jul 2023 15:54:55 +0200 Oleksij Rempel wrote:
> - rebase against latest next

Wrong next? IDK. Still doesn't apply :S
-- 
pw-bot: cr

Re: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Florian Fainelli]

On 7/21/2023 5:19 PM, Vladimir Oltean wrote:
> On Fri, Jul 21, 2023 at 10:09:57PM +0000, Tristram.Ha@microchip.com wrote:
>>> The switch MAC address is used for sending pause frames and for Wake on Magic
>>> Packet. So, make use of local-mac-address property in the switch node
>>> root and configure it in the HW.
>>>
>>
>>> @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds)
>>>          if (dev->wakeup_source)
>>>                  ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
>>>
>>> +       if (is_valid_ether_addr(dev->mac_addr)) {
>>> +               int i;
>>> +
>>> +               for (i = 0; i < ETH_ALEN; i++)
>>> +                       ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
>>> +                                  dev->mac_addr[i]);
>>> +       }
>>> +
>>
>>> +
>>> +               mac = of_get_property(dev->dev->of_node, "local-mac-address",
>>> +                                     NULL);
>>> +               if (mac)
>>> +                       memcpy(dev->mac_addr, mac, ETH_ALEN);
>>   
>> So the Magic Packet uses the same MAC address for all KSZ9477 switches
>> if local-mac-address is not defined.  Is that practical in real operating
>> environment?
> 
> "same address" which is 00:00:00:00:00:00 AFAIU.
> 
> Do you mean to ask "what do we do for systems without this device tree
> property, don't we want to support Magic Packet for them too?". If so,
> I agree, it is a valid concern (although we need to modify it anyway,
> to add "wakeup-source", apparently).
> 
> Additionally, "local-mac-address" would be overloaded with a secondary
> meaning compared to what ethernet-controller.yaml says:
> 
>        Specifies the MAC address that was assigned to the network device.
> 
> ...which this is not.
> 
> Since the switch hardware doesn't have a register per user port that
> could be kept in sync with the MAC address of the Linux interface, it
> means that the address for WOL and for flow control will always require
> a special way for the user to find out about it.
> 
> I'm thinking, would it be simpler to just program the hardware with the
> MAC address of the DSA master? Every DSA master has one; we can track
> changes to it; no special device tree properties are needed; it also
> kinda makes sense as a MAC SA for flow control as well; DSA user ports
> also inherit it if they don't have a local-mac-address of their own.
> If there needs to be an override for that auto-selected MAC address, a
> device tree property can be added later, in the "microchip," namespace.

Yes the MAC address of the DSA master should be used, that is what we 
would naturally want to target magic packets with.

> 
> But "how does the user find out what MAC address to use for WOL" still
> remains a concern, when it's stuffed in the device tree. Is there going
> to be a BIOS customized for the KSZ9477 which always fixes up the
> local-mac-address of the switch node with what's set there?

Agreed, I don't think we should be accepting that 'local-mac-address' be 
specified, it does not serve any good and only makes everything more 
confusing because this is not the MAC address that is set into the 
port's net_device::dev_addr at all.

> 
>> The DSA driver used to have an API to set the MAC address to the switch,
>> but it was removed because nobody used it.
>>
> 
> Which API is that? "git log -GREG_SW_MAC_ADDR_0 drivers/net/dsa/" came
> up with nothing.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=841f4f24053acad69240c6ab7427a1d24bc29491
-- 
Florian

Re: [PATCH net-next v2 6/6] net: dsa: microchip: ksz9477: make switch MAC address configurable

[Andrew Lunn]

> The DSA driver used to have an API to set the MAC address to the switch,
> but it was removed because nobody used it.

That is a long time ago, when Marvell was about the only supported
vendor. As far as i understood, it was used to set the MAC source
address used when sending pause frames. But since pause frames are
link local by definition, and the switches had a reasonable default,
it was removed.

    Andrew

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

[Florian Fainelli]

On 7/21/2023 6:54 AM, Oleksij Rempel wrote:
> 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>

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

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

[Florian Fainelli]

On 7/21/2023 6:54 AM, 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.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

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

[Florian Fainelli]

On 7/21/2023 6:55 AM, 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.

There is a word missing in the above sentence between "System 
Controller" and "can't be", and I believe the word is "that".

> So, add support for this property and enable
> PME switch output if this property is present.

A property that "enables" something means that you are using Device Tree 
as a way to encode some sort of policy, maybe this is  just the wording 
here and what you would want to say is that describing the node with a 
'wakeup-source' property indicates that the device is wake-up capable, 
regardless of the presence/absence of an interrupt parent?

With the commit message reworded:

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