diff mbox series

[01/25] dmaengine: dw-edma: Drop dma_slave_config.direction field usage

Message ID 20220324014836.19149-2-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Headers show
Series dmaengine: dw-edma: Add RP/EP local DMA controllers support | expand

Commit Message

Serge Semin March 24, 2022, 1:48 a.m. UTC
The dma_slave_config.direction field usage in the DW eDMA driver has been
introduced in the commit bd96f1b2f43a ("dmaengine: dw-edma: support local
dma device transfer semantics"). Mainly the change introduced there was
correct (indeed DEV_TO_MEM means using RD-channel and MEM_TO_DEV -
WR-channel for the case of having eDMA accessed locally from
CPU/Application side), but providing an additional
MEM_TO_MEM/DEV_TO_DEV-based semantics was quite redundant if not to say
potentially harmful (when it comes to removing the denoted field). First
of all since the dma_slave_config.direction field has been marked as
obsolete (see [1] and the structure dc [2]) and will be discarded in
future, using it especially in a non-standard way is discouraged. Secondly
in accordance with the commit denoted above the default
dw_edma_device_transfer() semantics has been changed despite what it's
message said. So claiming that the method was left backward compatible was
wrong.

Anyway let's fix the problems denoted above and simplify the
dw_edma_device_transfer() method by dropping the parsing of the
DMA-channel direction field. Instead of having that implicit
dma_slave_config.direction field semantic we can use the recently added
DW_EDMA_CHIP_LOCAL flag to distinguish between the local and remote DW
eDMA setups thus preserving both cases support. In addition to that an
ASCII-figure has been added to clarify the complication out.

[1] Documentation/driver-api/dmaengine/provider.rst
[2] include/linux/dmaengine.h: dma_slave_config.direction

Co-developed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

In accordance with agreement with Frank and Manivannan this patch is
supposed to be moved to the series:
Link: https://lore.kernel.org/dmaengine/20220310192457.3090-1-Frank.Li@nxp.com/
in place of the patch:
[PATCH v5 6/9] dmaengine: dw-edma: Don't rely on the deprecated "direction" member
Link: https://lore.kernel.org/dmaengine/20220310192457.3090-7-Frank.Li@nxp.com/
---
 drivers/dma/dw-edma/dw-edma-core.c | 49 +++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 15 deletions(-)

Comments

Serge Semin April 5, 2022, 11:15 a.m. UTC | #1
On Thu, Mar 24, 2022 at 07:00:04PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 24, 2022 at 04:48:12AM +0300, Serge Semin wrote:
> > The dma_slave_config.direction field usage in the DW eDMA driver has been
> > introduced in the commit bd96f1b2f43a ("dmaengine: dw-edma: support local
> > dma device transfer semantics"). Mainly the change introduced there was
> > correct (indeed DEV_TO_MEM means using RD-channel and MEM_TO_DEV -
> > WR-channel for the case of having eDMA accessed locally from
> > CPU/Application side), but providing an additional
> > MEM_TO_MEM/DEV_TO_DEV-based semantics was quite redundant if not to say
> > potentially harmful (when it comes to removing the denoted field). First
> > of all since the dma_slave_config.direction field has been marked as
> > obsolete (see [1] and the structure dc [2]) and will be discarded in
> > future, using it especially in a non-standard way is discouraged. Secondly
> > in accordance with the commit denoted above the default
> > dw_edma_device_transfer() semantics has been changed despite what it's
> > message said. So claiming that the method was left backward compatible was
> > wrong.
> > 
> > Anyway let's fix the problems denoted above and simplify the
> > dw_edma_device_transfer() method by dropping the parsing of the
> > DMA-channel direction field. Instead of having that implicit
> > dma_slave_config.direction field semantic we can use the recently added
> > DW_EDMA_CHIP_LOCAL flag to distinguish between the local and remote DW
> > eDMA setups thus preserving both cases support. In addition to that an
> > ASCII-figure has been added to clarify the complication out.
> > 
> > [1] Documentation/driver-api/dmaengine/provider.rst
> > [2] include/linux/dmaengine.h: dma_slave_config.direction
> > 
> > Co-developed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > In accordance with agreement with Frank and Manivannan this patch is
> > supposed to be moved to the series:
> > Link: https://lore.kernel.org/dmaengine/20220310192457.3090-1-Frank.Li@nxp.com/
> > in place of the patch:
> > [PATCH v5 6/9] dmaengine: dw-edma: Don't rely on the deprecated "direction" member
> > Link: https://lore.kernel.org/dmaengine/20220310192457.3090-7-Frank.Li@nxp.com/
> > ---
> >  drivers/dma/dw-edma/dw-edma-core.c | 49 +++++++++++++++++++++---------
> >  1 file changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 5be8a5944714..e9e32ed74aa9 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -339,21 +339,40 @@ 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;
> > -		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;
> > +	/*
> > +	 * Local Root Port/End-point              Remote End-point
> > +	 * +-----------------------+ PCIe bus +----------------------+
> > +	 * |                       |    +-+   |                      |
> > +	 * |    DEV_TO_MEM   Rx Ch <----+ +---+ Tx Ch  DEV_TO_MEM    |
> > +	 * |                       |    | |   |                      |
> > +	 * |    MEM_TO_DEV   Tx Ch +----+ +---> Rx Ch  MEM_TO_DEV    |
> > +	 * |                       |    +-+   |                      |
> > +	 * +-----------------------+          +----------------------+
> > +	 *
> > +	 * 1. Normal logic:
> > +	 * If eDMA is embedded into the DW PCIe RP/EP and controlled from the
> > +	 * CPU/Application side, the Rx channel (EDMA_DIR_READ) will be used
> > +	 * for the device read operations (DEV_TO_MEM) and the Tx channel
> > +	 * (EDMA_DIR_WRITE) - for the write operations (MEM_TO_DEV).
> > +	 *
> > +	 * 2. Inverted logic:
> > +	 * If eDMA is embedded into a Remote PCIe EP and is controlled by the
> > +	 * MWr/MRd TLPs sent from the CPU's PCIe host controller, the Tx
> > +	 * channel (EDMA_DIR_WRITE) will be used for the device read operations
> > +	 * (DEV_TO_MEM) and the Rx channel (EDMA_DIR_READ) - for the write
> > +	 * operations (MEM_TO_DEV).
> > +	 *
> > +	 * It is the client driver responsibility to choose a proper channel
> > +	 * for the DMA transfers.
> > +	 */
> 

> I think it'd be good to document this using some form in "enum dw_edma_dir"
> declaration.

Well, I'd rather leave the comment here.  The dw_edma_dir enumeration
only defines the actual device channel direction, while the actual
semantics is determined by the PCIe device residence/configuration
(local + host/end-point, remote + end-point) and is reflected in the
data transfer function - dw_edma_device_transfer(). So as I see it the
comment is more suitable to be left here in the place where the
semantic is actually implemented.

-Sergey

> 
> Thanks,
> Mani
> 
> > +	if (chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
> > +		if ((chan->dir == EDMA_DIR_READ && dir != DMA_DEV_TO_MEM) ||
> > +		    (chan->dir == EDMA_DIR_WRITE && dir != DMA_MEM_TO_DEV))
> > +			return NULL;
> > +	} else {
> > +		if ((chan->dir == EDMA_DIR_WRITE && dir != DMA_DEV_TO_MEM) ||
> > +		    (chan->dir == EDMA_DIR_READ && dir != DMA_MEM_TO_DEV))
> > +			return NULL;
> >  	}
> >  
> >  	if (xfer->type == EDMA_XFER_CYCLIC) {
> > -- 
> > 2.35.1
> >
diff mbox series

Patch

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 5be8a5944714..e9e32ed74aa9 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -339,21 +339,40 @@  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;
-		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;
+	/*
+	 * Local Root Port/End-point              Remote End-point
+	 * +-----------------------+ PCIe bus +----------------------+
+	 * |                       |    +-+   |                      |
+	 * |    DEV_TO_MEM   Rx Ch <----+ +---+ Tx Ch  DEV_TO_MEM    |
+	 * |                       |    | |   |                      |
+	 * |    MEM_TO_DEV   Tx Ch +----+ +---> Rx Ch  MEM_TO_DEV    |
+	 * |                       |    +-+   |                      |
+	 * +-----------------------+          +----------------------+
+	 *
+	 * 1. Normal logic:
+	 * If eDMA is embedded into the DW PCIe RP/EP and controlled from the
+	 * CPU/Application side, the Rx channel (EDMA_DIR_READ) will be used
+	 * for the device read operations (DEV_TO_MEM) and the Tx channel
+	 * (EDMA_DIR_WRITE) - for the write operations (MEM_TO_DEV).
+	 *
+	 * 2. Inverted logic:
+	 * If eDMA is embedded into a Remote PCIe EP and is controlled by the
+	 * MWr/MRd TLPs sent from the CPU's PCIe host controller, the Tx
+	 * channel (EDMA_DIR_WRITE) will be used for the device read operations
+	 * (DEV_TO_MEM) and the Rx channel (EDMA_DIR_READ) - for the write
+	 * operations (MEM_TO_DEV).
+	 *
+	 * It is the client driver responsibility to choose a proper channel
+	 * for the DMA transfers.
+	 */
+	if (chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
+		if ((chan->dir == EDMA_DIR_READ && dir != DMA_DEV_TO_MEM) ||
+		    (chan->dir == EDMA_DIR_WRITE && dir != DMA_MEM_TO_DEV))
+			return NULL;
+	} else {
+		if ((chan->dir == EDMA_DIR_WRITE && dir != DMA_DEV_TO_MEM) ||
+		    (chan->dir == EDMA_DIR_READ && dir != DMA_MEM_TO_DEV))
+			return NULL;
 	}
 
 	if (xfer->type == EDMA_XFER_CYCLIC) {