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

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

[Vineeth Karumanchi]

- Add provisioning for queue tie-off and queue disable during suspend.
- Add ARP packet support to WOL.

Vineeth Karumanchi (3):
  net: macb: queue tie-off or disable during WOL suspend
  dt-bindings: net: cdns,macb: Add wol-arp-packet property
  net: macb: Add ARP support to WOL

 .../devicetree/bindings/net/cdns,macb.yaml    |   5 +
 drivers/net/ethernet/cadence/macb.h           |   8 ++
 drivers/net/ethernet/cadence/macb_main.c      | 112 ++++++++++++++++--
 3 files changed, 112 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] 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 | 58 +++++++++++++++++++++++-
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index aa5700ac9c00..f68ff163aa18 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -645,6 +645,9 @@
 #define GEM_T2OFST_OFFSET			0 /* offset value */
 #define GEM_T2OFST_SIZE				7
 
+/* Bitfields in queue pointer registers */
+#define GEM_RBQP_DISABLE    BIT(0)
+
 /* 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
@@ -737,6 +740,7 @@
 #define MACB_CAPS_HIGH_SPEED			0x02000000
 #define MACB_CAPS_CLK_HW_CHG			0x04000000
 #define MACB_CAPS_MACB_IS_EMAC			0x08000000
+#define MACB_CAPS_QUEUE_DISABLE			0x00002000
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000
@@ -1254,6 +1258,7 @@ 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;
 	size_t			rx_buffer_size;
 
 	unsigned int		rx_ring_size;
@@ -1276,6 +1281,8 @@ struct macb {
 		struct gem_stats	gem;
 	}			hw_stats;
 
+	dma_addr_t		rx_ring_tieoff_dma;
+
 	struct macb_or_gem_ops	macbgem_ops;
 
 	struct mii_bus		*mii_bus;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 898debfd4db3..47cbea58e6c3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2479,6 +2479,12 @@ static void macb_free_consistent(struct macb *bp)
 
 	bp->macbgem_ops.mog_free_rx_buffers(bp);
 
+	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;
+	}
+
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		kfree(queue->tx_skb);
 		queue->tx_skb = NULL;
@@ -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,16 @@ static int macb_alloc_consistent(struct macb *bp)
 	return -ENOMEM;
 }
 
+static void macb_init_tieoff(struct macb *bp)
+{
+	struct macb_dma_desc *d = bp->rx_ring_tieoff;
+	/* Setup a wrapping descriptor with no free slots
+	 * (WRAP and USED) to tie off/disable unused RX queues.
+	 */
+	macb_set_addr(bp, d, MACB_BIT(RX_WRAP) | MACB_BIT(RX_USED));
+	d->ctrl = 0;
+}
+
 static void gem_init_rings(struct macb *bp)
 {
 	struct macb_queue *queue;
@@ -2598,6 +2624,8 @@ static void gem_init_rings(struct macb *bp)
 		gem_rx_refill(queue);
 	}
 
+	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
+		macb_init_tieoff(bp);
 }
 
 static void macb_init_rings(struct macb *bp)
@@ -2615,6 +2643,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)
@@ -4917,7 +4947,8 @@ 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,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
@@ -5214,6 +5245,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	struct macb_queue *queue;
 	unsigned long flags;
 	unsigned int q;
+	u32 ctrlmask;
 	int err;
 
 	if (!device_may_wakeup(&bp->dev->dev))
@@ -5224,6 +5256,30 @@ static int __maybe_unused macb_suspend(struct device *dev)
 
 	if (bp->wol & MACB_WOL_ENABLED) {
 		spin_lock_irqsave(&bp->lock, flags);
+
+		/* Disable Tx and Rx engines before  disabling the queues,
+		 * this is mandatory as per the IP spec sheet
+		 */
+		ctrlmask = macb_readl(bp, NCR);
+		ctrlmask &= ~(MACB_BIT(TE) | MACB_BIT(RE));
+		macb_writel(bp, NCR, ctrlmask);
+		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, GEM_RBQP_DISABLE);
+			} else {
+				/* Tie off RX queues */
+				queue_writel(queue, RBQP,
+					     lower_32_bits(bp->rx_ring_tieoff_dma));
+				queue_writel(queue, RBQPH,
+					     upper_32_bits(bp->rx_ring_tieoff_dma));
+			}
+		}
+		/* Enable Receive engine */
+		ctrlmask = macb_readl(bp, NCR);
+		ctrlmask |= MACB_BIT(RE);
+		macb_writel(bp, NCR, ctrlmask);
 		/* Flush all status bits */
 		macb_writel(bp, TSR, -1);
 		macb_writel(bp, RSR, -1);
-- 
2.34.1

[PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Vineeth Karumanchi]

"wol-arp-packet" property enables WOL with ARP packet.
It is an extension to "magic-packet for WOL.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
7c4a1d0cfdc1 net: macb: make magic-packet property generic
which added magic-property support and wol-arp-packet addition
is similar extension.
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index bf8894a0257e..4bea177e85bc 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -144,6 +144,11 @@ patternProperties:
         description:
           Indicates that the hardware supports waking up via magic packet.
 
+      wol-arp-packet:
+        type: boolean
+        description:
+          Indicates that the hardware supports waking up via ARP packet.
+
     unevaluatedProperties: false
 
 required:
-- 
2.34.1

[PATCH net-next 3/3] 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 the device tree.

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      |  1 +
 drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++------
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index f68ff163aa18..db7e95dc56e3 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1306,6 +1306,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 47cbea58e6c3..cbe1a9de692a 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
@@ -3276,19 +3278,21 @@ 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);
+	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
 		wol->supported |= WAKE_MAGIC;
-
-		if (bp->wol & MACB_WOL_ENABLED)
-			wol->wolopts |= WAKE_MAGIC;
-	}
+	if (bp->wol & MACB_WOL_HAS_ARP_PACKET)
+		wol->supported |= WAKE_ARP;
+	/* 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);
 	int ret;
+	bp->wolopts = 0;
 
 	/* Pass the order to phylink layer */
 	ret = phylink_ethtool_set_wol(bp->phylink, wol);
@@ -3298,11 +3302,16 @@ 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 |= WAKE_MAGIC;
+	if (wol->wolopts & WAKE_ARP)
+		bp->wolopts |= WAKE_ARP;
+
+	if (bp->wolopts)
 		bp->wol |= MACB_WOL_ENABLED;
 	else
 		bp->wol &= ~MACB_WOL_ENABLED;
@@ -5086,7 +5095,10 @@ 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);
+	if (of_property_read_bool(np, "wol-arp-packet"))
+		bp->wol |= MACB_WOL_HAS_ARP_PACKET;
+
+	device_set_wakeup_capable(&pdev->dev, (bp->wol) ? true : false);
 
 	bp->usrio = macb_config->usrio;
 
@@ -5243,6 +5255,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;
 	u32 ctrlmask;
@@ -5255,6 +5268,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 = rtnl_dereference(bp->dev->ip_ptr->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,
@@ -5291,6 +5310,17 @@ static int __maybe_unused macb_suspend(struct device *dev)
 			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 				queue_writel(queue, ISR, -1);
 		}
+
+		ctrlmask = 0;
+		if (bp->wolopts & WAKE_MAGIC)
+			ctrlmask = MACB_BIT(MAG);
+		if (bp->wolopts & WAKE_ARP) {
+			ctrlmask |= MACB_BIT(ARP);
+			/* write IP address into register */
+			ctrlmask |= cpu_to_be32p(&ifa->ifa_local)
+						& GENMASK(MACB_IP_SIZE - 1, 0);
+		}
+
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
 		 */
@@ -5306,7 +5336,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, ctrlmask);
 		} else {
 			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
 					       IRQF_SHARED, netdev->name, bp->queues);
@@ -5318,7 +5348,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, ctrlmask);
 		}
 		spin_unlock_irqrestore(&bp->lock, flags);
 
-- 
2.34.1

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
> "wol-arp-packet" property enables WOL with ARP packet.
> It is an extension to "magic-packet for WOL.

If it is an extension to "magic-packet" why does it not depend on
"magic-packet"? Are there systems that would only support the magic arp
packet but a regular magic packet?

> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
> 7c4a1d0cfdc1 net: macb: make magic-packet property generic
> which added magic-property support and wol-arp-packet addition
> is similar extension.
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bf8894a0257e..4bea177e85bc 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -144,6 +144,11 @@ patternProperties:
>          description:
>            Indicates that the hardware supports waking up via magic packet.
>  
> +      wol-arp-packet:

Bikeshedding perhaps, but why not call it "magic-arp-packet" if it has
the same function as the other property here?

Thanks,
Conor.

> +        type: boolean
> +        description:
> +          Indicates that the hardware supports waking up via ARP packet.
> +
>      unevaluatedProperties: false
>  
>  required:
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 3/3] net: macb: Add ARP support to WOL

[Vadim Fedorenko]

On 30/01/2024 10:48, 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 the device tree.
> 
> 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      |  1 +
>   drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++------
>   2 files changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index f68ff163aa18..db7e95dc56e3 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1306,6 +1306,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 47cbea58e6c3..cbe1a9de692a 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
> @@ -3276,19 +3278,21 @@ 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);
> +	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
>   		wol->supported |= WAKE_MAGIC;
> -
> -		if (bp->wol & MACB_WOL_ENABLED)
> -			wol->wolopts |= WAKE_MAGIC;
> -	}
> +	if (bp->wol & MACB_WOL_HAS_ARP_PACKET)
> +		wol->supported |= WAKE_ARP;
> +	/* 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);
>   	int ret;
> +	bp->wolopts = 0;

as there will be another spin, could you please use reverse xmas-tree order.

>   	/* Pass the order to phylink layer */
>   	ret = phylink_ethtool_set_wol(bp->phylink, wol);
> @@ -3298,11 +3302,16 @@ 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 |= WAKE_MAGIC;
> +	if (wol->wolopts & WAKE_ARP)
> +		bp->wolopts |= WAKE_ARP;
> +
> +	if (bp->wolopts)
>   		bp->wol |= MACB_WOL_ENABLED;
>   	else
>   		bp->wol &= ~MACB_WOL_ENABLED;
> @@ -5086,7 +5095,10 @@ 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);
> +	if (of_property_read_bool(np, "wol-arp-packet"))
> +		bp->wol |= MACB_WOL_HAS_ARP_PACKET;
> +
> +	device_set_wakeup_capable(&pdev->dev, (bp->wol) ? true : false);
>   
>   	bp->usrio = macb_config->usrio;
>   
> @@ -5243,6 +5255,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;
>   	u32 ctrlmask;
> @@ -5255,6 +5268,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 = rtnl_dereference(bp->dev->ip_ptr->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,
> @@ -5291,6 +5310,17 @@ static int __maybe_unused macb_suspend(struct device *dev)
>   			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>   				queue_writel(queue, ISR, -1);
>   		}
> +
> +		ctrlmask = 0;
> +		if (bp->wolopts & WAKE_MAGIC)
> +			ctrlmask = MACB_BIT(MAG);
> +		if (bp->wolopts & WAKE_ARP) {
> +			ctrlmask |= MACB_BIT(ARP);
> +			/* write IP address into register */
> +			ctrlmask |= cpu_to_be32p(&ifa->ifa_local)
> +						& GENMASK(MACB_IP_SIZE - 1, 0);
> +		}
> +
>   		/* Change interrupt handler and
>   		 * Enable WoL IRQ on queue 0
>   		 */
> @@ -5306,7 +5336,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, ctrlmask);
>   		} else {
>   			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
>   					       IRQF_SHARED, netdev->name, bp->queues);
> @@ -5318,7 +5348,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, ctrlmask);
>   		}
>   		spin_unlock_irqrestore(&bp->lock, flags);
>   

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Andrew Lunn]

On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
> "wol-arp-packet" property enables WOL with ARP packet.
> It is an extension to "magic-packet for WOL.

It not clear why this is needed. Is this not a standard feature of the
IP? Is there no hardware bit indicating the capability?

    Andrew

Re: [PATCH net-next 0/3] net: macb: WOL enhancements

[Jakub Kicinski]

On Tue, 30 Jan 2024 16:18:42 +0530 Vineeth Karumanchi wrote:
> - Add provisioning for queue tie-off and queue disable during suspend.
> - Add ARP packet support to WOL.

Try to build the driver with sparse enabled:

make C=1 drivers/net/ethernet/cadence/

Looks like you're adding new warnings.
-- 
pw-bot: cr

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Vineeth Karumanchi]

Hi Conor,

On 30/01/24 11:00 pm, Conor Dooley wrote:
> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
>> "wol-arp-packet" property enables WOL with ARP packet.
>> It is an extension to "magic-packet for WOL.
> 
> If it is an extension to "magic-packet" why does it not depend on
> "magic-packet"? Are there systems that would only support the magic arp
> packet but a regular magic packet?
> 

The IP version on ZU+ and Versal supports the below combinations for WOL 
event:

1. Magic packet (Wake-on magic packet only)
2. ARP (Wake-on ARP packet only)
3. Magic packet or ARP (Wake-on magic or ARP packets)

The existing DT binding already has one entry for
wol via magic packet. We are adding ARP packet support to the existing 
implementation.

I will change the commit message in v2.

>>
>> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
>> ---
>> 7c4a1d0cfdc1 net: macb: make magic-packet property generic
>> which added magic-property support and wol-arp-packet addition
>> is similar extension.
>> ---
>>   Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
>> index bf8894a0257e..4bea177e85bc 100644
>> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
>> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
>> @@ -144,6 +144,11 @@ patternProperties:
>>           description:
>>             Indicates that the hardware supports waking up via magic packet.
>>   
>> +      wol-arp-packet:
> 
> Bikeshedding perhaps, but why not call it "magic-arp-packet" if it has
> the same function as the other property here?
> 

Magic packet and ARP packets are two different wol events.
IP supports configuring in the above-mentioned ways.
Hence, I think it would be good to not mix with magic packet.

Please let me know your suggestions/comments.

Thanks,
Vineeth 🙏

> Thanks,
> Conor.
> 
>> +        type: boolean
>> +        description:
>> +          Indicates that the hardware supports waking up via ARP packet.
>> +
>>       unevaluatedProperties: false
>>   
>>   required:
>> -- 
>> 2.34.1
>>

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Vineeth Karumanchi]

Hi Andrew,


On 31/01/24 6:56 am, Andrew Lunn wrote:
> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
>> "wol-arp-packet" property enables WOL with ARP packet.
>> It is an extension to "magic-packet for WOL.
> 
> It not clear why this is needed. Is this not a standard feature of the
> IP? Is there no hardware bit indicating the capability?
>

WOL via both ARP and Magic packet is supported by the IP version on ZU+ 
and Versal. However, user can choose which type of packet to recognize 
as a WOL event - magic packet or ARP. The existing DT binding already 
describes one entry for wol via magic packet. Hence, adding a new packet 
type using the same methodology.


vineeth


>      Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Krzysztof Kozlowski]

On 31/01/2024 08:39, Vineeth Karumanchi wrote:
> Hi Andrew,
> 
> 
> On 31/01/24 6:56 am, Andrew Lunn wrote:
>> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
>>> "wol-arp-packet" property enables WOL with ARP packet.
>>> It is an extension to "magic-packet for WOL.
>>
>> It not clear why this is needed. Is this not a standard feature of the
>> IP? Is there no hardware bit indicating the capability?
>>
> 
> WOL via both ARP and Magic packet is supported by the IP version on ZU+ 
> and Versal. However, user can choose which type of packet to recognize 
> as a WOL event - magic packet or ARP. The existing DT binding already 
> describes one entry for wol via magic packet. Hence, adding a new packet 
> type using the same methodology.

And why would this be board-level configuration? This looks like OS policy.

Best regards,
Krzysztof

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Andrew Lunn]

On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote:
> Hi Andrew,
> 
> 
> On 31/01/24 6:56 am, Andrew Lunn wrote:
> > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
> > > "wol-arp-packet" property enables WOL with ARP packet.
> > > It is an extension to "magic-packet for WOL.
> > 
> > It not clear why this is needed. Is this not a standard feature of the
> > IP? Is there no hardware bit indicating the capability?
> > 
> 
> WOL via both ARP and Magic packet is supported by the IP version on ZU+ and
> Versal. However, user can choose which type of packet to recognize as a WOL
> event - magic packet or ARP.

ethtool says:

           wol p|u|m|b|a|g|s|f|d...
                  Sets Wake-on-LAN options.  Not all devices support this.  The argument to this  option  is  a
                  string of characters specifying which options to enable.
                  p   Wake on PHY activity
                  u   Wake on unicast messages
                  m   Wake on multicast messages
                  b   Wake on broadcast messages
                  a   Wake on ARP
                  g   Wake on MagicPacket™
                  s   Enable SecureOn™ password for MagicPacket™
                  f   Wake on filter(s)
                  d   Disable  (wake  on  nothing).  This option
                      clears all previous options.

So why do we need a DT property?

	Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Vineeth Karumanchi]

Hi Andrew, Krzysztof,



On 31/01/24 6:48 pm, Andrew Lunn wrote:
> On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote:
>> Hi Andrew,
>>
>>
>> On 31/01/24 6:56 am, Andrew Lunn wrote:
>>> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
>>>> "wol-arp-packet" property enables WOL with ARP packet.
>>>> It is an extension to "magic-packet for WOL.
>>>
>>> It not clear why this is needed. Is this not a standard feature of the
>>> IP? Is there no hardware bit indicating the capability?
>>>
>>
>> WOL via both ARP and Magic packet is supported by the IP version on ZU+ and
>> Versal. However, user can choose which type of packet to recognize as a WOL
>> event - magic packet or ARP.
> 
> ethtool says:
> 
>             wol p|u|m|b|a|g|s|f|d...
>                    Sets Wake-on-LAN options.  Not all devices support this.  The argument to this  option  is  a
>                    string of characters specifying which options to enable.
>                    p   Wake on PHY activity
>                    u   Wake on unicast messages
>                    m   Wake on multicast messages
>                    b   Wake on broadcast messages
>                    a   Wake on ARP
>                    g   Wake on MagicPacket™
>                    s   Enable SecureOn™ password for MagicPacket™
>                    f   Wake on filter(s)
>                    d   Disable  (wake  on  nothing).  This option
>                        clears all previous options.
> 
> So why do we need a DT property?
> 

The earlier implementation of WOL (magic-packet) was using DT property.
We added one more packet type using DT property to be in-line with the 
earlier implementation.

However, I echo with you that this feature should be in driver (CAPS).
We will re-work the implementation with the below flow:

- Add MACB_CAPS_WOL capability to the supported platforms
- Advertise supported WOL packet types based on the CAPS in ethtool.
- Users can set packet type using ethtool.

Please let me know your thoughts/suggestions.

🙏 vineeth

> 	Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Andrew Lunn]

On Thu, Feb 01, 2024 at 12:11:15PM +0530, Vineeth Karumanchi wrote:
> Hi Andrew, Krzysztof,
> 
> 
> 
> On 31/01/24 6:48 pm, Andrew Lunn wrote:
> > On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote:
> > > Hi Andrew,
> > > 
> > > 
> > > On 31/01/24 6:56 am, Andrew Lunn wrote:
> > > > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
> > > > > "wol-arp-packet" property enables WOL with ARP packet.
> > > > > It is an extension to "magic-packet for WOL.
> > > > 
> > > > It not clear why this is needed. Is this not a standard feature of the
> > > > IP? Is there no hardware bit indicating the capability?
> > > > 
> > > 
> > > WOL via both ARP and Magic packet is supported by the IP version on ZU+ and
> > > Versal. However, user can choose which type of packet to recognize as a WOL
> > > event - magic packet or ARP.
> > 
> > ethtool says:
> > 
> >             wol p|u|m|b|a|g|s|f|d...
> >                    Sets Wake-on-LAN options.  Not all devices support this.  The argument to this  option  is  a
> >                    string of characters specifying which options to enable.
> >                    p   Wake on PHY activity
> >                    u   Wake on unicast messages
> >                    m   Wake on multicast messages
> >                    b   Wake on broadcast messages
> >                    a   Wake on ARP
> >                    g   Wake on MagicPacket™
> >                    s   Enable SecureOn™ password for MagicPacket™
> >                    f   Wake on filter(s)
> >                    d   Disable  (wake  on  nothing).  This option
> >                        clears all previous options.
> > 
> > So why do we need a DT property?
> > 
> 
> The earlier implementation of WOL (magic-packet) was using DT property.
> We added one more packet type using DT property to be in-line with the
> earlier implementation.

I can understand that. It also suggests we did a bad job reviewing
that patch, and should of rejected it. But it was added a long time
ago, and we were less strict back then.

> 
> However, I echo with you that this feature should be in driver (CAPS).
> We will re-work the implementation with the below flow:
> 
> - Add MACB_CAPS_WOL capability to the supported platforms
> - Advertise supported WOL packet types based on the CAPS in ethtool.
> - Users can set packet type using ethtool.

Yes, this sounds good. Maybe add to that, mark magic-packet
deprecated, and a comment that ethtool should be used instead.

Thanks
	Andrew

Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Karumanchi, Vineeth]

Hi Andrew,

On 2/1/2024 6:42 PM, Andrew Lunn wrote:
> On Thu, Feb 01, 2024 at 12:11:15PM +0530, Vineeth Karumanchi wrote:
<...>
>> The earlier implementation of WOL (magic-packet) was using DT property.
>> We added one more packet type using DT property to be in-line with the
>> earlier implementation.
> 
> I can understand that. It also suggests we did a bad job reviewing
> that patch, and should of rejected it. But it was added a long time
> ago, and we were less strict back then.
> 
>>
>> However, I echo with you that this feature should be in driver (CAPS).
>> We will re-work the implementation with the below flow:
>>
>> - Add MACB_CAPS_WOL capability to the supported platforms
>> - Advertise supported WOL packet types based on the CAPS in ethtool.
>> - Users can set packet type using ethtool.
> 
> Yes, this sounds good. Maybe add to that, mark magic-packet
> deprecated, and a comment that ethtool should be used instead.

OK. We will implement above functionality and send V2.

Thanks
-- 
🙏 vineeth
> 
> Thanks
> 	Andrew

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

[claudiu beznea]

Hi, Vineeth,

On 30.01.2024 12:48, Vineeth Karumanchi wrote:
> 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.

I would add a dash line for each of these 2 items for better understanding.
E.g.:

- tie-off/dummy descriptior: ...
- queue disable: ...

> 
> 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 | 58 +++++++++++++++++++++++-
>  2 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index aa5700ac9c00..f68ff163aa18 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -645,6 +645,9 @@
>  #define GEM_T2OFST_OFFSET			0 /* offset value */
>  #define GEM_T2OFST_SIZE				7
>  
> +/* Bitfields in queue pointer registers */
> +#define GEM_RBQP_DISABLE    BIT(0)

You have spaces after macro name. However the approach in this driver is to
define bit offset and lenght and use {MACB,GEM}_BIT() macros (see the above
bitfield definitions).

> +
>  /* 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
> @@ -737,6 +740,7 @@
>  #define MACB_CAPS_HIGH_SPEED			0x02000000
>  #define MACB_CAPS_CLK_HW_CHG			0x04000000
>  #define MACB_CAPS_MACB_IS_EMAC			0x08000000
> +#define MACB_CAPS_QUEUE_DISABLE			0x00002000

Can you keep this sorted by value with the rest of the caps?

>  #define MACB_CAPS_FIFO_MODE			0x10000000
>  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
>  #define MACB_CAPS_SG_DISABLED			0x40000000
> @@ -1254,6 +1258,7 @@ 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;

Keep this ^

>  	size_t			rx_buffer_size;
>  
>  	unsigned int		rx_ring_size;
> @@ -1276,6 +1281,8 @@ struct macb {
>  		struct gem_stats	gem;
>  	}			hw_stats;
>  
> +	dma_addr_t		rx_ring_tieoff_dma;

And this ^ toghether.

> +
>  	struct macb_or_gem_ops	macbgem_ops;
>  
>  	struct mii_bus		*mii_bus;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 898debfd4db3..47cbea58e6c3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2479,6 +2479,12 @@ static void macb_free_consistent(struct macb *bp)
>  
>  	bp->macbgem_ops.mog_free_rx_buffers(bp);
>  
> +	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;
> +	}
> +

Keep the reverse order of operation as oposed to macb_alloc_consistent,
thus move this before bp->macbgem_ops.mog_free_rx_buffers();

>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  		kfree(queue->tx_skb);
>  		queue->tx_skb = NULL;
> @@ -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;

You also need to free the previously allocated rx buffers.

> +	}
> +
>  	return 0;
>  
>  out_err:
> @@ -2575,6 +2591,16 @@ static int macb_alloc_consistent(struct macb *bp)
>  	return -ENOMEM;
>  }
>  
> +static void macb_init_tieoff(struct macb *bp)
> +{
> +	struct macb_dma_desc *d = bp->rx_ring_tieoff;

s/d/desc to cope with the rest of descriptor usage in this file.

Add this here:

	if (bp->caps & MACB_CAPS_QUEUE_DISABLE)
		return;

to avoid checking it in different places where this function is called.

> +	/* Setup a wrapping descriptor with no free slots
> +	 * (WRAP and USED) to tie off/disable unused RX queues.
> +	 */
> +	macb_set_addr(bp, d, MACB_BIT(RX_WRAP) | MACB_BIT(RX_USED));
> +	d->ctrl = 0;
> +}
> +
>  static void gem_init_rings(struct macb *bp)
>  {
>  	struct macb_queue *queue;
> @@ -2598,6 +2624,8 @@ static void gem_init_rings(struct macb *bp)
>  		gem_rx_refill(queue);
>  	}
>  
> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
> +		macb_init_tieoff(bp);
>  }
>  
>  static void macb_init_rings(struct macb *bp)
> @@ -2615,6 +2643,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)
> @@ -4917,7 +4947,8 @@ 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,

This should go in a different patch.

>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = init_reset_optional,
> @@ -5214,6 +5245,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	struct macb_queue *queue;
>  	unsigned long flags;
>  	unsigned int q;
> +	u32 ctrlmask;

val/tmp should be enough for this as you are, anyway, re-using it in the
next patch for different purpose.

>  	int err;
>  
>  	if (!device_may_wakeup(&bp->dev->dev))
> @@ -5224,6 +5256,30 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
>  		spin_lock_irqsave(&bp->lock, flags);
> +
> +		/* Disable Tx and Rx engines before  disabling the queues,
> +		 * this is mandatory as per the IP spec sheet
> +		 */
> +		ctrlmask = macb_readl(bp, NCR);
> +		ctrlmask &= ~(MACB_BIT(TE) | MACB_BIT(RE));

You can remove this
> +		macb_writel(bp, NCR, ctrlmask);

And add this:
macb_writel(bp, NCR, ctrlmask & ~(MACB_BIT(TE) | MACB_BIT(RE));

> +		for (q = 0, queue = bp->queues; q < bp->num_queues;
> +		     ++q, ++queue) {
> +			/* Disable RX queues */

Operation in this for loop could be moved in the the above IRQ disable
loop. Have you tried it? are there any issues with it?

> +			if (bp->caps & MACB_CAPS_QUEUE_DISABLE) {
> +				queue_writel(queue, RBQP, GEM_RBQP_DISABLE);
> +			} else {
> +				/* Tie off RX queues */
> +				queue_writel(queue, RBQP,
> +					     lower_32_bits(bp->rx_ring_tieoff_dma));

I think this should be guarded by:
#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +				queue_writel(queue, RBQPH,
> +					     upper_32_bits(bp->rx_ring_tieoff_dma));
> +			}
> +		}
> +		/* Enable Receive engine */
> +		ctrlmask = macb_readl(bp, NCR);

Is this needed?

> +		ctrlmask |= MACB_BIT(RE);
> +		macb_writel(bp, NCR, ctrlmask);

These could be merged

>  		/* Flush all status bits */
>  		macb_writel(bp, TSR, -1);
>  		macb_writel(bp, RSR, -1);

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

[Karumanchi, Vineeth]

Hi Claudiu,


On 2/3/2024 9:08 PM, claudiu beznea wrote:
<...>
>> +		for (q = 0, queue = bp->queues; q < bp->num_queues;
>> +		     ++q, ++queue) {
>> +			/* Disable RX queues */
> Operation in this for loop could be moved in the the above IRQ disable
> loop. Have you tried it? are there any issues with it?

OK. I haven't tried it. I will try and update.

> 
>> +			if (bp->caps & MACB_CAPS_QUEUE_DISABLE) {
>> +				queue_writel(queue, RBQP, GEM_RBQP_DISABLE);
>> +			} else {
>> +				/* Tie off RX queues */
>> +				queue_writel(queue, RBQP,
>> +					     lower_32_bits(bp->rx_ring_tieoff_dma));
> I think this should be guarded by:
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> +				queue_writel(queue, RBQPH,
>> +					     upper_32_bits(bp->rx_ring_tieoff_dma));
>> +			}
>> +		}
>> +		/* Enable Receive engine */
>> +		ctrlmask = macb_readl(bp, NCR);
> Is this needed?

Yes, not needed, we can use earlier value directly.

> 
>> +		ctrlmask |= MACB_BIT(RE);
<...>
-- 
🙏 vineeth

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

[Karumanchi, Vineeth]

Hi Claudiu,

On 2/3/2024 9:08 PM, claudiu beznea wrote:
<...>
>>   		queue->tx_skb = NULL;
>> @@ -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;
> You also need to free the previously allocated rx buffers.

Are you referring to (bp->macbgem_ops.mog_alloc_rx_buffers(bp)) allocation ?
It was freed in macb_free_consistent():
			 ...
			 bp->macbgem_ops.mog_free_rx_buffers(bp);
			 ...

Please let me know if you are referring to different buffers.


> 
>> +	}
>> +
>>   	return 0;
>>   

-- 
🙏 vineeth

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

[claudiu beznea]

Hi, Vineeth,

On 15.02.2024 07:43, Karumanchi, Vineeth wrote:
> Hi Claudiu,
> 
> On 2/3/2024 9:08 PM, claudiu beznea wrote:
> <...>
>>>           queue->tx_skb = NULL;
>>> @@ -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;
>> You also need to free the previously allocated rx buffers.
> 
> Are you referring to (bp->macbgem_ops.mog_alloc_rx_buffers(bp)) allocation ?
> It was freed in macb_free_consistent():
>              ...
>              bp->macbgem_ops.mog_free_rx_buffers(bp);
>              ...

You're right, my bad.

> 
> Please let me know if you are referring to different buffers.

I was referring to bp->macbgem_ops.mog_alloc_rx_buffers but hat is covered
as you pointed out.

Thank you,
Claudiu Beznea