RE: [PATCH v3] dmaengine: dw-edma: Enable HDMA 64R/W Channels

[Verma, Devendra]

Public

> -----Original Message-----
> From: Frank Li <Frank.li@xxxxxxx>
> Sent: Friday, June 5, 2026 01:28
> To: Verma, Devendra <Devendra.Verma@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; mani@xxxxxxxxxx; vkoul@xxxxxxxxxx;
> Frank.Li@xxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>
> Subject: Re: [PATCH v3] dmaengine: dw-edma: Enable HDMA 64R/W
> Channels
>
> On Wed, Jun 03, 2026 at 08:11:47PM +0530, Devendra K Verma wrote:
> > As per 'Designware Cores PCI Express Controller Databook', Section 7.1
> > - Overview, HDMA supports 64 Read and 64 Write channels. Current
> > controller driver supports up to 8 read and write channels only. In
> > order to utilize all the channels the controller driver need to have
> > the channel related structs and variables as per the number of
> > channels supported by IP.
> > Following changes are made to enable 64 Read / 64 Write channel
> > support:
> >
> >  o Defined HDMA specific macros to reflect the channel count.
> >  o The count of ll_regions and dt_regions in dw_edma_chip and
> >    dw_edma_pcie_data shall be in accordance to number of read
> >    and write channels.
> >  o In dw_edma_probe() configure the channels as per the channels
> >    of the IP used.
> >  o Changed mask types to u64 for higher channel counts.
> >
> > Signed-off-by: Devendra K Verma <devendra.verma@xxxxxxx>
> > ---
> > Changes in v2:
> >   o Fixed the pre-existing bug related to GET_CH_32
> >     interchanging the channel direction and id.
> >     This bug was not caused by any version of this patch.
> >   o Fixed the issue when using for_each_set_bit() for mask
> >     of u64 type.
> >
> > Changes in v1:
> >   o On review recommendation of sashiko bot, in the function
> >     dw_hdma_v0_core_off(), the loop iterates over registers
> >     as per the number of channels enabled and not on total
> >     number of channels supported.
> >   o Changed mask types to u64 for higher channel counts.
> > ---
> ...
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > @@ -53,13 +53,24 @@ __dw_ch_regs(struct dw_edma *dw, enum
> dw_edma_dir
> > dir, u16 ch)  static void dw_hdma_v0_core_off(struct dw_edma *dw)  {
> >     int id;
> > +   enum dw_edma_dir dir;
> > +
> > +   dir = EDMA_DIR_WRITE;
> > +   for (id = 0; id < dw->wr_ch_cnt; id++) {
> > +           SET_CH_32(dw, dir, id, int_setup,
> > +                     HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > +           SET_CH_32(dw, dir, id, int_clear,
> > +                     HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > +           SET_CH_32(dw, dir, id, ch_en, 0);
> > +   }
> >
> > -   for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> > -           SET_BOTH_CH_32(dw, id, int_setup,
> > -                          HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > -           SET_BOTH_CH_32(dw, id, int_clear,
> > -                          HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > -           SET_BOTH_CH_32(dw, id, ch_en, 0);
> > +   dir = EDMA_DIR_READ;
> > +   for (id = 0; id < dw->rd_ch_cnt; id++) {
> > +           SET_CH_32(dw, dir, id, int_setup,
> > +                     HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > +           SET_CH_32(dw, dir, id, int_clear,
> > +                     HDMA_V0_STOP_INT_MASK |
> HDMA_V0_ABORT_INT_MASK);
> > +           SET_CH_32(dw, dir, id, ch_en, 0);
>
> why SET_BOTH_CH_32 not work for 64 channel?
>

SET_BOTH_CH_32 works, but this needs to be done on the channels enabled for the IP.
HDMA supports maximum of 64 channels. So if some IP enables 8 or fewer read / write channels only then the number of channels come from dw->wr_ch_cnt and dw->rd_ch_cnt. Now the logic is derived by individual read & write enabled channel count. Earlier, it was assumed that user will enable max of 8 channels which would have worked well using SET_BOTH_CH_32() but as the channels grow, the assumption that equal number of read / write channels and that they are set to max count are enabled might not hold true.

- Devendra

> >     }
> >  }
> >
> > @@ -79,7 +90,7 @@ static enum dma_status
> dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
> >     u32 tmp;
> >
> >     tmp = FIELD_GET(HDMA_V0_CH_STATUS_MASK,
> > -                   GET_CH_32(dw, chan->id, chan->dir, ch_stat));
> > +                   GET_CH_32(dw, chan->dir, chan->id, ch_stat));
>
> why need swtich id and dir here ?
>
> Frank

This is the correct order of arguments to the GET_CH_32. The second & third arguments shall be direction and channel_id respectively. It is a pre-existing issue reported by AI bot.

> >
> >     if (tmp == 1)
> >             return DMA_IN_PROGRESS;
> > @@ -118,7 +129,8 @@ dw_hdma_v0_core_handle_int(struct dw_edma_irq
> *dw_irq, enum dw_edma_dir dir,
> >     unsigned long total, pos, val;
> >     irqreturn_t ret = IRQ_NONE;
> >     struct dw_edma_chan *chan;
> > -   unsigned long off, mask;
> > +   unsigned long off;
> > +   u64 mask;
> >
> >     if (dir == EDMA_DIR_WRITE) {
> >             total = dw->wr_ch_cnt;
> > @@ -130,7 +142,11 @@ dw_hdma_v0_core_handle_int(struct
> dw_edma_irq *dw_irq, enum dw_edma_dir dir,
> >             mask = dw_irq->rd_mask;
> >     }
> >
> > -   for_each_set_bit(pos, &mask, total) {
> > +   while (mask) {
> > +           pos = __ffs64(mask);
> > +           if (pos >= total)
> > +                   break;
> > +
> >             chan = &dw->chan[pos + off];
> >
> >             val = dw_hdma_v0_core_status_int(chan); @@ -147,6 +163,7
> @@
> > dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum
> > dw_edma_dir dir,
> >
> >                     ret = IRQ_HANDLED;
> >             }
> > +           mask &= mask - 1;
> >     }
> >
> >     return ret;
> > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > index 7759ba9b4850..48e40efceb2e 100644
> > --- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > @@ -11,7 +11,7 @@
> >
> >  #include <linux/dmaengine.h>
> >
> > -#define HDMA_V0_MAX_NR_CH                  8
> > +#define HDMA_V0_MAX_NR_CH                  64
> >  #define HDMA_V0_CH_EN                              BIT(0)
> >  #define HDMA_V0_LOCAL_ABORT_INT_EN         BIT(6)
> >  #define HDMA_V0_REMOTE_ABORT_INT_EN                BIT(5)
> > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h index
> > 1fafd5b0e315..da7a5cc93ad4 100644
> > --- a/include/linux/dma/edma.h
> > +++ b/include/linux/dma/edma.h
> > @@ -14,6 +14,8 @@
> >
> >  #define EDMA_MAX_WR_CH                                  8
> >  #define EDMA_MAX_RD_CH                                  8
> > +#define HDMA_MAX_WR_CH                                  64
> > +#define HDMA_MAX_RD_CH                                  64
> >
> >  struct dw_edma;
> >
> > @@ -89,12 +91,12 @@ struct dw_edma_chip {
> >     u16                     ll_wr_cnt;
> >     u16                     ll_rd_cnt;
> >     /* link list address */
> > -   struct dw_edma_region   ll_region_wr[EDMA_MAX_WR_CH];
> > -   struct dw_edma_region   ll_region_rd[EDMA_MAX_RD_CH];
> > +   struct dw_edma_region   ll_region_wr[HDMA_MAX_WR_CH];
> > +   struct dw_edma_region   ll_region_rd[HDMA_MAX_RD_CH];
> >
> >     /* data region */
> > -   struct dw_edma_region   dt_region_wr[EDMA_MAX_WR_CH];
> > -   struct dw_edma_region   dt_region_rd[EDMA_MAX_RD_CH];
> > +   struct dw_edma_region   dt_region_wr[HDMA_MAX_WR_CH];
> > +   struct dw_edma_region   dt_region_rd[HDMA_MAX_RD_CH];
> >
> >     /* interrupt emulation */
> >     int                     db_irq;
> > --
> > 2.43.0
> >