* dmaengine: pl330: fix irq race with terminate_all
From: John Keeping @ 2018-07-17 10:48 UTC (permalink / raw)
To: dmaengine; +Cc: Vinod Koul, Dan Williams, linux-kernel, John Keeping
In pl330_update() when checking if a channel has been aborted, the
channel's lock is not taken, only the overall pl330_dmac lock. But in
pl330_terminate_all() the aborted flag (req_running==-1) is set under
the channel lock and not the pl330_dmac lock.
With threaded interrupts, this leads to a potential race:
pl330_terminate_all pl330_update
------------------- ------------
lock channel
entry
lock pl330
_stop channel
unlock pl330
lock pl330
check req_running != -1
req_running = -1
_start channel
Signed-off-by: John Keeping <john@metanate.com>
---
drivers/dma/pl330.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 370df2e74ddd..88750a34e859 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2256,13 +2256,14 @@ static int pl330_terminate_all(struct dma_chan *chan)
pm_runtime_get_sync(pl330->ddma.dev);
spin_lock_irqsave(&pch->lock, flags);
+
spin_lock(&pl330->lock);
_stop(pch->thread);
- spin_unlock(&pl330->lock);
-
pch->thread->req[0].desc = NULL;
pch->thread->req[1].desc = NULL;
pch->thread->req_running = -1;
+ spin_unlock(&pl330->lock);
+
power_down = pch->active;
pch->active = false;
^ permalink raw reply related
* [5/5] dmaengine: mv_xor_v2: enable COMPILE_TEST
From: hannah @ 2018-07-17 10:30 UTC (permalink / raw)
To: dan.j.williams, vkoul, dmaengine
Cc: thomas.petazzoni, linux-kernel, nadavh, omrii, oferh,
gregory.clement, Hanna Hawa
From: Hanna Hawa <hannah@marvell.com>
To get more coverage, enable COMPILE_TEST for this driver.
Signed-off-by: Hanna Hawa <hannah@marvell.com>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/dma/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index ca1680a..1f76129 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -366,7 +366,7 @@ config MV_XOR
config MV_XOR_V2
bool "Marvell XOR engine version 2 support "
- depends on ARM64
+ depends on ARM64 || COMPILE_TEST
select DMA_ENGINE
select DMA_ENGINE_RAID
select ASYNC_TX_ENABLE_CHANNEL_SWITCH
^ permalink raw reply related
* [4/5] dmaengine: mv_xor_v2: move unmap to before callback
From: hannah @ 2018-07-17 10:30 UTC (permalink / raw)
To: dan.j.williams, vkoul, dmaengine
Cc: thomas.petazzoni, linux-kernel, nadavh, omrii, oferh,
gregory.clement, Hanna Hawa
From: Hanna Hawa <hannah@marvell.com>
Completion callback should happen after dma_descriptor_unmap() has
happened. This allow the cache invalidate to happen and ensure that
the data accessed by the upper layer is in memory that was from DMA
rather than stale data. On some architecture this is done by the
hardware, however we should make the code consistent to not cause
confusion.
Signed-off-by: Hanna Hawa <hannah@marvell.com>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/dma/mv_xor_v2.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
index 14e2a7a..d41d916 100644
--- a/drivers/dma/mv_xor_v2.c
+++ b/drivers/dma/mv_xor_v2.c
@@ -589,10 +589,9 @@ static void mv_xor_v2_tasklet(unsigned long data)
*/
dma_cookie_complete(&next_pending_sw_desc->async_tx);
+ dma_descriptor_unmap(&next_pending_sw_desc->async_tx);
dmaengine_desc_get_callback_invoke(
&next_pending_sw_desc->async_tx, NULL);
-
- dma_descriptor_unmap(&next_pending_sw_desc->async_tx);
}
dma_run_dependencies(&next_pending_sw_desc->async_tx);
^ permalink raw reply related
* [3/5] dmaengine: mv_xor_v2: convert callback to helper function
From: hannah @ 2018-07-17 10:30 UTC (permalink / raw)
To: dan.j.williams, vkoul, dmaengine
Cc: thomas.petazzoni, linux-kernel, nadavh, omrii, oferh,
gregory.clement, Hanna Hawa
From: Hanna Hawa <hannah@marvell.com>
This is in preperation of moving to a callback that provides results to the
callback for the transaction. The conversion will maintain current behavior
and the driver must convert to new callback mechanism at a later time in
order to receive results.
Signed-off-by: Hanna Hawa <hannah@marvell.com>
---
drivers/dma/mv_xor_v2.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
index e718498..14e2a7a 100644
--- a/drivers/dma/mv_xor_v2.c
+++ b/drivers/dma/mv_xor_v2.c
@@ -589,9 +589,8 @@ static void mv_xor_v2_tasklet(unsigned long data)
*/
dma_cookie_complete(&next_pending_sw_desc->async_tx);
- if (next_pending_sw_desc->async_tx.callback)
- next_pending_sw_desc->async_tx.callback(
- next_pending_sw_desc->async_tx.callback_param);
+ dmaengine_desc_get_callback_invoke(
+ &next_pending_sw_desc->async_tx, NULL);
dma_descriptor_unmap(&next_pending_sw_desc->async_tx);
}
^ permalink raw reply related
* [2/5] dmaengine: mv_xor_v2: kill the tasklets upon exit
From: hannah @ 2018-07-17 10:30 UTC (permalink / raw)
To: dan.j.williams, vkoul, dmaengine
Cc: thomas.petazzoni, linux-kernel, nadavh, omrii, oferh,
gregory.clement, Hanna Hawa
From: Hanna Hawa <hannah@marvell.com>
The mv_xor_v2 driver uses a tasklet, initialized during the probe()
routine. However, it forgets to cleanup the tasklet using
tasklet_kill() function during the remove() routine, which this patch
fixes. This prevents the tasklet from potentially running after the
module has been removed.
Fixes: 19a340b1a820 ("dmaengine: mv_xor_v2: new driver")
Signed-off-by: Hanna Hawa <hannah@marvell.com>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
drivers/dma/mv_xor_v2.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
index e16083a..e718498 100644
--- a/drivers/dma/mv_xor_v2.c
+++ b/drivers/dma/mv_xor_v2.c
@@ -903,6 +903,8 @@ static int mv_xor_v2_remove(struct platform_device *pdev)
platform_msi_domain_free_irqs(&pdev->dev);
+ tasklet_kill(&xor_dev->irq_tasklet);
+
clk_disable_unprepare(xor_dev->clk);
return 0;
^ permalink raw reply related
* [1/5] dmaengine: mv_xor_v2: explicitly freeup irq
From: hannah @ 2018-07-17 10:29 UTC (permalink / raw)
To: dan.j.williams, vkoul, dmaengine
Cc: thomas.petazzoni, linux-kernel, nadavh, omrii, oferh,
gregory.clement, Hanna Hawa
From: Hanna Hawa <hannah@marvell.com>
dmaengine device should explicitly call devm_free_irq() when using
devm_request_irq().
The irq is still ON when devices remove is executed and irq should be
quiesced before remove is completed.
Signed-off-by: Hanna Hawa <hannah@marvell.com>
---
drivers/dma/mv_xor_v2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
index c6589cc..e16083a 100644
--- a/drivers/dma/mv_xor_v2.c
+++ b/drivers/dma/mv_xor_v2.c
@@ -174,6 +174,7 @@ struct mv_xor_v2_device {
int desc_size;
unsigned int npendings;
unsigned int hw_queue_idx;
+ struct msi_desc *msi_desc;
};
/**
@@ -780,6 +781,7 @@ static int mv_xor_v2_probe(struct platform_device *pdev)
msi_desc = first_msi_entry(&pdev->dev);
if (!msi_desc)
goto free_msi_irqs;
+ xor_dev->msi_desc = msi_desc;
ret = devm_request_irq(&pdev->dev, msi_desc->irq,
mv_xor_v2_interrupt_handler, 0,
@@ -897,6 +899,8 @@ static int mv_xor_v2_remove(struct platform_device *pdev)
xor_dev->desc_size * MV_XOR_V2_DESC_NUM,
xor_dev->hw_desq_virt, xor_dev->hw_desq);
+ devm_free_irq(&pdev->dev, xor_dev->msi_desc->irq, xor_dev);
+
platform_msi_domain_free_irqs(&pdev->dev);
clk_disable_unprepare(xor_dev->clk);
^ permalink raw reply related
* [v2,3/3] ARM: configs: imx_v6_v7_defconfig: add DMATEST support
From: Shawn Guo @ 2018-07-17 6:28 UTC (permalink / raw)
To: Robin Gong
Cc: vkoul, dan.j.williams, s.hauer, fabio.estevam, linux, dmaengine,
linux-imx, kernel, linux-arm-kernel, linux-kernel
On Fri, Jul 13, 2018 at 09:08:47PM +0800, Robin Gong wrote:
> Add DMATEST support and remove invalid options, such as
> CONFIG_BT_HCIUART_H4 is default enabled and CONFIG_SND_SOC_IMX_WM8962
> is out of date and not appear in any config file. Please refer to
> Documentation/driver-api/dmaengine/dmatest.rst to test MEMCPY feature
> of imx-sdma.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Applied this one, thanks.
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC
From: Rob Herring @ 2018-07-16 21:33 UTC (permalink / raw)
To: Vinod
Cc: Paul Cercueil, Mark Rutland, Ralf Baechle, Paul Burton,
James Hogan, Zubair Lutfullah Kakakhel, Mathieu Malaterre,
Daniel Silsby, dmaengine, devicetree, linux-kernel, linux-mips
On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
> On 03-07-18, 14:32, Paul Cercueil wrote:
>
> > enum jz_version {
> > + ID_JZ4740,
> > ID_JZ4770,
> > ID_JZ4780,
> > };
> > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> > }
> >
> > static const unsigned int jz4780_dma_ord_max[] = {
> > + [ID_JZ4740] = 5,
> > [ID_JZ4770] = 6,
> > [ID_JZ4780] = 7,
> > };
> > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> > }
> >
> > static const unsigned int jz4780_dma_nb_channels[] = {
> > + [ID_JZ4740] = 6,
> > [ID_JZ4770] = 6,
> > [ID_JZ4780] = 32,
> > };
>
> I feel these should be done away with if we describe hardware in DT
The compatible property can imply things like this.
But how this is structured is a bit strange. Normally you have a per
compatible struct with these as elements and the compatible matching
selects the struct.
>
> >
> > static const struct of_device_id jz4780_dma_dt_match[] = {
> > + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
>
> adding .compatible should be the only thing required, if at all for this
> addition :)
>
> --
> ~Vinod
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
From: Rob Herring @ 2018-07-16 21:28 UTC (permalink / raw)
To: Paul Burton
Cc: Vinod, Mark Rutland, Paul Cercueil, Ralf Baechle, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On Wed, Jul 11, 2018 at 04:27:15PM -0700, Paul Burton wrote:
> Hi Vinod,
>
> On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote:
> > > > > - jzdma->base = devm_ioremap_resource(dev, res);
> > > > > - if (IS_ERR(jzdma->base))
> > > > > - return PTR_ERR(jzdma->base);
> > > > > + jzdma->chn_base = devm_ioremap_resource(dev, res);
> > > > > + if (IS_ERR(jzdma->chn_base))
> > > > > + return PTR_ERR(jzdma->chn_base);
> > > > > +
> > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > > + if (!res) {
> > > > > + dev_err(dev, "failed to get I/O memory\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > >
> > > > okay and this breaks if you happen to get probed on older DT. I think DT
> > > > is treated as ABI so you need to continue support older method while
> > > > finding if DT has split resources
> > >
> > > See my response to PrasannaKumar. All the Ingenic-based boards do compile
> > > the devicetree within the kernel, so I think it's still fine to add breaking
> > > changes. I'll wait on @Rob to give his point of view on this, though.
> > >
> > > (It's not something hard to change, but I'd like to know what's the policy
> > > in that case. I have other DT-breaking patches to submit)
> >
> > The policy is that DT is an ABI and should not break :)
>
> I think in general that's a good policy to have for compatibility, but
> if it's known for certain that the DT for all users of a driver is
> always built into the kernel then I don't see why we shouldn't feel free
> to change a binding. I agree with Paul that it'd be interesting to hear
> the DT binding maintainers take on this.
If the platform maintainers (and their users) don't care, then I don't
have an issue with the change. It should still be an exception and not
just any change goes. The commit message should still highlight that
compatibility is being broken and why.
Rob
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [v2,3/3] ARM: configs: imx_v6_v7_defconfig: add DMATEST support
From: Robin Gong @ 2018-07-13 13:08 UTC (permalink / raw)
To: vkoul, dan.j.williams, shawnguo, s.hauer, fabio.estevam, linux
Cc: linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
Add DMATEST support and remove invalid options, such as
CONFIG_BT_HCIUART_H4 is default enabled and CONFIG_SND_SOC_IMX_WM8962
is out of date and not appear in any config file. Please refer to
Documentation/driver-api/dmaengine/dmatest.rst to test MEMCPY feature
of imx-sdma.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
arch/arm/configs/imx_v6_v7_defconfig | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index e381d05..f28d4d9 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -81,7 +81,6 @@ CONFIG_CAN=y
CONFIG_CAN_FLEXCAN=y
CONFIG_BT=y
CONFIG_BT_HCIUART=y
-CONFIG_BT_HCIUART_H4=y
CONFIG_BT_HCIUART_LL=y
CONFIG_CFG80211=y
CONFIG_CFG80211_WEXT=y
@@ -282,7 +281,6 @@ CONFIG_SND_SOC_FSL_ASRC=y
CONFIG_SND_IMX_SOC=y
CONFIG_SND_SOC_PHYCORE_AC97=y
CONFIG_SND_SOC_EUKREA_TLV320=y
-CONFIG_SND_SOC_IMX_WM8962=y
CONFIG_SND_SOC_IMX_ES8328=y
CONFIG_SND_SOC_IMX_SGTL5000=y
CONFIG_SND_SOC_IMX_SPDIF=y
@@ -371,6 +369,7 @@ CONFIG_DMADEVICES=y
CONFIG_FSL_EDMA=y
CONFIG_IMX_SDMA=y
CONFIG_MXS_DMA=y
+CONFIG_DMATEST=m
CONFIG_STAGING=y
CONFIG_STAGING_MEDIA=y
CONFIG_VIDEO_IMX_MEDIA=y
^ permalink raw reply related
* [v2,2/3] dmaengine: imx-sdma: add memcpy interface
From: Robin Gong @ 2018-07-13 13:08 UTC (permalink / raw)
To: vkoul, dan.j.williams, shawnguo, s.hauer, fabio.estevam, linux
Cc: linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
Add MEMCPY capability for imx-sdma driver.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/imx-sdma.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 92 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index e3d5e73..ef50f2c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -342,6 +342,7 @@ struct sdma_desc {
* @pc_from_device: script address for those device_2_memory
* @pc_to_device: script address for those memory_2_device
* @device_to_device: script address for those device_2_device
+ * @pc_to_pc: script address for those memory_2_memory
* @flags: loop mode or not
* @per_address: peripheral source or destination address in common case
* destination address in p_2_p case
@@ -367,6 +368,7 @@ struct sdma_channel {
enum dma_slave_buswidth word_size;
unsigned int pc_from_device, pc_to_device;
unsigned int device_to_device;
+ unsigned int pc_to_pc;
unsigned long flags;
dma_addr_t per_address, per_address2;
unsigned long event_mask[2];
@@ -869,14 +871,16 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
* These are needed once we start to support transfers between
* two peripherals or memory-to-memory transfers
*/
- int per_2_per = 0;
+ int per_2_per = 0, emi_2_emi = 0;
sdmac->pc_from_device = 0;
sdmac->pc_to_device = 0;
sdmac->device_to_device = 0;
+ sdmac->pc_to_pc = 0;
switch (peripheral_type) {
case IMX_DMATYPE_MEMORY:
+ emi_2_emi = sdma->script_addrs->ap_2_ap_addr;
break;
case IMX_DMATYPE_DSP:
emi_2_per = sdma->script_addrs->bp_2_ap_addr;
@@ -949,6 +953,7 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
sdmac->pc_from_device = per_2_emi;
sdmac->pc_to_device = emi_2_per;
sdmac->device_to_device = per_2_per;
+ sdmac->pc_to_pc = emi_2_emi;
}
static int sdma_load_context(struct sdma_channel *sdmac)
@@ -965,6 +970,8 @@ static int sdma_load_context(struct sdma_channel *sdmac)
load_address = sdmac->pc_from_device;
else if (sdmac->direction == DMA_DEV_TO_DEV)
load_address = sdmac->device_to_device;
+ else if (sdmac->direction == DMA_MEM_TO_MEM)
+ load_address = sdmac->pc_to_pc;
else
load_address = sdmac->pc_to_device;
@@ -1214,10 +1221,28 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
{
struct sdma_channel *sdmac = to_sdma_chan(chan);
struct imx_dma_data *data = chan->private;
+ struct imx_dma_data mem_data;
int prio, ret;
- if (!data)
- return -EINVAL;
+ /*
+ * MEMCPY may never setup chan->private by filter function such as
+ * dmatest, thus create 'struct imx_dma_data mem_data' for this case.
+ * Please note in any other slave case, you have to setup chan->private
+ * with 'struct imx_dma_data' in your own filter function if you want to
+ * request dma channel by dma_request_channel() rather than
+ * dma_request_slave_channel(). Othwise, 'MEMCPY in case?' will appear
+ * to warn you to correct your filter function.
+ */
+ if (!data) {
+ dev_dbg(sdmac->sdma->dev, "MEMCPY in case?\n");
+ mem_data.priority = 2;
+ mem_data.peripheral_type = IMX_DMATYPE_MEMORY;
+ mem_data.dma_request = 0;
+ mem_data.dma_request2 = 0;
+ data = &mem_data;
+
+ sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
+ }
switch (data->priority) {
case DMA_PRIO_HIGH:
@@ -1307,6 +1332,10 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
if (sdma_alloc_bd(desc))
goto err_desc_out;
+ /* No slave_config called in MEMCPY case, so do here */
+ if (direction == DMA_MEM_TO_MEM)
+ sdma_config_ownership(sdmac, false, true, false);
+
if (sdma_load_context(sdmac))
goto err_desc_out;
@@ -1318,6 +1347,63 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
return NULL;
}
+static struct dma_async_tx_descriptor *sdma_prep_memcpy(
+ struct dma_chan *chan, dma_addr_t dma_dst,
+ dma_addr_t dma_src, size_t len, unsigned long flags)
+{
+ struct sdma_channel *sdmac = to_sdma_chan(chan);
+ struct sdma_engine *sdma = sdmac->sdma;
+ int channel = sdmac->channel;
+ size_t count;
+ int i = 0, param;
+ struct sdma_buffer_descriptor *bd;
+ struct sdma_desc *desc;
+
+ if (!chan || !len)
+ return NULL;
+
+ dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
+ &dma_src, &dma_dst, len, channel);
+
+ desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM,
+ len / SDMA_BD_MAX_CNT + 1);
+ if (!desc)
+ return NULL;
+
+ do {
+ count = min_t(size_t, len, SDMA_BD_MAX_CNT);
+ bd = &desc->bd[i];
+ bd->buffer_addr = dma_src;
+ bd->ext_buffer_addr = dma_dst;
+ bd->mode.count = count;
+ desc->chn_count += count;
+ /* align with sdma->dma_device.copy_align: 4bytes */
+ bd->mode.command = 0;
+
+ dma_src += count;
+ dma_dst += count;
+ len -= count;
+ i++;
+
+ param = BD_DONE | BD_EXTD | BD_CONT;
+ /* last bd */
+ if (!len) {
+ param |= BD_INTR;
+ param |= BD_LAST;
+ param &= ~BD_CONT;
+ }
+
+ dev_dbg(sdma->dev, "entry %d: count: %zd dma: 0x%x %s%s\n",
+ i, count, bd->buffer_addr,
+ param & BD_WRAP ? "wrap" : "",
+ param & BD_INTR ? " intr" : "");
+
+ bd->mode.status = param;
+ } while (len);
+
+ return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
+}
+
static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
struct dma_chan *chan, struct scatterlist *sgl,
unsigned int sg_len, enum dma_transfer_direction direction,
@@ -1903,6 +1989,7 @@ static int sdma_probe(struct platform_device *pdev)
dma_cap_set(DMA_SLAVE, sdma->dma_device.cap_mask);
dma_cap_set(DMA_CYCLIC, sdma->dma_device.cap_mask);
+ dma_cap_set(DMA_MEMCPY, sdma->dma_device.cap_mask);
INIT_LIST_HEAD(&sdma->dma_device.channels);
/* Initialize channel parameters */
@@ -1969,8 +2056,10 @@ static int sdma_probe(struct platform_device *pdev)
sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+ sdma->dma_device.device_prep_dma_memcpy = sdma_prep_memcpy;
sdma->dma_device.device_issue_pending = sdma_issue_pending;
sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
+ sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);
platform_set_drvdata(pdev, sdma);
^ permalink raw reply related
* [v2,1/3] dmaengine: imx-sdma: add SDMA_BD_MAX_CNT to replace '0xffff'
From: Robin Gong @ 2018-07-13 13:08 UTC (permalink / raw)
To: vkoul, dan.j.williams, shawnguo, s.hauer, fabio.estevam, linux
Cc: linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
Add macro SDMA_BD_MAX_CNT to replace '0xffff'.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/imx-sdma.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 3b622d6..e3d5e73 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -185,6 +185,7 @@
* Mode/Count of data node descriptors - IPCv2
*/
struct sdma_mode_count {
+#define SDMA_BD_MAX_CNT 0xffff
u32 count : 16; /* size of the buffer pointed by this BD */
u32 status : 8; /* E,R,I,C,W,D status bits stored here */
u32 command : 8; /* command mostly used for channel 0 */
@@ -1344,9 +1345,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
count = sg_dma_len(sg);
- if (count > 0xffff) {
+ if (count > SDMA_BD_MAX_CNT) {
dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
- channel, count, 0xffff);
+ channel, count, SDMA_BD_MAX_CNT);
goto err_bd_out;
}
@@ -1421,9 +1422,9 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
sdmac->flags |= IMX_DMA_SG_LOOP;
- if (period_len > 0xffff) {
+ if (period_len > SDMA_BD_MAX_CNT) {
dev_err(sdma->dev, "SDMA channel %d: maximum period size exceeded: %zu > %d\n",
- channel, period_len, 0xffff);
+ channel, period_len, SDMA_BD_MAX_CNT);
goto err_bd_out;
}
@@ -1970,7 +1971,7 @@ static int sdma_probe(struct platform_device *pdev)
sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
sdma->dma_device.device_issue_pending = sdma_issue_pending;
sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
- dma_set_max_seg_size(sdma->dma_device.dev, 65535);
+ dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);
platform_set_drvdata(pdev, sdma);
^ permalink raw reply related
* dma: sh: rcar-dmac: avoid to write CHCR.TE to 1 if TCR is set to 0
From: Geert Uytterhoeven @ 2018-07-13 9:22 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: Vinod Koul, dmaengine, Linux-Renesas
On Mon, Jul 2, 2018 at 11:41 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> This patch fixes an issue that unexpected retransfering happens
> if TCR is set to 0 before rcar_dmac_sync_tcr() writes DE bit to
> the CHCR register. For example, sh-sci driver can reproduce this
> issue like below:
>
> In rx_timer_fn(): /* CHCR DE bit may be set to 1 */
> dmaengine_tx_status()
> rcar_dmac_tx_status()
> rcar_dmac_chan_get_residue()
> rcar_dmac_sync_tcr() /* TCR is possible to be set to 0 */
>
> According to the description of commit 73a47bd0da66 ("dmaengine:
> rcar-dmac: use TCRB instead of TCR for residue"), "this buffered data
> will be transferred if CHCR::DE bit was cleared". So, this patch
> doesn't need to check TCRB register.
>
> Fixes: 73a47bd0da66 ("dmaengine: rcar-dmac: use TCRB instead of TCR for residue")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Looks reasonable
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
^ permalink raw reply
* [v2,1/2] dmaengine: sh: rcar-dmac: add a new function to clear CHCR.DE with barrier
From: Geert Uytterhoeven @ 2018-07-13 9:16 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: Vinod Koul, dmaengine, Linux-Renesas
On Wed, Jul 11, 2018 at 4:11 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> This patch adds a new function rcar_dmac_clear_chcr_de() to simplify
> adding pause function later.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
^ permalink raw reply
* [v2,2/3] dmaengine: imx-sdma: add memcpy interface
From: Sascha Hauer @ 2018-07-13 6:16 UTC (permalink / raw)
To: Robin Gong
Cc: vkoul, dan.j.williams, shawnguo, fabio.estevam, linux,
linux-arm-kernel, kernel, dmaengine, linux-kernel, linux-imx
On Fri, Jul 13, 2018 at 09:08:46PM +0800, Robin Gong wrote:
> Add MEMCPY capability for imx-sdma driver.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
> drivers/dma/imx-sdma.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 92 insertions(+), 3 deletions(-)
>
> @@ -1318,6 +1347,63 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
> return NULL;
> }
>
> +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> + struct dma_chan *chan, dma_addr_t dma_dst,
> + dma_addr_t dma_src, size_t len, unsigned long flags)
> +{
> + struct sdma_channel *sdmac = to_sdma_chan(chan);
> + struct sdma_engine *sdma = sdmac->sdma;
> + int channel = sdmac->channel;
> + size_t count;
> + int i = 0, param;
> + struct sdma_buffer_descriptor *bd;
> + struct sdma_desc *desc;
> +
> + if (!chan || !len)
> + return NULL;
> +
> + dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> + &dma_src, &dma_dst, len, channel);
> +
> + desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM,
> + len / SDMA_BD_MAX_CNT + 1);
> + if (!desc)
> + return NULL;
> +
> + do {
> + count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> + bd = &desc->bd[i];
> + bd->buffer_addr = dma_src;
> + bd->ext_buffer_addr = dma_dst;
> + bd->mode.count = count;
> + desc->chn_count += count;
> + /* align with sdma->dma_device.copy_align: 4bytes */
> + bd->mode.command = 0;
> +
> + dma_src += count;
> + dma_dst += count;
> + len -= count;
> + i++;
NACK.
Please actually look at your code and find out where you do unaligned DMA
accesses. Hint: What happens when this loop body is executed more than once?
Sascha
> +
> + param = BD_DONE | BD_EXTD | BD_CONT;
> + /* last bd */
> + if (!len) {
> + param |= BD_INTR;
> + param |= BD_LAST;
> + param &= ~BD_CONT;
> + }
> +
> + dev_dbg(sdma->dev, "entry %d: count: %zd dma: 0x%x %s%s\n",
> + i, count, bd->buffer_addr,
> + param & BD_WRAP ? "wrap" : "",
> + param & BD_INTR ? " intr" : "");
> +
> + bd->mode.status = param;
> + } while (len);
> +
^ permalink raw reply
* [02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
From: Paul Burton @ 2018-07-11 23:27 UTC (permalink / raw)
To: Vinod, Rob Herring, Mark Rutland
Cc: Paul Cercueil, Ralf Baechle, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
Hi Vinod,
On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote:
> > > > - jzdma->base = devm_ioremap_resource(dev, res);
> > > > - if (IS_ERR(jzdma->base))
> > > > - return PTR_ERR(jzdma->base);
> > > > + jzdma->chn_base = devm_ioremap_resource(dev, res);
> > > > + if (IS_ERR(jzdma->chn_base))
> > > > + return PTR_ERR(jzdma->chn_base);
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > + if (!res) {
> > > > + dev_err(dev, "failed to get I/O memory\n");
> > > > + return -EINVAL;
> > > > + }
> > >
> > > okay and this breaks if you happen to get probed on older DT. I think DT
> > > is treated as ABI so you need to continue support older method while
> > > finding if DT has split resources
> >
> > See my response to PrasannaKumar. All the Ingenic-based boards do compile
> > the devicetree within the kernel, so I think it's still fine to add breaking
> > changes. I'll wait on @Rob to give his point of view on this, though.
> >
> > (It's not something hard to change, but I'd like to know what's the policy
> > in that case. I have other DT-breaking patches to submit)
>
> The policy is that DT is an ABI and should not break :)
I think in general that's a good policy to have for compatibility, but
if it's known for certain that the DT for all users of a driver is
always built into the kernel then I don't see why we shouldn't feel free
to change a binding. I agree with Paul that it'd be interesting to hear
the DT binding maintainers take on this.
Thanks,
Paul
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC
From: Paul Cercueil @ 2018-07-11 23:13 UTC (permalink / raw)
To: Vinod
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
Le mer. 11 juil. 2018 à 14:18, Vinod <vkoul@kernel.org> a écrit :
> On 10-07-18, 17:45, Paul Cercueil wrote:
>>
>>
>> Le lun. 9 juil. 2018 à 19:14, Vinod <vkoul@kernel.org> a écrit :
>> > On 03-07-18, 14:32, Paul Cercueil wrote:
>> > > The JZ4725B has one DMA core starring six DMA channels.
>> > > As for the JZ4770, each DMA channel's clock can be enabled with
>> > > a register write, the difference here being that once started,
>> it
>> > > is not possible to turn it off.
>> >
>> > ok so disable for this, right..
>> >
>> > > @@ -204,6 +205,8 @@ static inline void
>> > > jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
>> > > {
>> > > if (jzdma->version == ID_JZ4770)
>> > > jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
>> > > + else if (jzdma->version == ID_JZ4725B)
>> > > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
>> >
>> > but you are writing to a different register here..
>>
>> Yes. SoCs >= JZ4770 have the DCKE read-only register, and
>> DCKES/DCKEC to
>> set/clear bits in DCKE.
>> On JZ4725B, DCKE is read/write, but the zeros written are ignored
>> (at least
>> that's what the
>> documentation says).
>
> and that was not documented in the log... so i though it maybe a typo.
Right, I will add a comment in-code to explain that it's normal.
> --
> ~Vinod
Thanks,
-Paul
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
From: Paul Cercueil @ 2018-07-11 23:13 UTC (permalink / raw)
To: Vinod
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
Le mer. 11 juil. 2018 à 14:16, Vinod <vkoul@kernel.org> a écrit :
> On 10-07-18, 17:36, Paul Cercueil wrote:
>
>> > > @@ -3,7 +3,8 @@
>> > > Required properties:
>> > >
>> > > - compatible: Should be "ingenic,jz4780-dma"
>> > > -- reg: Should contain the DMA controller registers location
>> and
>> > > length.
>> > > +- reg: Should contain the DMA channel registers location and
>> > > length, followed
>> > > + by the DMA controller registers location and length.
>> > > - interrupts: Should contain the interrupt specifier of the
>> DMA
>> > > controller.
>> > > - interrupt-parent: Should be the phandle of the interrupt
>> > > controller that
>> > > - clocks: Should contain a clock specifier for the JZ4780 PDMA
>> > > clock.
>> > > @@ -22,7 +23,8 @@ Example:
>> > >
>> > > dma: dma@13420000 {
>> > > compatible = "ingenic,jz4780-dma";
>> > > - reg = <0x13420000 0x10000>;
>> > > + reg = <0x13420000 0x400
>> > > + 0x13421000 0x40>;
>> >
>> > Second should be optional or we break platform which may not have
>> > updated DT..
>>
>> See comment below.
>>
>> > > - jzdma->base = devm_ioremap_resource(dev, res);
>> > > - if (IS_ERR(jzdma->base))
>> > > - return PTR_ERR(jzdma->base);
>> > > + jzdma->chn_base = devm_ioremap_resource(dev, res);
>> > > + if (IS_ERR(jzdma->chn_base))
>> > > + return PTR_ERR(jzdma->chn_base);
>> > > +
>> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> > > + if (!res) {
>> > > + dev_err(dev, "failed to get I/O memory\n");
>> > > + return -EINVAL;
>> > > + }
>> >
>> > okay and this breaks if you happen to get probed on older DT. I
>> think DT
>> > is treated as ABI so you need to continue support older method
>> while
>> > finding if DT has split resources
>>
>> See my response to PrasannaKumar. All the Ingenic-based boards do
>> compile
>> the devicetree within the kernel, so I think it's still fine to add
>> breaking
>> changes. I'll wait on @Rob to give his point of view on this,
>> though.
>>
>> (It's not something hard to change, but I'd like to know what's the
>> policy
>> in that case. I have other DT-breaking patches to submit)
>
> The policy is that DT is an ABI and should not break :)
>
> Who maintains Ingenic arch. MAINTAINERS doesn't tell me.
Unofficially that would be me :)
Otherwise that would be the MIPS maintainers, Ralf and Paul (Burton).
> --
> ~Vinod
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [v2,1/4] dt-bindings: k3dma: add optional property hisilicon,dma-min-chan
From: Rob Herring @ 2018-07-11 16:09 UTC (permalink / raw)
To: Vinod, Guodong Xu
Cc: mark.rutland, dan.j.williams, xuwei5, catalin.marinas,
will.deacon, liyu65, suzhuangluan, xuhongtao8, zhongkaihua,
xuezhiliang, xupeng7, sunliang10, fengbaopeng, dmaengine,
devicetree, linux-kernel, linux-arm-kernel
On Mon, Jul 09, 2018 at 04:04:38PM +0530, Vinod wrote:
> On 06-07-18, 11:55, Guodong Xu wrote:
> > From: Li Yu <liyu65@hisilicon.com>
> >
> > Add optional property hisilicon,dma-min-chan for k3dma.
> >
> > Signed-off-by: Li Yu <liyu65@hisilicon.com>
> > Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> > ---
> > Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > index 4945aeac4dc4..f34202a80f3c 100644
> > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > @@ -12,6 +12,11 @@ Required properties:
> > have specific request line
> > - clocks: clock required
> >
> > +Optional properties:
> > +- hisilicon,dma-min-chan: the minimum DMA channel number which is usable
> > + Default value is 0, but in some platform it is
> > + configured 1, like in hi3660 platform
> > +
> > Example:
> >
> > Controller:
> > @@ -21,6 +26,7 @@ Controller:
> > #dma-cells = <1>;
> > dma-channels = <16>;
> > dma-requests = <27>;
> > + hisilicon,dma-min-chan = <1>;
>
> Am still expecting this to be a mask
If so calling it 'min' doesn't make sense. And a mask should have 'mask'
in the name.
Rob
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC
From: Vinod Koul @ 2018-07-11 12:18 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 10-07-18, 17:45, Paul Cercueil wrote:
>
>
> Le lun. 9 juil. 2018 à 19:14, Vinod <vkoul@kernel.org> a écrit :
> > On 03-07-18, 14:32, Paul Cercueil wrote:
> > > The JZ4725B has one DMA core starring six DMA channels.
> > > As for the JZ4770, each DMA channel's clock can be enabled with
> > > a register write, the difference here being that once started, it
> > > is not possible to turn it off.
> >
> > ok so disable for this, right..
> >
> > > @@ -204,6 +205,8 @@ static inline void
> > > jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
> > > {
> > > if (jzdma->version == ID_JZ4770)
> > > jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> > > + else if (jzdma->version == ID_JZ4725B)
> > > + jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
> >
> > but you are writing to a different register here..
>
> Yes. SoCs >= JZ4770 have the DCKE read-only register, and DCKES/DCKEC to
> set/clear bits in DCKE.
> On JZ4725B, DCKE is read/write, but the zeros written are ignored (at least
> that's what the
> documentation says).
and that was not documented in the log... so i though it maybe a typo.
^ permalink raw reply
* [02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers
From: Vinod Koul @ 2018-07-11 12:16 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
On 10-07-18, 17:36, Paul Cercueil wrote:
> > > @@ -3,7 +3,8 @@
> > > Required properties:
> > >
> > > - compatible: Should be "ingenic,jz4780-dma"
> > > -- reg: Should contain the DMA controller registers location and
> > > length.
> > > +- reg: Should contain the DMA channel registers location and
> > > length, followed
> > > + by the DMA controller registers location and length.
> > > - interrupts: Should contain the interrupt specifier of the DMA
> > > controller.
> > > - interrupt-parent: Should be the phandle of the interrupt
> > > controller that
> > > - clocks: Should contain a clock specifier for the JZ4780 PDMA
> > > clock.
> > > @@ -22,7 +23,8 @@ Example:
> > >
> > > dma: dma@13420000 {
> > > compatible = "ingenic,jz4780-dma";
> > > - reg = <0x13420000 0x10000>;
> > > + reg = <0x13420000 0x400
> > > + 0x13421000 0x40>;
> >
> > Second should be optional or we break platform which may not have
> > updated DT..
>
> See comment below.
>
> > > - jzdma->base = devm_ioremap_resource(dev, res);
> > > - if (IS_ERR(jzdma->base))
> > > - return PTR_ERR(jzdma->base);
> > > + jzdma->chn_base = devm_ioremap_resource(dev, res);
> > > + if (IS_ERR(jzdma->chn_base))
> > > + return PTR_ERR(jzdma->chn_base);
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > + if (!res) {
> > > + dev_err(dev, "failed to get I/O memory\n");
> > > + return -EINVAL;
> > > + }
> >
> > okay and this breaks if you happen to get probed on older DT. I think DT
> > is treated as ABI so you need to continue support older method while
> > finding if DT has split resources
>
> See my response to PrasannaKumar. All the Ingenic-based boards do compile
> the devicetree within the kernel, so I think it's still fine to add breaking
> changes. I'll wait on @Rob to give his point of view on this, though.
>
> (It's not something hard to change, but I'd like to know what's the policy
> in that case. I have other DT-breaking patches to submit)
The policy is that DT is an ABI and should not break :)
Who maintains Ingenic arch. MAINTAINERS doesn't tell me.
^ permalink raw reply
* [01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels
From: Vinod Koul @ 2018-07-11 12:14 UTC (permalink / raw)
To: Paul Cercueil, Rob Herring
Cc: Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Zubair Lutfullah Kakakhel, Mathieu Malaterre, Daniel Silsby,
dmaengine, devicetree, linux-kernel, linux-mips
Hi Paul,
On 10-07-18, 17:29, Paul Cercueil wrote:
> > > +static const unsigned int jz4780_dma_nb_channels[] = {
> > > + [ID_JZ4780] = 32,
> > > +};
> > > +
> > > +static const struct of_device_id jz4780_dma_dt_match[] = {
> > > + { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> > > + {},
> > > +};
> >
> > Looking at description I was hoping that channels would be in DT,
> > channels is hardware information, so should come from DT rather than
> > coding the kernel...
>
> I had a talk with Linus Walleij (GPIO maintainer) about that:
> http://lkml.iu.edu/hypermail/linux/kernel/1701.3/05422.html
>
> And I agree with him, we shouldn't have in devicetree what we can deduce
> from the compatible string. But there doesn't seem to be an enforced
> policy about it.
Looking at this, yes that can be done as you have implemented but adding
new compatible and tables every time seems not so great to me.
If DT can describe these hardware features then driver can take action generically
and we avoid these tables and skip some patches here..
>
> @Rob, what do you think?
Rob what is the recommendation here?
>
> > > - jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> > > + if (of_id)
> > > + version = (enum jz_version)of_id->data;
> > > + else
> > > + version = ID_JZ4780; /* Default when not probed from DT */
> >
> > where else would it be probed from.... ?
>
> Platform, MFD driver, etc. But not likely to happen.
> I can remove these lines if you want.
Lets add when we land support for those.
^ permalink raw reply
* [v1,3/4] dmaengine: imx-sdma: support dmatest
From: Vinod Koul @ 2018-07-11 8:58 UTC (permalink / raw)
To: Robin Gong
Cc: s.hauer@pengutronix.de, dan.j.williams@intel.com,
shawnguo@kernel.org, Fabio Estevam, linux@armlinux.org.uk,
linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de,
dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
dl-linux-imx
On 11-07-18, 08:16, Robin Gong wrote:
> > > The problem seems to be that we do not know whether we are doing
> > > memcpy or not. Normally we get the information how a channel is to be
> > > configured in dma_device->device_config, but this function is not
> > > called in the memcpy case.
> >
> > Not really true, device_config only provides parameters to be configured for
> > next slave transaction
> >
> > > An alternative might also be to do the setup in
> > dma_device->device_prep_dma_memcpy.
> >
> > Precisely, see how other drivers do this
> >
> > Let's roll back a bit and foresee why is this required.
> >
> > In case of memcpy, you need to tell DMA to do transfer from src to dstn and
> > size. Additional parameters like buswidth etc should be derived for maximum
> > throughput (after all we are dma, people want it to be done
> > fastest)
> >
> > Now for slave, you are interfacing with a peripheral and don't know how that is
> > setup. So you need to match the parameters, otherwise you get overflow or
> > underflow and hence need for device_config
> >
> > Please do not derive additional notions from these, please do not assume
> > anything else, unless provided in documentation :)
> I will move such prepare jobs from slave_config to device_prep_dma_memcpy
> Instead of device_alloc_chan_resources as I did in v1, thus we have no 'chan->private'
> issue, just like drivers/dma/stm32-mdma.c. The only limitation is those prepare jobs
> (some register setting) will be done every time memcpy instead of only one time in slave_config
> or v1 case. Is that ok?
sounds fine to me
^ permalink raw reply
* [v1,3/4] dmaengine: imx-sdma: support dmatest
From: Robin Gong @ 2018-07-11 8:16 UTC (permalink / raw)
To: Vinod, s.hauer@pengutronix.de
Cc: dan.j.williams@intel.com, shawnguo@kernel.org, Fabio Estevam,
linux@armlinux.org.uk, linux-arm-kernel@lists.infradead.org,
kernel@pengutronix.de, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVmlub2QgW21haWx0bzp2
a291bEBrZXJuZWwub3JnXQ0KPiBTZW50OiAyMDE45bm0N+aciDEx5pelIDE1OjE5DQo+IFRvOiBz
LmhhdWVyQHBlbmd1dHJvbml4LmRlDQo+IENjOiBSb2JpbiBHb25nIDx5aWJpbi5nb25nQG54cC5j
b20+OyBkYW4uai53aWxsaWFtc0BpbnRlbC5jb207DQo+IHNoYXduZ3VvQGtlcm5lbC5vcmc7IEZh
YmlvIEVzdGV2YW0gPGZhYmlvLmVzdGV2YW1AbnhwLmNvbT47DQo+IGxpbnV4QGFybWxpbnV4Lm9y
Zy51azsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOw0KPiBrZXJuZWxAcGVu
Z3V0cm9uaXguZGU7IGRtYWVuZ2luZUB2Z2VyLmtlcm5lbC5vcmc7DQo+IGxpbnV4LWtlcm5lbEB2
Z2VyLmtlcm5lbC5vcmc7IGRsLWxpbnV4LWlteCA8bGludXgtaW14QG54cC5jb20+DQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggdjEgMy80XSBkbWFlbmdpbmU6IGlteC1zZG1hOiBzdXBwb3J0IGRtYXRl
c3QNCj4gDQo+IE9uIDExLTA3LTE4LCAwODo1Mywgcy5oYXVlckBwZW5ndXRyb25peC5kZSB3cm90
ZToNCj4gPiBPbiBXZWQsIEp1bCAxMSwgMjAxOCBhdCAwNjozNzowMkFNICswMDAwLCBSb2JpbiBH
b25nIHdyb3RlOg0KPiA+ID4NCj4gPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4g
PiA+ID4gRnJvbTogVmlub2QgW21haWx0bzp2a291bEBrZXJuZWwub3JnXQ0KPiA+ID4gPiBTZW50
OiAyMDE45bm0N+aciDEw5pelIDIzOjMzDQo+ID4gPiA+IFRvOiBSb2JpbiBHb25nIDx5aWJpbi5n
b25nQG54cC5jb20+DQo+ID4gPiA+IENjOiBkYW4uai53aWxsaWFtc0BpbnRlbC5jb207IHNoYXdu
Z3VvQGtlcm5lbC5vcmc7DQo+ID4gPiA+IHMuaGF1ZXJAcGVuZ3V0cm9uaXguZGU7IEZhYmlvIEVz
dGV2YW0gPGZhYmlvLmVzdGV2YW1AbnhwLmNvbT47DQo+ID4gPiA+IGxpbnV4QGFybWxpbnV4Lm9y
Zy51azsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOw0KPiA+ID4gPiBrZXJu
ZWxAcGVuZ3V0cm9uaXguZGU7IGRtYWVuZ2luZUB2Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiA+IGxp
bnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IGRsLWxpbnV4LWlteCA8bGludXgtaW14QG54cC5j
b20+DQo+ID4gPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjEgMy80XSBkbWFlbmdpbmU6IGlteC1z
ZG1hOiBzdXBwb3J0IGRtYXRlc3QNCj4gPiA+ID4NCj4gPiA+ID4gT24gMTEtMDctMTgsIDAwOjIz
LCBSb2JpbiBHb25nIHdyb3RlOg0KPiA+ID4gPiA+IGRtYXRlc3QobWVtY3B5KSB3aWxsIG5ldmVy
IGNhbGwgZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBiZWZvcmUNCj4gPiA+ID4gPiBwcmVwLA0KPiA+
ID4gPg0KPiA+ID4gPiBhbmQgdGhhdCBzaG91bGQgaGF2ZSBiZWVuIGEgaGludCB0byB5b3UgdGhh
dCB5b3Ugc2hvdWxkIG5vdCBleHBlY3QNCj4gPiA+ID4gdGhhdA0KPiA+ID4gPg0KPiA+ID4gPiA+
IHNvIGpvYnMgaW4gZG1hZW5naW5lX3NsYXZlX2NvbmZpZyBuZWVkIHRvIGJlIG1vdmVkIGludG8N
Cj4gPiA+ID4gPiBzb21ld2hlcmUgYmVmb3JlIGRldmljZV9wcmVwX2RtYV9tZW1jcHkuIEJlc2lk
ZXMsIGRtYXRlc3QgbmV2ZXINCj4gPiA+ID4gPiBzZXR1cCBjaGFuDQo+ID4gPiA+ID4gLT5wcml2
YXRlIGFzIG90aGVyIGNvbW1vbiBjYXNlIGxpa2UgdWFydC9hdWRpby9zcGkgd2lsbCBhbHdheXMN
Cj4gPiA+ID4gPiAtPnNldHVwDQo+ID4gPiA+ID4gY2hhbi0+cHJpdmF0ZS4gSGVyZSBjaGVjayBp
dCB0byBqdWRnZSBpZiBpdCdzIGRtYXRlc3QgY2FzZSBhbmQNCj4gPiA+ID4gPiBjaGFuLT5kbw0K
PiA+ID4gPiA+IGpvYnMgaW4gc2xhdmVfY29uZmlnLg0KPiA+ID4gPg0KPiA+ID4gPiBhbmQgeW91
IHNob3VsZCBub3QgZG8gYW55dGhpbmcgZm9yIGRtYXRlc3QuIFN1cHBvcnRpbmcgaXQgbWVhbnMN
Cj4gPiA+ID4gbWVtY3B5IGltcGxlbWVudGF0aW9uIGlzIG5vdCBjb3JyZWN0IDopDQo+ID4gPiBP
a2F5LCBJIHdpbGwgYW55IHdvcmQgYWJvdXQgZG1hdGVzdCBoZXJlIHNpbmNlIG1lbWNweSBhc3N1
bWUgbm8NCj4gPiA+IGNhbGxpbmcgc2xhdmVfY29uZmlnLg0KPiA+ID4gPg0KPiA+ID4gPiA+DQo+
ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogUm9iaW4gR29uZyA8eWliaW4uZ29uZ0BueHAuY29tPg0K
PiA+ID4gPiA+IC0tLQ0KPiA+ID4gPiA+ICBkcml2ZXJzL2RtYS9pbXgtc2RtYS5jIHwgMzcNCj4g
PiA+ID4gPiArKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tDQo+ID4gPiA+ID4g
IDEgZmlsZSBjaGFuZ2VkLCAyOCBpbnNlcnRpb25zKCspLCA5IGRlbGV0aW9ucygtKQ0KPiA+ID4g
PiA+DQo+ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZG1hL2lteC1zZG1hLmMgYi9kcml2
ZXJzL2RtYS9pbXgtc2RtYS5jDQo+ID4gPiA+ID4gaW5kZXgNCj4gPiA+ID4gPiBlZDIyNjdkLi40
OGYzNzQ5IDEwMDY0NA0KPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvZG1hL2lteC1zZG1hLmMNCj4g
PiA+ID4gPiArKysgYi9kcml2ZXJzL2RtYS9pbXgtc2RtYS5jDQo+ID4gPiA+ID4gQEAgLTEyMjIs
MTAgKzEyMjIsMzYgQEAgc3RhdGljIGludA0KPiA+ID4gPiA+IHNkbWFfYWxsb2NfY2hhbl9yZXNv
dXJjZXMoc3RydWN0IGRtYV9jaGFuICpjaGFuKSAgew0KPiA+ID4gPiA+ICAJc3RydWN0IHNkbWFf
Y2hhbm5lbCAqc2RtYWMgPSB0b19zZG1hX2NoYW4oY2hhbik7DQo+ID4gPiA+ID4gIAlzdHJ1Y3Qg
aW14X2RtYV9kYXRhICpkYXRhID0gY2hhbi0+cHJpdmF0ZTsNCj4gPiA+ID4gPiArCXN0cnVjdCBp
bXhfZG1hX2RhdGEgZGVmYXVsdF9kYXRhOw0KPiA+ID4gPiA+ICAJaW50IHByaW8sIHJldDsNCj4g
PiA+ID4gPg0KPiA+ID4gPiA+IC0JaWYgKCFkYXRhKQ0KPiA+ID4gPiA+IC0JCXJldHVybiAtRUlO
VkFMOw0KPiA+ID4gPiA+ICsJcmV0ID0gY2xrX2VuYWJsZShzZG1hYy0+c2RtYS0+Y2xrX2lwZyk7
DQo+ID4gPiA+ID4gKwlpZiAocmV0KQ0KPiA+ID4gPiA+ICsJCXJldHVybiByZXQ7DQo+ID4gPiA+
ID4gKwlyZXQgPSBjbGtfZW5hYmxlKHNkbWFjLT5zZG1hLT5jbGtfYWhiKTsNCj4gPiA+ID4gPiAr
CWlmIChyZXQpDQo+ID4gPiA+ID4gKwkJZ290byBkaXNhYmxlX2Nsa19pcGc7DQo+ID4gPiA+ID4g
KwkvKg0KPiA+ID4gPiA+ICsJICogZG1hdGVzdChtZW1jcHkpIHdpbGwgbmV2ZXIgY2FsbCBkbWFl
bmdpbmVfc2xhdmVfY29uZmlnIGJlZm9yZQ0KPiBwcmVwLA0KPiA+ID4gPiA+ICsJICogc28gam9i
cyBpbiBkbWFlbmdpbmVfc2xhdmVfY29uZmlnIG5lZWQgdG8gYmUgbW92ZWQgaW50bw0KPiBzb21l
d2hlcmUNCj4gPiA+ID4gPiArCSAqIGJlZm9yZSBkZXZpY2VfcHJlcF9kbWFfbWVtY3B5LiBCZXNp
ZGVzLCBkbWF0ZXN0IG5ldmVyIHNldHVwDQo+IGNoYW4NCj4gPiA+ID4gPiArCSAqIC0+cHJpdmF0
ZSBhcyBvdGhlciBjb21tb24gY2FzZXMgbGlrZSB1YXJ0L2F1ZGlvL3NwaSB3aWxsIHNldHVwDQo+
ID4gPiA+ID4gKwkgKiBjaGFuLT5wcml2YXRlIGFsd2F5cy4gSGVyZSBjaGVjayBpdCB0byBqdWRn
ZSBpZiBpdCdzIGRtYXRlc3QgY2FzZQ0KPiA+ID4gPiA+ICsJICogYW5kIGRvIGpvYnMgaW4gc2xh
dmVfY29uZmlnLg0KPiA+ID4gPiA+ICsJICovDQo+ID4gPiA+ID4gKwlpZiAoIWRhdGEpIHsNCj4g
PiA+ID4gPiArCQlkZXZfd2FybihzZG1hYy0+c2RtYS0+ZGV2LCAiZG1hdGVzdCBpcyBydW5uaW5n
P1xuIik7DQo+ID4gPiA+DQo+ID4gPiA+IHdoeSBpcyB0aGF0IGEgd2FybmluZyENCj4gPiA+IEN1
cnJlbnQgU0RNQSBkcml2ZXIgYXNzdW1lIGZpbHRlciBmdW5jdGlvbiB0byBzZXQgY2hhbi0+cHJp
dmF0ZSB3aXRoDQo+ID4gPiBzcGVjaWZpYyBkYXRhIChzdHJ1Y3QgaW14X2RtYV9kYXRhIGRtYV9k
YXRhKWxpa2UgYmVsb3cNCj4gKHNvdW5kL3NvYy9mc2wvZnNsX2FzcmNfZG1hLmMpOg0KPiA+ID4g
c3RhdGljIGJvb2wgZmlsdGVyKHN0cnVjdCBkbWFfY2hhbiAqY2hhbiwgdm9pZCAqcGFyYW0pIHsN
Cj4gPiA+ICAgICAgICAgaWYgKCFpbXhfZG1hX2lzX2dlbmVyYWxfcHVycG9zZShjaGFuKSkNCj4g
PiA+ICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7DQo+ID4gPiAgICAgICAgIGNoYW4tPnBy
aXZhdGUgPSBwYXJhbTsNCj4gPiA+ICAgICAgICAgcmV0dXJuIHRydWU7DQo+ID4gPiB9DQo+ID4g
Pg0KPiA+ID4gQnV0IGluIG1lbWNweSBjYXNlLCBhdCBsZWFzZSBkbWF0ZXN0IGNhc2UsIG5vIGNo
YW4tPnByaXZhdGUgc2V0IGluIGl0cyBmaWx0ZXINCj4gZnVuY3Rpb24uDQo+ID4gPiBTbyBoZXJl
IHRha2UgZG1hdGVzdCBhIHNwZWNpYWwgY2FzZSBhbmQgZG8gc29tZSBwcmVwYXJlIGpvYnMgZm9y
DQo+ID4gPiBtZW1jcHkuIEJ1dCBpZiB0aGUgVXBwZXIgZGV2aWNlIGRyaXZlciBjYWxsIGRtYV9y
ZXF1ZXN0X2NoYW5uZWwoKQ0KPiA+ID4gd2l0aCB0aGVpciBzcGVjaWZpYyBmaWx0ZXIgd2l0aG91
dCAnY2hhbi0+cHJpdmF0ZScgc2V0dGluZyBpbiB0aGUNCj4gPiA+IGZ1dHVyZS4gVGhlIHdhcm5p
bmcgbWVzc2FnZSBpcyBhIHVzZWZ1bCBoaW50IHRvIHRoZW0gdG8gQWRkICdjaGFuLT5wcml2YXRl
Jw0KPiBpbiBmaWx0ZXIgZnVuY3Rpb24uICBPciBkb2MgaXQgc29tZXdoZXJlPw0KPiA+DQo+ID4g
SW5zdGVhZCBvZiBkb2luZyBoZXVyaXN0aWNzIHRvIGd1ZXNzIHdoZXRoZXIgd2UgYXJlIGRvaW5n
IG1lbWNweSB5b3UNCj4gPiBjb3VsZCBpbnN0ZWFkIG1ha2UgbWVtY3B5IHRoZSBkZWZhdWx0IHdo
ZW4gc2xhdmVfY29uZmlnIGlzIG5vdCBjYWxsZWQsDQo+ID4gaS5lLiBkcm9wIHRoZSBpZiAoIWRh
dGEpIGNoZWNrIGNvbXBsZXRlbHkuDQo+ID4NCj4gPiA+ID4NCj4gPiA+ID4gPiArCQlzZG1hYy0+
d29yZF9zaXplICA9DQo+IHNkbWFjLT5zZG1hLT5kbWFfZGV2aWNlLmNvcHlfYWxpZ247DQo+ID4g
PiA+ID4gKwkJZGVmYXVsdF9kYXRhLnByaW9yaXR5ID0gMjsNCj4gPiA+ID4gPiArCQlkZWZhdWx0
X2RhdGEucGVyaXBoZXJhbF90eXBlID0gSU1YX0RNQVRZUEVfTUVNT1JZOw0KPiA+ID4gPiA+ICsJ
CWRlZmF1bHRfZGF0YS5kbWFfcmVxdWVzdCA9IDA7DQo+ID4gPiA+ID4gKwkJZGVmYXVsdF9kYXRh
LmRtYV9yZXF1ZXN0MiA9IDA7DQo+ID4gPiA+ID4gKwkJZGF0YSA9ICZkZWZhdWx0X2RhdGE7DQo+
ID4gPiA+ID4gKw0KPiA+ID4gPiA+ICsJCXNkbWFfY29uZmlnX293bmVyc2hpcChzZG1hYywgZmFs
c2UsIHRydWUsIGZhbHNlKTsNCj4gPiA+ID4gPiArCQlzZG1hX2dldF9wYyhzZG1hYywgSU1YX0RN
QVRZUEVfTUVNT1JZKTsNCj4gPiA+ID4gPiArCQlzZG1hX2xvYWRfY29udGV4dChzZG1hYyk7DQo+
ID4gPiA+ID4gKwl9DQo+ID4gPiA+DQo+ID4gPiA+IHRoaXMgbmVlZHMgdG8gYmUgZGVmYXVsdCBm
b3IgbWVtY3B5DQo+ID4NCj4gPiBUaGUgcHJvYmxlbSBzZWVtcyB0byBiZSB0aGF0IHdlIGRvIG5v
dCBrbm93IHdoZXRoZXIgd2UgYXJlIGRvaW5nDQo+ID4gbWVtY3B5IG9yIG5vdC4gTm9ybWFsbHkg
d2UgZ2V0IHRoZSBpbmZvcm1hdGlvbiBob3cgYSBjaGFubmVsIGlzIHRvIGJlDQo+ID4gY29uZmln
dXJlZCBpbiBkbWFfZGV2aWNlLT5kZXZpY2VfY29uZmlnLCBidXQgdGhpcyBmdW5jdGlvbiBpcyBu
b3QNCj4gPiBjYWxsZWQgaW4gdGhlIG1lbWNweSBjYXNlLg0KPiANCj4gTm90IHJlYWxseSB0cnVl
LCBkZXZpY2VfY29uZmlnIG9ubHkgcHJvdmlkZXMgcGFyYW1ldGVycyB0byBiZSBjb25maWd1cmVk
IGZvcg0KPiBuZXh0IHNsYXZlIHRyYW5zYWN0aW9uDQo+IA0KPiA+IEFuIGFsdGVybmF0aXZlIG1p
Z2h0IGFsc28gYmUgdG8gZG8gdGhlIHNldHVwIGluDQo+IGRtYV9kZXZpY2UtPmRldmljZV9wcmVw
X2RtYV9tZW1jcHkuDQo+IA0KPiBQcmVjaXNlbHksIHNlZSBob3cgb3RoZXIgZHJpdmVycyBkbyB0
aGlzDQo+IA0KPiBMZXQncyByb2xsIGJhY2sgYSBiaXQgYW5kIGZvcmVzZWUgd2h5IGlzIHRoaXMg
cmVxdWlyZWQuDQo+IA0KPiBJbiBjYXNlIG9mIG1lbWNweSwgeW91IG5lZWQgdG8gdGVsbCBETUEg
dG8gZG8gdHJhbnNmZXIgZnJvbSBzcmMgdG8gZHN0biBhbmQNCj4gc2l6ZS4gQWRkaXRpb25hbCBw
YXJhbWV0ZXJzIGxpa2UgYnVzd2lkdGggZXRjIHNob3VsZCBiZSBkZXJpdmVkIGZvciBtYXhpbXVt
DQo+IHRocm91Z2hwdXQgKGFmdGVyIGFsbCB3ZSBhcmUgZG1hLCBwZW9wbGUgd2FudCBpdCB0byBi
ZSBkb25lDQo+IGZhc3Rlc3QpDQo+IA0KPiBOb3cgZm9yIHNsYXZlLCB5b3UgYXJlIGludGVyZmFj
aW5nIHdpdGggYSBwZXJpcGhlcmFsIGFuZCBkb24ndCBrbm93IGhvdyB0aGF0IGlzDQo+IHNldHVw
LiBTbyB5b3UgbmVlZCB0byBtYXRjaCB0aGUgcGFyYW1ldGVycywgb3RoZXJ3aXNlIHlvdSBnZXQg
b3ZlcmZsb3cgb3INCj4gdW5kZXJmbG93IGFuZCBoZW5jZSBuZWVkIGZvciBkZXZpY2VfY29uZmln
DQo+IA0KPiBQbGVhc2UgZG8gbm90IGRlcml2ZSBhZGRpdGlvbmFsIG5vdGlvbnMgZnJvbSB0aGVz
ZSwgcGxlYXNlIGRvIG5vdCBhc3N1bWUNCj4gYW55dGhpbmcgZWxzZSwgdW5sZXNzIHByb3ZpZGVk
IGluIGRvY3VtZW50YXRpb24gOikNCkkgd2lsbCBtb3ZlIHN1Y2ggcHJlcGFyZSBqb2JzIGZyb20g
c2xhdmVfY29uZmlnIHRvIGRldmljZV9wcmVwX2RtYV9tZW1jcHkNCkluc3RlYWQgb2YgZGV2aWNl
X2FsbG9jX2NoYW5fcmVzb3VyY2VzIGFzIEkgZGlkIGluIHYxLCB0aHVzIHdlIGhhdmUgbm8gJ2No
YW4tPnByaXZhdGUnDQppc3N1ZSwganVzdCBsaWtlIGRyaXZlcnMvZG1hL3N0bTMyLW1kbWEuYy4g
VGhlIG9ubHkgbGltaXRhdGlvbiBpcyB0aG9zZSBwcmVwYXJlIGpvYnMNCihzb21lIHJlZ2lzdGVy
IHNldHRpbmcpIHdpbGwgYmUgZG9uZSBldmVyeSB0aW1lIG1lbWNweSBpbnN0ZWFkIG9mIG9ubHkg
b25lIHRpbWUgaW4gc2xhdmVfY29uZmlnDQpvciB2MSBjYXNlLiBJcyB0aGF0IG9rPw0KPiANCj4g
SW4gZG91YnQsIGp1c3QgYXNrIQ0KPiANCj4gSFRIDQo+IC0tDQo+IH5WaW5vZA0K
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [v1,3/4] dmaengine: imx-sdma: support dmatest
From: Vinod Koul @ 2018-07-11 7:19 UTC (permalink / raw)
To: s.hauer@pengutronix.de
Cc: Robin Gong, dan.j.williams@intel.com, shawnguo@kernel.org,
Fabio Estevam, linux@armlinux.org.uk,
linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de,
dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
dl-linux-imx
On 11-07-18, 08:53, s.hauer@pengutronix.de wrote:
> On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote:
> >
> > > -----Original Message-----
> > > From: Vinod [mailto:vkoul@kernel.org]
> > > Sent: 2018年7月10日 23:33
> > > To: Robin Gong <yibin.gong@nxp.com>
> > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> > > kernel@pengutronix.de; dmaengine@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> > >
> > > On 11-07-18, 00:23, Robin Gong wrote:
> > > > dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > >
> > > and that should have been a hint to you that you should not expect that
> > >
> > > > so jobs in dmaengine_slave_config need to be moved into somewhere
> > > > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > > ->private as other common case like uart/audio/spi will always setup
> > > > chan->private. Here check it to judge if it's dmatest case and do
> > > > jobs in slave_config.
> > >
> > > and you should not do anything for dmatest. Supporting it means memcpy
> > > implementation is not correct :)
> > Okay, I will any word about dmatest here since memcpy assume no calling
> > slave_config.
> > >
> > > >
> > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > ---
> > > > drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
> > > > 1 file changed, 28 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > > ed2267d..48f3749 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct
> > > > dma_chan *chan) {
> > > > struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > struct imx_dma_data *data = chan->private;
> > > > + struct imx_dma_data default_data;
> > > > int prio, ret;
> > > >
> > > > - if (!data)
> > > > - return -EINVAL;
> > > > + ret = clk_enable(sdmac->sdma->clk_ipg);
> > > > + if (ret)
> > > > + return ret;
> > > > + ret = clk_enable(sdmac->sdma->clk_ahb);
> > > > + if (ret)
> > > > + goto disable_clk_ipg;
> > > > + /*
> > > > + * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > > > + * so jobs in dmaengine_slave_config need to be moved into somewhere
> > > > + * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > > + * ->private as other common cases like uart/audio/spi will setup
> > > > + * chan->private always. Here check it to judge if it's dmatest case
> > > > + * and do jobs in slave_config.
> > > > + */
> > > > + if (!data) {
> > > > + dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> > >
> > > why is that a warning!
> > Current SDMA driver assume filter function to set chan->private with specific data
> > (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c):
> > static bool filter(struct dma_chan *chan, void *param)
> > {
> > if (!imx_dma_is_general_purpose(chan))
> > return false;
> > chan->private = param;
> > return true;
> > }
> >
> > But in memcpy case, at lease dmatest case, no chan->private set in its filter function.
> > So here take dmatest a special case and do some prepare jobs for memcpy. But if the
> > Upper device driver call dma_request_channel() with their specific filter without
> > 'chan->private' setting in the future. The warning message is a useful hint to them to
> > Add 'chan->private' in filter function. Or doc it somewhere?
>
> Instead of doing heuristics to guess whether we are doing memcpy you
> could instead make memcpy the default when slave_config is not called,
> i.e. drop the if (!data) check completely.
>
> > >
> > > > + sdmac->word_size = sdmac->sdma->dma_device.copy_align;
> > > > + default_data.priority = 2;
> > > > + default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > > + default_data.dma_request = 0;
> > > > + default_data.dma_request2 = 0;
> > > > + data = &default_data;
> > > > +
> > > > + sdma_config_ownership(sdmac, false, true, false);
> > > > + sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > > + sdma_load_context(sdmac);
> > > > + }
> > >
> > > this needs to be default for memcpy
>
> The problem seems to be that we do not know whether we are doing memcpy
> or not. Normally we get the information how a channel is to be
> configured in dma_device->device_config, but this function is not called
> in the memcpy case.
Not really true, device_config only provides parameters to be
configured for next slave transaction
> An alternative might also be to do the setup in dma_device->device_prep_dma_memcpy.
Precisely, see how other drivers do this
Let's roll back a bit and foresee why is this required.
In case of memcpy, you need to tell DMA to do transfer from src to dstn
and size. Additional parameters like buswidth etc should be derived for
maximum throughput (after all we are dma, people want it to be done
fastest)
Now for slave, you are interfacing with a peripheral and don't know how
that is setup. So you need to match the parameters, otherwise you get
overflow or underflow and hence need for device_config
Please do not derive additional notions from these, please do not assume
anything else, unless provided in documentation :)
In doubt, just ask!
HTH
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox