Re: [PATCH net-next v5 5/9] net: stmmac: xgmac: support per-channel irq

[Serge Semin]

On Fri, Aug 18, 2023 at 12:57:45AM +0800, Jisheng Zhang wrote:
> The IP supports per channel interrupt, add support for this usage case.
> 
> Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> ---
>  .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 ++
>  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    | 33 +++++++++++--------
>  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 7f68bef456b7..18a042834d75 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -340,6 +340,8 @@
>  
>  /* DMA Registers */
>  #define XGMAC_DMA_MODE			0x00003000
> +#define XGMAC_INTM			GENMASK(13, 12)
> +#define XGMAC_INTM_MODE1		0x1
>  #define XGMAC_SWR			BIT(0)
>  #define XGMAC_DMA_SYSBUS_MODE		0x00003004
>  #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index 1ef8fc132c2d..ce228c362403 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->perch_irq_en) {
> +		value = readl(ioaddr + XGMAC_DMA_MODE);
> +		value &= ~XGMAC_INTM;
> +		value |= FIELD_PREP(XGMAC_INTM, XGMAC_INTM_MODE1);
> +		writel(value, ioaddr + XGMAC_DMA_MODE);
> +	}
>  }
>  
>  static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv,
> @@ -365,20 +372,20 @@ 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(&rx_q->rxq_stats.syncp);
> -			rx_q->rxq_stats.rx_normal_irq_n++;
> -			u64_stats_update_end(&rx_q->rxq_stats.syncp);
> -			ret |= handle_rx;
> -		}
> -		if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
> -			u64_stats_update_begin(&tx_q->txq_stats.syncp);
> -			tx_q->txq_stats.tx_normal_irq_n++;
> -			u64_stats_update_end(&tx_q->txq_stats.syncp);
> -			ret |= handle_tx;
> -		}
> +	if (likely(intr_status & XGMAC_RI)) {
> +		u64_stats_update_begin(&rx_q->rxq_stats.syncp);
> +		rx_q->rxq_stats.rx_normal_irq_n++;
> +		u64_stats_update_end(&rx_q->rxq_stats.syncp);
> +		ret |= handle_rx;
> +	}
> +	if (likely(intr_status & XGMAC_TI)) {
> +		u64_stats_update_begin(&tx_q->txq_stats.syncp);
> +		tx_q->txq_stats.tx_normal_irq_n++;
> +		u64_stats_update_end(&tx_q->txq_stats.syncp);
> +		ret |= handle_tx;
>  	}
> +	if (unlikely(intr_status & XGMAC_TBU))
> +		ret |= handle_tx;

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 */
>  	writel(intr_en & intr_status, ioaddr + XGMAC_DMA_CH_STATUS(chan));
> -- 
> 2.40.1
> 
>