[PATCH v1 0/2] Fix unmasking interrupt bit and remove watermark interrupt enablement

[PATCH v1 0/2] Fix unmasking interrupt bit and remove watermark interrupt enablement

[Mrinmay Sarkar]

This patch series reset STOP_INT_MASK and ABORT_INT_MASK bit and unmask
these interrupt for HDMA.

and also remove enablement of local watermark interrupt enable(LWIE)
and remote watermarek interrupt enable(RWIE) bit to avoid unnecessary
watermark interrupt event.

Testing
-------

Tested on Qualcomm SA8775P Platform.

Mrinmay Sarkar (2):
  dmaengine: dw-edma: Add fix to unmask the interrupt bit for HDMA
  dmaengine: dw-edma: Add change to remove watermark interrupt
    enablement

 drivers/dma/dw-edma/dw-hdma-v0-core.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] dmaengine: dw-edma: Add fix to unmask the interrupt bit for HDMA

[Mrinmay Sarkar]

The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK
bit. This is apparently masking those particular interrupt rather than
unmasking the same.

This change will reset STOP_INT_MASK and ABORT_INT_MASK bit and unmask
these interrupts.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 10e8f07..88bd652f 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -247,10 +247,11 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 	if (first) {
 		/* Enable engine */
 		SET_CH_32(dw, chan->dir, chan->id, ch_en, BIT(0));
-		/* Interrupt enable&unmask - done, abort */
-		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) |
-		      HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK |
-		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
+		/* Interrupt unmask - done, abort */
+		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &
+		      ~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;
+		/* Interrupt enable - done, abort */
+		tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
 		if (!(dw->chip->flags & DW_EDMA_CHIP_LOCAL))
 			tmp |= HDMA_V0_REMOTE_STOP_INT_EN | HDMA_V0_REMOTE_ABORT_INT_EN;
 		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
-- 
2.7.4

Re: [PATCH v1 1/2] dmaengine: dw-edma: Add fix to unmask the interrupt bit for HDMA

[Manivannan Sadhasivam]

On Fri, Jul 05, 2024 at 07:25:32PM +0530, Mrinmay Sarkar wrote:

Subject should be,

dmaengine: dw-edma: Fix unmasking STOP and ABORT interrupts for HDMA

> The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK
> bit. This is apparently masking those particular interrupt rather than

s/interrupt/interrupts

> unmasking the same.
> 

Please add the implications of this issue. I guess if the interrupts are masked,
they would never get triggered.

> This change will reset STOP_INT_MASK and ABORT_INT_MASK bit and unmask
> these interrupts.
> 

How about,

So fix the issue by unmasking the STOP and ABORT interrupts properly.

> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>

Please add fixes tag and CC stable as this is a potential bug fix.

> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 10e8f07..88bd652f 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -247,10 +247,11 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  	if (first) {
>  		/* Enable engine */
>  		SET_CH_32(dw, chan->dir, chan->id, ch_en, BIT(0));
> -		/* Interrupt enable&unmask - done, abort */
> -		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) |
> -		      HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK |
> -		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
> +		/* Interrupt unmask - done, abort */

There is no done interrupt in HDMA, only STOP. So use STOP, ABORT here and
below.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v1 2/2] dmaengine: dw-edma: Add change to remove watermark interrupt enablement

[Mrinmay Sarkar]

DW_HDMA_V0_LIE and DW_HDMA_V0_RIE are initialized as BIT(3) and BIT(4)
respectively in dw_hdma_control enum. But as per HDMA register these
bits are corresponds to LWIE and RWIE bit i.e local watermark interrupt
enable and remote watermarek interrupt enable. In linked list mode LWIE
and RWIE bits only enable the local and remote watermark interrupt.

As we are not handling watermark interruprt so removing watermark
interrupt enablement logic to avoid unnecessary watermark interrupt
event.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 88bd652f..aaf2e27 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -197,23 +197,13 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	struct dw_edma_burst *child;
 	struct dw_edma_chan *chan = chunk->chan;
 	u32 control = 0, i = 0;
-	int j;
 
 	if (chunk->cb)
 		control = DW_HDMA_V0_CB;
 
-	j = chunk->bursts_alloc;
-	list_for_each_entry(child, &chunk->burst->list, list) {
-		j--;
-		if (!j) {
-			control |= DW_HDMA_V0_LIE;
-			if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
-				control |= DW_HDMA_V0_RIE;
-		}
-
+	list_for_each_entry(child, &chunk->burst->list, list)
 		dw_hdma_v0_write_ll_data(chunk, i++, control, child->sz,
 					 child->sar, child->dar);
-	}
 
 	control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB;
 	if (!chunk->cb)
-- 
2.7.4

Re: [PATCH v1 2/2] dmaengine: dw-edma: Add change to remove watermark interrupt enablement

[kernel test robot]

Hi Mrinmay,

kernel test robot noticed the following build errors:

[auto build test ERROR on vkoul-dmaengine/next]
[also build test ERROR on mani-mhi/mhi-next linus/master v6.10-rc6 next-20240703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mrinmay-Sarkar/dmaengine-dw-edma-Add-fix-to-unmask-the-interrupt-bit-for-HDMA/20240706-040233
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link:    https://lore.kernel.org/r/1720187733-5380-3-git-send-email-quic_msarkar%40quicinc.com
patch subject: [PATCH v1 2/2] dmaengine: dw-edma: Add change to remove watermark interrupt enablement
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20240706/202407061620.Z6kfeKgF-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240706/202407061620.Z6kfeKgF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407061620.Z6kfeKgF-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   arch/mips/kernel/head.o: in function `__kernel_entry':
>> (.text+0x0): relocation truncated to fit: R_MIPS_26 against `kernel_entry'
   arch/mips/kernel/head.o: in function `smp_bootstrap':
>> (.ref.text+0xd8): relocation truncated to fit: R_MIPS_26 against `start_secondary'
   init/main.o: in function `set_reset_devices':
   main.c:(.init.text+0x10): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x18): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `debug_kernel':
   main.c:(.init.text+0x50): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x58): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `quiet_kernel':
   main.c:(.init.text+0x90): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x98): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `warn_bootconfig':
   main.c:(.init.text+0xd0): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0xd8): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `init_setup':
   main.c:(.init.text+0x108): additional relocation overflows omitted from the output
--
   drivers/dma/dw-edma/dw-hdma-v0-core.c: In function 'dw_hdma_v0_core_write_chunk':
>> drivers/dma/dw-edma/dw-hdma-v0-core.c:198:30: warning: unused variable 'chan' [-Wunused-variable]
     198 |         struct dw_edma_chan *chan = chunk->chan;
         |                              ^~~~


vim +/chan +198 drivers/dma/dw-edma/dw-hdma-v0-core.c

e74c39573d35e9 Cai Huoqing    2023-05-20  194  
e74c39573d35e9 Cai Huoqing    2023-05-20  195  static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
e74c39573d35e9 Cai Huoqing    2023-05-20  196  {
e74c39573d35e9 Cai Huoqing    2023-05-20  197  	struct dw_edma_burst *child;
e74c39573d35e9 Cai Huoqing    2023-05-20 @198  	struct dw_edma_chan *chan = chunk->chan;
e74c39573d35e9 Cai Huoqing    2023-05-20  199  	u32 control = 0, i = 0;
e74c39573d35e9 Cai Huoqing    2023-05-20  200  
e74c39573d35e9 Cai Huoqing    2023-05-20  201  	if (chunk->cb)
e74c39573d35e9 Cai Huoqing    2023-05-20  202  		control = DW_HDMA_V0_CB;
e74c39573d35e9 Cai Huoqing    2023-05-20  203  
882e8634dc8dd2 Mrinmay Sarkar 2024-07-05  204  	list_for_each_entry(child, &chunk->burst->list, list)
e74c39573d35e9 Cai Huoqing    2023-05-20  205  		dw_hdma_v0_write_ll_data(chunk, i++, control, child->sz,
e74c39573d35e9 Cai Huoqing    2023-05-20  206  					 child->sar, child->dar);
e74c39573d35e9 Cai Huoqing    2023-05-20  207  
e74c39573d35e9 Cai Huoqing    2023-05-20  208  	control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB;
e74c39573d35e9 Cai Huoqing    2023-05-20  209  	if (!chunk->cb)
e74c39573d35e9 Cai Huoqing    2023-05-20  210  		control |= DW_HDMA_V0_CB;
e74c39573d35e9 Cai Huoqing    2023-05-20  211  
e74c39573d35e9 Cai Huoqing    2023-05-20  212  	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
e74c39573d35e9 Cai Huoqing    2023-05-20  213  }
e74c39573d35e9 Cai Huoqing    2023-05-20  214  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] dmaengine: dw-edma: Add change to remove watermark interrupt enablement

[Manivannan Sadhasivam]

On Fri, Jul 05, 2024 at 07:25:33PM +0530, Mrinmay Sarkar wrote:

Subject should be:

dmaengine: dw-edma: Do not enable watermark interrupts for HDMA

> DW_HDMA_V0_LIE and DW_HDMA_V0_RIE are initialized as BIT(3) and BIT(4)
> respectively in dw_hdma_control enum. But as per HDMA register these
> bits are corresponds to LWIE and RWIE bit i.e local watermark interrupt
> enable and remote watermarek interrupt enable. In linked list mode LWIE
> and RWIE bits only enable the local and remote watermark interrupt.
> 

I guess you should also rename DW_HDMA_V0_LIE -> DW_HDMA_V0_LWIE and
DW_HDMA_V0_RIE -> DW_HDMA_V0_RWIE in the code, unless the register name changes
with mode.

> As we are not handling watermark interruprt so removing watermark
> interrupt enablement logic to avoid unnecessary watermark interrupt
> event.
> 

How about,

"Since the watermark interrupts are not used but enabled, this leads to
spurious interrupts getting generated. So remove the code that enables them
to avoid generating spurious watermark interrupts."

> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>

Again, please include Fixes tag and CC stable.

> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 88bd652f..aaf2e27 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -197,23 +197,13 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	struct dw_edma_burst *child;
>  	struct dw_edma_chan *chan = chunk->chan;

This becomes unused as reported by bot.

- Mani

-- 
மணிவண்ணன் சதாசிவம்