[PATCH v2 0/5] Fix support of dw-edma HDMA NATIVE IP in remote setup

[PATCH v2 0/5] Fix support of dw-edma HDMA NATIVE IP in remote setup

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

This patch series fix the support of dw-edma HDMA NATIVE IP.
I can only test it in remote HDMA IP setup with single dma transfer, but
with these fixes it works properly.

Few fixes has also been added for eDMA version. Similarly to HDMA I have
tested only eDMA in remote setup.

Kory Maincent (5):
  dmaengine: dw-edma: Fix the ch_count hdma callback
  dmaengine: dw-edma: Typos fixes
  dmaengine: dw-edma: Add HDMA remote interrupt configuration
  dmaengine: dw-edma: HDMA: Add sync read before starting the DMA
    transfer in remote setup
  dmaengine: dw-edma: eDMA: Add sync read before starting the DMA
    transfer in remote setup

 drivers/dma/dw-edma/dw-edma-v0-core.c | 22 ++++++++++++++
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 43 +++++++++++++++++++--------
 drivers/dma/dw-edma/dw-hdma-v0-regs.h |  2 +-
 3 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] dmaengine: dw-edma: Fix the ch_count hdma callback

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

The current check of ch_en enabled to know the maximum number of available
hardware channels is wrong as it check the number of ch_en register set
but all of them are unset at probe. This register is set at the
dw_hdma_v0_core_start function which is run lately before a DMA transfer.

The HDMA IP have no way to know the number of hardware channels available
like the eDMA IP, then let set it to maximum channels and let the platform
set the right number of channels.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

See the following thread mail that talk about this issue:
https://lore.kernel.org/lkml/20230607095832.6d6b1a73@kmaincent-XPS-13-7390/

Changes in v2:
- Add comment
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 00b735a0202a..3e78d4fd3955 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -65,18 +65,11 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
 
 static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
 {
-	u32 num_ch = 0;
-	int id;
-
-	for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
-		if (GET_CH_32(dw, id, dir, ch_en) & BIT(0))
-			num_ch++;
-	}
-
-	if (num_ch > HDMA_V0_MAX_NR_CH)
-		num_ch = HDMA_V0_MAX_NR_CH;
-
-	return (u16)num_ch;
+	/* The HDMA IP have no way to know the number of hardware channels
+	 * available, we set it to maximum channels and let the platform
+	 * set the right number of channels.
+	 */
+	return HDMA_V0_MAX_NR_CH;
 }
 
 static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
-- 
2.25.1

Re: [PATCH v2 1/5] dmaengine: dw-edma: Fix the ch_count hdma callback

[Serge Semin]

Hi Cai,

On Mon, Oct 02, 2023 at 03:17:45PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> The current check of ch_en enabled to know the maximum number of available
> hardware channels is wrong as it check the number of ch_en register set
> but all of them are unset at probe. This register is set at the
> dw_hdma_v0_core_start function which is run lately before a DMA transfer.
> 
> The HDMA IP have no way to know the number of hardware channels available
> like the eDMA IP, then let set it to maximum channels and let the platform
> set the right number of channels.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 

> See the following thread mail that talk about this issue:
> https://lore.kernel.org/lkml/20230607095832.6d6b1a73@kmaincent-XPS-13-7390/

Cai, do you have anything to add in regards to this change and to the
discussion available in the thread above? If no, I guess the solution
provided in this patch is the best we can currently come up with.

-Serge(y)

> 
> Changes in v2:
> - Add comment
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 00b735a0202a..3e78d4fd3955 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -65,18 +65,11 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
>  
>  static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
>  {
> -	u32 num_ch = 0;
> -	int id;
> -
> -	for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> -		if (GET_CH_32(dw, id, dir, ch_en) & BIT(0))
> -			num_ch++;
> -	}
> -
> -	if (num_ch > HDMA_V0_MAX_NR_CH)
> -		num_ch = HDMA_V0_MAX_NR_CH;
> -
> -	return (u16)num_ch;
> +	/* The HDMA IP have no way to know the number of hardware channels
> +	 * available, we set it to maximum channels and let the platform
> +	 * set the right number of channels.
> +	 */
> +	return HDMA_V0_MAX_NR_CH;
>  }
>  
>  static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/5] dmaengine: dw-edma: Fix the ch_count hdma callback

[Manivannan Sadhasivam]

On Mon, Oct 02, 2023 at 03:17:45PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> The current check of ch_en enabled to know the maximum number of available
> hardware channels is wrong as it check the number of ch_en register set
> but all of them are unset at probe. This register is set at the
> dw_hdma_v0_core_start function which is run lately before a DMA transfer.
> 
> The HDMA IP have no way to know the number of hardware channels available
> like the eDMA IP, then let set it to maximum channels and let the platform
> set the right number of channels.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

One nitpick below. With that,

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
> 
> See the following thread mail that talk about this issue:
> https://lore.kernel.org/lkml/20230607095832.6d6b1a73@kmaincent-XPS-13-7390/
> 
> Changes in v2:
> - Add comment
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 00b735a0202a..3e78d4fd3955 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -65,18 +65,11 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
>  
>  static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
>  {
> -	u32 num_ch = 0;
> -	int id;
> -
> -	for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> -		if (GET_CH_32(dw, id, dir, ch_en) & BIT(0))
> -			num_ch++;
> -	}
> -
> -	if (num_ch > HDMA_V0_MAX_NR_CH)
> -		num_ch = HDMA_V0_MAX_NR_CH;
> -
> -	return (u16)num_ch;
> +	/* The HDMA IP have no way to know the number of hardware channels

Please use below style for multi line comment:

	   /*
	    * ...
	    */

- Mani

> +	 * available, we set it to maximum channels and let the platform
> +	 * set the right number of channels.
> +	 */
> +	return HDMA_V0_MAX_NR_CH;
>  }
>  
>  static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
> -- 
> 2.25.1
> 

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

[PATCH v2 2/5] dmaengine: dw-edma: Typos fixes

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Fix "HDMA_V0_REMOTEL_STOP_INT_EN" typo error.
Fix "HDMA_V0_LOCAL_STOP_INT_EN" to "HDMA_V0_LOCAL_ABORT_INT_EN" as the STOP
bit is already set in the same line.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 +-
 drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 3e78d4fd3955..0afafc683a9e 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -235,7 +235,7 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		/* 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_STOP_INT_EN;
+		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
 		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
 		/* Channel control */
 		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
index a974abdf8aaf..eab5fd7177e5 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
+++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
@@ -15,7 +15,7 @@
 #define HDMA_V0_LOCAL_ABORT_INT_EN		BIT(6)
 #define HDMA_V0_REMOTE_ABORT_INT_EN		BIT(5)
 #define HDMA_V0_LOCAL_STOP_INT_EN		BIT(4)
-#define HDMA_V0_REMOTEL_STOP_INT_EN		BIT(3)
+#define HDMA_V0_REMOTE_STOP_INT_EN		BIT(3)
 #define HDMA_V0_ABORT_INT_MASK			BIT(2)
 #define HDMA_V0_STOP_INT_MASK			BIT(0)
 #define HDMA_V0_LINKLIST_EN			BIT(0)
-- 
2.25.1

Re: [PATCH v2 2/5] dmaengine: dw-edma: Typos fixes

[Manivannan Sadhasivam]

On Mon, Oct 02, 2023 at 03:17:46PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Fix "HDMA_V0_REMOTEL_STOP_INT_EN" typo error.
> Fix "HDMA_V0_LOCAL_STOP_INT_EN" to "HDMA_V0_LOCAL_ABORT_INT_EN" as the STOP
> bit is already set in the same line.
> 

You should split this into two patches. First one is a typo and is harmless, but
the second is a _bug_.

- Mani

> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 +-
>  drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 3e78d4fd3955..0afafc683a9e 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -235,7 +235,7 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		/* 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_STOP_INT_EN;
> +		      HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
>  		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
>  		/* Channel control */
>  		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> index a974abdf8aaf..eab5fd7177e5 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> @@ -15,7 +15,7 @@
>  #define HDMA_V0_LOCAL_ABORT_INT_EN		BIT(6)
>  #define HDMA_V0_REMOTE_ABORT_INT_EN		BIT(5)
>  #define HDMA_V0_LOCAL_STOP_INT_EN		BIT(4)
> -#define HDMA_V0_REMOTEL_STOP_INT_EN		BIT(3)
> +#define HDMA_V0_REMOTE_STOP_INT_EN		BIT(3)
>  #define HDMA_V0_ABORT_INT_MASK			BIT(2)
>  #define HDMA_V0_STOP_INT_MASK			BIT(0)
>  #define HDMA_V0_LINKLIST_EN			BIT(0)
> -- 
> 2.25.1
> 

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

Re: [PATCH v2 2/5] dmaengine: dw-edma: Typos fixes

[Köry Maincent]

On Tue, 10 Oct 2023 20:29:06 +0530
Manivannan Sadhasivam <mani@kernel.org> wrote:

> On Mon, Oct 02, 2023 at 03:17:46PM +0200, Köry Maincent wrote:
> > From: Kory Maincent <kory.maincent@bootlin.com>
> > 
> > Fix "HDMA_V0_REMOTEL_STOP_INT_EN" typo error.
> > Fix "HDMA_V0_LOCAL_STOP_INT_EN" to "HDMA_V0_LOCAL_ABORT_INT_EN" as the STOP
> > bit is already set in the same line.
> >   
> 
> You should split this into two patches. First one is a typo and is harmless,
> but the second is a _bug_.

Thanks for your review.
Ok I will do so.
Serge if it is ok for you I will keep your reviewed by on the two separate
patches.

Re: [PATCH v2 2/5] dmaengine: dw-edma: Typos fixes

[Serge Semin]

On Wed, Oct 11, 2023 at 09:23:50AM +0200, Köry Maincent wrote:
> On Tue, 10 Oct 2023 20:29:06 +0530
> Manivannan Sadhasivam <mani@kernel.org> wrote:
> 
> > On Mon, Oct 02, 2023 at 03:17:46PM +0200, Köry Maincent wrote:
> > > From: Kory Maincent <kory.maincent@bootlin.com>
> > > 
> > > Fix "HDMA_V0_REMOTEL_STOP_INT_EN" typo error.
> > > Fix "HDMA_V0_LOCAL_STOP_INT_EN" to "HDMA_V0_LOCAL_ABORT_INT_EN" as the STOP
> > > bit is already set in the same line.
> > >   
> > 
> > You should split this into two patches. First one is a typo and is harmless,
> > but the second is a _bug_.
> 
> Thanks for your review.
> Ok I will do so.
> Serge if it is ok for you I will keep your reviewed by on the two separate
> patches.

Yeah, it's ok.

-Serge(y)

[PATCH v2 3/5] dmaengine: dw-edma: Add HDMA remote interrupt configuration

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

Only the local interruption was configured, remote interrupt was left
behind. This patch fix it by setting stop and abort remote interrupts when
the DW_EDMA_CHIP_LOCAL flag is not set.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 0afafc683a9e..0cce1880cfdc 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -236,6 +236,8 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		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;
+		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);
 		/* Channel control */
 		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
-- 
2.25.1

Re: [PATCH v2 3/5] dmaengine: dw-edma: Add HDMA remote interrupt configuration

[Manivannan Sadhasivam]

On Mon, Oct 02, 2023 at 03:17:47PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Only the local interruption was configured, remote interrupt was left
> behind. This patch fix it by setting stop and abort remote interrupts when
> the DW_EDMA_CHIP_LOCAL flag is not set.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0afafc683a9e..0cce1880cfdc 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -236,6 +236,8 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		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;
> +		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);
>  		/* Channel control */
>  		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
> -- 
> 2.25.1
> 

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

[PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

The Linked list element and pointer are not stored in the same memory as
the HDMA controller register. If the doorbell register is toggled before
the full write of the linked list a race condition error can appears.
In remote setup we can only use a readl to the memory to assured the full
write has occurred.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move the sync read in a function.
- Add commments
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 0cce1880cfdc..26b5020dcc2a 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -221,6 +221,25 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
 }
 
+/**
+ * dw_hdma_v0_sync_ll_data() - sync the ll data write
+ * @chunk: dma chunk
+ *
+ * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
+ * configuration registers and Application memory are normally accesse
+ * over different buses. We need to insure ll data has been written before
+ * toggling the doorbell register.
+ */
+static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
+{
+	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+		/* Linux memory barriers don't cater for what's required here.
+		 * What's required is what's here - a read of the linked
+		 * list region.
+		 */
+		readl(chunk->ll_region.vaddr.io);
+}
+
 static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 {
 	struct dw_edma_chan *chan = chunk->chan;
@@ -251,6 +270,9 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 	/* Set consumer cycle */
 	SET_CH_32(dw, chan->dir, chan->id, cycle_sync,
 		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
+
+	dw_hdma_v0_sync_ll_data(chunk);
+
 	/* Doorbell */
 	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
 }
-- 
2.25.1

Re: [PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Serge Semin]

On Mon, Oct 02, 2023 at 03:17:48PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> The Linked list element and pointer are not stored in the same memory as
> the HDMA controller register. If the doorbell register is toggled before
> the full write of the linked list a race condition error can appears.
> In remote setup we can only use a readl to the memory to assured the full
> write has occurred.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v2:
> - Move the sync read in a function.
> - Add commments
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0cce1880cfdc..26b5020dcc2a 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -221,6 +221,25 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
>  }
>  
> +/**
> + * dw_hdma_v0_sync_ll_data() - sync the ll data write
> + * @chunk: dma chunk
> + *

> + * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
> + * configuration registers and Application memory are normally accesse

accessed

> + * over different buses. We need to insure ll data has been written before
> + * toggling the doorbell register.

1. Please replace "We need to insure ..." with "Ensure LL-data reaches
the memory before the doorbell register is toggled by issuing the
dummy-read from the remote LL memory in a hope that the posted MRd TLP
will return only after the last MWr TLP is completed".

2. Please move this comment to being above the if-statement. The
driver doesn't use kdoc at all. Having it for just a single function
doesn't look well, especially seeing it's static and isn't take part
of the kernel API.

> + */
> +static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
> +{
> +	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))

> +		/* Linux memory barriers don't cater for what's required here.
> +		 * What's required is what's here - a read of the linked
> +		 * list region.
> +		 */

1. The comment isn't that much informative. If it wasn't required then
why would have this been needed in the first place? Anyway just drop it since
you'll move the kdoc detailed comment to being above the if-statement.

2. Note the preferred style for the multi-line comments is:
   /*
    *...
    */
except for the networking subsystem. It's DMA so normal format should
be utilized. I know there are several comments in this driver which
are defined in the net-format. Just ignore them. These are legacy code
which should be eventually fixed to comply with the preferred style.

-Serge(y)

> +		readl(chunk->ll_region.vaddr.io);
> +}
> +
>  static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  {
>  	struct dw_edma_chan *chan = chunk->chan;
> @@ -251,6 +270,9 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  	/* Set consumer cycle */
>  	SET_CH_32(dw, chan->dir, chan->id, cycle_sync,
>  		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
> +
> +	dw_hdma_v0_sync_ll_data(chunk);
> +
>  	/* Doorbell */
>  	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
>  }
> -- 
> 2.25.1
> 

[PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

The Linked list element and pointer are not stored in the same memory as
the HDMA controller register. If the doorbell register is toggled before
the full write of the linked list a race condition error can appears.
In remote setup we can only use a readl to the memory to assured the full
write has occurred.

Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move the sync read in a function.
- Add commments
---
 drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 0cce1880cfdc..9109dd6c2e76 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -221,6 +221,20 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
 }
 
+static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
+{
+	/*
+	 * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
+	 * configuration registers and Application memory are normally accessed
+	 * over different buses. Ensure LL-data reaches the memory before the
+	 * doorbell register is toggled by issuing the dummy-read from the remote
+	 * LL memory in a hope that the posted MRd TLP will return only after the
+	 * last MWr TLP is completed
+	 */
+	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+		readl(chunk->ll_region.vaddr.io);
+}
+
 static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 {
 	struct dw_edma_chan *chan = chunk->chan;
@@ -251,6 +265,9 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 	/* Set consumer cycle */
 	SET_CH_32(dw, chan->dir, chan->id, cycle_sync,
 		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
+
+	dw_hdma_v0_sync_ll_data(chunk);
+
 	/* Doorbell */
 	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
 }
-- 
2.25.1

Re: [PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Serge Semin]

On Tue, Oct 03, 2023 at 02:15:42PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> The Linked list element and pointer are not stored in the same memory as
> the HDMA controller register. If the doorbell register is toggled before
> the full write of the linked list a race condition error can appears.
> In remote setup we can only use a readl to the memory to assured the full
> write has occurred.
> 
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v2:
> - Move the sync read in a function.
> - Add commments

Note you need to resubmit the entire series if any of its part has
changed. So please add these patches to your patchset (in place of the
4/5 and 5/5 patches I commented) and resend it as v3.

-Serge(y)

> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0cce1880cfdc..9109dd6c2e76 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -221,6 +221,20 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
>  }
>  
> +static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
> +{
> +	/*
> +	 * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
> +	 * configuration registers and Application memory are normally accessed
> +	 * over different buses. Ensure LL-data reaches the memory before the
> +	 * doorbell register is toggled by issuing the dummy-read from the remote
> +	 * LL memory in a hope that the posted MRd TLP will return only after the
> +	 * last MWr TLP is completed
> +	 */
> +	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +		readl(chunk->ll_region.vaddr.io);
> +}
> +
>  static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  {
>  	struct dw_edma_chan *chan = chunk->chan;
> @@ -251,6 +265,9 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  	/* Set consumer cycle */
>  	SET_CH_32(dw, chan->dir, chan->id, cycle_sync,
>  		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
> +
> +	dw_hdma_v0_sync_ll_data(chunk);
> +
>  	/* Doorbell */
>  	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
>  }
> -- 
> 2.25.1
> 

Re: [PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Köry Maincent]

On Tue, 3 Oct 2023 18:20:23 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> On Tue, Oct 03, 2023 at 02:15:42PM +0200, Köry Maincent wrote:
> > From: Kory Maincent <kory.maincent@bootlin.com>
> > 
> > The Linked list element and pointer are not stored in the same memory as
> > the HDMA controller register. If the doorbell register is toggled before
> > the full write of the linked list a race condition error can appears.
> > In remote setup we can only use a readl to the memory to assured the full
> > write has occurred.
> > 
> > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > ---
> > 
> > Changes in v2:
> > - Move the sync read in a function.
> > - Add commments  
> 
> Note you need to resubmit the entire series if any of its part has
> changed. So please add these patches to your patchset (in place of the
> 4/5 and 5/5 patches I commented) and resend it as v3.

Alright.
Should I wait for Cai's response for patch 1/5 before sending v3. He seems to
never having woken up in our discussions.

Re: [PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Serge Semin]

On Tue, Oct 03, 2023 at 05:34:32PM +0200, Köry Maincent wrote:
> On Tue, 3 Oct 2023 18:20:23 +0300
> Serge Semin <fancer.lancer@gmail.com> wrote:
> 
> > On Tue, Oct 03, 2023 at 02:15:42PM +0200, Köry Maincent wrote:
> > > From: Kory Maincent <kory.maincent@bootlin.com>
> > > 
> > > The Linked list element and pointer are not stored in the same memory as
> > > the HDMA controller register. If the doorbell register is toggled before
> > > the full write of the linked list a race condition error can appears.
> > > In remote setup we can only use a readl to the memory to assured the full
> > > write has occurred.
> > > 
> > > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > > ---
> > > 
> > > Changes in v2:
> > > - Move the sync read in a function.
> > > - Add commments  
> > 
> > Note you need to resubmit the entire series if any of its part has
> > changed. So please add these patches to your patchset (in place of the
> > 4/5 and 5/5 patches I commented) and resend it as v3.
> 
> Alright.
> Should I wait for Cai's response for patch 1/5 before sending v3. He seems to
> never having woken up in our discussions.

Ok. Let's wait for Cai for sometime. We are in the middle of the
dev-cycle anyway so no reason to rush.

-Serge(y)

[PATCH v2 5/5] dmaengine: dw-edma: eDMA: Add sync read before starting the DMA transfer in remote setup

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

The Linked list element and pointer are not stored in the same memory as
the eDMA controller register. If the doorbell register is toggled before
the full write of the linked list a race condition error can appears.
In remote setup we can only use a readl to the memory to assured the full
write has occurred.

Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/dma/dw-edma/dw-edma-v0-core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index b38786f0ad79..75c0b1fa9c40 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -346,6 +346,25 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	dw_edma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
 }
 
+/**
+ * dw_edma_v0_sync_ll_data() - sync the ll data write
+ * @chunk: dma chunk
+ *
+ * In case of remote eDMA engine setup, the DW PCIe RP/EP internals
+ * configuration registers and Application memory are normally accesse
+ * over different buses. We need to insure ll data has been written before
+ * toggling the doorbell register.
+ */
+static void dw_edma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
+{
+	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+		/* Linux memory barriers don't cater for what's required here.
+		 * What's required is what's here - a read of the linked
+		 * list region.
+		 */
+		readl(chunk->ll_region.vaddr.io);
+}
+
 static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 {
 	struct dw_edma_chan *chan = chunk->chan;
@@ -412,6 +431,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		SET_CH_32(dw, chan->dir, chan->id, llp.msb,
 			  upper_32_bits(chunk->ll_region.paddr));
 	}
+
+	dw_edma_v0_sync_ll_data(chunk);
+
 	/* Doorbell */
 	SET_RW_32(dw, chan->dir, doorbell,
 		  FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
-- 
2.25.1

Re: [PATCH v2 5/5] dmaengine: dw-edma: eDMA: Add sync read before starting the DMA transfer in remote setup

[Serge Semin]

On Mon, Oct 02, 2023 at 03:17:49PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> The Linked list element and pointer are not stored in the same memory as
> the eDMA controller register. If the doorbell register is toggled before
> the full write of the linked list a race condition error can appears.
> In remote setup we can only use a readl to the memory to assured the full
> write has occurred.
> 
> Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/dma/dw-edma/dw-edma-v0-core.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index b38786f0ad79..75c0b1fa9c40 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -346,6 +346,25 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	dw_edma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
>  }
>  

> +/**
> + * dw_edma_v0_sync_ll_data() - sync the ll data write
> + * @chunk: dma chunk
> + *
> + * In case of remote eDMA engine setup, the DW PCIe RP/EP internals
> + * configuration registers and Application memory are normally accesse
> + * over different buses. We need to insure ll data has been written before
> + * toggling the doorbell register.
> + */
> +static void dw_edma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
> +{
> +	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +		/* Linux memory barriers don't cater for what's required here.
> +		 * What's required is what's here - a read of the linked
> +		 * list region.
> +		 */

The same comments as for [PATCH v2 4/5].

-Serge(y)

> +		readl(chunk->ll_region.vaddr.io);
> +}
> +
>  static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  {
>  	struct dw_edma_chan *chan = chunk->chan;
> @@ -412,6 +431,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		SET_CH_32(dw, chan->dir, chan->id, llp.msb,
>  			  upper_32_bits(chunk->ll_region.paddr));
>  	}
> +
> +	dw_edma_v0_sync_ll_data(chunk);
> +
>  	/* Doorbell */
>  	SET_RW_32(dw, chan->dir, doorbell,
>  		  FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
> -- 
> 2.25.1
> 

[PATCH v2 5/5] dmaengine: dw-edma: eDMA: Add sync read before starting the DMA transfer in remote setup

[Köry Maincent]

From: Kory Maincent <kory.maincent@bootlin.com>

The Linked list element and pointer are not stored in the same memory as
the eDMA controller register. If the doorbell register is toggled before
the full write of the linked list a race condition error can appears.
In remote setup we can only use a readl to the memory to assured the full
write has occurred.

Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/dma/dw-edma/dw-edma-v0-core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index b38786f0ad79..6245b720fbfe 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -346,6 +346,20 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	dw_edma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
 }
 
+static void dw_edma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
+{
+	/*
+	 * In case of remote eDMA engine setup, the DW PCIe RP/EP internals
+	 * configuration registers and Application memory are normally accessed
+	 * over different buses. Ensure LL-data reaches the memory before the
+	 * doorbell register is toggled by issuing the dummy-read from the remote
+	 * LL memory in a hope that the posted MRd TLP will return only after the
+	 * last MWr TLP is completed
+	 */
+	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+		readl(chunk->ll_region.vaddr.io);
+}
+
 static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 {
 	struct dw_edma_chan *chan = chunk->chan;
@@ -412,6 +426,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		SET_CH_32(dw, chan->dir, chan->id, llp.msb,
 			  upper_32_bits(chunk->ll_region.paddr));
 	}
+
+	dw_edma_v0_sync_ll_data(chunk);
+
 	/* Doorbell */
 	SET_RW_32(dw, chan->dir, doorbell,
 		  FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
-- 
2.25.1

Re: [PATCH v2 0/5] Fix support of dw-edma HDMA NATIVE IP in remote setup

[Vinod Koul]

On 02-10-23, 15:17, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> This patch series fix the support of dw-edma HDMA NATIVE IP.
> I can only test it in remote HDMA IP setup with single dma transfer, but
> with these fixes it works properly.
> 
> Few fixes has also been added for eDMA version. Similarly to HDMA I have
> tested only eDMA in remote setup.

Changes in v2...?

> 
> Kory Maincent (5):
>   dmaengine: dw-edma: Fix the ch_count hdma callback
>   dmaengine: dw-edma: Typos fixes
>   dmaengine: dw-edma: Add HDMA remote interrupt configuration
>   dmaengine: dw-edma: HDMA: Add sync read before starting the DMA
>     transfer in remote setup
>   dmaengine: dw-edma: eDMA: Add sync read before starting the DMA
>     transfer in remote setup
> 
>  drivers/dma/dw-edma/dw-edma-v0-core.c | 22 ++++++++++++++
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 43 +++++++++++++++++++--------
>  drivers/dma/dw-edma/dw-hdma-v0-regs.h |  2 +-
>  3 files changed, 53 insertions(+), 14 deletions(-)
> 
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH v2 0/5] Fix support of dw-edma HDMA NATIVE IP in remote setup

[Köry Maincent]

On Wed, 4 Oct 2023 12:51:54 +0530
Vinod Koul <vkoul@kernel.org> wrote:

> On 02-10-23, 15:17, Köry Maincent wrote:
> > From: Kory Maincent <kory.maincent@bootlin.com>
> > 
> > This patch series fix the support of dw-edma HDMA NATIVE IP.
> > I can only test it in remote HDMA IP setup with single dma transfer, but
> > with these fixes it works properly.
> > 
> > Few fixes has also been added for eDMA version. Similarly to HDMA I have
> > tested only eDMA in remote setup.  
> 
> Changes in v2...?

Oops forget to add it to the cover letter, sorry.

Changes in v2:
- Update comments and fix typos.
- Removed patches that tackle hypothetical bug and then were not pertinent.
- Add the similar HDMA race condition in remote setup fix to eDMA IP driver.