Skip to contentSkip to conversation

dw_dmac: store direction in the custom channel structure

PATCH Superseded Andy Shevchenko 2 participants 5 messages Nov 20, 2012 dmaengine

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 dw_dmac: store direction in the custom channel structure

Currently the direction value is come from the generic slave configuration structure and explicitly as a preparation function parameter. The first one is kinda obsoleted. Thus, we have to store the value passed to the preparation function somewhere in our structures to be able to use it later. The best candidate to provide the storage is a custom channel structure. Until now we still keep and check the direction field of the slave config structure as well.

---
 drivers/dma/dw_dmac.c      |   10 ++++++++--
 drivers/dma/dw_dmac_regs.h |    2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)
drivers/dma/dw_dmac.c +8 -2
index 5e2c4dc..58c6d3c 100644
@@ -178,9 +178,9 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 		cfghi = dws->cfg_hi;
 		cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
 	} else {
-		if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
+		if (dwc->direction == DMA_MEM_TO_DEV)
 			cfghi = DWC_CFGH_DST_PER(dwc->dma_sconfig.slave_id);
-		else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
+		else if (dwc->direction == DMA_DEV_TO_MEM)
 			cfghi = DWC_CFGH_SRC_PER(dwc->dma_sconfig.slave_id);
 	}
 
@@ -807,6 +807,8 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	if (unlikely(!dws || !sg_len))
 		return NULL;
 
+	dwc->direction = direction;
+
 	prev = first = NULL;
 
 	switch (direction) {
@@ -983,6 +985,7 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 		return -EINVAL;
 
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
+	dwc->direction = sconfig->direction;
 
 	convert_burst(&dwc->dma_sconfig.src_maxburst);
 	convert_burst(&dwc->dma_sconfig.dst_maxburst);
@@ -1339,6 +1342,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	if (unlikely(!is_slave_xfer(direction)))
 		goto out_err;
 
+	dwc->direction = direction;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else
@@ -1713,6 +1718,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
 		channel_clear_bit(dw, CH_EN, dwc->mask);
 
 		dwc->dw = dw;
+		dwc->direction = DMA_TRANS_NONE;
 
 		/* hardware configuration */
 		if (autocfg) {
drivers/dma/dw_dmac_regs.h +2 -0
index 8881e9b..362aba1 100644
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/dmaengine.h>
 #include <linux/dw_dmac.h>
 
 #define DW_DMA_MAX_NR_CHANNELS	8
@@ -188,6 +189,7 @@ struct dw_dma_chan {
 	void __iomem		*ch_regs;
 	u8			mask;
 	u8			priority;
+	enum dma_transfer_direction direction;
 	bool			paused;
 	bool			initialized;
 
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 2/2 dw_dmac: make usage of dw_dma_slave optional

The driver requires a custom slave configuration to be present to be able to make the slave transfers. Nevertheless, in some cases we need only the request line as an additional information to the generic slave configuration. The request line is provided by slave_id parameter of the dma_slave_config structure. That's why the custom slave configuration could be optional for such cases.

---
 drivers/dma/dw_dmac.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/dma/dw_dmac.c +7 -6
index 58c6d3c..8d50374 100644
@@ -50,11 +50,12 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 		struct dw_dma_slave *__slave = (_chan->private);	\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
+		bool _is_slave = is_slave_xfer(_dwc->direction);	\
 		int _dms = dwc_get_dms(__slave);		\
 		int _sms = dwc_get_sms(__slave);		\
-		u8 _smsize = __slave ? _sconfig->src_maxburst :	\
+		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
-		u8 _dmsize = __slave ? _sconfig->dst_maxburst :	\
+		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 								\
 		(DWC_CTLL_DST_MSIZE(_dmsize)			\
@@ -325,7 +326,7 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 	list_splice_init(&desc->tx_list, &dwc->free_list);
 	list_move(&desc->desc_node, &dwc->free_list);
 
-	if (!dwc->chan.private) {
+	if (!is_slave_xfer(dwc->direction)) {
 		struct device *parent = chan2parent(&dwc->chan);
 		if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
 			if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
@@ -804,7 +805,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 
 	dev_vdbg(chan2dev(chan), "%s\n", __func__);
 
-	if (unlikely(!dws || !sg_len))
+	if (unlikely(!is_slave_xfer(direction) || !sg_len))
 		return NULL;
 
 	dwc->direction = direction;
@@ -980,8 +981,8 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 
-	/* Check if it is chan is configured for slave transfers */
-	if (!chan->private)
+	/* Check if chan will be configured for slave transfers */
+	if (!is_slave_xfer(sconfig->direction))
 		return -EINVAL;
 
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
Signature · 1 line
1.7.10.4
Acked-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

Quoted 8 lines · Andy Shevchenko

The driver requires a custom slave configuration to be present to be able to make the slave transfers. Nevertheless, in some cases we need only the request line as an additional information to the generic slave configuration. The request line is provided by slave_id parameter of the dma_slave_config structure. That's why the custom slave configuration could be optional for such cases.

Acked-by on v1 1/2
#3 lore raw
In reply to Andy Shevchenko
Quoted 1 line · Andy Shevchenko

Currently the direction value is come from the generic slave configuration

s/is come/comes

Quoted 17 lines

structure and explicitly as a preparation function parameter. The first one is kinda obsoleted. Thus, we have to store the value passed to the preparation function somewhere in our structures to be able to use it later. The best candidate to provide the storage is a custom channel structure. Until now we still keep and check the direction field of the slave config structure as well.

---
 drivers/dma/dw_dmac.c      |   10 ++++++++--
 drivers/dma/dw_dmac_regs.h |    2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)
drivers/dma/dw_dmac.c +0 -0
index 5e2c4dc..58c6d3c 100644

drivers/dma/dw_dmac.c static void dwc_initialize(struct dw_dma_chan *dwc) · line 185

Quoted 10 lines
drivers/dma/dw_dmac.c +2 -2
@@ -178,9 +178,9 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
                cfghi = dws->cfg_hi;
                cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
        } else {
-               if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
+               if (dwc->direction == DMA_MEM_TO_DEV)
                        cfghi = DWC_CFGH_DST_PER(dwc->dma_sconfig.slave_id);
-               else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
+               else if (dwc->direction == DMA_DEV_TO_MEM)
                        cfghi = DWC_CFGH_SRC_PER(dwc->dma_sconfig.slave_id);
        }

I have a doubt here. This routine is called both for memcpy and slave transfers. Earlier if (dws), code was used to check slave/non-slave transfer but with your earlier patch, for if (!dws) case too, there can be slave transfers based on direction.

Consider a situation. A channel is used for slave transfers and have direction set
to DMA_DEV_TO_MEM. Now, channel is freed and used for memcpy. Now the direction value is retained as we haven't updated it. So, is this a potential bug?

Quoted 47 lines
drivers/dma/dw_dmac.c +6 -0
@@ -807,6 +807,8 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
        if (unlikely(!dws || !sg_len))
                return NULL;
 
+       dwc->direction = direction;
+
        prev = first = NULL;
 
        switch (direction) {
@@ -983,6 +985,7 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
                return -EINVAL;
 
        memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
+       dwc->direction = sconfig->direction;
 
        convert_burst(&dwc->dma_sconfig.src_maxburst);
        convert_burst(&dwc->dma_sconfig.dst_maxburst);
@@ -1339,6 +1342,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
        if (unlikely(!is_slave_xfer(direction)))
                goto out_err;
 
+       dwc->direction = direction;
+
        if (direction == DMA_MEM_TO_DEV)
                reg_width = __ffs(sconfig->dst_addr_width);
        else
@@ -1713,6 +1718,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
                channel_clear_bit(dw, CH_EN, dwc->mask);
 
                dwc->dw = dw;
+               dwc->direction = DMA_TRANS_NONE;
 
                /* hardware configuration */
                if (autocfg) {
drivers/dma/dw_dmac_regs.h +1 -0
index 8881e9b..362aba1 100644
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/dmaengine.h>
 #include <linux/dw_dmac.h>
 
 #define DW_DMA_MAX_NR_CHANNELS 8

drivers/dma/dw_dmac_regs.h struct dw_dma_chan { · line 194

Quoted 6 lines
drivers/dma/dw_dmac_regs.h +1 -0
@@ -188,6 +189,7 @@ struct dw_dma_chan {
        void __iomem            *ch_regs;
        u8                      mask;
        u8                      priority;
+       enum dma_transfer_direction direction;
        bool                    paused;
        bool                    initialized;

better align it with tabs and update earlier ones too with extra tabs.

#4 lore raw
Quoted 39 lines · Viresh Kumar
Quoted 1 line · Andy Shevchenko

Currently the direction value is come from the generic slave configuration

s/is come/comes

Quoted 17 lines

structure and explicitly as a preparation function parameter. The first one is kinda obsoleted. Thus, we have to store the value passed to the preparation function somewhere in our structures to be able to use it later. The best candidate to provide the storage is a custom channel structure. Until now we still keep and check the direction field of the slave config structure as well.

---
 drivers/dma/dw_dmac.c      |   10 ++++++++--
 drivers/dma/dw_dmac_regs.h |    2 ++
 2 files changed, 10 insertions(+), 2 deletions(-)
drivers/dma/dw_dmac.c +0 -0
index 5e2c4dc..58c6d3c 100644

drivers/dma/dw_dmac.c static void dwc_initialize(struct dw_dma_chan *dwc) · line 185

Quoted 10 lines
drivers/dma/dw_dmac.c +2 -2
@@ -178,9 +178,9 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
                cfghi = dws->cfg_hi;
                cfglo |= dws->cfg_lo & ~DWC_CFGL_CH_PRIOR_MASK;
        } else {
-               if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
+               if (dwc->direction == DMA_MEM_TO_DEV)
                        cfghi = DWC_CFGH_DST_PER(dwc->dma_sconfig.slave_id);
-               else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
+               else if (dwc->direction == DMA_DEV_TO_MEM)
                        cfghi = DWC_CFGH_SRC_PER(dwc->dma_sconfig.slave_id);
        }

I have a doubt here. This routine is called both for memcpy and slave transfers. Earlier if (dws), code was used to check slave/non-slave transfer but with your earlier patch, for if (!dws) case too, there can be slave transfers based on direction.

Quoted 5 lines

Consider a situation. A channel is used for slave transfers and have direction set
to DMA_DEV_TO_MEM. Now, channel is freed and used for memcpy. Now the direction value is retained as we haven't updated it. So, is this a potential bug?

Yeah, good point. I think we have to set direction in all possible entry points (prep functions only?). It means at least we need to update memcpy prep as well.

I'll update patch tomorrow.

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy