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.
V1 -> V2: - Modified commit message and added Fixes, CC tag as suggested by Mani - reanme done to STOP - rename DW_HDMA_V0_LIE -> DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE -> DW_HDMA_V0_RWIE
Mrinmay Sarkar (2): dmaengine: dw-edma: Fix unmasking STOP and ABORT interrupts for HDMA dmaengine: dw-edma: Do not enable watermark interrupts for HDMA
drivers/dma/dw-edma/dw-hdma-v0-core.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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 - STOP, ABORT */ + tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) & + ~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK; + /* Interrupt enable - STOP, 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);
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com
Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
- Mani
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..fa89b3a 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 - STOP, ABORT */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;/* Interrupt enable - STOP, ABORT */tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;-- 2.7.4
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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;
Just curious, if all the interrupts were actually masked, how has this been even working?.. In other words if it affected both local and remote interrupts, then the HDMA driver has never actually worked, right?
/* Interrupt unmask - STOP, ABORT */
tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;
Please convert this to: + /* Interrupt unmask - stop, abort */ + tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup); + tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);
-Serge(y)
/* Interrupt enable - STOP, ABORT */tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;-- 2.7.4
On 8/2/2024 5:12 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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;Just curious, if all the interrupts were actually masked, how has this been even working?.. In other words if it affected both local and remote interrupts, then the HDMA driver has never actually worked, right?
Agreed, it should not work as interrupts were masked.
But as we are enabling LIE/RIE bits (LWIE/RWIE) that eventually enabling watermark interrupt in HDMA case and somehow on device I could see interrupt was generating with watermark and stop bit set and it was working. Since we were not clearing watermark interrupt, it was also causing storm of interrupt.
/* Interrupt unmask - STOP, ABORT */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;Please convert this to:
/* Interrupt unmask - stop, abort */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup);tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);-Serge(y)
Sure. Will do
Thanks, Mrinmay
/* Interrupt enable - STOP, ABORT */tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;-- 2.7.4
On Mon, Aug 05, 2024 at 07:30:14PM +0530, Mrinmay Sarkar wrote:
On 8/2/2024 5:12 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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;Just curious, if all the interrupts were actually masked, how has this been even working?.. In other words if it affected both local and remote interrupts, then the HDMA driver has never actually worked, right?
Agreed, it should not work as interrupts were masked.
But as we are enabling LIE/RIE bits (LWIE/RWIE) that eventually enabling watermark interrupt in HDMA case and somehow on device I could see interrupt was generating with watermark and stop bit set and it was working. Since we were not clearing watermark interrupt, it was also causing storm of interrupt.
Is it possible that the HDMA_V0_STOP_INT_MASK and HDMA_V0_ABORT_INT_MASK masks affect the local IRQs only? If so than that shall explain why for instance Kory hasn't met the problem.
Based on the "Interrupts and Error Handling" figures of the DW EDMA databook the DMA_READ_INT_MASK_OFF/DMA_WRITE_INT_MASK_OFF CSRs mask of the IRQ delivered via the edma_int[] wire. Meanwhile the IMWr TLPs generation depend on the RIE/LLRAIE flags state only.
/* Interrupt unmask - STOP, ABORT */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;Please convert this to:
/* Interrupt unmask - stop, abort */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup);tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);-Serge(y)
Sure. Will do
Thanks.
-Serge(y)
Thanks, Mrinmay
/* Interrupt enable - STOP, ABORT */tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;-- 2.7.4
On 8/7/2024 4:26 AM, Serge Semin wrote:
On Mon, Aug 05, 2024 at 07:30:14PM +0530, Mrinmay Sarkar wrote:
On 8/2/2024 5:12 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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;Just curious, if all the interrupts were actually masked, how has this been even working?.. In other words if it affected both local and remote interrupts, then the HDMA driver has never actually worked, right?
Agreed, it should not work as interrupts were masked.
But as we are enabling LIE/RIE bits (LWIE/RWIE) that eventually enabling watermark interrupt in HDMA case and somehow on device I could see interrupt was generating with watermark and stop bit set and it was working. Since we were not clearing watermark interrupt, it was also causing storm of interrupt.
Is it possible that the HDMA_V0_STOP_INT_MASK and HDMA_V0_ABORT_INT_MASK masks affect the local IRQs only? If so than that shall explain why for instance Kory hasn't met the problem.
Based on the "Interrupts and Error Handling" figures of the DW EDMA databook the DMA_READ_INT_MASK_OFF/DMA_WRITE_INT_MASK_OFF CSRs mask of the IRQ delivered via the edma_int[] wire. Meanwhile the IMWr TLPs generation depend on the RIE/LLRAIE flags state only.
Ideally HDMA_V0_STOP_INT_MASK and HDMA_V0_ABORT_INT_MASK masks affect both local and remote IRQs.
As per DW HDMA "Interrupts and Error Handling" figure HDMA_INT_SETUP_OFF_[WR| RD] mask of the IRQ delivered via the edma_int[] wire. And IMWr TLPs generation depend on 3 flags i.e HDMA_INT_SETUP_OFF_[WR| RD].RSIE flag for stop IMWr, RWIE flag for watermark IMWr and HDMA_INT_SETUP_OFF_[WR|R D].RAIE flag for abort IMWr generation.
Thanks, Mrinmay
/* Interrupt unmask - STOP, ABORT */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;Please convert this to:
/* Interrupt unmask - stop, abort */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup);tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);-Serge(y)
Sure. Will do
Thanks.
-Serge(y)
Thanks, Mrinmay
/* Interrupt enable - STOP, 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
On Wed, Aug 07, 2024 at 10:49:42PM +0530, Mrinmay Sarkar wrote:
On 8/7/2024 4:26 AM, Serge Semin wrote:
On Mon, Aug 05, 2024 at 07:30:14PM +0530, Mrinmay Sarkar wrote:
On 8/2/2024 5:12 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:31PM +0530, Mrinmay Sarkar wrote:
The current logic is enabling both STOP_INT_MASK and ABORT_INT_MASK bit. This is apparently masking those particular interrupts rather than unmasking the same. If the interrupts are masked, they would never get triggered.
So fix the issue by unmasking the STOP and ABORT interrupts properly.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org 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..fa89b3a 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;Just curious, if all the interrupts were actually masked, how has this been even working?.. In other words if it affected both local and remote interrupts, then the HDMA driver has never actually worked, right?
Agreed, it should not work as interrupts were masked.
But as we are enabling LIE/RIE bits (LWIE/RWIE) that eventually enabling watermark interrupt in HDMA case and somehow on device I could see interrupt was generating with watermark and stop bit set and it was working. Since we were not clearing watermark interrupt, it was also causing storm of interrupt.
Is it possible that the HDMA_V0_STOP_INT_MASK and HDMA_V0_ABORT_INT_MASK masks affect the local IRQs only? If so than that shall explain why for instance Kory hasn't met the problem.
Based on the "Interrupts and Error Handling" figures of the DW EDMA databook the DMA_READ_INT_MASK_OFF/DMA_WRITE_INT_MASK_OFF CSRs mask of the IRQ delivered via the edma_int[] wire. Meanwhile the IMWr TLPs generation depend on the RIE/LLRAIE flags state only.
Ideally HDMA_V0_STOP_INT_MASK and HDMA_V0_ABORT_INT_MASK masks affect both local and remote IRQs.
As per DW HDMA "Interrupts and Error Handling" figure HDMA_INT_SETUP_OFF_[WR| RD] mask of the IRQ delivered via the edma_int[] wire.
So it means they indeed affect the Local IRQs only, since the edma_int[] wire is the line locally connected to the host IRQ controller. From that perspective the semantics is the same as of the DMA_READ_INT_MASK_OFF/DMA_WRITE_INT_MASK_OFF CSR masks.
Thanks for clarification.
And IMWr TLPs generation depend on 3 flags i.e HDMA_INT_SETUP_OFF_[WR| RD].RSIE flag for stop IMWr, RWIE flag for watermark IMWr and HDMA_INT_SETUP_OFF_[WR|R D].RAIE flag for abort IMWr generation.
Thanks, Mrinmay
/* Interrupt unmask - STOP, ABORT */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup) &~HDMA_V0_STOP_INT_MASK & ~HDMA_V0_ABORT_INT_MASK;Please convert this to:
/* Interrupt unmask - stop, abort */tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup);tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);-Serge(y)
Sure. Will do
I'll wait v3 to finish up the review.
-Serge(y)
Thanks.
-Serge(y)
Thanks, Mrinmay
/* Interrupt enable - STOP, 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
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.
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.
And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits are corresponds to LWIE and RWIE bits.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com --- drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c index fa89b3a..9ad2e28 100644 --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c @@ -17,8 +17,8 @@ enum dw_hdma_control { DW_HDMA_V0_CB = BIT(0), DW_HDMA_V0_TCB = BIT(1), DW_HDMA_V0_LLP = BIT(2), - DW_HDMA_V0_LIE = BIT(3), - DW_HDMA_V0_RIE = BIT(4), + DW_HDMA_V0_LWIE = BIT(3), + DW_HDMA_V0_RWIE = BIT(4), DW_HDMA_V0_CCS = BIT(8), DW_HDMA_V0_LLE = BIT(9), }; @@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk, 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)
On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
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.
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.
And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits are corresponds to LWIE and RWIE bits.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com
Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
- Mani
drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c index fa89b3a..9ad2e28 100644 --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c @@ -17,8 +17,8 @@ enum dw_hdma_control { DW_HDMA_V0_CB = BIT(0), DW_HDMA_V0_TCB = BIT(1), DW_HDMA_V0_LLP = BIT(2),
- DW_HDMA_V0_LIE = BIT(3),
- DW_HDMA_V0_RIE = BIT(4),
- DW_HDMA_V0_LWIE = BIT(3),
- DW_HDMA_V0_RWIE = BIT(4), DW_HDMA_V0_CCS = BIT(8), DW_HDMA_V0_LLE = BIT(9),
}; @@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk, 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
On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
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.
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.
And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits are corresponds to LWIE and RWIE bits.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com
drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c index fa89b3a..9ad2e28 100644 --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c @@ -17,8 +17,8 @@ enum dw_hdma_control { DW_HDMA_V0_CB = BIT(0), DW_HDMA_V0_TCB = BIT(1), DW_HDMA_V0_LLP = BIT(2),
- DW_HDMA_V0_LIE = BIT(3),
- DW_HDMA_V0_RIE = BIT(4),
- DW_HDMA_V0_LWIE = BIT(3),
- DW_HDMA_V0_RWIE = BIT(4), DW_HDMA_V0_CCS = BIT(8), DW_HDMA_V0_LLE = BIT(9),
}; @@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk, 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);
- }
Hm, in case of DW EDMA the LIE/RIE flags of the LL entries gets to be moved to the LIE/RIE flags of the channel context register by the DMA-engine. In its turn the context register LIE/RIE flags determine whether the Local and Remote Done/Abort IRQs being raised. So without the LIE/RIE flags being set in the LL-entries the IRQs won't be raised and the whole procedure won't work. I have doubts it works differently in case of HDMA because changing the semantics would cause implementing additional logic in the DW HDMA RTL-model. Seeing the DW HDMA IP-core supports the eDMA compatibility mode it would needlessly expand the controller size. What are the rest of the CONTROL1 register fields? There must be LIE/RIE flags someplace there for the non-LL transfers and to preserve the values retrieved from the LL-entries.
Moreover the DW eDMA HW manual has a dedicated chapter called "Interrupts and Error Handling" with a very demonstrative figures describing the way the flags work. Does the DW HDMA databook have something like that?
Please also note, the DW _EDMA_ LIE and RIE flags can be also utilized for the intermediate IRQ raising, to implement the runtime LL-entries recycling pattern. The IRQ in that case is called as "watermark" IRQ in the DW EDMA HW databook, but the flags are still called as just LIE/RIE.
-Serge(y)
control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB; if (!chunk->cb) -- 2.7.4
On 8/2/2024 6:04 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
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.
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.
And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits are corresponds to LWIE and RWIE bits.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com
drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c index fa89b3a..9ad2e28 100644 --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c @@ -17,8 +17,8 @@ enum dw_hdma_control { DW_HDMA_V0_CB = BIT(0), DW_HDMA_V0_TCB = BIT(1), DW_HDMA_V0_LLP = BIT(2),
- DW_HDMA_V0_LIE = BIT(3),
- DW_HDMA_V0_RIE = BIT(4),
- DW_HDMA_V0_LWIE = BIT(3),
- DW_HDMA_V0_RWIE = BIT(4), DW_HDMA_V0_CCS = BIT(8), DW_HDMA_V0_LLE = BIT(9), };
@@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk, 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);
- }
Hm, in case of DW EDMA the LIE/RIE flags of the LL entries gets to be moved to the LIE/RIE flags of the channel context register by the DMA-engine. In its turn the context register LIE/RIE flags determine whether the Local and Remote Done/Abort IRQs being raised. So without the LIE/RIE flags being set in the LL-entries the IRQs won't be raised and the whole procedure won't work. I have doubts it works differently in case of HDMA because changing the semantics would cause implementing additional logic in the DW HDMA RTL-model. Seeing the DW HDMA IP-core supports the eDMA compatibility mode it would needlessly expand the controller size. What are the rest of the CONTROL1 register fields? There must be LIE/RIE flags someplace there for the non-LL transfers and to preserve the values retrieved from the LL-entries.
Moreover the DW eDMA HW manual has a dedicated chapter called "Interrupts and Error Handling" with a very demonstrative figures describing the way the flags work. Does the DW HDMA databook have something like that?
Please also note, the DW _EDMA_ LIE and RIE flags can be also utilized for the intermediate IRQ raising, to implement the runtime LL-entries recycling pattern. The IRQ in that case is called as "watermark" IRQ in the DW EDMA HW databook, but the flags are still called as just LIE/RIE.
-Serge(y)
Yes, you are right LIE/RIE flags need to be set without that the IRQs won't be raised in case of DW EDMA. But in DW HDMA case there in no such LIE/RIE flags and these particular bits has been mapped to LWIE and RWIE flags and these are used to enable watermark interrupt only. There is no LIE/RIE fields in HDMA_CONTROL1_OFF_WRCH register fields the same is present in EDMA CONTROL1 register.
DW HDMA has INT_SETUP register and it has LSIE/RSIE, LAIE/RAIE fields those are enabling Local and Remote Stop/Abort IRQs in LL mode.
yes DW HDMA data book also have figures in "Interrupts and Error Handling" section and there is no LIE/RIE flags and it is replaced with LWIE/RWIE flags as I mentioned above.
Thanks, Mrinmay
control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB; if (!chunk->cb) -- 2.7.4
On Mon, Aug 05, 2024 at 07:34:46PM +0530, Mrinmay Sarkar wrote:
On 8/2/2024 6:04 AM, Serge Semin wrote:
On Tue, Jul 23, 2024 at 06:49:32PM +0530, Mrinmay Sarkar wrote:
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.
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.
And also rename DW_HDMA_V0_LIE to DW_HDMA_V0_LWIE and DW_HDMA_V0_RIE to DW_HDMA_V0_RWIE as there is no LIE and RIE bits in HDMA and those bits are corresponds to LWIE and RWIE bits.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA") cc: stable@vger.kernel.org Signed-off-by: Mrinmay Sarkar quic_msarkar@quicinc.com
drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c index fa89b3a..9ad2e28 100644 --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c @@ -17,8 +17,8 @@ enum dw_hdma_control { DW_HDMA_V0_CB = BIT(0), DW_HDMA_V0_TCB = BIT(1), DW_HDMA_V0_LLP = BIT(2),
- DW_HDMA_V0_LIE = BIT(3),
- DW_HDMA_V0_RIE = BIT(4),
- DW_HDMA_V0_LWIE = BIT(3),
- DW_HDMA_V0_RWIE = BIT(4), DW_HDMA_V0_CCS = BIT(8), DW_HDMA_V0_LLE = BIT(9), };
@@ -195,25 +195,14 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk, 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);
- }
Hm, in case of DW EDMA the LIE/RIE flags of the LL entries gets to be moved to the LIE/RIE flags of the channel context register by the DMA-engine. In its turn the context register LIE/RIE flags determine whether the Local and Remote Done/Abort IRQs being raised. So without the LIE/RIE flags being set in the LL-entries the IRQs won't be raised and the whole procedure won't work. I have doubts it works differently in case of HDMA because changing the semantics would cause implementing additional logic in the DW HDMA RTL-model. Seeing the DW HDMA IP-core supports the eDMA compatibility mode it would needlessly expand the controller size. What are the rest of the CONTROL1 register fields? There must be LIE/RIE flags someplace there for the non-LL transfers and to preserve the values retrieved from the LL-entries.
Moreover the DW eDMA HW manual has a dedicated chapter called "Interrupts and Error Handling" with a very demonstrative figures describing the way the flags work. Does the DW HDMA databook have something like that?
Please also note, the DW _EDMA_ LIE and RIE flags can be also utilized for the intermediate IRQ raising, to implement the runtime LL-entries recycling pattern. The IRQ in that case is called as "watermark" IRQ in the DW EDMA HW databook, but the flags are still called as just LIE/RIE.
-Serge(y)
Yes, you are right LIE/RIE flags need to be set without that the IRQs won't be raised in case of DW EDMA. But in DW HDMA case there in no such LIE/RIE flags and these particular bits has been mapped to LWIE and RWIE flags and these are used to enable watermark interrupt only. There is no LIE/RIE fields in HDMA_CONTROL1_OFF_WRCH register fields the same is present in EDMA CONTROL1 register.
DW HDMA has INT_SETUP register and it has LSIE/RSIE, LAIE/RAIE fields those are enabling Local and Remote Stop/Abort IRQs in LL mode.
yes DW HDMA data book also have figures in "Interrupts and Error Handling" section and there is no LIE/RIE flags and it is replaced with LWIE/RWIE flags as I mentioned above.
Got it. Thanks for clarification. No more notes in this regard. Reviewed-by: Serge Semin fancer.lancer@gmail.com
-Serge(y)
Thanks, Mrinmay
control = DW_HDMA_V0_LLP | DW_HDMA_V0_TCB; if (!chunk->cb) -- 2.7.4
linux-stable-mirror@lists.linaro.org
-
Manivannan Sadhasivam -
Mrinmay Sarkar -
Serge Semin