[v4,6/8] dmaengine: dw-edma: Don't rely on the deprecated "direction" member

Commit Message

Frank Li March 9, 2022, 9:12 p.m. UTC

From: Manivannan Sadhasivam <[e-mail]>

The "direction" member of the "dma_slave_config" structure is deprecated.
The clients no longer use this field to specify the direction of the slave
channel. But in the eDMA core, this field is used to differentiate between the
Root complex (remote) and Endpoint (local) DMA accesses.

Nevertheless, we can't differentiate between local and remote accesses without
a dedicated flag. So let's get rid of the old check and add a new check for
verifying the DMA operation between local and remote memory instead.

Signed-off-by: Manivannan Sadhasivam <[e-mail]>
Signed-off-by: Frank Li <[e-mail]>
---
no chang between v1 to v4
 drivers/dma/dw-edma/dw-edma-core.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

Comments

Serge Semin March 10, 2022, 5:29 p.m. UTC | #1

On Wed, Mar 09, 2022 at 03:12:02PM -0600, Frank Li wrote:
> From: Manivannan Sadhasivam <[e-mail]>
> 
> The "direction" member of the "dma_slave_config" structure is deprecated.
> The clients no longer use this field to specify the direction of the slave
> channel. But in the eDMA core, this field is used to differentiate between the
> Root complex (remote) and Endpoint (local) DMA accesses.
> 
> Nevertheless, we can't differentiate between local and remote accesses without
> a dedicated flag. So let's get rid of the old check and add a new check for
> verifying the DMA operation between local and remote memory instead.
> 
> Signed-off-by: Manivannan Sadhasivam <[e-mail]>
> Signed-off-by: Frank Li <[e-mail]>
> ---
> no chang between v1 to v4
>  drivers/dma/dw-edma/dw-edma-core.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 507f08db1aad3..47c6a52929fcd 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -341,22 +341,9 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
>  	if (!chan->configured)
>  		return NULL;
>  
> -	switch (chan->config.direction) {
> -	case DMA_DEV_TO_MEM: /* local DMA */
> -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ)
> -			break;
> -		return NULL;
> -	case DMA_MEM_TO_DEV: /* local DMA */
> -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE)
> -			break;

> +	/* eDMA supports only read and write between local and remote memory */

The comment is a bit confusing because both cases are named as
"memory" while the permitted directions contains DEV-part, which
means "device". What I would suggest to write here is something like:
"DW eDMA supports transferring data from/to the CPU/Application memory
to/from the PCIe link partner device by injecting the PCIe MWr/MRd TLPs."

-Sergey

> +	if (dir != DMA_DEV_TO_MEM && dir != DMA_MEM_TO_DEV)
>  		return NULL;
> -	default: /* remote DMA */
> -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ)
> -			break;
> -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE)
> -			break;
> -		return NULL;
> -	}
>  
>  	if (xfer->type == EDMA_XFER_CYCLIC) {
>  		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> -- 
> 2.24.0.rc1
>

Manivannan Sadhasivam March 10, 2022, 5:41 p.m. UTC | #2

On Thu, Mar 10, 2022 at 08:29:30PM +0300, Serge Semin wrote:
> On Wed, Mar 09, 2022 at 03:12:02PM -0600, Frank Li wrote:
> > From: Manivannan Sadhasivam <[e-mail]>
> > 
> > The "direction" member of the "dma_slave_config" structure is deprecated.
> > The clients no longer use this field to specify the direction of the slave
> > channel. But in the eDMA core, this field is used to differentiate between the
> > Root complex (remote) and Endpoint (local) DMA accesses.
> > 
> > Nevertheless, we can't differentiate between local and remote accesses without
> > a dedicated flag. So let's get rid of the old check and add a new check for
> > verifying the DMA operation between local and remote memory instead.
> > 
> > Signed-off-by: Manivannan Sadhasivam <[e-mail]>
> > Signed-off-by: Frank Li <[e-mail]>
> > ---
> > no chang between v1 to v4
> >  drivers/dma/dw-edma/dw-edma-core.c | 17 ++---------------
> >  1 file changed, 2 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 507f08db1aad3..47c6a52929fcd 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -341,22 +341,9 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> >  	if (!chan->configured)
> >  		return NULL;
> >  
> > -	switch (chan->config.direction) {
> > -	case DMA_DEV_TO_MEM: /* local DMA */
> > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ)
> > -			break;
> > -		return NULL;
> > -	case DMA_MEM_TO_DEV: /* local DMA */
> > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE)
> > -			break;
> 
> > +	/* eDMA supports only read and write between local and remote memory */
> 
> The comment is a bit confusing because both cases are named as
> "memory" while the permitted directions contains DEV-part, which
> means "device". What I would suggest to write here is something like:
> "DW eDMA supports transferring data from/to the CPU/Application memory
> to/from the PCIe link partner device by injecting the PCIe MWr/MRd TLPs."
> 

End of the day, you'd be transferring data between remote and local memory
only and the terms (local and remote) are also used in the databook. So I think
the comment is fine.

Thanks,
Mani

> -Sergey
> 
> > +	if (dir != DMA_DEV_TO_MEM && dir != DMA_MEM_TO_DEV)
> >  		return NULL;
> > -	default: /* remote DMA */
> > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ)
> > -			break;
> > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE)
> > -			break;
> > -		return NULL;
> > -	}
> >  
> >  	if (xfer->type == EDMA_XFER_CYCLIC) {
> >  		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> > -- 
> > 2.24.0.rc1
> >

Serge Semin March 10, 2022, 5:52 p.m. UTC | #3

On Thu, Mar 10, 2022 at 11:11:59PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 10, 2022 at 08:29:30PM +0300, Serge Semin wrote:
> > On Wed, Mar 09, 2022 at 03:12:02PM -0600, Frank Li wrote:
> > > From: Manivannan Sadhasivam <[e-mail]>
> > > 
> > > The "direction" member of the "dma_slave_config" structure is deprecated.
> > > The clients no longer use this field to specify the direction of the slave
> > > channel. But in the eDMA core, this field is used to differentiate between the
> > > Root complex (remote) and Endpoint (local) DMA accesses.
> > > 
> > > Nevertheless, we can't differentiate between local and remote accesses without
> > > a dedicated flag. So let's get rid of the old check and add a new check for
> > > verifying the DMA operation between local and remote memory instead.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <[e-mail]>
> > > Signed-off-by: Frank Li <[e-mail]>
> > > ---
> > > no chang between v1 to v4
> > >  drivers/dma/dw-edma/dw-edma-core.c | 17 ++---------------
> > >  1 file changed, 2 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > > index 507f08db1aad3..47c6a52929fcd 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > > @@ -341,22 +341,9 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> > >  	if (!chan->configured)
> > >  		return NULL;
> > >  
> > > -	switch (chan->config.direction) {
> > > -	case DMA_DEV_TO_MEM: /* local DMA */
> > > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ)
> > > -			break;
> > > -		return NULL;
> > > -	case DMA_MEM_TO_DEV: /* local DMA */
> > > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE)
> > > -			break;
> > 
> > > +	/* eDMA supports only read and write between local and remote memory */
> > 
> > The comment is a bit confusing because both cases are named as
> > "memory" while the permitted directions contains DEV-part, which
> > means "device". What I would suggest to write here is something like:
> > "DW eDMA supports transferring data from/to the CPU/Application memory
> > to/from the PCIe link partner device by injecting the PCIe MWr/MRd TLPs."
> > 
> 

> End of the day, you'd be transferring data between remote and local memory
> only and the terms (local and remote) are also used in the databook. So I think
> the comment is fine.

Yes, but the databook either adds a note regarding what memory it is
or it can be inferred from the text context. So at least it would be
appropriate to preserve the notes here two:
"eDMA supports only read and write between local (CPU/application) and
remote (PCIe/link partner) memory." Otherwise it's hard to understand
what memory the comment states about.

-Sergey

> 
> Thanks,
> Mani
> 
> > -Sergey
> > 
> > > +	if (dir != DMA_DEV_TO_MEM && dir != DMA_MEM_TO_DEV)
> > >  		return NULL;
> > > -	default: /* remote DMA */
> > > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ)
> > > -			break;
> > > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE)
> > > -			break;
> > > -		return NULL;
> > > -	}
> > >  
> > >  	if (xfer->type == EDMA_XFER_CYCLIC) {
> > >  		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> > > -- 
> > > 2.24.0.rc1
> > >

Manivannan Sadhasivam March 11, 2022, 5:58 p.m. UTC | #4

On Thu, Mar 10, 2022 at 08:52:25PM +0300, Serge Semin wrote:
> On Thu, Mar 10, 2022 at 11:11:59PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Mar 10, 2022 at 08:29:30PM +0300, Serge Semin wrote:
> > > On Wed, Mar 09, 2022 at 03:12:02PM -0600, Frank Li wrote:
> > > > From: Manivannan Sadhasivam <[e-mail]>
> > > > 
> > > > The "direction" member of the "dma_slave_config" structure is deprecated.
> > > > The clients no longer use this field to specify the direction of the slave
> > > > channel. But in the eDMA core, this field is used to differentiate between the
> > > > Root complex (remote) and Endpoint (local) DMA accesses.
> > > > 
> > > > Nevertheless, we can't differentiate between local and remote accesses without
> > > > a dedicated flag. So let's get rid of the old check and add a new check for
> > > > verifying the DMA operation between local and remote memory instead.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <[e-mail]>
> > > > Signed-off-by: Frank Li <[e-mail]>
> > > > ---
> > > > no chang between v1 to v4
> > > >  drivers/dma/dw-edma/dw-edma-core.c | 17 ++---------------
> > > >  1 file changed, 2 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > > > index 507f08db1aad3..47c6a52929fcd 100644
> > > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > > > @@ -341,22 +341,9 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
> > > >  	if (!chan->configured)
> > > >  		return NULL;
> > > >  
> > > > -	switch (chan->config.direction) {
> > > > -	case DMA_DEV_TO_MEM: /* local DMA */
> > > > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ)
> > > > -			break;
> > > > -		return NULL;
> > > > -	case DMA_MEM_TO_DEV: /* local DMA */
> > > > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE)
> > > > -			break;
> > > 
> > > > +	/* eDMA supports only read and write between local and remote memory */
> > > 
> > > The comment is a bit confusing because both cases are named as
> > > "memory" while the permitted directions contains DEV-part, which
> > > means "device". What I would suggest to write here is something like:
> > > "DW eDMA supports transferring data from/to the CPU/Application memory
> > > to/from the PCIe link partner device by injecting the PCIe MWr/MRd TLPs."
> > > 
> > 
> 
> > End of the day, you'd be transferring data between remote and local memory
> > only and the terms (local and remote) are also used in the databook. So I think
> > the comment is fine.
> 
> Yes, but the databook either adds a note regarding what memory it is
> or it can be inferred from the text context. So at least it would be
> appropriate to preserve the notes here two:
> "eDMA supports only read and write between local (CPU/application) and
> remote (PCIe/link partner) memory." Otherwise it's hard to understand
> what memory the comment states about.
> 

Okay, I'm fine with this.

"eDMA supports only read and write between local (CPU/application) and
remote (PCIe/link partner) memory."

Thanks,
Mani

> -Sergey
> 
> > 
> > Thanks,
> > Mani
> > 
> > > -Sergey
> > > 
> > > > +	if (dir != DMA_DEV_TO_MEM && dir != DMA_MEM_TO_DEV)
> > > >  		return NULL;
> > > > -	default: /* remote DMA */
> > > > -		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ)
> > > > -			break;
> > > > -		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE)
> > > > -			break;
> > > > -		return NULL;
> > > > -	}
> > > >  
> > > >  	if (xfer->type == EDMA_XFER_CYCLIC) {
> > > >  		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)
> > > > -- 
> > > > 2.24.0.rc1
> > > >

Patch

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 507f08db1aad3..47c6a52929fcd 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -341,22 +341,9 @@  dw_edma_device_transfer(struct dw_edma_transfer *xfer)
 	if (!chan->configured)
 		return NULL;
 
-	switch (chan->config.direction) {
-	case DMA_DEV_TO_MEM: /* local DMA */
-		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ)
-			break;
-		return NULL;
-	case DMA_MEM_TO_DEV: /* local DMA */
-		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_WRITE)
-			break;
+	/* eDMA supports only read and write between local and remote memory */
+	if (dir != DMA_DEV_TO_MEM && dir != DMA_MEM_TO_DEV)
 		return NULL;
-	default: /* remote DMA */
-		if (dir == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ)
-			break;
-		if (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE)
-			break;
-		return NULL;
-	}
 
 	if (xfer->type == EDMA_XFER_CYCLIC) {
 		if (!xfer->xfer.cyclic.len || !xfer->xfer.cyclic.cnt)