[PATCH net-next v2 0/4] net: macb: WOL enhancements

[PATCH net-next v2 0/4] net: macb: WOL enhancements

[Vineeth Karumanchi]

- Add provisioning for queue tie-off and queue disable during suspend.
- Add WOL caps to support magic-packet and ARP modes.
- Depricate magic-packet property.

Changes in v2:
- Re-implement WOL using CAPS instead of device-tree attribute.
- Deprecate device-tree "magic-packet" property.
- Sorted CAPS values.
- New Bit fields inline with existing implementation.
- Optimize code.
- Fix sparse warnings.
- Addressed minor review comments.

v1 link : https://lore.kernel.org/lkml/20240130104845.3995341-1-vineeth.karumanchi@amd.com/#t

Vineeth Karumanchi (4):
  net: macb: queue tie-off or disable during WOL suspend
  net: macb: Add ARP support to WOL
  net: macb: Enable queue disable and WOL
  dt-bindings: net: cdns,macb: Deprecate magic-packet property

 .../devicetree/bindings/net/cdns,macb.yaml    |   1 +
 drivers/net/ethernet/cadence/macb.h           |   9 ++
 drivers/net/ethernet/cadence/macb_main.c      | 118 +++++++++++++++---
 3 files changed, 109 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/4] net: macb: queue tie-off or disable during WOL suspend

[Vineeth Karumanchi]

When GEM is used as a wake device, it is not mandatory for the RX DMA
to be active. The RX engine in IP only needs to receive and identify
a wake packet through an interrupt. The wake packet is of no further
significance; hence, it is not required to be copied into memory.
By disabling RX DMA during suspend, we can avoid unnecessary DMA
processing of any incoming traffic.

During suspend, perform either of the below operations:

- tie-off/dummy descriptor: Disable unused queues by connecting
  them to a looped descriptor chain without free slots.

- queue disable: The newer IP version allows disabling individual queues.

Co-developed-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      |  7 +++
 drivers/net/ethernet/cadence/macb_main.c | 60 ++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index aa5700ac9c00..50cd35ef21ad 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -645,6 +645,10 @@
 #define GEM_T2OFST_OFFSET			0 /* offset value */
 #define GEM_T2OFST_SIZE				7
 
+/* Bitfields in queue pointer registers */
+#define MACB_QUEUE_DISABLE_OFFSET		0 /* disable queue */
+#define MACB_QUEUE_DISABLE_SIZE			1
+
 /* Offset for screener type 2 compare values (T2CMPOFST).
  * Note the offset is applied after the specified point,
  * e.g. GEM_T2COMPOFST_ETYPE denotes the EtherType field, so an offset
@@ -733,6 +737,7 @@
 #define MACB_CAPS_NEEDS_RSTONUBR		0x00000100
 #define MACB_CAPS_MIIONRGMII			0x00000200
 #define MACB_CAPS_NEED_TSUCLK			0x00000400
+#define MACB_CAPS_QUEUE_DISABLE			0x00000800
 #define MACB_CAPS_PCS				0x01000000
 #define MACB_CAPS_HIGH_SPEED			0x02000000
 #define MACB_CAPS_CLK_HW_CHG			0x04000000
@@ -1254,6 +1259,8 @@ struct macb {
 	u32	(*macb_reg_readl)(struct macb *bp, int offset);
 	void	(*macb_reg_writel)(struct macb *bp, int offset, u32 value);
 
+	struct macb_dma_desc	*rx_ring_tieoff;
+	dma_addr_t		rx_ring_tieoff_dma;
 	size_t			rx_buffer_size;
 
 	unsigned int		rx_ring_size;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 898debfd4db3..f34933ef03b0 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2477,6 +2477,12 @@ static void macb_free_consistent(struct macb *bp)
 	unsigned int q;
 	int size;
 
+	if (bp->rx_ring_tieoff) {
+		dma_free_coherent(&bp->pdev->dev, macb_dma_desc_get_size(bp),
+				  bp->rx_ring_tieoff, bp->rx_ring_tieoff_dma);
+		bp->rx_ring_tieoff = NULL;
+	}
+
 	bp->macbgem_ops.mog_free_rx_buffers(bp);
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
@@ -2568,6 +2574,16 @@ static int macb_alloc_consistent(struct macb *bp)
 	if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
 		goto out_err;
 
+	/* Required for tie off descriptor for PM cases */
+	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE)) {
+		bp->rx_ring_tieoff = dma_alloc_coherent(&bp->pdev->dev,
+							macb_dma_desc_get_size(bp),
+							&bp->rx_ring_tieoff_dma,
+							GFP_KERNEL);
+		if (!bp->rx_ring_tieoff)
+			goto out_err;
+	}
+
 	return 0;
 
 out_err:
@@ -2575,6 +2591,19 @@ static int macb_alloc_consistent(struct macb *bp)
 	return -ENOMEM;
 }
 
+static void macb_init_tieoff(struct macb *bp)
+{
+	struct macb_dma_desc *desc = bp->rx_ring_tieoff;
+
+	if (bp->caps & MACB_CAPS_QUEUE_DISABLE)
+		return;
+	/* Setup a wrapping descriptor with no free slots
+	 * (WRAP and USED) to tie off/disable unused RX queues.
+	 */
+	macb_set_addr(bp, desc, MACB_BIT(RX_WRAP) | MACB_BIT(RX_USED));
+	desc->ctrl = 0;
+}
+
 static void gem_init_rings(struct macb *bp)
 {
 	struct macb_queue *queue;
@@ -2598,6 +2627,7 @@ static void gem_init_rings(struct macb *bp)
 		gem_rx_refill(queue);
 	}
 
+	macb_init_tieoff(bp);
 }
 
 static void macb_init_rings(struct macb *bp)
@@ -2615,6 +2645,8 @@ static void macb_init_rings(struct macb *bp)
 	bp->queues[0].tx_head = 0;
 	bp->queues[0].tx_tail = 0;
 	desc->ctrl |= MACB_BIT(TX_WRAP);
+
+	macb_init_tieoff(bp);
 }
 
 static void macb_reset_hw(struct macb *bp)
@@ -5215,6 +5247,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	unsigned long flags;
 	unsigned int q;
 	int err;
+	u32 tmp;
 
 	if (!device_may_wakeup(&bp->dev->dev))
 		phy_exit(bp->sgmii_phy);
@@ -5224,17 +5257,38 @@ static int __maybe_unused macb_suspend(struct device *dev)
 
 	if (bp->wol & MACB_WOL_ENABLED) {
 		spin_lock_irqsave(&bp->lock, flags);
-		/* Flush all status bits */
-		macb_writel(bp, TSR, -1);
-		macb_writel(bp, RSR, -1);
+
+		/* Disable Tx and Rx engines before  disabling the queues,
+		 * this is mandatory as per the IP spec sheet
+		 */
+		tmp = macb_readl(bp, NCR);
+		macb_writel(bp, NCR, tmp & ~(MACB_BIT(TE) | MACB_BIT(RE)));
 		for (q = 0, queue = bp->queues; q < bp->num_queues;
 		     ++q, ++queue) {
+			/* Disable RX queues */
+			if (bp->caps & MACB_CAPS_QUEUE_DISABLE) {
+				queue_writel(queue, RBQP, MACB_BIT(QUEUE_DISABLE));
+			} else {
+				/* Tie off RX queues */
+				queue_writel(queue, RBQP,
+					     lower_32_bits(bp->rx_ring_tieoff_dma));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+				queue_writel(queue, RBQPH,
+					     upper_32_bits(bp->rx_ring_tieoff_dma));
+#endif
+			}
 			/* Disable all interrupts */
 			queue_writel(queue, IDR, -1);
 			queue_readl(queue, ISR);
 			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 				queue_writel(queue, ISR, -1);
 		}
+		/* Enable Receive engine */
+		macb_writel(bp, NCR, tmp | MACB_BIT(RE));
+		/* Flush all status bits */
+		macb_writel(bp, TSR, -1);
+		macb_writel(bp, RSR, -1);
+
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
 		 */
-- 
2.34.1

[PATCH net-next v2 2/4] net: macb: Add ARP support to WOL

[Vineeth Karumanchi]

-Add wake-on LAN support using ARP with the provision to select
 through ethtool. Advertise wakeup capability in the probe and
 get the supported modes from OS policy (MACB_CAPS_WOL).

-Re-order MACB_WOL_<> macros for ease of extension.
-Add ARP support configurable through ethtool, "wolopts" variable in
 struct macb contains the current WOL options configured through ethtool.

-For WOL via ARP, ensure the IP address is assigned and
 report an error otherwise.

Co-developed-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      |  2 +
 drivers/net/ethernet/cadence/macb_main.c | 52 +++++++++++++++++-------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 50cd35ef21ad..c9ca61959f3c 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -738,6 +738,7 @@
 #define MACB_CAPS_MIIONRGMII			0x00000200
 #define MACB_CAPS_NEED_TSUCLK			0x00000400
 #define MACB_CAPS_QUEUE_DISABLE			0x00000800
+#define MACB_CAPS_WOL				0x00001000
 #define MACB_CAPS_PCS				0x01000000
 #define MACB_CAPS_HIGH_SPEED			0x02000000
 #define MACB_CAPS_CLK_HW_CHG			0x04000000
@@ -1306,6 +1307,7 @@ struct macb {
 	unsigned int		jumbo_max_len;
 
 	u32			wol;
+	u32			wolopts;
 
 	/* holds value of rx watermark value for pbuf_rxcutthru register */
 	u32			rx_watermark;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f34933ef03b0..62d796ef4035 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -38,6 +38,7 @@
 #include <linux/ptp_classify.h>
 #include <linux/reset.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/inetdevice.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -84,8 +85,9 @@ struct sifive_fu540_macb_mgmt {
 #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
 #define MACB_NETIF_LSO		NETIF_F_TSO
 
-#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
-#define MACB_WOL_ENABLED		(0x1 << 1)
+#define MACB_WOL_ENABLED		(0x1 << 0)
+#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 1)
+#define MACB_WOL_HAS_ARP_PACKET		(0x1 << 2)
 
 #define HS_SPEED_10000M			4
 #define MACB_SERDES_RATE_10G		1
@@ -3278,18 +3280,18 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 {
 	struct macb *bp = netdev_priv(netdev);
 
-	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
+	if (bp->wol & (MACB_WOL_HAS_MAGIC_PACKET | MACB_WOL_HAS_ARP_PACKET))
 		phylink_ethtool_get_wol(bp->phylink, wol);
-		wol->supported |= WAKE_MAGIC;
-
-		if (bp->wol & MACB_WOL_ENABLED)
-			wol->wolopts |= WAKE_MAGIC;
-	}
+	wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
+	wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;
+	/* Pass wolopts to ethtool */
+	wol->wolopts = bp->wolopts;
 }
 
 static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 {
 	struct macb *bp = netdev_priv(netdev);
+	bp->wolopts = 0;
 	int ret;
 
 	/* Pass the order to phylink layer */
@@ -3300,11 +3302,14 @@ static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (!ret || ret != -EOPNOTSUPP)
 		return ret;
 
-	if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
-	    (wol->wolopts & ~WAKE_MAGIC))
+	if (!(bp->wol & (MACB_WOL_HAS_MAGIC_PACKET | MACB_WOL_HAS_ARP_PACKET)) ||
+	    (wol->wolopts & ~(WAKE_MAGIC | WAKE_ARP)))
 		return -EOPNOTSUPP;
 
-	if (wol->wolopts & WAKE_MAGIC)
+	bp->wolopts |= (wol->wolopts & WAKE_MAGIC) ? WAKE_MAGIC : 0;
+	bp->wolopts |= (wol->wolopts & WAKE_ARP) ? WAKE_ARP : 0;
+
+	if (bp->wolopts)
 		bp->wol |= MACB_WOL_ENABLED;
 	else
 		bp->wol &= ~MACB_WOL_ENABLED;
@@ -5087,7 +5092,6 @@ static int macb_probe(struct platform_device *pdev)
 	bp->wol = 0;
 	if (of_property_read_bool(np, "magic-packet"))
 		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
-	device_set_wakeup_capable(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
 
 	bp->usrio = macb_config->usrio;
 
@@ -5115,6 +5119,11 @@ static int macb_probe(struct platform_device *pdev)
 	/* setup capabilities */
 	macb_configure_caps(bp, macb_config);
 
+	if (bp->caps & MACB_CAPS_WOL)
+		bp->wol |= (MACB_WOL_HAS_ARP_PACKET | MACB_WOL_HAS_MAGIC_PACKET);
+
+	device_set_wakeup_capable(&pdev->dev, (bp->wol) ? true : false);
+
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 	if (GEM_BFEXT(DAW64, gem_readl(bp, DCFG6))) {
 		dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(44));
@@ -5244,6 +5253,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 	struct macb_queue *queue;
+	struct in_ifaddr *ifa;
 	unsigned long flags;
 	unsigned int q;
 	int err;
@@ -5256,6 +5266,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		return 0;
 
 	if (bp->wol & MACB_WOL_ENABLED) {
+		/* Check for IP address in WOL ARP mode */
+		ifa = rcu_dereference(__in_dev_get_rcu(bp->dev)->ifa_list);
+		if ((bp->wolopts & WAKE_ARP) && !ifa) {
+			netdev_err(netdev, "IP address not assigned\n");
+			return -EOPNOTSUPP;
+		}
 		spin_lock_irqsave(&bp->lock, flags);
 
 		/* Disable Tx and Rx engines before  disabling the queues,
@@ -5289,6 +5305,14 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		macb_writel(bp, TSR, -1);
 		macb_writel(bp, RSR, -1);
 
+		tmp = (bp->wolopts & WAKE_MAGIC) ? MACB_BIT(MAG) : 0;
+		if (bp->wolopts & WAKE_ARP) {
+			tmp |= MACB_BIT(ARP);
+			/* write IP address into register */
+			tmp |= MACB_BFEXT(IP,
+					 (__force u32)(cpu_to_be32p((uint32_t *)&ifa->ifa_local)));
+		}
+
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
 		 */
@@ -5304,7 +5328,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 				return err;
 			}
 			queue_writel(bp->queues, IER, GEM_BIT(WOL));
-			gem_writel(bp, WOL, MACB_BIT(MAG));
+			gem_writel(bp, WOL, tmp);
 		} else {
 			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
 					       IRQF_SHARED, netdev->name, bp->queues);
@@ -5316,7 +5340,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 				return err;
 			}
 			queue_writel(bp->queues, IER, MACB_BIT(WOL));
-			macb_writel(bp, WOL, MACB_BIT(MAG));
+			macb_writel(bp, WOL, tmp);
 		}
 		spin_unlock_irqrestore(&bp->lock, flags);
 
-- 
2.34.1

[PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Vineeth Karumanchi]

- Enable WOL for ZynqMP devices.
- Enable queue disable and WOL for Versal devices.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 62d796ef4035..55d8c1f3ee80 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4903,7 +4903,7 @@ static const struct macb_config np4_config = {
 
 static const struct macb_config zynqmp_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
-		MACB_CAPS_JUMBO |
+		MACB_CAPS_JUMBO | MACB_CAPS_WOL |
 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
@@ -4954,7 +4954,9 @@ static const struct macb_config sama7g5_emac_config = {
 
 static const struct macb_config versal_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_NEED_TSUCLK |
+		MACB_CAPS_WOL,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
-- 
2.34.1

[PATCH net-next v2 4/4] dt-bindings: net: cdns,macb: Deprecate magic-packet property

[Vineeth Karumanchi]

WOL modes such as magic-packet should be an OS policy.
Leverage MACB_CAPS_WOL on supported devices to advertise
supported modes through ethtool. Use ethtool to activate
the required mode.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index bf8894a0257e..096309bf3235 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -141,6 +141,7 @@ patternProperties:
 
       magic-packet:
         type: boolean
+        deprecated: true
         description:
           Indicates that the hardware supports waking up via magic packet.
 
-- 
2.34.1

Re: [PATCH net-next v2 4/4] dt-bindings: net: cdns,macb: Deprecate magic-packet property

[Krzysztof Kozlowski]

On 22/02/2024 16:38, Vineeth Karumanchi wrote:
> WOL modes such as magic-packet should be an OS policy.
> Leverage MACB_CAPS_WOL on supported devices to advertise
> supported modes through ethtool. Use ethtool to activate
> the required mode.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH net-next v2 4/4] dt-bindings: net: cdns,macb: Deprecate magic-packet property

[Rob Herring]

On Thu, Feb 22, 2024 at 09:08:48PM +0530, Vineeth Karumanchi wrote:
> WOL modes such as magic-packet should be an OS policy.
> Leverage MACB_CAPS_WOL on supported devices to advertise
> supported modes through ethtool. Use ethtool to activate
> the required mode.

According to the description of the property, it indicates whether magic 
packet is supported by the h/w or not. Enabling WOL or not is a separate 
matter. OTOH, I would think WOL support would be implied by compatible 
properties. No objection from me, just highlighting the distinction.

> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bf8894a0257e..096309bf3235 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -141,6 +141,7 @@ patternProperties:
>  
>        magic-packet:
>          type: boolean
> +        deprecated: true
>          description:
>            Indicates that the hardware supports waking up via magic packet.
>  
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 2/4] net: macb: Add ARP support to WOL

[Andrew Lunn]

>  	u32			wol;
> +	u32			wolopts;

> +	wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
> +	wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;

> +	if (bp->caps & MACB_CAPS_WOL)
> +		bp->wol |= (MACB_WOL_HAS_ARP_PACKET | MACB_WOL_HAS_MAGIC_PACKET);

So bp->wol is the capabilities of the hardware? 

And bp->wolopts is what has been enabled via ethtool?

I just wounder if it would be easier to understand is bp->wol was
renamed wol_caps, similar to bp->caps?

	Andrew

Re: [PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Andrew Lunn]

On Thu, Feb 22, 2024 at 09:08:47PM +0530, Vineeth Karumanchi wrote:
> - Enable WOL for ZynqMP devices.
> - Enable queue disable and WOL for Versal devices.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 62d796ef4035..55d8c1f3ee80 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4903,7 +4903,7 @@ static const struct macb_config np4_config = {
>  
>  static const struct macb_config zynqmp_config = {
>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
> -		MACB_CAPS_JUMBO |
> +		MACB_CAPS_JUMBO | MACB_CAPS_WOL |
>  		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
> @@ -4954,7 +4954,9 @@ static const struct macb_config sama7g5_emac_config = {
>  
>  static const struct macb_config versal_config = {
>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> -		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
> +		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> +		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_NEED_TSUCLK |
> +		MACB_CAPS_WOL,

So WoL is a proprietary extension only available in AMD versions of
the MACB?

    Andrew

Re: [PATCH net-next v2 2/4] net: macb: Add ARP support to WOL

[Vineeth Karumanchi]

Hi Andrew,

On 23/02/24 1:02 am, Andrew Lunn wrote:
>>   	u32			wol;
>> +	u32			wolopts;
> 
>> +	wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
>> +	wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;
> 
>> +	if (bp->caps & MACB_CAPS_WOL)
>> +		bp->wol |= (MACB_WOL_HAS_ARP_PACKET | MACB_WOL_HAS_MAGIC_PACKET);
> 
> So bp->wol is the capabilities of the hardware?
> 

Yes, it holds the supported capabilities.

> And bp->wolopts is what has been enabled via ethtool?
> 

Yes, it holds the selected options through ethtool.

> I just wounder if it would be easier to understand is bp->wol was
> renamed wol_caps, similar to bp->caps?
> 
> 	Andrew

Sure, I will make the change.

🙏 vineeth

Re: [PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Vineeth Karumanchi]

Hi Andrew,

On 23/02/24 1:04 am, Andrew Lunn wrote:
> On Thu, Feb 22, 2024 at 09:08:47PM +0530, Vineeth Karumanchi wrote:
>> - Enable WOL for ZynqMP devices.
>> - Enable queue disable and WOL for Versal devices.
>>
>> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 62d796ef4035..55d8c1f3ee80 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -4903,7 +4903,7 @@ static const struct macb_config np4_config = {
>>   
>>   static const struct macb_config zynqmp_config = {
>>   	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>> -		MACB_CAPS_JUMBO |
>> +		MACB_CAPS_JUMBO | MACB_CAPS_WOL |
>>   		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>>   	.dma_burst_length = 16,
>>   	.clk_init = macb_clk_init,
>> @@ -4954,7 +4954,9 @@ static const struct macb_config sama7g5_emac_config = {
>>   
>>   static const struct macb_config versal_config = {
>>   	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>> -		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
>> +		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
>> +		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_NEED_TSUCLK |
>> +		MACB_CAPS_WOL,
> 
> So WoL is a proprietary extension only available in AMD versions of
> the MACB?
> 

It is not specific to AMD versions. All Cadence GEM IP versions have the 
capability, but specific vendors might enable or disable it as per their 
requirements.

WOL was previously enabled via the device-tree attribute. Some users 
might not leverage it.

However, we can add MACB_CAPS_WOL to "default_gem_config", but we don't 
know which devices have WOL support.

Please let me know your suggestions.

Vineeth
>      Andrew

Re: [PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Andrew Lunn]

> It is not specific to AMD versions. All Cadence GEM IP versions have the
> capability, but specific vendors might enable or disable it as per their
> requirements.

Do you mean it is an option to synthesizer it or not? So although the
basic IP licensed from Cadence has it, a silicon vendor could remove
it?

> WOL was previously enabled via the device-tree attribute. Some users might
> not leverage it.

This is not typical. If the hardware supports it, we let the end user
decided if they want to use it or not.

So if all silicon should have it, enable it everywhere. If there is an
option to save some gates and leave it out of the silicon, then we do
need some per device knowledge, or a register which tells us what the
synthesis options where.

	Andrew

Re: [PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Karumanchi, Vineeth]

Hi Andrew,

On 2/23/2024 6:56 PM, Andrew Lunn wrote:
>> It is not specific to AMD versions. All Cadence GEM IP versions have the
>> capability, but specific vendors might enable or disable it as per their
>> requirements.
> 
> Do you mean it is an option to synthesizer it or not? So although the
> basic IP licensed from Cadence has it, a silicon vendor could remove
> it?
> 

Regarding that, we are unsure. However, based on observation from all 
previous cadence IP's in AMD Soc's, this feature was available.

>> WOL was previously enabled via the device-tree attribute. Some users might
>> not leverage it.
> 
> This is not typical. If the hardware supports it, we let the end user
> decided if they want to use it or not.
> 
> So if all silicon should have it, enable it everywhere. If there is an
> option to save some gates and leave it out of the silicon, then we do
> need some per device knowledge, or a register which tells us what the
> synthesis options where.
> 

I have looked into all config_debug* registers of multiple versions 
available with us and there is no mention of WOL. I think we can add 
MACB_WOL_CAPS to default_config and advertise in ethtool by default.

Please let me know your suggestions/thoughts.
-- 
🙏 vineeth

Re: [PATCH net-next v2 3/4] net: macb: Enable queue disable and WOL

[Karumanchi, Vineeth]

Hi Andrew,

On 2/27/2024 10:34 AM, Karumanchi, Vineeth wrote:
> Hi Andrew,
> 
> On 2/23/2024 6:56 PM, Andrew Lunn wrote:
>>> It is not specific to AMD versions. All Cadence GEM IP versions have the
>>> capability, but specific vendors might enable or disable it as per their
>>> requirements.
>>
>> Do you mean it is an option to synthesizer it or not? So although the
>> basic IP licensed from Cadence has it, a silicon vendor could remove
>> it?
>>
> 
> Regarding that, we are unsure. However, based on observation from all 
> previous cadence IP's in AMD Soc's, this feature was available.
> 
>>> WOL was previously enabled via the device-tree attribute. Some users 
>>> might
>>> not leverage it.
>>
>> This is not typical. If the hardware supports it, we let the end user
>> decided if they want to use it or not.
>>
>> So if all silicon should have it, enable it everywhere. If there is an
>> option to save some gates and leave it out of the silicon, then we do
>> need some per device knowledge, or a register which tells us what the
>> synthesis options where.
>>
> 
> I have looked into all config_debug* registers of multiple versions 
> available with us and there is no mention of WOL. I think we can add 
> MACB_WOL_CAPS to default_config and advertise in ethtool by default.
> 
> Please let me know your suggestions/thoughts.


Adding WOL caps to default_gem_config is not effective.
The individual config is overriding default_config.

I only see enabling WOL by default,
- By default, Advertise magic-packet and ARP in ethtool.
- Always set device_set_wakeup_capable to true.
- "magic-packet" property is no longer useful.

Please let me know your suggestions.

-- 
🙏 vineeth

Re: [PATCH net-next v2 2/4] net: macb: Add ARP support to WOL

[claudiu beznea]

On 22.02.2024 17:38, Vineeth Karumanchi wrote:
> -Add wake-on LAN support using ARP with the provision to select
>  through ethtool. Advertise wakeup capability in the probe and
>  get the supported modes from OS policy (MACB_CAPS_WOL).
> 
> -Re-order MACB_WOL_<> macros for ease of extension.
> -Add ARP support configurable through ethtool, "wolopts" variable in
>  struct macb contains the current WOL options configured through ethtool.
> 
> -For WOL via ARP, ensure the IP address is assigned and
>  report an error otherwise.

Having '-' for each thing that you did makes the 1st time reader of this
commit message think that you did multiple things in this patch, which
should be avoided.

Also, please compose the commit message such that it responds to the
questions "what the patch does?" and "why it's necessary?"

> 
> Co-developed-by: Harini Katakam <harini.katakam@amd.com>
> Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |  2 +
>  drivers/net/ethernet/cadence/macb_main.c | 52 +++++++++++++++++-------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 50cd35ef21ad..c9ca61959f3c 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -738,6 +738,7 @@
>  #define MACB_CAPS_MIIONRGMII			0x00000200
>  #define MACB_CAPS_NEED_TSUCLK			0x00000400
>  #define MACB_CAPS_QUEUE_DISABLE			0x00000800
> +#define MACB_CAPS_WOL				0x00001000
>  #define MACB_CAPS_PCS				0x01000000
>  #define MACB_CAPS_HIGH_SPEED			0x02000000
>  #define MACB_CAPS_CLK_HW_CHG			0x04000000
> @@ -1306,6 +1307,7 @@ struct macb {
>  	unsigned int		jumbo_max_len;
>  
>  	u32			wol;
> +	u32			wolopts;
>  
>  	/* holds value of rx watermark value for pbuf_rxcutthru register */
>  	u32			rx_watermark;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f34933ef03b0..62d796ef4035 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -38,6 +38,7 @@
>  #include <linux/ptp_classify.h>
>  #include <linux/reset.h>
>  #include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/inetdevice.h>
>  #include "macb.h"
>  
>  /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -84,8 +85,9 @@ struct sifive_fu540_macb_mgmt {
>  #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
>  #define MACB_NETIF_LSO		NETIF_F_TSO
>  
> -#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
> -#define MACB_WOL_ENABLED		(0x1 << 1)

> +#define MACB_WOL_ENABLED		(0x1 << 0)> +#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 1)

Is there a reason you changed the values of these 2 macros?

> +#define MACB_WOL_HAS_ARP_PACKET		(0x1 << 2)
>  
>  #define HS_SPEED_10000M			4
>  #define MACB_SERDES_RATE_10G		1
> @@ -3278,18 +3280,18 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>  {
>  	struct macb *bp = netdev_priv(netdev);
>  
> -	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
> +	if (bp->wol & (MACB_WOL_HAS_MAGIC_PACKET | MACB_WOL_HAS_ARP_PACKET))
>  		phylink_ethtool_get_wol(bp->phylink, wol);
> -		wol->supported |= WAKE_MAGIC;
> -
> -		if (bp->wol & MACB_WOL_ENABLED)
> -			wol->wolopts |= WAKE_MAGIC;
> -	}
> +	wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
> +	wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;
> +	/* Pass wolopts to ethtool */
> +	wol->wolopts = bp->wolopts;
>  }
>  
>  static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>  {
>  	struct macb *bp = netdev_priv(netdev);
> +	bp->wolopts = 0;
>  	int ret;
>  
>  	/* Pass the order to phylink layer */
> @@ -3300,11 +3302,14 @@ static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>  	if (!ret || ret != -EOPNOTSUPP)
>  		return ret;
>  
> -	if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
> -	    (wol->wolopts & ~WAKE_MAGIC))
> +	if (!(bp->wol & (MACB_WOL_HAS_MAGIC_PACKET | MACB_WOL_HAS_ARP_PACKET)) ||
> +	    (wol->wolopts & ~(WAKE_MAGIC | WAKE_ARP)))
>  		return -EOPNOTSUPP;
>  
> -	if (wol->wolopts & WAKE_MAGIC)
> +	bp->wolopts |= (wol->wolopts & WAKE_MAGIC) ? WAKE_MAGIC : 0;
> +	bp->wolopts |= (wol->wolopts & WAKE_ARP) ? WAKE_ARP : 0;
> +
> +	if (bp->wolopts)
>  		bp->wol |= MACB_WOL_ENABLED;
>  	else
>  		bp->wol &= ~MACB_WOL_ENABLED;
> @@ -5087,7 +5092,6 @@ static int macb_probe(struct platform_device *pdev)
>  	bp->wol = 0;
>  	if (of_property_read_bool(np, "magic-packet"))
>  		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
> -	device_set_wakeup_capable(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
>  
>  	bp->usrio = macb_config->usrio;
>  
> @@ -5115,6 +5119,11 @@ static int macb_probe(struct platform_device *pdev)
>  	/* setup capabilities */
>  	macb_configure_caps(bp, macb_config);
>  
> +	if (bp->caps & MACB_CAPS_WOL)
> +		bp->wol |= (MACB_WOL_HAS_ARP_PACKET | MACB_WOL_HAS_MAGIC_PACKET);
> +
> +	device_set_wakeup_capable(&pdev->dev, (bp->wol) ? true : false);

It can be simplified with:

device_set_wakeup_capable(&pdev->dev, !!bp->wol);

> +
>  #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  	if (GEM_BFEXT(DAW64, gem_readl(bp, DCFG6))) {
>  		dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(44));
> @@ -5244,6 +5253,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
>  	struct macb_queue *queue;
> +	struct in_ifaddr *ifa;
>  	unsigned long flags;
>  	unsigned int q;
>  	int err;
> @@ -5256,6 +5266,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		return 0;
>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
> +		/* Check for IP address in WOL ARP mode */
> +		ifa = rcu_dereference(__in_dev_get_rcu(bp->dev)->ifa_list);
> +		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> +			netdev_err(netdev, "IP address not assigned\n");
> +			return -EOPNOTSUPP;
> +		}
>  		spin_lock_irqsave(&bp->lock, flags);
>  
>  		/* Disable Tx and Rx engines before  disabling the queues,
> @@ -5289,6 +5305,14 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		macb_writel(bp, TSR, -1);
>  		macb_writel(bp, RSR, -1);
>  
> +		tmp = (bp->wolopts & WAKE_MAGIC) ? MACB_BIT(MAG) : 0;
> +		if (bp->wolopts & WAKE_ARP) {
> +			tmp |= MACB_BIT(ARP);
> +			/* write IP address into register */
> +			tmp |= MACB_BFEXT(IP,
> +					 (__force u32)(cpu_to_be32p((uint32_t *)&ifa->ifa_local)));
> +		}
> +
>  		/* Change interrupt handler and
>  		 * Enable WoL IRQ on queue 0
>  		 */
> @@ -5304,7 +5328,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				return err;
>  			}
>  			queue_writel(bp->queues, IER, GEM_BIT(WOL));
> -			gem_writel(bp, WOL, MACB_BIT(MAG));
> +			gem_writel(bp, WOL, tmp);
>  		} else {
>  			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
>  					       IRQF_SHARED, netdev->name, bp->queues);
> @@ -5316,7 +5340,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				return err;
>  			}
>  			queue_writel(bp->queues, IER, MACB_BIT(WOL));
> -			macb_writel(bp, WOL, MACB_BIT(MAG));
> +			macb_writel(bp, WOL, tmp);
>  		}
>  		spin_unlock_irqrestore(&bp->lock, flags);
>