@@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) INIT_LIST_HEAD(&dma->channels); dma_cap_set(DMA_MEMCPY, dma->cap_mask); dma_cap_set(DMA_SLAVE, dma->cap_mask);+ dma_cap_set(DMA_PRIVATE, dma->cap_mask); for (i = 0; i < dma->chancnt; i++) { mchan = &mdma->channels[i];
What are the implications of this? Is a comment due?
I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
uses dma_get_slave_channel() instead of dma_request_channel()
(PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
but is not set in dma_get_slave_channel().
There are only two places in the mainline kernel, where
dma_get_slave_channel() is used. I've picked up the idea
at one of these places. Please look at this patch:
http://www.spinics.net/lists/arm-kernel/msg268718.html
I haven't found documentation about the DMA_PRIVATE flag, only
saw commit 59b5ec21446b9 "dmaengine: introduce
dma_request_channel and private channels".
Unfortunately I didn't find any description of DMA_PRIVATE flag too.
But the comment at the beginning of drivers/dma/dmaengine.c
may give a clue. Quotation:
* subsystem can get access to a channel by calling dmaengine_get() followed
* by dma_find_channel(), or if it has need for an exclusive channel
it can call
* dma_request_channel(). Once a channel is allocated a reference is taken
* against its corresponding driver to disable removal.
DMA_PRIVATE capability flag might indicate that the DMA controller
can provide exclusive channels to its clients. Please correct me if I'm wrong.
Alex, unless I'm
missing something this one-line change is quite a change in
semantics, and has dramatic influence on the code's behaviour
(ignores the DMA controller when looking for channels that can do
mem-to-mem transfers)
Excuse me, Gerhard, I don't see what you mean.
Could you point to the corresponding code?
Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.
Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.
Thanks!
Best regards,
Alexander
From: Gerhard Sittig <hidden> Date: 2014-01-08 16:47:27
[ dropping devicetree from the Cc: list ]
[ what is the semantics of DMA_PRIVATE capability flag?
is documentation available beyond the initial commit message?
need individual channels be handled instead of controllers? ]
On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
Hello Gerhard.
Thanks for your review.
2013/12/26 Gerhard Sittig [off-list ref]:
quoted
[ dropping devicetree, we're DMA specific here ]
On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
@@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) INIT_LIST_HEAD(&dma->channels); dma_cap_set(DMA_MEMCPY, dma->cap_mask); dma_cap_set(DMA_SLAVE, dma->cap_mask);+ dma_cap_set(DMA_PRIVATE, dma->cap_mask); for (i = 0; i < dma->chancnt; i++) { mchan = &mdma->channels[i];
What are the implications of this? Is a comment due?
I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
uses dma_get_slave_channel() instead of dma_request_channel()
(PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
but is not set in dma_get_slave_channel().
There are only two places in the mainline kernel, where
dma_get_slave_channel() is used. I've picked up the idea
at one of these places. Please look at this patch:
http://www.spinics.net/lists/arm-kernel/msg268718.html
I agree that the change looks simple, and there is no doubt that
other drivers apply the flag. None of this I questioned. Yet
I'm afraid that the implications are rather huge.
Unless I miss something, I'd happily learn where I'm wrong.
quoted
I haven't found documentation about the DMA_PRIVATE flag, only
saw commit 59b5ec21446b9 "dmaengine: introduce
dma_request_channel and private channels".
Unfortunately I didn't find any description of DMA_PRIVATE flag too.
But the comment at the beginning of drivers/dma/dmaengine.c
may give a clue. Quotation:
* subsystem can get access to a channel by calling dmaengine_get() followed
* by dma_find_channel(), or if it has need for an exclusive channel
it can call
* dma_request_channel(). Once a channel is allocated a reference is taken
* against its corresponding driver to disable removal.
DMA_PRIVATE capability flag might indicate that the DMA controller
can provide exclusive channels to its clients. Please correct me if I'm wrong.
quoted
Alex, unless I'm
missing something this one-line change is quite a change in
semantics, and has dramatic influence on the code's behaviour
(ignores the DMA controller when looking for channels that can do
mem-to-mem transfers)
Excuse me, Gerhard, I don't see what you mean.
Could you point to the corresponding code?
You did see `git show 59b5ec21446b9`, didn't you? The commit
message strongly suggests that DMA_PRIVATE applies to the whole
DMA controller and excludes _all_ of its channels from the
general purpose allocator which mem-to-mem transfers appear to be
using. It's not just a hint, but an active decision to reject
requests.
Not only checking code references, but doing a text search,
reveals one more comment on the DMA_PRIVATE flag in a crypto
related document, which supports my interpretation:
Documentation/crypto/async-tx-api.txt:203
Can somebody ACK or NAK my interpretation? Dan, you committed
this change which introduced the DMA_PRIVATE logic. What was the
motivation for it, or the goal to achieve? Do other platforms
have several dedicated DMA controllers, some for peripherals and
some for memory transfers? Should the "private" flag apply to
channels and not whole controllers? Am I over-estimating the
benefit or importance of DMA supported memory transfers?
Still I see a difference in the lookup approaches: Yours applies
DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
for memory transfers. While the __dma_request_channel() routine
only applies it _temporarily_ around a dma_chan_get() operation.
Allowing for use of DMA channels by both individual peripherals
as well as memory transfers.
quoted
Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.
Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.
What I meant here is that implications for all affected platforms
should be considered. There is one driver source, but the driver
applies to more than one platform (another issue of the driver is
that this is not apparent from the doc nor the compat strings).
MPC512x has one (GP) DMA controller, of which one channel is
dedicated to DDR, and all other channels can get used for memory
transfers as well. In addition to most channels being connected
to a specific peripheral for flow control. Which your patch set
introduces initial support for.
MPC8308 has _all_ channels for memory transfers exclusively (or
at least none of its channels supports flow control).
So blocking memory transfers in mpc512x_dma.c is a total breakage
for MPC8308 (removes the only previous feature and adds nothing),
and is a regression for MPC512x (removes the previously supported
memory transfers, while it may add peripheral supports with very
few users).
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
[ dropping devicetree from the Cc: list ]
[ what is the semantics of DMA_PRIVATE capability flag?
is documentation available beyond the initial commit message?
need individual channels be handled instead of controllers? ]
The DMA_PRIVATE means that your channels are not to be used for global memcpy,
as one can do in async cases (this is hwere DMAengine came into existence)
If the device has the capablity of doing genric memcpy then it should not set
this. For slave dma usage the dam channel can transfer data to a specfic
slave device(s), hence we should use this is geric fashion so setting
DMA_PRIVATE makes sense in those cases.
On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
quoted
Hello Gerhard.
Thanks for your review.
2013/12/26 Gerhard Sittig [off-list ref]:
quoted
[ dropping devicetree, we're DMA specific here ]
On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
@@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) INIT_LIST_HEAD(&dma->channels); dma_cap_set(DMA_MEMCPY, dma->cap_mask); dma_cap_set(DMA_SLAVE, dma->cap_mask);+ dma_cap_set(DMA_PRIVATE, dma->cap_mask); for (i = 0; i < dma->chancnt; i++) { mchan = &mdma->channels[i];
What are the implications of this? Is a comment due?
I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
uses dma_get_slave_channel() instead of dma_request_channel()
(PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
but is not set in dma_get_slave_channel().
Which makes me thing you are targetting slave usages. Do you intend to use for
mempcy too on all controllers you support. in that case you should set it
selectively.
quoted
There are only two places in the mainline kernel, where
dma_get_slave_channel() is used. I've picked up the idea
at one of these places. Please look at this patch:
http://www.spinics.net/lists/arm-kernel/msg268718.html
I agree that the change looks simple, and there is no doubt that
other drivers apply the flag. None of this I questioned. Yet
I'm afraid that the implications are rather huge.
Unless I miss something, I'd happily learn where I'm wrong.
quoted
quoted
I haven't found documentation about the DMA_PRIVATE flag, only
saw commit 59b5ec21446b9 "dmaengine: introduce
dma_request_channel and private channels".
Unfortunately I didn't find any description of DMA_PRIVATE flag too.
But the comment at the beginning of drivers/dma/dmaengine.c
may give a clue. Quotation:
* subsystem can get access to a channel by calling dmaengine_get() followed
* by dma_find_channel(), or if it has need for an exclusive channel
it can call
* dma_request_channel(). Once a channel is allocated a reference is taken
* against its corresponding driver to disable removal.
DMA_PRIVATE capability flag might indicate that the DMA controller
can provide exclusive channels to its clients. Please correct me if I'm wrong.
quoted
Alex, unless I'm
missing something this one-line change is quite a change in
semantics, and has dramatic influence on the code's behaviour
(ignores the DMA controller when looking for channels that can do
mem-to-mem transfers)
Excuse me, Gerhard, I don't see what you mean.
Could you point to the corresponding code?
You did see `git show 59b5ec21446b9`, didn't you? The commit
message strongly suggests that DMA_PRIVATE applies to the whole
DMA controller and excludes _all_ of its channels from the
general purpose allocator which mem-to-mem transfers appear to be
using. It's not just a hint, but an active decision to reject
requests.
Not only checking code references, but doing a text search,
reveals one more comment on the DMA_PRIVATE flag in a crypto
related document, which supports my interpretation:
Documentation/crypto/async-tx-api.txt:203
Can somebody ACK or NAK my interpretation? Dan, you committed
this change which introduced the DMA_PRIVATE logic. What was the
motivation for it, or the goal to achieve? Do other platforms
have several dedicated DMA controllers, some for peripherals and
some for memory transfers? Should the "private" flag apply to
channels and not whole controllers? Am I over-estimating the
benefit or importance of DMA supported memory transfers?
The DMA_PRIVATE flag is more on how the channel is allocated and will it be used
by generic allocator or not. You cna still use mecpy ops for a controller with
DMA_PRIVATE flag if the controller supports.
Still I see a difference in the lookup approaches: Yours applies
DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
for memory transfers. While the __dma_request_channel() routine
only applies it _temporarily_ around a dma_chan_get() operation.
Allowing for use of DMA channels by both individual peripherals
as well as memory transfers.
No it doesnt prevent. You can still use it for memcpy once you have the channel.
--
~Vinod
quoted
quoted
Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.
Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.
What I meant here is that implications for all affected platforms
should be considered. There is one driver source, but the driver
applies to more than one platform (another issue of the driver is
that this is not apparent from the doc nor the compat strings).
MPC512x has one (GP) DMA controller, of which one channel is
dedicated to DDR, and all other channels can get used for memory
transfers as well. In addition to most channels being connected
to a specific peripheral for flow control. Which your patch set
introduces initial support for.
MPC8308 has _all_ channels for memory transfers exclusively (or
at least none of its channels supports flow control).
So blocking memory transfers in mpc512x_dma.c is a total breakage
for MPC8308 (removes the only previous feature and adds nothing),
and is a regression for MPC512x (removes the previously supported
memory transfers, while it may add peripheral supports with very
few users).
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
From: Alexander Popov <hidden> Date: 2014-01-13 08:17:23
Thanks for your replies, Gerhard and Vinod.
2014/1/9 Vinod Koul [off-list ref]:
On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
quoted
[ what is the semantics of DMA_PRIVATE capability flag?
is documentation available beyond the initial commit message?
need individual channels be handled instead of controllers? ]
The DMA_PRIVATE means that your channels are not to be used for global memcpy,
as one can do in async cases (this is hwere DMAengine came into existence)
If the device has the capablity of doing genric memcpy then it should not set
this. For slave dma usage the dam channel can transfer data to a specfic
slave device(s), hence we should use this is geric fashion so setting
DMA_PRIVATE makes sense in those cases.
Each DMA channel of MPC512x DMA controller can do _both_
mem-to-mem transfers and transfers between mem and some slave peripheral
(only one DMA channel is fully dedicated to DDR).
All DMA channels of MPC512x DMA controller belong to one dma_device.
So we _don't_ need setting DMA_PRIVATE flag for this dma_device at all, do we?
quoted
On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
quoted
I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
uses dma_get_slave_channel() instead of dma_request_channel()
(PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
but is not set in dma_get_slave_channel().
Which makes me thing you are targetting slave usages. Do you intend to use for
mempcy too on all controllers you support. in that case you should set it
selectively.
Vinod, please correct me if I'm wrong.
As I could understand from your comments and the code,
DMA_PRIVATE flag is needed for dma_devices with DMA channels
which _can_ work with slave peripheral but _can't_ do mem-to-mem transfers.
If DMA_PRIVATE flag is set for some dma_device before
dma_async_device_register()
then its DMA channels are not published in tables for kernel slab allocator
(because these channels are simply useless for memcpy).
quoted
Still I see a difference in the lookup approaches: Yours applies
DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
for memory transfers. While the __dma_request_channel() routine
only applies it _temporarily_ around a dma_chan_get() operation.
Allowing for use of DMA channels by both individual peripherals
as well as memory transfers.
No it doesnt prevent. You can still use it for memcpy once you have the channel.
Excuse me, I don't completely understand why dma_request_channel()
needs to set DMA_PRIVATE flag.
If dma_request_channel() for some dma_device without DMA_PRIVATE
is called before the first dmaengine_get()
then no DMA channels of this dma_device will become available for memcpy
by slab allocator.
Could you give me a clue?
quoted
quoted
quoted
Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.
Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.
What I meant here is that implications for all affected platforms
should be considered. There is one driver source, but the driver
applies to more than one platform (another issue of the driver is
that this is not apparent from the doc nor the compat strings).
I'll add a comment with information about the supported platforms to
mpc512x_dma.c
in RFC PATCH 1/5. Ok?
quoted
So blocking memory transfers in mpc512x_dma.c is a total breakage
for MPC8308 (removes the only previous feature and adds nothing),
and is a regression for MPC512x (removes the previously supported
memory transfers, while it may add peripheral supports with very
few users).
Yes, I see. MPC512x and MPC8308 should be treated differently.
Thanks!
Alexander
From: Gerhard Sittig <hidden> Date: 2014-01-16 14:26:10
On Mon, Jan 13, 2014 at 12:17 +0400, Alexander Popov wrote:
Thanks for your replies, Gerhard and Vinod.
2014/1/9 Vinod Koul [off-list ref]:
quoted
On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
quoted
[ what is the semantics of DMA_PRIVATE capability flag?
is documentation available beyond the initial commit message?
need individual channels be handled instead of controllers? ]
The DMA_PRIVATE means that your channels are not to be used for global memcpy,
as one can do in async cases (this is hwere DMAengine came into existence)
If the device has the capablity of doing genric memcpy then it should not set
this. For slave dma usage the dam channel can transfer data to a specfic
slave device(s), hence we should use this is geric fashion so setting
DMA_PRIVATE makes sense in those cases.
Each DMA channel of MPC512x DMA controller can do _both_
mem-to-mem transfers and transfers between mem and some slave peripheral
(only one DMA channel is fully dedicated to DDR).
All DMA channels of MPC512x DMA controller belong to one dma_device.
So we _don't_ need setting DMA_PRIVATE flag for this dma_device at all, do we?
I'd phrase it a little stronger. It's not that we don't _need_
the DMA_PRIVATE flag, it's actually that we _must_not_ use it
(unless I'm being dense, and keep missing something). With the
DMA_PRIVATE flag set, the generic allocator will refuse to use
any channel of the only DMA controller, which totally eliminates
general use, and only leaves us with explicitly configured uses
(that would be MMC only in mainline, and nothing else).
quoted
quoted
Still I see a difference in the lookup approaches: Yours applies
DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
for memory transfers. While the __dma_request_channel() routine
only applies it _temporarily_ around a dma_chan_get() operation.
Allowing for use of DMA channels by both individual peripherals
as well as memory transfers.
No it doesnt prevent. You can still use it for memcpy once you have the channel.
Vinod, what am I missing here? Before probe() there is no DMA
controller. After probe() the DMA_PRIVATE flag is set and thus
general allocation won't happen. How exactly does one get to
"have the channel" for memory transfers? Aren't the channel
references acquired upon demand, as the need arises? While the
DMA controller has no means to know whether "all memory transfer
channel aquisition was done" or whether "all slave peripherals
have their channel" (if at all such a situation exists, given we
have dynamically loadable modules), such that the DMA_PRIVATE
toggle could get thrown one way or another?
This brings me back to a question I raised earlier: Am I
overestimating the benefit or importance of DMA supported memory
transfers? Am I wrong assuming that there are users of this
feature which need not get configured explicitly (i.e. they
operate in transparent ways, using whatever they find to be
available), and that the set of these users and their consumption
of DMA resources is something that is dynamic (i.e. driven by
demand, instead of pre-allocated and then probably inappropriate
for the workload they see)?
Excuse me, I don't completely understand why dma_request_channel()
needs to set DMA_PRIVATE flag.
If dma_request_channel() for some dma_device without DMA_PRIVATE
is called before the first dmaengine_get()
then no DMA channels of this dma_device will become available for memcpy
by slab allocator.
Could you give me a clue?
quoted
quoted
quoted
quoted
Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.
Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.
What I meant here is that implications for all affected platforms
should be considered. There is one driver source, but the driver
applies to more than one platform (another issue of the driver is
that this is not apparent from the doc nor the compat strings).
I'll add a comment with information about the supported platforms to
mpc512x_dma.c
in RFC PATCH 1/5. Ok?
quoted
quoted
So blocking memory transfers in mpc512x_dma.c is a total breakage
for MPC8308 (removes the only previous feature and adds nothing),
and is a regression for MPC512x (removes the previously supported
memory transfers, while it may add peripheral supports with very
few users).
Yes, I see. MPC512x and MPC8308 should be treated differently.
Alexander, are you suggesting to treat 512x and 8308 differently,
and did you decide how to do that? Previous review feedback
raised the question whether this is needed or appropriate, while
there has not been an answer yet AFAICT. I would not jump to
conclusions here, especially when you cannot test what you
change.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de