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

["Verma, Devendra" <devverma@amd.com>]

On 05-Jun-26 23:45, Frank Li wrote:
> On Fri, Jun 05, 2026 at 11:48:05AM +0000, Verma, Devendra wrote:
>> Public
>>
>>> -----Original Message-----
>>> From: Frank Li <Frank.li@nxp.com>
>>> Sent: Friday, June 5, 2026 01:28
>>> To: Verma, Devendra <Devendra.Verma@amd.com>
>>> Cc: bhelgaas@google.com; mani@kernel.org; vkoul@kernel.org;
>>> Frank.Li@kernel.org; dmaengine@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>
>>> 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@amd.com>
>>>> ---
>>>> 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.
> 
> Make sense, please wrap your reply, it is hard to read
> 

[Reformatted the comment to fit within the visible window]

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 to be
configured shall come from dw->wr_ch_cnt and dw->rd_ch_cnt. In the new
code 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.
> 
> AI found existing problem, need seperate patch to fix it.
> 
> Frank

Removed this fix. Will include it as part of separate series.
I will upload the next version with the above recommendation.

-Devendra