* [2/3] dmaengine: sprd: Add new DMA engine translation function
From: Baolin Wang @ 2019-01-22 13:20 UTC (permalink / raw)
To: vkoul, robh+dt, mark.rutland
Cc: arnd, olof, orsonzhai, zhang.lyra, dan.j.williams, devicetree,
arm, linux-arm-kernel, linux-kernel, dmaengine, eric.long,
broonie, baolin.wang
Add new DMA engine translation function to get the hardware slave id
of the corresponding DMA engine channel. Meanwhile we do not need
to set default slave id in sprd_dma_alloc_chan_resources(), remove it.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 49 ++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index e2f0167..7d180bb 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -204,9 +204,9 @@ struct sprd_dma_dev {
struct sprd_dma_chn channels[0];
};
-static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param);
-static struct of_dma_filter_info sprd_dma_info = {
- .filter_fn = sprd_dma_filter_fn,
+struct sprd_dma_filter_param {
+ u32 chn_id;
+ u32 slave_id;
};
static inline struct sprd_dma_chn *to_sprd_dma_chan(struct dma_chan *c)
@@ -580,15 +580,7 @@ static irqreturn_t dma_irq_handle(int irq, void *dev_id)
static int sprd_dma_alloc_chan_resources(struct dma_chan *chan)
{
- struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
- int ret;
-
- ret = pm_runtime_get_sync(chan->device->dev);
- if (ret < 0)
- return ret;
-
- schan->dev_id = SPRD_DMA_SOFTWARE_UID;
- return 0;
+ return pm_runtime_get_sync(chan->device->dev);
}
static void sprd_dma_free_chan_resources(struct dma_chan *chan)
@@ -1022,12 +1014,31 @@ static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
{
struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
struct sprd_dma_dev *sdev = to_sprd_dma_dev(&schan->vc.chan);
- u32 req = *(u32 *)param;
+ struct sprd_dma_filter_param *sparam = param;
- if (req < sdev->total_chns)
- return req == schan->chn_num + 1;
- else
- return false;
+ if (sparam->chn_id < sdev->total_chns &&
+ sparam->chn_id == schan->chn_num + 1) {
+ schan->dev_id = sparam->slave_id;
+ return true;
+ }
+
+ return false;
+}
+
+static struct dma_chan *sprd_dma_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct sprd_dma_dev *sdev = ofdma->of_dma_data;
+ dma_cap_mask_t mask = sdev->dma_dev.cap_mask;
+ struct sprd_dma_filter_param param;
+
+ if (dma_spec->args_count != 2)
+ return NULL;
+
+ param.chn_id = dma_spec->args[0];
+ param.slave_id = dma_spec->args[1];
+
+ return dma_request_channel(mask, sprd_dma_filter_fn, ¶m);
}
static int sprd_dma_probe(struct platform_device *pdev)
@@ -1133,9 +1144,7 @@ static int sprd_dma_probe(struct platform_device *pdev)
goto err_register;
}
- sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
- ret = of_dma_controller_register(np, of_dma_simple_xlate,
- &sprd_dma_info);
+ ret = of_dma_controller_register(np, sprd_dma_xlate, sdev);
if (ret)
goto err_of_register;
^ permalink raw reply related
* [1/3] dt-bindings: dmaengine: Add one new cell to present hardware slave id
From: Baolin Wang @ 2019-01-22 13:20 UTC (permalink / raw)
To: vkoul, robh+dt, mark.rutland
Cc: arnd, olof, orsonzhai, zhang.lyra, dan.j.williams, devicetree,
arm, linux-arm-kernel, linux-kernel, dmaengine, eric.long,
broonie, baolin.wang
The DMA engine clients can trigger DMA engine automatically by setting
the corresponding hardware slave id for the DMA engine. Thus add one
cell to present the hardware slave id for DMA clients.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Documentation/devicetree/bindings/dma/sprd-dma.txt | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/dma/sprd-dma.txt b/Documentation/devicetree/bindings/dma/sprd-dma.txt
index 7a10fea..7812cf0 100644
--- a/Documentation/devicetree/bindings/dma/sprd-dma.txt
+++ b/Documentation/devicetree/bindings/dma/sprd-dma.txt
@@ -6,8 +6,8 @@ Required properties:
- compatible: Should be "sprd,sc9860-dma".
- reg: Should contain DMA registers location and length.
- interrupts: Should contain one interrupt shared by all channel.
-- #dma-cells: must be <1>. Used to represent the number of integer
- cells in the dmas property of client device.
+- #dma-cells: must be <2>. Used to represent the channel id and slave id
+ of integer cells in the dmas property of client device.
- #dma-channels : Number of DMA channels supported. Should be 32.
- clock-names: Should contain the clock of the DMA controller.
- clocks: Should contain a clock specifier for each entry in clock-names.
@@ -28,14 +28,16 @@ apdma: dma-controller@20100000 {
Client:
DMA clients connected to the Spreadtrum DMA controller must use the format
-described in the dma.txt file, using a two-cell specifier for each channel.
-The two cells in order are:
+described in the dma.txt file, using a three-cell specifier for each channel.
+The three cells in order are:
1. A phandle pointing to the DMA controller.
2. The channel id.
+3. The hardware slave id which is used for clients to trigger DMA engine
+automatically.
spi0: spi@70a00000{
...
dma-names = "rx_chn", "tx_chn";
- dmas = <&apdma 11>, <&apdma 12>;
+ dmas = <&apdma 11 11>, <&apdma 12 12>;
...
};
^ permalink raw reply related
* dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Ludovic Desroches @ 2019-01-22 9:13 UTC (permalink / raw)
To: Codrin Ciubotariu - M19940
Cc: vkoul@kernel.org, dmaengine@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On Thu, Jan 17, 2019 at 05:10:38PM +0100, Codrin Ciubotariu - M19940 wrote:
> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>
> atchan->status is used for two things:
> - pass channel interrupts status from interrupt handler to tasklet;
> - channel information like whether it is cyclic or paused;
>
> Since these operations have nothing in common, this patch adds a
> different struct member to keep the interrupts status.
>
> Fixes a bug in which a channel is wrongfully reported as in use when
> trying to obtain a new descriptior for a previously used cyclic channel.
>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
As Vinod suggested, you may change the commit message to emphasize
that the bug comes from the fact that a single variable is used to store
different information.
The Fixes: tag should be added too.
Regards
Ludovic
> ---
> drivers/dma/at_xdmac.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index 4e557684f792..fe69dccfa0c0 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -203,6 +203,7 @@ struct at_xdmac_chan {
> u32 save_cim;
> u32 save_cnda;
> u32 save_cndc;
> + u32 irq_status;
> unsigned long status;
> struct tasklet_struct tasklet;
> struct dma_slave_config sconfig;
> @@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
> struct at_xdmac_desc *desc;
> u32 error_mask;
>
> - dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
> - __func__, atchan->status);
> + dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
> + __func__, atchan->irq_status);
>
> error_mask = AT_XDMAC_CIS_RBEIS
> | AT_XDMAC_CIS_WBEIS
> @@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
>
> if (at_xdmac_chan_is_cyclic(atchan)) {
> at_xdmac_handle_cyclic(atchan);
> - } else if ((atchan->status & AT_XDMAC_CIS_LIS)
> - || (atchan->status & error_mask)) {
> + } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
> + || (atchan->irq_status & error_mask)) {
> struct dma_async_tx_descriptor *txd;
>
> - if (atchan->status & AT_XDMAC_CIS_RBEIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
> dev_err(chan2dev(&atchan->chan), "read bus error!!!");
> - if (atchan->status & AT_XDMAC_CIS_WBEIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
> dev_err(chan2dev(&atchan->chan), "write bus error!!!");
> - if (atchan->status & AT_XDMAC_CIS_ROIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
> dev_err(chan2dev(&atchan->chan), "request overflow error!!!");
>
> spin_lock(&atchan->lock);
> @@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
> atchan = &atxdmac->chan[i];
> chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
> chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
> - atchan->status = chan_status & chan_imr;
> + atchan->irq_status = chan_status & chan_imr;
> dev_vdbg(atxdmac->dma.dev,
> "%s: chan%d: imr=0x%x, status=0x%x\n",
> __func__, i, chan_imr, chan_status);
> @@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
> at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
> at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
>
> - if (atchan->status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
> + if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
> at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask);
>
> tasklet_schedule(&atchan->tasklet);
> --
> 2.17.1
>
^ permalink raw reply
* [2/8,v4] Documentation: bindings: dma: Add binding for dma-channel-mask
From: Rob Herring @ 2019-01-22 1:13 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Vinod Koul, Mark Rutland, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Manivannan Sadhasivam, dmaengine, devicetree
On Wed, 16 Jan 2019 09:10:23 -0800, John Stultz 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.
>
> 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(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [RFC,v3,7/7] dmaengine: Add Synopsys eDMA IP test and sample driver
From: Gustavo Pimentel @ 2019-01-21 15:59 UTC (permalink / raw)
To: Vinod Koul, Gustavo Pimentel
Cc: Andy Shevchenko, linux-pci@vger.kernel.org,
dmaengine@vger.kernel.org, Dan Williams, Eugeniy Paltsev,
Russell King, Niklas Cassel, Joao Pinto, Jose Abreu,
Luis Oliveira, Vitor Soares, Nelson Costa, Pedro Sousa
On 17/01/2019 05:03, Vinod Koul wrote:
> On 15-01-19, 13:02, Gustavo Pimentel wrote:
>> 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.
>
> With decent testing we should be able to iron out kinks in dmatest
> module. It gets tested for memcpy controllers so we should easily catch
> issues.
Ok, since I don't have a setup to test that, may I count with you for test it?
>
> Said that, it would make sense to add support in generic dmatest so that
> other folks can reuse and contribute as well. Future work always gets
> side tracked by life, so lets do it now :)
But there are some points that will need to be reworked or clarified.
For instance, I require to know the physical and virtual address and the maximum
size allowed for the Endpoint (in my case), where I will put/get the data.
Another difference is, I create all threads and only after that I start them all
simultaneously, the dmatest doesn't have that behavior.
There are 1 or 2 more details, but the ones that have the greatest impact are those.
>
>
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-21 15:49 UTC (permalink / raw)
To: Vinod Koul, Gustavo Pimentel
Cc: Jose Abreu, linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Dan Williams, Eugeniy Paltsev, Andy Shevchenko, Russell King,
Niklas Cassel, Joao Pinto, Luis de Oliveira, Vitor Soares,
Nelson Costa, Pedro Sousa
On 20/01/2019 11:47, Vinod Koul wrote:
> On 16-01-19, 11:53, Gustavo Pimentel wrote:
>
>>>> + /* Free irqs */
>>>
>>> But devm will automatically free it, no ?
>>
>> Yes, it shouldn't be there. I'll remove it.
>
> Nope this is correct. we need to ensure irqs are freed and tasklets are
> quiesced otherwise you can end up with a case when a spurious interrupt
> and then tasklets spinning while core unrolls. devm for irq is not a
> good idea unless you really know what you are doing!
>
Ok. Nice explanation.
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-21 15:48 UTC (permalink / raw)
To: Vinod Koul, Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Dan Williams, Eugeniy Paltsev, Andy Shevchenko, Russell King,
Niklas Cassel, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On 20/01/2019 11:44, Vinod Koul wrote:
> On 11-01-19, 19:33, 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.
>
> A description of eDMA IP will help review the driver
I've the IP description on the cover-letter, but I'll bring it to this patch, if
it helps.
>
>> 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:
>
> These do not belong to change log, either move them to cover-letter or
> keep them after s-o-b lines..
Ok, Andy has also referred that. I'll keep them after s-o-b lines.
>
>> @@ -0,0 +1,1059 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
>
> 2019 now
I've changed to "Copyright (c) 2018-present Synopsys, Inc. and/or its
affiliates." this way it's always up to date and I also kept 2018, because it
was the date that I started to develop this driver, if you don't mind.
>
>> +static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk)
>> +{
>> + struct dw_edma_chan *chan = chunk->chan;
>> + struct dw_edma_burst *burst;
>> +
>> + burst = kvzalloc(sizeof(*burst), GFP_NOWAIT);
>
> Is there a specific reason for kvzalloc(), do you submit this to HW..
There is not reason for that, it were remains of an initial implementation. I'll
replace them by kzalloc.
Nicely spotted!
>
>> +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);
>
> cb ..?
CB = change bit, is a property of this eDMA IP. Basically it is a kind of
handshake which serves to validate whether the linked list has been updated or
not, especially useful in cases of recycled linked list elements (every linked
list recycle is a new chunk, this will allow to differentiate each chunk).
>
>> +static int dw_edma_device_config(struct dma_chan *dchan,
>> + struct dma_slave_config *config)
>> +{
>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
>> + unsigned long flags;
>> + int err = 0;
>> +
>> + spin_lock_irqsave(&chan->vc.lock, flags);
>> +
>> + if (!config) {
>> + err = -EINVAL;
>> + goto err_config;
>> + }
>> +
>> + if (chan->status != EDMA_ST_IDLE) {
>> + dev_err(chan2dev(chan), "channel is busy or paused\n");
>> + err = -EPERM;
>
> this is not correct behaviour, device_config can be called anytime and
> values can take affect on next transaction submitted..
Hum, I thought we could only reconfigure after transfer being finished.
>
>> + goto err_config;
>> + }
>> +
>> + dev_dbg(chan2dev(chan), "addr(physical) src=%pa, dst=%pa\n",
>> + &config->src_addr, &config->dst_addr);
>> +
>> + chan->src_addr = config->src_addr;
>> + chan->dst_addr = config->dst_addr;
>> +
>> + err = ops->device_config(dchan);
>
> what does this do?
This is an initialization procedure to setup interrupts (data and addresses) to
each channel on the eDMA IP, in order to be triggered after transfer being
completed or aborted. Due the fact the config() can be called at anytime,
doesn't make sense to have this procedure here, I'll moved it to probe().
>
>> +static enum dma_status
>> +dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
>> + struct dma_tx_state *txstate)
>> +{
>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> + const struct dw_edma_core_ops *ops = chan2ops(chan);
>> + unsigned long flags;
>> + enum dma_status ret;
>> +
>> + spin_lock_irqsave(&chan->vc.lock, flags);
>> +
>> + ret = ops->ch_status(chan);
>> + if (ret == DMA_ERROR) {
>> + goto ret_status;
>> + } else if (ret == DMA_IN_PROGRESS) {
>> + chan->status = EDMA_ST_BUSY;
>> + goto ret_status;
>
> so in this case you set residue as zero, which is not correct
Ok, the residue should be the remaining bytes to transfer, right?
>
>> + } else {
>> + /* DMA_COMPLETE */
>> + if (chan->status == EDMA_ST_PAUSE)
>> + ret = DMA_PAUSED;
>> + else if (chan->status == EDMA_ST_BUSY)
>> + ret = DMA_IN_PROGRESS;
>
> ?? if txn is complete how are you returning DMA_IN_PROGRESS?
This IP reports DMA_COMPLETE when it successfully completes the transfer of all
linked list elements, given that there may be even more elements of the linked
list to be transferred the overall state is set to DMA_IN_PROGRESS.
>
>> + else
>> + ret = DMA_COMPLETE;
>> + }
>
> this looks incorrect interpretation to me. The status is to be retrieved
> for the given cookie passed and given that you do not even use this
> argument tells me that you have understood this as 'channel' status
> reporting, which is not correct
Yes, you're right, my interpretation assumes this function reports
channel/transaction status. What would be the correct implementation?
Something like this?
{
struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
const struct dw_edma_core_ops *ops = chan2ops(chan);
struct dw_edma_desc *desc;
struct virt_dma_desc *vd;
unsigned long flags;
enum dma_status ret;
u32 residue = 0;
spin_lock_irqsave(&chan->vc.lock, flags);
ret = dma_cookie_status(chan, cookie, txstate);
if (ret == DMA_COMPLETE)
goto ret_status;
vd = vchan_next_desc(&chan->vc);
if (!vd)
goto ret_status;
desc = vd2dw_edma_desc(vd);
if (!desc)
residue = desc->alloc_sz - desc->xfer_sz;
if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
ret = DMA_PAUSED;
ret_status:
spin_unlock_irqrestore(&chan->vc.lock, flags);
dma_set_residue(txstate, residue);
return ret;
}
>
>> +static struct dma_async_tx_descriptor *
>> +dw_edma_device_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>> + unsigned int sg_len,
>> + enum dma_transfer_direction direction,
>> + unsigned long flags, void *context)
>> +{
>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> + struct dw_edma_desc *desc;
>> + struct dw_edma_chunk *chunk;
>> + struct dw_edma_burst *burst;
>> + struct scatterlist *sg;
>> + unsigned long sflags;
>> + phys_addr_t src_addr;
>> + phys_addr_t dst_addr;
>> + int i;
>> +
>> + if (sg_len < 1) {
>> + dev_err(chan2dev(chan), "invalid sg length %u\n", sg_len);
>> + return NULL;
>> + }
>> +
>> + if (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
>
> what is the second part of the check, can you explain that, who sets
> chan->dir?
The chan->dir is set on probe() during the process of configuring each channel.
>
>> + dev_dbg(chan2dev(chan), "prepare operation (WRITE)\n");
>> + } else if (direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
>> + dev_dbg(chan2dev(chan), "prepare operation (READ)\n");
>> + } else {
>> + dev_err(chan2dev(chan), "invalid direction\n");
>> + return NULL;
>> + }
>> +
>> + if (!chan->configured) {
>> + dev_err(chan2dev(chan), "(prep_slave_sg) channel not configured\n");
>> + return NULL;
>> + }
>> +
>> + if (chan->status != EDMA_ST_IDLE) {
>> + dev_err(chan2dev(chan), "channel is busy or paused\n");
>> + return NULL;
>> + }
>
> No, wrong again. The txn must be prepared and then on submit added to a
> queue. You are writing a driver for dmaengine, surely you dont expect
> the channel to be free and then do a txn.. that would be very
> inefficient!
I did not realize that the flow could be as you mentioned. The documentation I
read about the subsystem did not give me this idea. Thank you for clarifying me.
>
>> +static struct dma_async_tx_descriptor *
>> +dw_edma_device_prep_dma_cyclic(struct dma_chan *dchan, dma_addr_t buf_addr,
>> + size_t len, size_t cyclic_cnt,
>> + enum dma_transfer_direction direction,
>> + unsigned long flags)
>> +{
>> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> + struct dw_edma_desc *desc;
>> + struct dw_edma_chunk *chunk;
>> + struct dw_edma_burst *burst;
>> + unsigned long sflags;
>> + phys_addr_t src_addr;
>> + phys_addr_t dst_addr;
>> + u32 i;
>> +
>> + if (!len || !cyclic_cnt) {
>> + dev_err(chan2dev(chan), "invalid len or cyclic count\n");
>> + return NULL;
>> + }
>> +
>> + if (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
>> + dev_dbg(chan2dev(chan), "prepare operation (WRITE)\n");
>> + } else if (direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
>> + dev_dbg(chan2dev(chan), "prepare operation (READ)\n");
>> + } else {
>> + dev_err(chan2dev(chan), "invalid direction\n");
>> + return NULL;
>> + }
>> +
>> + if (!chan->configured) {
>> + dev_err(chan2dev(chan), "(prep_dma_cyclic) channel not configured\n");
>> + return NULL;
>> + }
>> +
>> + if (chan->status != EDMA_ST_IDLE) {
>> + dev_err(chan2dev(chan), "channel is busy or paused\n");
>> + return NULL;
>> + }
>> +
>> + spin_lock_irqsave(&chan->vc.lock, sflags);
>> +
>> + desc = dw_edma_alloc_desc(chan);
>> + if (unlikely(!desc))
>> + goto err_alloc;
>> +
>> + chunk = dw_edma_alloc_chunk(desc);
>> + if (unlikely(!chunk))
>> + goto err_alloc;
>> +
>> + src_addr = chan->src_addr;
>> + dst_addr = chan->dst_addr;
>> +
>> + for (i = 0; i < cyclic_cnt; i++) {
>> + if (chunk->bursts_alloc == chan->ll_max) {
>> + chunk = dw_edma_alloc_chunk(desc);
>> + if (unlikely(!chunk))
>> + goto err_alloc;
>> + }
>> +
>> + burst = dw_edma_alloc_burst(chunk);
>> +
>> + if (unlikely(!burst))
>> + goto err_alloc;
>> +
>> + burst->sz = len;
>> + chunk->ll_region.sz += burst->sz;
>> + desc->alloc_sz += burst->sz;
>> +
>> + burst->sar = src_addr;
>> + burst->dar = dst_addr;
>> +
>> + dev_dbg(chan2dev(chan), "lli %u/%u, sar=0x%.8llx, dar=0x%.8llx, size=%u bytes\n",
>> + i + 1, cyclic_cnt, burst->sar, burst->dar, burst->sz);
>> + }
>> +
>> + spin_unlock_irqrestore(&chan->vc.lock, sflags);
>> + return vchan_tx_prep(&chan->vc, &desc->vd, flags);
>
> how is it different from previous? am sure we can reuse bits!
There are some differences, but I agree I can rewrite the code of this and
previous function in order to reuse code between them. This function was a last
minute added feature :).
>
>> +static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>> +{
>> + struct dw_edma *dw = chan->chip->dw;
>> + const struct dw_edma_core_ops *ops = dw->ops;
>> + struct virt_dma_desc *vd;
>> + struct dw_edma_desc *desc;
>> + unsigned long flags;
>> +
>> + ops->clear_done_int(chan);
>> + dev_dbg(chan2dev(chan), "clear done interrupt\n");
>> +
>> + spin_lock_irqsave(&chan->vc.lock, flags);
>> + vd = vchan_next_desc(&chan->vc);
>> + switch (chan->request) {
>> + case EDMA_REQ_NONE:
>> + if (!vd)
>> + break;
>> +
>> + desc = vd2dw_edma_desc(vd);
>> + if (desc->chunks_alloc) {
>> + dev_dbg(chan2dev(chan), "sub-transfer complete\n");
>> + chan->status = EDMA_ST_BUSY;
>> + dev_dbg(chan2dev(chan), "transferred %u bytes\n",
>> + desc->xfer_sz);
>> + dw_edma_start_transfer(chan);
>> + } else {
>> + list_del(&vd->node);
>> + vchan_cookie_complete(vd);
>> + chan->status = EDMA_ST_IDLE;
>> + dev_dbg(chan2dev(chan), "transfer complete\n");
>> + }
>> + break;
>> + case EDMA_REQ_STOP:
>> + if (!vd)
>> + break;
>> +
>> + list_del(&vd->node);
>> + vchan_cookie_complete(vd);
>> + chan->request = EDMA_REQ_NONE;
>> + chan->status = EDMA_ST_IDLE;
>> + chan->configured = false;
>
> why is configuration deemed invalid, it can still be used again!
At the time I thought, that was necessary to reconfigure every time that we
required to do a new transfer. You already explained to me that is not
necessary. My bad!
>
>> +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);
>> +
>> + /* Callback operation selection accordingly to eDMA version */
>> + switch (dw->version) {
>> + default:
>> + dev_err(dev, "unsupported version\n");
>> + return -EPERM;
>> + }
>
> So we have only one case which returns error, was this code tested?
Yes it was, but I understand what your point of view.
This was done like this, because I wanna to segment the patch series like this:
1) Adding eDMA driver core, which contains the driver skeleton and the whole
logic associated.
2) and 3) Adding the callbacks for the eDMA register mapping version 0 (it will
appear in the future a new version, I thought that this new version would came
while I was trying to get the feedback about this patch series, therefore would
have another 2 patches for the version 1 isolated and independent from the
version 0).
4) and 5) Adding the PCIe glue-logic and device ID associated.
6) Adding maintainer for this driver.
7) Adding a test driver.
Since this switch will only have the associated case on patch 2, that why on
patch 1 doesn't appear any possibility.
If you feel logic to squash patch 2 with patch 1, just say something and I'll do
it for you :)
>
>> +
>> + pm_runtime_get_sync(dev);
>> +
>> + /* 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);
>
> you may use struct_size() here
Hum, this would be useful if I wanted to allocate the dw struct as well, right?
Since dw struct is already allocated, it looks like this can't be used, or am I
missing something?
>
>> + 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);
>
> this is similar to previous with obvious changes, I think this can be
> made common routine invoke for R and W ...
OK, I can rewrite the code to simplify this.
>
> So HW provides read channels and write channels and not generic channels
> which can be used for either R or W?
>
The HW, provides a well defined channels, with different registers sets.
Initially I thought to maskared it, but since the number of channels can be
client configurable [0..8] for each type of channel and also asymmetric (write
vs read), I prefer to kept then separated.
Once again, thanks Vinod!
^ permalink raw reply
* [v2,2/3] dma: imx-sdma: add clock ratio 1:1 check
From: Angus Ainslie @ 2019-01-21 14:43 UTC (permalink / raw)
To: Lucas Stach
Cc: linux-kernel, Vinod Koul, NXP Linux Team, Pengutronix Kernel Team,
dmaengine, linux-arm-kernel
Hi Lucas,
On 2019-01-21 02:17, Lucas Stach wrote:
> Hi Angus,
>
> Am Samstag, den 19.01.2019, 19:31 -0700 schrieb Angus Ainslie (Purism):
>> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
>> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
>> to 500Mhz, so use 1:1 instead.
>>
>> based on NXP commit MLK-16841-1
>>
>> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 +
>> drivers/dma/imx-sdma.c | 20
>> +++++++++++++++----
>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> index 3c9a57a8443b..17544c1820b7 100644
>> --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> @@ -67,6 +67,7 @@ Optional properties:
>> reg is the GPR register offset.
>> shift is the bit position inside the GPR register.
>> val is the value of the bit (0 or 1).
>> +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this.
>
> Why does this need a separate DT property? Shouldn't the driver be able
> to infer this from the clock rates going into the SDMA hardware?
>
That's probably a better way to do it then setting the property can't
get missed. I'll address it in v3.
Angus
> Regards,
> Lucas
>
>> Examples:
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index 0b3a67ff8e82..65dada21d3c1 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -440,6 +440,8 @@ struct sdma_engine {
>> > > unsigned int irq;
>> > > dma_addr_t bd0_phys;
>> > > struct sdma_buffer_descriptor *bd0;
>> > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
>> > > + bool clk_ratio;
>> };
>>
>> static int sdma_config_write(struct dma_chan *chan,
>> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine
>> *sdma)
>> > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>>
>> > /* Set bits of CONFIG register with dynamic context switching */
>> > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
>> > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
>> > + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) {
>> > + if (sdma->clk_ratio)
>> > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR;
>> > + else
>> > + reg = SDMA_H_CONFIG_CSM;
>> +
>> > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
>> > + }
>>
>> > return ret;
>> }
>> @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma)
>> > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
>>
>> > /* Set bits of CONFIG register but with static context switching */
>> > - /* FIXME: Check whether to set ACR bit depending on clock ratios */
>> > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>> > + if (sdma->clk_ratio)
>> > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG);
>> > + else
>> > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>>
>> > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
>>
>> @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device
>> *pdev)
>> > if (!sdma)
>> > return -ENOMEM;
>>
>> > + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1");
>> +
>> > spin_lock_init(&sdma->channel_0_lock);
>>
>> > sdma->dev = &pdev->dev;
^ permalink raw reply
* dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Codrin Ciubotariu @ 2019-01-21 14:38 UTC (permalink / raw)
To: vkoul; +Cc: Ludovic.Desroches, dmaengine, linux-arm-kernel, linux-kernel
On 20.01.2019 13:04, Vinod Koul wrote:
> Hi Codrin,
>
> On 17-01-19, 16:10, Codrin.Ciubotariu@microchip.com wrote:
>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>
>> atchan->status is used for two things:
>> - pass channel interrupts status from interrupt handler to tasklet;
>> - channel information like whether it is cyclic or paused;
>>
>> Since these operations have nothing in common, this patch adds a
>> different struct member to keep the interrupts status.
>>
>> Fixes a bug in which a channel is wrongfully reported as in use when
>> trying to obtain a new descriptior for a previously used cyclic channel.
>
> This looks reasonable but am unable to see how the bug is fixed. Perhaps
> would be great to split the bug part (which can go to fixes) and remove
> the reuse of variable as a subsequent patch..
Hi Vinod,
This patch is the fix, since it moves the operations on atchan->status,
in which the interrupt status register is saved, to a different member
(irq_status). The AT_XDMAC_CHAN_IS_CYCLIC and AT_XDMAC_CHAN_IS_PAUSED
bits have nothing in common with the interrupt status register.
The bug reproduces when a device_terminate_all() is called,
(AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
a new descriptor for a cyclic transfer is created, the driver reports
the channel as in use:
if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
dev_err(chan2dev(chan), "channel currently used\n");
return NULL;
}
I can send v2 if you consider the commit message misleading.
Best regards,
Codrin
>
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> ---
>> drivers/dma/at_xdmac.c | 19 ++++++++++---------
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
>> index 4e557684f792..fe69dccfa0c0 100644
>> --- a/drivers/dma/at_xdmac.c
>> +++ b/drivers/dma/at_xdmac.c
>> @@ -203,6 +203,7 @@ struct at_xdmac_chan {
>> u32 save_cim;
>> u32 save_cnda;
>> u32 save_cndc;
>> + u32 irq_status;
>> unsigned long status;
>> struct tasklet_struct tasklet;
>> struct dma_slave_config sconfig;
>> @@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
>> struct at_xdmac_desc *desc;
>> u32 error_mask;
>>
>> - dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
>> - __func__, atchan->status);
>> + dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
>> + __func__, atchan->irq_status);
>>
>> error_mask = AT_XDMAC_CIS_RBEIS
>> | AT_XDMAC_CIS_WBEIS
>> @@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
>>
>> if (at_xdmac_chan_is_cyclic(atchan)) {
>> at_xdmac_handle_cyclic(atchan);
>> - } else if ((atchan->status & AT_XDMAC_CIS_LIS)
>> - || (atchan->status & error_mask)) {
>> + } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
>> + || (atchan->irq_status & error_mask)) {
>> struct dma_async_tx_descriptor *txd;
>>
>> - if (atchan->status & AT_XDMAC_CIS_RBEIS)
>> + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
>> dev_err(chan2dev(&atchan->chan), "read bus error!!!");
>> - if (atchan->status & AT_XDMAC_CIS_WBEIS)
>> + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
>> dev_err(chan2dev(&atchan->chan), "write bus error!!!");
>> - if (atchan->status & AT_XDMAC_CIS_ROIS)
>> + if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
>> dev_err(chan2dev(&atchan->chan), "request overflow error!!!");
>>
>> spin_lock(&atchan->lock);
>> @@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
>> atchan = &atxdmac->chan[i];
>> chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
>> chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
>> - atchan->status = chan_status & chan_imr;
>> + atchan->irq_status = chan_status & chan_imr;
>> dev_vdbg(atxdmac->dma.dev,
>> "%s: chan%d: imr=0x%x, status=0x%x\n",
>> __func__, i, chan_imr, chan_status);
>> @@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
>> at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
>> at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
>>
>> - if (atchan->status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
>> + if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
>> at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask);
>>
>> tasklet_schedule(&atchan->tasklet);
>> --
>> 2.17.1
>
^ permalink raw reply
* [v10,3/3] dt-bindings: dma: uart: rename binding
From: Rob Herring @ 2019-01-21 14:18 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Nicolas Boichat, Matthias Brugger, 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
On Fri, 18 Jan 2019 11:10:16 +0800, Long Cheng wrote:
> The filename matches mtk-uart-apdma.c.
> So using "mtk-uart-apdma.txt" should be better.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> .../devicetree/bindings/dma/8250_mtk_dma.txt | 33 --------------------
> .../devicetree/bindings/dma/mtk-uart-apdma.txt | 33 ++++++++++++++++++++
> 2 files changed, 33 insertions(+), 33 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
> create mode 100644 Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* dmaengine: fsl-edma: dma map slave device address
From: Laurentiu Tudor @ 2019-01-21 14:13 UTC (permalink / raw)
To: Angelo Dureghello
Cc: vkoul@kernel.org, dmaengine@vger.kernel.org, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
robin.murphy@arm.com
Hi Angelo,
On 18.01.2019 23:50, Angelo Dureghello wrote:
> Hi Laurentiu,
>
> On Fri, Jan 18, 2019 at 12:06:23PM +0200, Laurentiu Tudor wrote:
>> This mapping needs to be created in order for slave dma transfers
>> to work on systems with SMMU. The implementation mostly mimics the
>> one in pl330 dma driver, authored by Robin Murphy.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> Original approach was to add the missing mappings in the i2c client driver,
>> see here for discussion: https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026013%2F&data=02%7C01%7Claurentiu.tudor%40nxp.com%7C7861dfe95dfb4fceeb8208d67d907488%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636834456718898365&sdata=XM5shQdcIRgFLtCmuRuFtViR6ttPDWI%2BNHXoPi68Xs8%3D&reserved=0
>>
>> drivers/dma/fsl-edma-common.c | 66 ++++++++++++++++++++++++++++++++---
>> drivers/dma/fsl-edma-common.h | 4 +++
>> drivers/dma/fsl-edma.c | 1 +
>> drivers/dma/mcf-edma.c | 1 +
>> 4 files changed, 68 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
>> index 8876c4c1bb2c..0e95ee24b6d4 100644
>> --- a/drivers/dma/fsl-edma-common.c
>> +++ b/drivers/dma/fsl-edma-common.c
>> @@ -6,6 +6,7 @@
>> #include <linux/dmapool.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> +#include <linux/dma-mapping.h>
>>
>> #include "fsl-edma-common.h"
>>
>> @@ -173,12 +174,62 @@ int fsl_edma_resume(struct dma_chan *chan)
>> }
>> EXPORT_SYMBOL_GPL(fsl_edma_resume);
>>
>> +static void fsl_edma_unprep_slave_dma(struct fsl_edma_chan *fsl_chan)
>> +{
>> + if (fsl_chan->dma_dir != DMA_NONE)
>> + dma_unmap_resource(fsl_chan->vchan.chan.device->dev,
>> + fsl_chan->dma_dev_addr,
>> + fsl_chan->dma_dev_size,
>> + fsl_chan->dma_dir, 0);
>> + fsl_chan->dma_dir = DMA_NONE;
>> +}
>> +
>> +static bool fsl_edma_prep_slave_dma(struct fsl_edma_chan *fsl_chan,
>> + enum dma_transfer_direction dir)
>> +{
>> + struct device *dev = fsl_chan->vchan.chan.device->dev;
>> + enum dma_data_direction dma_dir;
>> + phys_addr_t addr = 0;
>> + u32 size = 0;
>> +
>> + switch (dir) {
>> + case DMA_MEM_TO_DEV:
>> + dma_dir = DMA_FROM_DEVICE;
>> + addr = fsl_chan->cfg.dst_addr;
>> + size = fsl_chan->cfg.dst_maxburst;
>> + break;
>> + case DMA_DEV_TO_MEM:
>> + dma_dir = DMA_TO_DEVICE;
>> + addr = fsl_chan->cfg.src_addr;
>> + size = fsl_chan->cfg.src_maxburst;
>> + break;
>> + default:
>> + dma_dir = DMA_NONE;
>> + break;
>> + }
>> +
>> + /* Already mapped for this config? */
>> + if (fsl_chan->dma_dir == dma_dir)
>> + return true;
>> +
>> + fsl_edma_unprep_slave_dma(fsl_chan);
>> +
>> + fsl_chan->dma_dev_addr = dma_map_resource(dev, addr, size, dma_dir, 0);
>> + if (dma_mapping_error(dev, fsl_chan->dma_dev_addr))
>> + return false;
>> + fsl_chan->dma_dev_size = size;
>> + fsl_chan->dma_dir = dma_dir;
>> +
>> + return true;
>> +}
>> +
>> int fsl_edma_slave_config(struct dma_chan *chan,
>> struct dma_slave_config *cfg)
>> {
>> struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
>>
>> memcpy(&fsl_chan->cfg, cfg, sizeof(*cfg));
>> + fsl_edma_unprep_slave_dma(fsl_chan);
>>
>> return 0;
>> }
>> @@ -378,6 +429,9 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
>> if (!is_slave_direction(direction))
>> return NULL;
>>
>> + if (!fsl_edma_prep_slave_dma(fsl_chan, direction))
>> + return NULL;
>> +
>> sg_len = buf_len / period_len;
>> fsl_desc = fsl_edma_alloc_desc(fsl_chan, sg_len);
>> if (!fsl_desc)
>> @@ -409,11 +463,11 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
>>
>> if (direction == DMA_MEM_TO_DEV) {
>> src_addr = dma_buf_next;
>> - dst_addr = fsl_chan->cfg.dst_addr;
>> + dst_addr = fsl_chan->dma_dev_addr;
>> soff = fsl_chan->cfg.dst_addr_width;
>> doff = 0;
>> } else {
>> - src_addr = fsl_chan->cfg.src_addr;
>> + src_addr = fsl_chan->dma_dev_addr;
>> dst_addr = dma_buf_next;
>> soff = 0;
>> doff = fsl_chan->cfg.src_addr_width;
>> @@ -444,6 +498,9 @@ struct dma_async_tx_descriptor *fsl_edma_prep_slave_sg(
>> if (!is_slave_direction(direction))
>> return NULL;
>>
>> + if (!fsl_edma_prep_slave_dma(fsl_chan, direction))
>> + return NULL;
>> +
>> fsl_desc = fsl_edma_alloc_desc(fsl_chan, sg_len);
>> if (!fsl_desc)
>> return NULL;
>> @@ -468,11 +525,11 @@ struct dma_async_tx_descriptor *fsl_edma_prep_slave_sg(
>>
>> if (direction == DMA_MEM_TO_DEV) {
>> src_addr = sg_dma_address(sg);
>> - dst_addr = fsl_chan->cfg.dst_addr;
>> + dst_addr = fsl_chan->dma_dev_addr;
>> soff = fsl_chan->cfg.dst_addr_width;
>> doff = 0;
>> } else {
>> - src_addr = fsl_chan->cfg.src_addr;
>> + src_addr = fsl_chan->dma_dev_addr;
>> dst_addr = sg_dma_address(sg);
>> soff = 0;
>> doff = fsl_chan->cfg.src_addr_width;
>> @@ -555,6 +612,7 @@ void fsl_edma_free_chan_resources(struct dma_chan *chan)
>> fsl_edma_chan_mux(fsl_chan, 0, false);
>> fsl_chan->edesc = NULL;
>> vchan_get_all_descriptors(&fsl_chan->vchan, &head);
>> + fsl_edma_unprep_slave_dma(fsl_chan);
>> spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
>>
>> vchan_dma_desc_free_list(&fsl_chan->vchan, &head);
>> diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h
>> index 8917e8865959..b435d8e1e3a1 100644
>> --- a/drivers/dma/fsl-edma-common.h
>> +++ b/drivers/dma/fsl-edma-common.h
>> @@ -6,6 +6,7 @@
>> #ifndef _FSL_EDMA_COMMON_H_
>> #define _FSL_EDMA_COMMON_H_
>>
>> +#include <linux/dma-direction.h>
>> #include "virt-dma.h"
>>
>> #define EDMA_CR_EDBG BIT(1)
>> @@ -120,6 +121,9 @@ struct fsl_edma_chan {
>> struct dma_slave_config cfg;
>> u32 attr;
>> struct dma_pool *tcd_pool;
>> + dma_addr_t dma_dev_addr;
>> + u32 dma_dev_size;
>> + enum dma_data_direction dma_dir;
>> };
>>
>> struct fsl_edma_desc {
>> diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c
>> index 34d70112fcc9..75e8a7ba3a22 100644
>> --- a/drivers/dma/fsl-edma.c
>> +++ b/drivers/dma/fsl-edma.c
>> @@ -254,6 +254,7 @@ static int fsl_edma_probe(struct platform_device *pdev)
>> fsl_chan->pm_state = RUNNING;
>> fsl_chan->slave_id = 0;
>> fsl_chan->idle = true;
>> + fsl_chan->dma_dir = DMA_NONE;
>> fsl_chan->vchan.desc_free = fsl_edma_free_desc;
>> vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev);
>>
>> diff --git a/drivers/dma/mcf-edma.c b/drivers/dma/mcf-edma.c
>> index 5de1b07eddff..7de54b2fafdb 100644
>> --- a/drivers/dma/mcf-edma.c
>> +++ b/drivers/dma/mcf-edma.c
>> @@ -214,6 +214,7 @@ static int mcf_edma_probe(struct platform_device *pdev)
>> mcf_chan->edma = mcf_edma;
>> mcf_chan->slave_id = i;
>> mcf_chan->idle = true;
>> + mcf_chan->dma_dir = DMA_NONE;
>> mcf_chan->vchan.desc_free = fsl_edma_free_desc;
>> vchan_init(&mcf_chan->vchan, &mcf_edma->dma_dev);
>> iowrite32(0x0, ®s->tcd[i].csr);
>> --
>> 2.17.1
>>
>
> I tested this patch on:
>
> - Vybrid VF50N (Toradex Colibri VF50)
> - ColdFire mcf54415 (Sysam stmark2 board)
>
> and dma still works properly.
>
> Tested-by: Angelo Dureghello <angelo@sysam.it>
>
Thanks a lot for testing!
---
Best Regards, Laurentiu
^ permalink raw reply
* dma: imx: Fix arm64 compilation issue due to min() size issue
From: Andre Przywara @ 2019-01-21 11:26 UTC (permalink / raw)
To: Vinod Koul
Cc: dmaengine, linux-arm-kernel, linux-kernel, Lucas Stach,
Baruch Siach
Compiling the i.MX DMA driver for arm64 results in a warning due to
imxdma_desc->len being a size_t, while scatterlist->len being an uint:
drivers/dma/imx-dma.c: In function ‘imxdma_sg_next’:
/src/linux/include/linux/kernel.h:846:29: warning: comparison of distinct pointer types lacks a cast
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
...
drivers/dma/imx-dma.c:288:8: note: in expansion of macro ‘min’
now = min(d->len, sg_dma_len(sg));
^~~
I am not sure if size_t is the right type for len in the first place,
but anyway the fix is pretty simple.
This fixes compiling an arm64 kernel with i.MX (DMA) support enabled.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,
apologies if this has been sent already (just ignore it then), but I
couldn't find anything quickly enough. I see this since -rc1, but it's
still annoying me in -rc3, hence this patch.
Cheers,
Andre.
drivers/dma/imx-dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index c2fff3f6c9ca..fa3ef43fe314 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -285,7 +285,7 @@ static inline int imxdma_sg_next(struct imxdma_desc *d)
struct scatterlist *sg = d->sg;
unsigned long now;
- now = min(d->len, sg_dma_len(sg));
+ now = min_t(size_t, d->len, sg_dma_len(sg));
if (d->len != IMX_DMA_LENGTH_LOOP)
d->len -= now;
^ permalink raw reply related
* [RFC,v3,5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic
From: Gustavo Pimentel @ 2019-01-21 9:21 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 19/01/2019 15:45, Andy Shevchenko wrote:
> On Tue, Jan 15, 2019 at 12:48:34PM +0000, Gustavo Pimentel wrote:
>> 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.
>
> Keep out-of-tree patch for your needs.
Ok.
>
>>>>>> + 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?
>
> Not adding, removing. That's what I put before.
I thought there was some API or code design to force that. Sorry my bad.
>
>>
>>> 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.
>
^ permalink raw reply
* [v2,2/3] dma: imx-sdma: add clock ratio 1:1 check
From: Lucas Stach @ 2019-01-21 9:17 UTC (permalink / raw)
To: Angus Ainslie (Purism)
Cc: linux-kernel, Vinod Koul, NXP Linux Team, Pengutronix Kernel Team,
dmaengine, linux-arm-kernel
Hi Angus,
Am Samstag, den 19.01.2019, 19:31 -0700 schrieb Angus Ainslie (Purism):
> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
> to 500Mhz, so use 1:1 instead.
>
> based on NXP commit MLK-16841-1
>
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 +
> drivers/dma/imx-sdma.c | 20 +++++++++++++++----
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
> index 3c9a57a8443b..17544c1820b7 100644
> --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
> @@ -67,6 +67,7 @@ Optional properties:
> reg is the GPR register offset.
> shift is the bit position inside the GPR register.
> val is the value of the bit (0 or 1).
> +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this.
Why does this need a separate DT property? Shouldn't the driver be able
to infer this from the clock rates going into the SDMA hardware?
Regards,
Lucas
> Examples:
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 0b3a67ff8e82..65dada21d3c1 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -440,6 +440,8 @@ struct sdma_engine {
> > > unsigned int irq;
> > > dma_addr_t bd0_phys;
> > > struct sdma_buffer_descriptor *bd0;
> > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
> > > + bool clk_ratio;
> };
>
> static int sdma_config_write(struct dma_chan *chan,
> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma)
> > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>
> > /* Set bits of CONFIG register with dynamic context switching */
> > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
> > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
> > + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) {
> > + if (sdma->clk_ratio)
> > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR;
> > + else
> > + reg = SDMA_H_CONFIG_CSM;
> +
> > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
> > + }
>
> > return ret;
> }
> @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma)
> > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
>
> > /* Set bits of CONFIG register but with static context switching */
> > - /* FIXME: Check whether to set ACR bit depending on clock ratios */
> > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
> > + if (sdma->clk_ratio)
> > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG);
> > + else
> > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>
> > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
>
> @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device *pdev)
> > if (!sdma)
> > return -ENOMEM;
>
> > + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1");
> +
> > spin_lock_init(&sdma->channel_0_lock);
>
> > sdma->dev = &pdev->dev;
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Gustavo Pimentel @ 2019-01-21 9:14 UTC (permalink / raw)
To: Andy Shevchenko, Gustavo Pimentel
Cc: Jose Abreu, linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Vinod Koul, Dan Williams, Eugeniy Paltsev, Russell King,
Niklas Cassel, Joao Pinto, Luis de Oliveira, Vitor Soares,
Nelson Costa, Pedro Sousa
On 19/01/2019 16:21, Andy Shevchenko wrote:
> On Wed, Jan 16, 2019 at 11:53:00AM +0000, Gustavo Pimentel wrote:
>> On 16/01/2019 10:21, Jose Abreu wrote:
>>> On 1/11/2019 6:33 PM, Gustavo Pimentel wrote:
>
>>>> + /* Free irqs */
>>>
>>> But devm will automatically free it, no ?
>>
>> Yes, it shouldn't be there. I'll remove it.
>
> It depends. If the driver is using tasklets it must free it explicitly like this.
In this driver I'm not using tasklets.
>
> P.S. devm_*_irq() is quite doubtful to have in the first place.
For curiosity, why?
>
>
^ permalink raw reply
* [v9,1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Long Cheng @ 2019-01-21 8:12 UTC (permalink / raw)
To: Vinod Koul
Cc: Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee, Sean Wang,
Nicolas Boichat, Matthias Brugger, 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
On Sun, 2019-01-20 at 10:57 +0530, Vinod Koul wrote:
> On 10-01-19, 18:33, Long Cheng wrote:
> > On Fri, 2019-01-04 at 22:49 +0530, Vinod Koul wrote:
> > > On 02-01-19, 10:12, Long Cheng wrote:
> > > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > > the performance, can enable the function.
> > >
> > > Is the DMA controller UART specific, can it work with other controllers
> > > as well, if so you should get rid of uart name in patch
> >
> > I don't know that it can work or not on other controller. but it's for
> > MediaTek SOC
>
> What I meant was that if can work with other controllers (users) apart
> from UART, how about say audio, spi etc!!
>
it's just for UART APDMA. can't work on spi ...
> >
> > > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> > >
> > > Why are the channels not coming from DT?
> > >
> >
> > i will using dma-requests install of it.
> >
> > > > +
> > > > +#define VFF_EN_B BIT(0)
> > > > +#define VFF_STOP_B BIT(0)
> > > > +#define VFF_FLUSH_B BIT(0)
> > > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> > >
> > > space around /* space */ also run checkpatch to check for style errors
> > >
> >
> > ok.
> >
> > > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > > +{
> > > > + unsigned int len, send, left, wpt, d_wpt, tmp;
> > > > + int ret;
> > > > +
> > > > + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> > > > + if (!left) {
> > > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > > + return;
> > > > + }
> > > > +
> > > > + /* Wait 1sec for flush, can't sleep*/
> > > > + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> > > > + tmp != VFF_FLUSH_B, 0, 1000000);
> > > > + if (ret)
> > > > + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> > > > + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > > > +
> > > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > > + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> > > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > > +
> > > > + d_wpt = wpt + send;
> > > > + if ((d_wpt & VFF_RING_SIZE) >= len) {
> > > > + d_wpt = d_wpt - len;
> > > > + d_wpt = d_wpt ^ VFF_RING_WRAP;
> > > > + }
> > > > + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> > > > +
> > > > + c->desc->avail_len -= send;
> > > > +
> > > > + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > > > + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> > > > + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > > +{
> > > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > > + unsigned int len, wg, rg, cnt;
> > > > +
> > > > + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> > > > + !d || !vchan_next_desc(&c->vc))
> > > > + return;
> > > > +
> > > > + len = mtk_uart_apdma_read(c, VFF_LEN);
> > > > + rg = mtk_uart_apdma_read(c, VFF_RPT);
> > > > + wg = mtk_uart_apdma_read(c, VFF_WPT);
> > > > + if ((rg ^ wg) & VFF_RING_WRAP)
> > > > + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> > > > + else
> > > > + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> > > > +
> > > > + c->rx_status = cnt;
> > > > + mtk_uart_apdma_write(c, VFF_RPT, wg);
> > > > +
> > > > + list_del(&d->vd.node);
> > > > + vchan_cookie_complete(&d->vd);
> > > > +}
> > >
> > > this looks odd, why do you have different rx and tx start routines?
> > >
> >
> > Would you like explain it in more detail? thanks.
> > In tx function, will wait the last data flush done. and the count the
> > size that send.
> > In Rx function, will count the size that receive.
> > Any way, in rx / tx, need andle WPT or RPT.
> >
> > > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + u32 tmp;
> > > > + int ret;
> > > > +
> > > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > > +
> > > > + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> > > > + mtk_uart_apdma_write(c, VFF_THRE, 0);
> > > > + mtk_uart_apdma_write(c, VFF_LEN, 0);
> > > > + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> > > > +
> > > > + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> > > > + tmp == 0, 10, 100);
> > > > + if (ret) {
> > > > + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> > > > + return ret;
> > > > + }
> > >
> > > register read does reset?
> > >
> >
> > 'mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);' is reset. resd just
> > poll reset done.
> >
> > > > +
> > > > + if (!c->requested) {
> > > > + c->requested = true;
> > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> > > > + KBUILD_MODNAME, chan);
> > >
> > > why is the irq not requested in driver probe?
> > >
> >
> > I have explained in below,
> > http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016418.html
> >
> > > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > > + dma_cookie_t cookie,
> > > > + struct dma_tx_state *txstate)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + enum dma_status ret;
> > > > + unsigned long flags;
> > > > +
> > > > + if (!txstate)
> > > > + return DMA_ERROR;
> > > > +
> > > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + if (ret == DMA_IN_PROGRESS) {
> > > > + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> > > > + dma_set_residue(txstate, c->rx_status);
> > > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > >
> > > why set reside when it is complete? also reside can be null, that should
> > > be checked as well
> > >
> > In different status, set different reside.
>
> Can you explain that..
>
RPT is different every time. When RX interrupt coming, update the
rx_status and notify uart that there have data. One transfer is
complete. Uart will call tx_status to get the length at the time of
interruption. Other case, if want to get the tx_status, directly read
register.
> >
> > > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > > + unsigned int sglen, enum dma_transfer_direction dir,
> > > > + unsigned long tx_flags, void *context)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdma_desc *d;
> > > > +
> > > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > > + (dir != DMA_MEM_TO_DEV)) {
> > > > + dev_err(chan->device->dev, "bad direction\n");
> > > > + return NULL;
> > > > + }
> > >
> > > we have a macro for this
> >
> > Thanks for your suggestion. i will modify it.
> > >
> > > > +
> > > > + /* Now allocate and setup the descriptor */
> > > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > > + if (!d)
> > > > + return NULL;
> > > > +
> > > > + /* sglen is 1 */
> > >
> > > ?
dmaengine_prep_slave_single is called. the parameter 'sglen' is 1.
Add annotation to indicate that the total length is sg_dma_len(sgl), no
more.
> > >
> > > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > > + struct dma_slave_config *cfg)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdmadev *mtkd =
> > > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > > +
> > > > + c->cfg = *cfg;
> > > > +
> > > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > >
> > > fg->direction is deprecated, in fact I have removed all users recently,
> > > please do not use this
> >
> > You will remove 'direction' in 'struct dma_slave_config'? if remove, how
> > do confirm direction?
>
> Yes please use the direction argument in prep_xx calls
I had modified on v10. thanks.
^ permalink raw reply
* dmaengine: dmatest: wrap src & dst data into a struct
From: Alexandru Ardelean @ 2019-01-21 7:54 UTC (permalink / raw)
To: vkoul@kernel.org; +Cc: dmaengine@vger.kernel.org
On Sat, 2019-01-19 at 18:37 +0530, Vinod Koul wrote:
>
>
> Hi Alexandru,
>
> On 26-11-18, 10:43, Alexandru Ardelean wrote:
> > There's a bit too much un-wrapped & duplicated code that can be
> > organized
> > into some common logic/functions.
> >
> > This change wraps all the common parts between srcs & dsts into a
> > `dmatest_data` struct. The purpose is to split the `dmatestfunc` into
> > smaller chunks that are easier to follow & extend.
>
> Thanks for the patch, this looks good but somehow seems to have slipped
> by..
Hey,
Replies inline
>
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/dma/dmatest.c | 257 ++++++++++++++++++++++--------------------
> > 1 file changed, 133 insertions(+), 124 deletions(-)
> >
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index e71aa1e3451c..c37c643e7d29 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -166,15 +166,20 @@ struct dmatest_done {
> > wait_queue_head_t *wait;
> > };
> >
> > +struct dmatest_data {
> > + u8 **raw;
> > + u8 **aligned;
> > + unsigned int cnt;
> > + unsigned int off;
> > +};
> > +
> > struct dmatest_thread {
> > struct list_head node;
> > struct dmatest_info *info;
> > struct task_struct *task;
> > struct dma_chan *chan;
> > - u8 **srcs;
> > - u8 **usrcs;
> > - u8 **dsts;
> > - u8 **udsts;
> > + struct dmatest_data src;
> > + struct dmatest_data dst;
> > enum dma_transaction_type type;
> > wait_queue_head_t done_wait;
> > struct dmatest_done test_done;
> > @@ -428,6 +433,53 @@ static unsigned long long dmatest_KBs(s64 runtime,
> > unsigned long long len)
> > return dmatest_persec(runtime, len >> 10);
> > }
> >
> > +static void __dmatest_free_test_data(struct dmatest_data *d, unsigned
> > int cnt)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < cnt; i++)
> > + kfree(d->raw[i]);
> > +
> > + kfree(d->aligned);
> > + kfree(d->raw);
> > +}
> > +
> > +static void dmatest_free_test_data(struct dmatest_data *d)
> > +{
> > + __dmatest_free_test_data(d, d->cnt);
> > +}
>
> why do we need a wrapper here?
see <comment1> below
>
> > +
> > +static int dmatest_alloc_test_data(struct dmatest_data *d,
> > + unsigned int buf_size, u8 align)
> > +{
> > + unsigned int i = 0;
>
> superfluous initialization
ack, will remove
>
> > + d->raw = kcalloc(d->cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > + if (!d->raw)
> > + return -ENOMEM;
> > +
> > + d->aligned = kcalloc(d->cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > + if (!d->aligned)
> > + goto err;
> > +
> > + for (i = 0; i < d->cnt; i++) {
> > + d->raw[i] = kmalloc(buf_size + align, GFP_KERNEL);
> > + if (!d->raw[i])
> > + goto err;
> > +
> > + /* align to alignment restriction */
> > + if (align)
> > + d->aligned[i] = PTR_ALIGN(d->raw[i], align);
> > + else
> > + d->aligned[i] = d->raw[i];
> > + }
> > +
> > + return 0;
> > +err:
> > + __dmatest_free_test_data(d, i);
<comment1>: the __dmatest_free_test_data() will be used with the `i` index
here in case a kmalloc() has failed while allocating `d->cnt` memory
locations
> > + return -ENOMEM;
> > +}
> > +
> > /*
> > * This function repeatedly tests DMA transfers of various lengths and
> > * offsets for a given operation type until it is told to exit by
> > @@ -458,8 +510,9 @@ static int dmatest_func(void *data)
> > enum dma_ctrl_flags flags;
> > u8 *pq_coefs = NULL;
> > int ret;
> > - int src_cnt;
> > - int dst_cnt;
> > + unsigned int buf_size;
> > + struct dmatest_data *src;
> > + struct dmatest_data *dst;
> > int i;
> > ktime_t ktime, start, diff;
> > ktime_t filltime = 0;
> > @@ -480,99 +533,64 @@ static int dmatest_func(void *data)
> > params = &info->params;
> > chan = thread->chan;
> > dev = chan->device;
> > + src = &thread->src;
> > + dst = &thread->dst;
> > if (thread->type == DMA_MEMCPY) {
> > align = dev->copy_align;
> > - src_cnt = dst_cnt = 1;
> > + src->cnt = dst->cnt = 1;
> > } else if (thread->type == DMA_MEMSET) {
> > align = dev->fill_align;
> > - src_cnt = dst_cnt = 1;
> > + src->cnt = dst->cnt = 1;
> > is_memset = true;
> > } else if (thread->type == DMA_XOR) {
> > /* force odd to ensure dst = src */
> > - src_cnt = min_odd(params->xor_sources | 1, dev->max_xor);
> > - dst_cnt = 1;
> > + src->cnt = min_odd(params->xor_sources | 1, dev-
> > >max_xor);
> > + dst->cnt = 1;
> > align = dev->xor_align;
> > } else if (thread->type == DMA_PQ) {
> > /* force odd to ensure dst = src */
> > - src_cnt = min_odd(params->pq_sources | 1, dma_maxpq(dev,
> > 0));
> > - dst_cnt = 2;
> > + src->cnt = min_odd(params->pq_sources | 1, dma_maxpq(dev,
> > 0));
> > + dst->cnt = 2;
> > align = dev->pq_align;
> >
> > pq_coefs = kmalloc(params->pq_sources + 1, GFP_KERNEL);
> > if (!pq_coefs)
> > goto err_thread_type;
> >
> > - for (i = 0; i < src_cnt; i++)
> > + for (i = 0; i < src->cnt; i++)
> > pq_coefs[i] = 1;
> > } else
> > goto err_thread_type;
> >
> > /* Check if buffer count fits into map count variable (u8) */
> > - if ((src_cnt + dst_cnt) >= 255) {
> > + if ((src->cnt + dst->cnt) >= 255) {
> > pr_err("too many buffers (%d of 255 supported)\n",
> > - src_cnt + dst_cnt);
> > + src->cnt + dst->cnt);
> > goto err_thread_type;
> > }
> >
> > + buf_size = params->buf_size;
> > if (1 << align > params->buf_size) {
> > pr_err("%u-byte buffer too small for %d-byte
> > alignment\n",
> > params->buf_size, 1 << align);
> > goto err_thread_type;
> > }
> >
> > - thread->srcs = kcalloc(src_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > - if (!thread->srcs)
> > - goto err_srcs;
> > -
> > - thread->usrcs = kcalloc(src_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > - if (!thread->usrcs)
> > - goto err_usrcs;
> > + if (dmatest_alloc_test_data(src, buf_size, align) < 0)
> > + goto err_src;
> >
> > - for (i = 0; i < src_cnt; i++) {
> > - thread->usrcs[i] = kmalloc(params->buf_size + align,
> > - GFP_KERNEL);
> > - if (!thread->usrcs[i])
> > - goto err_srcbuf;
> > -
> > - /* align srcs to alignment restriction */
> > - if (align)
> > - thread->srcs[i] = PTR_ALIGN(thread->usrcs[i],
> > align);
> > - else
> > - thread->srcs[i] = thread->usrcs[i];
> > - }
> > - thread->srcs[i] = NULL;
> > -
> > - thread->dsts = kcalloc(dst_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > - if (!thread->dsts)
> > - goto err_dsts;
> > -
> > - thread->udsts = kcalloc(dst_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> > - if (!thread->udsts)
> > - goto err_udsts;
> > -
> > - for (i = 0; i < dst_cnt; i++) {
> > - thread->udsts[i] = kmalloc(params->buf_size + align,
> > - GFP_KERNEL);
> > - if (!thread->udsts[i])
> > - goto err_dstbuf;
> > -
> > - /* align dsts to alignment restriction */
> > - if (align)
> > - thread->dsts[i] = PTR_ALIGN(thread->udsts[i],
> > align);
> > - else
> > - thread->dsts[i] = thread->udsts[i];
> > - }
> > - thread->dsts[i] = NULL;
> > + if (dmatest_alloc_test_data(dst, buf_size, align) < 0)
> > + goto err_dst;
> >
> > set_user_nice(current, 10);
> >
> > - srcs = kcalloc(src_cnt, sizeof(dma_addr_t), GFP_KERNEL);
> > + srcs = kcalloc(src->cnt, sizeof(dma_addr_t), GFP_KERNEL);
> > if (!srcs)
> > - goto err_dstbuf;
> > + goto err_srcs;
> >
> > - dma_pq = kcalloc(dst_cnt, sizeof(dma_addr_t), GFP_KERNEL);
> > + dma_pq = kcalloc(dst->cnt, sizeof(dma_addr_t), GFP_KERNEL);
> > if (!dma_pq)
> > - goto err_srcs_array;
> > + goto err_dma_pq;
> >
> > /*
> > * src and dst buffers are freed by ourselves below
> > @@ -585,7 +603,7 @@ static int dmatest_func(void *data)
> > struct dma_async_tx_descriptor *tx = NULL;
> > struct dmaengine_unmap_data *um;
> > dma_addr_t *dsts;
> > - unsigned int src_off, dst_off, len;
> > + unsigned int len;
> >
> > total_tests++;
> >
> > @@ -601,59 +619,59 @@ static int dmatest_func(void *data)
> > total_len += len;
> >
> > if (params->norandom) {
> > - src_off = 0;
> > - dst_off = 0;
> > + src->off = 0;
> > + dst->off = 0;
> > } else {
> > - src_off = dmatest_random() % (params->buf_size -
> > len + 1);
> > - dst_off = dmatest_random() % (params->buf_size -
> > len + 1);
> > + src->off = dmatest_random() % (params->buf_size -
> > len + 1);
> > + dst->off = dmatest_random() % (params->buf_size -
> > len + 1);
> >
> > - src_off = (src_off >> align) << align;
> > - dst_off = (dst_off >> align) << align;
> > + src->off = (src->off >> align) << align;
> > + dst->off = (dst->off >> align) << align;
> > }
> >
> > if (!params->noverify) {
> > start = ktime_get();
> > - dmatest_init_srcs(thread->srcs, src_off, len,
> > + dmatest_init_srcs(src->aligned, src->off, len,
> > params->buf_size, is_memset);
> > - dmatest_init_dsts(thread->dsts, dst_off, len,
> > + dmatest_init_dsts(dst->aligned, dst->off, len,
> > params->buf_size, is_memset);
> >
> > diff = ktime_sub(ktime_get(), start);
> > filltime = ktime_add(filltime, diff);
> > }
> >
> > - um = dmaengine_get_unmap_data(dev->dev, src_cnt +
> > dst_cnt,
> > + um = dmaengine_get_unmap_data(dev->dev, src->cnt + dst-
> > >cnt,
> > GFP_KERNEL);
> > if (!um) {
> > failed_tests++;
> > result("unmap data NULL", total_tests,
> > - src_off, dst_off, len, ret);
> > + src->off, dst->off, len, ret);
> > continue;
> > }
> >
> > um->len = params->buf_size;
> > - for (i = 0; i < src_cnt; i++) {
> > - void *buf = thread->srcs[i];
> > + for (i = 0; i < src->cnt; i++) {
> > + void *buf = src->aligned[i];
> > struct page *pg = virt_to_page(buf);
> > unsigned long pg_off = offset_in_page(buf);
> >
> > um->addr[i] = dma_map_page(dev->dev, pg, pg_off,
> > um->len,
> > DMA_TO_DEVICE);
> > - srcs[i] = um->addr[i] + src_off;
> > + srcs[i] = um->addr[i] + src->off;
> > ret = dma_mapping_error(dev->dev, um->addr[i]);
> > if (ret) {
> > dmaengine_unmap_put(um);
> > result("src mapping error", total_tests,
> > - src_off, dst_off, len, ret);
> > + src->off, dst->off, len, ret);
> > failed_tests++;
> > continue;
> > }
> > um->to_cnt++;
> > }
> > /* map with DMA_BIDIRECTIONAL to force
> > writeback/invalidate */
> > - dsts = &um->addr[src_cnt];
> > - for (i = 0; i < dst_cnt; i++) {
> > - void *buf = thread->dsts[i];
> > + dsts = &um->addr[src->cnt];
> > + for (i = 0; i < dst->cnt; i++) {
> > + void *buf = dst->aligned[i];
> > struct page *pg = virt_to_page(buf);
> > unsigned long pg_off = offset_in_page(buf);
> >
> > @@ -663,7 +681,7 @@ static int dmatest_func(void *data)
> > if (ret) {
> > dmaengine_unmap_put(um);
> > result("dst mapping error", total_tests,
> > - src_off, dst_off, len, ret);
> > + src->off, dst->off, len, ret);
> > failed_tests++;
> > continue;
> > }
> > @@ -672,30 +690,30 @@ static int dmatest_func(void *data)
> >
> > if (thread->type == DMA_MEMCPY)
> > tx = dev->device_prep_dma_memcpy(chan,
> > - dsts[0] +
> > dst_off,
> > + dsts[0] + dst-
> > >off,
> > srcs[0], len,
> > flags);
> > else if (thread->type == DMA_MEMSET)
> > tx = dev->device_prep_dma_memset(chan,
> > - dsts[0] + dst_off,
> > - *(thread->srcs[0] +
> > src_off),
> > + dsts[0] + dst->off,
> > + *(src->aligned[0] + src-
> > >off),
> > len, flags);
> > else if (thread->type == DMA_XOR)
> > tx = dev->device_prep_dma_xor(chan,
> > - dsts[0] + dst_off,
> > - srcs, src_cnt,
> > + dsts[0] + dst->off,
> > + srcs, src->cnt,
> > len, flags);
> > else if (thread->type == DMA_PQ) {
> > - for (i = 0; i < dst_cnt; i++)
> > - dma_pq[i] = dsts[i] + dst_off;
> > + for (i = 0; i < dst->cnt; i++)
> > + dma_pq[i] = dsts[i] + dst->off;
> > tx = dev->device_prep_dma_pq(chan, dma_pq, srcs,
> > - src_cnt, pq_coefs,
> > + src->cnt, pq_coefs,
> > len, flags);
> > }
> >
> > if (!tx) {
> > dmaengine_unmap_put(um);
> > - result("prep error", total_tests, src_off,
> > - dst_off, len, ret);
> > + result("prep error", total_tests, src->off,
> > + dst->off, len, ret);
> > msleep(100);
> > failed_tests++;
> > continue;
> > @@ -708,8 +726,8 @@ static int dmatest_func(void *data)
> >
> > if (dma_submit_error(cookie)) {
> > dmaengine_unmap_put(um);
> > - result("submit error", total_tests, src_off,
> > - dst_off, len, ret);
> > + result("submit error", total_tests, src->off,
> > + dst->off, len, ret);
> > msleep(100);
> > failed_tests++;
> > continue;
> > @@ -724,58 +742,58 @@ static int dmatest_func(void *data)
> > dmaengine_unmap_put(um);
> >
> > if (!done->done) {
> > - result("test timed out", total_tests, src_off,
> > dst_off,
> > + result("test timed out", total_tests, src->off,
> > dst->off,
> > len, 0);
> > failed_tests++;
> > continue;
> > } else if (status != DMA_COMPLETE) {
> > result(status == DMA_ERROR ?
> > "completion error status" :
> > - "completion busy status", total_tests,
> > src_off,
> > - dst_off, len, ret);
> > + "completion busy status", total_tests,
> > src->off,
> > + dst->off, len, ret);
> > failed_tests++;
> > continue;
> > }
> >
> > if (params->noverify) {
> > - verbose_result("test passed", total_tests,
> > src_off,
> > - dst_off, len, 0);
> > + verbose_result("test passed", total_tests, src-
> > >off,
> > + dst->off, len, 0);
> > continue;
> > }
> >
> > start = ktime_get();
> > pr_debug("%s: verifying source buffer...\n", current-
> > >comm);
> > - error_count = dmatest_verify(thread->srcs, 0, src_off,
> > + error_count = dmatest_verify(src->aligned, 0, src->off,
> > 0, PATTERN_SRC, true, is_memset);
> > - error_count += dmatest_verify(thread->srcs, src_off,
> > - src_off + len, src_off,
> > + error_count += dmatest_verify(src->aligned, src->off,
> > + src->off + len, src->off,
> > PATTERN_SRC | PATTERN_COPY, true,
> > is_memset);
> > - error_count += dmatest_verify(thread->srcs, src_off +
> > len,
> > - params->buf_size, src_off + len,
> > + error_count += dmatest_verify(src->aligned, src->off +
> > len,
> > + params->buf_size, src->off + len,
> > PATTERN_SRC, true, is_memset);
> >
> > pr_debug("%s: verifying dest buffer...\n", current-
> > >comm);
> > - error_count += dmatest_verify(thread->dsts, 0, dst_off,
> > + error_count += dmatest_verify(dst->aligned, 0, dst->off,
> > 0, PATTERN_DST, false, is_memset);
> >
> > - error_count += dmatest_verify(thread->dsts, dst_off,
> > - dst_off + len, src_off,
> > + error_count += dmatest_verify(dst->aligned, dst->off,
> > + dst->off + len, src->off,
> > PATTERN_SRC | PATTERN_COPY, false,
> > is_memset);
> >
> > - error_count += dmatest_verify(thread->dsts, dst_off +
> > len,
> > - params->buf_size, dst_off + len,
> > + error_count += dmatest_verify(dst->aligned, dst->off +
> > len,
> > + params->buf_size, dst->off + len,
> > PATTERN_DST, false, is_memset);
> >
> > diff = ktime_sub(ktime_get(), start);
> > comparetime = ktime_add(comparetime, diff);
> >
> > if (error_count) {
> > - result("data error", total_tests, src_off,
> > dst_off,
> > + result("data error", total_tests, src->off, dst-
> > >off,
> > len, error_count);
> > failed_tests++;
> > } else {
> > - verbose_result("test passed", total_tests,
> > src_off,
> > - dst_off, len, 0);
> > + verbose_result("test passed", total_tests, src-
> > >off,
> > + dst->off, len, 0);
> > }
> > }
> > ktime = ktime_sub(ktime_get(), ktime);
> > @@ -785,22 +803,13 @@ static int dmatest_func(void *data)
> >
> > ret = 0;
> > kfree(dma_pq);
> > -err_srcs_array:
> > +err_dma_pq:
> > kfree(srcs);
> > -err_dstbuf:
> > - for (i = 0; thread->udsts[i]; i++)
> > - kfree(thread->udsts[i]);
> > - kfree(thread->udsts);
> > -err_udsts:
> > - kfree(thread->dsts);
> > -err_dsts:
> > -err_srcbuf:
> > - for (i = 0; thread->usrcs[i]; i++)
> > - kfree(thread->usrcs[i]);
> > - kfree(thread->usrcs);
> > -err_usrcs:
> > - kfree(thread->srcs);
> > err_srcs:
> > + dmatest_free_test_data(dst);
> > +err_dst:
> > + dmatest_free_test_data(src);
> > +err_src:
> > kfree(pq_coefs);
> > err_thread_type:
> > pr_info("%s: summary %u tests, %u failures %llu iops %llu KB/s
> > (%d)\n",
> > --
> > 2.17.1
>
> It would also help review if things are moved in smaller chunks. I can
> think of creating the common mem alloc and free routine by moving
> existing code and then adding new structs..
I'll take a look about splitting this into smaller chunks.
I think this patch may need to be re-applied ; not sure if it applies now.
Thanks
Alex
>
> --
> ~Vinod
^ permalink raw reply
* [PATCHv2,1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status
From: Yoshihiro Shimoda @ 2019-01-21 3:16 UTC (permalink / raw)
To: REE dirk.behme@de.bosch.com, linux-renesas-soc@vger.kernel.org,
Vinod Koul
Cc: dmaengine@vger.kernel.org, geert+renesas@glider.be,
mfarooq@visteon.com, Achim Dahlhoff, Niklas Söderlund,
laurent.pinchart+renesas@ideasonboard.com
Hi Dirk,
(revised Vinod's email address)
> From: Dirk Behme, Sent: Tuesday, January 15, 2019 9:44 PM
>
> 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.
According to the Documentation/driver-api/dmaengine/provider.rst [1],
device_tx_status should report the bytes left. So, we should fix
the current implementation as you said.
---
[1]
``device_tx_status``
- Should report the bytes left to go over on the given channel
<snip>
- In the case of a cyclic transfer, it should only take into
account the current period.
---
> In our case with cyclic, never ending DMA, changing this to
>
> --- 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.
So, at least, we should apply this code above.
> 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;
>
> dma_set_residue(txstate, residue);
>
> ?
I could not find whether a cyclic transfer should not return DMA_COMPLETE on
Documentations/. I only found the following in the include/linux/dmaengine.h:
/**
* enum dma_status - DMA transaction status
* @DMA_COMPLETE: transaction completed
Best regards,
Yoshihiro Shimoda
> 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=3
> 544d2878817bd139dda238cdd86a15e1c03d037
^ permalink raw reply
* [v2,2/3] dma: imx-sdma: add clock ratio 1:1 check
From: Daniel Baluta @ 2019-01-20 18:00 UTC (permalink / raw)
To: Angus Ainslie
Cc: Vinod Koul, dmaengine, NXP Linux Team, Pengutronix Kernel Team,
linux-arm-kernel, Linux Kernel Mailing List
On Sun, Jan 20, 2019 at 4:38 PM Angus Ainslie <angus@akkea.ca> wrote:
>
> Hi Daniel,
>
> On 2019-01-20 02:58, Daniel Baluta wrote:
> > On Sun, Jan 20, 2019 at 4:32 AM Angus Ainslie (Purism) <angus@akkea.ca>
> > wrote:
> >>
> >> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
> >> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
> >> to 500Mhz, so use 1:1 instead.
> >>
> >> based on NXP commit MLK-16841-1
> >
> > Hi Angus,
> >
> > Thanks for doing this!
> >
> > I'm not sure specifying the MLK here helps. I think it would be better
> > to somehow add the original Signed-off-by and mention that the commit
> > was pulled from NXP linux-imx tree.
>
> If it was exactly the same patch I would have but as I added the lines
> in
> sdma_run_channel0 I didn't think it would be right to put signed off by
> "Robin Gong <yibin.gong@nxp.com>" as the code isn't what he signed off
> on.
I see your point. I often encounter this scenario when sending patches.
I think the best solution would be to mention in the commit message
the author of the initial patch which you based your work on.
^ permalink raw reply
* [v2,1/3] dma: imx-sdma: fix NULL pointer de-reference
From: Angus Ainslie @ 2019-01-20 15:04 UTC (permalink / raw)
To: Daniel Baluta
Cc: Vinod Koul, dmaengine, NXP Linux Team, Pengutronix Kernel Team,
linux-arm-kernel, Linux Kernel Mailing List
On 2019-01-20 02:54, Daniel Baluta wrote:
> On Sun, Jan 20, 2019 at 4:34 AM Angus Ainslie (Purism) <angus@akkea.ca>
> wrote:
>>
>> On the imx8mq I get NULL pointer de-deference errors if the device
>> isn't passed in during allocation.
>>
>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>
> Hi Angus,
>
> I have already sent a fix for this:
>
> https://patchwork.kernel.org/patch/10758203/
Sorry, I missed that. I'll drop it for V3.
Angus
^ permalink raw reply
* [v2,2/3] dma: imx-sdma: add clock ratio 1:1 check
From: Angus Ainslie @ 2019-01-20 14:38 UTC (permalink / raw)
To: Daniel Baluta
Cc: Vinod Koul, dmaengine, NXP Linux Team, Pengutronix Kernel Team,
linux-arm-kernel, Linux Kernel Mailing List
Hi Daniel,
On 2019-01-20 02:58, Daniel Baluta wrote:
> On Sun, Jan 20, 2019 at 4:32 AM Angus Ainslie (Purism) <angus@akkea.ca>
> wrote:
>>
>> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
>> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
>> to 500Mhz, so use 1:1 instead.
>>
>> based on NXP commit MLK-16841-1
>
> Hi Angus,
>
> Thanks for doing this!
>
> I'm not sure specifying the MLK here helps. I think it would be better
> to somehow add the original Signed-off-by and mention that the commit
> was pulled from NXP linux-imx tree.
If it was exactly the same patch I would have but as I added the lines
in
sdma_run_channel0 I didn't think it would be right to put signed off by
"Robin Gong <yibin.gong@nxp.com>" as the code isn't what he signed off
on.
>>
>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>> .../devicetree/bindings/dma/fsl-imx-sdma.txt | 1 +
>> drivers/dma/imx-sdma.c | 20
>> +++++++++++++++----
>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> index 3c9a57a8443b..17544c1820b7 100644
>> --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
>> @@ -67,6 +67,7 @@ Optional properties:
>> reg is the GPR register offset.
>> shift is the bit position inside the GPR register.
>> val is the value of the bit (0 or 1).
>> +- fsl,ratio-1-1: AHB/SDMA core clock ration 1:1, 2:1 without this.
>>
>> Examples:
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index 0b3a67ff8e82..65dada21d3c1 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -440,6 +440,8 @@ struct sdma_engine {
>> unsigned int irq;
>> dma_addr_t bd0_phys;
>> struct sdma_buffer_descriptor *bd0;
>> + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
>> + bool clk_ratio;
>> };
>>
>> static int sdma_config_write(struct dma_chan *chan,
>> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine
>> *sdma)
>> dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>>
>> /* Set bits of CONFIG register with dynamic context switching
>> */
>> - if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
>> - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs +
>> SDMA_H_CONFIG);
>> + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) {
>> + if (sdma->clk_ratio)
>> + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR;
>> + else
>> + reg = SDMA_H_CONFIG_CSM;
>> +
>> + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
>> + }
>>
This is the code that I added out of an over abundance of prudence.
Angus
>> return ret;
>> }
>> @@ -1880,8 +1888,10 @@ static int sdma_init(struct sdma_engine *sdma)
>> writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
>>
>> /* Set bits of CONFIG register but with static context
>> switching */
>> - /* FIXME: Check whether to set ACR bit depending on clock
>> ratios */
>> - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>> + if (sdma->clk_ratio)
>> + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs +
>> SDMA_H_CONFIG);
>> + else
>> + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>>
>> writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
>>
>> @@ -1975,6 +1985,8 @@ static int sdma_probe(struct platform_device
>> *pdev)
>> if (!sdma)
>> return -ENOMEM;
>>
>> + sdma->clk_ratio = of_property_read_bool(np, "fsl,ratio-1-1");
>> +
>> spin_lock_init(&sdma->channel_0_lock);
>>
>> sdma->dev = &pdev->dev;
>> --
>> 2.17.1
>>
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-01-20 11:47 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: Jose Abreu, linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Dan Williams, Eugeniy Paltsev, Andy Shevchenko, Russell King,
Niklas Cassel, Joao Pinto, Luis de Oliveira, Vitor Soares,
Nelson Costa, Pedro Sousa
On 16-01-19, 11:53, Gustavo Pimentel wrote:
> >> + /* Free irqs */
> >
> > But devm will automatically free it, no ?
>
> Yes, it shouldn't be there. I'll remove it.
Nope this is correct. we need to ensure irqs are freed and tasklets are
quiesced otherwise you can end up with a case when a spurious interrupt
and then tasklets spinning while core unrolls. devm for irq is not a
good idea unless you really know what you are doing!
^ permalink raw reply
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-01-20 11:44 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci, dmaengine, Dan Williams, Eugeniy Paltsev,
Andy Shevchenko, Russell King, Niklas Cassel, Joao Pinto,
Jose Abreu, Luis Oliveira, Vitor Soares, Nelson Costa,
Pedro Sousa
On 11-01-19, 19:33, 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.
A description of eDMA IP will help review the driver
> 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:
These do not belong to change log, either move them to cover-letter or
keep them after s-o-b lines..
> @@ -0,0 +1,1059 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
2019 now
> +static struct dw_edma_burst *dw_edma_alloc_burst(struct dw_edma_chunk *chunk)
> +{
> + struct dw_edma_chan *chan = chunk->chan;
> + struct dw_edma_burst *burst;
> +
> + burst = kvzalloc(sizeof(*burst), GFP_NOWAIT);
Is there a specific reason for kvzalloc(), do you submit this to HW..
> +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);
cb ..?
> +static int dw_edma_device_config(struct dma_chan *dchan,
> + struct dma_slave_config *config)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + const struct dw_edma_core_ops *ops = chan2ops(chan);
> + unsigned long flags;
> + int err = 0;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> +
> + if (!config) {
> + err = -EINVAL;
> + goto err_config;
> + }
> +
> + if (chan->status != EDMA_ST_IDLE) {
> + dev_err(chan2dev(chan), "channel is busy or paused\n");
> + err = -EPERM;
this is not correct behaviour, device_config can be called anytime and
values can take affect on next transaction submitted..
> + goto err_config;
> + }
> +
> + dev_dbg(chan2dev(chan), "addr(physical) src=%pa, dst=%pa\n",
> + &config->src_addr, &config->dst_addr);
> +
> + chan->src_addr = config->src_addr;
> + chan->dst_addr = config->dst_addr;
> +
> + err = ops->device_config(dchan);
what does this do?
> +static enum dma_status
> +dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + const struct dw_edma_core_ops *ops = chan2ops(chan);
> + unsigned long flags;
> + enum dma_status ret;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> +
> + ret = ops->ch_status(chan);
> + if (ret == DMA_ERROR) {
> + goto ret_status;
> + } else if (ret == DMA_IN_PROGRESS) {
> + chan->status = EDMA_ST_BUSY;
> + goto ret_status;
so in this case you set residue as zero, which is not correct
> + } else {
> + /* DMA_COMPLETE */
> + if (chan->status == EDMA_ST_PAUSE)
> + ret = DMA_PAUSED;
> + else if (chan->status == EDMA_ST_BUSY)
> + ret = DMA_IN_PROGRESS;
?? if txn is complete how are you returning DMA_IN_PROGRESS?
> + else
> + ret = DMA_COMPLETE;
> + }
this looks incorrect interpretation to me. The status is to be retrieved
for the given cookie passed and given that you do not even use this
argument tells me that you have understood this as 'channel' status
reporting, which is not correct
> +static struct dma_async_tx_descriptor *
> +dw_edma_device_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
> + unsigned int sg_len,
> + enum dma_transfer_direction direction,
> + unsigned long flags, void *context)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + struct dw_edma_desc *desc;
> + struct dw_edma_chunk *chunk;
> + struct dw_edma_burst *burst;
> + struct scatterlist *sg;
> + unsigned long sflags;
> + phys_addr_t src_addr;
> + phys_addr_t dst_addr;
> + int i;
> +
> + if (sg_len < 1) {
> + dev_err(chan2dev(chan), "invalid sg length %u\n", sg_len);
> + return NULL;
> + }
> +
> + if (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
what is the second part of the check, can you explain that, who sets
chan->dir?
> + dev_dbg(chan2dev(chan), "prepare operation (WRITE)\n");
> + } else if (direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
> + dev_dbg(chan2dev(chan), "prepare operation (READ)\n");
> + } else {
> + dev_err(chan2dev(chan), "invalid direction\n");
> + return NULL;
> + }
> +
> + if (!chan->configured) {
> + dev_err(chan2dev(chan), "(prep_slave_sg) channel not configured\n");
> + return NULL;
> + }
> +
> + if (chan->status != EDMA_ST_IDLE) {
> + dev_err(chan2dev(chan), "channel is busy or paused\n");
> + return NULL;
> + }
No, wrong again. The txn must be prepared and then on submit added to a
queue. You are writing a driver for dmaengine, surely you dont expect
the channel to be free and then do a txn.. that would be very
inefficient!
> +static struct dma_async_tx_descriptor *
> +dw_edma_device_prep_dma_cyclic(struct dma_chan *dchan, dma_addr_t buf_addr,
> + size_t len, size_t cyclic_cnt,
> + enum dma_transfer_direction direction,
> + unsigned long flags)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> + struct dw_edma_desc *desc;
> + struct dw_edma_chunk *chunk;
> + struct dw_edma_burst *burst;
> + unsigned long sflags;
> + phys_addr_t src_addr;
> + phys_addr_t dst_addr;
> + u32 i;
> +
> + if (!len || !cyclic_cnt) {
> + dev_err(chan2dev(chan), "invalid len or cyclic count\n");
> + return NULL;
> + }
> +
> + if (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
> + dev_dbg(chan2dev(chan), "prepare operation (WRITE)\n");
> + } else if (direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
> + dev_dbg(chan2dev(chan), "prepare operation (READ)\n");
> + } else {
> + dev_err(chan2dev(chan), "invalid direction\n");
> + return NULL;
> + }
> +
> + if (!chan->configured) {
> + dev_err(chan2dev(chan), "(prep_dma_cyclic) channel not configured\n");
> + return NULL;
> + }
> +
> + if (chan->status != EDMA_ST_IDLE) {
> + dev_err(chan2dev(chan), "channel is busy or paused\n");
> + return NULL;
> + }
> +
> + spin_lock_irqsave(&chan->vc.lock, sflags);
> +
> + desc = dw_edma_alloc_desc(chan);
> + if (unlikely(!desc))
> + goto err_alloc;
> +
> + chunk = dw_edma_alloc_chunk(desc);
> + if (unlikely(!chunk))
> + goto err_alloc;
> +
> + src_addr = chan->src_addr;
> + dst_addr = chan->dst_addr;
> +
> + for (i = 0; i < cyclic_cnt; i++) {
> + if (chunk->bursts_alloc == chan->ll_max) {
> + chunk = dw_edma_alloc_chunk(desc);
> + if (unlikely(!chunk))
> + goto err_alloc;
> + }
> +
> + burst = dw_edma_alloc_burst(chunk);
> +
> + if (unlikely(!burst))
> + goto err_alloc;
> +
> + burst->sz = len;
> + chunk->ll_region.sz += burst->sz;
> + desc->alloc_sz += burst->sz;
> +
> + burst->sar = src_addr;
> + burst->dar = dst_addr;
> +
> + dev_dbg(chan2dev(chan), "lli %u/%u, sar=0x%.8llx, dar=0x%.8llx, size=%u bytes\n",
> + i + 1, cyclic_cnt, burst->sar, burst->dar, burst->sz);
> + }
> +
> + spin_unlock_irqrestore(&chan->vc.lock, sflags);
> + return vchan_tx_prep(&chan->vc, &desc->vd, flags);
how is it different from previous? am sure we can reuse bits!
> +static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> +{
> + struct dw_edma *dw = chan->chip->dw;
> + const struct dw_edma_core_ops *ops = dw->ops;
> + struct virt_dma_desc *vd;
> + struct dw_edma_desc *desc;
> + unsigned long flags;
> +
> + ops->clear_done_int(chan);
> + dev_dbg(chan2dev(chan), "clear done interrupt\n");
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> + vd = vchan_next_desc(&chan->vc);
> + switch (chan->request) {
> + case EDMA_REQ_NONE:
> + if (!vd)
> + break;
> +
> + desc = vd2dw_edma_desc(vd);
> + if (desc->chunks_alloc) {
> + dev_dbg(chan2dev(chan), "sub-transfer complete\n");
> + chan->status = EDMA_ST_BUSY;
> + dev_dbg(chan2dev(chan), "transferred %u bytes\n",
> + desc->xfer_sz);
> + dw_edma_start_transfer(chan);
> + } else {
> + list_del(&vd->node);
> + vchan_cookie_complete(vd);
> + chan->status = EDMA_ST_IDLE;
> + dev_dbg(chan2dev(chan), "transfer complete\n");
> + }
> + break;
> + case EDMA_REQ_STOP:
> + if (!vd)
> + break;
> +
> + list_del(&vd->node);
> + vchan_cookie_complete(vd);
> + chan->request = EDMA_REQ_NONE;
> + chan->status = EDMA_ST_IDLE;
> + chan->configured = false;
why is configuration deemed invalid, it can still be used again!
> +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);
> +
> + /* Callback operation selection accordingly to eDMA version */
> + switch (dw->version) {
> + default:
> + dev_err(dev, "unsupported version\n");
> + return -EPERM;
> + }
So we have only one case which returns error, was this code tested?
> +
> + pm_runtime_get_sync(dev);
> +
> + /* 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);
you may use struct_size() here
> + 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);
this is similar to previous with obvious changes, I think this can be
made common routine invoke for R and W ...
So HW provides read channels and write channels and not generic channels
which can be used for either R or W?
^ permalink raw reply
* [3/8,v4] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: Vinod Koul @ 2019-01-20 11:11 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Youlin Wang, Dan Williams, Zhuangluan Su, Ryan Grachek,
Manivannan Sadhasivam, dmaengine, Tanglei Han
On 16-01-19, 09:10, John Stultz wrote:
> 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
^^^
typo
> 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.
Great thanks for the effort!
>
> 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)
why not use BIT()
space between two please
> +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,
> +};
So basically this is default right, do we need to set dma_data and not
assume default..
> +
> +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;
So this is not optional, either ways fine by me :)
^ permalink raw reply
* [2/8,v4] Documentation: bindings: dma: Add binding for dma-channel-mask
From: Vinod Koul @ 2019-01-20 11:06 UTC (permalink / raw)
To: John Stultz
Cc: Manivannan Sadhasivam, lkml, Rob Herring, Mark Rutland,
Tanglei Han, Zhuangluan Su, Ryan Grachek,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On 17-01-19, 09:43, John Stultz wrote:
> On Thu, Jan 17, 2019 at 9:08 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Wed, Jan 16, 2019 at 09:10:23AM -0800, John Stultz 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.
> > >
> > > 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.
> >
> > A general assumption is, "dma-channel-mask" refers to the bit fields of
> > the channels which needs to be masked. But here, it refers to the channels
> > which are available. Doesn't it contradict?
>
> Hrm. So while I can sort of understand the common usage of "mask" as
> to "hide", thus the desire to have a bitfield mean "the channels we
> hide" or "don't use", but in my experience bitmasking is more commonly
> used to keep only a portion of the the bits, so from that perspective
> its more intuitive that a mask be the channels we keep to use. So I'm
> not sure if your suggestion makes it more clear.
>
> But I'm not very particular here, so I'll defer to others on this.
Given the context and documentation which explicitly says it is bitmask
of available channels, i think we are fine :)
^ 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