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

Serge Semin
2023-08-18T17:11:15Z

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@kernel.org>
> ---
> .../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
>
>