* [4/8,v4] dma: k3dma: Delete axi_config
From: John Stultz @ 2019-01-16 17:10 UTC (permalink / raw)
To: lkml
Cc: Li Yu, Dan Williams, Vinod Koul, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam, dmaengine, Guodong Xu,
John Stultz
From: Li Yu <liyu65@hisilicon.com>
Axi_config controls whether DMA resources can be accessed in non-secure
mode, such as linux kernel. The register should be set by the bootloader
stage and depends on the device.
Thus, this patch removes axi_config from k3dma driver.
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Tanglei Han <hantanglei@huawei.com>
Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
Cc: Ryan Grachek <ryan@edited.us>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: dmaengine@vger.kernel.org
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Li Yu <liyu65@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
[jstultz: Minor tweaks to commit message]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
drivers/dma/k3dma.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index df61406..b2060bf 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -52,8 +52,6 @@
#define CX_SRC 0x814
#define CX_DST 0x818
#define CX_CFG 0x81c
-#define AXI_CFG 0x820
-#define AXI_CFG_DEFAULT 0x201201
#define CX_LLI_CHAIN_EN 0x2
#define CX_CFG_EN 0x1
@@ -168,7 +166,6 @@ static void k3_dma_set_desc(struct k3_dma_phy *phy, struct k3_desc_hw *hw)
writel_relaxed(hw->count, phy->base + CX_CNT0);
writel_relaxed(hw->saddr, phy->base + CX_SRC);
writel_relaxed(hw->daddr, phy->base + CX_DST);
- writel_relaxed(AXI_CFG_DEFAULT, phy->base + AXI_CFG);
writel_relaxed(hw->config, phy->base + CX_CFG);
}
^ permalink raw reply related
* [3/8,v4] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: John Stultz @ 2019-01-16 17:10 UTC (permalink / raw)
To: lkml
Cc: Youlin Wang, Dan Williams, Vinod Koul, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam, dmaengine, Tanglei Han,
John Stultz
From: Youlin Wang <wwx575822@notesmail.huawei.com>
On the hi3660 hardware there are two (at least) DMA controllers,
the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The
two blocks are similar, but have some slight differences. This
resulted in the vendor implementing two separate drivers, which
after review, they have been able to condense and re-use the
existing k3dma driver.
Thus, this patch adds support for the new "hisi-pcm-asp-dma-1.0"
compatible string in the binding.
One difference with the DMA-A controller, is that it does not
need to initialize a clock. So we skip this by adding and using
soc data flags.
After above this driver will support both k3 and hisi_asp dma
hardware.
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
Cc: Ryan Grachek <ryan@edited.us>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: dmaengine@vger.kernel.org
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com>
Signed-off-by: Tanglei Han <hantanglei@huawei.com>
[jstultz: Reworked to use of_match_data, commit msg improvements]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Reworked to use of_match_data
v3:
* Further rework of the commit message
---
drivers/dma/k3dma.c | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index fdec2b6..df61406 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -116,6 +116,13 @@ struct k3_dma_dev {
unsigned int irq;
};
+
+#define K3_FLAG_NOCLK (1<<0)
+struct k3dma_soc_data {
+ unsigned long flags;
+};
+
+
#define to_k3_dma(dmadev) container_of(dmadev, struct k3_dma_dev, slave)
static int k3_dma_config_write(struct dma_chan *chan,
@@ -790,8 +797,21 @@ static int k3_dma_transfer_resume(struct dma_chan *chan)
return 0;
}
+static const struct k3dma_soc_data k3_v1_dma_data = {
+ .flags = 0,
+};
+
+static const struct k3dma_soc_data asp_v1_dma_data = {
+ .flags = K3_FLAG_NOCLK,
+};
+
static const struct of_device_id k3_pdma_dt_ids[] = {
- { .compatible = "hisilicon,k3-dma-1.0", },
+ { .compatible = "hisilicon,k3-dma-1.0",
+ .data = &k3_v1_dma_data
+ },
+ { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0",
+ .data = &asp_v1_dma_data
+ },
{}
};
MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids);
@@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
static int k3_dma_probe(struct platform_device *op)
{
+ const struct k3dma_soc_data *soc_data;
struct k3_dma_dev *d;
const struct of_device_id *of_id;
struct resource *iores;
@@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op)
if (!d)
return -ENOMEM;
+ soc_data = device_get_match_data(&op->dev);
+ if (!soc_data)
+ return -EINVAL;
+
d->base = devm_ioremap_resource(&op->dev, iores);
if (IS_ERR(d->base))
return PTR_ERR(d->base);
@@ -835,10 +860,12 @@ static int k3_dma_probe(struct platform_device *op)
"dma-requests", &d->dma_requests);
}
- d->clk = devm_clk_get(&op->dev, NULL);
- if (IS_ERR(d->clk)) {
- dev_err(&op->dev, "no dma clk\n");
- return PTR_ERR(d->clk);
+ if (!(soc_data->flags & K3_FLAG_NOCLK)) {
+ d->clk = devm_clk_get(&op->dev, NULL);
+ if (IS_ERR(d->clk)) {
+ dev_err(&op->dev, "no dma clk\n");
+ return PTR_ERR(d->clk);
+ }
}
irq = platform_get_irq(op, 0);
^ permalink raw reply related
* [2/8,v4] Documentation: bindings: dma: Add binding for dma-channel-mask
From: John Stultz @ 2019-01-16 17:10 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Vinod Koul, Rob Herring, Mark Rutland, Tanglei Han,
Zhuangluan Su, Ryan Grachek, Manivannan Sadhasivam, dmaengine,
devicetree
Some dma channels can be reserved for secure mode or other
hardware on the SoC, so provide a binding for a bitmask
listing the available channels for the kernel to use.
This follows the pre-existing bcm,dma-channel-mask binding.
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Tanglei Han <hantanglei@huawei.com>
Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
Cc: Ryan Grachek <ryan@edited.us>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: dmaengine@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3: Renamed to hisi-dma-avail-chan
v4: Reworked to generic dma-channel-mask
---
Documentation/devicetree/bindings/dma/dma.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
index 6312fb0..eeb4e4d 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -16,6 +16,9 @@ Optional properties:
- dma-channels: Number of DMA channels supported by the controller.
- dma-requests: Number of DMA request signals supported by the
controller.
+- dma-channel-mask: Bitmask of available DMA channels in ascending order
+ that are not reserved by firmware and are available to
+ the kernel. i.e. first channel corresponds to LSB.
Example:
@@ -29,6 +32,7 @@ Example:
#dma-cells = <1>;
dma-channels = <32>;
dma-requests = <127>;
+ dma-channel-mask = <0xfffe>
};
* DMA router
^ permalink raw reply related
* [1/8,v4] Documentation: bindings: k3dma: Extend the k3dma driver binding to support hisi-asp
From: John Stultz @ 2019-01-16 17:10 UTC (permalink / raw)
To: lkml
Cc: Youlin Wang, Vinod Koul, Rob Herring, Mark Rutland, Zhuangluan Su,
Tanglei Han, Ryan Grachek, Manivannan Sadhasivam, dmaengine,
devicetree, John Stultz
From: Youlin Wang <wwx575822@notesmail.huawei.com>
Extend the k3dma driver binding to support hisi-asp hardware
variants.
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
Cc: Tanglei Han <hantanglei@huawei.com>
Cc: Ryan Grachek <ryan@edited.us>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: dmaengine@vger.kernel.org
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com>
Signed-off-by: Tanglei Han <hantanglei@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Simplify patch, removing extranious examples
---
Documentation/devicetree/bindings/dma/k3dma.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
index 4945aea..10a2f15 100644
--- a/Documentation/devicetree/bindings/dma/k3dma.txt
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -3,7 +3,9 @@
See dma.txt first
Required properties:
-- compatible: Should be "hisilicon,k3-dma-1.0"
+- compatible: Must be one of
+- "hisilicon,k3-dma-1.0"
+- "hisilicon,hisi-pcm-asp-dma-1.0"
- reg: Should contain DMA registers location and length.
- interrupts: Should contain one interrupt shared by all channel
- #dma-cells: see dma.txt, should be 1, para number
^ permalink raw reply related
* [RFC,v3,2/7] dmaengine: Add Synopsys eDMA IP version 0 support
From: Gustavo Pimentel @ 2019-01-16 14:02 UTC (permalink / raw)
To: Jose Abreu, Gustavo Pimentel, linux-pci@vger.kernel.org,
dmaengine@vger.kernel.org
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
Hi Jose,
On 16/01/2019 10:33, Jose Abreu wrote:
> Hi Gustavo,
>
> On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
>> Add support for the eDMA IP version 0 driver for both register maps (legacy
>> and unroll).
>>
>> The legacy register mapping was the initial implementation, which consisted
>> in having all registers belonging to channels multiplexed, which could be
>> change anytime (which could led a race-condition) by view port register
>> (access to only one channel available each time).
>>
>> This register mapping is not very effective and efficient in a multithread
>> environment, which has led to the development of unroll registers mapping,
>> which consists of having all channels registers accessible any time by
>> spreading all channels registers by an offset between them.
>>
>> This version supports a maximum of 16 independent channels (8 write +
>> 8 read), which can run simultaneously.
>>
>> Implements a scatter-gather transfer through a linked list, where the size
>> of linked list depends on the allocated memory divided equally among all
>> channels.
>>
>> Each linked list descriptor can transfer from 1 byte to 4 Gbytes and is
>> alignmented to DWORD.
>>
>> Both SAR (Source Address Register) and DAR (Destination Address Register)
>> are alignmented to byte.
>>
>> Changes:
>> RFC v1->RFC v2:
>> - Replace comments // (C99 style) by /**/
>> - Replace magic numbers by defines
>> - Replace boolean return from ternary operation by a double negation
>> operation
>> - Replace QWORD_HI/QWORD_LO macros by upper_32_bits()/lower_32_bits()
>> - Fix the headers of the .c and .h files according to the most recent
>> convention
>> - Fix errors and checks pointed out by checkpatch with --strict option
>> - Replace patch small description tag from dma by dmaengine
>> - Refactor code to replace atomic_t by u32 variable type
>> RFC v2->RFC v3:
>> - Code rewrite to use FIELD_PREP() and FIELD_GET()
>> - Add define to magic numbers
>> - Fix minor bugs
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Russell King <rmk+kernel@armlinux.org.uk>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>> Cc: Luis Oliveira <lolivei@synopsys.com>
>> Cc: Vitor Soares <vitor.soares@synopsys.com>
>> Cc: Nelson Costa <nelson.costa@synopsys.com>
>> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
>
>> +void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>> +{
>> + struct dw_edma_chan *chan = chunk->chan;
>> + struct dw_edma *dw = chan->chip->dw;
>> + u32 tmp;
>> + u64 llp;
>> +
>> + dw_edma_v0_core_write_chunk(chunk);
>> +
>> + if (first) {
>> + /* Enable engine */
>> + SET_RW(dw, chan->dir, engine_en, BIT(0));
>> + /* Interrupt unmask - done, abort */
>> + tmp = GET_RW(dw, chan->dir, int_mask);
>> + tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
>> + tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
>> + SET_RW(dw, chan->dir, int_mask, tmp);
>> + /* Linked list error */
>> + tmp = GET_RW(dw, chan->dir, linked_list_err_en);
>> + tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
>> + SET_RW(dw, chan->dir, linked_list_err_en, tmp);
>> + /* Channel control */
>> + SET_CH(dw, chan->dir, chan->id, ch_control1,
>> + (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
>> + /* Linked list - low, high */
>> + llp = cpu_to_le64(chunk->ll_region.paddr);
>> + SET_CH(dw, chan->dir, chan->id, llp_low, lower_32_bits(llp));
>> + SET_CH(dw, chan->dir, chan->id, llp_high, upper_32_bits(llp));
>> + }
>> + /* Doorbell */
>
> Not sure if DMA subsystem does this but maybe you need some kind
> of barrier to ensure everything is coherent before granting
> control to eDMA ?
Each readl and writel inside of helper macros (SET_RW/GETRW and SET_CH/GET_CH)
has a memory barrier, that grants that the coherency, however this is platform
dependent.
>
>> + SET_RW(dw, chan->dir, doorbell,
>> + FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
>> +}
>> +
Thanks!
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Gustavo Pimentel @ 2019-01-16 11:56 UTC (permalink / raw)
To: Jose Abreu, Gustavo Pimentel, linux-pci@vger.kernel.org,
dmaengine@vger.kernel.org
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Luis de Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
Hi Jose,
On 16/01/2019 10:45, Jose Abreu wrote:
> Hi Gustavo,
>
> On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
>> Add Synopsys eDMA IP test and sample driver to be use for testing
>> purposes and also as a reference for any developer who needs to
>> implement and use Synopsys eDMA.
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA_TEST option in kernel
>> configuration, however it requires and selects automatically DW_EDMA
>> option too.
>>
>> Changes:
>> RFC v1->RFC v2:
>> - No changes
>> RFC v2->RFC v3:
>> - Add test module
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Russell King <rmk+kernel@armlinux.org.uk>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>> Cc: Luis Oliveira <lolivei@synopsys.com>
>> Cc: Vitor Soares <vitor.soares@synopsys.com>
>> Cc: Nelson Costa <nelson.costa@synopsys.com>
>> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
>
>> +static int dw_edma_test_add_channel(struct dw_edma_test_info *info,
>> + struct dma_chan *chan,
>> + u32 channel)
>> +{
>> + struct dw_edma_test_params *params = &info->params;
>> + struct dw_edma_test_thread *thread;
>> + struct dw_edma_test_chan *tchan;
>> +
>> + tchan = kvmalloc(sizeof(*tchan), GFP_KERNEL);
>> + if (!tchan)
>> + return -ENOMEM;
>> +
>> + tchan->chan = chan;
>> +
>> + thread = kvzalloc(sizeof(*thread), GFP_KERNEL);
>> + if (!thread) {
>> + kvfree(tchan);
>> + return -ENOMEM;
>> + }
>> +
>> + thread->info = info;
>> + thread->chan = tchan->chan;
>> + switch (channel) {
>> + case EDMA_CH_WR:
>> + thread->direction = DMA_DEV_TO_MEM;
>> + break;
>> + case EDMA_CH_RD:
>> + thread->direction = DMA_MEM_TO_DEV;
>> + break;
>> + default:
>> + kvfree(tchan);
>
> You are leaking thread here.
Yes, indeed.
>
>> + return -EPERM;
>> + }
>> + thread->test_done.wait = &thread->done_wait;
>> + init_waitqueue_head(&thread->done_wait);
>> +
>> + if (!params->repetitions)
>> + thread->task = kthread_create(dw_edma_test_sg, thread, "%s",
>> + dma_chan_name(chan));
>> + else
>> + thread->task = kthread_create(dw_edma_test_cyclic, thread, "%s",
>> + dma_chan_name(chan));
>> +
>> + if (IS_ERR(thread->task)) {
>> + pr_err("failed to create thread %s\n", dma_chan_name(chan));
>> + kvfree(tchan);
>> + kvfree(thread);
>> + return -EPERM;
>> + }
>> +
>> + tchan->thread = thread;
>> + dev_dbg(chan->device->dev, "add thread %s\n", dma_chan_name(chan));
>> + list_add_tail(&tchan->node, &info->channels);
>> +
>> + return 0;
>> +}
>> +
Thanks!
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-16 11:53 UTC (permalink / raw)
To: Jose Abreu, Gustavo Pimentel, linux-pci@vger.kernel.org,
dmaengine@vger.kernel.org
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Luis de Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
Hi Jose,
On 16/01/2019 10:21, Jose Abreu wrote:
> Hi Gustavo,
>
> On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
>> Add Synopsys eDMA IP core driver to kernel.
>>
>> This core driver, initializes and configures the eDMA IP using vma-helpers
>> functions and dma-engine subsystem.
>>
>> Also creates an abstration layer through callbacks allowing different
>> registers mappings in the future, organized in to versions.
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA option in kernel configuration,
>> however it requires and selects automatically DMA_ENGINE and
>> DMA_VIRTUAL_CHANNELS option too.
>>
>> Changes:
>> RFC v1->RFC v2:
>> - Replace comments // (C99 style) by /**/
>> - Fix the headers of the .c and .h files according to the most recent
>> convention
>> - Fix errors and checks pointed out by checkpatch with --strict option
>> - Replace patch small description tag from dma by dmaengine
>> - Change some dev_info() into dev_dbg()
>> - Remove unnecessary zero initialization after kzalloc
>> - Remove direction validation on config() API, since the direction
>> parameter is deprecated
>> - Refactor code to replace atomic_t by u32 variable type
>> - Replace start_transfer() name by dw_edma_start_transfer()
>> - Add spinlock to dw_edma_device_prep_slave_sg()
>> - Add spinlock to dw_edma_free_chunk()
>> - Simplify switch case into if on dw_edma_device_pause(),
>> dw_edma_device_resume() and dw_edma_device_terminate_all()
>> RFC v2->RFC v3:
>> - Add driver parameter to disable msix feature
>> - Fix printk variable of phys_addr_t type
>> - Fix printk variable of __iomem type
>> - Fix printk variable of size_t type
>> - Add comments or improve existing ones
>> - Add possibility to work with multiple IRQs feature
>> - Fix source and destination addresses
>> - Add define to magic numbers
>> - Add DMA cyclic transfer feature
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Russell King <rmk+kernel@armlinux.org.uk>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>> Cc: Luis Oliveira <lolivei@synopsys.com>
>> Cc: Vitor Soares <vitor.soares@synopsys.com>
>> Cc: Nelson Costa <nelson.costa@synopsys.com>
>> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
>
>
>> +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
>> +{
>> + struct dw_edma_chan *chan = desc->chan;
>> + struct dw_edma *dw = chan->chip->dw;
>> + struct dw_edma_chunk *chunk;
>> +
>> + chunk = kvzalloc(sizeof(*chunk), GFP_NOWAIT);
>> + if (unlikely(!chunk))
>> + return NULL;
>> +
>> + INIT_LIST_HEAD(&chunk->list);
>> + chunk->chan = chan;
>> + chunk->cb = !(desc->chunks_alloc % 2);
>> + chunk->ll_region.paddr = dw->ll_region.paddr + chan->ll_off;
>> + chunk->ll_region.vaddr = dw->ll_region.vaddr + chan->ll_off;
>> +
>> + if (desc->chunk) {
>> + /* Create and add new element into the linked list */
>> + desc->chunks_alloc++;
>> + dev_dbg(chan2dev(chan), "alloc new chunk element (%d)\n",
>> + desc->chunks_alloc);
>> + list_add_tail(&chunk->list, &desc->chunk->list);
>> + dw_edma_alloc_burst(chunk);
>
> No return check ?
Nice catch! I'll do a return check.
>
>> + } else {
>> + /* List head */
>> + chunk->burst = NULL;
>> + desc->chunks_alloc = 0;
>> + desc->chunk = chunk;
>> + dev_dbg(chan2dev(chan), "alloc new chunk head\n");
>> + }
>> +
>> + return chunk;
>> +}
>> +
>> +static struct dw_edma_desc *dw_edma_alloc_desc(struct dw_edma_chan *chan)
>> +{
>> + struct dw_edma_desc *desc;
>> +
>> + dev_dbg(chan2dev(chan), "alloc new descriptor\n");
>> +
>> + desc = kvzalloc(sizeof(*desc), GFP_NOWAIT);
>> + if (unlikely(!desc))
>> + return NULL;
>> +
>> + desc->chan = chan;
>> + dw_edma_alloc_chunk(desc);
>
> No return check ?
Ditto.
>
>> +
>> + return desc;
>> +}
>> +
>
>> +static void dw_edma_start_transfer(struct dw_edma_chan *chan)
>> +{
>> + struct virt_dma_desc *vd;
>> + struct dw_edma_desc *desc;
>> + struct dw_edma_chunk *child;
>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
>
> Reverse tree order here and in remaining functions ...
Ok, I'll do that.
>
>> +
>> + vd = vchan_next_desc(&chan->vc);
>> + if (!vd)
>> + return;
>> +
>> + desc = vd2dw_edma_desc(vd);
>> + if (!desc)
>> + return;
>> +
>> + child = list_first_entry_or_null(&desc->chunk->list,
>> + struct dw_edma_chunk, list);
>> + if (!child)
>> + return;
>> +
>> + ops->start(child, !desc->xfer_sz);
>> + desc->xfer_sz += child->ll_region.sz;
>> + dev_dbg(chan2dev(chan), "transfer of %u bytes started\n",
>> + child->ll_region.sz);
>> +
>> + dw_edma_free_burst(child);
>> + if (child->bursts_alloc)
>> + dev_dbg(chan2dev(chan), "%u bursts still allocated\n",
>> + child->bursts_alloc);
>> + list_del(&child->list);
>> + kvfree(child);
>> + desc->chunks_alloc--;
>> +}
>> +
>
>> +int dw_edma_probe(struct dw_edma_chip *chip)
>> +{
>> + struct dw_edma *dw = chip->dw;
>> + struct device *dev = chip->dev;
>> + const struct dw_edma_core_ops *ops;
>> + size_t ll_chunk = dw->ll_region.sz;
>> + size_t dt_chunk = dw->dt_region.sz;
>> + u32 ch_tot;
>> + int i, j, err;
>> +
>> + raw_spin_lock_init(&dw->lock);
>
> Why raw ?
Marc Zyngier told me some time ago raw spin locks it would preferable, since it
doesn't bring any additional effect for the mainline kernel, but will make it
work correctly if you use the RT patches.
>
>> +
>> + /* Callback operation selection accordingly to eDMA version */
>> + switch (dw->version) {
>> + default:
>> + dev_err(dev, "unsupported version\n");
>> + return -EPERM;
>> + }
>> +
>> + pm_runtime_get_sync(dev);
>
> Why do you need to increase usage count at probe ? And shouldn't
> this be after pm_runtime_enable() ?
It shouldn't be there. I'll remove it.
>
>> +
>> + /* Find out how many write channels are supported by hardware */
>> + dw->wr_ch_cnt = ops->ch_count(dw, EDMA_DIR_WRITE);
>> + if (!dw->wr_ch_cnt) {
>> + dev_err(dev, "invalid number of write channels(0)\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Find out how many read channels are supported by hardware */
>> + dw->rd_ch_cnt = ops->ch_count(dw, EDMA_DIR_READ);
>> + if (!dw->rd_ch_cnt) {
>> + dev_err(dev, "invalid number of read channels(0)\n");
>> + return -EINVAL;
>> + }
>> +
>> + dev_dbg(dev, "Channels:\twrite=%d, read=%d\n",
>> + dw->wr_ch_cnt, dw->rd_ch_cnt);
>> +
>> + ch_tot = dw->wr_ch_cnt + dw->rd_ch_cnt;
>> +
>> + /* Allocate channels */
>> + dw->chan = devm_kcalloc(dev, ch_tot, sizeof(*dw->chan), GFP_KERNEL);
>> + if (!dw->chan)
>> + return -ENOMEM;
>> +
>> + /* Calculate the linked list chunk for each channel */
>> + ll_chunk /= roundup_pow_of_two(ch_tot);
>> +
>> + /* Calculate the linked list chunk for each channel */
>> + dt_chunk /= roundup_pow_of_two(ch_tot);
>> +
>> + /* Disable eDMA, only to establish the ideal initial conditions */
>> + ops->off(dw);
>> +
>> + snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id);
>> +
>> + /* Request IRQs */
>> + if (dw->nr_irqs != 1) {
>> + dev_err(dev, "invalid number of irqs (%u)\n", dw->nr_irqs);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < dw->nr_irqs; i++) {
>> + err = devm_request_irq(dev, pci_irq_vector(to_pci_dev(dev), i),
>> + dw_edma_interrupt_all,
>> + IRQF_SHARED, dw->name, chip);
>> + if (err)
>> + return err;
>> + }
>> +
>> + /* Create write channels */
>> + INIT_LIST_HEAD(&dw->wr_edma.channels);
>> + for (i = 0; i < dw->wr_ch_cnt; i++) {
>> + struct dw_edma_chan *chan = &dw->chan[i];
>> + struct dw_edma_region *dt_region;
>> +
>> + dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL);
>> + if (!dt_region)
>> + return -ENOMEM;
>> +
>> + chan->vc.chan.private = dt_region;
>> +
>> + chan->chip = chip;
>> + chan->id = i;
>> + chan->dir = EDMA_DIR_WRITE;
>> + chan->configured = false;
>> + chan->request = EDMA_REQ_NONE;
>> + chan->status = EDMA_ST_IDLE;
>> +
>> + chan->ll_off = (ll_chunk * i);
>> + chan->ll_max = (ll_chunk / EDMA_LL_SZ) - 1;
>> +
>> + chan->dt_off = (dt_chunk * i);
>> +
>> + dev_dbg(dev, "L. List:\tChannel write[%u] off=0x%.8lx, max_cnt=%u\n",
>> + i, chan->ll_off, chan->ll_max);
>> +
>> + memcpy(&chan->msi, &dw->msi[0], sizeof(chan->msi));
>> +
>> + dev_dbg(dev, "MSI:\t\tChannel write[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
>> + i, chan->msi.address_hi, chan->msi.address_lo,
>> + chan->msi.data);
>> +
>> + chan->vc.desc_free = vchan_free_desc;
>> + vchan_init(&chan->vc, &dw->wr_edma);
>> +
>> + dt_region->paddr = dw->dt_region.paddr + chan->dt_off;
>> + dt_region->vaddr = dw->dt_region.vaddr + chan->dt_off;
>> + dt_region->sz = dt_chunk;
>> +
>> + dev_dbg(dev, "Data:\tChannel write[%u] off=0x%.8lx\n",
>> + i, chan->dt_off);
>> + }
>> + dma_cap_zero(dw->wr_edma.cap_mask);
>> + dma_cap_set(DMA_SLAVE, dw->wr_edma.cap_mask);
>> + dma_cap_set(DMA_CYCLIC, dw->wr_edma.cap_mask);
>> + dw->wr_edma.directions = BIT(DMA_DEV_TO_MEM);
>> + dw->wr_edma.chancnt = dw->wr_ch_cnt;
>> +
>> + /* Create read channels */
>> + INIT_LIST_HEAD(&dw->rd_edma.channels);
>> + for (j = 0; j < dw->rd_ch_cnt; j++, i++) {
>> + struct dw_edma_chan *chan = &dw->chan[i];
>> + struct dw_edma_region *dt_region;
>> +
>> + dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL);
>> + if (!dt_region)
>> + return -ENOMEM;
>> +
>> + chan->vc.chan.private = dt_region;
>> +
>> + chan->chip = chip;
>> + chan->id = j;
>> + chan->dir = EDMA_DIR_READ;
>> + chan->configured = false;
>> + chan->request = EDMA_REQ_NONE;
>> + chan->status = EDMA_ST_IDLE;
>> +
>> + chan->ll_off = (ll_chunk * i);
>> + chan->ll_max = (ll_chunk / EDMA_LL_SZ) - 1;
>> +
>> + chan->dt_off = (dt_chunk * i);
>> +
>> + dev_dbg(dev, "L. List:\tChannel read[%u] off=0x%.8lx, max_cnt=%u\n",
>> + j, chan->ll_off, chan->ll_max);
>> +
>> + memcpy(&chan->msi, &dw->msi[0], sizeof(chan->msi));
>> +
>> + dev_dbg(dev, "MSI:\t\tChannel read[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
>> + j, chan->msi.address_hi, chan->msi.address_lo,
>> + chan->msi.data);
>> +
>> + chan->vc.desc_free = vchan_free_desc;
>> + vchan_init(&chan->vc, &dw->rd_edma);
>> +
>> + dt_region->paddr = dw->dt_region.paddr + chan->dt_off;
>> + dt_region->vaddr = dw->dt_region.vaddr + chan->dt_off;
>> + dt_region->sz = dt_chunk;
>> +
>> + dev_dbg(dev, "Data:\tChannel read[%u] off=0x%.8lx\n",
>> + i, chan->dt_off);
>> + }
>> + dma_cap_zero(dw->rd_edma.cap_mask);
>> + dma_cap_set(DMA_SLAVE, dw->rd_edma.cap_mask);
>> + dma_cap_set(DMA_CYCLIC, dw->rd_edma.cap_mask);
>> + dw->rd_edma.directions = BIT(DMA_MEM_TO_DEV);
>> + dw->rd_edma.chancnt = dw->rd_ch_cnt;
>> +
>> + /* Set DMA channels capabilities */
>> + SET_BOTH_CH(src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
>> + SET_BOTH_CH(dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
>> + SET_BOTH_CH(residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);
>> +
>> + SET_BOTH_CH(dev, dev);
>> +
>> + SET_BOTH_CH(device_alloc_chan_resources, dw_edma_alloc_chan_resources);
>> + SET_BOTH_CH(device_free_chan_resources, dw_edma_free_chan_resources);
>> +
>> + SET_BOTH_CH(device_config, dw_edma_device_config);
>> + SET_BOTH_CH(device_pause, dw_edma_device_pause);
>> + SET_BOTH_CH(device_resume, dw_edma_device_resume);
>> + SET_BOTH_CH(device_terminate_all, dw_edma_device_terminate_all);
>> + SET_BOTH_CH(device_issue_pending, dw_edma_device_issue_pending);
>> + SET_BOTH_CH(device_tx_status, dw_edma_device_tx_status);
>> + SET_BOTH_CH(device_prep_slave_sg, dw_edma_device_prep_slave_sg);
>> + SET_BOTH_CH(device_prep_dma_cyclic, dw_edma_device_prep_dma_cyclic);
>> +
>> + /* Power management */
>> + pm_runtime_enable(dev);
>> +
>> + /* Register DMA device */
>> + err = dma_async_device_register(&dw->wr_edma);
>> + if (err)
>> + goto err_pm_disable;
>> +
>> + err = dma_async_device_register(&dw->rd_edma);
>> + if (err)
>> + goto err_pm_disable;
>> +
>> + /* Turn debugfs on */
>> + err = ops->debugfs_on(chip);
>> + if (err) {
>> + dev_err(dev, "unable to create debugfs structure\n");
>> + goto err_pm_disable;
>> + }
>> +
>> + dev_info(dev, "DesignWare eDMA controller driver loaded completely\n");
>> +
>> + return 0;
>> +
>> +err_pm_disable:
>> + pm_runtime_disable(dev);
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(dw_edma_probe);
>> +
>> +int dw_edma_remove(struct dw_edma_chip *chip)
>> +{
>> + struct dw_edma *dw = chip->dw;
>> + struct device *dev = chip->dev;
>> + const struct dw_edma_core_ops *ops = dw->ops;
>> + struct dw_edma_chan *chan, *_chan;
>> + int i;
>> +
>> + /* Disable eDMA */
>> + if (ops)
>> + ops->off(dw);
>> +
>> + /* Free irqs */
>
> But devm will automatically free it, no ?
Yes, it shouldn't be there. I'll remove it.
>
>> + for (i = 0; i < dw->nr_irqs; i++) {
>> + if (pci_irq_vector(to_pci_dev(dev), i) < 0)
>> + continue;
>> +
>> + devm_free_irq(dev, pci_irq_vector(to_pci_dev(dev), i), chip);
>> + }
>> +
>> + /* Power management */
>> + pm_runtime_disable(dev);
>> +
>> + list_for_each_entry_safe(chan, _chan, &dw->wr_edma.channels,
>> + vc.chan.device_node) {
>> + list_del(&chan->vc.chan.device_node);
>> + tasklet_kill(&chan->vc.task);
>> + }
>> +
>> + list_for_each_entry_safe(chan, _chan, &dw->rd_edma.channels,
>> + vc.chan.device_node) {
>> + list_del(&chan->vc.chan.device_node);
>> + tasklet_kill(&chan->vc.task);
>> + }
>> +
>> + /* Deregister eDMA device */
>> + dma_async_device_unregister(&dw->wr_edma);
>> + dma_async_device_unregister(&dw->rd_edma);
>> +
>> + /* Turn debugfs off */
>> + if (ops)
>> + ops->debugfs_off();
>> +
>> + dev_info(dev, "DesignWare eDMA controller driver unloaded complete\n");
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dw_edma_remove);
Thanks!
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Jose Abreu @ 2019-01-16 10:45 UTC (permalink / raw)
To: Gustavo Pimentel, linux-pci, dmaengine
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Jose Abreu,
Luis Oliveira, Vitor Soares, Nelson Costa, Pedro Sousa
Hi Gustavo,
On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
> Add Synopsys eDMA IP test and sample driver to be use for testing
> purposes and also as a reference for any developer who needs to
> implement and use Synopsys eDMA.
>
> This driver can be compile as built-in or external module in kernel.
>
> To enable this driver just select DW_EDMA_TEST option in kernel
> configuration, however it requires and selects automatically DW_EDMA
> option too.
>
> Changes:
> RFC v1->RFC v2:
> - No changes
> RFC v2->RFC v3:
> - Add test module
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Jose Abreu <jose.abreu@synopsys.com>
> Cc: Luis Oliveira <lolivei@synopsys.com>
> Cc: Vitor Soares <vitor.soares@synopsys.com>
> Cc: Nelson Costa <nelson.costa@synopsys.com>
> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
> +static int dw_edma_test_add_channel(struct dw_edma_test_info *info,
> + struct dma_chan *chan,
> + u32 channel)
> +{
> + struct dw_edma_test_params *params = &info->params;
> + struct dw_edma_test_thread *thread;
> + struct dw_edma_test_chan *tchan;
> +
> + tchan = kvmalloc(sizeof(*tchan), GFP_KERNEL);
> + if (!tchan)
> + return -ENOMEM;
> +
> + tchan->chan = chan;
> +
> + thread = kvzalloc(sizeof(*thread), GFP_KERNEL);
> + if (!thread) {
> + kvfree(tchan);
> + return -ENOMEM;
> + }
> +
> + thread->info = info;
> + thread->chan = tchan->chan;
> + switch (channel) {
> + case EDMA_CH_WR:
> + thread->direction = DMA_DEV_TO_MEM;
> + break;
> + case EDMA_CH_RD:
> + thread->direction = DMA_MEM_TO_DEV;
> + break;
> + default:
> + kvfree(tchan);
You are leaking thread here.
> + return -EPERM;
> + }
> + thread->test_done.wait = &thread->done_wait;
> + init_waitqueue_head(&thread->done_wait);
> +
> + if (!params->repetitions)
> + thread->task = kthread_create(dw_edma_test_sg, thread, "%s",
> + dma_chan_name(chan));
> + else
> + thread->task = kthread_create(dw_edma_test_cyclic, thread, "%s",
> + dma_chan_name(chan));
> +
> + if (IS_ERR(thread->task)) {
> + pr_err("failed to create thread %s\n", dma_chan_name(chan));
> + kvfree(tchan);
> + kvfree(thread);
> + return -EPERM;
> + }
> +
> + tchan->thread = thread;
> + dev_dbg(chan->device->dev, "add thread %s\n", dma_chan_name(chan));
> + list_add_tail(&tchan->node, &info->channels);
> +
> + return 0;
> +}
> +
^ permalink raw reply
* [RFC,v3,2/7] dmaengine: Add Synopsys eDMA IP version 0 support
From: Jose Abreu @ 2019-01-16 10:33 UTC (permalink / raw)
To: Gustavo Pimentel, linux-pci, dmaengine
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Jose Abreu,
Luis Oliveira, Vitor Soares, Nelson Costa, Pedro Sousa
Hi Gustavo,
On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
> Add support for the eDMA IP version 0 driver for both register maps (legacy
> and unroll).
>
> The legacy register mapping was the initial implementation, which consisted
> in having all registers belonging to channels multiplexed, which could be
> change anytime (which could led a race-condition) by view port register
> (access to only one channel available each time).
>
> This register mapping is not very effective and efficient in a multithread
> environment, which has led to the development of unroll registers mapping,
> which consists of having all channels registers accessible any time by
> spreading all channels registers by an offset between them.
>
> This version supports a maximum of 16 independent channels (8 write +
> 8 read), which can run simultaneously.
>
> Implements a scatter-gather transfer through a linked list, where the size
> of linked list depends on the allocated memory divided equally among all
> channels.
>
> Each linked list descriptor can transfer from 1 byte to 4 Gbytes and is
> alignmented to DWORD.
>
> Both SAR (Source Address Register) and DAR (Destination Address Register)
> are alignmented to byte.
>
> Changes:
> RFC v1->RFC v2:
> - Replace comments // (C99 style) by /**/
> - Replace magic numbers by defines
> - Replace boolean return from ternary operation by a double negation
> operation
> - Replace QWORD_HI/QWORD_LO macros by upper_32_bits()/lower_32_bits()
> - Fix the headers of the .c and .h files according to the most recent
> convention
> - Fix errors and checks pointed out by checkpatch with --strict option
> - Replace patch small description tag from dma by dmaengine
> - Refactor code to replace atomic_t by u32 variable type
> RFC v2->RFC v3:
> - Code rewrite to use FIELD_PREP() and FIELD_GET()
> - Add define to magic numbers
> - Fix minor bugs
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Jose Abreu <jose.abreu@synopsys.com>
> Cc: Luis Oliveira <lolivei@synopsys.com>
> Cc: Vitor Soares <vitor.soares@synopsys.com>
> Cc: Nelson Costa <nelson.costa@synopsys.com>
> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
> +void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> +{
> + struct dw_edma_chan *chan = chunk->chan;
> + struct dw_edma *dw = chan->chip->dw;
> + u32 tmp;
> + u64 llp;
> +
> + dw_edma_v0_core_write_chunk(chunk);
> +
> + if (first) {
> + /* Enable engine */
> + SET_RW(dw, chan->dir, engine_en, BIT(0));
> + /* Interrupt unmask - done, abort */
> + tmp = GET_RW(dw, chan->dir, int_mask);
> + tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
> + tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> + SET_RW(dw, chan->dir, int_mask, tmp);
> + /* Linked list error */
> + tmp = GET_RW(dw, chan->dir, linked_list_err_en);
> + tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
> + SET_RW(dw, chan->dir, linked_list_err_en, tmp);
> + /* Channel control */
> + SET_CH(dw, chan->dir, chan->id, ch_control1,
> + (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
> + /* Linked list - low, high */
> + llp = cpu_to_le64(chunk->ll_region.paddr);
> + SET_CH(dw, chan->dir, chan->id, llp_low, lower_32_bits(llp));
> + SET_CH(dw, chan->dir, chan->id, llp_high, upper_32_bits(llp));
> + }
> + /* Doorbell */
Not sure if DMA subsystem does this but maybe you need some kind
of barrier to ensure everything is coherent before granting
control to eDMA ?
> + SET_RW(dw, chan->dir, doorbell,
> + FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
> +}
> +
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Jose Abreu @ 2019-01-16 10:21 UTC (permalink / raw)
To: Gustavo Pimentel, linux-pci, dmaengine
Cc: Vinod Koul, Dan Williams, Eugeniy Paltsev, Andy Shevchenko,
Russell King, Niklas Cassel, Joao Pinto, Jose Abreu,
Luis Oliveira, Vitor Soares, Nelson Costa, Pedro Sousa
Hi Gustavo,
On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
> Add Synopsys eDMA IP core driver to kernel.
>
> This core driver, initializes and configures the eDMA IP using vma-helpers
> functions and dma-engine subsystem.
>
> Also creates an abstration layer through callbacks allowing different
> registers mappings in the future, organized in to versions.
>
> This driver can be compile as built-in or external module in kernel.
>
> To enable this driver just select DW_EDMA option in kernel configuration,
> however it requires and selects automatically DMA_ENGINE and
> DMA_VIRTUAL_CHANNELS option too.
>
> Changes:
> RFC v1->RFC v2:
> - Replace comments // (C99 style) by /**/
> - Fix the headers of the .c and .h files according to the most recent
> convention
> - Fix errors and checks pointed out by checkpatch with --strict option
> - Replace patch small description tag from dma by dmaengine
> - Change some dev_info() into dev_dbg()
> - Remove unnecessary zero initialization after kzalloc
> - Remove direction validation on config() API, since the direction
> parameter is deprecated
> - Refactor code to replace atomic_t by u32 variable type
> - Replace start_transfer() name by dw_edma_start_transfer()
> - Add spinlock to dw_edma_device_prep_slave_sg()
> - Add spinlock to dw_edma_free_chunk()
> - Simplify switch case into if on dw_edma_device_pause(),
> dw_edma_device_resume() and dw_edma_device_terminate_all()
> RFC v2->RFC v3:
> - Add driver parameter to disable msix feature
> - Fix printk variable of phys_addr_t type
> - Fix printk variable of __iomem type
> - Fix printk variable of size_t type
> - Add comments or improve existing ones
> - Add possibility to work with multiple IRQs feature
> - Fix source and destination addresses
> - Add define to magic numbers
> - Add DMA cyclic transfer feature
>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eugeniy Paltsev <paltsev@synopsys.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Jose Abreu <jose.abreu@synopsys.com>
> Cc: Luis Oliveira <lolivei@synopsys.com>
> Cc: Vitor Soares <vitor.soares@synopsys.com>
> Cc: Nelson Costa <nelson.costa@synopsys.com>
> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
> +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> +{
> + struct dw_edma_chan *chan = desc->chan;
> + struct dw_edma *dw = chan->chip->dw;
> + struct dw_edma_chunk *chunk;
> +
> + chunk = kvzalloc(sizeof(*chunk), GFP_NOWAIT);
> + if (unlikely(!chunk))
> + return NULL;
> +
> + INIT_LIST_HEAD(&chunk->list);
> + chunk->chan = chan;
> + chunk->cb = !(desc->chunks_alloc % 2);
> + chunk->ll_region.paddr = dw->ll_region.paddr + chan->ll_off;
> + chunk->ll_region.vaddr = dw->ll_region.vaddr + chan->ll_off;
> +
> + if (desc->chunk) {
> + /* Create and add new element into the linked list */
> + desc->chunks_alloc++;
> + dev_dbg(chan2dev(chan), "alloc new chunk element (%d)\n",
> + desc->chunks_alloc);
> + list_add_tail(&chunk->list, &desc->chunk->list);
> + dw_edma_alloc_burst(chunk);
No return check ?
> + } else {
> + /* List head */
> + chunk->burst = NULL;
> + desc->chunks_alloc = 0;
> + desc->chunk = chunk;
> + dev_dbg(chan2dev(chan), "alloc new chunk head\n");
> + }
> +
> + return chunk;
> +}
> +
> +static struct dw_edma_desc *dw_edma_alloc_desc(struct dw_edma_chan *chan)
> +{
> + struct dw_edma_desc *desc;
> +
> + dev_dbg(chan2dev(chan), "alloc new descriptor\n");
> +
> + desc = kvzalloc(sizeof(*desc), GFP_NOWAIT);
> + if (unlikely(!desc))
> + return NULL;
> +
> + desc->chan = chan;
> + dw_edma_alloc_chunk(desc);
No return check ?
> +
> + return desc;
> +}
> +
> +static void dw_edma_start_transfer(struct dw_edma_chan *chan)
> +{
> + struct virt_dma_desc *vd;
> + struct dw_edma_desc *desc;
> + struct dw_edma_chunk *child;
> + const struct dw_edma_core_ops *ops = chan2ops(chan);
Reverse tree order here and in remaining functions ...
> +
> + vd = vchan_next_desc(&chan->vc);
> + if (!vd)
> + return;
> +
> + desc = vd2dw_edma_desc(vd);
> + if (!desc)
> + return;
> +
> + child = list_first_entry_or_null(&desc->chunk->list,
> + struct dw_edma_chunk, list);
> + if (!child)
> + return;
> +
> + ops->start(child, !desc->xfer_sz);
> + desc->xfer_sz += child->ll_region.sz;
> + dev_dbg(chan2dev(chan), "transfer of %u bytes started\n",
> + child->ll_region.sz);
> +
> + dw_edma_free_burst(child);
> + if (child->bursts_alloc)
> + dev_dbg(chan2dev(chan), "%u bursts still allocated\n",
> + child->bursts_alloc);
> + list_del(&child->list);
> + kvfree(child);
> + desc->chunks_alloc--;
> +}
> +
> +int dw_edma_probe(struct dw_edma_chip *chip)
> +{
> + struct dw_edma *dw = chip->dw;
> + struct device *dev = chip->dev;
> + const struct dw_edma_core_ops *ops;
> + size_t ll_chunk = dw->ll_region.sz;
> + size_t dt_chunk = dw->dt_region.sz;
> + u32 ch_tot;
> + int i, j, err;
> +
> + raw_spin_lock_init(&dw->lock);
Why raw ?
> +
> + /* Callback operation selection accordingly to eDMA version */
> + switch (dw->version) {
> + default:
> + dev_err(dev, "unsupported version\n");
> + return -EPERM;
> + }
> +
> + pm_runtime_get_sync(dev);
Why do you need to increase usage count at probe ? And shouldn't
this be after pm_runtime_enable() ?
> +
> + /* Find out how many write channels are supported by hardware */
> + dw->wr_ch_cnt = ops->ch_count(dw, EDMA_DIR_WRITE);
> + if (!dw->wr_ch_cnt) {
> + dev_err(dev, "invalid number of write channels(0)\n");
> + return -EINVAL;
> + }
> +
> + /* Find out how many read channels are supported by hardware */
> + dw->rd_ch_cnt = ops->ch_count(dw, EDMA_DIR_READ);
> + if (!dw->rd_ch_cnt) {
> + dev_err(dev, "invalid number of read channels(0)\n");
> + return -EINVAL;
> + }
> +
> + dev_dbg(dev, "Channels:\twrite=%d, read=%d\n",
> + dw->wr_ch_cnt, dw->rd_ch_cnt);
> +
> + ch_tot = dw->wr_ch_cnt + dw->rd_ch_cnt;
> +
> + /* Allocate channels */
> + dw->chan = devm_kcalloc(dev, ch_tot, sizeof(*dw->chan), GFP_KERNEL);
> + if (!dw->chan)
> + return -ENOMEM;
> +
> + /* Calculate the linked list chunk for each channel */
> + ll_chunk /= roundup_pow_of_two(ch_tot);
> +
> + /* Calculate the linked list chunk for each channel */
> + dt_chunk /= roundup_pow_of_two(ch_tot);
> +
> + /* Disable eDMA, only to establish the ideal initial conditions */
> + ops->off(dw);
> +
> + snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%d", chip->id);
> +
> + /* Request IRQs */
> + if (dw->nr_irqs != 1) {
> + dev_err(dev, "invalid number of irqs (%u)\n", dw->nr_irqs);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < dw->nr_irqs; i++) {
> + err = devm_request_irq(dev, pci_irq_vector(to_pci_dev(dev), i),
> + dw_edma_interrupt_all,
> + IRQF_SHARED, dw->name, chip);
> + if (err)
> + return err;
> + }
> +
> + /* Create write channels */
> + INIT_LIST_HEAD(&dw->wr_edma.channels);
> + for (i = 0; i < dw->wr_ch_cnt; i++) {
> + struct dw_edma_chan *chan = &dw->chan[i];
> + struct dw_edma_region *dt_region;
> +
> + dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL);
> + if (!dt_region)
> + return -ENOMEM;
> +
> + chan->vc.chan.private = dt_region;
> +
> + chan->chip = chip;
> + chan->id = i;
> + chan->dir = EDMA_DIR_WRITE;
> + chan->configured = false;
> + chan->request = EDMA_REQ_NONE;
> + chan->status = EDMA_ST_IDLE;
> +
> + chan->ll_off = (ll_chunk * i);
> + chan->ll_max = (ll_chunk / EDMA_LL_SZ) - 1;
> +
> + chan->dt_off = (dt_chunk * i);
> +
> + dev_dbg(dev, "L. List:\tChannel write[%u] off=0x%.8lx, max_cnt=%u\n",
> + i, chan->ll_off, chan->ll_max);
> +
> + memcpy(&chan->msi, &dw->msi[0], sizeof(chan->msi));
> +
> + dev_dbg(dev, "MSI:\t\tChannel write[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> + i, chan->msi.address_hi, chan->msi.address_lo,
> + chan->msi.data);
> +
> + chan->vc.desc_free = vchan_free_desc;
> + vchan_init(&chan->vc, &dw->wr_edma);
> +
> + dt_region->paddr = dw->dt_region.paddr + chan->dt_off;
> + dt_region->vaddr = dw->dt_region.vaddr + chan->dt_off;
> + dt_region->sz = dt_chunk;
> +
> + dev_dbg(dev, "Data:\tChannel write[%u] off=0x%.8lx\n",
> + i, chan->dt_off);
> + }
> + dma_cap_zero(dw->wr_edma.cap_mask);
> + dma_cap_set(DMA_SLAVE, dw->wr_edma.cap_mask);
> + dma_cap_set(DMA_CYCLIC, dw->wr_edma.cap_mask);
> + dw->wr_edma.directions = BIT(DMA_DEV_TO_MEM);
> + dw->wr_edma.chancnt = dw->wr_ch_cnt;
> +
> + /* Create read channels */
> + INIT_LIST_HEAD(&dw->rd_edma.channels);
> + for (j = 0; j < dw->rd_ch_cnt; j++, i++) {
> + struct dw_edma_chan *chan = &dw->chan[i];
> + struct dw_edma_region *dt_region;
> +
> + dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL);
> + if (!dt_region)
> + return -ENOMEM;
> +
> + chan->vc.chan.private = dt_region;
> +
> + chan->chip = chip;
> + chan->id = j;
> + chan->dir = EDMA_DIR_READ;
> + chan->configured = false;
> + chan->request = EDMA_REQ_NONE;
> + chan->status = EDMA_ST_IDLE;
> +
> + chan->ll_off = (ll_chunk * i);
> + chan->ll_max = (ll_chunk / EDMA_LL_SZ) - 1;
> +
> + chan->dt_off = (dt_chunk * i);
> +
> + dev_dbg(dev, "L. List:\tChannel read[%u] off=0x%.8lx, max_cnt=%u\n",
> + j, chan->ll_off, chan->ll_max);
> +
> + memcpy(&chan->msi, &dw->msi[0], sizeof(chan->msi));
> +
> + dev_dbg(dev, "MSI:\t\tChannel read[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> + j, chan->msi.address_hi, chan->msi.address_lo,
> + chan->msi.data);
> +
> + chan->vc.desc_free = vchan_free_desc;
> + vchan_init(&chan->vc, &dw->rd_edma);
> +
> + dt_region->paddr = dw->dt_region.paddr + chan->dt_off;
> + dt_region->vaddr = dw->dt_region.vaddr + chan->dt_off;
> + dt_region->sz = dt_chunk;
> +
> + dev_dbg(dev, "Data:\tChannel read[%u] off=0x%.8lx\n",
> + i, chan->dt_off);
> + }
> + dma_cap_zero(dw->rd_edma.cap_mask);
> + dma_cap_set(DMA_SLAVE, dw->rd_edma.cap_mask);
> + dma_cap_set(DMA_CYCLIC, dw->rd_edma.cap_mask);
> + dw->rd_edma.directions = BIT(DMA_MEM_TO_DEV);
> + dw->rd_edma.chancnt = dw->rd_ch_cnt;
> +
> + /* Set DMA channels capabilities */
> + SET_BOTH_CH(src_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
> + SET_BOTH_CH(dst_addr_widths, BIT(DMA_SLAVE_BUSWIDTH_4_BYTES));
> + SET_BOTH_CH(residue_granularity, DMA_RESIDUE_GRANULARITY_DESCRIPTOR);
> +
> + SET_BOTH_CH(dev, dev);
> +
> + SET_BOTH_CH(device_alloc_chan_resources, dw_edma_alloc_chan_resources);
> + SET_BOTH_CH(device_free_chan_resources, dw_edma_free_chan_resources);
> +
> + SET_BOTH_CH(device_config, dw_edma_device_config);
> + SET_BOTH_CH(device_pause, dw_edma_device_pause);
> + SET_BOTH_CH(device_resume, dw_edma_device_resume);
> + SET_BOTH_CH(device_terminate_all, dw_edma_device_terminate_all);
> + SET_BOTH_CH(device_issue_pending, dw_edma_device_issue_pending);
> + SET_BOTH_CH(device_tx_status, dw_edma_device_tx_status);
> + SET_BOTH_CH(device_prep_slave_sg, dw_edma_device_prep_slave_sg);
> + SET_BOTH_CH(device_prep_dma_cyclic, dw_edma_device_prep_dma_cyclic);
> +
> + /* Power management */
> + pm_runtime_enable(dev);
> +
> + /* Register DMA device */
> + err = dma_async_device_register(&dw->wr_edma);
> + if (err)
> + goto err_pm_disable;
> +
> + err = dma_async_device_register(&dw->rd_edma);
> + if (err)
> + goto err_pm_disable;
> +
> + /* Turn debugfs on */
> + err = ops->debugfs_on(chip);
> + if (err) {
> + dev_err(dev, "unable to create debugfs structure\n");
> + goto err_pm_disable;
> + }
> +
> + dev_info(dev, "DesignWare eDMA controller driver loaded completely\n");
> +
> + return 0;
> +
> +err_pm_disable:
> + pm_runtime_disable(dev);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(dw_edma_probe);
> +
> +int dw_edma_remove(struct dw_edma_chip *chip)
> +{
> + struct dw_edma *dw = chip->dw;
> + struct device *dev = chip->dev;
> + const struct dw_edma_core_ops *ops = dw->ops;
> + struct dw_edma_chan *chan, *_chan;
> + int i;
> +
> + /* Disable eDMA */
> + if (ops)
> + ops->off(dw);
> +
> + /* Free irqs */
But devm will automatically free it, no ?
> + for (i = 0; i < dw->nr_irqs; i++) {
> + if (pci_irq_vector(to_pci_dev(dev), i) < 0)
> + continue;
> +
> + devm_free_irq(dev, pci_irq_vector(to_pci_dev(dev), i), chip);
> + }
> +
> + /* Power management */
> + pm_runtime_disable(dev);
> +
> + list_for_each_entry_safe(chan, _chan, &dw->wr_edma.channels,
> + vc.chan.device_node) {
> + list_del(&chan->vc.chan.device_node);
> + tasklet_kill(&chan->vc.task);
> + }
> +
> + list_for_each_entry_safe(chan, _chan, &dw->rd_edma.channels,
> + vc.chan.device_node) {
> + list_del(&chan->vc.chan.device_node);
> + tasklet_kill(&chan->vc.task);
> + }
> +
> + /* Deregister eDMA device */
> + dma_async_device_unregister(&dw->wr_edma);
> + dma_async_device_unregister(&dw->rd_edma);
> +
> + /* Turn debugfs off */
> + if (ops)
> + ops->debugfs_off();
> +
> + dev_info(dev, "DesignWare eDMA controller driver unloaded complete\n");
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_edma_remove);
^ permalink raw reply
* dmaengine: imx-dma: fix wrong callback invoke
From: Leonid Iziumtsev @ 2019-01-15 17:15 UTC (permalink / raw)
To: dmaengine, Dan Williams, Vinod Koul, linux-kernel, m.grzeschik
Cc: Leonid Iziumtsev
Once the "ld_queue" list is not empty, next descriptor will migrate
into "ld_active" list. The "desc" variable will be overwritten
during that transition. And later the dmaengine_desc_get_callback_invoke()
will use it as an argument. As result we invoke wrong callback.
That behaviour was in place since:
commit fcaaba6c7136 ("dmaengine: imx-dma: fix callback path in tasklet").
But after commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job")
things got worse, since possible delay between tasklet_schedule()
from DMA irq handler and actual tasklet function execution got bigger.
And that gave more time for new DMA request to be submitted and
to be put into "ld_queue" list.
It has been noticed that DMA issue is causing problems for "mxc-mmc"
driver. While stressing the system with heavy network traffic and
writing/reading to/from sd card simultaneously the timeout may happen:
10013000.sdhci: mxcmci_watchdog: read time out (status = 0x30004900)
That often lead to file system corruption.
Signed-off-by: Leonid Iziumtsev <leonid.iziumtsev@gmail.com>
---
drivers/dma/imx-dma.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index c2fff3f6c9ca..4a09af3cd546 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -618,7 +618,7 @@ static void imxdma_tasklet(unsigned long data)
{
struct imxdma_channel *imxdmac = (void *)data;
struct imxdma_engine *imxdma = imxdmac->imxdma;
- struct imxdma_desc *desc;
+ struct imxdma_desc *desc, *next_desc;
unsigned long flags;
spin_lock_irqsave(&imxdma->lock, flags);
@@ -648,10 +648,10 @@ static void imxdma_tasklet(unsigned long data)
list_move_tail(imxdmac->ld_active.next, &imxdmac->ld_free);
if (!list_empty(&imxdmac->ld_queue)) {
- desc = list_first_entry(&imxdmac->ld_queue, struct imxdma_desc,
- node);
+ next_desc = list_first_entry(&imxdmac->ld_queue,
+ struct imxdma_desc, node);
list_move_tail(imxdmac->ld_queue.next, &imxdmac->ld_active);
- if (imxdma_xfer_desc(desc) < 0)
+ if (imxdma_xfer_desc(next_desc) < 0)
dev_warn(imxdma->dev, "%s: channel: %d couldn't xfer desc\n",
__func__, imxdmac->channel);
}
^ permalink raw reply related
* dma: ixm-dma: fix warning comparison of distinct pointer types
From: Anders Roxell @ 2019-01-15 14:14 UTC (permalink / raw)
To: Fabio Estevam; +Cc: Vinod, Dan Williams, dmaengine, linux-kernel
On Mon, 14 Jan 2019 at 22:24, Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Anders,
>
> On Thu, Jan 10, 2019 at 9:15 AM Anders Roxell <anders.roxell@linaro.org> wrote:
> >
> > The warning got introduced by commit 930507c18304 ("arm64: add basic
> > Kconfig symbols for i.MX8"). Since it got enabled for arm64. The warning
> > haven't been seen before since size_t was 'unsigned int' when built on
> > arm32.
> >
> > ../drivers/dma/imx-dma.c: In function ‘imxdma_sg_next’:
> > ../include/linux/kernel.h:846:29: warning: comparison of distinct pointer types lacks a cast
> > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> > ^~
> > ../include/linux/kernel.h:860:4: note: in expansion of macro ‘__typecheck’
> > (__typecheck(x, y) && __no_side_effects(x, y))
> > ^~~~~~~~~~~
> > ../include/linux/kernel.h:870:24: note: in expansion of macro ‘__safe_cmp’
> > __builtin_choose_expr(__safe_cmp(x, y), \
> > ^~~~~~~~~~
> > ../include/linux/kernel.h:879:19: note: in expansion of macro ‘__careful_cmp’
> > #define min(x, y) __careful_cmp(x, y, <)
> > ^~~~~~~~~~~~~
> > ../drivers/dma/imx-dma.c:288:8: note: in expansion of macro ‘min’
> > now = min(d->len, sg_dma_len(sg));
> > ^~~
> >
> > Rework so that we use min_t and pass in the size_t that returns the
> > minimum of two values, using the specified type.
> >
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>
> There is a typo in the Subject: s/ixm/imx/ and the prefix should be
> dmaengine instead:
>
> dmaengine: imx-dma: fix warning comparison of distinct pointer types
>
> With that fixed:
>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
OK, thanks Fabio.
Dan, do you want me to resend the patch with an updated shortlog or will
you do that when you apply the patch?
Cheers,
Anders
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Gustavo Pimentel @ 2019-01-15 13:02 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Joao Pinto, Jose Abreu, Luis Oliveira, Vitor Soares, Nelson Costa,
Pedro Sousa
On 15/01/2019 05:45, Andy Shevchenko wrote:
> On Mon, Jan 14, 2019 at 11:44:22AM +0000, Gustavo Pimentel wrote:
>> On 11/01/2019 19:48, Andy Shevchenko wrote:
>>> On Fri, Jan 11, 2019 at 07:33:43PM +0100, Gustavo Pimentel wrote:
>>>> Add Synopsys eDMA IP test and sample driver to be use for testing
>>>> purposes and also as a reference for any developer who needs to
>>>> implement and use Synopsys eDMA.
>>>>
>>>> This driver can be compile as built-in or external module in kernel.
>>>>
>>>> To enable this driver just select DW_EDMA_TEST option in kernel
>>>> configuration, however it requires and selects automatically DW_EDMA
>>>> option too.
>>>>
>>>
>>> Hmm... This doesn't explain what's wrong with dmatest module.
>>
>> There isn't anything wrong with dmatest module, that I know of. In beginning I
>> was planning to used it, however only works with MEM_TO_MEM transfers, that's
>> why I created a similar module but for MEM_TO_DEV and DEV_TO_MEM with
>> scatter-gather and cyclic transfers type for my use case. I don't know if can be
>> applied to other cases, if that is feasible, I'm glad to share it.
>
> What I'm trying to tell is that the dmatest driver would be nice to have such
> capability.
>
At the beginning I thought to add those features to dmatest module, but because
of several points such as:
- I didn't want, by any chance, to broke dmatest module while implementing the
support of those new features;
- Since I'm still gaining knowledge about this subsystem I preferred to do in a
more controlled environment;
- Since this driver is also a reference driver aim to teach and to be use by
someone how to use the dmaengine API, I preferred to keep it simple.
Maybe in the future both modules could be merged in just one tool, but for now I
would prefer to keep it separated specially to fix any immaturity error of this
module.
^ permalink raw reply
* [RFC,v3,5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic
From: Gustavo Pimentel @ 2019-01-15 12:48 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Lorenzo Pieralisi, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On 15/01/2019 05:43, Andy Shevchenko wrote:
> On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
>> On 11/01/2019 19:47, Andy Shevchenko wrote:
>>> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
>
>>>> +static bool disable_msix;
>>>> +module_param(disable_msix, bool, 0644);
>>>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
>>>
>>> Why?!
>>> We are no allow new module parameters without very strong arguments.
>>
>> Since this is a reference driver and might be used to test customized HW
>> solutions, I added this parameter to allow the possibility to test the solution
>> forcing the MSI feature binding. This is required specially if who will test
>> this solution has a Root Complex with both features available (MSI and MSI-X),
>> because the Kernel will give always preference to MSI-X binding (assuming that
>> the EP has also both features available).
>
> Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.
Ok, but how should I proceed? May I leave it or substitute by another way to do
it? If so, how?
As I said, the intended is to be only used for this test case, on normal
operation the parameter it should be always false.
>
>>>> + if (!pdata) {
>>>> + dev_err(dev, "%s missing data structure\n", pci_name(pdev));
>>>> + return -EFAULT;
>>>> + }
>>>
>>> Useless check.
>>
>> Why? It's just a precaution, isn't it a good practice always to think of the
>> worst case?
>
> You just can put an implicit requirement of pdata rather than doing this
Ok, how can I do it? What I should add to the code to force that?
> useless check. I don't believe it would make sense to have NULL pdata for the
> driver since it wouldn't be functional anyhow.
Yes, you're right without pdata the driver can't do anything.
>
>>>> + /* Mapping PCI BAR regions */
>>>> + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
>>>> + BIT(pdata->ll_bar) |
>>>> + BIT(pdata->dt_bar),
>>>> + pci_name(pdev));
>>>> + if (err) {
>
>>>> + return err;
>>>> + }
>
>>>> + if (!pcim_iomap_table(pdev))
>>>> + return -EACCES;
>>>
>>> Never happen condition. Thus useless.
>>
>> pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
>> isn't it a good practice always to think of the worst case?
>
> No, it can't in the conditions your have in the code. See above the lines I left.
> If pcim_iomap_regions() successfully finished...
Nice catch, I didn't saw that. I added that validation because of coverity
analysis. I'll add a comment here to suppress this coverity false positive error.
>
>>>> + dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
>>>
>>> Useless.
>>
>> It's helpful for bring up, I can pass it to dbg.
>
> It just shows that someone didn't use existing tools and features. This message and similar are useless.
> Hint: initcall_debug.
I wasn't aware of it. Thanks for the hint :)
>
>>>> + dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
>>>
>>> Ditto.
>>
>> It's helpful for bring up, I can pass it to dbg.
>
> Ditto.
>
^ permalink raw reply
* [PATCHv2,1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status
From: Dirk Behme @ 2019-01-15 12:44 UTC (permalink / raw)
To: Niklas Söderlund, linux-renesas-soc,
laurent.pinchart+renesas
Cc: dmaengine, vinod.koul, geert+renesas, mfarooq, Achim Dahlhoff
On 30.06.2016 17:15, Niklas Söderlund wrote:
> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
>
> The hardware might have complete the transfer but the interrupt handler
> might not have had a chance to run. If rcar_dmac_chan_get_residue()
> which reads HW registers finds that there is no residue return
> DMA_COMPLETE.
>
> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> [Niklas: add explanation in commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/dma/sh/rcar-dmac.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index dfb1792..791a064 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
> residue = rcar_dmac_chan_get_residue(rchan, cookie);
> spin_unlock_irqrestore(&rchan->lock, flags);
>
> + /* if there's no residue, the cookie is complete */
> + if (!residue)
> + return DMA_COMPLETE;
> +
> dma_set_residue(txstate, residue);
>
> return status;
We have some doubts about this change (mainline commit [1]). Not being a
DMA expert, let me try to explain:
We are configuring a cyclic, never ending DMA
(dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll
the residue (txstate->residue) via rcar_dmac_tx_status(). Having a
cyclic, never ending DMA we think that residue == 0 is a valid value.
However, with above change, a residue value of 0 is 'dropped' and never
written via dma_set_residue() to txstate. So in case we continuously
poll, this value is lost, resulting in wrong behavior of the caller.
In our case with cyclic, never ending DMA, changing this to
dma_set_residue(txstate, residue);
?
Opinions?
Best regards
Dirk
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3544d2878817bd139dda238cdd86a15e1c03d037
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct
dma_chan *chan,
/* if there's no residue, the cookie is complete */
if (!residue)
- return DMA_COMPLETE;
+ status = DMA_COMPLETE;
dma_set_residue(txstate, residue);
seems to help. If we ignore the still wrong DMA_COMPLETE status (which
in case of cyclic DMA doesn't make any sense?) the caller get the
correct txstate->residue values (not dropping the 0).
So maybe we need anything like
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct
dma_chan *chan,
spin_unlock_irqrestore(&rchan->lock, flags);
/* if there's no residue, the cookie is complete */
- if (!residue)
+ if (!residue && !chan->desc.running->cyclic)
return DMA_COMPLETE;
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Andy Shevchenko @ 2019-01-15 5:45 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Joao Pinto, Jose Abreu, Luis Oliveira, Vitor Soares, Nelson Costa,
Pedro Sousa
On Mon, Jan 14, 2019 at 11:44:22AM +0000, Gustavo Pimentel wrote:
> On 11/01/2019 19:48, Andy Shevchenko wrote:
> > On Fri, Jan 11, 2019 at 07:33:43PM +0100, Gustavo Pimentel wrote:
> >> Add Synopsys eDMA IP test and sample driver to be use for testing
> >> purposes and also as a reference for any developer who needs to
> >> implement and use Synopsys eDMA.
> >>
> >> This driver can be compile as built-in or external module in kernel.
> >>
> >> To enable this driver just select DW_EDMA_TEST option in kernel
> >> configuration, however it requires and selects automatically DW_EDMA
> >> option too.
> >>
> >
> > Hmm... This doesn't explain what's wrong with dmatest module.
>
> There isn't anything wrong with dmatest module, that I know of. In beginning I
> was planning to used it, however only works with MEM_TO_MEM transfers, that's
> why I created a similar module but for MEM_TO_DEV and DEV_TO_MEM with
> scatter-gather and cyclic transfers type for my use case. I don't know if can be
> applied to other cases, if that is feasible, I'm glad to share it.
What I'm trying to tell is that the dmatest driver would be nice to have such
capability.
^ permalink raw reply
* [RFC,v3,5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic
From: Andy Shevchenko @ 2019-01-15 5:43 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Lorenzo Pieralisi, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
> On 11/01/2019 19:47, Andy Shevchenko wrote:
> > On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
> >> +static bool disable_msix;
> >> +module_param(disable_msix, bool, 0644);
> >> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
> >
> > Why?!
> > We are no allow new module parameters without very strong arguments.
>
> Since this is a reference driver and might be used to test customized HW
> solutions, I added this parameter to allow the possibility to test the solution
> forcing the MSI feature binding. This is required specially if who will test
> this solution has a Root Complex with both features available (MSI and MSI-X),
> because the Kernel will give always preference to MSI-X binding (assuming that
> the EP has also both features available).
Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.
> >> + if (!pdata) {
> >> + dev_err(dev, "%s missing data structure\n", pci_name(pdev));
> >> + return -EFAULT;
> >> + }
> >
> > Useless check.
>
> Why? It's just a precaution, isn't it a good practice always to think of the
> worst case?
You just can put an implicit requirement of pdata rather than doing this
useless check. I don't believe it would make sense to have NULL pdata for the
driver since it wouldn't be functional anyhow.
> >> + /* Mapping PCI BAR regions */
> >> + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
> >> + BIT(pdata->ll_bar) |
> >> + BIT(pdata->dt_bar),
> >> + pci_name(pdev));
> >> + if (err) {
> >> + return err;
> >> + }
> >> + if (!pcim_iomap_table(pdev))
> >> + return -EACCES;
> >
> > Never happen condition. Thus useless.
>
> pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
> isn't it a good practice always to think of the worst case?
No, it can't in the conditions your have in the code. See above the lines I left.
If pcim_iomap_regions() successfully finished...
> >> + dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
> >
> > Useless.
>
> It's helpful for bring up, I can pass it to dbg.
It just shows that someone didn't use existing tools and features. This message and similar are useless.
Hint: initcall_debug.
> >> + dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
> >
> > Ditto.
>
> It's helpful for bring up, I can pass it to dbg.
Ditto.
^ permalink raw reply
* dma: ixm-dma: fix warning comparison of distinct pointer types
From: Fabio Estevam @ 2019-01-14 21:24 UTC (permalink / raw)
To: Anders Roxell; +Cc: Vinod, Dan Williams, dmaengine, linux-kernel
Hi Anders,
On Thu, Jan 10, 2019 at 9:15 AM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> The warning got introduced by commit 930507c18304 ("arm64: add basic
> Kconfig symbols for i.MX8"). Since it got enabled for arm64. The warning
> haven't been seen before since size_t was 'unsigned int' when built on
> arm32.
>
> ../drivers/dma/imx-dma.c: In function ‘imxdma_sg_next’:
> ../include/linux/kernel.h:846:29: warning: comparison of distinct pointer types lacks a cast
> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> ^~
> ../include/linux/kernel.h:860:4: note: in expansion of macro ‘__typecheck’
> (__typecheck(x, y) && __no_side_effects(x, y))
> ^~~~~~~~~~~
> ../include/linux/kernel.h:870:24: note: in expansion of macro ‘__safe_cmp’
> __builtin_choose_expr(__safe_cmp(x, y), \
> ^~~~~~~~~~
> ../include/linux/kernel.h:879:19: note: in expansion of macro ‘__careful_cmp’
> #define min(x, y) __careful_cmp(x, y, <)
> ^~~~~~~~~~~~~
> ../drivers/dma/imx-dma.c:288:8: note: in expansion of macro ‘min’
> now = min(d->len, sg_dma_len(sg));
> ^~~
>
> Rework so that we use min_t and pass in the size_t that returns the
> minimum of two values, using the specified type.
>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
There is a typo in the Subject: s/ixm/imx/ and the prefix should be
dmaengine instead:
dmaengine: imx-dma: fix warning comparison of distinct pointer types
With that fixed:
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply
* [RFC,v3,4/7] PCI: Add Synopsys endpoint EDDA Device id
From: Bjorn Helgaas @ 2019-01-14 14:41 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Kishon Vijay Abraham I, Lorenzo Pieralisi,
Niklas Cassel, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On Fri, Jan 11, 2019 at 07:33:40PM +0100, Gustavo Pimentel wrote:
> Create and add Synopsys Endpoint EDDA Device id to PCI id list, since
> this id is now being use on two different drivers (pci_endpoint_test.ko
> and dw-edma-pcie.ko).
Nit if you update this series for some other reason: s/id/ID/ in
subject and changelog. "Id" is an English word, but it's not
applicable in this context, so using "ID" makes it clear that we don't
mean the psychoanalysis term.
Bjorn
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Gustavo Pimentel @ 2019-01-14 11:44 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Joao Pinto, Jose Abreu, Luis Oliveira, Vitor Soares, Nelson Costa,
Pedro Sousa
On 11/01/2019 19:48, Andy Shevchenko wrote:
> On Fri, Jan 11, 2019 at 07:33:43PM +0100, Gustavo Pimentel wrote:
>> Add Synopsys eDMA IP test and sample driver to be use for testing
>> purposes and also as a reference for any developer who needs to
>> implement and use Synopsys eDMA.
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA_TEST option in kernel
>> configuration, however it requires and selects automatically DW_EDMA
>> option too.
>>
>
> Hmm... This doesn't explain what's wrong with dmatest module.
There isn't anything wrong with dmatest module, that I know of. In beginning I
was planning to used it, however only works with MEM_TO_MEM transfers, that's
why I created a similar module but for MEM_TO_DEV and DEV_TO_MEM with
scatter-gather and cyclic transfers type for my use case. I don't know if can be
applied to other cases, if that is feasible, I'm glad to share it.
>
^ permalink raw reply
* [RFC,v3,5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic
From: Gustavo Pimentel @ 2019-01-14 11:38 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org, Vinod Koul,
Dan Williams, Eugeniy Paltsev, Russell King, Niklas Cassel,
Lorenzo Pieralisi, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On 11/01/2019 19:47, Andy Shevchenko wrote:
> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
>> Synopsys eDMA IP is normally distributed along with Synopsys PCIe
>> EndPoint IP (depends of the use and licensing agreement).
>>
>> This IP requires some basic configurations, such as:
>> - eDMA registers BAR
>> - eDMA registers offset
>> - eDMA registers size
>> - eDMA linked list memory BAR
>> - eDMA linked list memory offset
>> - eDMA linked list memory sze
>> - eDMA data memory BAR
>> - eDMA data memory offset
>> - eDMA data memory size
>> - eDMA version
>> - eDMA mode
>> - IRQs available for eDMA
>>
>> As a working example, PCIe glue-logic will attach to a Synopsys PCIe
>> EndPoint IP prototype kit (Vendor ID = 0x16c3, Device ID = 0xedda),
>> which has built-in an eDMA IP with this default configuration:
>> - eDMA registers BAR = 0
>> - eDMA registers offset = 0x00001000 (4 Kbytes)
>> - eDMA registers size = 0x00002000 (8 Kbytes)
>> - eDMA linked list memory BAR = 2
>> - eDMA linked list memory offset = 0x00000000 (0 Kbytes)
>> - eDMA linked list memory size = 0x00800000 (8 Mbytes)
>> - eDMA data memory BAR = 2
>> - eDMA data memory offset = 0x00800000 (8 Mbytes)
>> - eDMA data memory size = 0x03800000 (56 Mbytes)
>> - eDMA version = 0
>> - eDMA mode = EDMA_MODE_UNROLL
>> - IRQs = 1
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA_PCIE option in kernel
>> configuration, however it requires and selects automatically DW_EDMA
>> option too.
>>
>
>> Changes:
>> RFC v1->RFC v2:
>
> Changes go after '--- ' line.
At the last Linux Plumbers Conference there were some subsystem maintainers who
asked that the track changes be included in the description as a way to not lose
the previous work done. That why I put it before the '---' line, but it's
indifferent to me, I can put it after the '---' line.
>
>> - Replace comments // (C99 style) by /**/
>> - Merge two pcim_iomap_regions() calls into just one call
>> - Remove pci_try_set_mwi() call
>> - Replace some dev_info() by dev_dbg() to reduce *noise*
>> - Remove pci_name(pdev) call after being call dw_edma_remove()
>> - Remove all power management support
>> - Fix the headers of the .c and .h files according to the most recent
>> convention
>> - Fix errors and checks pointed out by checkpatch with --strict option
>> - Replace patch small description tag from dma by dmaengine
>> RFC v2->RFC v3:
>> - Fix printk variable of phys_addr_t type
>> - Fix missing variable initialization (chan->configured)
>> - Change linked list size to 512 Kbytes
>> - Add data memory information
>> - Add register size information
>> - Add comments or improve existing ones
>> - Add possibility to work with multiple IRQs feature
>> - Replace MSI and MSI-X enable condition by pci_dev_msi_enabled()
>> - Replace code to acquire MSI(-X) address and data by
>> get_cached_msi_msg()
>
>> +enum dw_edma_pcie_bar {
>> + BAR_0,
>> + BAR_1,
>> + BAR_2,
>> + BAR_3,
>> + BAR_4,
>> + BAR_5
>> +};
>
> pci-epf.h has this.
> Why duplicate?
I can use that header sure. Thanks.
>
>
> What else is being duplicated from PCI core?
>
>> +static bool disable_msix;
>> +module_param(disable_msix, bool, 0644);
>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
>
> Why?!
> We are no allow new module parameters without very strong arguments.
Since this is a reference driver and might be used to test customized HW
solutions, I added this parameter to allow the possibility to test the solution
forcing the MSI feature binding. This is required specially if who will test
this solution has a Root Complex with both features available (MSI and MSI-X),
because the Kernel will give always preference to MSI-X binding (assuming that
the EP has also both features available).
>
>> +
>> +static int dw_edma_pcie_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *pid)
>> +{
>> + const struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
>> + struct device *dev = &pdev->dev;
>> + struct dw_edma_chip *chip;
>> + struct dw_edma *dw;
>> + unsigned int irq_flags = PCI_IRQ_MSI;
>> + int err, nr_irqs, i;
>> +
>
>> + if (!pdata) {
>> + dev_err(dev, "%s missing data structure\n", pci_name(pdev));
>> + return -EFAULT;
>> + }
>
> Useless check.
Why? It's just a precaution, isn't it a good practice always to think of the
worst case?
>
>> +
>> + /* Enable PCI device */
>> + err = pcim_enable_device(pdev);
>> + if (err) {
>> + dev_err(dev, "%s enabling device failed\n", pci_name(pdev));
>> + return err;
>> + }
>> +
>> + /* Mapping PCI BAR regions */
>> + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
>> + BIT(pdata->ll_bar) |
>> + BIT(pdata->dt_bar),
>> + pci_name(pdev));
>> + if (err) {
>
>> + dev_err(dev, "%s eDMA BAR I/O remapping failed\n",
>> + pci_name(pdev));
>
> Isn't it pci_err() ?
> Same comment for the rest similar cases above and below.
Ok, I'll replace all dev_* function in this file.
Thanks.
>
>> + return err;
>> + }
>> +
>> + pci_set_master(pdev);
>> +
>> + nr_irqs = pci_alloc_irq_vectors(pdev, 1, pdata->irqs_cnt, irq_flags);
>> + if (nr_irqs < 1) {
>> + dev_err(dev, "%s failed to alloc IRQ vector (Number of IRQs=%u)\n",
>> + pci_name(pdev), nr_irqs);
>> + return -EPERM;
>> + }
>> +
>> + /* Data structure initialization */
>> + chip->dw = dw;
>> + chip->dev = dev;
>> + chip->id = pdev->devfn;
>> + chip->irq = pdev->irq;
>> +
>
>> + if (!pcim_iomap_table(pdev))
>> + return -EACCES;
>
> Never happen condition. Thus useless.
pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
isn't it a good practice always to think of the worst case?
>
>> + dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
>
> Useless.
It's helpful for bring up, I can pass it to dbg.
>
>> +}
>> +
>> +static void dw_edma_pcie_remove(struct pci_dev *pdev)
>> +{
>> + struct dw_edma_chip *chip = pci_get_drvdata(pdev);
>> + struct device *dev = &pdev->dev;
>> + int err;
>> +
>> + /* Stopping eDMA driver */
>> + err = dw_edma_remove(chip);
>> + if (err)
>> + dev_warn(dev, "can't remove device properly: %d\n", err);
>> +
>> + /* Freeing IRQs */
>> + pci_free_irq_vectors(pdev);
>> +
>> + dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
>
> Ditto.
It's helpful for bring up, I can pass it to dbg.
>
>> +}
>
>> +MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
>> +
>> +static struct pci_driver dw_edma_pcie_driver = {
>> + .name = "dw-edma-pcie",
>> + .id_table = dw_edma_pcie_id_table,
>> + .probe = dw_edma_pcie_probe,
>> + .remove = dw_edma_pcie_remove,
>
> Power management?
I've removed the power management for now, since with my current setup I don't
have the necessary conditions to test it. I prefer not submitting that code for now.
>
>> +};
>
Thanks for the inputs Andy! They have been pretty good!
Regards,
Gustavo
^ permalink raw reply
* dma: ixm-dma: fix warning comparison of distinct pointer types
From: Olof Johansson @ 2019-01-13 18:35 UTC (permalink / raw)
To: Anders Roxell
Cc: Vinod Koul, Dan Williams, dmaengine, Linux Kernel Mailing List
On Thu, Jan 10, 2019 at 3:15 AM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> The warning got introduced by commit 930507c18304 ("arm64: add basic
> Kconfig symbols for i.MX8"). Since it got enabled for arm64. The warning
> haven't been seen before since size_t was 'unsigned int' when built on
> arm32.
>
> ../drivers/dma/imx-dma.c: In function ‘imxdma_sg_next’:
> ../include/linux/kernel.h:846:29: warning: comparison of distinct pointer types lacks a cast
> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> ^~
> ../include/linux/kernel.h:860:4: note: in expansion of macro ‘__typecheck’
> (__typecheck(x, y) && __no_side_effects(x, y))
> ^~~~~~~~~~~
> ../include/linux/kernel.h:870:24: note: in expansion of macro ‘__safe_cmp’
> __builtin_choose_expr(__safe_cmp(x, y), \
> ^~~~~~~~~~
> ../include/linux/kernel.h:879:19: note: in expansion of macro ‘__careful_cmp’
> #define min(x, y) __careful_cmp(x, y, <)
> ^~~~~~~~~~~~~
> ../drivers/dma/imx-dma.c:288:8: note: in expansion of macro ‘min’
> now = min(d->len, sg_dma_len(sg));
> ^~~
>
> Rework so that we use min_t and pass in the size_t that returns the
> minimum of two values, using the specified type.
>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
'now' should probably also be a size_t, but it can be done separately.
imxdma_sg_next()'s return value is never checked anywhere, so the
function can be changed to void as well.
That being said, I think this specific patch should go in now to keep
warnings out of our standard builds. The more we keep around, the
harder it is to spot when a new legitimate one shows up.
Acked-by: Olof Johansson <olof@lixom.net>
-Olof
^ permalink raw reply
* [2/8,v3] Documentation: bindings: k3dma: Add binding for hisi-dma-avail-chan
From: John Stultz @ 2019-01-11 21:30 UTC (permalink / raw)
To: Rob Herring
Cc: lkml, Vinod Koul, Mark Rutland, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Fri, Jan 11, 2019 at 12:04 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jan 11, 2019 at 1:58 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Jan 10, 2019 at 11:34 AM John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > Some dma channels can be reserved for secure mode or other
> > > hardware on the SoC, so provide a binding for a bitmask
> > > listing the available channels for the kernel to use.
> > >
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > Cc: Ryan Grachek <ryan@edited.us>
> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Cc: dmaengine@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > ---
> > > v3: Renamed to hisi-dma-avail-chan
> > > ---
> > > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > index 10a2f15..38825d4 100644
> > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > @@ -14,6 +14,9 @@ Required properties:
> > > have specific request line
> > > - clocks: clock required
> > >
> > > +Optional properties:
> > > +- hisi-dma-avail-chan: Bitmask of available physical channels
> >
> > Not quite right. Should be: hisilicon,dma-avail-chan
>
> Actually, we already have the same case elsewhere with
> 'brcm,dma-channel-mask'. Maybe there are others. So make the property
> common (i.e. documented in dma.txt) and called 'dma-channel-mask'.
Ok. I'll rework it for that then.
> Whether or not the dmaengine handles this or not is irrelevant to
> whether the binding is common or not. I have no say over OS design
> decisions.
Ok. I'll keep it in the driver for now unless otherwise directed.
Thanks so much for the review!
-john
^ permalink raw reply
* [2/8,v3] Documentation: bindings: k3dma: Add binding for hisi-dma-avail-chan
From: Rob Herring @ 2019-01-11 20:04 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Vinod Koul, Mark Rutland, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, devicetree
On Fri, Jan 11, 2019 at 1:58 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Jan 10, 2019 at 11:34 AM John Stultz <john.stultz@linaro.org> wrote:
> >
> > Some dma channels can be reserved for secure mode or other
> > hardware on the SoC, so provide a binding for a bitmask
> > listing the available channels for the kernel to use.
> >
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Tanglei Han <hantanglei@huawei.com>
> > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > Cc: Ryan Grachek <ryan@edited.us>
> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Cc: dmaengine@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v3: Renamed to hisi-dma-avail-chan
> > ---
> > Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > index 10a2f15..38825d4 100644
> > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > @@ -14,6 +14,9 @@ Required properties:
> > have specific request line
> > - clocks: clock required
> >
> > +Optional properties:
> > +- hisi-dma-avail-chan: Bitmask of available physical channels
>
> Not quite right. Should be: hisilicon,dma-avail-chan
Actually, we already have the same case elsewhere with
'brcm,dma-channel-mask'. Maybe there are others. So make the property
common (i.e. documented in dma.txt) and called 'dma-channel-mask'.
Whether or not the dmaengine handles this or not is irrelevant to
whether the binding is common or not. I have no say over OS design
decisions.
Rob
^ permalink raw reply
* [2/8,v3] Documentation: bindings: k3dma: Add binding for hisi-dma-avail-chan
From: Rob Herring @ 2019-01-11 19:58 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Vinod Koul, Mark Rutland, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, devicetree
On Thu, Jan 10, 2019 at 11:34 AM John Stultz <john.stultz@linaro.org> wrote:
>
> Some dma channels can be reserved for secure mode or other
> hardware on the SoC, so provide a binding for a bitmask
> listing the available channels for the kernel to use.
>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Tanglei Han <hantanglei@huawei.com>
> Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> Cc: Ryan Grachek <ryan@edited.us>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v3: Renamed to hisi-dma-avail-chan
> ---
> Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 10a2f15..38825d4 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -14,6 +14,9 @@ Required properties:
> have specific request line
> - clocks: clock required
>
> +Optional properties:
> +- hisi-dma-avail-chan: Bitmask of available physical channels
Not quite right. Should be: hisilicon,dma-avail-chan
^ 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