* Re: [PATCH 01/16] firmware: ti_sci: Add resource management APIs for ringacc, psi-l and udma
From: Lokesh Vutla @ 2019-06-06 6:00 UTC (permalink / raw)
To: Peter Ujfalusi, vkoul, robh+dt, nm, ssantosh
Cc: dan.j.williams, dmaengine, linux-arm-kernel, devicetree,
linux-kernel, grygorii.strashko, t-kristo, tony
In-Reply-To: <20190506123456.6777-2-peter.ujfalusi@ti.com>
Hi Peter,
On 06/05/19 6:04 PM, Peter Ujfalusi wrote:
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Patch has the following checkpatch warnings and checks which can be fixed:
WARNING: Missing commit description - Add an appropriate one
CHECK: Lines should not end with a '('
#262: FILE: drivers/firmware/ti_sci.c:2286:
+static int ti_sci_cmd_rm_udmap_tx_ch_cfg(
CHECK: Lines should not end with a '('
#323: FILE: drivers/firmware/ti_sci.c:2347:
+static int ti_sci_cmd_rm_udmap_rx_ch_cfg(
CHECK: Lines should not end with a '('
#383: FILE: drivers/firmware/ti_sci.c:2407:
+static int ti_sci_cmd_rm_udmap_rx_flow_cfg1(
CHECK: Lines should not end with a '('
#1414: FILE: include/linux/soc/ti/ti_sci_protocol.h:455:
+ int (*rx_flow_cfg)(
total: 0 errors, 2 warnings, 4 checks, 1399 lines checked
> ---
> drivers/firmware/ti_sci.c | 439 +++++++++++++++
> drivers/firmware/ti_sci.h | 704 +++++++++++++++++++++++++
> include/linux/soc/ti/ti_sci_protocol.h | 216 ++++++++
> 3 files changed, 1359 insertions(+)
>
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 64d895b80bc3..af3ebcdeab18 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
[..snip.]
> +}
> +
> +static int ti_sci_cmd_rm_psil_pair(const struct ti_sci_handle *handle,
> + u32 nav_id, u32 src_thread, u32 dst_thread)
> +{
All the psil ops doesn't have the kernel-doc function comments. Just be
consistent with other functions :)
> + struct ti_sci_msg_hdr *resp;
> + struct ti_sci_msg_psil_pair *req;
> + struct ti_sci_xfer *xfer;
> + struct ti_sci_info *info;
> + struct device *dev;
> + int ret = 0;
> +
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> + if (!handle)
> + return -EINVAL;
> +
> + info = handle_to_ti_sci_info(handle);
> + dev = info->dev;
> +
> + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_RM_PSIL_PAIR,
> + TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> + sizeof(*req), sizeof(*resp));
> + if (IS_ERR(xfer)) {
> + ret = PTR_ERR(xfer);
> + dev_err(dev, "RM_PSIL:Message reconfig failed(%d)\n", ret);
> + return ret;
> + }
> + req = (struct ti_sci_msg_psil_pair *)xfer->xfer_buf;
> + req->nav_id = nav_id;
> + req->src_thread = src_thread;
> + req->dst_thread = dst_thread;
> +
> + ret = ti_sci_do_xfer(info, xfer);
> + if (ret) {
> + dev_err(dev, "RM_PSIL:Mbox send fail %d\n", ret);
> + goto fail;
> + }
> +
> + resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
> + ret = ti_sci_is_response_ack(resp) ? 0 : -EINVAL;
> +
> +fail:
> + ti_sci_put_one_xfer(&info->minfo, xfer);
> +
> + return ret;
> +}
> +
[..snip..]
> + */
> +struct ti_sci_msg_rm_ring_cfg_req {
> + struct ti_sci_msg_hdr hdr;
> + u32 valid_params;
> + u16 nav_id;
> + u16 index;
> + u32 addr_lo;
> + u32 addr_hi;
> + u32 count;
> + u8 mode;
> + u8 size;
> + u8 order_id;
> +} __packed;
> +
> +/**
> + * struct ti_sci_msg_rm_ring_cfg_resp - Response to configuring a ring.
> + *
> + * @hdr: Generic Header
> + */
> +struct ti_sci_msg_rm_ring_cfg_resp {
> + struct ti_sci_msg_hdr hdr;
> +} __packed;
If it is a generic ACK, NACK response, just use the header directly.
[..snip..]
> + */
> +struct ti_sci_msg_rm_udmap_rx_ch_cfg_req {
> + struct ti_sci_msg_hdr hdr;
> + u32 valid_params;
> + u16 nav_id;
> + u16 index;
> + u16 rx_fetch_size;
> + u16 rxcq_qnum;
> + u8 rx_priority;
> + u8 rx_qos;
> + u8 rx_orderid;
> + u8 rx_sched_priority;
> + u16 flowid_start;
> + u16 flowid_cnt;
> + u8 rx_pause_on_err;
> + u8 rx_atype;
> + u8 rx_chan_type;
> + u8 rx_ignore_short;
> + u8 rx_ignore_long;
> + u8 rx_burst_size;
> +
extra line?
> +} __packed;
> +
> +/**
Thanks and regards,
Lokesh
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-06 6:00 UTC (permalink / raw)
To: Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, jonathanh, dmaengine, linux-kernel,
sharadg, rlokhande, dramesh, mkumard
In-Reply-To: <b7e28e73-7214-f1dc-866f-102410c88323@nvidia.com>
Hi Sameer,
On 06/06/2019 6.49, Sameer Pujar wrote:
> Sorry for late reply.
> [Resending the reply since delivery failed for few recipients]
> I discussed this internally with HW folks and below is the reason why
> DMA needs
> to know FIFO size.
>
> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
> to the audio sub-system.
> - ADMAIF has multiple channels and share FIFO buffer for individual
> operations. There is a provision
> to allocate specific fifo size for each individual ADMAIF channel from
> the shared buffer.
> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
> engines, which you described earlier.
It is not really different than what other DMAs are doing.
> - The flow control logic is placed inside ADMA. Slave peripheral
> device(ADMAIF) signals ADMA whenever a
> read or write happens on the FIFO(per WORD basis). Please note that
> the signaling is per channel. There is
> no other signaling present from ADMAIF to ADMA.
> - ADMA keeps a counter related to above signaling. Whenever a sufficient
> space is available, it initiates a transfer.
> But the question is, how does it know when to transfer. This is the
> reason, why ADMA has to be aware of FIFO
> depth of ADMAIF channel. Depending on the counters and FIFO depth, it
> knows exactly when a free space is available
> in the context of a specific channel. On ADMA, FIFO_SIZE is just a
> value which should match to actual FIFO_DEPTH/SIZE
> of ADMAIF channel.
> - Now consider two cases based on above logic,
> * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
> In this case, ADMA thinks that there is enough space available for
> transfer, when actually the FIFO data
> on slave is not consumed yet. It would result in OVERRUN.
> * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
> This is case where ADMA won’t transfer, even though sufficient space
> is available, resulting in UNDERRUN.
> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
> way to communicate fifo size info to ADMA.
The src_maxburst / dst_maxburst is exactly for this reason. To
communicate how much data should be transferred per DMA request to/from
peripheral.
In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
which is dynamically configured (you can see the AFIFO of McASP as a
small DMA: on McASP side it services the peripheral, on the other side
it interacts with the given system DMA used by the SoC - EDMA, sDMA or
UDMAP). All DMAs needs a bit different configuration, but the AFIFO
depth on the McASP side is coming in via the src/dst_maxburst and the
drivers just need to interpret it correctly.
It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as well.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Sameer Pujar @ 2019-06-06 6:41 UTC (permalink / raw)
To: Peter Ujfalusi, Vinod Koul
Cc: dan.j.williams, tiwai, jonathanh, dmaengine, linux-kernel,
sharadg, rlokhande, dramesh, mkumard
In-Reply-To: <ed95f03a-bbe7-ad62-f2e1-9bfe22ec733a@ti.com>
On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
> Hi Sameer,
>
> On 06/06/2019 6.49, Sameer Pujar wrote:
>> Sorry for late reply.
>> [Resending the reply since delivery failed for few recipients]
>> I discussed this internally with HW folks and below is the reason why
>> DMA needs
>> to know FIFO size.
>>
>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>> to the audio sub-system.
>> - ADMAIF has multiple channels and share FIFO buffer for individual
>> operations. There is a provision
>> to allocate specific fifo size for each individual ADMAIF channel from
>> the shared buffer.
>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>> engines, which you described earlier.
> It is not really different than what other DMAs are doing.
>
>> - The flow control logic is placed inside ADMA. Slave peripheral
>> device(ADMAIF) signals ADMA whenever a
>> read or write happens on the FIFO(per WORD basis). Please note that
>> the signaling is per channel. There is
>> no other signaling present from ADMAIF to ADMA.
>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>> space is available, it initiates a transfer.
>> But the question is, how does it know when to transfer. This is the
>> reason, why ADMA has to be aware of FIFO
>> depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>> knows exactly when a free space is available
>> in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>> value which should match to actual FIFO_DEPTH/SIZE
>> of ADMAIF channel.
>> - Now consider two cases based on above logic,
>> * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>> In this case, ADMA thinks that there is enough space available for
>> transfer, when actually the FIFO data
>> on slave is not consumed yet. It would result in OVERRUN.
>> * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>> This is case where ADMA won’t transfer, even though sufficient space
>> is available, resulting in UNDERRUN.
>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>> way to communicate fifo size info to ADMA.
> The src_maxburst / dst_maxburst is exactly for this reason. To
> communicate how much data should be transferred per DMA request to/from
> peripheral.
>
> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
> which is dynamically configured (you can see the AFIFO of McASP as a
> small DMA: on McASP side it services the peripheral, on the other side
> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
> depth on the McASP side is coming in via the src/dst_maxburst and the
> drivers just need to interpret it correctly.
>
> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as well.
Not exactly equal.
ADMA burst_size can range from 1(WORD) to 16(WORDS)
FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
multiples of 16]
So without this RFC patch, I need to do following.
- pass FIFO_SIZE via src/dst_maxburst (from peripheral ADMAIF driver)
- DMA driver can have following code,
* Program DMA_FIFO_SIZE value from src/dst_maxburst
* Add below logic
if (src/dst_maxburst > 16)
program ADMA burst_size = 16
else
program ADMA burst_size = 8 (1/2 of minimum FIFO depth of
ADMAIF channel)
With above, the flexibility to program ADMA burst_size to any of the
required values in the specified range is not there.
Hence ideally would have liked to have independent control over
burst_size and fifo_size on ADMA side.
If there is no merit in this, I will update ADMA/ADMAIF driver as
mentioned above.
Thanks,
Sameer.
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 7:14 UTC (permalink / raw)
To: Sameer Pujar, Peter Ujfalusi, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard
In-Reply-To: <4cab47d0-41c3-5a87-48e1-d7f085c2e091@nvidia.com>
On 06/06/2019 07:41, Sameer Pujar wrote:
>
> On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
>> Hi Sameer,
>>
>> On 06/06/2019 6.49, Sameer Pujar wrote:
>>> Sorry for late reply.
>>> [Resending the reply since delivery failed for few recipients]
>>> I discussed this internally with HW folks and below is the reason why
>>> DMA needs
>>> to know FIFO size.
>>>
>>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>>> to the audio sub-system.
>>> - ADMAIF has multiple channels and share FIFO buffer for individual
>>> operations. There is a provision
>>> to allocate specific fifo size for each individual ADMAIF channel
>>> from
>>> the shared buffer.
>>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>>> engines, which you described earlier.
>> It is not really different than what other DMAs are doing.
>>
>>> - The flow control logic is placed inside ADMA. Slave peripheral
>>> device(ADMAIF) signals ADMA whenever a
>>> read or write happens on the FIFO(per WORD basis). Please note that
>>> the signaling is per channel. There is
>>> no other signaling present from ADMAIF to ADMA.
>>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>>> space is available, it initiates a transfer.
>>> But the question is, how does it know when to transfer. This is the
>>> reason, why ADMA has to be aware of FIFO
>>> depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>>> knows exactly when a free space is available
>>> in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>>> value which should match to actual FIFO_DEPTH/SIZE
>>> of ADMAIF channel.
>>> - Now consider two cases based on above logic,
>>> * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>> In this case, ADMA thinks that there is enough space available for
>>> transfer, when actually the FIFO data
>>> on slave is not consumed yet. It would result in OVERRUN.
>>> * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>> This is case where ADMA won’t transfer, even though sufficient
>>> space
>>> is available, resulting in UNDERRUN.
>>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>> way to communicate fifo size info to ADMA.
>> The src_maxburst / dst_maxburst is exactly for this reason. To
>> communicate how much data should be transferred per DMA request to/from
>> peripheral.
>>
>> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
>> which is dynamically configured (you can see the AFIFO of McASP as a
>> small DMA: on McASP side it services the peripheral, on the other side
>> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
>> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
>> depth on the McASP side is coming in via the src/dst_maxburst and the
>> drivers just need to interpret it correctly.
>>
>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>> well.
> Not exactly equal.
> ADMA burst_size can range from 1(WORD) to 16(WORDS)
> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
> multiples of 16]
So I think that the key thing to highlight here, is that the as Sameer
highlighted above for the Tegra ADMA there are two values that need to
be programmed; the DMA client FIFO size and the max burst size. The ADMA
has register fields for both of these.
As you can see from the above the FIFO size can be much greater than the
burst size and so ideally both of these values would be passed to the DMA.
We could get by with just passing the FIFO size (as the max burst size)
and then have the DMA driver set the max burst size depending on this,
but this does feel quite correct for this DMA. Hence, ideally, we would
like to pass both.
We are also open to other ideas.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-06 10:22 UTC (permalink / raw)
To: Jon Hunter, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard
In-Reply-To: <8a5b84db-c00b-fff4-543f-69d90c245660@nvidia.com>
Hi Jon, Sameer,
On 06/06/2019 10.14, Jon Hunter wrote:
>
> On 06/06/2019 07:41, Sameer Pujar wrote:
>>
>> On 6/6/2019 11:30 AM, Peter Ujfalusi wrote:
>>> Hi Sameer,
>>>
>>> On 06/06/2019 6.49, Sameer Pujar wrote:
>>>> Sorry for late reply.
>>>> [Resending the reply since delivery failed for few recipients]
>>>> I discussed this internally with HW folks and below is the reason why
>>>> DMA needs
>>>> to know FIFO size.
>>>>
>>>> - FIFOs reside in peripheral device(ADMAIF), which is the ADMA interface
>>>> to the audio sub-system.
>>>> - ADMAIF has multiple channels and share FIFO buffer for individual
>>>> operations. There is a provision
>>>> to allocate specific fifo size for each individual ADMAIF channel
>>>> from
>>>> the shared buffer.
>>>> - Tegra Audio DMA(ADMA) architecture is different from the usual DMA
>>>> engines, which you described earlier.
>>> It is not really different than what other DMAs are doing.
>>>
>>>> - The flow control logic is placed inside ADMA. Slave peripheral
>>>> device(ADMAIF) signals ADMA whenever a
>>>> read or write happens on the FIFO(per WORD basis). Please note that
>>>> the signaling is per channel. There is
>>>> no other signaling present from ADMAIF to ADMA.
>>>> - ADMA keeps a counter related to above signaling. Whenever a sufficient
>>>> space is available, it initiates a transfer.
>>>> But the question is, how does it know when to transfer. This is the
>>>> reason, why ADMA has to be aware of FIFO
>>>> depth of ADMAIF channel. Depending on the counters and FIFO depth, it
>>>> knows exactly when a free space is available
>>>> in the context of a specific channel. On ADMA, FIFO_SIZE is just a
>>>> value which should match to actual FIFO_DEPTH/SIZE
>>>> of ADMAIF channel.
>>>> - Now consider two cases based on above logic,
>>>> * Case 1: when DMA_FIFO_SIZE > SLAVE_FIFO_SIZE
>>>> In this case, ADMA thinks that there is enough space available for
>>>> transfer, when actually the FIFO data
>>>> on slave is not consumed yet. It would result in OVERRUN.
>>>> * Case 2: when DMA_FIFO_SIZE < SLAVE_FIFO_SIZE
>>>> This is case where ADMA won’t transfer, even though sufficient
>>>> space
>>>> is available, resulting in UNDERRUN.
>>>> - The guideline is to program, DMA_FIFO_SIZE(on ADMA side) =
>>>> SLAVE_FIFO_SIZE(on ADMAIF side) and hence we need a
>>>> way to communicate fifo size info to ADMA.
>>> The src_maxburst / dst_maxburst is exactly for this reason. To
>>> communicate how much data should be transferred per DMA request to/from
>>> peripheral.
>>>
>>> In TI land we have now 3 DMA engines servicing McASP. McASP has FIFO
>>> which is dynamically configured (you can see the AFIFO of McASP as a
>>> small DMA: on McASP side it services the peripheral, on the other side
>>> it interacts with the given system DMA used by the SoC - EDMA, sDMA or
>>> UDMAP). All DMAs needs a bit different configuration, but the AFIFO
>>> depth on the McASP side is coming in via the src/dst_maxburst and the
>>> drivers just need to interpret it correctly.
>>>
>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>> well.
>> Not exactly equal.
>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>> multiples of 16]
>
> So I think that the key thing to highlight here, is that the as Sameer
> highlighted above for the Tegra ADMA there are two values that need to
> be programmed; the DMA client FIFO size and the max burst size. The ADMA
> has register fields for both of these.
How does the ADMA uses the 'client FIFO size' and 'max burst size'
values and what is the relation of these values to the peripheral side
(ADMAIF)?
> As you can see from the above the FIFO size can be much greater than the
> burst size and so ideally both of these values would be passed to the DMA.
>
> We could get by with just passing the FIFO size (as the max burst size)
> and then have the DMA driver set the max burst size depending on this,
> but this does feel quite correct for this DMA. Hence, ideally, we would
> like to pass both.
>
> We are also open to other ideas.
I can not find public documentation (I think they are walled off by
registration), but correct me if I'm wrong:
ADMAIF - peripheral side
- kind of a small DMA for audio preipheral(s)?
- Variable FIFO size
- sends DMA request to ADMA per words
ADMA - system DMA
- receives the DMA requests from ADMAIF
- counts the requests
- based on some threshold of the counter it will send/read from ADMAIF?
- maxburst number of words probably?
ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
managing that FIFO from the outside, making sure that it does not over
or underrun?
And it is the one who sets the pace (in effect the DMA burst size - how
many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* [PATCH 1/4] dmaengine: virt-dma: store result on dma descriptor
From: Alexandru Ardelean @ 2019-06-06 10:45 UTC (permalink / raw)
To: dmaengine; +Cc: Alexandru Ardelean
This allows each virtual channel to store information about each transfer
that completed, i.e. which transfer succeeded (or which failed) and if
there was any residue data on each (completed) transfer.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/virt-dma.c | 4 ++--
drivers/dma/virt-dma.h | 4 ++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 88ad8ed2a8d6..bf560a20c8a8 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -101,7 +101,7 @@ static void vchan_complete(unsigned long arg)
}
spin_unlock_irq(&vc->lock);
- dmaengine_desc_callback_invoke(&cb, NULL);
+ dmaengine_desc_callback_invoke(&cb, &vd->tx_result);
list_for_each_entry_safe(vd, _vd, &head, node) {
dmaengine_desc_get_callback(&vd->tx, &cb);
@@ -109,7 +109,7 @@ static void vchan_complete(unsigned long arg)
list_del(&vd->node);
vchan_vdesc_fini(vd);
- dmaengine_desc_callback_invoke(&cb, NULL);
+ dmaengine_desc_callback_invoke(&cb, &vd->tx_result);
}
}
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index b09b75ab0751..eb767c583b7e 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -17,6 +17,7 @@
struct virt_dma_desc {
struct dma_async_tx_descriptor tx;
+ struct dmaengine_result tx_result;
/* protected by vc.lock */
struct list_head node;
};
@@ -65,6 +66,9 @@ static inline struct dma_async_tx_descriptor *vchan_tx_prep(struct virt_dma_chan
vd->tx.tx_submit = vchan_tx_submit;
vd->tx.desc_free = vchan_tx_desc_free;
+ vd->tx_result.result = DMA_TRANS_NOERROR;
+ vd->tx_result.residue = 0;
+
spin_lock_irqsave(&vc->lock, flags);
list_add_tail(&vd->node, &vc->desc_allocated);
spin_unlock_irqrestore(&vc->lock, flags);
--
2.20.1
^ permalink raw reply related
* [PATCH 3/4] dmaengine: axi-dmac: terminate early DMA transfers after a partial one
From: Alexandru Ardelean @ 2019-06-06 10:45 UTC (permalink / raw)
To: dmaengine; +Cc: Alexandru Ardelean
In-Reply-To: <20190606104550.32336-1-alexandru.ardelean@analog.com>
When a partial transfer is received, the driver should not submit any more
segments to the hardware, as they will be ignored/unused until a new
transfer start operation is done.
This change implements this by adding a new flag on the AXI DMAC
descriptor. This flags is set to true, if there was a partial transfer in
a previously completed segment. When that flag is true, the TLAST flag is
added to the to the submitted segment, signaling the controller to stop
receiving more segments.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/dma-axi-dmac.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index e0702697e2b3..3b418d545c7a 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -97,6 +97,7 @@ struct axi_dmac_sg {
struct axi_dmac_desc {
struct virt_dma_desc vdesc;
bool cyclic;
+ bool have_partial_xfer;
unsigned int num_submitted;
unsigned int num_completed;
@@ -221,7 +222,8 @@ static void axi_dmac_start_transfer(struct axi_dmac_chan *chan)
}
desc->num_submitted++;
- if (desc->num_submitted == desc->num_sgs) {
+ if (desc->num_submitted == desc->num_sgs ||
+ desc->have_partial_xfer) {
if (desc->cyclic)
desc->num_submitted = 0; /* Start again */
else
@@ -295,6 +297,7 @@ static void axi_dmac_dequeue_partial_xfers(struct axi_dmac_chan *chan)
if (sg->id == AXI_DMAC_SG_UNUSED)
continue;
if (sg->id == id) {
+ desc->have_partial_xfer = true;
sg->partial_len = len;
found_sg = true;
break;
--
2.20.1
^ permalink raw reply related
* [PATCH 4/4] dmaengine: axi-dmac: add regmap support
From: Alexandru Ardelean @ 2019-06-06 10:45 UTC (permalink / raw)
To: dmaengine; +Cc: Alexandru Ardelean
In-Reply-To: <20190606104550.32336-1-alexandru.ardelean@analog.com>
The registers for AXI DMAC are detailed at:
https://wiki.analog.com/resources/fpga/docs/axi_dmac#register_map
This change adds regmap support for these registers, in case some wants to
have a more direct access to them via this interface.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/Kconfig | 1 +
drivers/dma/dma-axi-dmac.c | 41 ++++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index eaf78f4e07ce..ae631c6e8bc5 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -102,6 +102,7 @@ config AXI_DMAC
depends on MICROBLAZE || NIOS2 || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_SOCFPGA || COMPILE_TEST
select DMA_ENGINE
select DMA_VIRTUAL_CHANNELS
+ select REGMAP_MMIO
help
Enable support for the Analog Devices AXI-DMAC peripheral. This DMA
controller is often used in Analog Device's reference designs for FPGA
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index 3b418d545c7a..a35b76f08dfa 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -19,6 +19,7 @@
#include <linux/of.h>
#include <linux/of_dma.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/fpga/adi-axi-common.h>
@@ -679,6 +680,44 @@ static void axi_dmac_desc_free(struct virt_dma_desc *vdesc)
kfree(container_of(vdesc, struct axi_dmac_desc, vdesc));
}
+static bool axi_dmac_regmap_rdwr(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case AXI_DMAC_REG_IRQ_MASK:
+ case AXI_DMAC_REG_IRQ_SOURCE:
+ case AXI_DMAC_REG_IRQ_PENDING:
+ case AXI_DMAC_REG_CTRL:
+ case AXI_DMAC_REG_TRANSFER_ID:
+ case AXI_DMAC_REG_START_TRANSFER:
+ case AXI_DMAC_REG_FLAGS:
+ case AXI_DMAC_REG_DEST_ADDRESS:
+ case AXI_DMAC_REG_SRC_ADDRESS:
+ case AXI_DMAC_REG_X_LENGTH:
+ case AXI_DMAC_REG_Y_LENGTH:
+ case AXI_DMAC_REG_DEST_STRIDE:
+ case AXI_DMAC_REG_SRC_STRIDE:
+ case AXI_DMAC_REG_TRANSFER_DONE:
+ case AXI_DMAC_REG_ACTIVE_TRANSFER_ID :
+ case AXI_DMAC_REG_STATUS:
+ case AXI_DMAC_REG_CURRENT_SRC_ADDR:
+ case AXI_DMAC_REG_CURRENT_DEST_ADDR:
+ case AXI_DMAC_REG_PARTIAL_XFER_LEN:
+ case AXI_DMAC_REG_PARTIAL_XFER_ID:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config axi_dmac_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = AXI_DMAC_REG_PARTIAL_XFER_ID,
+ .readable_reg = axi_dmac_regmap_rdwr,
+ .writeable_reg = axi_dmac_regmap_rdwr,
+};
+
/*
* The configuration stored in the devicetree matches the configuration
* parameters of the peripheral instance and allows the driver to know which
@@ -883,6 +922,8 @@ static int axi_dmac_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dmac);
+ devm_regmap_init_mmio(&pdev->dev, dmac->base, &axi_dmac_regmap_config);
+
return 0;
err_unregister_of:
--
2.20.1
^ permalink raw reply related
* [PATCH 2/4] dmaengine: axi-dmac: populate residue info for completed xfers
From: Alexandru Ardelean @ 2019-06-06 10:45 UTC (permalink / raw)
To: dmaengine; +Cc: Alexandru Ardelean
In-Reply-To: <20190606104550.32336-1-alexandru.ardelean@analog.com>
Starting with version 4.2.a, the AXI DMAC controller can report partial
transfers that have been issued.
This change implements computing DMA residue information for transfers,
based on that reported information.
The way this is done, is to dequeue the partial transfers from the FIFO of
partial transfers, store the partial length to the correct segment &
descriptor, and compute the residue before submitting the DMA cookie to the
DMA framework.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/dma-axi-dmac.c | 99 +++++++++++++++++++++++++++++++++++++-
1 file changed, 98 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index d5e29bbc3d43..e0702697e2b3 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -64,6 +64,8 @@
#define AXI_DMAC_REG_STATUS 0x430
#define AXI_DMAC_REG_CURRENT_SRC_ADDR 0x434
#define AXI_DMAC_REG_CURRENT_DEST_ADDR 0x438
+#define AXI_DMAC_REG_PARTIAL_XFER_LEN 0x44c
+#define AXI_DMAC_REG_PARTIAL_XFER_ID 0x450
#define AXI_DMAC_CTRL_ENABLE BIT(0)
#define AXI_DMAC_CTRL_PAUSE BIT(1)
@@ -73,6 +75,9 @@
#define AXI_DMAC_FLAG_CYCLIC BIT(0)
#define AXI_DMAC_FLAG_LAST BIT(1)
+#define AXI_DMAC_FLAG_PARTIAL_REPORT BIT(2)
+
+#define AXI_DMAC_FLAG_PARTIAL_XFER_DONE BIT(31)
/* The maximum ID allocated by the hardware is 31 */
#define AXI_DMAC_SG_UNUSED 32U
@@ -85,6 +90,7 @@ struct axi_dmac_sg {
unsigned int dest_stride;
unsigned int src_stride;
unsigned int id;
+ unsigned int partial_len;
bool schedule_when_free;
};
@@ -114,6 +120,7 @@ struct axi_dmac_chan {
unsigned int address_align_mask;
unsigned int length_align_mask;
+ bool hw_partial_xfer;
bool hw_cyclic;
bool hw_2d;
};
@@ -245,6 +252,9 @@ static void axi_dmac_start_transfer(struct axi_dmac_chan *chan)
desc->num_sgs == 1)
flags |= AXI_DMAC_FLAG_CYCLIC;
+ if (chan->hw_partial_xfer)
+ flags |= AXI_DMAC_FLAG_PARTIAL_REPORT;
+
axi_dmac_write(dmac, AXI_DMAC_REG_X_LENGTH, sg->x_len - 1);
axi_dmac_write(dmac, AXI_DMAC_REG_Y_LENGTH, sg->y_len - 1);
axi_dmac_write(dmac, AXI_DMAC_REG_FLAGS, flags);
@@ -257,6 +267,82 @@ static struct axi_dmac_desc *axi_dmac_active_desc(struct axi_dmac_chan *chan)
struct axi_dmac_desc, vdesc.node);
}
+static inline unsigned int axi_dmac_total_sg_bytes(struct axi_dmac_chan *chan,
+ struct axi_dmac_sg *sg)
+{
+ if (chan->hw_2d)
+ return sg->x_len * sg->y_len;
+ else
+ return sg->x_len;
+}
+
+static void axi_dmac_dequeue_partial_xfers(struct axi_dmac_chan *chan)
+{
+ struct axi_dmac *dmac = chan_to_axi_dmac(chan);
+ struct axi_dmac_desc *desc;
+ struct axi_dmac_sg *sg;
+ u32 xfer_done, len, id, i;
+ bool found_sg;
+
+ do {
+ len = axi_dmac_read(dmac, AXI_DMAC_REG_PARTIAL_XFER_LEN);
+ id = axi_dmac_read(dmac, AXI_DMAC_REG_PARTIAL_XFER_ID);
+
+ found_sg = false;
+ list_for_each_entry(desc, &chan->active_descs, vdesc.node) {
+ for (i = 0; i < desc->num_sgs; i++) {
+ sg = &desc->sg[i];
+ if (sg->id == AXI_DMAC_SG_UNUSED)
+ continue;
+ if (sg->id == id) {
+ sg->partial_len = len;
+ found_sg = true;
+ break;
+ }
+ }
+ if (found_sg)
+ break;
+ }
+
+ if (found_sg) {
+ dev_dbg(dmac->dma_dev.dev,
+ "Found partial segment id=%u, len=%u\n",
+ id, len);
+ } else {
+ dev_warn(dmac->dma_dev.dev,
+ "Not found partial segment id=%u, len=%u\n",
+ id, len);
+ }
+
+ /* Check if we have any more partial transfers */
+ xfer_done = axi_dmac_read(dmac, AXI_DMAC_REG_TRANSFER_DONE);
+ xfer_done = !(xfer_done & AXI_DMAC_FLAG_PARTIAL_XFER_DONE);
+
+ } while (!xfer_done);
+}
+
+static void axi_dmac_compute_residue(struct axi_dmac_chan *chan,
+ struct axi_dmac_desc *active)
+{
+ struct dmaengine_result *rslt = &active->vdesc.tx_result;
+ unsigned int start = active->num_completed - 1;
+ struct axi_dmac_sg *sg;
+ unsigned int i, total;
+
+ rslt->result = DMA_TRANS_NOERROR;
+ rslt->residue = 0;
+
+ /*
+ * We get here if the last completed segment is partial, which
+ * means we can compute the residue from that segment onwards
+ */
+ for (i = start; i < active->num_sgs; i++) {
+ sg = &active->sg[i];
+ total = axi_dmac_total_sg_bytes(chan, sg);
+ rslt->residue += (total - sg->partial_len);
+ }
+}
+
static bool axi_dmac_transfer_done(struct axi_dmac_chan *chan,
unsigned int completed_transfers)
{
@@ -268,6 +354,10 @@ static bool axi_dmac_transfer_done(struct axi_dmac_chan *chan,
if (!active)
return false;
+ if (chan->hw_partial_xfer &&
+ (completed_transfers & AXI_DMAC_FLAG_PARTIAL_XFER_DONE))
+ axi_dmac_dequeue_partial_xfers(chan);
+
do {
sg = &active->sg[active->num_completed];
if (sg->id == AXI_DMAC_SG_UNUSED) /* Not yet submitted */
@@ -281,10 +371,14 @@ static bool axi_dmac_transfer_done(struct axi_dmac_chan *chan,
start_next = true;
}
+ if (sg->partial_len)
+ axi_dmac_compute_residue(chan, active);
+
if (active->cyclic)
vchan_cyclic_callback(&active->vdesc);
- if (active->num_completed == active->num_sgs) {
+ if (active->num_completed == active->num_sgs ||
+ sg->partial_len) {
if (active->cyclic) {
active->num_completed = 0; /* wrap around */
} else {
@@ -675,6 +769,9 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac)
return -ENODEV;
}
+ if (version >= ADI_AXI_PCORE_VER(4, 2, 'a'))
+ chan->hw_partial_xfer = true;
+
if (version >= ADI_AXI_PCORE_VER(4, 1, 'a')) {
axi_dmac_write(dmac, AXI_DMAC_REG_X_LENGTH, 0x00);
chan->length_align_mask =
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 10:49 UTC (permalink / raw)
To: Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <3f836a10-eaf3-f59b-7170-6fe937cf2e43@ti.com>
On 06/06/2019 11:22, Peter Ujfalusi wrote:
...
>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>> well.
>>> Not exactly equal.
>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>> multiples of 16]
>>
>> So I think that the key thing to highlight here, is that the as Sameer
>> highlighted above for the Tegra ADMA there are two values that need to
>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>> has register fields for both of these.
>
> How does the ADMA uses the 'client FIFO size' and 'max burst size'
> values and what is the relation of these values to the peripheral side
> (ADMAIF)?
Per Sameer's previous comment, the FIFO size is used by the ADMA to
determine how much space is available in the FIFO. I assume the burst
size just limits how much data is transferred per transaction.
>> As you can see from the above the FIFO size can be much greater than the
>> burst size and so ideally both of these values would be passed to the DMA.
>>
>> We could get by with just passing the FIFO size (as the max burst size)
>> and then have the DMA driver set the max burst size depending on this,
>> but this does feel quite correct for this DMA. Hence, ideally, we would
>> like to pass both.
>>
>> We are also open to other ideas.
>
> I can not find public documentation (I think they are walled off by
> registration), but correct me if I'm wrong:
No unfortunately, you are not wrong here :-(
> ADMAIF - peripheral side
> - kind of a small DMA for audio preipheral(s)?
Yes this is the interface to the APE (audio processing engine) and data
sent to the ADMAIF is then sent across a crossbar to one of many
devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
configurable depending on the use-case.
> - Variable FIFO size
Yes.
> - sends DMA request to ADMA per words
From Sameer's notes it says the ADMAIF send a signal to the ADMA per
word, yes.
> ADMA - system DMA
> - receives the DMA requests from ADMAIF
> - counts the requests
> - based on some threshold of the counter it will send/read from ADMAIF?
> - maxburst number of words probably?
Sounds about right to me.
> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
> managing that FIFO from the outside, making sure that it does not over
> or underrun?
Yes.
> And it is the one who sets the pace (in effect the DMA burst size - how
> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
Yes.
So currently, if you look at the ADMA driver
(drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
for the burst, but the FIFO size is hard-coded (see the
TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
Ideally, we should not hard-code this but pass it.
Given that there are no current users of the ADMA upstream, we could
change the usage of the src/dst_maxburst, but being able to set the FIFO
size as well would be ideal.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* [PATCH] dmaengine: axi-dmac: update license header
From: Alexandru Ardelean @ 2019-06-06 10:53 UTC (permalink / raw)
To: dmaengine; +Cc: Alexandru Ardelean
The change replaces the old license information in the comment header with
the new SPDX license specifier.
As well as bumping the year range from 2013-2015 to 2013-2019.
The latter also reflects recent changes that were added to the driver.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/dma/dma-axi-dmac.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index a35b76f08dfa..5c81b798cadb 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -1,10 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* Driver for the Analog Devices AXI-DMAC core
*
- * Copyright 2013-2015 Analog Devices Inc.
+ * Copyright 2013-2019 Analog Devices Inc.
* Author: Lars-Peter Clausen <lars@metafoo.de>
- *
- * Licensed under the GPL-2.
*/
#include <linux/clk.h>
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-06 11:54 UTC (permalink / raw)
To: Jon Hunter, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <a36302fc-3173-070b-5c97-7d2c55d5e2cc@nvidia.com>
On 06/06/2019 13.49, Jon Hunter wrote:
>
> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>
> ...
>
>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>> well.
>>>> Not exactly equal.
>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>> multiples of 16]
>>>
>>> So I think that the key thing to highlight here, is that the as Sameer
>>> highlighted above for the Tegra ADMA there are two values that need to
>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>> has register fields for both of these.
>>
>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>> values and what is the relation of these values to the peripheral side
>> (ADMAIF)?
>
> Per Sameer's previous comment, the FIFO size is used by the ADMA to
> determine how much space is available in the FIFO. I assume the burst
> size just limits how much data is transferred per transaction.
>
>>> As you can see from the above the FIFO size can be much greater than the
>>> burst size and so ideally both of these values would be passed to the DMA.
>>>
>>> We could get by with just passing the FIFO size (as the max burst size)
>>> and then have the DMA driver set the max burst size depending on this,
>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>> like to pass both.
>>>
>>> We are also open to other ideas.
>>
>> I can not find public documentation (I think they are walled off by
>> registration), but correct me if I'm wrong:
>
> No unfortunately, you are not wrong here :-(
>
>> ADMAIF - peripheral side
>> - kind of a small DMA for audio preipheral(s)?
>
> Yes this is the interface to the APE (audio processing engine) and data
> sent to the ADMAIF is then sent across a crossbar to one of many
> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
> configurable depending on the use-case.
>
>> - Variable FIFO size
>
> Yes.
>
>> - sends DMA request to ADMA per words
>
> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
> word, yes.
>
>> ADMA - system DMA
>> - receives the DMA requests from ADMAIF
>> - counts the requests
>> - based on some threshold of the counter it will send/read from ADMAIF?
>> - maxburst number of words probably?
>
> Sounds about right to me.
>
>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>> managing that FIFO from the outside, making sure that it does not over
>> or underrun?
>
> Yes.
>
>> And it is the one who sets the pace (in effect the DMA burst size - how
>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>
> Yes.
>
> So currently, if you look at the ADMA driver
> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
> for the burst, but the FIFO size is hard-coded (see the
> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
> Ideally, we should not hard-code this but pass it.
Sure, hardcoding is never good ;)
> Given that there are no current users of the ADMA upstream, we could
> change the usage of the src/dst_maxburst, but being able to set the FIFO
> size as well would be ideal.
Looking at the drivers/dma/tegra210-adma.c for the
TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
remote FIFO size would fit.
There are fields for overflow and starvation(?) thresholds and TX/RX
size (assuming word length, 3 == 32bits?).
Both threshold is set to one, so I assume currently ADMA is
pushing/pulling data word by word.
Not sure what the burst size is used for, my guess would be that it is
used on the memory (DDR) side for optimized, more efficient accesses?
My guess is that the threshold values are the counter limits, if the DMA
request counter reaches it then ADMA would do a threshold limit worth of
push/pull to ADMAIF.
Or there is another register where the remote FIFO size can be written
and ADMA is counting back from there until it reaches the threshold (and
pushes/pulling again threshold amount of data) so it keeps the FIFO
filled with at least threshold amount of data?
I think in both cases the threshold would be the maxburst.
I suppose you have the patch for adma on how to use the fifo_size
parameter? That would help understand what you are trying to achieve better.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH 01/16] firmware: ti_sci: Add resource management APIs for ringacc, psi-l and udma
From: Peter Ujfalusi @ 2019-06-06 12:04 UTC (permalink / raw)
To: Lokesh Vutla, vkoul, robh+dt, nm, ssantosh
Cc: dan.j.williams, dmaengine, linux-arm-kernel, devicetree,
linux-kernel, grygorii.strashko, t-kristo, tony
In-Reply-To: <f2056b18-3f65-b7ae-90ba-5ebf9ac425bc@ti.com>
Hi Lokesh,
On 06/06/2019 9.00, Lokesh Vutla wrote:
> Hi Peter,
>
> On 06/05/19 6:04 PM, Peter Ujfalusi wrote:
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>
> Patch has the following checkpatch warnings and checks which can be fixed:
>
> WARNING: Missing commit description - Add an appropriate one
How did I missed it?
> CHECK: Lines should not end with a '('
> #262: FILE: drivers/firmware/ti_sci.c:2286:
> +static int ti_sci_cmd_rm_udmap_tx_ch_cfg(
>
> CHECK: Lines should not end with a '('
> #323: FILE: drivers/firmware/ti_sci.c:2347:
> +static int ti_sci_cmd_rm_udmap_rx_ch_cfg(
>
> CHECK: Lines should not end with a '('
> #383: FILE: drivers/firmware/ti_sci.c:2407:
> +static int ti_sci_cmd_rm_udmap_rx_flow_cfg1(
>
> CHECK: Lines should not end with a '('
> #1414: FILE: include/linux/soc/ti/ti_sci_protocol.h:455:
> + int (*rx_flow_cfg)(
>
> total: 0 errors, 2 warnings, 4 checks, 1399 lines checked
There must be a reason why these left, but I will take another look.
>> ---
>> drivers/firmware/ti_sci.c | 439 +++++++++++++++
>> drivers/firmware/ti_sci.h | 704 +++++++++++++++++++++++++
>> include/linux/soc/ti/ti_sci_protocol.h | 216 ++++++++
>> 3 files changed, 1359 insertions(+)
>>
>> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
>> index 64d895b80bc3..af3ebcdeab18 100644
>> --- a/drivers/firmware/ti_sci.c
>> +++ b/drivers/firmware/ti_sci.c
>
> [..snip.]
>
>> +}
>> +
>> +static int ti_sci_cmd_rm_psil_pair(const struct ti_sci_handle *handle,
>> + u32 nav_id, u32 src_thread, u32 dst_thread)
>> +{
>
> All the psil ops doesn't have the kernel-doc function comments. Just be
> consistent with other functions :)
OK.
>> + struct ti_sci_msg_hdr *resp;
>> + struct ti_sci_msg_psil_pair *req;
>> + struct ti_sci_xfer *xfer;
>> + struct ti_sci_info *info;
>> + struct device *dev;
>> + int ret = 0;
>> +
>> + if (IS_ERR(handle))
>> + return PTR_ERR(handle);
>> + if (!handle)
>> + return -EINVAL;
>> +
>> + info = handle_to_ti_sci_info(handle);
>> + dev = info->dev;
>> +
>> + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_RM_PSIL_PAIR,
>> + TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
>> + sizeof(*req), sizeof(*resp));
>> + if (IS_ERR(xfer)) {
>> + ret = PTR_ERR(xfer);
>> + dev_err(dev, "RM_PSIL:Message reconfig failed(%d)\n", ret);
>> + return ret;
>> + }
>> + req = (struct ti_sci_msg_psil_pair *)xfer->xfer_buf;
>> + req->nav_id = nav_id;
>> + req->src_thread = src_thread;
>> + req->dst_thread = dst_thread;
>> +
>> + ret = ti_sci_do_xfer(info, xfer);
>> + if (ret) {
>> + dev_err(dev, "RM_PSIL:Mbox send fail %d\n", ret);
>> + goto fail;
>> + }
>> +
>> + resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
>> + ret = ti_sci_is_response_ack(resp) ? 0 : -EINVAL;
>> +
>> +fail:
>> + ti_sci_put_one_xfer(&info->minfo, xfer);
>> +
>> + return ret;
>> +}
>> +
>
> [..snip..]
>
>> + */
>> +struct ti_sci_msg_rm_ring_cfg_req {
>> + struct ti_sci_msg_hdr hdr;
>> + u32 valid_params;
>> + u16 nav_id;
>> + u16 index;
>> + u32 addr_lo;
>> + u32 addr_hi;
>> + u32 count;
>> + u8 mode;
>> + u8 size;
>> + u8 order_id;
>> +} __packed;
>> +
>> +/**
>> + * struct ti_sci_msg_rm_ring_cfg_resp - Response to configuring a ring.
>> + *
>> + * @hdr: Generic Header
>> + */
>> +struct ti_sci_msg_rm_ring_cfg_resp {
>> + struct ti_sci_msg_hdr hdr;
>> +} __packed;
>
> If it is a generic ACK, NACK response, just use the header directly.
Sure, I'll fix it and other places if any.
>
> [..snip..]
>
>> + */
>> +struct ti_sci_msg_rm_udmap_rx_ch_cfg_req {
>> + struct ti_sci_msg_hdr hdr;
>> + u32 valid_params;
>> + u16 nav_id;
>> + u16 index;
>> + u16 rx_fetch_size;
>> + u16 rxcq_qnum;
>> + u8 rx_priority;
>> + u8 rx_qos;
>> + u8 rx_orderid;
>> + u8 rx_sched_priority;
>> + u16 flowid_start;
>> + u16 flowid_cnt;
>> + u8 rx_pause_on_err;
>> + u8 rx_atype;
>> + u8 rx_chan_type;
>> + u8 rx_ignore_short;
>> + u8 rx_ignore_long;
>> + u8 rx_burst_size;
>> +
>
> extra line?
Will remove it.
>
>> +} __packed;
>> +
>> +/**
>
>
> Thanks and regards,
> Lokesh
>
Thanks,
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 12:37 UTC (permalink / raw)
To: Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <a08bec36-b375-6520-eff4-3d847ddfe07d@ti.com>
On 06/06/2019 12:54, Peter Ujfalusi wrote:
>
>
> On 06/06/2019 13.49, Jon Hunter wrote:
>>
>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>
>> ...
>>
>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>> well.
>>>>> Not exactly equal.
>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>> multiples of 16]
>>>>
>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>> has register fields for both of these.
>>>
>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>> values and what is the relation of these values to the peripheral side
>>> (ADMAIF)?
>>
>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>> determine how much space is available in the FIFO. I assume the burst
>> size just limits how much data is transferred per transaction.
>>
>>>> As you can see from the above the FIFO size can be much greater than the
>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>
>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>> and then have the DMA driver set the max burst size depending on this,
>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>> like to pass both.
>>>>
>>>> We are also open to other ideas.
>>>
>>> I can not find public documentation (I think they are walled off by
>>> registration), but correct me if I'm wrong:
>>
>> No unfortunately, you are not wrong here :-(
>>
>>> ADMAIF - peripheral side
>>> - kind of a small DMA for audio preipheral(s)?
>>
>> Yes this is the interface to the APE (audio processing engine) and data
>> sent to the ADMAIF is then sent across a crossbar to one of many
>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>> configurable depending on the use-case.
>>
>>> - Variable FIFO size
>>
>> Yes.
>>
>>> - sends DMA request to ADMA per words
>>
>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>> word, yes.
>>
>>> ADMA - system DMA
>>> - receives the DMA requests from ADMAIF
>>> - counts the requests
>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>> - maxburst number of words probably?
>>
>> Sounds about right to me.
>>
>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>> managing that FIFO from the outside, making sure that it does not over
>>> or underrun?
>>
>> Yes.
>>
>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>
>> Yes.
>>
>> So currently, if you look at the ADMA driver
>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>> for the burst, but the FIFO size is hard-coded (see the
>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>> Ideally, we should not hard-code this but pass it.
>
> Sure, hardcoding is never good ;)
>
>> Given that there are no current users of the ADMA upstream, we could
>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>> size as well would be ideal.
>
> Looking at the drivers/dma/tegra210-adma.c for the
> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
> remote FIFO size would fit.
> There are fields for overflow and starvation(?) thresholds and TX/RX
> size (assuming word length, 3 == 32bits?).
The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
bytes.
> Both threshold is set to one, so I assume currently ADMA is
> pushing/pulling data word by word.
That's different. That indicates thresholds when transfers start.
> Not sure what the burst size is used for, my guess would be that it is
> used on the memory (DDR) side for optimized, more efficient accesses?
That is the actual burst size.
> My guess is that the threshold values are the counter limits, if the DMA
> request counter reaches it then ADMA would do a threshold limit worth of
> push/pull to ADMAIF.
> Or there is another register where the remote FIFO size can be written
> and ADMA is counting back from there until it reaches the threshold (and
> pushes/pulling again threshold amount of data) so it keeps the FIFO
> filled with at least threshold amount of data?
>
> I think in both cases the threshold would be the maxburst.
>
> I suppose you have the patch for adma on how to use the fifo_size
> parameter? That would help understand what you are trying to achieve better.
Its quite simple, we would just use the FIFO size to set the fields
TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 13:45 UTC (permalink / raw)
To: Jon Hunter, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <4593f37c-5e89-8559-4e80-99dbfe4235de@nvidia.com>
06.06.2019 15:37, Jon Hunter пишет:
>
> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>
>>
>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>
>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>
>>> ...
>>>
>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>> well.
>>>>>> Not exactly equal.
>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>> multiples of 16]
>>>>>
>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>> has register fields for both of these.
>>>>
>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>> values and what is the relation of these values to the peripheral side
>>>> (ADMAIF)?
>>>
>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>> determine how much space is available in the FIFO. I assume the burst
>>> size just limits how much data is transferred per transaction.
>>>
>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>
>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>> like to pass both.
>>>>>
>>>>> We are also open to other ideas.
>>>>
>>>> I can not find public documentation (I think they are walled off by
>>>> registration), but correct me if I'm wrong:
>>>
>>> No unfortunately, you are not wrong here :-(
>>>
>>>> ADMAIF - peripheral side
>>>> - kind of a small DMA for audio preipheral(s)?
>>>
>>> Yes this is the interface to the APE (audio processing engine) and data
>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>> configurable depending on the use-case.
>>>
>>>> - Variable FIFO size
>>>
>>> Yes.
>>>
>>>> - sends DMA request to ADMA per words
>>>
>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>> word, yes.
>>>
>>>> ADMA - system DMA
>>>> - receives the DMA requests from ADMAIF
>>>> - counts the requests
>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>> - maxburst number of words probably?
>>>
>>> Sounds about right to me.
>>>
>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>> managing that FIFO from the outside, making sure that it does not over
>>>> or underrun?
>>>
>>> Yes.
>>>
>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>
>>> Yes.
>>>
>>> So currently, if you look at the ADMA driver
>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>> for the burst, but the FIFO size is hard-coded (see the
>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>> Ideally, we should not hard-code this but pass it.
>>
>> Sure, hardcoding is never good ;)
>>
>>> Given that there are no current users of the ADMA upstream, we could
>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>> size as well would be ideal.
>>
>> Looking at the drivers/dma/tegra210-adma.c for the
>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>> remote FIFO size would fit.
>> There are fields for overflow and starvation(?) thresholds and TX/RX
>> size (assuming word length, 3 == 32bits?).
>
> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
> bytes.
>
>> Both threshold is set to one, so I assume currently ADMA is
>> pushing/pulling data word by word.
>
> That's different. That indicates thresholds when transfers start.
>
>> Not sure what the burst size is used for, my guess would be that it is
>> used on the memory (DDR) side for optimized, more efficient accesses?
>
> That is the actual burst size.
>
>> My guess is that the threshold values are the counter limits, if the DMA
>> request counter reaches it then ADMA would do a threshold limit worth of
>> push/pull to ADMAIF.
>> Or there is another register where the remote FIFO size can be written
>> and ADMA is counting back from there until it reaches the threshold (and
>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>> filled with at least threshold amount of data?
>>
>> I think in both cases the threshold would be the maxburst.
>>
>> I suppose you have the patch for adma on how to use the fifo_size
>> parameter? That would help understand what you are trying to achieve better.
>
> Its quite simple, we would just use the FIFO size to set the fields
> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>
> Jon
>
Hi,
If I understood everything correctly, the FIFO buffer is shared among
all of the ADMA clients and hence it should be up to the ADMA driver to
manage the quotas of the clients. So if there is only one client that
uses ADMA at a time, then this client will get a whole FIFO buffer, but
once another client starts to use ADMA, then the ADMA driver will have
to reconfigure hardware to split the quotas.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 13:55 UTC (permalink / raw)
To: Jon Hunter, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <deae510a-f6ae-6a51-2875-a7463cac9169@gmail.com>
06.06.2019 16:45, Dmitry Osipenko пишет:
> 06.06.2019 15:37, Jon Hunter пишет:
>>
>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>
>>>
>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>
>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>
>>>> ...
>>>>
>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>> well.
>>>>>>> Not exactly equal.
>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>> multiples of 16]
>>>>>>
>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>> has register fields for both of these.
>>>>>
>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>> values and what is the relation of these values to the peripheral side
>>>>> (ADMAIF)?
>>>>
>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>> determine how much space is available in the FIFO. I assume the burst
>>>> size just limits how much data is transferred per transaction.
>>>>
>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>
>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>> like to pass both.
>>>>>>
>>>>>> We are also open to other ideas.
>>>>>
>>>>> I can not find public documentation (I think they are walled off by
>>>>> registration), but correct me if I'm wrong:
>>>>
>>>> No unfortunately, you are not wrong here :-(
>>>>
>>>>> ADMAIF - peripheral side
>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>
>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>> configurable depending on the use-case.
>>>>
>>>>> - Variable FIFO size
>>>>
>>>> Yes.
>>>>
>>>>> - sends DMA request to ADMA per words
>>>>
>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>> word, yes.
>>>>
>>>>> ADMA - system DMA
>>>>> - receives the DMA requests from ADMAIF
>>>>> - counts the requests
>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>> - maxburst number of words probably?
>>>>
>>>> Sounds about right to me.
>>>>
>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>> or underrun?
>>>>
>>>> Yes.
>>>>
>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>
>>>> Yes.
>>>>
>>>> So currently, if you look at the ADMA driver
>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>> for the burst, but the FIFO size is hard-coded (see the
>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>> Ideally, we should not hard-code this but pass it.
>>>
>>> Sure, hardcoding is never good ;)
>>>
>>>> Given that there are no current users of the ADMA upstream, we could
>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>> size as well would be ideal.
>>>
>>> Looking at the drivers/dma/tegra210-adma.c for the
>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>> remote FIFO size would fit.
>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>> size (assuming word length, 3 == 32bits?).
>>
>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>> bytes.
>>
>>> Both threshold is set to one, so I assume currently ADMA is
>>> pushing/pulling data word by word.
>>
>> That's different. That indicates thresholds when transfers start.
>>
>>> Not sure what the burst size is used for, my guess would be that it is
>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>
>> That is the actual burst size.
>>
>>> My guess is that the threshold values are the counter limits, if the DMA
>>> request counter reaches it then ADMA would do a threshold limit worth of
>>> push/pull to ADMAIF.
>>> Or there is another register where the remote FIFO size can be written
>>> and ADMA is counting back from there until it reaches the threshold (and
>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>> filled with at least threshold amount of data?
>>>
>>> I think in both cases the threshold would be the maxburst.
>>>
>>> I suppose you have the patch for adma on how to use the fifo_size
>>> parameter? That would help understand what you are trying to achieve better.
>>
>> Its quite simple, we would just use the FIFO size to set the fields
>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>
>> Jon
>>
>
> Hi,
>
> If I understood everything correctly, the FIFO buffer is shared among
> all of the ADMA clients and hence it should be up to the ADMA driver to
> manage the quotas of the clients. So if there is only one client that
> uses ADMA at a time, then this client will get a whole FIFO buffer, but
> once another client starts to use ADMA, then the ADMA driver will have
> to reconfigure hardware to split the quotas.
>
You could also simply hardcode the quotas per client in the ADMA driver
if the quotas are going to be static anyway.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 14:25 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <deae510a-f6ae-6a51-2875-a7463cac9169@gmail.com>
On 06/06/2019 14:45, Dmitry Osipenko wrote:
> 06.06.2019 15:37, Jon Hunter пишет:
>>
>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>
>>>
>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>
>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>
>>>> ...
>>>>
>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>> well.
>>>>>>> Not exactly equal.
>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>> multiples of 16]
>>>>>>
>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>> has register fields for both of these.
>>>>>
>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>> values and what is the relation of these values to the peripheral side
>>>>> (ADMAIF)?
>>>>
>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>> determine how much space is available in the FIFO. I assume the burst
>>>> size just limits how much data is transferred per transaction.
>>>>
>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>
>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>> like to pass both.
>>>>>>
>>>>>> We are also open to other ideas.
>>>>>
>>>>> I can not find public documentation (I think they are walled off by
>>>>> registration), but correct me if I'm wrong:
>>>>
>>>> No unfortunately, you are not wrong here :-(
>>>>
>>>>> ADMAIF - peripheral side
>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>
>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>> configurable depending on the use-case.
>>>>
>>>>> - Variable FIFO size
>>>>
>>>> Yes.
>>>>
>>>>> - sends DMA request to ADMA per words
>>>>
>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>> word, yes.
>>>>
>>>>> ADMA - system DMA
>>>>> - receives the DMA requests from ADMAIF
>>>>> - counts the requests
>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>> - maxburst number of words probably?
>>>>
>>>> Sounds about right to me.
>>>>
>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>> or underrun?
>>>>
>>>> Yes.
>>>>
>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>
>>>> Yes.
>>>>
>>>> So currently, if you look at the ADMA driver
>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>> for the burst, but the FIFO size is hard-coded (see the
>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>> Ideally, we should not hard-code this but pass it.
>>>
>>> Sure, hardcoding is never good ;)
>>>
>>>> Given that there are no current users of the ADMA upstream, we could
>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>> size as well would be ideal.
>>>
>>> Looking at the drivers/dma/tegra210-adma.c for the
>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>> remote FIFO size would fit.
>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>> size (assuming word length, 3 == 32bits?).
>>
>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>> bytes.
>>
>>> Both threshold is set to one, so I assume currently ADMA is
>>> pushing/pulling data word by word.
>>
>> That's different. That indicates thresholds when transfers start.
>>
>>> Not sure what the burst size is used for, my guess would be that it is
>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>
>> That is the actual burst size.
>>
>>> My guess is that the threshold values are the counter limits, if the DMA
>>> request counter reaches it then ADMA would do a threshold limit worth of
>>> push/pull to ADMAIF.
>>> Or there is another register where the remote FIFO size can be written
>>> and ADMA is counting back from there until it reaches the threshold (and
>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>> filled with at least threshold amount of data?
>>>
>>> I think in both cases the threshold would be the maxburst.
>>>
>>> I suppose you have the patch for adma on how to use the fifo_size
>>> parameter? That would help understand what you are trying to achieve better.
>>
>> Its quite simple, we would just use the FIFO size to set the fields
>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>
>> Jon
>>
>
> Hi,
>
> If I understood everything correctly, the FIFO buffer is shared among
> all of the ADMA clients and hence it should be up to the ADMA driver to
> manage the quotas of the clients. So if there is only one client that
> uses ADMA at a time, then this client will get a whole FIFO buffer, but
> once another client starts to use ADMA, then the ADMA driver will have
> to reconfigure hardware to split the quotas.
The FIFO quotas are managed by the ADMAIF driver (does not exist in
mainline currently but we are working to upstream this) because it is
this device that owns and needs to configure the FIFOs. So it is really
a means to pass the information from the ADMAIF to the ADMA.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 14:26 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <71795bb0-2b8f-2b58-281c-e7e15bca3164@gmail.com>
On 06/06/2019 14:55, Dmitry Osipenko wrote:
> 06.06.2019 16:45, Dmitry Osipenko пишет:
>> 06.06.2019 15:37, Jon Hunter пишет:
>>>
>>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>>
>>>>
>>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>>
>>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>>> well.
>>>>>>>> Not exactly equal.
>>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>>> multiples of 16]
>>>>>>>
>>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>>> has register fields for both of these.
>>>>>>
>>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>>> values and what is the relation of these values to the peripheral side
>>>>>> (ADMAIF)?
>>>>>
>>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>>> determine how much space is available in the FIFO. I assume the burst
>>>>> size just limits how much data is transferred per transaction.
>>>>>
>>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>>
>>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>>> like to pass both.
>>>>>>>
>>>>>>> We are also open to other ideas.
>>>>>>
>>>>>> I can not find public documentation (I think they are walled off by
>>>>>> registration), but correct me if I'm wrong:
>>>>>
>>>>> No unfortunately, you are not wrong here :-(
>>>>>
>>>>>> ADMAIF - peripheral side
>>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>>
>>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>>> configurable depending on the use-case.
>>>>>
>>>>>> - Variable FIFO size
>>>>>
>>>>> Yes.
>>>>>
>>>>>> - sends DMA request to ADMA per words
>>>>>
>>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>>> word, yes.
>>>>>
>>>>>> ADMA - system DMA
>>>>>> - receives the DMA requests from ADMAIF
>>>>>> - counts the requests
>>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>>> - maxburst number of words probably?
>>>>>
>>>>> Sounds about right to me.
>>>>>
>>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>>> or underrun?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>>
>>>>> Yes.
>>>>>
>>>>> So currently, if you look at the ADMA driver
>>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>>> for the burst, but the FIFO size is hard-coded (see the
>>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>>> Ideally, we should not hard-code this but pass it.
>>>>
>>>> Sure, hardcoding is never good ;)
>>>>
>>>>> Given that there are no current users of the ADMA upstream, we could
>>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>>> size as well would be ideal.
>>>>
>>>> Looking at the drivers/dma/tegra210-adma.c for the
>>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>>> remote FIFO size would fit.
>>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>>> size (assuming word length, 3 == 32bits?).
>>>
>>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>>> bytes.
>>>
>>>> Both threshold is set to one, so I assume currently ADMA is
>>>> pushing/pulling data word by word.
>>>
>>> That's different. That indicates thresholds when transfers start.
>>>
>>>> Not sure what the burst size is used for, my guess would be that it is
>>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>>
>>> That is the actual burst size.
>>>
>>>> My guess is that the threshold values are the counter limits, if the DMA
>>>> request counter reaches it then ADMA would do a threshold limit worth of
>>>> push/pull to ADMAIF.
>>>> Or there is another register where the remote FIFO size can be written
>>>> and ADMA is counting back from there until it reaches the threshold (and
>>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>>> filled with at least threshold amount of data?
>>>>
>>>> I think in both cases the threshold would be the maxburst.
>>>>
>>>> I suppose you have the patch for adma on how to use the fifo_size
>>>> parameter? That would help understand what you are trying to achieve better.
>>>
>>> Its quite simple, we would just use the FIFO size to set the fields
>>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>>
>>> Jon
>>>
>>
>> Hi,
>>
>> If I understood everything correctly, the FIFO buffer is shared among
>> all of the ADMA clients and hence it should be up to the ADMA driver to
>> manage the quotas of the clients. So if there is only one client that
>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>> once another client starts to use ADMA, then the ADMA driver will have
>> to reconfigure hardware to split the quotas.
>>
>
> You could also simply hardcode the quotas per client in the ADMA driver
> if the quotas are going to be static anyway.
Essentially this is what we have done so far, but Sameer is looking for
a way to make this more programmable/flexible. We can always do that if
there is no other option indeed. However, seems like a good time to see
if there is a better way.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 14:36 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <2eab4777-79b8-0aea-c22f-ac9d11284889@nvidia.com>
On 06/06/2019 15:26, Jon Hunter wrote:
...
>>> If I understood everything correctly, the FIFO buffer is shared among
>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>> manage the quotas of the clients. So if there is only one client that
>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>> once another client starts to use ADMA, then the ADMA driver will have
>>> to reconfigure hardware to split the quotas.
>>>
>>
>> You could also simply hardcode the quotas per client in the ADMA driver
>> if the quotas are going to be static anyway.
>
> Essentially this is what we have done so far, but Sameer is looking for
> a way to make this more programmable/flexible. We can always do that if
> there is no other option indeed. However, seems like a good time to see
> if there is a better way.
My thoughts on resolving this, in order of preference, would be ...
1. Add a new 'fifo_size' variable as Sameer is proposing.
2. Update the ADMA driver to use src/dst_maxburst as the fifo size and
then have the ADMA driver set a suitable burst size for its burst
size.
3. Resort to a static configuration.
I can see that #1 only makes sense if others would find it useful,
otherwise #2, may give us enough flexibility for now.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 14:36 UTC (permalink / raw)
To: Jon Hunter, Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar,
Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <2eab4777-79b8-0aea-c22f-ac9d11284889@nvidia.com>
06.06.2019 17:26, Jon Hunter пишет:
>
> On 06/06/2019 14:55, Dmitry Osipenko wrote:
>> 06.06.2019 16:45, Dmitry Osipenko пишет:
>>> 06.06.2019 15:37, Jon Hunter пишет:
>>>>
>>>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>>>
>>>>>
>>>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>>>
>>>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>>>> well.
>>>>>>>>> Not exactly equal.
>>>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>>>> multiples of 16]
>>>>>>>>
>>>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>>>> has register fields for both of these.
>>>>>>>
>>>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>>>> values and what is the relation of these values to the peripheral side
>>>>>>> (ADMAIF)?
>>>>>>
>>>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>>>> determine how much space is available in the FIFO. I assume the burst
>>>>>> size just limits how much data is transferred per transaction.
>>>>>>
>>>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>>>
>>>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>>>> like to pass both.
>>>>>>>>
>>>>>>>> We are also open to other ideas.
>>>>>>>
>>>>>>> I can not find public documentation (I think they are walled off by
>>>>>>> registration), but correct me if I'm wrong:
>>>>>>
>>>>>> No unfortunately, you are not wrong here :-(
>>>>>>
>>>>>>> ADMAIF - peripheral side
>>>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>>>
>>>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>>>> configurable depending on the use-case.
>>>>>>
>>>>>>> - Variable FIFO size
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> - sends DMA request to ADMA per words
>>>>>>
>>>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>>>> word, yes.
>>>>>>
>>>>>>> ADMA - system DMA
>>>>>>> - receives the DMA requests from ADMAIF
>>>>>>> - counts the requests
>>>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>>>> - maxburst number of words probably?
>>>>>>
>>>>>> Sounds about right to me.
>>>>>>
>>>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>>>> or underrun?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>> So currently, if you look at the ADMA driver
>>>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>>>> for the burst, but the FIFO size is hard-coded (see the
>>>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>>>> Ideally, we should not hard-code this but pass it.
>>>>>
>>>>> Sure, hardcoding is never good ;)
>>>>>
>>>>>> Given that there are no current users of the ADMA upstream, we could
>>>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>>>> size as well would be ideal.
>>>>>
>>>>> Looking at the drivers/dma/tegra210-adma.c for the
>>>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>>>> remote FIFO size would fit.
>>>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>>>> size (assuming word length, 3 == 32bits?).
>>>>
>>>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>>>> bytes.
>>>>
>>>>> Both threshold is set to one, so I assume currently ADMA is
>>>>> pushing/pulling data word by word.
>>>>
>>>> That's different. That indicates thresholds when transfers start.
>>>>
>>>>> Not sure what the burst size is used for, my guess would be that it is
>>>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>>>
>>>> That is the actual burst size.
>>>>
>>>>> My guess is that the threshold values are the counter limits, if the DMA
>>>>> request counter reaches it then ADMA would do a threshold limit worth of
>>>>> push/pull to ADMAIF.
>>>>> Or there is another register where the remote FIFO size can be written
>>>>> and ADMA is counting back from there until it reaches the threshold (and
>>>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>>>> filled with at least threshold amount of data?
>>>>>
>>>>> I think in both cases the threshold would be the maxburst.
>>>>>
>>>>> I suppose you have the patch for adma on how to use the fifo_size
>>>>> parameter? That would help understand what you are trying to achieve better.
>>>>
>>>> Its quite simple, we would just use the FIFO size to set the fields
>>>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>>>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>>>
>>>> Jon
>>>>
>>>
>>> Hi,
>>>
>>> If I understood everything correctly, the FIFO buffer is shared among
>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>> manage the quotas of the clients. So if there is only one client that
>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>> once another client starts to use ADMA, then the ADMA driver will have
>>> to reconfigure hardware to split the quotas.
>>>
>>
>> You could also simply hardcode the quotas per client in the ADMA driver
>> if the quotas are going to be static anyway.
>
> Essentially this is what we have done so far, but Sameer is looking for
> a way to make this more programmable/flexible. We can always do that if
> there is no other option indeed. However, seems like a good time to see
> if there is a better way.
If the default values are good enough, then why bother? Otherwise it
looks like you'll need some kind of "quotas manager", please try to
figure out if it's really needed. It's always better to avoid
over-engineering.
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 14:47 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <26a8e261-4872-78df-3620-ee4a1e843fa4@gmail.com>
On 06/06/2019 15:36, Dmitry Osipenko wrote:
> 06.06.2019 17:26, Jon Hunter пишет:
>>
>> On 06/06/2019 14:55, Dmitry Osipenko wrote:
>>> 06.06.2019 16:45, Dmitry Osipenko пишет:
>>>> 06.06.2019 15:37, Jon Hunter пишет:
>>>>>
>>>>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>>>>
>>>>>>
>>>>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>>>>> well.
>>>>>>>>>> Not exactly equal.
>>>>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>>>>> multiples of 16]
>>>>>>>>>
>>>>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>>>>> has register fields for both of these.
>>>>>>>>
>>>>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>>>>> values and what is the relation of these values to the peripheral side
>>>>>>>> (ADMAIF)?
>>>>>>>
>>>>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>>>>> determine how much space is available in the FIFO. I assume the burst
>>>>>>> size just limits how much data is transferred per transaction.
>>>>>>>
>>>>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>>>>
>>>>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>>>>> like to pass both.
>>>>>>>>>
>>>>>>>>> We are also open to other ideas.
>>>>>>>>
>>>>>>>> I can not find public documentation (I think they are walled off by
>>>>>>>> registration), but correct me if I'm wrong:
>>>>>>>
>>>>>>> No unfortunately, you are not wrong here :-(
>>>>>>>
>>>>>>>> ADMAIF - peripheral side
>>>>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>>>>
>>>>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>>>>> configurable depending on the use-case.
>>>>>>>
>>>>>>>> - Variable FIFO size
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> - sends DMA request to ADMA per words
>>>>>>>
>>>>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>>>>> word, yes.
>>>>>>>
>>>>>>>> ADMA - system DMA
>>>>>>>> - receives the DMA requests from ADMAIF
>>>>>>>> - counts the requests
>>>>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>>>>> - maxburst number of words probably?
>>>>>>>
>>>>>>> Sounds about right to me.
>>>>>>>
>>>>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>>>>> or underrun?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>> So currently, if you look at the ADMA driver
>>>>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>>>>> for the burst, but the FIFO size is hard-coded (see the
>>>>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>>>>> Ideally, we should not hard-code this but pass it.
>>>>>>
>>>>>> Sure, hardcoding is never good ;)
>>>>>>
>>>>>>> Given that there are no current users of the ADMA upstream, we could
>>>>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>>>>> size as well would be ideal.
>>>>>>
>>>>>> Looking at the drivers/dma/tegra210-adma.c for the
>>>>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>>>>> remote FIFO size would fit.
>>>>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>>>>> size (assuming word length, 3 == 32bits?).
>>>>>
>>>>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>>>>> bytes.
>>>>>
>>>>>> Both threshold is set to one, so I assume currently ADMA is
>>>>>> pushing/pulling data word by word.
>>>>>
>>>>> That's different. That indicates thresholds when transfers start.
>>>>>
>>>>>> Not sure what the burst size is used for, my guess would be that it is
>>>>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>>>>
>>>>> That is the actual burst size.
>>>>>
>>>>>> My guess is that the threshold values are the counter limits, if the DMA
>>>>>> request counter reaches it then ADMA would do a threshold limit worth of
>>>>>> push/pull to ADMAIF.
>>>>>> Or there is another register where the remote FIFO size can be written
>>>>>> and ADMA is counting back from there until it reaches the threshold (and
>>>>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>>>>> filled with at least threshold amount of data?
>>>>>>
>>>>>> I think in both cases the threshold would be the maxburst.
>>>>>>
>>>>>> I suppose you have the patch for adma on how to use the fifo_size
>>>>>> parameter? That would help understand what you are trying to achieve better.
>>>>>
>>>>> Its quite simple, we would just use the FIFO size to set the fields
>>>>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>>>>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>>>>
>>>>> Jon
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> If I understood everything correctly, the FIFO buffer is shared among
>>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>>> manage the quotas of the clients. So if there is only one client that
>>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>>> once another client starts to use ADMA, then the ADMA driver will have
>>>> to reconfigure hardware to split the quotas.
>>>>
>>>
>>> You could also simply hardcode the quotas per client in the ADMA driver
>>> if the quotas are going to be static anyway.
>>
>> Essentially this is what we have done so far, but Sameer is looking for
>> a way to make this more programmable/flexible. We can always do that if
>> there is no other option indeed. However, seems like a good time to see
>> if there is a better way.
>
> If the default values are good enough, then why bother? Otherwise it
> looks like you'll need some kind of "quotas manager", please try to
> figure out if it's really needed. It's always better to avoid
> over-engineering.
We are proposing to add one variable. Hardly seems like over-engineering
to me. Again we are just looking to see if this would be acceptable or not.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 15:18 UTC (permalink / raw)
To: Jon Hunter, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <ac9a965d-0166-3d80-5ac4-ae841d7ae726@nvidia.com>
06.06.2019 17:25, Jon Hunter пишет:
>
>
> On 06/06/2019 14:45, Dmitry Osipenko wrote:
>> 06.06.2019 15:37, Jon Hunter пишет:
>>>
>>> On 06/06/2019 12:54, Peter Ujfalusi wrote:
>>>>
>>>>
>>>> On 06/06/2019 13.49, Jon Hunter wrote:
>>>>>
>>>>> On 06/06/2019 11:22, Peter Ujfalusi wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>>> It does sounds like that FIFO_SIZE == src/dst_maxburst in your case as
>>>>>>>>> well.
>>>>>>>> Not exactly equal.
>>>>>>>> ADMA burst_size can range from 1(WORD) to 16(WORDS)
>>>>>>>> FIFO_SIZE can be adjusted from 16(WORDS) to 1024(WORDS) [can vary in
>>>>>>>> multiples of 16]
>>>>>>>
>>>>>>> So I think that the key thing to highlight here, is that the as Sameer
>>>>>>> highlighted above for the Tegra ADMA there are two values that need to
>>>>>>> be programmed; the DMA client FIFO size and the max burst size. The ADMA
>>>>>>> has register fields for both of these.
>>>>>>
>>>>>> How does the ADMA uses the 'client FIFO size' and 'max burst size'
>>>>>> values and what is the relation of these values to the peripheral side
>>>>>> (ADMAIF)?
>>>>>
>>>>> Per Sameer's previous comment, the FIFO size is used by the ADMA to
>>>>> determine how much space is available in the FIFO. I assume the burst
>>>>> size just limits how much data is transferred per transaction.
>>>>>
>>>>>>> As you can see from the above the FIFO size can be much greater than the
>>>>>>> burst size and so ideally both of these values would be passed to the DMA.
>>>>>>>
>>>>>>> We could get by with just passing the FIFO size (as the max burst size)
>>>>>>> and then have the DMA driver set the max burst size depending on this,
>>>>>>> but this does feel quite correct for this DMA. Hence, ideally, we would
>>>>>>> like to pass both.
>>>>>>>
>>>>>>> We are also open to other ideas.
>>>>>>
>>>>>> I can not find public documentation (I think they are walled off by
>>>>>> registration), but correct me if I'm wrong:
>>>>>
>>>>> No unfortunately, you are not wrong here :-(
>>>>>
>>>>>> ADMAIF - peripheral side
>>>>>> - kind of a small DMA for audio preipheral(s)?
>>>>>
>>>>> Yes this is the interface to the APE (audio processing engine) and data
>>>>> sent to the ADMAIF is then sent across a crossbar to one of many
>>>>> devices/interfaces (I2S, DMIC, etc). Basically a large mux that is user
>>>>> configurable depending on the use-case.
>>>>>
>>>>>> - Variable FIFO size
>>>>>
>>>>> Yes.
>>>>>
>>>>>> - sends DMA request to ADMA per words
>>>>>
>>>>> From Sameer's notes it says the ADMAIF send a signal to the ADMA per
>>>>> word, yes.
>>>>>
>>>>>> ADMA - system DMA
>>>>>> - receives the DMA requests from ADMAIF
>>>>>> - counts the requests
>>>>>> - based on some threshold of the counter it will send/read from ADMAIF?
>>>>>> - maxburst number of words probably?
>>>>>
>>>>> Sounds about right to me.
>>>>>
>>>>>> ADMA needs to know the ADMAIF's FIFO size because, it is the one who is
>>>>>> managing that FIFO from the outside, making sure that it does not over
>>>>>> or underrun?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> And it is the one who sets the pace (in effect the DMA burst size - how
>>>>>> many bytes the DMA jumps between refills) of refills to the ADMAIF's FIFO?
>>>>>
>>>>> Yes.
>>>>>
>>>>> So currently, if you look at the ADMA driver
>>>>> (drivers/dma/tegra210-adma.c) you will see we use the src/dst_maxburst
>>>>> for the burst, but the FIFO size is hard-coded (see the
>>>>> TEGRA210_FIFO_CTRL_DEFAULT and TEGRA186_FIFO_CTRL_DEFAULT definitions).
>>>>> Ideally, we should not hard-code this but pass it.
>>>>
>>>> Sure, hardcoding is never good ;)
>>>>
>>>>> Given that there are no current users of the ADMA upstream, we could
>>>>> change the usage of the src/dst_maxburst, but being able to set the FIFO
>>>>> size as well would be ideal.
>>>>
>>>> Looking at the drivers/dma/tegra210-adma.c for the
>>>> TEGRA*_FIFO_CTRL_DEFAULT definition it is still not clear where the
>>>> remote FIFO size would fit.
>>>> There are fields for overflow and starvation(?) thresholds and TX/RX
>>>> size (assuming word length, 3 == 32bits?).
>>>
>>> The TX/RX size are the FIFO size. So 3 equates to a FIFO size of 3 * 64
>>> bytes.
>>>
>>>> Both threshold is set to one, so I assume currently ADMA is
>>>> pushing/pulling data word by word.
>>>
>>> That's different. That indicates thresholds when transfers start.
>>>
>>>> Not sure what the burst size is used for, my guess would be that it is
>>>> used on the memory (DDR) side for optimized, more efficient accesses?
>>>
>>> That is the actual burst size.
>>>
>>>> My guess is that the threshold values are the counter limits, if the DMA
>>>> request counter reaches it then ADMA would do a threshold limit worth of
>>>> push/pull to ADMAIF.
>>>> Or there is another register where the remote FIFO size can be written
>>>> and ADMA is counting back from there until it reaches the threshold (and
>>>> pushes/pulling again threshold amount of data) so it keeps the FIFO
>>>> filled with at least threshold amount of data?
>>>>
>>>> I think in both cases the threshold would be the maxburst.
>>>>
>>>> I suppose you have the patch for adma on how to use the fifo_size
>>>> parameter? That would help understand what you are trying to achieve better.
>>>
>>> Its quite simple, we would just use the FIFO size to set the fields
>>> TEGRAXXX_ADMA_CH_FIFO_CTRL_TXSIZE/RXSIZE in the
>>> TEGRAXXX_ADMA_CH_FIFO_CTRL register. That's all.
>>>
>>> Jon
>>>
>>
>> Hi,
>>
>> If I understood everything correctly, the FIFO buffer is shared among
>> all of the ADMA clients and hence it should be up to the ADMA driver to
>> manage the quotas of the clients. So if there is only one client that
>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>> once another client starts to use ADMA, then the ADMA driver will have
>> to reconfigure hardware to split the quotas.
>
> The FIFO quotas are managed by the ADMAIF driver (does not exist in
> mainline currently but we are working to upstream this) because it is
> this device that owns and needs to configure the FIFOs. So it is really
> a means to pass the information from the ADMAIF to the ADMA.
So you'd want to reserve a larger FIFO for an audio channel that has a
higher audio rate since it will perform reads more often. You could also
prioritize one channel over the others, like in a case of audio call for
example.
Is the shared buffer smaller than may be needed by clients in a worst
case scenario? If you could split the quotas statically such that each
client won't ever starve, then seems there is no much need in the
dynamic configuration.
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 16:32 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <50e1f9ed-1ea0-38f6-1a77-febd6a3a0848@gmail.com>
On 06/06/2019 16:18, Dmitry Osipenko wrote:
...
>>> If I understood everything correctly, the FIFO buffer is shared among
>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>> manage the quotas of the clients. So if there is only one client that
>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>> once another client starts to use ADMA, then the ADMA driver will have
>>> to reconfigure hardware to split the quotas.
>>
>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>> mainline currently but we are working to upstream this) because it is
>> this device that owns and needs to configure the FIFOs. So it is really
>> a means to pass the information from the ADMAIF to the ADMA.
>
> So you'd want to reserve a larger FIFO for an audio channel that has a
> higher audio rate since it will perform reads more often. You could also
> prioritize one channel over the others, like in a case of audio call for
> example.
>
> Is the shared buffer smaller than may be needed by clients in a worst
> case scenario? If you could split the quotas statically such that each
> client won't ever starve, then seems there is no much need in the
> dynamic configuration.
Actually, this is still very much relevant for the static case. Even if
we defined a static configuration of the FIFO mapping in the ADMAIF
driver we still need to pass this information to the ADMA. I don't
really like the idea of having it statically defined in two different
drivers.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 16:44 UTC (permalink / raw)
To: Jon Hunter, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <4b098fb6-1a5b-1100-ae16-978a887c9535@nvidia.com>
06.06.2019 19:32, Jon Hunter пишет:
>
> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>
> ...
>
>>>> If I understood everything correctly, the FIFO buffer is shared among
>>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>>> manage the quotas of the clients. So if there is only one client that
>>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>>> once another client starts to use ADMA, then the ADMA driver will have
>>>> to reconfigure hardware to split the quotas.
>>>
>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>> mainline currently but we are working to upstream this) because it is
>>> this device that owns and needs to configure the FIFOs. So it is really
>>> a means to pass the information from the ADMAIF to the ADMA.
>>
>> So you'd want to reserve a larger FIFO for an audio channel that has a
>> higher audio rate since it will perform reads more often. You could also
>> prioritize one channel over the others, like in a case of audio call for
>> example.
>>
>> Is the shared buffer smaller than may be needed by clients in a worst
>> case scenario? If you could split the quotas statically such that each
>> client won't ever starve, then seems there is no much need in the
>> dynamic configuration.
>
> Actually, this is still very much relevant for the static case. Even if
> we defined a static configuration of the FIFO mapping in the ADMAIF
> driver we still need to pass this information to the ADMA. I don't
> really like the idea of having it statically defined in two different
> drivers.
Ah, so you need to apply the same configuration in two places. Correct?
Are ADMAIF and ADMA really two different hardware blocks? Or you
artificially decoupled the ADMA driver?
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-06 16:53 UTC (permalink / raw)
To: Dmitry Osipenko, Peter Ujfalusi, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <e6741e07-be0c-d16b-36d7-77a3288f0500@gmail.com>
On 06/06/2019 17:44, Dmitry Osipenko wrote:
> 06.06.2019 19:32, Jon Hunter пишет:
>>
>> On 06/06/2019 16:18, Dmitry Osipenko wrote:
>>
>> ...
>>
>>>>> If I understood everything correctly, the FIFO buffer is shared among
>>>>> all of the ADMA clients and hence it should be up to the ADMA driver to
>>>>> manage the quotas of the clients. So if there is only one client that
>>>>> uses ADMA at a time, then this client will get a whole FIFO buffer, but
>>>>> once another client starts to use ADMA, then the ADMA driver will have
>>>>> to reconfigure hardware to split the quotas.
>>>>
>>>> The FIFO quotas are managed by the ADMAIF driver (does not exist in
>>>> mainline currently but we are working to upstream this) because it is
>>>> this device that owns and needs to configure the FIFOs. So it is really
>>>> a means to pass the information from the ADMAIF to the ADMA.
>>>
>>> So you'd want to reserve a larger FIFO for an audio channel that has a
>>> higher audio rate since it will perform reads more often. You could also
>>> prioritize one channel over the others, like in a case of audio call for
>>> example.
>>>
>>> Is the shared buffer smaller than may be needed by clients in a worst
>>> case scenario? If you could split the quotas statically such that each
>>> client won't ever starve, then seems there is no much need in the
>>> dynamic configuration.
>>
>> Actually, this is still very much relevant for the static case. Even if
>> we defined a static configuration of the FIFO mapping in the ADMAIF
>> driver we still need to pass this information to the ADMA. I don't
>> really like the idea of having it statically defined in two different
>> drivers.
>
> Ah, so you need to apply the same configuration in two places. Correct?
>
> Are ADMAIF and ADMA really two different hardware blocks? Or you
> artificially decoupled the ADMA driver?
These are two different hardware modules with their own register sets.
Yes otherwise, it would be a lot simpler!
Jon
--
nvpublic
^ 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