* 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
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 17:25 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: <a652b103-979d-7910-5e3f-ec4bca3a3a3b@nvidia.com>
06.06.2019 19:53, Jon Hunter пишет:
>
> 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!
The register sets are indeed separated, but it looks like that ADMAIF is
really a part of ADMA that is facing to Audio Crossbar. No? What is the
purpose of ADMAIF? Maybe you could amend the ADMA hardware description
with the ADMAIF addition until it's too late.
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-06 17:56 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: <457eb5e1-40cc-8c0f-e21c-3881c3c04de2@gmail.com>
06.06.2019 20:25, Dmitry Osipenko пишет:
> 06.06.2019 19:53, Jon Hunter пишет:
>>
>> 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!
>
> The register sets are indeed separated, but it looks like that ADMAIF is
> really a part of ADMA that is facing to Audio Crossbar. No? What is the
> purpose of ADMAIF? Maybe you could amend the ADMA hardware description
> with the ADMAIF addition until it's too late.
>
Ugh.. I now regret looking at the TRM. That Audio Processor Engine is a
horrifying beast, it even has FPGA :)
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-07 5:50 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: <4593f37c-5e89-8559-4e80-99dbfe4235de@nvidia.com>
Jon,
On 06/06/2019 15.37, Jon Hunter wrote:
>> 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.
Hrm, it is still not clear how all of these fits together.
What happens if you configure ADMA side:
BURST = 10
TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
*THRES = 5
And if you change the *THRES to 10?
And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
And if you change the BURST to 5?
In other words what is the relation between all of these?
There must be a rule and constraints around these and if we do really
need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
generic way so others could benefit without 'misusing' a fifo_size
parameter for similar, but not quite fifo_size information.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
From: Geert Uytterhoeven @ 2019-06-07 7:37 UTC (permalink / raw)
To: Dinh Nguyen
Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <55cc6016-f297-539d-df08-777903b79005@kernel.org>
Hi Dinh,
On Wed, Jun 5, 2019 at 5:31 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> On 6/5/19 9:41 AM, Dinh Nguyen wrote:
> > On 6/4/19 11:31 AM, Geert Uytterhoeven wrote:
> >> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> >>> On 6/4/19 7:14 AM, Vinod Koul wrote:
> >>>> On 23-05-19, 19:28, Dinh Nguyen wrote:
> >>>>> The DMA controller on some SoCs can be held in reset, and thus requires
> >>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset
> >>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an
> >>>>> additional reset signal, referred to as the OCP.
> >>>>>
> >>>>> Add code to get the reset property from the device tree for deassert and
> >>>>> assert.
> >>>>>
> >>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> >>>>> --- a/drivers/dma/pl330.c
> >>>>> +++ b/drivers/dma/pl330.c
> >>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
> >>>>>
> >>>>> amba_set_drvdata(adev, pl330);
> >>>>>
> >>>>> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
> >>>>> + if (IS_ERR(pl330->rstc)) {
> >>>>> + dev_err(&adev->dev, "No reset controller specified.\n");
"No reset controller specified.\n"
> >>>>
> >>>> Wasnt this optional??
> >>>
> >>> Yes, this is optional. The call devm_reset_control_get_optional() will
> >>> just return NULL if the reset property is not there, but an error
> >>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
> >>> error checking.
> >>
> >> So the error message is incorrect, as this is a real error condition?
> >
> > Yes, you're right! Will correct in V2.
>
> Looking at this again, I think the error message is correct. The
> optional call will return NULL if the resets property is not specified,
> and will return an error pointer if the reset propert is specified, but
> the pointer to the reset controller is not found.
>
> So I think the error message is correct.
Please reread the error message, and what you wrote above.
Error message: "No reset controller specified".
Rationale: NULL (i.e. no error) if "the resets property is not specified".
If an error pointer is returned, this may be due to probe deferral (to be
propagated, but further ignored), or due to a real failure.
So IMHO the code should read:
if (IS_ERR(pl330->rstc)) {
if (PTR_ERR(pl330->rstc) != -EPROBE_DEFER)
dev_err(&adev->dev, "Failed to get reset.\n");
return PTR_ERR(pl330->rstc);
} else { ... }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-07 9:18 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: <d0db90e3-3d05-dfba-8768-28511d9ee3ac@ti.com>
On 07/06/2019 06:50, Peter Ujfalusi wrote:
> Jon,
>
> On 06/06/2019 15.37, Jon Hunter wrote:
>>> 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.
>
> Hrm, it is still not clear how all of these fits together.
>
> What happens if you configure ADMA side:
> BURST = 10
> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
> *THRES = 5
>
> And if you change the *THRES to 10?
> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
> And if you change the BURST to 5?
>
> In other words what is the relation between all of these?
So the THRES values are only applicable when the FETCHING_POLICY (bit 31
of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes;
a threshold based transfer mode or a burst based transfer mode. The
burst mode transfer data as and when there is room for a burst in the FIFO.
We use the burst mode and so we really should not be setting the THRES
fields as they are not applicable. Oh well something else to correct,
but this is side issue.
> There must be a rule and constraints around these and if we do really
> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
> generic way so others could benefit without 'misusing' a fifo_size
> parameter for similar, but not quite fifo_size information.
Yes I see what you are saying. One option would be to define both a
src/dst_maxburst and src/dst_minburst size. Then we could use max for
the FIFO size and min for the actual burst size.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-07 9:24 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: <457eb5e1-40cc-8c0f-e21c-3881c3c04de2@gmail.com>
On 06/06/2019 18:25, Dmitry Osipenko wrote:
> 06.06.2019 19:53, Jon Hunter пишет:
>>
>> 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!
>
> The register sets are indeed separated, but it looks like that ADMAIF is
> really a part of ADMA that is facing to Audio Crossbar. No? What is the
> purpose of ADMAIF? Maybe you could amend the ADMA hardware description
> with the ADMAIF addition until it's too late.
The ADMA can perform the following transfers (per the CH_CONFIG
register) ...
MEMORY_TO_MEMORY
AHUB_TO_MEMORY
MEMORY_TO_AHUB
AHUB_TO_AHUB
Hence it is possible to use the ADMA to do memory-to-memory transfers
that do not involve the ADMAIF.
So no the ADMAIF is not part of the ADMA. It is purely the interface to
the crossbar (AHUB/APE), but from a hardware standpoint they are
separate. And so no we will not amend the hardware description.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-07 10:27 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: <5208a50a-9ca0-8f24-9ad0-d7503ec53f1c@nvidia.com>
On 07/06/2019 10:18, Jon Hunter wrote:
>
> On 07/06/2019 06:50, Peter Ujfalusi wrote:
>> Jon,
>>
>> On 06/06/2019 15.37, Jon Hunter wrote:
>>>> 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.
>>
>> Hrm, it is still not clear how all of these fits together.
>>
>> What happens if you configure ADMA side:
>> BURST = 10
>> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
>> *THRES = 5
>>
>> And if you change the *THRES to 10?
>> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
>> And if you change the BURST to 5?
>>
>> In other words what is the relation between all of these?
>
> So the THRES values are only applicable when the FETCHING_POLICY (bit 31
> of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes;
> a threshold based transfer mode or a burst based transfer mode. The
> burst mode transfer data as and when there is room for a burst in the FIFO.
>
> We use the burst mode and so we really should not be setting the THRES
> fields as they are not applicable. Oh well something else to correct,
> but this is side issue.
>
>> There must be a rule and constraints around these and if we do really
>> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
>> generic way so others could benefit without 'misusing' a fifo_size
>> parameter for similar, but not quite fifo_size information.
>
> Yes I see what you are saying. One option would be to define both a
> src/dst_maxburst and src/dst_minburst size. Then we could use max for
> the FIFO size and min for the actual burst size.
Actually, we don't even need to do that. We only use src_maxburst for
DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why
we could not use both the src_maxburst for dst_maxburst for both
DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one
represents that DMA burst size.
Sorry should have thought of that before. Any objections to using these
this way? Obviously we would document is clearly in the driver.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* [PATCH trivial] dmaengine: Grammar s/the its/its/, s/need/needs/
From: Geert Uytterhoeven @ 2019-06-07 11:30 UTC (permalink / raw)
To: Vinod Koul, Dan Williams, Jiri Kosina
Cc: dmaengine, linux-kernel, Geert Uytterhoeven
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/dma/dmaengine.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index c92260b1b8972346..03ac4b96117cd8db 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -61,7 +61,7 @@ static long dmaengine_ref_count;
/* --- sysfs implementation --- */
/**
- * dev_to_dma_chan - convert a device pointer to the its sysfs container object
+ * dev_to_dma_chan - convert a device pointer to its sysfs container object
* @dev - device node
*
* Must be called under dma_list_mutex
@@ -705,7 +705,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
chan = acpi_dma_request_slave_chan_by_name(dev, name);
if (chan) {
- /* Valid channel found or requester need to be deferred */
+ /* Valid channel found or requester needs to be deferred */
if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
return chan;
}
--
2.17.1
^ permalink raw reply related
* [PATCH/RFC] dmaengine: Create symlinks from DMA channels to slaves
From: Geert Uytterhoeven @ 2019-06-07 11:38 UTC (permalink / raw)
To: Dan Williams, Vinod Koul
Cc: dmaengine, linux-renesas-soc, linux-kernel, Geert Uytterhoeven
Currently it is not easy to find out which DMA channels are in use, and
by which slave devices.
Fix this by creating in sysfs a "slave" symlink from the DMA channel to
the actual slave device when a channel is requested, and removing it
again when the channel is released.
For now this is limited to DT and ACPI.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Questions:
1. Do you think this is useful?
2. Should backlinks (e.g. "dma:<name>") be created from the slave
device to the DMA channel?
This requires storing the name in struct dma_chan, for later
symlink removal.
3. Should this be extended to other ways of requesting channels?
In many cases, no device pointer is available, so a device pointer
parameter has to be added to all DMA channel request APIs that
don't have it yet.
---
drivers/dma/dmaengine.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 03ac4b96117cd8db..c11476f76fc96bcf 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -706,6 +706,10 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
if (chan) {
/* Valid channel found or requester needs to be deferred */
+ if (!IS_ERR(chan) &&
+ sysfs_create_link(&chan->dev->device.kobj, &dev->kobj,
+ "slave"))
+ dev_err(dev, "Cannot create DMA slave symlink\n");
if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
return chan;
}
@@ -786,6 +790,7 @@ void dma_release_channel(struct dma_chan *chan)
/* drop PRIVATE cap enabled by __dma_request_channel() */
if (--chan->device->privatecnt == 0)
dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask);
+ sysfs_remove_link(&chan->dev->device.kobj, "slave");
mutex_unlock(&dma_list_mutex);
}
EXPORT_SYMBOL_GPL(dma_release_channel);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-07 12:17 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: <ba845a19-5dfb-a891-719f-43821b2dd412@nvidia.com>
On 07/06/2019 13.27, Jon Hunter wrote:
>>> Hrm, it is still not clear how all of these fits together.
>>>
>>> What happens if you configure ADMA side:
>>> BURST = 10
>>> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
>>> *THRES = 5
>>>
>>> And if you change the *THRES to 10?
>>> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
>>> And if you change the BURST to 5?
>>>
>>> In other words what is the relation between all of these?
>>
>> So the THRES values are only applicable when the FETCHING_POLICY (bit 31
>> of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes;
>> a threshold based transfer mode or a burst based transfer mode. The
>> burst mode transfer data as and when there is room for a burst in the FIFO.
>>
>> We use the burst mode and so we really should not be setting the THRES
>> fields as they are not applicable. Oh well something else to correct,
>> but this is side issue.
>>
>>> There must be a rule and constraints around these and if we do really
>>> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
>>> generic way so others could benefit without 'misusing' a fifo_size
>>> parameter for similar, but not quite fifo_size information.
>>
>> Yes I see what you are saying. One option would be to define both a
>> src/dst_maxburst and src/dst_minburst size. Then we could use max for
>> the FIFO size and min for the actual burst size.
>
> Actually, we don't even need to do that. We only use src_maxburst for
> DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why
> we could not use both the src_maxburst for dst_maxburst for both
> DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one
> represents that DMA burst size.
>
> Sorry should have thought of that before. Any objections to using these
> this way? Obviously we would document is clearly in the driver.
Imho if you can explain it without using 'HACK' in the sentences it
might be OK, but it does not feel right.
However since your ADMA and ADMIF is highly coupled and it does needs
special maxburst information (burst and allocated FIFO depth) I would
rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
So lower 1 byte is the burst value you want from ADMA
the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
Sure, you need a header for this to make sure there is no
misunderstanding between the two sides.
Or pass the allocated FIFO size via maxburst and then the ADMA driver
will pick a 'good/safe' burst value for it.
Or new member, but do you need two of them for src/dst? Probably
fifo_depth is better word for it, or allocated_fifo_depth.
- 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-07 12:58 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: <e67a2d7c-5bd1-93ad-fe75-afcab38bc17c@ti.com>
On 07/06/2019 13:17, Peter Ujfalusi wrote:
>
>
> On 07/06/2019 13.27, Jon Hunter wrote:
>>>> Hrm, it is still not clear how all of these fits together.
>>>>
>>>> What happens if you configure ADMA side:
>>>> BURST = 10
>>>> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */
>>>> *THRES = 5
>>>>
>>>> And if you change the *THRES to 10?
>>>> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?)
>>>> And if you change the BURST to 5?
>>>>
>>>> In other words what is the relation between all of these?
>>>
>>> So the THRES values are only applicable when the FETCHING_POLICY (bit 31
>>> of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes;
>>> a threshold based transfer mode or a burst based transfer mode. The
>>> burst mode transfer data as and when there is room for a burst in the FIFO.
>>>
>>> We use the burst mode and so we really should not be setting the THRES
>>> fields as they are not applicable. Oh well something else to correct,
>>> but this is side issue.
>>>
>>>> There must be a rule and constraints around these and if we do really
>>>> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a
>>>> generic way so others could benefit without 'misusing' a fifo_size
>>>> parameter for similar, but not quite fifo_size information.
>>>
>>> Yes I see what you are saying. One option would be to define both a
>>> src/dst_maxburst and src/dst_minburst size. Then we could use max for
>>> the FIFO size and min for the actual burst size.
>>
>> Actually, we don't even need to do that. We only use src_maxburst for
>> DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why
>> we could not use both the src_maxburst for dst_maxburst for both
>> DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one
>> represents that DMA burst size.
>>
>> Sorry should have thought of that before. Any objections to using these
>> this way? Obviously we would document is clearly in the driver.
>
> Imho if you can explain it without using 'HACK' in the sentences it
> might be OK, but it does not feel right.
I don't perceive this as a hack. Although from looking at the
description of the src/dst_maxburst these are burst size with regard to
the device, so maybe it is a stretch.
> However since your ADMA and ADMIF is highly coupled and it does needs
> special maxburst information (burst and allocated FIFO depth) I would
> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>
> ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
> ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
>
> So lower 1 byte is the burst value you want from ADMA
> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>
> Sure, you need a header for this to make sure there is no
> misunderstanding between the two sides.
I don't like this because as I mentioned to Dmitry, the ADMA can perform
memory-to-memory transfers where such encoding would not be applicable.
That does not align with the description in the
include/linux/dmaengine.h either.
> Or pass the allocated FIFO size via maxburst and then the ADMA driver
> will pick a 'good/safe' burst value for it.
>
> Or new member, but do you need two of them for src/dst? Probably
> fifo_depth is better word for it, or allocated_fifo_depth.
Right, so looking at the struct dma_slave_config we have ...
u32 src_maxburst;
u32 dst_maxburst;
u32 src_port_window_size;
u32 dst_port_window_size;
Now if we could make these window sizes a union like the following this
could work ...
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8fcdee1c0cf9..851251263527 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -360,8 +360,14 @@ struct dma_slave_config {
enum dma_slave_buswidth dst_addr_width;
u32 src_maxburst;
u32 dst_maxburst;
- u32 src_port_window_size;
- u32 dst_port_window_size;
+ union {
+ u32 port_window_size;
+ u32 port_fifo_size;
+ } src;
+ union {
+ u32 port_window_size;
+ u32 port_fifo_size;
+ } dst;
Cheers,
Jon
--
nvpublic
^ permalink raw reply related
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Peter Ujfalusi @ 2019-06-07 13:35 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: <a65f2b07-4a3a-7f83-e21f-8b374844a4b9@nvidia.com>
On 07/06/2019 15.58, Jon Hunter wrote:
>> Imho if you can explain it without using 'HACK' in the sentences it
>> might be OK, but it does not feel right.
>
> I don't perceive this as a hack. Although from looking at the
> description of the src/dst_maxburst these are burst size with regard to
> the device, so maybe it is a stretch.
>
>> However since your ADMA and ADMIF is highly coupled and it does needs
>> special maxburst information (burst and allocated FIFO depth) I would
>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>
>> ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
>> ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
>>
>> So lower 1 byte is the burst value you want from ADMA
>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>
>> Sure, you need a header for this to make sure there is no
>> misunderstanding between the two sides.
>
> I don't like this because as I mentioned to Dmitry, the ADMA can perform
> memory-to-memory transfers where such encoding would not be applicable.
mem2mem does not really use dma_slave_config, it is for used by
is_slave_direction() == true type of transfers.
But true, if you use ADMA against anything other than ADMAIF then this
might be not right for non cyclic transfers.
> That does not align with the description in the
> include/linux/dmaengine.h either.
True.
>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>> will pick a 'good/safe' burst value for it.
>>
>> Or new member, but do you need two of them for src/dst? Probably
>> fifo_depth is better word for it, or allocated_fifo_depth.
>
> Right, so looking at the struct dma_slave_config we have ...
>
> u32 src_maxburst;
> u32 dst_maxburst;
> u32 src_port_window_size;
> u32 dst_port_window_size;
>
> Now if we could make these window sizes a union like the following this
> could work ...
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8fcdee1c0cf9..851251263527 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -360,8 +360,14 @@ struct dma_slave_config {
> enum dma_slave_buswidth dst_addr_width;
> u32 src_maxburst;
> u32 dst_maxburst;
> - u32 src_port_window_size;
> - u32 dst_port_window_size;
> + union {
> + u32 port_window_size;
> + u32 port_fifo_size;
> + } src;
> + union {
> + u32 port_window_size;
> + u32 port_fifo_size;
> + } dst;
What if in the future someone will have a setup where they would need both?
So not sure. Your problems are coming from a split DMA setup where the
two are highly coupled, but sits in a different place and need to be
configured as one device.
I think xilinx_dma is facing with similar issues and they have a custom
API to set parameters which does not fit or is peripheral specific:
include/linux/dma/xilinx_dma.h
Not sure if that is an acceptable solution.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* [PATCH] dma: jz4780: Make probe function __init_or_module
From: Paul Cercueil @ 2019-06-07 16:07 UTC (permalink / raw)
To: Zubair Lutfullah Kakakhel, Vinod Koul
Cc: dmaengine, linux-kernel, od, Paul Cercueil
This allows the probe function to be dropped after the kernel finished
its initialization, in the case where the driver was not compiled as a
module.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/dma/dma-jz4780.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 7204fdeff6c5..b2f7e6660ad6 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -815,7 +815,7 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
}
}
-static int jz4780_dma_probe(struct platform_device *pdev)
+static int __init_or_module jz4780_dma_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
const struct jz4780_dma_soc_data *soc_data;
@@ -966,7 +966,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
return ret;
}
-static int jz4780_dma_remove(struct platform_device *pdev)
+static int __exit jz4780_dma_remove(struct platform_device *pdev)
{
struct jz4780_dma_dev *jzdma = platform_get_drvdata(pdev);
int i;
--
2.21.0.593.g511ec345e18
^ permalink raw reply related
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Dmitry Osipenko @ 2019-06-07 20:53 UTC (permalink / raw)
To: Peter Ujfalusi, Jon Hunter, Sameer Pujar, Vinod Koul
Cc: dan.j.williams, tiwai, dmaengine, linux-kernel, sharadg,
rlokhande, dramesh, mkumard, linux-tegra
In-Reply-To: <56aa6f45-cfd8-7f1e-9392-628ceb58093f@ti.com>
07.06.2019 16:35, Peter Ujfalusi пишет:
>
>
> On 07/06/2019 15.58, Jon Hunter wrote:
>>> Imho if you can explain it without using 'HACK' in the sentences it
>>> might be OK, but it does not feel right.
>>
>> I don't perceive this as a hack. Although from looking at the
>> description of the src/dst_maxburst these are burst size with regard to
>> the device, so maybe it is a stretch.
>>
>>> However since your ADMA and ADMIF is highly coupled and it does needs
>>> special maxburst information (burst and allocated FIFO depth) I would
>>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>>
>>> ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
>>> ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
>>>
>>> So lower 1 byte is the burst value you want from ADMA
>>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>>
>>> Sure, you need a header for this to make sure there is no
>>> misunderstanding between the two sides.
>>
>> I don't like this because as I mentioned to Dmitry, the ADMA can perform
>> memory-to-memory transfers where such encoding would not be applicable.
>
> mem2mem does not really use dma_slave_config, it is for used by
> is_slave_direction() == true type of transfers.
>
> But true, if you use ADMA against anything other than ADMAIF then this
> might be not right for non cyclic transfers.
>
>> That does not align with the description in the
>> include/linux/dmaengine.h either.
>
> True.
>
>>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>>> will pick a 'good/safe' burst value for it.
>>>
>>> Or new member, but do you need two of them for src/dst? Probably
>>> fifo_depth is better word for it, or allocated_fifo_depth.
>>
>> Right, so looking at the struct dma_slave_config we have ...
>>
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> u32 src_port_window_size;
>> u32 dst_port_window_size;
>>
>> Now if we could make these window sizes a union like the following this
>> could work ...
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 8fcdee1c0cf9..851251263527 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -360,8 +360,14 @@ struct dma_slave_config {
>> enum dma_slave_buswidth dst_addr_width;
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> - u32 src_port_window_size;
>> - u32 dst_port_window_size;
>> + union {
>> + u32 port_window_size;
>> + u32 port_fifo_size;
>> + } src;
>> + union {
>> + u32 port_window_size;
>> + u32 port_fifo_size;
>> + } dst;
>
> What if in the future someone will have a setup where they would need both?
>
> So not sure. Your problems are coming from a split DMA setup where the
> two are highly coupled, but sits in a different place and need to be
> configured as one device.
>
> I think xilinx_dma is facing with similar issues and they have a custom
> API to set parameters which does not fit or is peripheral specific:
> include/linux/dma/xilinx_dma.h
>
> Not sure if that is an acceptable solution.
If there are no other drivers with the exactly same requirement, then
the custom API is an a good variant given that there is a precedent
already. It is always possible to convert to a common thing later on
since that's all internal to kernel.
Jon / Sameer, you should check all the other drivers thoroughly to find
anyone who is doing the same thing as you need in order to achieve
something that is really common. I'm also wondering if it will be
possible to make dma_slave_config more flexible in order to start
accepting vendor specific properties in a somewhat common fashion, maybe
Vinod and Dan already have some thoughts on it? Apparently there is
already a need for the customization and people are just starting to
invent their own thing, but maybe that's fine too. That's really up to
subsys maintainer to decide in what direction to steer.
^ permalink raw reply
* [GIT PULL]: dmaengine fixes for v5.2-rc4
From: Vinod Koul @ 2019-06-08 9:09 UTC (permalink / raw)
To: Linus Torvalds; +Cc: dma, LKML
[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]
Hi Linus,
Please pull to receive dmaengine fixes. These fixes are on drivers.
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:
Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)
are available in the Git repository at:
git://git.infradead.org/users/vkoul/slave-dma.git tags/dmaengine-fix-5.2-rc4
for you to fetch changes up to 9bb9fe0cfbe0aa72fed906ade0590e1702815e5d:
dmaengine: sprd: Add interrupt support for 2-stage transfer (2019-05-21 19:23:54 +0530)
----------------------------------------------------------------
dmaengine fixes for v5.2-rc4
The fixes for this round are in drivers:
- jz4780 transfer fix for acking descriptors early
- fsl-qdma: clean registers on error
- dw-axi-dmac: null pointer dereference fix
- mediatek-cqdma: fix sleeping in atomic context
- tegra210-adma: fix bunch os issues like crashing in driver
probe, channel FIFO configuration etc.
- sprd: Fixes for possible crash on descriptor status, block
length overflow. For 2-stage transfer fix incorrect start,
configuration and interrupt handling.
----------------------------------------------------------------
Baolin Wang (3):
dmaengine: sprd: Fix the possible crash when getting descriptor status
dmaengine: sprd: Add validation of current descriptor in irq handler
dmaengine: sprd: Add interrupt support for 2-stage transfer
Colin Ian King (1):
dmaengine: dw-axi-dmac: fix null dereference when pointer first is null
Dan Carpenter (1):
dmaengine: mediatek-cqdma: sleeping in atomic context
Eric Long (3):
dmaengine: sprd: Fix the incorrect start for 2-stage destination channels
dmaengine: sprd: Fix block length overflow
dmaengine: sprd: Fix the right place to configure 2-stage transfer
Jon Hunter (3):
dmaengine: tegra210-adma: Fix crash during probe
dmaengine: tegra210-adma: Fix channel FIFO configuration
dmaengine: tegra210-adma: Fix spelling
Paul Cercueil (1):
dmaengine: jz4780: Fix transfers being ACKed too soon
Peng Ma (1):
dmaengine: fsl-qdma: Add improvement
drivers/dma/dma-jz4780.c | 32 ++++++++++-----
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 3 +-
drivers/dma/fsl-qdma.c | 4 +-
drivers/dma/mediatek/mtk-cqdma.c | 4 +-
drivers/dma/sprd-dma.c | 49 +++++++++++++++++-----
drivers/dma/tegra210-adma.c | 57 ++++++++++++++++----------
6 files changed, 100 insertions(+), 49 deletions(-)
Thanks
--
~Vinod
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH] dma: jz4780: Make probe function __init_or_module
From: Paul Cercueil @ 2019-06-08 10:23 UTC (permalink / raw)
To: Zubair Lutfullah Kakakhel, Vinod Koul; +Cc: dmaengine, linux-kernel, od
In-Reply-To: <20190607160758.16794-1-paul@crapouillou.net>
I misunderstood what __init_or_module was for. Please ignore this
patch. Sorry for the noise.
Le ven. 7 juin 2019 à 18:07, Paul Cercueil <paul@crapouillou.net> a
écrit :
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> drivers/dma/dma-jz4780.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 7204fdeff6c5..b2f7e6660ad6 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -815,7 +815,7 @@ static struct dma_chan
> *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> }
> }
>
> -static int jz4780_dma_probe(struct platform_device *pdev)
> +static int __init_or_module jz4780_dma_probe(struct platform_device
> *pdev)
> {
> struct device *dev = &pdev->dev;
> const struct jz4780_dma_soc_data *soc_data;
> @@ -966,7 +966,7 @@ static int jz4780_dma_probe(struct
> platform_device *pdev)
> return ret;
> }
>
> -static int jz4780_dma_remove(struct platform_device *pdev)
> +static int __exit jz4780_dma_remove(struct platform_device *pdev)
> {
> struct jz4780_dma_dev *jzdma = platform_get_drvdata(pdev);
> int i;
> --
> 2.21.0.593.g511ec345e18
>
^ permalink raw reply
* Re: [GIT PULL]: dmaengine fixes for v5.2-rc4
From: pr-tracker-bot @ 2019-06-08 20:10 UTC (permalink / raw)
To: Vinod Koul; +Cc: Linus Torvalds, dma, LKML
In-Reply-To: <20190608090908.GE9160@vkoul-mobl.Dlink>
The pull request you sent on Sat, 8 Jun 2019 14:39:08 +0530:
> git://git.infradead.org/users/vkoul/slave-dma.git tags/dmaengine-fix-5.2-rc4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/66b59f2b5e48969de862908c2d32c8b3d3724738
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v3 0/7] Allwinner H6 DMA support
From: Clément Péron @ 2019-06-09 20:42 UTC (permalink / raw)
To: Maxime Ripard
Cc: Vinod Koul, Rob Herring, Mark Rutland, Chen-Yu Tsai, Dan Williams,
dmaengine, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20190528111024.gj25jh5vstizze74@flea>
Hi Maxime,
On Tue, 28 May 2019 at 13:10, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, May 27, 2019 at 10:14:52PM +0200, Clément Péron wrote:
> > Hi,
> >
> > This series has been first proposed by Jernej Skrabec[1].
> > As this series is mandatory for SPDIF/I2S support and because he is
> > busy on Cedrus stuff. I asked him to make the minor change requested
> > and repost it.
> > Authorship remains to him.
> >
> > I have tested this series with SPDIF driver and added a patch to enable
> > DMA_SUN6I_CONFIG for arm64.
> >
> > Original Post:
> > "
> > DMA engine engine on H6 almost the same as on older SoCs. The biggest
> > difference is that it has slightly rearranged bits in registers and
> > it needs additional clock, probably due to iommu.
> >
> > These patches were tested with I2S connected to HDMI. I2S needs
> > additional patches which will be sent later.
>
> For the whole series,
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Thanks, Is it ok to pick patch 5/6/7 to sunxi tree ?
Regards,
Clément
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply
* Re: [PATCH] dmaengine: dmatest: Add support for completion polling
From: Vinod Koul @ 2019-06-10 7:04 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: dan.j.williams, dmaengine, andriy.shevchenko
In-Reply-To: <0e909b8a-8296-7c6a-058a-3fc780d66195@ti.com>
On 04-06-19, 16:35, Peter Ujfalusi wrote:
>
>
> On 04/06/2019 15.45, Vinod Koul wrote:
> > On 03-06-19, 10:05, Peter Ujfalusi wrote:
> >
> >>> I think the main question is how polling for completion should be
> >>> handled when client does not request for completion interrupt, thus we
> >>> will have no callback in the DMA driver when the transfer is completed.
> >>>
> >>> If DMA_PREP_INTERRUPT is set for the tx_descriptor then the polling will
> >>> wait until the DMA driver internally receives the interrupt that the
> >>> transfer is done and sets the cookie to completed state.
> >>>
> >>> However if DMA_PREP_INTERRUPT is not set, the DMA driver will not get
> >>> notification from the HW that is the transfer is done, the only way to
> >>> know is to check the tx_status and based on the residue (if it is 0 then
> >>> it is done) decide what to tell the client.
> >>>
> >>> Should the client call dmaengine_terminate_* after the polling returned
> >>> unconditionally to free up the descriptor?
> >>
> >> This is how omap-dma is handling the polled memcpy support.
> >
> > Yes that is a good question. Even if the client does not set
> > DMA_PREP_INTERRUPT would there be no interrupt generated by controller
> > on txn completion? If not how will next txn be submitted to the
> > hardware.
> >
> > I think we should view DMA_PREP_INTERRUPT from client pov, but
> > controller cannot get away with disabling interrupts IMO.
>
> What happens if client is issuing a DMA memcpy (short one) while
> interrupts are disabled?
>
> The user for this is:
> drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>
> commit: f5b9930b85dc6319fd6bcc259e447eff62fc691c
>
> The interrupt based completion is not going to work in some cases, the
> DMA driver should obey that the missing DMA_PREP_INTERRUPT really
> implies that interrupts can not be used.
well yes but how do we *assume* completion and issue subsequent txns?
Does driver create a task and poll?
>
> > Assuming I had enough caffeine before I thought process, then client would
> > poll descriptor status using cookie and should free up once the cookie
> > is freed, makes sense?
>
> OK, so clients are expected to call dmaengine_terminate_*
> unconditionally after the transfer is completed, right?
How do you know/detect transfer is completed?
>
> If we use interrupts then the handler would anyway free up the
> descriptor, so terminating should not do any harm, if we can not have
> interrupts then terminate will clear up the completed descriptor
> proactively.
yes terminate part is fine.
> In any case I have updated the EDMA patch to do the same thing in case
> of polling w/o interrupts as it would do in the completion irq handler,
> and similar approach prepared for omap-dma as well.
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
~Vinod
^ permalink raw reply
* Re: [PATCH v2 0/6] dmaengine: Add Synopsys eDMA IP driver (version 0)
From: Vinod Koul @ 2019-06-10 7:42 UTC (permalink / raw)
To: Gustavo Pimentel; +Cc: linux-pci, dmaengine
In-Reply-To: <cover.1559654565.git.gustavo.pimentel@synopsys.com>
On 04-06-19, 15:29, Gustavo Pimentel wrote:
> Add Synopsys eDMA IP driver (version 0 and for EP side only) to Linux
> kernel. This IP is generally distributed with Synopsys PCIe EndPoint IP
> (depends of the use and licensing agreement), which supports:
> - legacy and unroll modes
> - 16 independent and concurrent channels (8 write + 8 read)
> - supports linked list (scatter-gather) transfer
> - each linked list descriptor can transfer from 1 byte to 4 Gbytes
> - supports cyclic transfer
> - PCIe EndPoint glue-logic
>
> This patch series contains:
> - eDMA core + eDMA core v0 driver (implements the interface with
> DMAengine controller APIs and interfaces with eDMA HW block)
> - eDMA PCIe glue-logic reference driver (attaches to Synopsys EP and
> provides memory access to eDMA core driver)
Applied all, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dmaengine: axi-dmac: update license header
From: Vinod Koul @ 2019-06-10 7:49 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: dmaengine
In-Reply-To: <20190606105344.3405-1-alexandru.ardelean@analog.com>
On 06-06-19, 13:53, Alexandru Ardelean wrote:
> 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.
Applied, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH trivial] dmaengine: Grammar s/the its/its/, s/need/needs/
From: Vinod Koul @ 2019-06-10 7:51 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Dan Williams, Jiri Kosina, dmaengine, linux-kernel
In-Reply-To: <20190607113039.14560-1-geert+renesas@glider.be>
On 07-06-19, 13:30, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH/RFC] dmaengine: Create symlinks from DMA channels to slaves
From: Vinod Koul @ 2019-06-10 7:54 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dan Williams, dmaengine, linux-renesas-soc, linux-kernel
In-Reply-To: <20190607113835.15376-1-geert+renesas@glider.be>
On 07-06-19, 13:38, Geert Uytterhoeven wrote:
> Currently it is not easy to find out which DMA channels are in use, and
> by which slave devices.
>
> Fix this by creating in sysfs a "slave" symlink from the DMA channel to
> the actual slave device when a channel is requested, and removing it
> again when the channel is released.
>
> For now this is limited to DT and ACPI.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Questions:
> 1. Do you think this is useful?
Yes for me at least :)
> 2. Should backlinks (e.g. "dma:<name>") be created from the slave
> device to the DMA channel?
> This requires storing the name in struct dma_chan, for later
> symlink removal.
that would certainly be more helpful
> 3. Should this be extended to other ways of requesting channels?
> In many cases, no device pointer is available, so a device pointer
> parameter has to be added to all DMA channel request APIs that
> don't have it yet.
I think that would need to be done.
> ---
> drivers/dma/dmaengine.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 03ac4b96117cd8db..c11476f76fc96bcf 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -706,6 +706,10 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
>
> if (chan) {
> /* Valid channel found or requester needs to be deferred */
> + if (!IS_ERR(chan) &&
> + sysfs_create_link(&chan->dev->device.kobj, &dev->kobj,
> + "slave"))
> + dev_err(dev, "Cannot create DMA slave symlink\n");
> if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
> return chan;
> }
> @@ -786,6 +790,7 @@ void dma_release_channel(struct dma_chan *chan)
> /* drop PRIVATE cap enabled by __dma_request_channel() */
> if (--chan->device->privatecnt == 0)
> dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask);
> + sysfs_remove_link(&chan->dev->device.kobj, "slave");
> mutex_unlock(&dma_list_mutex);
> }
> EXPORT_SYMBOL_GPL(dma_release_channel);
> --
> 2.17.1
--
~Vinod
^ permalink raw reply
* Re: [PATCH] [RFC] dmaengine: add fifo_size member
From: Jon Hunter @ 2019-06-10 7:59 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: <56aa6f45-cfd8-7f1e-9392-628ceb58093f@ti.com>
On 07/06/2019 14:35, Peter Ujfalusi wrote:
>
>
> On 07/06/2019 15.58, Jon Hunter wrote:
>>> Imho if you can explain it without using 'HACK' in the sentences it
>>> might be OK, but it does not feel right.
>>
>> I don't perceive this as a hack. Although from looking at the
>> description of the src/dst_maxburst these are burst size with regard to
>> the device, so maybe it is a stretch.
>>
>>> However since your ADMA and ADMIF is highly coupled and it does needs
>>> special maxburst information (burst and allocated FIFO depth) I would
>>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>>
>>> ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff)
>>> ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff)
>>>
>>> So lower 1 byte is the burst value you want from ADMA
>>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>>
>>> Sure, you need a header for this to make sure there is no
>>> misunderstanding between the two sides.
>>
>> I don't like this because as I mentioned to Dmitry, the ADMA can perform
>> memory-to-memory transfers where such encoding would not be applicable.
>
> mem2mem does not really use dma_slave_config, it is for used by
> is_slave_direction() == true type of transfers.
>
> But true, if you use ADMA against anything other than ADMAIF then this
> might be not right for non cyclic transfers.
>
>> That does not align with the description in the
>> include/linux/dmaengine.h either.
>
> True.
>
>>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>>> will pick a 'good/safe' burst value for it.
>>>
>>> Or new member, but do you need two of them for src/dst? Probably
>>> fifo_depth is better word for it, or allocated_fifo_depth.
>>
>> Right, so looking at the struct dma_slave_config we have ...
>>
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> u32 src_port_window_size;
>> u32 dst_port_window_size;
>>
>> Now if we could make these window sizes a union like the following this
>> could work ...
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 8fcdee1c0cf9..851251263527 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -360,8 +360,14 @@ struct dma_slave_config {
>> enum dma_slave_buswidth dst_addr_width;
>> u32 src_maxburst;
>> u32 dst_maxburst;
>> - u32 src_port_window_size;
>> - u32 dst_port_window_size;
>> + union {
>> + u32 port_window_size;
>> + u32 port_fifo_size;
>> + } src;
>> + union {
>> + u32 port_window_size;
>> + u32 port_fifo_size;
>> + } dst;
>
> What if in the future someone will have a setup where they would need both?
I think if you look at the description for the port_window_size you will
see that this is not applicable for FIFOs and so these would be mutually
exclusive AFAICT. However, if there was an even weirder DMA out there in
the future it could always be patched :-)
> So not sure. Your problems are coming from a split DMA setup where the
> two are highly coupled, but sits in a different place and need to be
> configured as one device.
>
> I think xilinx_dma is facing with similar issues and they have a custom
> API to set parameters which does not fit or is peripheral specific:
> include/linux/dma/xilinx_dma.h
>
> Not sure if that is an acceptable solution.
I am not a fan of that but it could work.
Cheers
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