[PATCH net-next v1 0/4] net: stmmac: Enable Per DMA Channel interrupt

[PATCH net-next v1 0/4] net: stmmac: Enable Per DMA Channel interrupt

[Leong Ching Swee]

From: Swee Leong Ching <leong.ching.swee@intel.com>

Hi,
Add Per DMA Channel interrupt feature for DWXGMAC IP.
 
Patchset (link below) contains per DMA channel interrupt, But it was 
achieved.
https://lore.kernel.org/lkml/20230821203328.GA2197059-
robh@kernel.org/t/#m849b529a642e1bff89c05a07efc25d6a94c8bfb4
 
Some of the changes in this patchset are based on reviewer comment on 
patchset mentioned beforehand.

Swee Leong Ching (4):
  dt-bindings: net: snps,dwmac: per channel irq
  net: stmmac: Make MSI interrupt routine generic
  net: stmmac: Add support for TX/RX channel interrupt
  net: stmmac: Use interrupt mode INTM=1 for per channel irq

 .../devicetree/bindings/net/snps,dwmac.yaml   | 24 ++++++++++----
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +--
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  3 ++
 .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 32 +++++++++++--------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++--------
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 24 ++++++++++++++
 include/linux/stmmac.h                        |  4 +--
 8 files changed, 84 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH net-next v1 1/4] dt-bindings: net: snps,dwmac: per channel irq

[Leong Ching Swee]

From: Swee Leong Ching <leong.ching.swee@intel.com>

Add dt-bindings for per channel irq.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 5c2769dc689a..e72dded824f4 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -103,17 +103,27 @@ properties:
 
   interrupts:
     minItems: 1
-    items:
-      - description: Combined signal for various interrupt events
-      - description: The interrupt to manage the remote wake-up packet detection
-      - description: The interrupt that occurs when Rx exits the LPI state
+    maxItems: 19
 
   interrupt-names:
     minItems: 1
+    maxItems: 19
     items:
-      - const: macirq
-      - enum: [eth_wake_irq, eth_lpi]
-      - const: eth_lpi
+      oneOf:
+        - description: Combined signal for various interrupt events
+          const: macirq
+        - description: The interrupt to manage the remote wake-up packet detection
+          const: eth_wake_irq
+        - description: The interrupt that occurs when Rx exits the LPI state
+          const: eth_lpi
+        - description: DMA Tx per-channel interrupt
+          pattern: '^dma_tx[0-7]?$'
+        - description: DMA Rx per-channel interrupt
+          pattern: '^dma_rx[0-7]?$'
+
+    allOf:
+      - contains:
+          const: macirq
 
   clocks:
     minItems: 1
-- 
2.34.1

Re: [PATCH net-next v1 1/4] dt-bindings: net: snps,dwmac: per channel irq

[Serge Semin]

On Fri, Dec 22, 2023 at 01:44:48PM +0800, Leong Ching Swee wrote:
> From: Swee Leong Ching <leong.ching.swee@intel.com>
> 
> Add dt-bindings for per channel irq.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   | 24 +++++++++++++------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 5c2769dc689a..e72dded824f4 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -103,17 +103,27 @@ properties:
>  
>    interrupts:
>      minItems: 1
> -    items:
> -      - description: Combined signal for various interrupt events
> -      - description: The interrupt to manage the remote wake-up packet detection
> -      - description: The interrupt that occurs when Rx exits the LPI state
> +    maxItems: 19
>  
>    interrupt-names:
>      minItems: 1
> +    maxItems: 19
>      items:
> -      - const: macirq
> -      - enum: [eth_wake_irq, eth_lpi]
> -      - const: eth_lpi
> +      oneOf:
> +        - description: Combined signal for various interrupt events
> +          const: macirq
> +        - description: The interrupt to manage the remote wake-up packet detection
> +          const: eth_wake_irq
> +        - description: The interrupt that occurs when Rx exits the LPI state
> +          const: eth_lpi
> +        - description: DMA Tx per-channel interrupt
> +          pattern: '^dma_tx[0-7]?$'
> +        - description: DMA Rx per-channel interrupt
> +          pattern: '^dma_rx[0-7]?$'
> +

> +    allOf:
> +      - contains:
> +          const: macirq

As Rob correctly noted it's also better to make sure that 'macirq' is
placed first in the array. So instead of the constraint above I guess
the next one would make sure both the array has 'macirq' name and it's
the first item:

allOf:
  - maxItems: 34
    items:
      - const: macirq

-Serge(y)

>  
>    clocks:
>      minItems: 1
> -- 
> 2.34.1
> 
> 

RE: [PATCH net-next v1 1/4] dt-bindings: net: snps,dwmac: per channel irq

[Swee, Leong Ching]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Friday, December 22, 2023 10:51 PM
> To: Swee, Leong Ching <leong.ching.swee@intel.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> linux-stm32@st-md-mailman.stormreply.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; G Thomas, Rohan
> <rohan.g.thomas@intel.com>
> Subject: Re: [PATCH net-next v1 1/4] dt-bindings: net: snps,dwmac: per
> channel irq
> 
> On Fri, Dec 22, 2023 at 01:44:48PM +0800, Leong Ching Swee wrote:
> > From: Swee Leong Ching <leong.ching.swee@intel.com>
> >
> > Add dt-bindings for per channel irq.
> >
> > Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> > Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml   | 24 +++++++++++++------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index 5c2769dc689a..e72dded824f4 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -103,17 +103,27 @@ properties:
> >
> >    interrupts:
> >      minItems: 1
> > -    items:
> > -      - description: Combined signal for various interrupt events
> > -      - description: The interrupt to manage the remote wake-up packet
> detection
> > -      - description: The interrupt that occurs when Rx exits the LPI state
> > +    maxItems: 19
> >
> >    interrupt-names:
> >      minItems: 1
> > +    maxItems: 19
> >      items:
> > -      - const: macirq
> > -      - enum: [eth_wake_irq, eth_lpi]
> > -      - const: eth_lpi
> > +      oneOf:
> > +        - description: Combined signal for various interrupt events
> > +          const: macirq
> > +        - description: The interrupt to manage the remote wake-up packet
> detection
> > +          const: eth_wake_irq
> > +        - description: The interrupt that occurs when Rx exits the LPI state
> > +          const: eth_lpi
> > +        - description: DMA Tx per-channel interrupt
> > +          pattern: '^dma_tx[0-7]?$'
> > +        - description: DMA Rx per-channel interrupt
> > +          pattern: '^dma_rx[0-7]?$'
> > +
> 
> > +    allOf:
> > +      - contains:
> > +          const: macirq
> 
> As Rob correctly noted it's also better to make sure that 'macirq' is placed first
> in the array. So instead of the constraint above I guess the next one would
> make sure both the array has 'macirq' name and it's the first item:
> 
> allOf:
>   - maxItems: 34
>     items:
>       - const: macirq
> 
> -Serge(y)
> 
  interrupt-names:
    minItems: 1
    maxItems: 19
    items:
      oneOf:
        - description: Combined signal for various interrupt events
          const: macirq
        - description: The interrupt to manage the remote wake-up packet detection
          const: eth_wake_irq
        - description: The interrupt that occurs when Rx exits the LPI state
          const: eth_lpi
        - description: DMA Tx per-channel interrupt
          pattern: '^dma_tx[0-7]?$'
        - description: DMA Rx per-channel interrupt
          pattern: '^dma_rx[0-7]?$'

    allOf:
      - maxItems: 19
        items:
         - const: macirq

I am getting the warning/error below when I tried the changes above (but it does make sure macirq is placed first). 
Removing maxItems create other error, did I miss anything?

/home/swiplab/build/jim/linux/net-next/Documentation/devicetree/bindings/net/snps,dwmac.yaml: properties:interrupt-names:allOf:0: {'maxItems': 19, 'items': [{'const': 'macirq'}]} should not be 
valid under {'required': ['maxItems']}
        hint: "maxItems" is not needed with an "items" list
        from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/home/swiplab/build/jim/linux/net-next/Documentation/devicetree/bindings/net/snps,dwmac.yaml: properties:interrupt-names:allOf:0: {'maxItems': 19, 'items': [{'const': 'macirq'}]} should not be 
valid under {'required': ['maxItems']}
        hint: "maxItems" is not needed with an "items" list
> >
> >    clocks:
> >      minItems: 1
> > --
> > 2.34.1
> >
> >

[PATCH net-next v1 2/4] net: stmmac: Make MSI interrupt routine generic

[Leong Ching Swee]

From: Swee Leong Ching <leong.ching.swee@intel.com>

There is no support for per DMA channel interrupt for non-MSI platform,
where the MAC's per channel interrupt hooks up to interrupt controller(GIC)
through shared peripheral interrupt(SPI) to handle interrupt from TX/RX
transmit channel.

This patch generalize the existing MSI ISR to also support non-MSI
platform.

Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +--
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++---------
 include/linux/stmmac.h                        |  4 +--
 4 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 60283543ffc8..f0ec69af96c9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -952,7 +952,7 @@ static int stmmac_config_single_msi(struct pci_dev *pdev,
 
 	res->irq = pci_irq_vector(pdev, 0);
 	res->wol_irq = res->irq;
-	plat->flags &= ~STMMAC_FLAG_MULTI_MSI_EN;
+	plat->flags &= ~STMMAC_FLAG_MULTI_IRQ_EN;
 	dev_info(&pdev->dev, "%s: Single IRQ enablement successful\n",
 		 __func__);
 
@@ -1004,7 +1004,7 @@ static int stmmac_config_multi_msi(struct pci_dev *pdev,
 	if (plat->msi_sfty_ue_vec < STMMAC_MSI_VEC_MAX)
 		res->sfty_ue_irq = pci_irq_vector(pdev, plat->msi_sfty_ue_vec);
 
-	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
+	plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
 	dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__);
 
 	return 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index 84d3a8551b03..5f649106ffcd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -175,7 +175,7 @@ static void dwmac4_dma_init(void __iomem *ioaddr,
 
 	value = readl(ioaddr + DMA_BUS_MODE);
 
-	if (dma_cfg->multi_msi_en) {
+	if (dma_cfg->multi_irq_en) {
 		value &= ~DMA_BUS_MODE_INTM_MASK;
 		value |= (DMA_BUS_MODE_INTM_MODE1 << DMA_BUS_MODE_INTM_SHIFT);
 	}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 47de466e432c..30cc9edb4198 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -129,8 +129,8 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id);
 /* For MSI interrupts handling */
 static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id);
 static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id);
-static irqreturn_t stmmac_msi_intr_tx(int irq, void *data);
-static irqreturn_t stmmac_msi_intr_rx(int irq, void *data);
+static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data);
+static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data);
 static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue);
 static void stmmac_reset_tx_queue(struct stmmac_priv *priv, u32 queue);
 static void stmmac_reset_queues_param(struct stmmac_priv *priv);
@@ -3602,7 +3602,7 @@ static void stmmac_free_irq(struct net_device *dev,
 	}
 }
 
-static int stmmac_request_irq_multi_msi(struct net_device *dev)
+static int stmmac_request_irq_multi(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 	enum request_irq_err irq_err;
@@ -3701,13 +3701,13 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 	for (i = 0; i < priv->plat->rx_queues_to_use; i++) {
 		if (i >= MTL_MAX_RX_QUEUES)
 			break;
-		if (priv->rx_irq[i] == 0)
+		if (priv->rx_irq[i] <= 0)
 			continue;
 
 		int_name = priv->int_name_rx_irq[i];
 		sprintf(int_name, "%s:%s-%d", dev->name, "rx", i);
 		ret = request_irq(priv->rx_irq[i],
-				  stmmac_msi_intr_rx,
+				  stmmac_rx_queue_interrupt,
 				  0, int_name, &priv->dma_conf.rx_queue[i]);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
@@ -3726,13 +3726,13 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
 		if (i >= MTL_MAX_TX_QUEUES)
 			break;
-		if (priv->tx_irq[i] == 0)
+		if (priv->tx_irq[i] <= 0)
 			continue;
 
 		int_name = priv->int_name_tx_irq[i];
 		sprintf(int_name, "%s:%s-%d", dev->name, "tx", i);
 		ret = request_irq(priv->tx_irq[i],
-				  stmmac_msi_intr_tx,
+				  stmmac_tx_queue_interrupt,
 				  0, int_name, &priv->dma_conf.tx_queue[i]);
 		if (unlikely(ret < 0)) {
 			netdev_err(priv->dev,
@@ -3811,8 +3811,8 @@ static int stmmac_request_irq(struct net_device *dev)
 	int ret;
 
 	/* Request the IRQ lines */
-	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN)
-		ret = stmmac_request_irq_multi_msi(dev);
+	if (priv->plat->flags & STMMAC_FLAG_MULTI_IRQ_EN)
+		ret = stmmac_request_irq_multi(dev);
 	else
 		ret = stmmac_request_irq_single(dev);
 
@@ -6075,7 +6075,7 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
+static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data)
 {
 	struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
 	struct stmmac_dma_conf *dma_conf;
@@ -6107,7 +6107,7 @@ static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
+static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data)
 {
 	struct stmmac_rx_queue *rx_q = (struct stmmac_rx_queue *)data;
 	struct stmmac_dma_conf *dma_conf;
@@ -7456,8 +7456,11 @@ int stmmac_dvr_probe(struct device *device,
 	priv->plat = plat_dat;
 	priv->ioaddr = res->addr;
 	priv->dev->base_addr = (unsigned long)res->addr;
-	priv->plat->dma_cfg->multi_msi_en =
-		(priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN);
+
+	if (res->rx_irq[0] > 0 && res->tx_irq[0] > 0) {
+		priv->plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
+		priv->plat->dma_cfg->multi_irq_en = true;
+	}
 
 	priv->dev->irq = res->irq;
 	priv->wol_irq = res->wol_irq;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index dee5ad6e48c5..b950e6f9761d 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -98,7 +98,7 @@ struct stmmac_dma_cfg {
 	int mixed_burst;
 	bool aal;
 	bool eame;
-	bool multi_msi_en;
+	bool multi_irq_en;
 	bool dche;
 };
 
@@ -215,7 +215,7 @@ struct dwmac4_addrs {
 #define STMMAC_FLAG_TSO_EN			BIT(4)
 #define STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP	BIT(5)
 #define STMMAC_FLAG_VLAN_FAIL_Q_EN		BIT(6)
-#define STMMAC_FLAG_MULTI_MSI_EN		BIT(7)
+#define STMMAC_FLAG_MULTI_IRQ_EN		BIT(7)
 #define STMMAC_FLAG_EXT_SNAPSHOT_EN		BIT(8)
 #define STMMAC_FLAG_INT_SNAPSHOT_EN		BIT(9)
 #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI		BIT(10)
-- 
2.34.1

Re: [PATCH net-next v1 2/4] net: stmmac: Make MSI interrupt routine generic

[Serge Semin]

On Fri, Dec 22, 2023 at 01:44:49PM +0800, Leong Ching Swee wrote:
> From: Swee Leong Ching <leong.ching.swee@intel.com>
> 
> There is no support for per DMA channel interrupt for non-MSI platform,
> where the MAC's per channel interrupt hooks up to interrupt controller(GIC)
> through shared peripheral interrupt(SPI) to handle interrupt from TX/RX
> transmit channel.
> 
> This patch generalize the existing MSI ISR to also support non-MSI
> platform.
> 
> Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +--
>  .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++---------
>  include/linux/stmmac.h                        |  4 +--
>  4 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 60283543ffc8..f0ec69af96c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -952,7 +952,7 @@ static int stmmac_config_single_msi(struct pci_dev *pdev,
>  
>  	res->irq = pci_irq_vector(pdev, 0);
>  	res->wol_irq = res->irq;
> -	plat->flags &= ~STMMAC_FLAG_MULTI_MSI_EN;
> +	plat->flags &= ~STMMAC_FLAG_MULTI_IRQ_EN;
>  	dev_info(&pdev->dev, "%s: Single IRQ enablement successful\n",
>  		 __func__);
>  
> @@ -1004,7 +1004,7 @@ static int stmmac_config_multi_msi(struct pci_dev *pdev,
>  	if (plat->msi_sfty_ue_vec < STMMAC_MSI_VEC_MAX)
>  		res->sfty_ue_irq = pci_irq_vector(pdev, plat->msi_sfty_ue_vec);
>  
> -	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> +	plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
>  	dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__);
>  
>  	return 0;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> index 84d3a8551b03..5f649106ffcd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> @@ -175,7 +175,7 @@ static void dwmac4_dma_init(void __iomem *ioaddr,
>  
>  	value = readl(ioaddr + DMA_BUS_MODE);
>  
> -	if (dma_cfg->multi_msi_en) {
> +	if (dma_cfg->multi_irq_en) {
>  		value &= ~DMA_BUS_MODE_INTM_MASK;
>  		value |= (DMA_BUS_MODE_INTM_MODE1 << DMA_BUS_MODE_INTM_SHIFT);
>  	}
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 47de466e432c..30cc9edb4198 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -129,8 +129,8 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id);
>  /* For MSI interrupts handling */
>  static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id);
>  static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id);

> -static irqreturn_t stmmac_msi_intr_tx(int irq, void *data);
> -static irqreturn_t stmmac_msi_intr_rx(int irq, void *data);
> +static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data);
> +static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data);

Let's use the next names instead:

+static irqreturn_t stmmac_dma_tx_interrupt(int irq, void *data);
+static irqreturn_t stmmac_dma_rx_interrupt(int irq, void *data);

It would be semantically more correct and would refer to the
stmmac_dma_interrupt() handler.

>  static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue);
>  static void stmmac_reset_tx_queue(struct stmmac_priv *priv, u32 queue);
>  static void stmmac_reset_queues_param(struct stmmac_priv *priv);
> @@ -3602,7 +3602,7 @@ static void stmmac_free_irq(struct net_device *dev,
>  	}
>  }
>  
> -static int stmmac_request_irq_multi_msi(struct net_device *dev)
> +static int stmmac_request_irq_multi(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	enum request_irq_err irq_err;
> @@ -3701,13 +3701,13 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
>  	for (i = 0; i < priv->plat->rx_queues_to_use; i++) {
>  		if (i >= MTL_MAX_RX_QUEUES)
>  			break;

> -		if (priv->rx_irq[i] == 0)
> +		if (priv->rx_irq[i] <= 0)

Why? What about just using a temporary variable in
stmmac_get_platform_resources() to get the Per-channel DMA IRQs and
not saving error number in priv->rx_irq[]?

>  			continue;
>  
>  		int_name = priv->int_name_rx_irq[i];
>  		sprintf(int_name, "%s:%s-%d", dev->name, "rx", i);
>  		ret = request_irq(priv->rx_irq[i],
> -				  stmmac_msi_intr_rx,
> +				  stmmac_rx_queue_interrupt,
>  				  0, int_name, &priv->dma_conf.rx_queue[i]);
>  		if (unlikely(ret < 0)) {
>  			netdev_err(priv->dev,
> @@ -3726,13 +3726,13 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
>  	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
>  		if (i >= MTL_MAX_TX_QUEUES)
>  			break;

> -		if (priv->tx_irq[i] == 0)
> +		if (priv->tx_irq[i] <= 0)

ditto.

>  			continue;
>  
>  		int_name = priv->int_name_tx_irq[i];
>  		sprintf(int_name, "%s:%s-%d", dev->name, "tx", i);
>  		ret = request_irq(priv->tx_irq[i],
> -				  stmmac_msi_intr_tx,
> +				  stmmac_tx_queue_interrupt,
>  				  0, int_name, &priv->dma_conf.tx_queue[i]);
>  		if (unlikely(ret < 0)) {
>  			netdev_err(priv->dev,

Please fix the error message strings in stmmac_request_irq_multi_msi()
too.

> @@ -3811,8 +3811,8 @@ static int stmmac_request_irq(struct net_device *dev)
>  	int ret;
>  
>  	/* Request the IRQ lines */
> -	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN)
> -		ret = stmmac_request_irq_multi_msi(dev);
> +	if (priv->plat->flags & STMMAC_FLAG_MULTI_IRQ_EN)
> +		ret = stmmac_request_irq_multi(dev);
>  	else
>  		ret = stmmac_request_irq_single(dev);
>  
> @@ -6075,7 +6075,7 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
> +static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data)
>  {
>  	struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
>  	struct stmmac_dma_conf *dma_conf;
> @@ -6107,7 +6107,7 @@ static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
> +static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data)
>  {
>  	struct stmmac_rx_queue *rx_q = (struct stmmac_rx_queue *)data;
>  	struct stmmac_dma_conf *dma_conf;
> @@ -7456,8 +7456,11 @@ int stmmac_dvr_probe(struct device *device,
>  	priv->plat = plat_dat;
>  	priv->ioaddr = res->addr;
>  	priv->dev->base_addr = (unsigned long)res->addr;
> -	priv->plat->dma_cfg->multi_msi_en =
> -		(priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN);
> +

> +	if (res->rx_irq[0] > 0 && res->tx_irq[0] > 0) {
> +		priv->plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
> +		priv->plat->dma_cfg->multi_irq_en = true;
> +	}

This is wrong. It activates the stmmac_request_irq_multi_msi() method
to assign all the IRQ handlers to the individual IRQs. Even if DMA
IRQs line are available it doesn't mean that for instance Safety
Feature IRQs too. So it's better to rely on the glue drivers to set
that flag as before and leave the code as is.

-Serge(y) 

>  
>  	priv->dev->irq = res->irq;
>  	priv->wol_irq = res->wol_irq;
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index dee5ad6e48c5..b950e6f9761d 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -98,7 +98,7 @@ struct stmmac_dma_cfg {
>  	int mixed_burst;
>  	bool aal;
>  	bool eame;
> -	bool multi_msi_en;
> +	bool multi_irq_en;
>  	bool dche;
>  };
>  
> @@ -215,7 +215,7 @@ struct dwmac4_addrs {
>  #define STMMAC_FLAG_TSO_EN			BIT(4)
>  #define STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP	BIT(5)
>  #define STMMAC_FLAG_VLAN_FAIL_Q_EN		BIT(6)
> -#define STMMAC_FLAG_MULTI_MSI_EN		BIT(7)
> +#define STMMAC_FLAG_MULTI_IRQ_EN		BIT(7)
>  #define STMMAC_FLAG_EXT_SNAPSHOT_EN		BIT(8)
>  #define STMMAC_FLAG_INT_SNAPSHOT_EN		BIT(9)
>  #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI		BIT(10)
> -- 
> 2.34.1
> 
> 

RE: [PATCH net-next v1 2/4] net: stmmac: Make MSI interrupt routine generic

[Swee, Leong Ching]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Saturday, December 23, 2023 3:03 AM
> To: Swee, Leong Ching <leong.ching.swee@intel.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> linux-stm32@st-md-mailman.stormreply.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; Teoh Ji Sheng
> <ji.sheng.teoh@intel.com>
> Subject: Re: [PATCH net-next v1 2/4] net: stmmac: Make MSI interrupt
> routine generic
> 
> On Fri, Dec 22, 2023 at 01:44:49PM +0800, Leong Ching Swee wrote:
> > From: Swee Leong Ching <leong.ching.swee@intel.com>
> >
> > There is no support for per DMA channel interrupt for non-MSI
> > platform, where the MAC's per channel interrupt hooks up to interrupt
> > controller(GIC) through shared peripheral interrupt(SPI) to handle
> > interrupt from TX/RX transmit channel.
> >
> > This patch generalize the existing MSI ISR to also support non-MSI
> > platform.
> >
> > Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> > Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +--
> > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
> > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++--------
> -
> >  include/linux/stmmac.h                        |  4 +--
> >  4 files changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index 60283543ffc8..f0ec69af96c9 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -952,7 +952,7 @@ static int stmmac_config_single_msi(struct pci_dev
> > *pdev,
> >
> >  	res->irq = pci_irq_vector(pdev, 0);
> >  	res->wol_irq = res->irq;
> > -	plat->flags &= ~STMMAC_FLAG_MULTI_MSI_EN;
> > +	plat->flags &= ~STMMAC_FLAG_MULTI_IRQ_EN;
> >  	dev_info(&pdev->dev, "%s: Single IRQ enablement successful\n",
> >  		 __func__);
> >
> > @@ -1004,7 +1004,7 @@ static int stmmac_config_multi_msi(struct
> pci_dev *pdev,
> >  	if (plat->msi_sfty_ue_vec < STMMAC_MSI_VEC_MAX)
> >  		res->sfty_ue_irq = pci_irq_vector(pdev, plat-
> >msi_sfty_ue_vec);
> >
> > -	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > +	plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
> >  	dev_info(&pdev->dev, "%s: multi MSI enablement successful\n",
> > __func__);
> >
> >  	return 0;
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> > index 84d3a8551b03..5f649106ffcd 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> > @@ -175,7 +175,7 @@ static void dwmac4_dma_init(void __iomem
> *ioaddr,
> >
> >  	value = readl(ioaddr + DMA_BUS_MODE);
> >
> > -	if (dma_cfg->multi_msi_en) {
> > +	if (dma_cfg->multi_irq_en) {
> >  		value &= ~DMA_BUS_MODE_INTM_MASK;
> >  		value |= (DMA_BUS_MODE_INTM_MODE1 <<
> DMA_BUS_MODE_INTM_SHIFT);
> >  	}
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 47de466e432c..30cc9edb4198 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -129,8 +129,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
> > *dev_id);
> >  /* For MSI interrupts handling */
> >  static irqreturn_t stmmac_mac_interrupt(int irq, void *dev_id);
> > static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id);
> 
> > -static irqreturn_t stmmac_msi_intr_tx(int irq, void *data); -static
> > irqreturn_t stmmac_msi_intr_rx(int irq, void *data);
> > +static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data);
> > +static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data);
> 
> Let's use the next names instead:
> 
> +static irqreturn_t stmmac_dma_tx_interrupt(int irq, void *data); static
> +irqreturn_t stmmac_dma_rx_interrupt(int irq, void *data);
> 
> It would be semantically more correct and would refer to the
> stmmac_dma_interrupt() handler.
> 
Will rename this in v2
> >  static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32
> > queue);  static void stmmac_reset_tx_queue(struct stmmac_priv *priv,
> > u32 queue);  static void stmmac_reset_queues_param(struct stmmac_priv
> > *priv); @@ -3602,7 +3602,7 @@ static void stmmac_free_irq(struct
> net_device *dev,
> >  	}
> >  }
> >
> > -static int stmmac_request_irq_multi_msi(struct net_device *dev)
> > +static int stmmac_request_irq_multi(struct net_device *dev)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	enum request_irq_err irq_err;
> > @@ -3701,13 +3701,13 @@ static int stmmac_request_irq_multi_msi(struct
> net_device *dev)
> >  	for (i = 0; i < priv->plat->rx_queues_to_use; i++) {
> >  		if (i >= MTL_MAX_RX_QUEUES)
> >  			break;
> 
> > -		if (priv->rx_irq[i] == 0)
> > +		if (priv->rx_irq[i] <= 0)
> 
> Why? What about just using a temporary variable in
> stmmac_get_platform_resources() to get the Per-channel DMA IRQs and not
> saving error number in priv->rx_irq[]?
> 
stmmac_get_platform_resources() populate stmmac_resources struct, and the checking is on stmmac_request_irq_multi()
 so add 2 array to store the irq error instead?

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -35,6 +35,8 @@ struct stmmac_resources {
        int sfty_ue_irq;
        int rx_irq[MTL_MAX_RX_QUEUES];
        int tx_irq[MTL_MAX_TX_QUEUES];
+       int tx_irq_error[MTL_MAX_RX_QUEUES];
+       int rx_irq_error[MTL_MAX_TX_QUEUES];
 
So do stmmac_priv struct.
@@ -301,6 +303,8 @@ struct stmmac_priv {
        int sfty_ue_irq;
        int rx_irq[MTL_MAX_RX_QUEUES];
        int tx_irq[MTL_MAX_TX_QUEUES];
+       int rx_irq_error[MTL_MAX_RX_QUEUES];
+       int tx_irq_error[MTL_MAX_TX_QUEUES];

-               if (priv->tx_irq[i] <= 0)
+               if (priv->tx_irq[i] == 0 || priv->tx_irq_error[i] < 0)
                        continue;
 };> >  			continue;
> >
> >  		int_name = priv->int_name_rx_irq[i];
> >  		sprintf(int_name, "%s:%s-%d", dev->name, "rx", i);
> >  		ret = request_irq(priv->rx_irq[i],
> > -				  stmmac_msi_intr_rx,
> > +				  stmmac_rx_queue_interrupt,
> >  				  0, int_name, &priv-
> >dma_conf.rx_queue[i]);
> >  		if (unlikely(ret < 0)) {
> >  			netdev_err(priv->dev,
> > @@ -3726,13 +3726,13 @@ static int stmmac_request_irq_multi_msi(struct
> net_device *dev)
> >  	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> >  		if (i >= MTL_MAX_TX_QUEUES)
> >  			break;
> 
> > -		if (priv->tx_irq[i] == 0)
> > +		if (priv->tx_irq[i] <= 0)
> 
> ditto.
> 
> >  			continue;
> >
> >  		int_name = priv->int_name_tx_irq[i];
> >  		sprintf(int_name, "%s:%s-%d", dev->name, "tx", i);
> >  		ret = request_irq(priv->tx_irq[i],
> > -				  stmmac_msi_intr_tx,
> > +				  stmmac_tx_queue_interrupt,
> >  				  0, int_name, &priv-
> >dma_conf.tx_queue[i]);
> >  		if (unlikely(ret < 0)) {
> >  			netdev_err(priv->dev,
> 
> Please fix the error message strings in stmmac_request_irq_multi_msi() too.
> 
Sure, will change the error to below in v2.
                if (unlikely(ret < 0)) {
                        netdev_err(priv->dev,
-                                  "%s: alloc rx-%d  MSI %d (error: %d)\n",
+                                  "%s: alloc rx-%d  dma rx_irq %d (error: %d)\n",
                                   __func__, i, priv->rx_irq[i], ret);
                        irq_err = REQ_IRQ_ERR_RX;
                        irq_idx = i;

                if (unlikely(ret < 0)) {
                        netdev_err(priv->dev,
-                                  "%s: alloc tx-%d  MSI %d (error: %d)\n",
+                                  "%s: alloc tx-%d  dma tx_irq %d (error: %d)\n",
                                   __func__, i, priv->tx_irq[i], ret);
                        irq_err = REQ_IRQ_ERR_TX;

> > @@ -3811,8 +3811,8 @@ static int stmmac_request_irq(struct net_device
> *dev)
> >  	int ret;
> >
> >  	/* Request the IRQ lines */
> > -	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN)
> > -		ret = stmmac_request_irq_multi_msi(dev);
> > +	if (priv->plat->flags & STMMAC_FLAG_MULTI_IRQ_EN)
> > +		ret = stmmac_request_irq_multi(dev);
> >  	else
> >  		ret = stmmac_request_irq_single(dev);
> >
> > @@ -6075,7 +6075,7 @@ static irqreturn_t stmmac_safety_interrupt(int
> irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > -static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
> > +static irqreturn_t stmmac_tx_queue_interrupt(int irq, void *data)
> >  {
> >  	struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
> >  	struct stmmac_dma_conf *dma_conf;
> > @@ -6107,7 +6107,7 @@ static irqreturn_t stmmac_msi_intr_tx(int irq,
> void *data)
> >  	return IRQ_HANDLED;
> >  }
> >
> > -static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
> > +static irqreturn_t stmmac_rx_queue_interrupt(int irq, void *data)
> >  {
> >  	struct stmmac_rx_queue *rx_q = (struct stmmac_rx_queue *)data;
> >  	struct stmmac_dma_conf *dma_conf;
> > @@ -7456,8 +7456,11 @@ int stmmac_dvr_probe(struct device *device,
> >  	priv->plat = plat_dat;
> >  	priv->ioaddr = res->addr;
> >  	priv->dev->base_addr = (unsigned long)res->addr;
> > -	priv->plat->dma_cfg->multi_msi_en =
> > -		(priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN);
> > +
> 
> > +	if (res->rx_irq[0] > 0 && res->tx_irq[0] > 0) {
> > +		priv->plat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
> > +		priv->plat->dma_cfg->multi_irq_en = true;
> > +	}
> 
> This is wrong. It activates the stmmac_request_irq_multi_msi() method to
> assign all the IRQ handlers to the individual IRQs. Even if DMA IRQs line are
> available it doesn't mean that for instance Safety Feature IRQs too. So it's
> better to rely on the glue drivers to set that flag as before and leave the code
> as is.
> 
> -Serge(y)
> 
Will move flags handling to dwmac-socfpga.c in v2.
        plat_dat->bsp_priv = dwmac;
        plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;
 
+       if (stmmac_res.rx_irq[0] > 0 && stmmac_res.tx_irq[0] > 0) {
+               plat_dat->flags |= STMMAC_FLAG_MULTI_IRQ_EN;
> >
> >  	priv->dev->irq = res->irq;
> >  	priv->wol_irq = res->wol_irq;
> > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index
> > dee5ad6e48c5..b950e6f9761d 100644
> > --- a/include/linux/stmmac.h
> > +++ b/include/linux/stmmac.h
> > @@ -98,7 +98,7 @@ struct stmmac_dma_cfg {
> >  	int mixed_burst;
> >  	bool aal;
> >  	bool eame;
> > -	bool multi_msi_en;
> > +	bool multi_irq_en;
> >  	bool dche;
> >  };
> >
> > @@ -215,7 +215,7 @@ struct dwmac4_addrs {
> >  #define STMMAC_FLAG_TSO_EN			BIT(4)
> >  #define STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP	BIT(5)
> >  #define STMMAC_FLAG_VLAN_FAIL_Q_EN		BIT(6)
> > -#define STMMAC_FLAG_MULTI_MSI_EN		BIT(7)
> > +#define STMMAC_FLAG_MULTI_IRQ_EN		BIT(7)
> >  #define STMMAC_FLAG_EXT_SNAPSHOT_EN		BIT(8)
> >  #define STMMAC_FLAG_INT_SNAPSHOT_EN		BIT(9)
> >  #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI		BIT(10)
> > --
> > 2.34.1
> >
> >

[PATCH net-next v1 3/4] net: stmmac: Add support for TX/RX channel interrupt

[Leong Ching Swee]

From: Swee Leong Ching <leong.ching.swee@intel.com>

Enable TX/RX channel interrupt registration for MAC that interrupts CPU
through shared peripheral interrupt (SPI).

Per channel interrupts and interrupt-names are registered through,
Eg: 4 tx and 4 rx channels:
interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
             <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
             <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
             <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
             <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
             <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
             <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
             <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "dma_tx0",
                  "dma_tx1",
                  "dma_tx2",
                  "dma_tx3",
                  "dma_rx0",
                  "dma_rx1",
                  "dma_rx2",
                  "dma_rx3";

Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 70eadc83ca68..f857907f13a0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -710,6 +710,8 @@ EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
 int stmmac_get_platform_resources(struct platform_device *pdev,
 				  struct stmmac_resources *stmmac_res)
 {
+	char irq_name[8];
+	int i;
 	memset(stmmac_res, 0, sizeof(*stmmac_res));
 
 	/* Get IRQ information early to have an ability to ask for deferred
@@ -719,6 +721,28 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
 	if (stmmac_res->irq < 0)
 		return stmmac_res->irq;
 
+	/* For RX Channel */
+	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
+		snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
+		stmmac_res->rx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
+		if (stmmac_res->rx_irq[i] < 0) {
+			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			break;
+		}
+	}
+
+	/* For TX Channel */
+	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
+		snprintf(irq_name, sizeof(irq_name), "dma_tx%i", i);
+		stmmac_res->tx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
+		if (stmmac_res->tx_irq[i] < 0) {
+			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			break;
+		}
+	}
+
 	/* On some platforms e.g. SPEAr the wake up irq differs from the mac irq
 	 * The external wake up irq can be passed through the platform code
 	 * named as "eth_wake_irq"
-- 
2.34.1

Re: [PATCH net-next v1 3/4] net: stmmac: Add support for TX/RX channel interrupt

[Serge Semin]

On Fri, Dec 22, 2023 at 01:44:50PM +0800, Leong Ching Swee wrote:
> From: Swee Leong Ching <leong.ching.swee@intel.com>
> 
> Enable TX/RX channel interrupt registration for MAC that interrupts CPU
> through shared peripheral interrupt (SPI).
> 
> Per channel interrupts and interrupt-names are registered through,
> Eg: 4 tx and 4 rx channels:
> interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
>              <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
>              <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
>              <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
>              <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
>              <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
>              <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
>              <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-names = "dma_tx0",
>                   "dma_tx1",
>                   "dma_tx2",
>                   "dma_tx3",
>                   "dma_rx0",
>                   "dma_rx1",
>                   "dma_rx2",
>                   "dma_rx3";
> 
> Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> ---
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 70eadc83ca68..f857907f13a0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -710,6 +710,8 @@ EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
>  int stmmac_get_platform_resources(struct platform_device *pdev,
>  				  struct stmmac_resources *stmmac_res)
>  {

> +	char irq_name[8];

By DW XGMAC v2.x IP-core design there can be up to 16 Tx channels and
12 Rx channels. Thus it's better to set irq_name size being at least
(strlen("dma_tx16") + 1) == 9 beforehand since you are adding this
code anyway and for some reason didn't consider to pick the Jisheng'
patch up which fixed the MTL_MAX_TX_QUEUES/MTL_MAX_RX_QUEUES macros.

> +	int i;

Please add an empty line between the variables declaration and the
next statement.

>  	memset(stmmac_res, 0, sizeof(*stmmac_res));
>  
>  	/* Get IRQ information early to have an ability to ask for deferred
> @@ -719,6 +721,28 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
>  	if (stmmac_res->irq < 0)
>  		return stmmac_res->irq;
>  

> +	/* For RX Channel */
> +	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
> +		snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
> +		stmmac_res->rx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
> +		if (stmmac_res->rx_irq[i] < 0) {
> +			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			break;
> +		}
> +	}

What about:

+	/* Get optional Tx/Rx DMA per-channel IRQs, which otherwise
+	 * are supposed to be delivered via the common MAC IRQ line
+	 */
+	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
+		snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
+		irq = platform_get_irq_byname_optional(pdev, irq_name);
+		if (irq == -EPROBE_DEFER)
+			return irq;
+		else if (irq < 0)
+			break;
+
+		stmmac_res->rx_irq[i] = irq;
+	}

It's cleaner a bit with less indentations and doesn't pollute
rx_irq[]/tx_irq[] arrays with the error numbers.

> +
> +	/* For TX Channel */
> +	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
> +		snprintf(irq_name, sizeof(irq_name), "dma_tx%i", i);
> +		stmmac_res->tx_irq[i] = platform_get_irq_byname_optional(pdev, irq_name);
> +		if (stmmac_res->tx_irq[i] < 0) {
> +			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			break;
> +		}
> +	}
> +

Please move the Tx/Rx IRQs getting loops to the bottom of the
stmmac_get_platform_resources() method. Thus the order of the IRQs
getting would be the same as the order of the IRQs requests
implemented in the stmmac_request_irq_multi_msi() and
stmmac_request_irq_single() methods.

-Serge(y)

>  	/* On some platforms e.g. SPEAr the wake up irq differs from the mac irq
>  	 * The external wake up irq can be passed through the platform code
>  	 * named as "eth_wake_irq"
> -- 
> 2.34.1
> 
> 

RE: [PATCH net-next v1 3/4] net: stmmac: Add support for TX/RX channel interrupt

[Swee, Leong Ching]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Saturday, December 23, 2023 5:49 AM
> To: Swee, Leong Ching <leong.ching.swee@intel.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> linux-stm32@st-md-mailman.stormreply.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; Teoh Ji Sheng
> <ji.sheng.teoh@intel.com>
> Subject: Re: [PATCH net-next v1 3/4] net: stmmac: Add support for TX/RX
> channel interrupt
> 
> On Fri, Dec 22, 2023 at 01:44:50PM +0800, Leong Ching Swee wrote:
> > From: Swee Leong Ching <leong.ching.swee@intel.com>
> >
> > Enable TX/RX channel interrupt registration for MAC that interrupts
> > CPU through shared peripheral interrupt (SPI).
> >
> > Per channel interrupts and interrupt-names are registered through,
> > Eg: 4 tx and 4 rx channels:
> > interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
> >              <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
> >              <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
> >              <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> >              <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
> >              <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
> >              <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
> >              <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>; interrupt-names =
> > "dma_tx0",
> >                   "dma_tx1",
> >                   "dma_tx2",
> >                   "dma_tx3",
> >                   "dma_rx0",
> >                   "dma_rx1",
> >                   "dma_rx2",
> >                   "dma_rx3";
> >
> > Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> > Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> > ---
> >  .../ethernet/stmicro/stmmac/stmmac_platform.c | 24
> > +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 70eadc83ca68..f857907f13a0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -710,6 +710,8 @@
> EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
> >  int stmmac_get_platform_resources(struct platform_device *pdev,
> >  				  struct stmmac_resources *stmmac_res)  {
> 
> > +	char irq_name[8];
> 
> By DW XGMAC v2.x IP-core design there can be up to 16 Tx channels and
> 12 Rx channels. Thus it's better to set irq_name size being at least
> (strlen("dma_tx16") + 1) == 9 beforehand since you are adding this code
> anyway and for some reason didn't consider to pick the Jisheng'
> patch up which fixed the MTL_MAX_TX_QUEUES/MTL_MAX_RX_QUEUES
> macros.
> 
I only have 8 channels tx/rx dma irq setup, so I could not test 16 channels
Patch. Will update to 9 in v2.
> > +	int i;
> 
> Please add an empty line between the variables declaration and the next
> statement.
> 
Thanks. Will update this in v2.
> >  	memset(stmmac_res, 0, sizeof(*stmmac_res));
> >
> >  	/* Get IRQ information early to have an ability to ask for deferred
> > @@ -719,6 +721,28 @@ int stmmac_get_platform_resources(struct
> platform_device *pdev,
> >  	if (stmmac_res->irq < 0)
> >  		return stmmac_res->irq;
> >
> 
> > +	/* For RX Channel */
> > +	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
> > +		snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
> > +		stmmac_res->rx_irq[i] =
> platform_get_irq_byname_optional(pdev, irq_name);
> > +		if (stmmac_res->rx_irq[i] < 0) {
> > +			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			break;
> > +		}
> > +	}
> 
> What about:
> 
> +	/* Get optional Tx/Rx DMA per-channel IRQs, which otherwise
> +	 * are supposed to be delivered via the common MAC IRQ line
> +	 */
> +	for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
> +		snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
> +		irq = platform_get_irq_byname_optional(pdev, irq_name);
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		else if (irq < 0)
> +			break;
> +
> +		stmmac_res->rx_irq[i] = irq;
> +	}
> 
> It's cleaner a bit with less indentations and doesn't pollute rx_irq[]/tx_irq[]
> arrays with the error numbers.
> 
Sure, will update this in v2.
> > +
> > +	/* For TX Channel */
> > +	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
> > +		snprintf(irq_name, sizeof(irq_name), "dma_tx%i", i);
> > +		stmmac_res->tx_irq[i] =
> platform_get_irq_byname_optional(pdev, irq_name);
> > +		if (stmmac_res->tx_irq[i] < 0) {
> > +			if (stmmac_res->rx_irq[i] == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			break;
> > +		}
> > +	}
> > +
> 
> Please move the Tx/Rx IRQs getting loops to the bottom of the
> stmmac_get_platform_resources() method. Thus the order of the IRQs
> getting would be the same as the order of the IRQs requests implemented in
> the stmmac_request_irq_multi_msi() and
> stmmac_request_irq_single() methods.
> 
> -Serge(y)
> 
Will move those methods to bottom in v2.
> >  	/* On some platforms e.g. SPEAr the wake up irq differs from the
> mac irq
> >  	 * The external wake up irq can be passed through the platform code
> >  	 * named as "eth_wake_irq"
> > --
> > 2.34.1
> >
> >

[PATCH net-next v1 4/4] net: stmmac: Use interrupt mode INTM=1 for per channel irq

[Leong Ching Swee]

From: Swee Leong Ching <leong.ching.swee@intel.com>

Enable per DMA channel interrupt that uses shared peripheral
interrupt (SPI), so only per channel TX and RX intr (TI/RI)
are handled by TX/RX ISR without calling common interrupt ISR.

Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
---
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  3 ++
 .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 32 +++++++++++--------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 207ff1799f2c..04bf731cb7ea 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -346,6 +346,9 @@
 /* DMA Registers */
 #define XGMAC_DMA_MODE			0x00003000
 #define XGMAC_SWR			BIT(0)
+#define XGMAC_DMA_MODE_INTM_MASK	GENMASK(13, 12)
+#define XGMAC_DMA_MODE_INTM_SHIFT	12
+#define XGMAC_DMA_MODE_INTM_MODE1	0x1
 #define XGMAC_DMA_SYSBUS_MODE		0x00003004
 #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
 #define XGMAC_WR_OSR_LMT_SHIFT		24
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 3cde695fec91..dcb9f094415d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -31,6 +31,13 @@ static void dwxgmac2_dma_init(void __iomem *ioaddr,
 		value |= XGMAC_EAME;
 
 	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
+
+	if (dma_cfg->multi_irq_en) {
+		value = readl(ioaddr + XGMAC_DMA_MODE);
+		value &= ~XGMAC_DMA_MODE_INTM_MASK;
+		value |= (XGMAC_DMA_MODE_INTM_MODE1 << XGMAC_DMA_MODE_INTM_SHIFT);
+		writel(value, ioaddr + XGMAC_DMA_MODE);
+	}
 }
 
 static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv,
@@ -365,19 +372,18 @@ static int dwxgmac2_dma_interrupt(struct stmmac_priv *priv,
 	}
 
 	/* TX/RX NORMAL interrupts */
-	if (likely(intr_status & XGMAC_NIS)) {
-		if (likely(intr_status & XGMAC_RI)) {
-			u64_stats_update_begin(&rxq_stats->syncp);
-			rxq_stats->rx_normal_irq_n++;
-			u64_stats_update_end(&rxq_stats->syncp);
-			ret |= handle_rx;
-		}
-		if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
-			u64_stats_update_begin(&txq_stats->syncp);
-			txq_stats->tx_normal_irq_n++;
-			u64_stats_update_end(&txq_stats->syncp);
-			ret |= handle_tx;
-		}
+	if (likely(intr_status & XGMAC_RI)) {
+		u64_stats_update_begin(&rxq_stats->syncp);
+		rxq_stats->rx_normal_irq_n++;
+		u64_stats_update_end(&rxq_stats->syncp);
+		ret |= handle_rx;
+	}
+
+	if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
+		u64_stats_update_begin(&txq_stats->syncp);
+		txq_stats->tx_normal_irq_n++;
+		u64_stats_update_end(&txq_stats->syncp);
+		ret |= handle_tx;
 	}
 
 	/* Clear interrupts */
-- 
2.34.1

Re: [PATCH net-next v1 4/4] net: stmmac: Use interrupt mode INTM=1 for per channel irq

[Serge Semin]

On Fri, Dec 22, 2023 at 01:44:51PM +0800, Leong Ching Swee wrote:
> From: Swee Leong Ching <leong.ching.swee@intel.com>
> 
> Enable per DMA channel interrupt that uses shared peripheral
> interrupt (SPI), so only per channel TX and RX intr (TI/RI)
> are handled by TX/RX ISR without calling common interrupt ISR.
> 
> Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  3 ++
>  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 32 +++++++++++--------
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 207ff1799f2c..04bf731cb7ea 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -346,6 +346,9 @@
>  /* DMA Registers */
>  #define XGMAC_DMA_MODE			0x00003000
>  #define XGMAC_SWR			BIT(0)
> +#define XGMAC_DMA_MODE_INTM_MASK	GENMASK(13, 12)
> +#define XGMAC_DMA_MODE_INTM_SHIFT	12
> +#define XGMAC_DMA_MODE_INTM_MODE1	0x1
>  #define XGMAC_DMA_SYSBUS_MODE		0x00003004
>  #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
>  #define XGMAC_WR_OSR_LMT_SHIFT		24
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index 3cde695fec91..dcb9f094415d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> @@ -31,6 +31,13 @@ static void dwxgmac2_dma_init(void __iomem *ioaddr,
>  		value |= XGMAC_EAME;
>  
>  	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
> +
> +	if (dma_cfg->multi_irq_en) {
> +		value = readl(ioaddr + XGMAC_DMA_MODE);
> +		value &= ~XGMAC_DMA_MODE_INTM_MASK;
> +		value |= (XGMAC_DMA_MODE_INTM_MODE1 << XGMAC_DMA_MODE_INTM_SHIFT);
> +		writel(value, ioaddr + XGMAC_DMA_MODE);
> +	}
>  }
>  
>  static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv,
> @@ -365,19 +372,18 @@ static int dwxgmac2_dma_interrupt(struct stmmac_priv *priv,
>  	}
>  

>  	/* TX/RX NORMAL interrupts */
> -	if (likely(intr_status & XGMAC_NIS)) {
> -		if (likely(intr_status & XGMAC_RI)) {
> -			u64_stats_update_begin(&rxq_stats->syncp);
> -			rxq_stats->rx_normal_irq_n++;
> -			u64_stats_update_end(&rxq_stats->syncp);
> -			ret |= handle_rx;
> -		}
> -		if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
> -			u64_stats_update_begin(&txq_stats->syncp);
> -			txq_stats->tx_normal_irq_n++;
> -			u64_stats_update_end(&txq_stats->syncp);
> -			ret |= handle_tx;
> -		}
> +	if (likely(intr_status & XGMAC_RI)) {
> +		u64_stats_update_begin(&rxq_stats->syncp);
> +		rxq_stats->rx_normal_irq_n++;
> +		u64_stats_update_end(&rxq_stats->syncp);
> +		ret |= handle_rx;
> +	}
> +
> +	if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
> +		u64_stats_update_begin(&txq_stats->syncp);
> +		txq_stats->tx_normal_irq_n++;
> +		u64_stats_update_end(&txq_stats->syncp);
> +		ret |= handle_tx;

Could you please clarify my comment to the original patch:

On Fri, Aug 18, 2023 at 20:10:21PM +0300, Serge Semin wrote:
> Just curious. Is this change really necessary seeing NIS IRQ is
> unmasked and it is unmasked-OR of the RI/TI/TBU flags in the
> DMA_CHx_Status register? Moreover based on the HW manual,
> DMA_CHx_Status reflects raw IRQ flags status except NIS and AIS which
> are the masked OR of the respective flags. So AFAIU NIS will be set in
> anyway if you have RI/TI/TBU IRQs enabled.

-Serge(y)

>  	}
>  
>  	/* Clear interrupts */
> -- 
> 2.34.1
> 
> 

RE: [PATCH net-next v1 4/4] net: stmmac: Use interrupt mode INTM=1 for per channel irq

[Swee, Leong Ching]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Saturday, December 23, 2023 5:57 AM
> To: Swee, Leong Ching <leong.ching.swee@intel.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> linux-stm32@st-md-mailman.stormreply.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; Teoh Ji Sheng
> <ji.sheng.teoh@intel.com>
> Subject: Re: [PATCH net-next v1 4/4] net: stmmac: Use interrupt mode
> INTM=1 for per channel irq
> 
> On Fri, Dec 22, 2023 at 01:44:51PM +0800, Leong Ching Swee wrote:
> > From: Swee Leong Ching <leong.ching.swee@intel.com>
> >
> > Enable per DMA channel interrupt that uses shared peripheral interrupt
> > (SPI), so only per channel TX and RX intr (TI/RI) are handled by TX/RX
> > ISR without calling common interrupt ISR.
> >
> > Signed-off-by: Teoh Ji Sheng <ji.sheng.teoh@intel.com>
> > Signed-off-by: Swee Leong Ching <leong.ching.swee@intel.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  3 ++
> >  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 32 +++++++++++-------
> -
> >  2 files changed, 22 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > index 207ff1799f2c..04bf731cb7ea 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > @@ -346,6 +346,9 @@
> >  /* DMA Registers */
> >  #define XGMAC_DMA_MODE			0x00003000
> >  #define XGMAC_SWR			BIT(0)
> > +#define XGMAC_DMA_MODE_INTM_MASK	GENMASK(13, 12)
> > +#define XGMAC_DMA_MODE_INTM_SHIFT	12
> > +#define XGMAC_DMA_MODE_INTM_MODE1	0x1
> >  #define XGMAC_DMA_SYSBUS_MODE		0x00003004
> >  #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
> >  #define XGMAC_WR_OSR_LMT_SHIFT		24
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> > index 3cde695fec91..dcb9f094415d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> > @@ -31,6 +31,13 @@ static void dwxgmac2_dma_init(void __iomem
> *ioaddr,
> >  		value |= XGMAC_EAME;
> >
> >  	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
> > +
> > +	if (dma_cfg->multi_irq_en) {
> > +		value = readl(ioaddr + XGMAC_DMA_MODE);
> > +		value &= ~XGMAC_DMA_MODE_INTM_MASK;
> > +		value |= (XGMAC_DMA_MODE_INTM_MODE1 <<
> XGMAC_DMA_MODE_INTM_SHIFT);
> > +		writel(value, ioaddr + XGMAC_DMA_MODE);
> > +	}
> >  }
> >
> >  static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv, @@
> > -365,19 +372,18 @@ static int dwxgmac2_dma_interrupt(struct
> stmmac_priv *priv,
> >  	}
> >
> 
> >  	/* TX/RX NORMAL interrupts */
> > -	if (likely(intr_status & XGMAC_NIS)) {
> > -		if (likely(intr_status & XGMAC_RI)) {
> > -			u64_stats_update_begin(&rxq_stats->syncp);
> > -			rxq_stats->rx_normal_irq_n++;
> > -			u64_stats_update_end(&rxq_stats->syncp);
> > -			ret |= handle_rx;
> > -		}
> > -		if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
> > -			u64_stats_update_begin(&txq_stats->syncp);
> > -			txq_stats->tx_normal_irq_n++;
> > -			u64_stats_update_end(&txq_stats->syncp);
> > -			ret |= handle_tx;
> > -		}
> > +	if (likely(intr_status & XGMAC_RI)) {
> > +		u64_stats_update_begin(&rxq_stats->syncp);
> > +		rxq_stats->rx_normal_irq_n++;
> > +		u64_stats_update_end(&rxq_stats->syncp);
> > +		ret |= handle_rx;
> > +	}
> > +
> > +	if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
> > +		u64_stats_update_begin(&txq_stats->syncp);
> > +		txq_stats->tx_normal_irq_n++;
> > +		u64_stats_update_end(&txq_stats->syncp);
> > +		ret |= handle_tx;
> 
> Could you please clarify my comment to the original patch:
> 
> On Fri, Aug 18, 2023 at 20:10:21PM +0300, Serge Semin wrote:
> > Just curious. Is this change really necessary seeing NIS IRQ is
> > unmasked and it is unmasked-OR of the RI/TI/TBU flags in the
> > DMA_CHx_Status register? Moreover based on the HW manual,
> > DMA_CHx_Status reflects raw IRQ flags status except NIS and AIS which
> > are the masked OR of the respective flags. So AFAIU NIS will be set in
> > anyway if you have RI/TI/TBU IRQs enabled.
> 
> -Serge(y)
> 
Thanks for your comment. 
From my test, NIS bit value is 0 once INTM is set, even with RI/TI flags value is 1. 
This might be the reason why previous patch set had this change.
-Jim
> >  	}
> >
> >  	/* Clear interrupts */
> > --
> > 2.34.1
> >
> >

Re: [PATCH net-next v1 0/4] net: stmmac: Enable Per DMA Channel interrupt

[Serge Semin]

Hi Leong

On Fri, Dec 22, 2023 at 01:44:47PM +0800, Leong Ching Swee wrote:
> From: Swee Leong Ching <leong.ching.swee@intel.com>
> 
> Hi,
> Add Per DMA Channel interrupt feature for DWXGMAC IP.
>  
> Patchset (link below) contains per DMA channel interrupt, But it was 
> achieved.
> https://lore.kernel.org/lkml/20230821203328.GA2197059-
> robh@kernel.org/t/#m849b529a642e1bff89c05a07efc25d6a94c8bfb4
>  
> Some of the changes in this patchset are based on reviewer comment on 
> patchset mentioned beforehand.

Thanks for resubmitting the patches. At some point in the past they
saved me some time in fixing the DW XGMAC on my platform.

-Serge(y)

> 
> Swee Leong Ching (4):
>   dt-bindings: net: snps,dwmac: per channel irq
>   net: stmmac: Make MSI interrupt routine generic
>   net: stmmac: Add support for TX/RX channel interrupt
>   net: stmmac: Use interrupt mode INTM=1 for per channel irq
> 
>  .../devicetree/bindings/net/snps,dwmac.yaml   | 24 ++++++++++----
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  4 +--
>  .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
>  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  3 ++
>  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 32 +++++++++++--------
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++--------
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 24 ++++++++++++++
>  include/linux/stmmac.h                        |  4 +--
>  8 files changed, 84 insertions(+), 38 deletions(-)
> 
> -- 
> 2.34.1
> 
>