* [PATCH v12 0/4] add uart DMA function
From: Long Cheng @ 2019-04-27 3:36 UTC (permalink / raw)
To: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Nicolas Boichat, Matthias Brugger
Cc: Dan Williams, Greg Kroah-Hartman, Jiri Slaby, Sean Wang,
dmaengine, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, linux-serial, srv_heupstream, Yingjoe Chen, YT Shen,
Zhenbao Liu, Long Cheng
In Mediatek SOCs, the uart can support DMA function.
Base on DMA engine formwork, we add the DMA code to support uart. And put the code under drivers/dma/mediatek.
This series contains document bindings, Kconfig to control the function enable or not,
device tree including interrupt and dma device node, the code of UART DMA
Changes compared to v11
-modify TX/RX
-pause function by software
Changes compared to v10
-modify DMA tx status function
-modify 8250_mtk for DMA rx
-add notes to binding Document.
Changes compared to v9
-rename dt-bindings file
-remove direction from device_config
-simplified code
Changes compared to v8
-revise missing items
Changes compared to v7:
-modify apdma uart tx
Changes compared to v6:
-Correct spelling
Changes compared to v5:
-move 'requst irqs' to alloc channel
-remove tasklet.
Changes compared to v4:
-modify Kconfig depends on.
Changes compared to v3:
-fix CONFIG_PM, will cause build fail
Changes compared to v2:
-remove unimportant parameters
-instead of cookie, use APIs of virtual channel.
-use of_dma_xlate_by_chan_id.
Changes compared to v1:
-mian revised file, 8250_mtk_dma.c
--parameters renamed for standard
--remove atomic operation
Long Cheng (4):
dmaengine: mediatek: Add MediaTek UART APDMA support
arm: dts: mt2712: add uart APDMA to device tree
dt-bindings: dma: uart: rename binding
serial: 8250-mtk: modify uart DMA rx
.../devicetree/bindings/dma/8250_mtk_dma.txt | 33 -
.../devicetree/bindings/dma/mtk-uart-apdma.txt | 55 ++
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 51 ++
drivers/dma/mediatek/Kconfig | 11 +
drivers/dma/mediatek/Makefile | 1 +
drivers/dma/mediatek/mtk-uart-apdma.c | 666 ++++++++++++++++++++
drivers/tty/serial/8250/8250_mtk.c | 53 +-
7 files changed, 807 insertions(+), 63 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
create mode 100644 Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
create mode 100644 drivers/dma/mediatek/mtk-uart-apdma.c
--
1.7.9.5
^ permalink raw reply
* dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Peng Ma @ 2019-04-28 2:00 UTC (permalink / raw)
To: Vinod Koul
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Vinod,
Thanks your comments.
Please see my comments inline.
Best Regards,
Peng
>-----Original Message-----
>From: Vinod Koul <vkoul@kernel.org>
>Sent: 2019年4月26日 19:51
>To: Peng Ma <peng.ma@nxp.com>
>Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination
>descriptior format
>
>Caution: EXT Email
>
>On 19-04-19, 08:46, Peng Ma wrote:
>> CMD of Source/Destination descriptior format should be lower of
>
>s/descriptior/descriptor
>
[Peng Ma] Got it.
>> struct fsl_qdma_engine number data address.
>>
>> Signed-off-by: Peng Ma <peng.ma@nxp.com>
>> ---
>> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
>> 1 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
>> aa1d0ae..542765a 100644
>> --- a/drivers/dma/fsl-qdma.c
>> +++ b/drivers/dma/fsl-qdma.c
>> @@ -113,6 +113,7 @@
>> /* Field definition for Descriptor offset */
>> #define QDMA_CCDF_STATUS 20
>> #define QDMA_CCDF_OFFSET 20
>> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
>>
>> /* Field definition for safe loop count*/
>> #define FSL_QDMA_HALT_COUNT 1500
>> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
>>
>> };
>>
>> +static inline void
>> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
>> + sddf->data = QDMA_SDDF_CMD(val); }
>> +
>> static inline u64
>> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
>-341,6
>> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
>> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
>*fsl_comp,
>> dma_addr_t dst, dma_addr_t
>src,
>> u32 len) {
>> + u32 cmd;
>> struct fsl_qdma_format *sdf, *ddf;
>> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest;
>>
>> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> fsl_qdma_comp *fsl_comp,
>>
>> memset(fsl_comp->virt_addr, 0,
>FSL_QDMA_COMMAND_BUFFER_SIZE);
>> memset(fsl_comp->desc_virt_addr, 0,
>> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
>> +
>> /* Head Command Descriptor(Frame Descriptor) */
>> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
>> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
>> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
>fsl_qdma_comp *fsl_comp,
>> /* This entry is the last entry. */
>> qdma_csgf_set_f(csgf_dest, len);
>> /* Descriptor Buffer */
>> - sdf->data =
>> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> - ddf->data =
>> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> - ddf->data |=
>> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
>FSL_QDMA_CMD_LWC_OFFSET);
>> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> + qdma_sddf_set_cmd(sdf, cmd);
>> +
>> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
>FSL_QDMA_CMD_LWC_OFFSET);
>> + qdma_sddf_set_cmd(ddf, cmd);
>> }
>>
>> /*
>> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
>> irq, void *dev_id)
>>
>> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>>
>> - if (intr) {
>> + if (intr)
>> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
>error!\n");
>> - return IRQ_NONE;
>> - }
>
>this seems unrelated can you explain?
>
[Peng Ma] This is an added improvement. When an error occurs we should clean the error reg then to return.
I forgot to explain it on comments. Should I add this changed to the comments?
>>
>> qdma_writel(fsl_qdma, FSL_QDMA_DEDR_CLEAR, status +
>FSL_QDMA_DEDR);
>> return IRQ_HANDLED;
>> --
>> 1.7.1
>
>--
>~Vinod
^ permalink raw reply
* RE: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Peng Ma @ 2019-04-28 2:00 UTC (permalink / raw)
To: Vinod Koul
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190426115047.GW28103@vkoul-mobl>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 4197 bytes --]
Hi Vinod,
Thanks your comments.
Please see my comments inline.
Best Regards,
Peng
>-----Original Message-----
>From: Vinod Koul <vkoul@kernel.org>
>Sent: 2019Äê4ÔÂ26ÈÕ 19:51
>To: Peng Ma <peng.ma@nxp.com>
>Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination
>descriptior format
>
>Caution: EXT Email
>
>On 19-04-19, 08:46, Peng Ma wrote:
>> CMD of Source/Destination descriptior format should be lower of
>
>s/descriptior/descriptor
>
[Peng Ma] Got it.
>> struct fsl_qdma_engine number data address.
>>
>> Signed-off-by: Peng Ma <peng.ma@nxp.com>
>> ---
>> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
>> 1 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
>> aa1d0ae..542765a 100644
>> --- a/drivers/dma/fsl-qdma.c
>> +++ b/drivers/dma/fsl-qdma.c
>> @@ -113,6 +113,7 @@
>> /* Field definition for Descriptor offset */
>> #define QDMA_CCDF_STATUS 20
>> #define QDMA_CCDF_OFFSET 20
>> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
>>
>> /* Field definition for safe loop count*/
>> #define FSL_QDMA_HALT_COUNT 1500
>> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
>>
>> };
>>
>> +static inline void
>> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
>> + sddf->data = QDMA_SDDF_CMD(val); }
>> +
>> static inline u64
>> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
>-341,6
>> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
>> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
>*fsl_comp,
>> dma_addr_t dst, dma_addr_t
>src,
>> u32 len) {
>> + u32 cmd;
>> struct fsl_qdma_format *sdf, *ddf;
>> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest;
>>
>> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> fsl_qdma_comp *fsl_comp,
>>
>> memset(fsl_comp->virt_addr, 0,
>FSL_QDMA_COMMAND_BUFFER_SIZE);
>> memset(fsl_comp->desc_virt_addr, 0,
>> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
>> +
>> /* Head Command Descriptor(Frame Descriptor) */
>> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
>> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
>> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
>fsl_qdma_comp *fsl_comp,
>> /* This entry is the last entry. */
>> qdma_csgf_set_f(csgf_dest, len);
>> /* Descriptor Buffer */
>> - sdf->data =
>> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> - ddf->data =
>> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> - ddf->data |=
>> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
>FSL_QDMA_CMD_LWC_OFFSET);
>> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> + qdma_sddf_set_cmd(sdf, cmd);
>> +
>> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
>FSL_QDMA_CMD_LWC_OFFSET);
>> + qdma_sddf_set_cmd(ddf, cmd);
>> }
>>
>> /*
>> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
>> irq, void *dev_id)
>>
>> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>>
>> - if (intr) {
>> + if (intr)
>> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
>error!\n");
>> - return IRQ_NONE;
>> - }
>
>this seems unrelated can you explain?
>
[Peng Ma] This is an added improvement. When an error occurs we should clean the error reg then to return.
I forgot to explain it on comments. Should I add this changed to the comments?
>>
>> qdma_writel(fsl_qdma, FSL_QDMA_DEDR_CLEAR, status +
>FSL_QDMA_DEDR);
>> return IRQ_HANDLED;
>> --
>> 1.7.1
>
>--
>~Vinod
^ permalink raw reply
* [PATCH next v2 1/2] dmaengine: bcm-sba-raid: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-28 4:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Kefeng Wang, Vinod Koul, dmaengine
In-Reply-To: <20190426115235.GX28103@vkoul-mobl>
Using dev_get_drvdata directly.
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/dma/bcm-sba-raid.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 72878ac5c78d..fa81d0177765 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -1459,8 +1459,7 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
static int sba_debugfs_stats_show(struct seq_file *file, void *offset)
{
- struct platform_device *pdev = to_platform_device(file->private);
- struct sba_device *sba = platform_get_drvdata(pdev);
+ struct sba_device *sba = dev_get_drvdata(file->private);
/* Write stats in file */
sba_write_stats_in_seqfile(sba, file);
--
2.20.1
^ permalink raw reply related
* [PATCH next v2 2/2] dmaengine: nbpfaxi: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-28 4:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Kefeng Wang, Vinod Koul, dmaengine
In-Reply-To: <20190428040025.142181-1-wangkefeng.wang@huawei.com>
Using dev_get_drvdata directly.
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/dma/nbpfaxi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/nbpfaxi.c b/drivers/dma/nbpfaxi.c
index a67b292190f4..594409a6e975 100644
--- a/drivers/dma/nbpfaxi.c
+++ b/drivers/dma/nbpfaxi.c
@@ -1491,14 +1491,14 @@ MODULE_DEVICE_TABLE(platform, nbpf_ids);
#ifdef CONFIG_PM
static int nbpf_runtime_suspend(struct device *dev)
{
- struct nbpf_device *nbpf = platform_get_drvdata(to_platform_device(dev));
+ struct nbpf_device *nbpf = dev_get_drvdata(dev);
clk_disable_unprepare(nbpf->clk);
return 0;
}
static int nbpf_runtime_resume(struct device *dev)
{
- struct nbpf_device *nbpf = platform_get_drvdata(to_platform_device(dev));
+ struct nbpf_device *nbpf = dev_get_drvdata(dev);
return clk_prepare_enable(nbpf->clk);
}
#endif
--
2.20.1
^ permalink raw reply related
* [next,v2,1/2] dmaengine: bcm-sba-raid: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-28 4:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Kefeng Wang, Vinod Koul, dmaengine
Using dev_get_drvdata directly.
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/dma/bcm-sba-raid.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 72878ac5c78d..fa81d0177765 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -1459,8 +1459,7 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
static int sba_debugfs_stats_show(struct seq_file *file, void *offset)
{
- struct platform_device *pdev = to_platform_device(file->private);
- struct sba_device *sba = platform_get_drvdata(pdev);
+ struct sba_device *sba = dev_get_drvdata(file->private);
/* Write stats in file */
sba_write_stats_in_seqfile(sba, file);
^ permalink raw reply related
* [next,v2,2/2] dmaengine: nbpfaxi: Use dev_get_drvdata()
From: Kefeng Wang @ 2019-04-28 4:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Kefeng Wang, Vinod Koul, dmaengine
Using dev_get_drvdata directly.
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/dma/nbpfaxi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/nbpfaxi.c b/drivers/dma/nbpfaxi.c
index a67b292190f4..594409a6e975 100644
--- a/drivers/dma/nbpfaxi.c
+++ b/drivers/dma/nbpfaxi.c
@@ -1491,14 +1491,14 @@ MODULE_DEVICE_TABLE(platform, nbpf_ids);
#ifdef CONFIG_PM
static int nbpf_runtime_suspend(struct device *dev)
{
- struct nbpf_device *nbpf = platform_get_drvdata(to_platform_device(dev));
+ struct nbpf_device *nbpf = dev_get_drvdata(dev);
clk_disable_unprepare(nbpf->clk);
return 0;
}
static int nbpf_runtime_resume(struct device *dev)
{
- struct nbpf_device *nbpf = platform_get_drvdata(to_platform_device(dev));
+ struct nbpf_device *nbpf = dev_get_drvdata(dev);
return clk_prepare_enable(nbpf->clk);
}
#endif
^ permalink raw reply related
* [GIT PULL]: late dmaengine fixes for v5.1-rc7
From: Vinod Koul @ 2019-04-28 6:26 UTC (permalink / raw)
To: Linus Torvalds; +Cc: dma
[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]
Hi Linus,
Here is the late pull request for v5.1-rc7
Please consider pulling to receive:
The following changes since commit 79a3aaa7b82e3106be97842dedfd8429248896e6:
Linux 5.1-rc3 (2019-03-31 14:39:29 -0700)
are available in the Git repository at:
git://git.infradead.org/users/vkoul/slave-dma.git tags/dmaengine-fix-5.1-rc7
for you to fetch changes up to 5bb5c3a3ac102158b799bf5eda871223aa5e9c25:
dmaengine: mediatek-cqdma: fix wrong register usage in mtk_cqdma_start (2019-04-26 17:26:38 +0530)
----------------------------------------------------------------
dmaengine-fix-5.1-rc7
dmaengine fixes for v5.1-rc7
- fix for wrong register use in mediatek driver
- fix in sh driver for glitch is tx_status and treating 0 a valid
reside for cyclic
- fix in bcm driver for using right memory allocation flag
----------------------------------------------------------------
Achim Dahlhoff (1):
dmaengine: sh: rcar-dmac: Fix glitch in dmaengine_tx_status
Dirk Behme (1):
dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid
Shun-Chih Yu (1):
dmaengine: mediatek-cqdma: fix wrong register usage in mtk_cqdma_start
Stefan Wahren (1):
dmaengine: bcm2835: Avoid GFP_KERNEL in device_prep_slave_sg
drivers/dma/bcm2835-dma.c | 2 +-
drivers/dma/mediatek/mtk-cqdma.c | 2 +-
drivers/dma/sh/rcar-dmac.c | 30 ++++++++++++++++++++++++++----
3 files changed, 28 insertions(+), 6 deletions(-)
Thanks
--
~Vinod
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Vinod Koul @ 2019-04-29 5:13 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
On 26-04-19, 15:41, Arnaud Pouliquen wrote:
> >> During residue calculation. the DMA can switch to the next sg. When
> >> this race condition occurs, the residue returned value is not valid.
> >> Indeed the position in the sg returned by the hardware is the position
> >> of the next sg, not the current sg.
> >> Solution is to check the sg after the calculation to verify it.
> >> If a transition is detected we consider that the DMA has switched to
> >> the beginning of next sg.
> >
> > Now, that sounds like duct tape. Why should we bother doing that.
> >
> > Also looking back at the stm32_dma_desc_residue() and calls to it from
> > stm32_dma_tx_status() am not sure we are doing the right thing
> Please, could you explain what you have in mind here?
So when we call vchan_find_desc() that tells us if the descriptor is in
the issued queue or not.. Ideally it should not matter if we have one
or N descriptors issued to hardware.
So why should you bother checking for next_sg.
> > why are we looking at next_sg here, can you explain me that please
>
> This solution is similar to one implemented in the at_hdmac.c driver
> (atc_get_bytes_left function).
>
> Yes could be consider as a workaround for a hardware issue...
>
> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
> mode is mainly use for audio transfer initiated by an ALSA driver.
>
> >From hardware point of view the DMA transfers first block based on sg1,
> then it updates registers to prepare sg2 transfer, and then generates an
> IRQ to inform that it issues the next transfer (sg2).
>
> Then driver can update sg1 to prepare the third transfer...
>
> In parallel the client driver can requests status to get the residue to
> update internal pointer.
> The issue is in the race condition between the call of the
> device_tx_status ops and the update of the DMA register on sg switch.
Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
irqs are disabled, so even if sg2 was loaded, you will not get an
interrupt and wont know. By looking at sg1 register you will see that
sg1 is telling you that it has finished and residue can be zero. That is
fine and correct to report.
Most important thing here is that reside is for _requested_ descriptor
and not _current_ descriptor, so looking into sg2 doesnt not fit.
> During a short time the hardware updated the registers containing the
> sg ID but not the transfer counter(SxNDTR). In this case there is a
> mismatch between the Sg ID and the associated transfer counter.
> So residue calculation is wrong.
> Idea of this patch is to perform the calculation and then to crosscheck
> that the hardware has not switched to the next sg during the
> calculation. The way to crosscheck is to compare the the sg ID before
> and after the calculation.
>
> I tested the solution to force a new recalculation but no real solution
> to trust the registers during this phase. In this case an approximation
> is to consider that the DMA is transferring the first bytes of the next sg.
> So we return the residue corresponding to the beginning of the next buffer.
And that is wrong!. The argument is 'cookie' and you return residue for
that cookie.
For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
is processing cookie 2, then for tx_status on:
cookie 1: return DMA_COMPLETE, residue 0
cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
cookie 3: return DMA_IN_PROGRESS, residue txn length
cookie 4: return DMA_IN_PROGRESS, residue txn length
Thanks
^ permalink raw reply
* Re: [PATCH] dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Vinod Koul @ 2019-04-29 5:13 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
In-Reply-To: <6894b54e-651f-1caf-d363-79d1ef0eee14@st.com>
On 26-04-19, 15:41, Arnaud Pouliquen wrote:
> >> During residue calculation. the DMA can switch to the next sg. When
> >> this race condition occurs, the residue returned value is not valid.
> >> Indeed the position in the sg returned by the hardware is the position
> >> of the next sg, not the current sg.
> >> Solution is to check the sg after the calculation to verify it.
> >> If a transition is detected we consider that the DMA has switched to
> >> the beginning of next sg.
> >
> > Now, that sounds like duct tape. Why should we bother doing that.
> >
> > Also looking back at the stm32_dma_desc_residue() and calls to it from
> > stm32_dma_tx_status() am not sure we are doing the right thing
> Please, could you explain what you have in mind here?
So when we call vchan_find_desc() that tells us if the descriptor is in
the issued queue or not.. Ideally it should not matter if we have one
or N descriptors issued to hardware.
So why should you bother checking for next_sg.
> > why are we looking at next_sg here, can you explain me that please
>
> This solution is similar to one implemented in the at_hdmac.c driver
> (atc_get_bytes_left function).
>
> Yes could be consider as a workaround for a hardware issue...
>
> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
> mode is mainly use for audio transfer initiated by an ALSA driver.
>
> >From hardware point of view the DMA transfers first block based on sg1,
> then it updates registers to prepare sg2 transfer, and then generates an
> IRQ to inform that it issues the next transfer (sg2).
>
> Then driver can update sg1 to prepare the third transfer...
>
> In parallel the client driver can requests status to get the residue to
> update internal pointer.
> The issue is in the race condition between the call of the
> device_tx_status ops and the update of the DMA register on sg switch.
Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
irqs are disabled, so even if sg2 was loaded, you will not get an
interrupt and wont know. By looking at sg1 register you will see that
sg1 is telling you that it has finished and residue can be zero. That is
fine and correct to report.
Most important thing here is that reside is for _requested_ descriptor
and not _current_ descriptor, so looking into sg2 doesnt not fit.
> During a short time the hardware updated the registers containing the
> sg ID but not the transfer counter(SxNDTR). In this case there is a
> mismatch between the Sg ID and the associated transfer counter.
> So residue calculation is wrong.
> Idea of this patch is to perform the calculation and then to crosscheck
> that the hardware has not switched to the next sg during the
> calculation. The way to crosscheck is to compare the the sg ID before
> and after the calculation.
>
> I tested the solution to force a new recalculation but no real solution
> to trust the registers during this phase. In this case an approximation
> is to consider that the DMA is transferring the first bytes of the next sg.
> So we return the residue corresponding to the beginning of the next buffer.
And that is wrong!. The argument is 'cookie' and you return residue for
that cookie.
For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
is processing cookie 2, then for tx_status on:
cookie 1: return DMA_COMPLETE, residue 0
cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
cookie 3: return DMA_IN_PROGRESS, residue txn length
cookie 4: return DMA_IN_PROGRESS, residue txn length
Thanks
--
~Vinod
^ permalink raw reply
* dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Vinod Koul @ 2019-04-29 5:15 UTC (permalink / raw)
To: Peng Ma
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
On 28-04-19, 02:00, Peng Ma wrote:
> Hi Vinod,
>
> Thanks your comments.
> Please see my comments inline.
>
> Best Regards,
> Peng
>
> >-----Original Message-----
> >From: Vinod Koul <vkoul@kernel.org>
> >Sent: 2019年4月26日 19:51
> >To: Peng Ma <peng.ma@nxp.com>
> >Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
> >dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> >Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination
> >descriptior format
> >
> >Caution: EXT Email
> >
> >On 19-04-19, 08:46, Peng Ma wrote:
> >> CMD of Source/Destination descriptior format should be lower of
> >
> >s/descriptior/descriptor
> >
> [Peng Ma] Got it.
> >> struct fsl_qdma_engine number data address.
> >>
> >> Signed-off-by: Peng Ma <peng.ma@nxp.com>
> >> ---
> >> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
> >> 1 files changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
> >> aa1d0ae..542765a 100644
> >> --- a/drivers/dma/fsl-qdma.c
> >> +++ b/drivers/dma/fsl-qdma.c
> >> @@ -113,6 +113,7 @@
> >> /* Field definition for Descriptor offset */
> >> #define QDMA_CCDF_STATUS 20
> >> #define QDMA_CCDF_OFFSET 20
> >> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
> >>
> >> /* Field definition for safe loop count*/
> >> #define FSL_QDMA_HALT_COUNT 1500
> >> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
> >>
> >> };
> >>
> >> +static inline void
> >> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
> >> + sddf->data = QDMA_SDDF_CMD(val); }
> >> +
> >> static inline u64
> >> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
> >-341,6
> >> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
> >> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
> >*fsl_comp,
> >> dma_addr_t dst, dma_addr_t
> >src,
> >> u32 len) {
> >> + u32 cmd;
> >> struct fsl_qdma_format *sdf, *ddf;
> >> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest;
> >>
> >> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
> >> fsl_qdma_comp *fsl_comp,
> >>
> >> memset(fsl_comp->virt_addr, 0,
> >FSL_QDMA_COMMAND_BUFFER_SIZE);
> >> memset(fsl_comp->desc_virt_addr, 0,
> >> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
> >> +
> >> /* Head Command Descriptor(Frame Descriptor) */
> >> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
> >> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
> >> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
> >fsl_qdma_comp *fsl_comp,
> >> /* This entry is the last entry. */
> >> qdma_csgf_set_f(csgf_dest, len);
> >> /* Descriptor Buffer */
> >> - sdf->data =
> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> - ddf->data =
> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> - ddf->data |=
> >> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
> >FSL_QDMA_CMD_LWC_OFFSET);
> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> + qdma_sddf_set_cmd(sdf, cmd);
> >> +
> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
> >FSL_QDMA_CMD_LWC_OFFSET);
> >> + qdma_sddf_set_cmd(ddf, cmd);
> >> }
> >>
> >> /*
> >> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
> >> irq, void *dev_id)
> >>
> >> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> >>
> >> - if (intr) {
> >> + if (intr)
> >> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
> >error!\n");
> >> - return IRQ_NONE;
> >> - }
> >
> >this seems unrelated can you explain?
> >
> [Peng Ma] This is an added improvement. When an error occurs we should clean the error reg then to return.
> I forgot to explain it on comments. Should I add this changed to the comments?
Yes and you should make it a separate patch. A patch should do only 1
thing!
^ permalink raw reply
* Re: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Vinod Koul @ 2019-04-29 5:15 UTC (permalink / raw)
To: Peng Ma
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <VI1PR04MB4431E13D34650C2EE3D25861ED380@VI1PR04MB4431.eurprd04.prod.outlook.com>
On 28-04-19, 02:00, Peng Ma wrote:
> Hi Vinod,
>
> Thanks your comments.
> Please see my comments inline.
>
> Best Regards,
> Peng
>
> >-----Original Message-----
> >From: Vinod Koul <vkoul@kernel.org>
> >Sent: 2019年4月26日 19:51
> >To: Peng Ma <peng.ma@nxp.com>
> >Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
> >dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> >Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination
> >descriptior format
> >
> >Caution: EXT Email
> >
> >On 19-04-19, 08:46, Peng Ma wrote:
> >> CMD of Source/Destination descriptior format should be lower of
> >
> >s/descriptior/descriptor
> >
> [Peng Ma] Got it.
> >> struct fsl_qdma_engine number data address.
> >>
> >> Signed-off-by: Peng Ma <peng.ma@nxp.com>
> >> ---
> >> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
> >> 1 files changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
> >> aa1d0ae..542765a 100644
> >> --- a/drivers/dma/fsl-qdma.c
> >> +++ b/drivers/dma/fsl-qdma.c
> >> @@ -113,6 +113,7 @@
> >> /* Field definition for Descriptor offset */
> >> #define QDMA_CCDF_STATUS 20
> >> #define QDMA_CCDF_OFFSET 20
> >> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
> >>
> >> /* Field definition for safe loop count*/
> >> #define FSL_QDMA_HALT_COUNT 1500
> >> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
> >>
> >> };
> >>
> >> +static inline void
> >> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
> >> + sddf->data = QDMA_SDDF_CMD(val); }
> >> +
> >> static inline u64
> >> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
> >-341,6
> >> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
> >> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
> >*fsl_comp,
> >> dma_addr_t dst, dma_addr_t
> >src,
> >> u32 len) {
> >> + u32 cmd;
> >> struct fsl_qdma_format *sdf, *ddf;
> >> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest;
> >>
> >> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
> >> fsl_qdma_comp *fsl_comp,
> >>
> >> memset(fsl_comp->virt_addr, 0,
> >FSL_QDMA_COMMAND_BUFFER_SIZE);
> >> memset(fsl_comp->desc_virt_addr, 0,
> >> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
> >> +
> >> /* Head Command Descriptor(Frame Descriptor) */
> >> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
> >> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
> >> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
> >fsl_qdma_comp *fsl_comp,
> >> /* This entry is the last entry. */
> >> qdma_csgf_set_f(csgf_dest, len);
> >> /* Descriptor Buffer */
> >> - sdf->data =
> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> - ddf->data =
> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> - ddf->data |=
> >> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
> >FSL_QDMA_CMD_LWC_OFFSET);
> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> + qdma_sddf_set_cmd(sdf, cmd);
> >> +
> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
> >> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
> >FSL_QDMA_CMD_LWC_OFFSET);
> >> + qdma_sddf_set_cmd(ddf, cmd);
> >> }
> >>
> >> /*
> >> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
> >> irq, void *dev_id)
> >>
> >> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> >>
> >> - if (intr) {
> >> + if (intr)
> >> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
> >error!\n");
> >> - return IRQ_NONE;
> >> - }
> >
> >this seems unrelated can you explain?
> >
> [Peng Ma] This is an added improvement. When an error occurs we should clean the error reg then to return.
> I forgot to explain it on comments. Should I add this changed to the comments?
Yes and you should make it a separate patch. A patch should do only 1
thing!
--
~Vinod
^ permalink raw reply
* [next,v2,1/2] dmaengine: bcm-sba-raid: Use dev_get_drvdata()
From: Vinod Koul @ 2019-04-29 5:17 UTC (permalink / raw)
To: Kefeng Wang; +Cc: linux-kernel, Vinod Koul, dmaengine
On 28-04-19, 12:00, Kefeng Wang wrote:
> Using dev_get_drvdata directly.
>
Applied both, thanks
^ permalink raw reply
* Re: [PATCH next v2 1/2] dmaengine: bcm-sba-raid: Use dev_get_drvdata()
From: Vinod Koul @ 2019-04-29 5:17 UTC (permalink / raw)
To: Kefeng Wang; +Cc: linux-kernel, Vinod Koul, dmaengine
In-Reply-To: <20190428040025.142181-1-wangkefeng.wang@huawei.com>
On 28-04-19, 12:00, Kefeng Wang wrote:
> Using dev_get_drvdata directly.
>
Applied both, thanks
--
~Vinod
^ permalink raw reply
* [V3,2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs
From: Vinod Koul @ 2019-04-29 5:32 UTC (permalink / raw)
To: Peng Ma; +Cc: dan.j.williams, leoyang.li, linux-kernel, dmaengine
On 09-04-19, 15:22, Peng Ma wrote:
> DPPA2(Data Path Acceleration Architecture 2) qDMA
> The qDMA supports channel virtualization by allowing DMA jobs to be enqueued
> into different frame queues. Core can initiate a DMA transaction by preparing
> a frame descriptor(FD) for each DMA job and enqueuing this job to a frame queue.
> through a hardware portal. The qDMA prefetches DMA jobs from the frame queues.
> It then schedules and dispatches to internal DMA hardware engines, which
> generate read and write requests. Both qDMA source data and destination data can
> be either contiguous or non-contiguous using one or more scatter/gather tables.
> The qDMA supports global bandwidth flow control where all DMA transactions are
> stalled if the bandwidth threshold has been reached. Also supported are
> transaction based read throttling.
>
> Add NXP dppa2 qDMA to support some of Layerscape SoCs.
> such as: LS1088A, LS208xA, LX2, etc.
>
> Signed-off-by: Peng Ma <peng.ma@nxp.com>
> ---
> changed for v3:
> - Add depends on arm64 for dpaa2 qdma driver
> - The dpaa2_io_service_[de]register functions have a new parameter
> So update all calls to some functions
>
> drivers/dma/Kconfig | 2 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-dpaa2-qdma/Kconfig | 9 +
> drivers/dma/fsl-dpaa2-qdma/Makefile | 3 +
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 782 +++++++++++++++++++++++++++++++
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h | 152 ++++++
> 6 files changed, 949 insertions(+), 0 deletions(-)
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/Kconfig
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/Makefile
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index eaf78f4..08aae01 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -671,6 +671,8 @@ source "drivers/dma/sh/Kconfig"
>
> source "drivers/dma/ti/Kconfig"
>
> +source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
> +
> # clients
> comment "DMA Clients"
> depends on DMA_ENGINE
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 6126e1c..2499ed8 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_UNIPHIER_MDMAC) += uniphier-mdmac.o
> obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
> obj-$(CONFIG_ZX_DMA) += zx_dma.o
> obj-$(CONFIG_ST_FDMA) += st_fdma.o
> +obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
>
> obj-y += mediatek/
> obj-y += qcom/
> diff --git a/drivers/dma/fsl-dpaa2-qdma/Kconfig b/drivers/dma/fsl-dpaa2-qdma/Kconfig
> new file mode 100644
> index 0000000..258ed6b
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/Kconfig
> @@ -0,0 +1,9 @@
> +menuconfig FSL_DPAA2_QDMA
> + tristate "NXP DPAA2 QDMA"
> + depends on ARM64
> + depends on FSL_MC_BUS && FSL_MC_DPIO
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + NXP Data Path Acceleration Architecture 2 QDMA driver,
> + using the NXP MC bus driver.
> diff --git a/drivers/dma/fsl-dpaa2-qdma/Makefile b/drivers/dma/fsl-dpaa2-qdma/Makefile
> new file mode 100644
> index 0000000..c1d0226
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for the NXP DPAA2 qDMA controllers
> +obj-$(CONFIG_FSL_DPAA2_QDMA) += dpaa2-qdma.o dpdmai.o
> diff --git a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> new file mode 100644
> index 0000000..0cdde0f
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> @@ -0,0 +1,782 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2014-2018 NXP
> +
> +/*
> + * Author: Changming Huang <jerry.huang@nxp.com>
> + *
> + * Driver for the NXP QDMA engine with QMan mode.
> + * Channel virtualization is supported through enqueuing of DMA jobs to,
> + * or dequeuing DMA jobs from different work queues with QMan portal.
> + * This module can be found on NXP LS2 SoCs.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/dmapool.h>
> +#include <linux/of_irq.h>
> +#include <linux/iommu.h>
> +#include <linux/sys_soc.h>
> +#include <linux/fsl/mc.h>
> +#include <soc/fsl/dpaa2-io.h>
> +
> +#include "../virt-dma.h"
> +#include "dpdmai_cmd.h"
> +#include "dpdmai.h"
> +#include "dpaa2-qdma.h"
> +
> +static bool smmu_disable = true;
> +
> +static struct dpaa2_qdma_chan *to_dpaa2_qdma_chan(struct dma_chan *chan)
> +{
> + return container_of(chan, struct dpaa2_qdma_chan, vchan.chan);
> +}
> +
> +static struct dpaa2_qdma_comp *to_fsl_qdma_comp(struct virt_dma_desc *vd)
> +{
> + return container_of(vd, struct dpaa2_qdma_comp, vdesc);
> +}
> +
> +static int dpaa2_qdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + struct device *dev = &dpaa2_qdma->priv->dpdmai_dev->dev;
> +
> + dpaa2_chan->fd_pool = dma_pool_create("fd_pool", dev,
> + FD_POOL_SIZE, 32, 0);
> + if (!dpaa2_chan->fd_pool)
> + return -ENOMEM;
> +
> + return dpaa2_qdma->desc_allocated++;
> +}
> +
> +static void dpaa2_qdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + unsigned long flags;
> +
> + LIST_HEAD(head);
> +
> + spin_lock_irqsave(&dpaa2_chan->vchan.lock, flags);
> + vchan_get_all_descriptors(&dpaa2_chan->vchan, &head);
> + spin_unlock_irqrestore(&dpaa2_chan->vchan.lock, flags);
> +
> + vchan_dma_desc_free_list(&dpaa2_chan->vchan, &head);
> +
> + dpaa2_dpdmai_free_comp(dpaa2_chan, &dpaa2_chan->comp_used);
> + dpaa2_dpdmai_free_comp(dpaa2_chan, &dpaa2_chan->comp_free);
> +
> + dma_pool_destroy(dpaa2_chan->fd_pool);
> + dpaa2_qdma->desc_allocated--;
> +}
> +
> +/*
> + * Request a command descriptor for enqueue.
> + */
> +static struct dpaa2_qdma_comp *
> +dpaa2_qdma_request_desc(struct dpaa2_qdma_chan *dpaa2_chan)
> +{
> + struct dpaa2_qdma_comp *comp_temp = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
> + if (list_empty(&dpaa2_chan->comp_free)) {
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
GFP_NOWAIT?
> + if (!comp_temp)
> + goto err;
> + comp_temp->fd_virt_addr =
> + dma_pool_alloc(dpaa2_chan->fd_pool, GFP_NOWAIT,
> + &comp_temp->fd_bus_addr);
> + if (!comp_temp->fd_virt_addr)
err handling seems incorrect, you dont clean up, caller doesnt check
return!
> + goto err;
> +
> + comp_temp->fl_virt_addr =
> + (void *)((struct dpaa2_fd *)
> + comp_temp->fd_virt_addr + 1);
casts and pointer math, what could go wrong!! This doesnt smell right!
> + comp_temp->fl_bus_addr = comp_temp->fd_bus_addr +
> + sizeof(struct dpaa2_fd);
why not use fl_virt_addr and get the bus_address?
> + comp_temp->desc_virt_addr =
> + (void *)((struct dpaa2_fl_entry *)
> + comp_temp->fl_virt_addr + 3);
> + comp_temp->desc_bus_addr = comp_temp->fl_bus_addr +
> + sizeof(struct dpaa2_fl_entry) * 3;
pointer math in the two calls doesnt match and as I said doesnt look
good...
> +
> + comp_temp->qchan = dpaa2_chan;
> + return comp_temp;
> + }
> + comp_temp = list_first_entry(&dpaa2_chan->comp_free,
> + struct dpaa2_qdma_comp, list);
> + list_del(&comp_temp->list);
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> +
> + comp_temp->qchan = dpaa2_chan;
> +err:
> + return comp_temp;
> +}
> +
> +static void
> +dpaa2_qdma_populate_fd(u32 format, struct dpaa2_qdma_comp *dpaa2_comp)
> +{
> + struct dpaa2_fd *fd;
> +
> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
whats with the casts! you seem to like them! You are casting away from
void!
> + memset(fd, 0, sizeof(struct dpaa2_fd));
> +
> + /* fd populated */
> + dpaa2_fd_set_addr(fd, dpaa2_comp->fl_bus_addr);
> + /* Bypass memory translation, Frame list format, short length disable */
> + /* we need to disable BMT if fsl-mc use iova addr */
> + if (smmu_disable)
> + dpaa2_fd_set_bpid(fd, QMAN_FD_BMT_ENABLE);
> + dpaa2_fd_set_format(fd, QMAN_FD_FMT_ENABLE | QMAN_FD_SL_DISABLE);
> +
> + dpaa2_fd_set_frc(fd, format | QDMA_SER_CTX);
> +}
> +
> +/* first frame list for descriptor buffer */
> +static void
> +dpaa2_qdma_populate_first_framel(struct dpaa2_fl_entry *f_list,
> + struct dpaa2_qdma_comp *dpaa2_comp,
> + bool wrt_changed)
> +{
> + struct dpaa2_qdma_sd_d *sdd;
> +
> + sdd = (struct dpaa2_qdma_sd_d *)dpaa2_comp->desc_virt_addr;
again
> + memset(sdd, 0, 2 * (sizeof(*sdd)));
> +
> + /* source descriptor CMD */
> + sdd->cmd = cpu_to_le32(QDMA_SD_CMD_RDTTYPE_COHERENT);
> + sdd++;
> +
> + /* dest descriptor CMD */
> + if (wrt_changed)
> + sdd->cmd = cpu_to_le32(LX2160_QDMA_DD_CMD_WRTTYPE_COHERENT);
> + else
> + sdd->cmd = cpu_to_le32(QDMA_DD_CMD_WRTTYPE_COHERENT);
> +
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + /* first frame list to source descriptor */
> + dpaa2_fl_set_addr(f_list, dpaa2_comp->desc_bus_addr);
> + dpaa2_fl_set_len(f_list, 0x20);
> + dpaa2_fl_set_format(f_list, QDMA_FL_FMT_SBF | QDMA_FL_SL_LONG);
> +
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +}
> +
> +/* source and destination frame list */
> +static void
> +dpaa2_qdma_populate_frames(struct dpaa2_fl_entry *f_list,
> + dma_addr_t dst, dma_addr_t src,
> + size_t len, uint8_t fmt)
> +{
> + /* source frame list to source buffer */
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + dpaa2_fl_set_addr(f_list, src);
> + dpaa2_fl_set_len(f_list, len);
> +
> + /* single buffer frame or scatter gather frame */
> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
> +
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +
> + f_list++;
> +
> + /* destination frame list to destination buffer */
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + dpaa2_fl_set_addr(f_list, dst);
> + dpaa2_fl_set_len(f_list, len);
> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
> + /* single buffer frame or scatter gather frame */
> + dpaa2_fl_set_final(f_list, QDMA_FL_F);
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +}
> +
> +static struct dma_async_tx_descriptor
> +*dpaa2_qdma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst,
> + dma_addr_t src, size_t len, ulong flags)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma;
> + struct dpaa2_qdma_comp *dpaa2_comp;
> + struct dpaa2_fl_entry *f_list;
> + bool wrt_changed;
> + u32 format;
> +
> + dpaa2_qdma = dpaa2_chan->qdma;
> + dpaa2_comp = dpaa2_qdma_request_desc(dpaa2_chan);
> + wrt_changed = (bool)dpaa2_qdma->qdma_wrtype_fixup;
> +
> +#ifdef LONG_FORMAT
compile flag and define, so else part is dead code??
> + format = QDMA_FD_LONG_FORMAT;
> +#else
> + format = QDMA_FD_SHORT_FORMAT;
> +#endif
> + /* populate Frame descriptor */
> + dpaa2_qdma_populate_fd(format, dpaa2_comp);
> +
> + f_list = (struct dpaa2_fl_entry *)dpaa2_comp->fl_virt_addr;
> +
> +#ifdef LONG_FORMAT
> + /* first frame list for descriptor buffer (logn format) */
> + dpaa2_qdma_populate_first_framel(f_list, dpaa2_comp, wrt_changed);
> +
> + f_list++;
> +#endif
> +
> + dpaa2_qdma_populate_frames(f_list, dst, src, len, QDMA_FL_FMT_SBF);
> +
> + return vchan_tx_prep(&dpaa2_chan->vchan, &dpaa2_comp->vdesc, flags);
> +}
> +
> +static enum
> +dma_status dpaa2_qdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + return dma_cookie_status(chan, cookie, txstate);
> +}
> +
> +static void dpaa2_qdma_issue_pending(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + struct dpaa2_qdma_priv *priv = dpaa2_qdma->priv;
> + struct dpaa2_qdma_comp *dpaa2_comp;
> + struct virt_dma_desc *vdesc;
> + struct dpaa2_fd *fd;
> + unsigned long flags;
> + int err;
> +
> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
> + spin_lock(&dpaa2_chan->vchan.lock);
> + if (vchan_issue_pending(&dpaa2_chan->vchan)) {
> + vdesc = vchan_next_desc(&dpaa2_chan->vchan);
> + if (!vdesc)
> + goto err_enqueue;
> + dpaa2_comp = to_fsl_qdma_comp(vdesc);
> +
> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
> +
> + list_del(&vdesc->node);
> + list_add_tail(&dpaa2_comp->list, &dpaa2_chan->comp_used);
what does this list do?
> +
> + /* TOBO: priority hard-coded to zero */
You mean TODO?
> + err = dpaa2_io_service_enqueue_fq(NULL,
> + priv->tx_queue_attr[0].fqid, fd);
> + if (err) {
> + list_del(&dpaa2_comp->list);
> + list_add_tail(&dpaa2_comp->list,
> + &dpaa2_chan->comp_free);
> + }
> + }
> +err_enqueue:
> + spin_unlock(&dpaa2_chan->vchan.lock);
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> +}
> +
> +static int __cold dpaa2_qdma_setup(struct fsl_mc_device *ls_dev)
> +{
> + struct dpaa2_qdma_priv_per_prio *ppriv;
> + struct device *dev = &ls_dev->dev;
> + struct dpaa2_qdma_priv *priv;
> + u8 prio_def = DPDMAI_PRIO_NUM;
> + int err;
> + int i;
> +
> + priv = dev_get_drvdata(dev);
> +
> + priv->dev = dev;
> + priv->dpqdma_id = ls_dev->obj_desc.id;
> +
> + /*Get the handle for the DPDMAI this interface is associate with */
Please run checkpatch, it should have told you that you need space after
comment marker /* foo...
> + err = dpdmai_open(priv->mc_io, 0, priv->dpqdma_id, &ls_dev->mc_handle);
> + if (err) {
> + dev_err(dev, "dpdmai_open() failed\n");
> + return err;
> + }
> + dev_info(dev, "Opened dpdmai object successfully\n");
> +
> + err = dpdmai_get_attributes(priv->mc_io, 0, ls_dev->mc_handle,
> + &priv->dpdmai_attr);
> + if (err) {
> + dev_err(dev, "dpdmai_get_attributes() failed\n");
> + return err;
so you dont close what you opened in dpdmai_open() Please give a serious
thought and testing to this driver
> + }
> +
> + if (priv->dpdmai_attr.version.major > DPDMAI_VER_MAJOR) {
> + dev_err(dev, "DPDMAI major version mismatch\n"
> + "Found %u.%u, supported version is %u.%u\n",
> + priv->dpdmai_attr.version.major,
> + priv->dpdmai_attr.version.minor,
> + DPDMAI_VER_MAJOR, DPDMAI_VER_MINOR);
> + }
> +
> + if (priv->dpdmai_attr.version.minor > DPDMAI_VER_MINOR) {
> + dev_err(dev, "DPDMAI minor version mismatch\n"
> + "Found %u.%u, supported version is %u.%u\n",
> + priv->dpdmai_attr.version.major,
> + priv->dpdmai_attr.version.minor,
> + DPDMAI_VER_MAJOR, DPDMAI_VER_MINOR);
what is the implication of these error, why not bail out on these?
> + }
> +
> + priv->num_pairs = min(priv->dpdmai_attr.num_of_priorities, prio_def);
> + ppriv = kcalloc(priv->num_pairs, sizeof(*ppriv), GFP_KERNEL);
what is the context of the fn, sleepy, atomic?
> + if (!ppriv) {
> + dev_err(dev, "kzalloc for ppriv failed\n");
this need not be logged, core will do so
> + return -1;
really -1??
I think this driver needs more work, please fix these issues in the
comments above and also see in rest of the code
^ permalink raw reply
* Re: [V3 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs
From: Vinod Koul @ 2019-04-29 5:32 UTC (permalink / raw)
To: Peng Ma; +Cc: dan.j.williams, leoyang.li, linux-kernel, dmaengine
In-Reply-To: <20190409072212.15860-2-peng.ma@nxp.com>
On 09-04-19, 15:22, Peng Ma wrote:
> DPPA2(Data Path Acceleration Architecture 2) qDMA
> The qDMA supports channel virtualization by allowing DMA jobs to be enqueued
> into different frame queues. Core can initiate a DMA transaction by preparing
> a frame descriptor(FD) for each DMA job and enqueuing this job to a frame queue.
> through a hardware portal. The qDMA prefetches DMA jobs from the frame queues.
> It then schedules and dispatches to internal DMA hardware engines, which
> generate read and write requests. Both qDMA source data and destination data can
> be either contiguous or non-contiguous using one or more scatter/gather tables.
> The qDMA supports global bandwidth flow control where all DMA transactions are
> stalled if the bandwidth threshold has been reached. Also supported are
> transaction based read throttling.
>
> Add NXP dppa2 qDMA to support some of Layerscape SoCs.
> such as: LS1088A, LS208xA, LX2, etc.
>
> Signed-off-by: Peng Ma <peng.ma@nxp.com>
> ---
> changed for v3:
> - Add depends on arm64 for dpaa2 qdma driver
> - The dpaa2_io_service_[de]register functions have a new parameter
> So update all calls to some functions
>
> drivers/dma/Kconfig | 2 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-dpaa2-qdma/Kconfig | 9 +
> drivers/dma/fsl-dpaa2-qdma/Makefile | 3 +
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 782 +++++++++++++++++++++++++++++++
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h | 152 ++++++
> 6 files changed, 949 insertions(+), 0 deletions(-)
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/Kconfig
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/Makefile
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index eaf78f4..08aae01 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -671,6 +671,8 @@ source "drivers/dma/sh/Kconfig"
>
> source "drivers/dma/ti/Kconfig"
>
> +source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
> +
> # clients
> comment "DMA Clients"
> depends on DMA_ENGINE
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 6126e1c..2499ed8 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_UNIPHIER_MDMAC) += uniphier-mdmac.o
> obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
> obj-$(CONFIG_ZX_DMA) += zx_dma.o
> obj-$(CONFIG_ST_FDMA) += st_fdma.o
> +obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
>
> obj-y += mediatek/
> obj-y += qcom/
> diff --git a/drivers/dma/fsl-dpaa2-qdma/Kconfig b/drivers/dma/fsl-dpaa2-qdma/Kconfig
> new file mode 100644
> index 0000000..258ed6b
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/Kconfig
> @@ -0,0 +1,9 @@
> +menuconfig FSL_DPAA2_QDMA
> + tristate "NXP DPAA2 QDMA"
> + depends on ARM64
> + depends on FSL_MC_BUS && FSL_MC_DPIO
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + NXP Data Path Acceleration Architecture 2 QDMA driver,
> + using the NXP MC bus driver.
> diff --git a/drivers/dma/fsl-dpaa2-qdma/Makefile b/drivers/dma/fsl-dpaa2-qdma/Makefile
> new file mode 100644
> index 0000000..c1d0226
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for the NXP DPAA2 qDMA controllers
> +obj-$(CONFIG_FSL_DPAA2_QDMA) += dpaa2-qdma.o dpdmai.o
> diff --git a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> new file mode 100644
> index 0000000..0cdde0f
> --- /dev/null
> +++ b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
> @@ -0,0 +1,782 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2014-2018 NXP
> +
> +/*
> + * Author: Changming Huang <jerry.huang@nxp.com>
> + *
> + * Driver for the NXP QDMA engine with QMan mode.
> + * Channel virtualization is supported through enqueuing of DMA jobs to,
> + * or dequeuing DMA jobs from different work queues with QMan portal.
> + * This module can be found on NXP LS2 SoCs.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/dmapool.h>
> +#include <linux/of_irq.h>
> +#include <linux/iommu.h>
> +#include <linux/sys_soc.h>
> +#include <linux/fsl/mc.h>
> +#include <soc/fsl/dpaa2-io.h>
> +
> +#include "../virt-dma.h"
> +#include "dpdmai_cmd.h"
> +#include "dpdmai.h"
> +#include "dpaa2-qdma.h"
> +
> +static bool smmu_disable = true;
> +
> +static struct dpaa2_qdma_chan *to_dpaa2_qdma_chan(struct dma_chan *chan)
> +{
> + return container_of(chan, struct dpaa2_qdma_chan, vchan.chan);
> +}
> +
> +static struct dpaa2_qdma_comp *to_fsl_qdma_comp(struct virt_dma_desc *vd)
> +{
> + return container_of(vd, struct dpaa2_qdma_comp, vdesc);
> +}
> +
> +static int dpaa2_qdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + struct device *dev = &dpaa2_qdma->priv->dpdmai_dev->dev;
> +
> + dpaa2_chan->fd_pool = dma_pool_create("fd_pool", dev,
> + FD_POOL_SIZE, 32, 0);
> + if (!dpaa2_chan->fd_pool)
> + return -ENOMEM;
> +
> + return dpaa2_qdma->desc_allocated++;
> +}
> +
> +static void dpaa2_qdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + unsigned long flags;
> +
> + LIST_HEAD(head);
> +
> + spin_lock_irqsave(&dpaa2_chan->vchan.lock, flags);
> + vchan_get_all_descriptors(&dpaa2_chan->vchan, &head);
> + spin_unlock_irqrestore(&dpaa2_chan->vchan.lock, flags);
> +
> + vchan_dma_desc_free_list(&dpaa2_chan->vchan, &head);
> +
> + dpaa2_dpdmai_free_comp(dpaa2_chan, &dpaa2_chan->comp_used);
> + dpaa2_dpdmai_free_comp(dpaa2_chan, &dpaa2_chan->comp_free);
> +
> + dma_pool_destroy(dpaa2_chan->fd_pool);
> + dpaa2_qdma->desc_allocated--;
> +}
> +
> +/*
> + * Request a command descriptor for enqueue.
> + */
> +static struct dpaa2_qdma_comp *
> +dpaa2_qdma_request_desc(struct dpaa2_qdma_chan *dpaa2_chan)
> +{
> + struct dpaa2_qdma_comp *comp_temp = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
> + if (list_empty(&dpaa2_chan->comp_free)) {
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
GFP_NOWAIT?
> + if (!comp_temp)
> + goto err;
> + comp_temp->fd_virt_addr =
> + dma_pool_alloc(dpaa2_chan->fd_pool, GFP_NOWAIT,
> + &comp_temp->fd_bus_addr);
> + if (!comp_temp->fd_virt_addr)
err handling seems incorrect, you dont clean up, caller doesnt check
return!
> + goto err;
> +
> + comp_temp->fl_virt_addr =
> + (void *)((struct dpaa2_fd *)
> + comp_temp->fd_virt_addr + 1);
casts and pointer math, what could go wrong!! This doesnt smell right!
> + comp_temp->fl_bus_addr = comp_temp->fd_bus_addr +
> + sizeof(struct dpaa2_fd);
why not use fl_virt_addr and get the bus_address?
> + comp_temp->desc_virt_addr =
> + (void *)((struct dpaa2_fl_entry *)
> + comp_temp->fl_virt_addr + 3);
> + comp_temp->desc_bus_addr = comp_temp->fl_bus_addr +
> + sizeof(struct dpaa2_fl_entry) * 3;
pointer math in the two calls doesnt match and as I said doesnt look
good...
> +
> + comp_temp->qchan = dpaa2_chan;
> + return comp_temp;
> + }
> + comp_temp = list_first_entry(&dpaa2_chan->comp_free,
> + struct dpaa2_qdma_comp, list);
> + list_del(&comp_temp->list);
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> +
> + comp_temp->qchan = dpaa2_chan;
> +err:
> + return comp_temp;
> +}
> +
> +static void
> +dpaa2_qdma_populate_fd(u32 format, struct dpaa2_qdma_comp *dpaa2_comp)
> +{
> + struct dpaa2_fd *fd;
> +
> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
whats with the casts! you seem to like them! You are casting away from
void!
> + memset(fd, 0, sizeof(struct dpaa2_fd));
> +
> + /* fd populated */
> + dpaa2_fd_set_addr(fd, dpaa2_comp->fl_bus_addr);
> + /* Bypass memory translation, Frame list format, short length disable */
> + /* we need to disable BMT if fsl-mc use iova addr */
> + if (smmu_disable)
> + dpaa2_fd_set_bpid(fd, QMAN_FD_BMT_ENABLE);
> + dpaa2_fd_set_format(fd, QMAN_FD_FMT_ENABLE | QMAN_FD_SL_DISABLE);
> +
> + dpaa2_fd_set_frc(fd, format | QDMA_SER_CTX);
> +}
> +
> +/* first frame list for descriptor buffer */
> +static void
> +dpaa2_qdma_populate_first_framel(struct dpaa2_fl_entry *f_list,
> + struct dpaa2_qdma_comp *dpaa2_comp,
> + bool wrt_changed)
> +{
> + struct dpaa2_qdma_sd_d *sdd;
> +
> + sdd = (struct dpaa2_qdma_sd_d *)dpaa2_comp->desc_virt_addr;
again
> + memset(sdd, 0, 2 * (sizeof(*sdd)));
> +
> + /* source descriptor CMD */
> + sdd->cmd = cpu_to_le32(QDMA_SD_CMD_RDTTYPE_COHERENT);
> + sdd++;
> +
> + /* dest descriptor CMD */
> + if (wrt_changed)
> + sdd->cmd = cpu_to_le32(LX2160_QDMA_DD_CMD_WRTTYPE_COHERENT);
> + else
> + sdd->cmd = cpu_to_le32(QDMA_DD_CMD_WRTTYPE_COHERENT);
> +
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + /* first frame list to source descriptor */
> + dpaa2_fl_set_addr(f_list, dpaa2_comp->desc_bus_addr);
> + dpaa2_fl_set_len(f_list, 0x20);
> + dpaa2_fl_set_format(f_list, QDMA_FL_FMT_SBF | QDMA_FL_SL_LONG);
> +
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +}
> +
> +/* source and destination frame list */
> +static void
> +dpaa2_qdma_populate_frames(struct dpaa2_fl_entry *f_list,
> + dma_addr_t dst, dma_addr_t src,
> + size_t len, uint8_t fmt)
> +{
> + /* source frame list to source buffer */
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + dpaa2_fl_set_addr(f_list, src);
> + dpaa2_fl_set_len(f_list, len);
> +
> + /* single buffer frame or scatter gather frame */
> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
> +
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +
> + f_list++;
> +
> + /* destination frame list to destination buffer */
> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
> +
> + dpaa2_fl_set_addr(f_list, dst);
> + dpaa2_fl_set_len(f_list, len);
> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
> + /* single buffer frame or scatter gather frame */
> + dpaa2_fl_set_final(f_list, QDMA_FL_F);
> + /* bypass memory translation */
> + if (smmu_disable)
> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
> +}
> +
> +static struct dma_async_tx_descriptor
> +*dpaa2_qdma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst,
> + dma_addr_t src, size_t len, ulong flags)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma;
> + struct dpaa2_qdma_comp *dpaa2_comp;
> + struct dpaa2_fl_entry *f_list;
> + bool wrt_changed;
> + u32 format;
> +
> + dpaa2_qdma = dpaa2_chan->qdma;
> + dpaa2_comp = dpaa2_qdma_request_desc(dpaa2_chan);
> + wrt_changed = (bool)dpaa2_qdma->qdma_wrtype_fixup;
> +
> +#ifdef LONG_FORMAT
compile flag and define, so else part is dead code??
> + format = QDMA_FD_LONG_FORMAT;
> +#else
> + format = QDMA_FD_SHORT_FORMAT;
> +#endif
> + /* populate Frame descriptor */
> + dpaa2_qdma_populate_fd(format, dpaa2_comp);
> +
> + f_list = (struct dpaa2_fl_entry *)dpaa2_comp->fl_virt_addr;
> +
> +#ifdef LONG_FORMAT
> + /* first frame list for descriptor buffer (logn format) */
> + dpaa2_qdma_populate_first_framel(f_list, dpaa2_comp, wrt_changed);
> +
> + f_list++;
> +#endif
> +
> + dpaa2_qdma_populate_frames(f_list, dst, src, len, QDMA_FL_FMT_SBF);
> +
> + return vchan_tx_prep(&dpaa2_chan->vchan, &dpaa2_comp->vdesc, flags);
> +}
> +
> +static enum
> +dma_status dpaa2_qdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + return dma_cookie_status(chan, cookie, txstate);
> +}
> +
> +static void dpaa2_qdma_issue_pending(struct dma_chan *chan)
> +{
> + struct dpaa2_qdma_chan *dpaa2_chan = to_dpaa2_qdma_chan(chan);
> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
> + struct dpaa2_qdma_priv *priv = dpaa2_qdma->priv;
> + struct dpaa2_qdma_comp *dpaa2_comp;
> + struct virt_dma_desc *vdesc;
> + struct dpaa2_fd *fd;
> + unsigned long flags;
> + int err;
> +
> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
> + spin_lock(&dpaa2_chan->vchan.lock);
> + if (vchan_issue_pending(&dpaa2_chan->vchan)) {
> + vdesc = vchan_next_desc(&dpaa2_chan->vchan);
> + if (!vdesc)
> + goto err_enqueue;
> + dpaa2_comp = to_fsl_qdma_comp(vdesc);
> +
> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
> +
> + list_del(&vdesc->node);
> + list_add_tail(&dpaa2_comp->list, &dpaa2_chan->comp_used);
what does this list do?
> +
> + /* TOBO: priority hard-coded to zero */
You mean TODO?
> + err = dpaa2_io_service_enqueue_fq(NULL,
> + priv->tx_queue_attr[0].fqid, fd);
> + if (err) {
> + list_del(&dpaa2_comp->list);
> + list_add_tail(&dpaa2_comp->list,
> + &dpaa2_chan->comp_free);
> + }
> + }
> +err_enqueue:
> + spin_unlock(&dpaa2_chan->vchan.lock);
> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
> +}
> +
> +static int __cold dpaa2_qdma_setup(struct fsl_mc_device *ls_dev)
> +{
> + struct dpaa2_qdma_priv_per_prio *ppriv;
> + struct device *dev = &ls_dev->dev;
> + struct dpaa2_qdma_priv *priv;
> + u8 prio_def = DPDMAI_PRIO_NUM;
> + int err;
> + int i;
> +
> + priv = dev_get_drvdata(dev);
> +
> + priv->dev = dev;
> + priv->dpqdma_id = ls_dev->obj_desc.id;
> +
> + /*Get the handle for the DPDMAI this interface is associate with */
Please run checkpatch, it should have told you that you need space after
comment marker /* foo...
> + err = dpdmai_open(priv->mc_io, 0, priv->dpqdma_id, &ls_dev->mc_handle);
> + if (err) {
> + dev_err(dev, "dpdmai_open() failed\n");
> + return err;
> + }
> + dev_info(dev, "Opened dpdmai object successfully\n");
> +
> + err = dpdmai_get_attributes(priv->mc_io, 0, ls_dev->mc_handle,
> + &priv->dpdmai_attr);
> + if (err) {
> + dev_err(dev, "dpdmai_get_attributes() failed\n");
> + return err;
so you dont close what you opened in dpdmai_open() Please give a serious
thought and testing to this driver
> + }
> +
> + if (priv->dpdmai_attr.version.major > DPDMAI_VER_MAJOR) {
> + dev_err(dev, "DPDMAI major version mismatch\n"
> + "Found %u.%u, supported version is %u.%u\n",
> + priv->dpdmai_attr.version.major,
> + priv->dpdmai_attr.version.minor,
> + DPDMAI_VER_MAJOR, DPDMAI_VER_MINOR);
> + }
> +
> + if (priv->dpdmai_attr.version.minor > DPDMAI_VER_MINOR) {
> + dev_err(dev, "DPDMAI minor version mismatch\n"
> + "Found %u.%u, supported version is %u.%u\n",
> + priv->dpdmai_attr.version.major,
> + priv->dpdmai_attr.version.minor,
> + DPDMAI_VER_MAJOR, DPDMAI_VER_MINOR);
what is the implication of these error, why not bail out on these?
> + }
> +
> + priv->num_pairs = min(priv->dpdmai_attr.num_of_priorities, prio_def);
> + ppriv = kcalloc(priv->num_pairs, sizeof(*ppriv), GFP_KERNEL);
what is the context of the fn, sleepy, atomic?
> + if (!ppriv) {
> + dev_err(dev, "kzalloc for ppriv failed\n");
this need not be logged, core will do so
> + return -1;
really -1??
I think this driver needs more work, please fix these issues in the
comments above and also see in rest of the code
--
~Vinod
^ permalink raw reply
* dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Peng Ma @ 2019-04-29 6:28 UTC (permalink / raw)
To: Vinod Koul
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
>-----Original Message-----
>From: Vinod Koul <vkoul@kernel.org>
>Sent: 2019年4月29日 13:16
>To: Peng Ma <peng.ma@nxp.com>
>Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>source/destination descriptior format
>
>Caution: EXT Email
>
>On 28-04-19, 02:00, Peng Ma wrote:
>> Hi Vinod,
>>
>> Thanks your comments.
>> Please see my comments inline.
>>
>> Best Regards,
>> Peng
>>
>> >-----Original Message-----
>> >From: Vinod Koul <vkoul@kernel.org>
>> >Sent: 2019年4月26日 19:51
>> >To: Peng Ma <peng.ma@nxp.com>
>> >Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>> >dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>> >Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>> >source/destination descriptior format
>> >
>> >Caution: EXT Email
>> >
>> >On 19-04-19, 08:46, Peng Ma wrote:
>> >> CMD of Source/Destination descriptior format should be lower of
>> >
>> >s/descriptior/descriptor
>> >
>> [Peng Ma] Got it.
>> >> struct fsl_qdma_engine number data address.
>> >>
>> >> Signed-off-by: Peng Ma <peng.ma@nxp.com>
>> >> ---
>> >> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
>> >> 1 files changed, 18 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
>> >> aa1d0ae..542765a 100644
>> >> --- a/drivers/dma/fsl-qdma.c
>> >> +++ b/drivers/dma/fsl-qdma.c
>> >> @@ -113,6 +113,7 @@
>> >> /* Field definition for Descriptor offset */
>> >> #define QDMA_CCDF_STATUS 20
>> >> #define QDMA_CCDF_OFFSET 20
>> >> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
>> >>
>> >> /* Field definition for safe loop count*/
>> >> #define FSL_QDMA_HALT_COUNT 1500
>> >> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
>> >>
>> >> };
>> >>
>> >> +static inline void
>> >> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
>> >> + sddf->data = QDMA_SDDF_CMD(val); }
>> >> +
>> >> static inline u64
>> >> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
>> >-341,6
>> >> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
>> >> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
>> >*fsl_comp,
>> >> dma_addr_t dst,
>dma_addr_t
>> >src,
>> >> u32 len) {
>> >> + u32 cmd;
>> >> struct fsl_qdma_format *sdf, *ddf;
>> >> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src,
>> >> *csgf_dest;
>> >>
>> >> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >> fsl_qdma_comp *fsl_comp,
>> >>
>> >> memset(fsl_comp->virt_addr, 0,
>> >FSL_QDMA_COMMAND_BUFFER_SIZE);
>> >> memset(fsl_comp->desc_virt_addr, 0,
>> >> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
>> >> +
>> >> /* Head Command Descriptor(Frame Descriptor) */
>> >> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
>> >> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
>> >> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >fsl_qdma_comp *fsl_comp,
>> >> /* This entry is the last entry. */
>> >> qdma_csgf_set_f(csgf_dest, len);
>> >> /* Descriptor Buffer */
>> >> - sdf->data =
>> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> - ddf->data =
>> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> - ddf->data |=
>> >> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> + qdma_sddf_set_cmd(sdf, cmd);
>> >> +
>> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> + qdma_sddf_set_cmd(ddf, cmd);
>> >> }
>> >>
>> >> /*
>> >> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
>> >> irq, void *dev_id)
>> >>
>> >> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>> >>
>> >> - if (intr) {
>> >> + if (intr)
>> >> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
>> >error!\n");
>> >> - return IRQ_NONE;
>> >> - }
>> >
>> >this seems unrelated can you explain?
>> >
>> [Peng Ma] This is an added improvement. When an error occurs we should
>clean the error reg then to return.
>> I forgot to explain it on comments. Should I add this changed to the
>comments?
>
>Yes and you should make it a separate patch. A patch should do only 1 thing!
>
[Peng Ma] OK, Got it, thanks.
Best Regards,
Peng
>--
>~Vinod
^ permalink raw reply
* RE: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination descriptior format
From: Peng Ma @ 2019-04-29 6:28 UTC (permalink / raw)
To: Vinod Koul
Cc: dan.j.williams@intel.com, Leo Li, dmaengine@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190429051554.GD3845@vkoul-mobl.Dlink>
>-----Original Message-----
>From: Vinod Koul <vkoul@kernel.org>
>Sent: 2019年4月29日 13:16
>To: Peng Ma <peng.ma@nxp.com>
>Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>source/destination descriptior format
>
>Caution: EXT Email
>
>On 28-04-19, 02:00, Peng Ma wrote:
>> Hi Vinod,
>>
>> Thanks your comments.
>> Please see my comments inline.
>>
>> Best Regards,
>> Peng
>>
>> >-----Original Message-----
>> >From: Vinod Koul <vkoul@kernel.org>
>> >Sent: 2019年4月26日 19:51
>> >To: Peng Ma <peng.ma@nxp.com>
>> >Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
>> >dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
>> >Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>> >source/destination descriptior format
>> >
>> >Caution: EXT Email
>> >
>> >On 19-04-19, 08:46, Peng Ma wrote:
>> >> CMD of Source/Destination descriptior format should be lower of
>> >
>> >s/descriptior/descriptor
>> >
>> [Peng Ma] Got it.
>> >> struct fsl_qdma_engine number data address.
>> >>
>> >> Signed-off-by: Peng Ma <peng.ma@nxp.com>
>> >> ---
>> >> drivers/dma/fsl-qdma.c | 29 ++++++++++++++++++-----------
>> >> 1 files changed, 18 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
>> >> aa1d0ae..542765a 100644
>> >> --- a/drivers/dma/fsl-qdma.c
>> >> +++ b/drivers/dma/fsl-qdma.c
>> >> @@ -113,6 +113,7 @@
>> >> /* Field definition for Descriptor offset */
>> >> #define QDMA_CCDF_STATUS 20
>> >> #define QDMA_CCDF_OFFSET 20
>> >> +#define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
>> >>
>> >> /* Field definition for safe loop count*/
>> >> #define FSL_QDMA_HALT_COUNT 1500
>> >> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
>> >>
>> >> };
>> >>
>> >> +static inline void
>> >> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
>> >> + sddf->data = QDMA_SDDF_CMD(val); }
>> >> +
>> >> static inline u64
>> >> qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) { @@
>> >-341,6
>> >> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
>> >> *chan) static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
>> >*fsl_comp,
>> >> dma_addr_t dst,
>dma_addr_t
>> >src,
>> >> u32 len) {
>> >> + u32 cmd;
>> >> struct fsl_qdma_format *sdf, *ddf;
>> >> struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src,
>> >> *csgf_dest;
>> >>
>> >> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >> fsl_qdma_comp *fsl_comp,
>> >>
>> >> memset(fsl_comp->virt_addr, 0,
>> >FSL_QDMA_COMMAND_BUFFER_SIZE);
>> >> memset(fsl_comp->desc_virt_addr, 0,
>> >> FSL_QDMA_DESCRIPTOR_BUFFER_SIZE);
>> >> +
>> >> /* Head Command Descriptor(Frame Descriptor) */
>> >> qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16);
>> >> qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); @@
>> >> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >fsl_qdma_comp *fsl_comp,
>> >> /* This entry is the last entry. */
>> >> qdma_csgf_set_f(csgf_dest, len);
>> >> /* Descriptor Buffer */
>> >> - sdf->data =
>> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> - ddf->data =
>> >> - cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> - FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> - ddf->data |=
>> >> - cpu_to_le64(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> + qdma_sddf_set_cmd(sdf, cmd);
>> >> +
>> >> + cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> + FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> + cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> + qdma_sddf_set_cmd(ddf, cmd);
>> >> }
>> >>
>> >> /*
>> >> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
>> >> irq, void *dev_id)
>> >>
>> >> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>> >>
>> >> - if (intr) {
>> >> + if (intr)
>> >> dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
>> >error!\n");
>> >> - return IRQ_NONE;
>> >> - }
>> >
>> >this seems unrelated can you explain?
>> >
>> [Peng Ma] This is an added improvement. When an error occurs we should
>clean the error reg then to return.
>> I forgot to explain it on comments. Should I add this changed to the
>comments?
>
>Yes and you should make it a separate patch. A patch should do only 1 thing!
>
[Peng Ma] OK, Got it, thanks.
Best Regards,
Peng
>--
>~Vinod
^ permalink raw reply
* [1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Vinod Koul @ 2019-04-29 11:35 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
On 15-04-19, 20:14, Baolin Wang wrote:
> We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> has been submitted, that will crash the kernel when getting the engine status.
No that is wrong, status is for descriptor and not engine!
> In this case, since the descriptor has been submitted, which means the pointer
> 'schan->cur_desc' will point to the current descriptor, then we can use
> 'schan->cur_desc' to get the engine status to avoid this issue.
Nope, since the descriptor is completed, you return with residue as 0
and DMA_COMPLETE status!
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 48431e2..e29342a 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> else
> pos = 0;
> } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> + struct sprd_dma_desc *sdesc = schan->cur_desc;
>
> if (sdesc->dir == DMA_DEV_TO_MEM)
> pos = sprd_dma_get_dst_addr(schan);
> --
> 1.7.9.5
^ permalink raw reply
* Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Vinod Koul @ 2019-04-29 11:35 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
In-Reply-To: <2eecd528e85377f03e6fbc5e7d6544b9c9f59cb1.1555330115.git.baolin.wang@linaro.org>
On 15-04-19, 20:14, Baolin Wang wrote:
> We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> has been submitted, that will crash the kernel when getting the engine status.
No that is wrong, status is for descriptor and not engine!
> In this case, since the descriptor has been submitted, which means the pointer
> 'schan->cur_desc' will point to the current descriptor, then we can use
> 'schan->cur_desc' to get the engine status to avoid this issue.
Nope, since the descriptor is completed, you return with residue as 0
and DMA_COMPLETE status!
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 48431e2..e29342a 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> else
> pos = 0;
> } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> + struct sprd_dma_desc *sdesc = schan->cur_desc;
>
> if (sdesc->dir == DMA_DEV_TO_MEM)
> pos = sprd_dma_get_dst_addr(schan);
> --
> 1.7.9.5
--
~Vinod
^ permalink raw reply
* [1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Baolin Wang @ 2019-04-29 11:49 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
Hi Vinod,
On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > has been submitted, that will crash the kernel when getting the engine status.
>
> No that is wrong, status is for descriptor and not engine!
Sure, will fix the commit message.
>
> > In this case, since the descriptor has been submitted, which means the pointer
> > 'schan->cur_desc' will point to the current descriptor, then we can use
> > 'schan->cur_desc' to get the engine status to avoid this issue.
>
> Nope, since the descriptor is completed, you return with residue as 0
> and DMA_COMPLETE status!
No, the descriptor is not completed now. If it is completed, we will
return 0 with DMA_COMPLETE status. But now the descriptor is on
progress, we should get the descriptor to return current residue.
Sorry for confusing description.
>
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 48431e2..e29342a 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> > else
> > pos = 0;
> > } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > + struct sprd_dma_desc *sdesc = schan->cur_desc;
> >
> > if (sdesc->dir == DMA_DEV_TO_MEM)
> > pos = sprd_dma_get_dst_addr(schan);
> > --
> > 1.7.9.5
>
> --
> ~Vinod
^ permalink raw reply
* Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Baolin Wang @ 2019-04-29 11:49 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190429113555.GI3845@vkoul-mobl.Dlink>
Hi Vinod,
On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > has been submitted, that will crash the kernel when getting the engine status.
>
> No that is wrong, status is for descriptor and not engine!
Sure, will fix the commit message.
>
> > In this case, since the descriptor has been submitted, which means the pointer
> > 'schan->cur_desc' will point to the current descriptor, then we can use
> > 'schan->cur_desc' to get the engine status to avoid this issue.
>
> Nope, since the descriptor is completed, you return with residue as 0
> and DMA_COMPLETE status!
No, the descriptor is not completed now. If it is completed, we will
return 0 with DMA_COMPLETE status. But now the descriptor is on
progress, we should get the descriptor to return current residue.
Sorry for confusing description.
>
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 48431e2..e29342a 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> > else
> > pos = 0;
> > } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > + struct sprd_dma_desc *sdesc = schan->cur_desc;
> >
> > if (sdesc->dir == DMA_DEV_TO_MEM)
> > pos = sprd_dma_get_dst_addr(schan);
> > --
> > 1.7.9.5
>
> --
> ~Vinod
--
Baolin Wang
Best Regards
^ permalink raw reply
* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-29 11:57 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
On 15-04-19, 20:14, Baolin Wang wrote:
> From: Eric Long <eric.long@unisoc.com>
>
> Since we can support multiple DMA engine controllers, we should add
> device validation in filter function to check if the correct controller
> to be requested.
>
> Signed-off-by: Eric Long <eric.long@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 0f92e60..9f99d4b 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> {
> struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> + struct of_phandle_args *dma_spec =
> + container_of(param, struct of_phandle_args, args[0]);
> u32 slave_id = *(u32 *)param;
>
> + if (chan->device->dev->of_node != dma_spec->np)
Are you not using of_dma_find_controller() that does this, so this would
be useless!
> + return false;
> +
> schan->dev_id = slave_id;
> return true;
> }
> --
> 1.7.9.5
^ permalink raw reply
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-29 11:57 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
In-Reply-To: <d77dca51a14087873627d735a17adcfde5517398.1555330115.git.baolin.wang@linaro.org>
On 15-04-19, 20:14, Baolin Wang wrote:
> From: Eric Long <eric.long@unisoc.com>
>
> Since we can support multiple DMA engine controllers, we should add
> device validation in filter function to check if the correct controller
> to be requested.
>
> Signed-off-by: Eric Long <eric.long@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 0f92e60..9f99d4b 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> {
> struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> + struct of_phandle_args *dma_spec =
> + container_of(param, struct of_phandle_args, args[0]);
> u32 slave_id = *(u32 *)param;
>
> + if (chan->device->dev->of_node != dma_spec->np)
Are you not using of_dma_find_controller() that does this, so this would
be useless!
> + return false;
> +
> schan->dev_id = slave_id;
> return true;
> }
> --
> 1.7.9.5
--
~Vinod
^ permalink raw reply
* [7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Vinod Koul @ 2019-04-29 12:01 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
On 15-04-19, 20:15, Baolin Wang wrote:
> For 2-stage transfer, some users like Audio still need transaction interrupt
> to notify when the 2-stage transfer is completed. Thus we should enable
> 2-stage transfer interrupt to support this feature.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index cc9c24d..4c18f44 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -62,6 +62,8 @@
> /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> #define SPRD_DMA_GLB_2STAGE_EN BIT(24)
> #define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
> +#define SPRD_DMA_GLB_DEST_INT BIT(22)
> +#define SPRD_DMA_GLB_SRC_INT BIT(20)
> #define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
> #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> @@ -135,6 +137,7 @@
> /* define DMA channel mode & trigger mode mask */
> #define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
> #define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
> +#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
>
> /* define the DMA transfer step type */
> #define SPRD_DMA_NONE_STEP 0
> @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> u32 dev_id;
> enum sprd_dma_chn_mode chn_mode;
> enum sprd_dma_trg_mode trg_mode;
> + enum sprd_dma_int_type int_type;
> struct sprd_dma_desc *cur_desc;
> };
>
> @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
Who configure int_type?
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> schan->linklist.virt_addr = 0;
> }
>
> - /* Set channel mode and trigger mode for 2-stage transfer */
> + /*
> + * Set channel mode, interrupt mode and trigger mode for 2-stage
> + * transfer.
> + */
> schan->chn_mode =
> (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> schan->trg_mode =
> (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> + schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
>
> sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> if (!sdesc)
> --
> 1.7.9.5
^ 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