Re: [PATCH] of: Add generic device tree DMA helpers
On 03/19/2012 03:45 PM, Grant Likely :
> On Mon, 19 Mar 2012 14:30:05 +0100, Nicolas Ferre
> wrote:
>> On 03/17/2012 10:40 AM, Grant Likely :
>>> On Thu, 15 Mar 2012 09:38:10 +0100, Nicolas Ferre
>>> wrote:
+struct of_dma {
+ struct list_head of_dma_controllers;
+ struct device_node *of_node;
+ int of_dma_n_cells;
+ int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
+};
>>>
>>> This _xlate is nearly useless as a generic API. It solves the problem for
>>> the specific case where the driver is hard-coded to know which DMA engine
>>> to talk to, but since the returned data doesn't provide any context, it
>>> isn't useful if there are multiple DMA controllers to choose from.
>>
>> You mean, if there is no DMA controller phandle specified in the
>> property?
>
> No; I'm assuming that dma-channel properties will alwasy have a
> phandle to the dma controller node.
>
>> I think that it is not the purpose of this API to choose a DMA
>> controller, Nor to provide a channel. The only purpose of this API is to
>> give a HW request to be used by a DMA slave driver. This slave should
>> already have a channel to use and a controller to talk to.
>
> Then where is the function that finds the reference to the DMA
> controller? I don't understand why it would be useful to decode that
> separately.
>
>>> The void *data pointer must be replaced with a typed structure so that
>>> context can be returned.
>>
>> I am not sure to follow you entirely... How do I address the fact that
>> several types of request value can be returned then?
>>
>> BTW, can we imagine a phandle property with a sting as a argument?
>> should it be written this way?
>> dma-request = <&testdmac1>, "slave-rx", "slave-tx";
>
> No, I'm not suggesting that. Mixing phandles and strings in a single
> property is possible but ugly. The phandle-args pattern which uses
> zero or more cells as arguments should be used.
>
>>
>> If yes, the of_parse_phandle_with_args() is not working on this type...
>
> Right; of_parse_phandle_with_args() should do the job.
>
>>
>> (I realize that there seems to be no way out for a generic API: maybe we
>> should move to one or two cases to address and concentrate on them).
>
> The way I read this patch, the xlate function returns a single
> integer representing the DMA request number, but it doesn't provide
> any data about *which* dma controller is associated with that channel.
> The result of xlate needs to be something like a dma_chan reference
> that identifies both the DMA engine and the channel/request on that
> dma engine.
>
> [...]
+/**
+ * of_dma_xlate_onenumbercell() - Generic DMA xlate for direct one cell
bindings
+ *
+ * Device Tree DMA translation function which works with one cell bindings
+ * where the cell values map directly to the hardware request number
understood
+ * by the DMA controller.
+ */
+int of_dma_xlate_onenumbercell(struct of_phandle_args *dma_spec, void
*dma_req)
+{
+ if (!dma_spec)
+ return -EINVAL;
+ if (WARN_ON(dma_spec->args_count != 1))
+ return -EINVAL;
+ *(int *)dma_req = dma_spec->args[0];
>>>
>>> Following on from comment above; the void *dma_req parameter is dangerous.
>>> How
>>> does this function know that it has been passed an int* pointer?
>>
>> Well, that is a drawback that comes from having to address generic
>> cases.
>
> Not if you do it right. If a specific data structure is returned,
> then there can be context attached as to what the data means and which
> dma controller knows how to parse it.
>
>> But anyway, if the DMA controller decide to register a .xlate()
>> function that returns an integer, the slave driver that will ask a
>> "request line" to this DMA controller will be aware that an integer has
>> to be passed to of_get_dma_request().
>
> The problem still remains; I don't see how the dma slave can figure
> out *which* dma controller it needs to talk to.
Is not it what the phandle to the dma controller is made for?
Bye,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, Mar 15, 2012 at 10:39:12PM +0100, Cousson, Benoit wrote: > On 3/15/2012 9:41 PM, Arnd Bergmann wrote: > >The numbers definitely need to become local to each of the controllers, but > >that is the case pretty much automatically using the proposed binding, > >because each dma request identifier starts with the phandle of the > >controller. > > Indeed, and in the case of the OMAP SDMA controller, it can handle > up to 127 DMA request lines numbered from 0 to 126... So a local > number seems to be a good representation... especially for a number. > I'm not sure to understand the issue with this binding. > > And AFAIK, there is the only one general purpose DMA controller in > OMAP so far. The other ones are private to some IPs like MMC or USB, > so they do not need necessarily need any DT representation. AM335x has an EDMA controller instead of SDMA. This is the same EDMA found on mach-davinci/ parts. -Matt -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Mon, Mar 19, 2012 at 9:41 PM, Arnd Bergmann wrote: > >> Secondly, there are platforms (Samsung) where peripherals are connected >> to more than one DMA controller, and either DMA controller can be used - >> I'm told by Jassi that there's reasons why you'd want to select one or >> other as the target at runtime. > > How common is that? If there are only a few drivers that have this > requirement, we could have this represented in the driver binding, by > listing multiple channels and giving the device the choice. > All Samsung SoCs (last ~4yrs) with PL330 have some peripherals that could be served by more than one PL330 instance. The requirement is not from any driver and it is nothing Samsung specific. Any platform with 1:n client:dmac might want to do stuff like serving a new channel request from an already active DMAC, rather than invoking otherwise fully idle one. Similarly for Mem->Mem, the PL330 distributes its cycles between active channels, so a platform might want channel allocation uniformly across PL330 instances. -jassi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
Hi Nico,
On 3/19/2012 5:31 PM, Nicolas Ferre wrote:
On 03/19/2012 04:33 PM, Russell King - ARM Linux :
On Mon, Mar 19, 2012 at 02:06:34PM +, Arnd Bergmann wrote:
mmci
/* in drivers/dma/ste_dma40.c, others in pl330.c, coh901318.c, ... */
bool stedma40_filter(struct dma_chan *chan, void *data)
{
struct stedma40_chan_cfg *info = data;
struct d40_chan *d40c = container_of(chan, struct d40_chan, chan);
int err;
err = d40_validate_conf(d40c, info);
if (!err)
d40c->dma_cfg = *info;
d40c->configured = true;
return err == 0;
}
EXPORT_SYMBOL(stedma40_filter);
/* in mmci.h */
struct mmci_platform_data {
...
bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
void *dma_rx_param;
void *dma_tx_param;
};
/* in mmci.c */
static void __devinit mmci_dma_setup(struct mmci_host *host)
{
...
host->dma_rx_channel = dma_request_channel(mask, plat->dma_filter,
plat->dma_rx_param);
...
}
Whatever we come up with obviously needs to work with both drivers.
I think we will end up with something closer to the second one, except
that the dma parameters do not come from platform_data but from the
#dma-request property, which also identifies the controller and channel.
Firstly, define what you mean by "the DMA parameters". Things like burst
size, register width, register address? That should all be known by the
peripheral driver and _not_ be encoded into DT in any way - and this
information should be passed to the DMA engine driver for the well defined
API for that purpose.
Secondly, there are platforms (Samsung) where peripherals are connected
to more than one DMA controller, and either DMA controller can be used -
I'm told by Jassi that there's reasons why you'd want to select one or
other as the target at runtime.
Whether it's appropriate for DT to know that or not, I'm not certain at
the moment - I suspect the _right_ solution would be a radical DMA engine
redesign which moves _all_ slave DMA to a virtual channel representation,
and for the virtual channels to be scheduled onto a set of physical DMA
channels over a range of DMA devices. Obviously, there's restrictions on
which virtual channels could be scheduled onto which physical channels of
which DMA devices.
OMG! the dmaengine drivers are already not so obvious to understand. I
fear that trying to mimic some special behaviors within relatively
simple dmaengine drivers will bring confusion and prevent people to
read/contribute and fix those simple ones...
It seems to me, therefore, that any attempt to come up with some kind of
DT based representation of the current scheme is doomed to fail, and will
at some point become obsolete.
I think instead, rather than trying to fix this now, we persist with what
we have, but organize an effort to clean up and restructure the DMA engine
code so that:
(a) things like the Samsung, and ARM board oddities can be easily dealt
with in a driver independent manner.
(b) get rid of all the duplicated functionality between each different
DMA engine implementation, separating this out into core code, and
simplifying the drivers.
The _big_ problem I see is that if we try to represent the existing
solution in DT, we're going to get locked into that way of doing things
and then any major restructuring of the DMA engine stuff will become an
impossibility - especially if we start specifying things by DMA request
signal numbers on DMA engines.
Even if I understand your point, I wonder what is the solution for us
that have a pretty simple representation of *hardware* to code into DT:
should we wait for a big rework? Should we go for a private DMA binding
for each of us that have just the need for a quite simple representation?
I must say that I am puzzled by recent discussion on the topic (mix of
"channel" and "request" notions, plan for a complete rework of dmaengine
that can delay DT representation of DMA slave-controller relationship,
even my own doubts on the "void *" output parameter, etc.). It seems
that I am not familiar with all those cases and that I cannot go further
with a generic DT representation...
I do agree that it appears that some DMA controllers are way more
complex that the one we were trying to support.
On OMAP, the DMA channels and the DMA requests are completely
orthogonal, and thus I do not have to provide any information about the
channel in the DT binding since the SW can affect any request to any
channel.
Reading the various points from Russell and Arnd, it seems that this is
not always the case :-(
I guess this is probably the main cause of confusion in this discussion.
So I do not know either how we can progress further on that without a
clear understanding of what all these DMA controllers are capable of.
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/19/2012 09:45 AM, Arnd Bergmann wrote:
> On Monday 19 March 2012, Stephen Warren wrote:
>>> Maybe one can use named properties in a new device node in that case,
>>> like this:
>>>
>>> bus {
>>> dma: dma-controller {
>>> #dma-cells = <1>;
>>> };
>>>
>>> device {
>>> compatible = "device";
>>> channel: dma-channel {
>>> type = <0x1>;
>>> name = "foo";
>>> number = <23>;
>>> direction = <3>;
>>> };
>>> dma-requests = <&dma &channel>;
>>> };
>>> };
>>
>> For reference, this is very similar to how the pinctrl bindings work,
>> except that they require the "channel" node to be a child of the DMA
>> controller, and hence "dma-requests" doesn't contain <&dma &channel>,
>> just <&channel>, since "dma" is the parent (or grand-parent) of "channel".
>
> Right, but the difference beytween the pinctrl binding and what I
> describe here is that the channel description would be part of the
> dma engine driver specific binding, not the generic binding that
> is visible to device drivers.
That's actually true for pinctrl as well: Common pinctrl bindings cover
the content/format of "dma-requests" and a requirement for a
"dma-channel" node, whereas the per-pin-controller bindings define the
content of node "dma-channel".
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/19/2012 04:33 PM, Russell King - ARM Linux :
> On Mon, Mar 19, 2012 at 02:06:34PM +, Arnd Bergmann wrote:
>> mmci
>> /* in drivers/dma/ste_dma40.c, others in pl330.c, coh901318.c, ... */
>> bool stedma40_filter(struct dma_chan *chan, void *data)
>> {
>> struct stedma40_chan_cfg *info = data;
>> struct d40_chan *d40c = container_of(chan, struct d40_chan, chan);
>> int err;
>>
>> err = d40_validate_conf(d40c, info);
>> if (!err)
>> d40c->dma_cfg = *info;
>> d40c->configured = true;
>>
>> return err == 0;
>> }
>> EXPORT_SYMBOL(stedma40_filter);
>>
>> /* in mmci.h */
>> struct mmci_platform_data {
>> ...
>> bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
>> void *dma_rx_param;
>> void *dma_tx_param;
>> };
>>
>> /* in mmci.c */
>> static void __devinit mmci_dma_setup(struct mmci_host *host)
>> {
>> ...
>> host->dma_rx_channel = dma_request_channel(mask, plat->dma_filter,
>> plat->dma_rx_param);
>> ...
>> }
>>
>>
>>
>> Whatever we come up with obviously needs to work with both drivers.
>> I think we will end up with something closer to the second one, except
>> that the dma parameters do not come from platform_data but from the
>> #dma-request property, which also identifies the controller and channel.
>
> Firstly, define what you mean by "the DMA parameters". Things like burst
> size, register width, register address? That should all be known by the
> peripheral driver and _not_ be encoded into DT in any way - and this
> information should be passed to the DMA engine driver for the well defined
> API for that purpose.
>
> Secondly, there are platforms (Samsung) where peripherals are connected
> to more than one DMA controller, and either DMA controller can be used -
> I'm told by Jassi that there's reasons why you'd want to select one or
> other as the target at runtime.
>
> Whether it's appropriate for DT to know that or not, I'm not certain at
> the moment - I suspect the _right_ solution would be a radical DMA engine
> redesign which moves _all_ slave DMA to a virtual channel representation,
> and for the virtual channels to be scheduled onto a set of physical DMA
> channels over a range of DMA devices. Obviously, there's restrictions on
> which virtual channels could be scheduled onto which physical channels of
> which DMA devices.
OMG! the dmaengine drivers are already not so obvious to understand. I
fear that trying to mimic some special behaviors within relatively
simple dmaengine drivers will bring confusion and prevent people to
read/contribute and fix those simple ones...
> It seems to me, therefore, that any attempt to come up with some kind of
> DT based representation of the current scheme is doomed to fail, and will
> at some point become obsolete.
>
> I think instead, rather than trying to fix this now, we persist with what
> we have, but organize an effort to clean up and restructure the DMA engine
> code so that:
>
> (a) things like the Samsung, and ARM board oddities can be easily dealt
> with in a driver independent manner.
> (b) get rid of all the duplicated functionality between each different
> DMA engine implementation, separating this out into core code, and
> simplifying the drivers.
>
> The _big_ problem I see is that if we try to represent the existing
> solution in DT, we're going to get locked into that way of doing things
> and then any major restructuring of the DMA engine stuff will become an
> impossibility - especially if we start specifying things by DMA request
> signal numbers on DMA engines.
Even if I understand your point, I wonder what is the solution for us
that have a pretty simple representation of *hardware* to code into DT:
should we wait for a big rework? Should we go for a private DMA binding
for each of us that have just the need for a quite simple representation?
I must say that I am puzzled by recent discussion on the topic (mix of
"channel" and "request" notions, plan for a complete rework of dmaengine
that can delay DT representation of DMA slave-controller relationship,
even my own doubts on the "void *" output parameter, etc.). It seems
that I am not familiar with all those cases and that I cannot go further
with a generic DT representation...
Best regards,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Monday 19 March 2012, Russell King - ARM Linux wrote:
> Firstly, define what you mean by "the DMA parameters". Things like burst
> size, register width, register address? That should all be known by the
> peripheral driver and not be encoded into DT in any way - and this
> information should be passed to the DMA engine driver for the well defined
> API for that purpose.
This seems to be different for each controller, I tried to use
a generic term here. In case of stedma40 (which is probably the most
complex one today), it would be this structure:
struct stedma40_chan_cfg {
enum stedma40_xfer_dir dir;
bool high_priority;
bool realtime;
enum stedma40_mode mode;
enum stedma40_mode_opt mode_opt;
int src_dev_type;
int dst_dev_type;
struct stedma40_half_channel_infosrc_info;
struct stedma40_half_channel_infodst_info;
bool use_fixed_channel;
int phy_channel;
};
All these fields are set up by the platform code today, not by
the device driver, so it would be logical to have them represented
in the device tree, though some of the fields might turn out to
be in the wrong place already.
> Secondly, there are platforms (Samsung) where peripherals are connected
> to more than one DMA controller, and either DMA controller can be used -
> I'm told by Jassi that there's reasons why you'd want to select one or
> other as the target at runtime.
How common is that? If there are only a few drivers that have this
requirement, we could have this represented in the driver binding, by
listing multiple channels and giving the device the choice.
A possible way to deal with this is to list all the alternatives
in the device tree and using the dma-names property to name them,
giving the same name to those that we need to pick one of.
> I think instead, rather than trying to fix this now, we persist with what
> we have, but organize an effort to clean up and restructure the DMA engine
> code so that:
>
> (a) things like the Samsung, and ARM board oddities can be easily dealt
> with in a driver independent manner.
> (b) get rid of all the duplicated functionality between each different
> DMA engine implementation, separating this out into core code, and
> simplifying the drivers.
At that point, we definitely need to involve the dmaenigne maintainers
who have unfortunately not been on Cc for the whole discussion so
far.
I actually hope that a lot of the issues go away if we come up with
good device tree bindings that can cover the existing corner cases
while providing a simple interface for device drivers to use.
> The big problem I see is that if we try to represent the existing
> solution in DT, we're going to get locked into that way of doing things
> and then any major restructuring of the DMA engine stuff will become an
> impossibility - especially if we start specifying things by DMA request
> signal numbers on DMA engines.
Note that Thomas Abraham has already introduced a DT support for
pl330, which is sadly seems to be anything but generic.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 3/19/2012 4:20 PM, Russell King - ARM Linux wrote:
On Mon, Mar 19, 2012 at 02:37:56PM +0100, Nicolas Ferre wrote:
On 03/18/2012 09:13 PM, Arnd Bergmann :
On Saturday 17 March 2012, Grant Likely wrote:
+static LIST_HEAD(of_dma_list);
+
+struct of_dma {
+ struct list_head of_dma_controllers;
+ struct device_node *of_node;
+ int of_dma_n_cells;
+ int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
+};
This _xlate is nearly useless as a generic API. It solves the problem for
the specific case where the driver is hard-coded to know which DMA engine
to talk to, but since the returned data doesn't provide any context, it
isn't useful if there are multiple DMA controllers to choose from.
The void *data pointer must be replaced with a typed structure so that
context can be returned.
I've read up a bit more on how the existing drivers use the filter
functions, it seems there are multiple classes of them, the classes
that I've encountered are:
1. matches on chan->device pointer and/or chan_id
I have the impression that we are now talking about *channel* selection.
It is not the purpose of those helper functions. It is just to retrieve
a *request line* for a particular slave interface.
The request line on what though? The DMA controller, or something
else?
The idea is just to represent the request line from the IP to the DMA
controller. This is exactly the same pattern than for the IRQ line
toward the IRQ controller.
The reason I ask is because on ARM boards, the DMA controller has N
request lines. Three of those request lines are multiplexed in an
external FPGA to select between N different peripherals.
For example, the UART and MMCI may be routed to the FPGA multiplexer,
and either could be routed to DMA controller request signal 0, 1, or 2,
whereas a different UART might be routed directly to DMA controller
request signal 3 and 4.
At the moment, we handle this via platform data and callbacks passed
into the PL080 DMA driver, by matching virtual DMA channels based upon
strings looked up in the platform data. This gives the range of
DMA request signal numbers on the controller, and calls a platform
provided function to setup the multiplexer.
Clearly, if your proposal is all about DMA controller request signals
only, the above scheme can't be represented in DT...
Yes, this is the goal. I guess in your case you do need some DMA nexus
node like DT is already providing for the IRQ. But you still should be
able to describe how the HW blocks are interconnected.
and that's what
worries me - the fact is that the DMA engine API can and does cope
with this but it seems that we're building restrictions in with DT on
what can be represented.
But then you are mixing the SW representation and the HW one. DMAengine
is the Linux way of managing DMA channel. DT is supposed to be OS
agnostic and just describe the HW.
So you cannot take into account some DMAengine specificity to define the
DT binding. At least in theory.
That makes me wonder whether the model that's being chosen is anywhere
near correct for the DMA engine API.
That's not the goal. The goal is to describe how the DMA request lines
are connected from the IP to the controller.
Now, as I've said before, I'm trying to work on creating an OMAP DMA engine
implementation, and I'd _really_ like to have the freedom to do what's
necessary there without having to think one bit about DT getting in the
way. To put it another way, I don't want to be constrained by any weirdo
DT representations at the moment, and I really don't want to waste time
discussing them at the moment, rather than getting on with that job.
Because if I end up discussing this at length, I'm not going to be able
to do anything on the OMAP DMA engine stuff.
The current way of representing the DMA requests for OMAP SDMA is pretty
straightforward and fit well with the current binding. It looks to me
that this is the ARM boards that will be tricky to handle.
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Monday 19 March 2012, Stephen Warren wrote:
> > Maybe one can use named properties in a new device node in that case,
> > like this:
> >
> > bus {
> > dma: dma-controller {
> > #dma-cells = <1>;
> > };
> >
> > device {
> > compatible = "device";
> > channel: dma-channel {
> > type = <0x1>;
> > name = "foo";
> > number = <23>;
> > direction = <3>;
> > };
> > dma-requests = <&dma &channel>;
> > };
> > };
>
> For reference, this is very similar to how the pinctrl bindings work,
> except that they require the "channel" node to be a child of the DMA
> controller, and hence "dma-requests" doesn't contain <&dma &channel>,
> just <&channel>, since "dma" is the parent (or grand-parent) of "channel".
Right, but the difference beytween the pinctrl binding and what I
describe here is that the channel description would be part of the
dma engine driver specific binding, not the generic binding that
is visible to device drivers.
As I said, one dmaengine could use a phandle while another one could
just use a single number or nothing at all. If it chooses to use
a phandle (or possible a combination of phandle and number), the
binding could mandate for the device node to be either a child of the
dma controller or the device using it. In either case it would be
sensible to do it the same way for all dmaengine drivers that want
to have a phandle in the argument.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Mon, Mar 19, 2012 at 02:06:34PM +, Arnd Bergmann wrote:
> mmci
> /* in drivers/dma/ste_dma40.c, others in pl330.c, coh901318.c, ... */
> bool stedma40_filter(struct dma_chan *chan, void *data)
> {
> struct stedma40_chan_cfg *info = data;
> struct d40_chan *d40c = container_of(chan, struct d40_chan, chan);
> int err;
>
> err = d40_validate_conf(d40c, info);
> if (!err)
> d40c->dma_cfg = *info;
> d40c->configured = true;
>
> return err == 0;
> }
> EXPORT_SYMBOL(stedma40_filter);
>
> /* in mmci.h */
> struct mmci_platform_data {
> ...
> bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
> void *dma_rx_param;
> void *dma_tx_param;
> };
>
> /* in mmci.c */
> static void __devinit mmci_dma_setup(struct mmci_host *host)
> {
> ...
> host->dma_rx_channel = dma_request_channel(mask, plat->dma_filter,
> plat->dma_rx_param);
> ...
> }
>
>
>
> Whatever we come up with obviously needs to work with both drivers.
> I think we will end up with something closer to the second one, except
> that the dma parameters do not come from platform_data but from the
> #dma-request property, which also identifies the controller and channel.
Firstly, define what you mean by "the DMA parameters". Things like burst
size, register width, register address? That should all be known by the
peripheral driver and _not_ be encoded into DT in any way - and this
information should be passed to the DMA engine driver for the well defined
API for that purpose.
Secondly, there are platforms (Samsung) where peripherals are connected
to more than one DMA controller, and either DMA controller can be used -
I'm told by Jassi that there's reasons why you'd want to select one or
other as the target at runtime.
Whether it's appropriate for DT to know that or not, I'm not certain at
the moment - I suspect the _right_ solution would be a radical DMA engine
redesign which moves _all_ slave DMA to a virtual channel representation,
and for the virtual channels to be scheduled onto a set of physical DMA
channels over a range of DMA devices. Obviously, there's restrictions on
which virtual channels could be scheduled onto which physical channels of
which DMA devices.
It seems to me, therefore, that any attempt to come up with some kind of
DT based representation of the current scheme is doomed to fail, and will
at some point become obsolete.
I think instead, rather than trying to fix this now, we persist with what
we have, but organize an effort to clean up and restructure the DMA engine
code so that:
(a) things like the Samsung, and ARM board oddities can be easily dealt
with in a driver independent manner.
(b) get rid of all the duplicated functionality between each different
DMA engine implementation, separating this out into core code, and
simplifying the drivers.
The _big_ problem I see is that if we try to represent the existing
solution in DT, we're going to get locked into that way of doing things
and then any major restructuring of the DMA engine stuff will become an
impossibility - especially if we start specifying things by DMA request
signal numbers on DMA engines.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Mon, Mar 19, 2012 at 02:37:56PM +0100, Nicolas Ferre wrote:
> On 03/18/2012 09:13 PM, Arnd Bergmann :
> > On Saturday 17 March 2012, Grant Likely wrote:
> >>> +static LIST_HEAD(of_dma_list);
> >>> +
> >>> +struct of_dma {
> >>> + struct list_head of_dma_controllers;
> >>> + struct device_node *of_node;
> >>> + int of_dma_n_cells;
> >>> + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
> >>> +};
> >>
> >> This _xlate is nearly useless as a generic API. It solves the problem for
> >> the specific case where the driver is hard-coded to know which DMA engine
> >> to talk to, but since the returned data doesn't provide any context, it
> >> isn't useful if there are multiple DMA controllers to choose from.
> >>
> >> The void *data pointer must be replaced with a typed structure so that
> >> context can be returned.
> >
> > I've read up a bit more on how the existing drivers use the filter
> > functions, it seems there are multiple classes of them, the classes
> > that I've encountered are:
> >
> > 1. matches on chan->device pointer and/or chan_id
>
> I have the impression that we are now talking about *channel* selection.
> It is not the purpose of those helper functions. It is just to retrieve
> a *request line* for a particular slave interface.
The request line on what though? The DMA controller, or something
else?
The reason I ask is because on ARM boards, the DMA controller has N
request lines. Three of those request lines are multiplexed in an
external FPGA to select between N different peripherals.
For example, the UART and MMCI may be routed to the FPGA multiplexer,
and either could be routed to DMA controller request signal 0, 1, or 2,
whereas a different UART might be routed directly to DMA controller
request signal 3 and 4.
At the moment, we handle this via platform data and callbacks passed
into the PL080 DMA driver, by matching virtual DMA channels based upon
strings looked up in the platform data. This gives the range of
DMA request signal numbers on the controller, and calls a platform
provided function to setup the multiplexer.
Clearly, if your proposal is all about DMA controller request signals
only, the above scheme can't be represented in DT... and that's what
worries me - the fact is that the DMA engine API can and does cope
with this but it seems that we're building restrictions in with DT on
what can be represented.
That makes me wonder whether the model that's being chosen is anywhere
near correct for the DMA engine API.
Now, as I've said before, I'm trying to work on creating an OMAP DMA engine
implementation, and I'd _really_ like to have the freedom to do what's
necessary there without having to think one bit about DT getting in the
way. To put it another way, I don't want to be constrained by any weirdo
DT representations at the moment, and I really don't want to waste time
discussing them at the moment, rather than getting on with that job.
Because if I end up discussing this at length, I'm not going to be able
to do anything on the OMAP DMA engine stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/19/2012 09:01 AM, Arnd Bergmann wrote:
> On Monday 19 March 2012, Mark Brown wrote:
>> On Sat, Mar 17, 2012 at 10:47:51AM +, Grant Likely wrote:
>>
>>> After implementing both schemes (ie. interrupts+interrupt-names &&
>>> [*-]gpios)
>>> I definitely prefer the fixed property name plus a separate names property.
>>> It is easier to use common code with that scheme, and easier to statically
>>> check for correctness.
>>
>> It's not a fantastic experience when using the bindings as the arrays
>> grow large, though - keeping things matched up isn't much fun especially
>> if any of the elements in the array are optional.
>
> Maybe one can use named properties in a new device node in that case,
> like this:
>
> bus {
> dma: dma-controller {
> #dma-cells = <1>;
> };
>
> device {
> compatible = "device";
> channel: dma-channel {
> type = <0x1>;
> name = "foo";
> number = <23>;
> direction = <3>;
> };
> dma-requests = <&dma &channel>;
> };
> };
For reference, this is very similar to how the pinctrl bindings work,
except that they require the "channel" node to be a child of the DMA
controller, and hence "dma-requests" doesn't contain <&dma &channel>,
just <&channel>, since "dma" is the parent (or grand-parent) of "channel".
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Monday 19 March 2012, Mark Brown wrote:
> On Sat, Mar 17, 2012 at 10:47:51AM +, Grant Likely wrote:
>
> > After implementing both schemes (ie. interrupts+interrupt-names &&
> > [*-]gpios)
> > I definitely prefer the fixed property name plus a separate names property.
> > It is easier to use common code with that scheme, and easier to statically
> > check for correctness.
>
> It's not a fantastic experience when using the bindings as the arrays
> grow large, though - keeping things matched up isn't much fun especially
> if any of the elements in the array are optional.
Maybe one can use named properties in a new device node in that case,
like this:
bus {
dma: dma-controller {
#dma-cells = <1>;
};
device {
compatible = "device";
channel: dma-channel {
type = <0x1>;
name = "foo";
number = <23>;
direction = <3>;
};
dma-requests = <&dma &channel>;
};
};
In this case, the dma engine's filter function would look up
the dma-channel device node from the phandle passed in the argument
to the dma-requests property, while a simpler dma engine would
just use a single number to identify a channel there.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Mon, 19 Mar 2012 14:30:05 +0100, Nicolas Ferre
wrote:
> On 03/17/2012 10:40 AM, Grant Likely :
> > On Thu, 15 Mar 2012 09:38:10 +0100, Nicolas Ferre
> > wrote:
> >> +struct of_dma {
> >> + struct list_head of_dma_controllers;
> >> + struct device_node *of_node;
> >> + int of_dma_n_cells;
> >> + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
> >> +};
> >
> > This _xlate is nearly useless as a generic API. It solves the problem for
> > the specific case where the driver is hard-coded to know which DMA engine
> > to talk to, but since the returned data doesn't provide any context, it
> > isn't useful if there are multiple DMA controllers to choose from.
>
> You mean, if there is no DMA controller phandle specified in the
> property?
No; I'm assuming that dma-channel properties will alwasy have a
phandle to the dma controller node.
> I think that it is not the purpose of this API to choose a DMA
> controller, Nor to provide a channel. The only purpose of this API is to
> give a HW request to be used by a DMA slave driver. This slave should
> already have a channel to use and a controller to talk to.
Then where is the function that finds the reference to the DMA
controller? I don't understand why it would be useful to decode that
separately.
> > The void *data pointer must be replaced with a typed structure so that
> > context can be returned.
>
> I am not sure to follow you entirely... How do I address the fact that
> several types of request value can be returned then?
>
> BTW, can we imagine a phandle property with a sting as a argument?
> should it be written this way?
> dma-request = <&testdmac1>, "slave-rx", "slave-tx";
No, I'm not suggesting that. Mixing phandles and strings in a single
property is possible but ugly. The phandle-args pattern which uses
zero or more cells as arguments should be used.
>
> If yes, the of_parse_phandle_with_args() is not working on this type...
Right; of_parse_phandle_with_args() should do the job.
>
> (I realize that there seems to be no way out for a generic API: maybe we
> should move to one or two cases to address and concentrate on them).
The way I read this patch, the xlate function returns a single
integer representing the DMA request number, but it doesn't provide
any data about *which* dma controller is associated with that channel.
The result of xlate needs to be something like a dma_chan reference
that identifies both the DMA engine and the channel/request on that
dma engine.
[...]
> >> +/**
> >> + * of_dma_xlate_onenumbercell() - Generic DMA xlate for direct one cell
> >> bindings
> >> + *
> >> + * Device Tree DMA translation function which works with one cell bindings
> >> + * where the cell values map directly to the hardware request number
> >> understood
> >> + * by the DMA controller.
> >> + */
> >> +int of_dma_xlate_onenumbercell(struct of_phandle_args *dma_spec, void
> >> *dma_req)
> >> +{
> >> + if (!dma_spec)
> >> + return -EINVAL;
> >> + if (WARN_ON(dma_spec->args_count != 1))
> >> + return -EINVAL;
> >> + *(int *)dma_req = dma_spec->args[0];
> >
> > Following on from comment above; the void *dma_req parameter is dangerous.
> > How
> > does this function know that it has been passed an int* pointer?
>
> Well, that is a drawback that comes from having to address generic
> cases.
Not if you do it right. If a specific data structure is returned,
then there can be context attached as to what the data means and which
dma controller knows how to parse it.
> But anyway, if the DMA controller decide to register a .xlate()
> function that returns an integer, the slave driver that will ask a
> "request line" to this DMA controller will be aware that an integer has
> to be passed to of_get_dma_request().
The problem still remains; I don't see how the dma slave can figure
out *which* dma controller it needs to talk to.
g.
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Monday 19 March 2012, Nicolas Ferre wrote:
> > This _xlate is nearly useless as a generic API. It solves the problem for
> > the specific case where the driver is hard-coded to know which DMA engine
> > to talk to, but since the returned data doesn't provide any context, it
> > isn't useful if there are multiple DMA controllers to choose from.
>
> You mean, if there is no DMA controller phandle specified in the
> property? I think that it is not the purpose of this API to choose a DMA
> controller, Nor to provide a channel. The only purpose of this API is to
> give a HW request to be used by a DMA slave driver. This slave should
> already have a channel to use and a controller to talk to.
I don't think there is consensus on this point. I would expect that
the device driver requests the channel with the same operation as
passing the request data.
Contrast the two ways this is done in atmel-mci.c and mmci.c:
atmel
/* interface between platform and driver */
struct at_dma_slave {
struct device *dma_dev;
dma_addr_t tx_reg;
dma_addr_t rx_reg;
enum at_dma_slave_width reg_width;
u32 cfg;
u32 ctrla;
};
struct mci_dma_data {
struct at_dma_slave sdata;
};
#define slave_data_ptr(s) (&(s)->sdata)
#define find_slave_dev(s) ((s)->sdata.dma_dev)
/* in atmel-mci.c */
static bool atmci_filter(struct dma_chan *chan, void *slave)
{
struct mci_dma_data *sl = slave;
if (sl && find_slave_dev(sl) == chan->device->dev) {
chan->private = slave_data_ptr(sl);
return true;
} else {
return false;
}
}
static bool atmci_configure_dma(struct atmel_mci *host)
{
...
host->dma.chan = dma_request_channel(mask, atmci_filter,
pdata->dma_slave);
...
}
mmci
/* in drivers/dma/ste_dma40.c, others in pl330.c, coh901318.c, ... */
bool stedma40_filter(struct dma_chan *chan, void *data)
{
struct stedma40_chan_cfg *info = data;
struct d40_chan *d40c = container_of(chan, struct d40_chan, chan);
int err;
err = d40_validate_conf(d40c, info);
if (!err)
d40c->dma_cfg = *info;
d40c->configured = true;
return err == 0;
}
EXPORT_SYMBOL(stedma40_filter);
/* in mmci.h */
struct mmci_platform_data {
...
bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
void *dma_rx_param;
void *dma_tx_param;
};
/* in mmci.c */
static void __devinit mmci_dma_setup(struct mmci_host *host)
{
...
host->dma_rx_channel = dma_request_channel(mask, plat->dma_filter,
plat->dma_rx_param);
...
}
Whatever we come up with obviously needs to work with both drivers.
I think we will end up with something closer to the second one, except
that the dma parameters do not come from platform_data but from the
#dma-request property, which also identifies the controller and channel.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/18/2012 09:13 PM, Arnd Bergmann :
> On Saturday 17 March 2012, Grant Likely wrote:
>>> +static LIST_HEAD(of_dma_list);
>>> +
>>> +struct of_dma {
>>> + struct list_head of_dma_controllers;
>>> + struct device_node *of_node;
>>> + int of_dma_n_cells;
>>> + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
>>> +};
>>
>> This _xlate is nearly useless as a generic API. It solves the problem for
>> the specific case where the driver is hard-coded to know which DMA engine
>> to talk to, but since the returned data doesn't provide any context, it
>> isn't useful if there are multiple DMA controllers to choose from.
>>
>> The void *data pointer must be replaced with a typed structure so that
>> context can be returned.
>
> I've read up a bit more on how the existing drivers use the filter
> functions, it seems there are multiple classes of them, the classes
> that I've encountered are:
>
> 1. matches on chan->device pointer and/or chan_id
I have the impression that we are now talking about *channel* selection.
It is not the purpose of those helper functions. It is just to retrieve
a *request line* for a particular slave interface. The
selection/filtering of a channel is a different topic (that we can
address in the future).
Maybe some DMA controllers are able to handle several "request lines"
among several DMA controller instances. But I suspect that this case
should be handled in another place, before calling this API.
>(8 drivers)
> 2. will match anything
>(6 drivers)
> 3. requires specific dma engine driver, then behaves like 1 or 2
>(8 drivers, almost all freescale)
> 4. one of a kind, matches resource name string or device->dev_id
>(two drivers)
> 5. filter function and data both provided by platform code,
>platform picks dmaengine driver.
>(4 amba pl* drivers, used on ARM, ux500, ...)
>
> The last category is interesting because here, the dmaengine
> driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter
> function while in the other cases that is provided by the device
> driver! Out of these, the ste_dma40 is special because it's the
> only one where the data is a complex data structure describing the
> constraints on the driver, while all others just find the right
> channel.
>
> Some drivers also pass assign driver specific data to chan->private.
>
> I would hope that we can all make them use something like
> struct dma_channel *of_dma_request_channel(struct of_node*,
> int index, void *driver_data);
> with an appropriate common definition behind it. In the cases
> where the driver can just match anything, I'd assume that all
> channels are equal, so #dma-cells would be 0. For the ste_dma40,
> #dma-cells needs to cover all of stedma40_chan_cfg. In most
> other cases, #dma-cells can be 1 and just enumerate the channels,
> unless we want to simplify the cases that Russell mentioned where
> we want to keep a two stage mapping channel identifiers and physical
> channel numbers.
>
> How about an implementation like this:?
>
> typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param)
> {
> /* zero #dma-cells, accept anything */
> return true;
> }
>
> struct dma_channel *of_dma_request_channel(struct of_node*, int index,
> dma_cap_mask_t *mask,
> void *driver_data)
> {
> struct of_phandle_args dma_spec;
> struct dma_device *device;
> struct dma_chan *chan = NULL;
> dma_filter_fn *filter;
>
> ret = of_parse_phandle_with_args(np, "dma-request", "#dma-cells",
>index, &dma_spec);
Well, this property handles "request lines" not "channels". So if we
move the selection/filtering of channels in DT, we may need to create a
new property of this purpose...
>
> device = dma_find_device(dma_spec->np);
> if (!device)
> goto out;
>
> if (dma_spec->args_count == 0)
> filter = dma_filter_simple;
> else
> filter = device->dma_dt_filter; /* new member */
>
> chan = private_candidate(mask, device, filter, dma_spec->args);
>
> if (chan && !chan->private)
> chan->private = driver_data;
> out:
> return chan;
> }
>
> Arnd
>
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/17/2012 10:40 AM, Grant Likely :
> On Thu, 15 Mar 2012 09:38:10 +0100, Nicolas Ferre
> wrote:
>> Add some basic helpers to retrieve a DMA controller device_node and the
>> DMA request specifications. By making DMA controllers register a generic
>> translation function, we allow the management of any type of DMA requests
>> specification.
>> The void * output of an of_dma_xlate() function that will be implemented
>> by the DMA controller can carry any type of "dma-request" argument.
>>
>> The DMA client will search its associated DMA controller in the list and
>> call the registered of_dam_xlate() function to retrieve the request values.
>>
>> One simple xlate function is provided for the "single number" type of
>> request binding.
>>
>> This implementation is independent from dmaengine so it can also be used
>> by legacy drivers.
>>
>> For legacy reason another API will export the DMA request number into a
>> Linux resource of type IORESOURCE_DMA.
>>
>> Signed-off-by: Nicolas Ferre
>> Signed-off-by: Benoit Cousson
>
> Hi Nicolas,
>
> Comments below.
>
>> Cc: Stephen Warren
>> Cc: Grant Likely
>> Cc: Russell King
>> Cc: Rob Herring
>> ---
>> Documentation/devicetree/bindings/dma/dma.txt | 45 +
>> drivers/of/Kconfig|5 +
>> drivers/of/Makefile |1 +
>> drivers/of/dma.c | 260
>> +
>> include/linux/of_dma.h| 67 +++
>> 5 files changed, 378 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/dma/dma.txt
>> create mode 100644 drivers/of/dma.c
>> create mode 100644 include/linux/of_dma.h
>>
>> diff --git a/Documentation/devicetree/bindings/dma/dma.txt
>> b/Documentation/devicetree/bindings/dma/dma.txt
>> new file mode 100644
>> index 000..c49e98d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/dma.txt
>> @@ -0,0 +1,45 @@
>> +* Generic DMA Controller and DMA request bindings
>> +
>> +Generic binding to provide a way for a driver to retrieve the
>> +DMA request line that goes from an IP to a DMA controller.
>> +
>> +
>> +* DMA controller
>> +
>> +Required property:
>> +- #dma-cells: Number of cells for each DMA line.
>> +
>> +
>> +Example:
>> +
>> +sdma: dma-controller@4800 {
>> +compatible = "ti,sdma-omap4"
>> +reg = <0x4800 0x1000>;
>> +interrupts = <12>;
>> +#dma-cells = <1>;
>> +};
>> +
>> +
>> +
>> +* DMA client
>> +
>> +Client drivers should specify the DMA request property using a phandle to
>> +the controller. If needed, the DMA request identity on that controller is
>> then
>> +added followed by optional request specifications.
>> +
>> +Required property:
>> +- dma-request: List of phandle + dma-request + request specifications,
>> + one group per request "line".
>> +Optional property:
>> +- dma-request-names: list of strings in the same order as the
>> dma-request
>> + in the dma-request property.
>> +
>> +
>> +Example:
>> +
>> +i2c1: i2c@1 {
>> +...
>> +dma-request = <&sdma 2 &sdma 3>;
>> +dma-request-names = "tx", "rx";
>> +...
>> +};
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 268163d..7d1f06b 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -90,4 +90,9 @@ config OF_PCI_IRQ
>> help
>>OpenFirmware PCI IRQ routing helpers
>>
>> +config OF_DMA
>> +def_bool y
>> +help
>> + Device Tree DMA routing helpers
>
> Not really any point in having this config symbol at all if it is
> always enabled.
Well, it is the same for:
config OF_DEVICE
But for sure, I can remove it: so I will add it to
obj-y = base.o
in the Makefile.
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index a73f5a5..6eb50c6 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_SELFTEST) += selftest.o
>> obj-$(CONFIG_OF_MDIO) += of_mdio.o
>> obj-$(CONFIG_OF_PCI)+= of_pci.o
>> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
>> +obj-$(CONFIG_OF_DMA)+= dma.o
>> diff --git a/drivers/of/dma.c b/drivers/of/dma.c
>> new file mode 100644
>> index 000..3315844
>> --- /dev/null
>> +++ b/drivers/of/dma.c
>> @@ -0,0 +1,260 @@
>> +/*
>> + * Device tree helpers for DMA request / controller
>> + *
>> + * Based on of_gpio.c
>> + *
>> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +static LIST_HEAD(of_dma_list);
>> +
>> +struct of_dma {
>> +struct list_head of_dma_controller
Re: [PATCH] of: Add generic device tree DMA helpers
On Sat, Mar 17, 2012 at 10:47:51AM +, Grant Likely wrote: > After implementing both schemes (ie. interrupts+interrupt-names && [*-]gpios) > I definitely prefer the fixed property name plus a separate names property. > It is easier to use common code with that scheme, and easier to statically > check for correctness. It's not a fantastic experience when using the bindings as the arrays grow large, though - keeping things matched up isn't much fun especially if any of the elements in the array are optional. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Arnd Bergmann wrote:
> >
> > Is dma_find_device() a new function? How does it look up the dma
> > device?
>
> Yes, it would be similar to the proposed function in Benoit's patch
>
Well, actually we would not even need a new list with all the devices,
it can simply become
/* must be called with dma_list_mutex held */
static struct dma_device *dma_find_device(struct of_node *node)
{
struct dma_device *device;
list_for_each_entry(device, &dma_device_list, global_node) {
if (device->dev.of_node == node)
break;
}
return device;
}
Given that dma_device_list is most likely be a very short list, this will
be a fast operation.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Grant Likely wrote:
> > struct dma_channel *of_dma_request_channel(struct of_node*, int index,
> > dma_cap_mask_t *mask,
> > void *driver_data)
> > {
> > struct of_phandle_args dma_spec;
> > struct dma_device *device;
> > struct dma_chan *chan = NULL;
> > dma_filter_fn *filter;
> >
> > ret = of_parse_phandle_with_args(np, "dma-request", "#dma-cells",
> >index, &dma_spec);
> >
> > device = dma_find_device(dma_spec->np);
>
> Is dma_find_device() a new function? How does it look up the dma
> device?
Yes, it would be similar to the proposed function in Benoit's patch
> >
> > if (dma_spec->args_count == 0)
> > filter = dma_filter_simple;
> > else
> > filter = device->dma_dt_filter; /* new member */
>
> I'm not thrilled with this if/else hunk; even the case of
> #dma-cells=<0> should provide a hook; even it if is the stock simple
> filter. Leaving filter as NULL is the same as accepting everything
> anyway.
Right, good point. So a dmaengine driver would either register a trivial
filter if it wants to do anything here, or it would just leave it as a NULL
pointer otherwise.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sun, 18 Mar 2012 20:13:22 +, Arnd Bergmann wrote:
> On Saturday 17 March 2012, Grant Likely wrote:
> > > +static LIST_HEAD(of_dma_list);
> > > +
> > > +struct of_dma {
> > > + struct list_head of_dma_controllers;
> > > + struct device_node *of_node;
> > > + int of_dma_n_cells;
> > > + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
> > > +};
> >
> > This _xlate is nearly useless as a generic API. It solves the problem for
> > the specific case where the driver is hard-coded to know which DMA engine
> > to talk to, but since the returned data doesn't provide any context, it
> > isn't useful if there are multiple DMA controllers to choose from.
> >
> > The void *data pointer must be replaced with a typed structure so that
> > context can be returned.
>
> I've read up a bit more on how the existing drivers use the filter
> functions, it seems there are multiple classes of them, the classes
> that I've encountered are:
>
> 1. matches on chan->device pointer and/or chan_id
>(8 drivers)
> 2. will match anything
>(6 drivers)
> 3. requires specific dma engine driver, then behaves like 1 or 2
>(8 drivers, almost all freescale)
> 4. one of a kind, matches resource name string or device->dev_id
>(two drivers)
> 5. filter function and data both provided by platform code,
>platform picks dmaengine driver.
>(4 amba pl* drivers, used on ARM, ux500, ...)
>
> The last category is interesting because here, the dmaengine
> driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter
> function while in the other cases that is provided by the device
> driver! Out of these, the ste_dma40 is special because it's the
> only one where the data is a complex data structure describing the
> constraints on the driver, while all others just find the right
> channel.
>
> Some drivers also pass assign driver specific data to chan->private.
>
> I would hope that we can all make them use something like
> struct dma_channel *of_dma_request_channel(struct of_node*,
> int index, void *driver_data);
certainly my hope too!
> with an appropriate common definition behind it. In the cases
> where the driver can just match anything, I'd assume that all
> channels are equal, so #dma-cells would be 0. For the ste_dma40,
> #dma-cells needs to cover all of stedma40_chan_cfg. In most
> other cases, #dma-cells can be 1 and just enumerate the channels,
> unless we want to simplify the cases that Russell mentioned where
> we want to keep a two stage mapping channel identifiers and physical
> channel numbers.
>
> How about an implementation like this:?
>
> typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param)
> {
> /* zero #dma-cells, accept anything */
> return true;
> }
>
> struct dma_channel *of_dma_request_channel(struct of_node*, int index,
> dma_cap_mask_t *mask,
> void *driver_data)
> {
> struct of_phandle_args dma_spec;
> struct dma_device *device;
> struct dma_chan *chan = NULL;
> dma_filter_fn *filter;
>
> ret = of_parse_phandle_with_args(np, "dma-request", "#dma-cells",
>index, &dma_spec);
>
> device = dma_find_device(dma_spec->np);
Is dma_find_device() a new function? How does it look up the dma
device?
> if (!device)
> goto out;
Just return NULL here.
>
> if (dma_spec->args_count == 0)
> filter = dma_filter_simple;
> else
> filter = device->dma_dt_filter; /* new member */
I'm not thrilled with this if/else hunk; even the case of
#dma-cells=<0> should provide a hook; even it if is the stock simple
filter. Leaving filter as NULL is the same as accepting everything
anyway.
>
> chan = private_candidate(mask, device, filter, dma_spec->args);
>
> if (chan && !chan->private)
> chan->private = driver_data;
> out:
> return chan;
I think this looks right, except for the comments above.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Saturday 17 March 2012, Grant Likely wrote:
> > +static LIST_HEAD(of_dma_list);
> > +
> > +struct of_dma {
> > + struct list_head of_dma_controllers;
> > + struct device_node *of_node;
> > + int of_dma_n_cells;
> > + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
> > +};
>
> This _xlate is nearly useless as a generic API. It solves the problem for
> the specific case where the driver is hard-coded to know which DMA engine
> to talk to, but since the returned data doesn't provide any context, it
> isn't useful if there are multiple DMA controllers to choose from.
>
> The void *data pointer must be replaced with a typed structure so that
> context can be returned.
I've read up a bit more on how the existing drivers use the filter
functions, it seems there are multiple classes of them, the classes
that I've encountered are:
1. matches on chan->device pointer and/or chan_id
(8 drivers)
2. will match anything
(6 drivers)
3. requires specific dma engine driver, then behaves like 1 or 2
(8 drivers, almost all freescale)
4. one of a kind, matches resource name string or device->dev_id
(two drivers)
5. filter function and data both provided by platform code,
platform picks dmaengine driver.
(4 amba pl* drivers, used on ARM, ux500, ...)
The last category is interesting because here, the dmaengine
driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter
function while in the other cases that is provided by the device
driver! Out of these, the ste_dma40 is special because it's the
only one where the data is a complex data structure describing the
constraints on the driver, while all others just find the right
channel.
Some drivers also pass assign driver specific data to chan->private.
I would hope that we can all make them use something like
struct dma_channel *of_dma_request_channel(struct of_node*,
int index, void *driver_data);
with an appropriate common definition behind it. In the cases
where the driver can just match anything, I'd assume that all
channels are equal, so #dma-cells would be 0. For the ste_dma40,
#dma-cells needs to cover all of stedma40_chan_cfg. In most
other cases, #dma-cells can be 1 and just enumerate the channels,
unless we want to simplify the cases that Russell mentioned where
we want to keep a two stage mapping channel identifiers and physical
channel numbers.
How about an implementation like this:?
typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param)
{
/* zero #dma-cells, accept anything */
return true;
}
struct dma_channel *of_dma_request_channel(struct of_node*, int index,
dma_cap_mask_t *mask,
void *driver_data)
{
struct of_phandle_args dma_spec;
struct dma_device *device;
struct dma_chan *chan = NULL;
dma_filter_fn *filter;
ret = of_parse_phandle_with_args(np, "dma-request", "#dma-cells",
index, &dma_spec);
device = dma_find_device(dma_spec->np);
if (!device)
goto out;
if (dma_spec->args_count == 0)
filter = dma_filter_simple;
else
filter = device->dma_dt_filter; /* new member */
chan = private_candidate(mask, device, filter, dma_spec->args);
if (chan && !chan->private)
chan->private = driver_data;
out:
return chan;
}
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sun, 18 Mar 2012 10:22:41 +0100, Thierry Reding wrote: > * Grant Likely wrote: > > On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding > > wrote: > > > So if we decide to explicitly allow specifying names, then we can always > > > add > > > a pwm-names property (or -pwm-names respectively) to use as label > > > and > > > fallback to the user OF device node name if that property is not present. > > > > After implementing both schemes (ie. interrupts+interrupt-names && > > [*-]gpios) > > I definitely prefer the fixed property name plus a separate names property. > > It is easier to use common code with that scheme, and easier to statically > > check for correctness. > > Okay. Would everyone be happy with "pwms" and "pwm-names"? okay. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Thierry Reding wrote: > Not enough information to check signature validity. Show Details > * Grant Likely wrote: > > On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding > > wrote: > > > So if we decide to explicitly allow specifying names, then we can always > > > add > > > a pwm-names property (or -pwm-names respectively) to use as label > > > and > > > fallback to the user OF device node name if that property is not present. > > > > After implementing both schemes (ie. interrupts+interrupt-names && > > [*-]gpios) > > I definitely prefer the fixed property name plus a separate names property. > > It is easier to use common code with that scheme, and easier to statically > > check for correctness. > > Okay. Would everyone be happy with "pwms" and "pwm-names"? Sounds good. I would have suggested "pwm", but the plural also works since that is used for "interrupts", too. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
* Grant Likely wrote: > On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding > wrote: > > So if we decide to explicitly allow specifying names, then we can always add > > a pwm-names property (or -pwm-names respectively) to use as label and > > fallback to the user OF device node name if that property is not present. > > After implementing both schemes (ie. interrupts+interrupt-names && [*-]gpios) > I definitely prefer the fixed property name plus a separate names property. > It is easier to use common code with that scheme, and easier to statically > check for correctness. Okay. Would everyone be happy with "pwms" and "pwm-names"? Thierry pgplUvVS1Lb0u.pgp Description: PGP signature
Re: [PATCH] of: Add generic device tree DMA helpers
On Sat, 17 Mar 2012 16:03:02 +, Arnd Bergmann wrote: > On Saturday 17 March 2012, Grant Likely wrote: > > On Thu, 15 Mar 2012 09:26:52 +, Russell King - ARM Linux > > wrote: > > > On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: > > > > On Thursday 15 March 2012, Nicolas Ferre wrote: > > > > > Add some basic helpers to retrieve a DMA controller device_node and > > > > > the > > > > > DMA request specifications. By making DMA controllers register a > > > > > generic > > > > > translation function, we allow the management of any type of DMA > > > > > requests > > > > > specification. > > > > > The void * output of an of_dma_xlate() function that will be > > > > > implemented > > > > > by the DMA controller can carry any type of "dma-request" argument. > > > > > > > > > > The DMA client will search its associated DMA controller in the list > > > > > and > > > > > call the registered of_dam_xlate() function to retrieve the request > > > > > values. > > > > > > > > > > One simple xlate function is provided for the "single number" type of > > > > > request binding. > > > > > > > > > > This implementation is independent from dmaengine so it can also be > > > > > used > > > > > by legacy drivers. > > > > > > > > > > For legacy reason another API will export the DMA request number into > > > > > a > > > > > Linux resource of type IORESOURCE_DMA. > > > > > > > > This looks very good. I missed the first version of this patch, but was > > > > thinking of very similar bindings. > > > > > > There's one issue which is concerning me - when we switch OMAP to use > > > DMA engine, it won't use numeric stuff anymore but the DMA engine way > > > of requesting a channel (match function + data). > > > > > > How does that fit into this scheme? > > > > Not well as the current patch set stands. The xlate function doesn't return > > any context for the dma channel number that it returns, so the driver cannot > > figure out which DMA controller to use if there are multiple. > > Shouldn't that be part of the data returned by xlate? I was under the > impression > that this would be the data that you would pass into dma_request_channel, > together > with a filter function that find the instance from the data. Yes, that's my point. The patch as it is currently written doesn't do what it needs to. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Saturday 17 March 2012, Grant Likely wrote: > On Thu, 15 Mar 2012 09:26:52 +, Russell King - ARM Linux > wrote: > > On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: > > > On Thursday 15 March 2012, Nicolas Ferre wrote: > > > > Add some basic helpers to retrieve a DMA controller device_node and the > > > > DMA request specifications. By making DMA controllers register a generic > > > > translation function, we allow the management of any type of DMA > > > > requests > > > > specification. > > > > The void * output of an of_dma_xlate() function that will be implemented > > > > by the DMA controller can carry any type of "dma-request" argument. > > > > > > > > The DMA client will search its associated DMA controller in the list and > > > > call the registered of_dam_xlate() function to retrieve the request > > > > values. > > > > > > > > One simple xlate function is provided for the "single number" type of > > > > request binding. > > > > > > > > This implementation is independent from dmaengine so it can also be used > > > > by legacy drivers. > > > > > > > > For legacy reason another API will export the DMA request number into a > > > > Linux resource of type IORESOURCE_DMA. > > > > > > This looks very good. I missed the first version of this patch, but was > > > thinking of very similar bindings. > > > > There's one issue which is concerning me - when we switch OMAP to use > > DMA engine, it won't use numeric stuff anymore but the DMA engine way > > of requesting a channel (match function + data). > > > > How does that fit into this scheme? > > Not well as the current patch set stands. The xlate function doesn't return > any context for the dma channel number that it returns, so the driver cannot > figure out which DMA controller to use if there are multiple. Shouldn't that be part of the data returned by xlate? I was under the impression that this would be the data that you would pass into dma_request_channel, together with a filter function that find the instance from the data. That's why I suggested adding another helper function that would provide a generic filter function for the dt binding doing just that. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, 15 Mar 2012 09:26:52 +, Russell King - ARM Linux wrote: > On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: > > On Thursday 15 March 2012, Nicolas Ferre wrote: > > > Add some basic helpers to retrieve a DMA controller device_node and the > > > DMA request specifications. By making DMA controllers register a generic > > > translation function, we allow the management of any type of DMA requests > > > specification. > > > The void * output of an of_dma_xlate() function that will be implemented > > > by the DMA controller can carry any type of "dma-request" argument. > > > > > > The DMA client will search its associated DMA controller in the list and > > > call the registered of_dam_xlate() function to retrieve the request > > > values. > > > > > > One simple xlate function is provided for the "single number" type of > > > request binding. > > > > > > This implementation is independent from dmaengine so it can also be used > > > by legacy drivers. > > > > > > For legacy reason another API will export the DMA request number into a > > > Linux resource of type IORESOURCE_DMA. > > > > This looks very good. I missed the first version of this patch, but was > > thinking of very similar bindings. > > There's one issue which is concerning me - when we switch OMAP to use > DMA engine, it won't use numeric stuff anymore but the DMA engine way > of requesting a channel (match function + data). > > How does that fit into this scheme? Not well as the current patch set stands. The xlate function doesn't return any context for the dma channel number that it returns, so the driver cannot figure out which DMA controller to use if there are multiple. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, 15 Mar 2012 09:38:10 +0100, Nicolas Ferre
wrote:
> Add some basic helpers to retrieve a DMA controller device_node and the
> DMA request specifications. By making DMA controllers register a generic
> translation function, we allow the management of any type of DMA requests
> specification.
> The void * output of an of_dma_xlate() function that will be implemented
> by the DMA controller can carry any type of "dma-request" argument.
>
> The DMA client will search its associated DMA controller in the list and
> call the registered of_dam_xlate() function to retrieve the request values.
>
> One simple xlate function is provided for the "single number" type of
> request binding.
>
> This implementation is independent from dmaengine so it can also be used
> by legacy drivers.
>
> For legacy reason another API will export the DMA request number into a
> Linux resource of type IORESOURCE_DMA.
>
> Signed-off-by: Nicolas Ferre
> Signed-off-by: Benoit Cousson
Hi Nicolas,
Comments below.
> Cc: Stephen Warren
> Cc: Grant Likely
> Cc: Russell King
> Cc: Rob Herring
> ---
> Documentation/devicetree/bindings/dma/dma.txt | 45 +
> drivers/of/Kconfig|5 +
> drivers/of/Makefile |1 +
> drivers/of/dma.c | 260
> +
> include/linux/of_dma.h| 67 +++
> 5 files changed, 378 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/dma/dma.txt
> create mode 100644 drivers/of/dma.c
> create mode 100644 include/linux/of_dma.h
>
> diff --git a/Documentation/devicetree/bindings/dma/dma.txt
> b/Documentation/devicetree/bindings/dma/dma.txt
> new file mode 100644
> index 000..c49e98d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/dma.txt
> @@ -0,0 +1,45 @@
> +* Generic DMA Controller and DMA request bindings
> +
> +Generic binding to provide a way for a driver to retrieve the
> +DMA request line that goes from an IP to a DMA controller.
> +
> +
> +* DMA controller
> +
> +Required property:
> +- #dma-cells: Number of cells for each DMA line.
> +
> +
> +Example:
> +
> + sdma: dma-controller@4800 {
> + compatible = "ti,sdma-omap4"
> + reg = <0x4800 0x1000>;
> + interrupts = <12>;
> + #dma-cells = <1>;
> + };
> +
> +
> +
> +* DMA client
> +
> +Client drivers should specify the DMA request property using a phandle to
> +the controller. If needed, the DMA request identity on that controller is
> then
> +added followed by optional request specifications.
> +
> +Required property:
> +- dma-request: List of phandle + dma-request + request specifications,
> + one group per request "line".
> +Optional property:
> +- dma-request-names: list of strings in the same order as the dma-request
> + in the dma-request property.
> +
> +
> +Example:
> +
> + i2c1: i2c@1 {
> + ...
> + dma-request = <&sdma 2 &sdma 3>;
> + dma-request-names = "tx", "rx";
> + ...
> + };
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 268163d..7d1f06b 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -90,4 +90,9 @@ config OF_PCI_IRQ
> help
> OpenFirmware PCI IRQ routing helpers
>
> +config OF_DMA
> + def_bool y
> + help
> + Device Tree DMA routing helpers
Not really any point in having this config symbol at all if it is
always enabled.
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index a73f5a5..6eb50c6 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_SELFTEST) += selftest.o
> obj-$(CONFIG_OF_MDIO)+= of_mdio.o
> obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> +obj-$(CONFIG_OF_DMA) += dma.o
> diff --git a/drivers/of/dma.c b/drivers/of/dma.c
> new file mode 100644
> index 000..3315844
> --- /dev/null
> +++ b/drivers/of/dma.c
> @@ -0,0 +1,260 @@
> +/*
> + * Device tree helpers for DMA request / controller
> + *
> + * Based on of_gpio.c
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static LIST_HEAD(of_dma_list);
> +
> +struct of_dma {
> + struct list_head of_dma_controllers;
> + struct device_node *of_node;
> + int of_dma_n_cells;
> + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
> +};
This _xlate is nearly useless as a generic API. It solves the problem for
the specific case where the driver is hard-coded to know which DMA engine
to talk to, but sinc
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding
wrote:
> * Arnd Bergmann wrote:
> > On Thursday 15 March 2012, Nicolas Ferre wrote:
> [...]
> > > + i2c1: i2c@1 {
> > > + ...
> > > + dma-request = <&sdma 2 &sdma 3>;
> > > + dma-request-names = "tx", "rx";
> > > + ...
> > > + };
> >
> > This is slightly different from how the proposed pwm binding works that
> > Thierry is working on, which uses an arbitrary property name instead of
> > requiring the use of a specific property but then allowing to give names
> > in another property.
> >
> > I don't care much which way it's done, but please try to agree on one
> > approach that is used for both.
> >
> > The one you have here is already used by reg and irq, while the other
> > one is used in gpio.
>
> I think we can just use pwm as the fixed property name. Or alternatively do
> something along the lines of the regulator bindings, where we use "-pwm" as
> the suffix for specifying PWM devices. For instance if a named PWM is
> requested, the OF support code would look for a -pwm property, while
> requesting an unnamed PWM would simply look at the pwm property.
>
> When it comes to the labelling of PWM devices, I don't think both variants
> are exclusive. Currently the PWM framework uses name of the user OF device
> node for the PWM label. That is, if I have the following in the DTS:
>
> pwm {
> ..
> };
>
> backlight {
> compatible = "pwm-backlight";
> pwm = <&pwm 0 500>;
> ...
> };
>
> Then the PWM will be labelled "backlight":
>
> $ cat cat /sys/kernel/debug/pwm
> platform/tegra-pwm, 4 PWM devices
> pwm-0 (backlight ): requested enabled
> pwm-1 ((null) ):
> pwm-2 ((null) ):
> pwm-3 ((null) ):
>
> So if we decide to explicitly allow specifying names, then we can always add
> a pwm-names property (or -pwm-names respectively) to use as label and
> fallback to the user OF device node name if that property is not present.
After implementing both schemes (ie. interrupts+interrupt-names && [*-]gpios)
I definitely prefer the fixed property name plus a separate names property.
It is easier to use common code with that scheme, and easier to statically
check for correctness.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 03/16/2012 02:28 PM, Cousson, Benoit :
> On 3/16/2012 1:04 PM, Arnd Bergmann wrote:
>> On Friday 16 March 2012, Cousson, Benoit wrote:
>>> And it seems that other ARM SoCs are using it for the same purpose.
>>> There are at least 230+ IORESOURCE_DMA instances in the kernel today.
>>
>> These tend to be the ones that don't use dmaengine but either the
>> ISA dma api or a platform specific variant of that, right?
>>
>> Also, I think that most of those definitions are for the same few
>> devices. The number that I see is much lower:
>>
>> $ git grep -l IORESOURCE_DMA drivers/ sound/ | wc -l
>> 51
>
> Gosh, good point... I've just done a dumb grep, but most of them are in
> the device creation inside arch code:-(
>
> Assuming OMAP driver does contains omap in the name, the result is
> indeed way smaller.
>
> git grep -l IORESOURCE_DMA drivers/ sound/ | grep omap
>
> drivers/crypto/omap-aes.c
> drivers/crypto/omap-sham.c
> drivers/spi/spi-omap2-mcspi.c
> drivers/tty/serial/omap-serial.c
> sound/soc/omap/omap-dmic.c
> sound/soc/omap/omap-hdmi.c
>
> We do have 127 DMA requests and only 6 drivers are using them, that's a
> pity...
>
>> Out of those, a quite number are mips or blackfin or xtensa based, or are
>> for legacy ISA devices, which leaves 29 drivers for ARM.
>>
>>> For the moment it is still used in a lot of places, and without any
>>> other better API yet, it is still useful. As written it is there only
>>> for simple single DMA controller case.
>>>
>>> By maintaining that IORESOURCE_DMA for the moment we can adapt some
>>> driver to DT without having to change the way they are retrieving
>>> information. By using IORESOURCE_IRQ and IORESOURCE_MEM, we had the same
>>> advantage.
>>
>> The main difference to IORESOURCE_IRQ and IORESOURCE_MEM that I see
>> is that those are going to start for any forseeable time and are actually
>> helpful in a lot of ways. We are not going to remove the single number
>> space for interrupts in the next few years. For DMA, the dmaengine API
>> has already moved away from the flat number space of the ISA API.
>>
>>> Otherwise how are we supposed to get the DMA channel for non-DT boot
>>> until we have migrated everything to DT? A bunch of ARM SoC are using
>>> IORESOURCE_DMA for the same purpose.
>>>
>>> The goal here is really to maintain that during the transition phase
>>> only. As soon as the full DT support is there, we can switch to the
>>> of_ API.
>>>
>>> Ideally, we should define and use a generic API non dependent of DT.
>>> AFAIK, that does not exist so far.
>>> And since most drivers are not using dmaengine, we do not even have a
>>> common DMA fmwk to define an API on top.
>>>
>>> I fully agree that this IORESOURCE_DMA is not scalable for multiple
>>> controller, ugly, and must be avoided like the plague.
>>> But what other options do we have during the transition?
>>
>> We could use the same binding for the nonstandard controllers, but
>> keep the dma channel number lookup separate for those, and let them
>> call of_get_dma_request directly.
>>
>> Since there are not too many drivers using those controllers with
>> dma resources today, it's fairly easy to go through those as we write
>> the driver bindings and just do
>>
>> err = of_get_dma_request(pdev->dev.of_node, 0,&dma);
>> if (err) {
>> struct resource *r = platform_get_resource(pdev,
>> IORESOURCE_DMA, 0);
>> if (r)
>> dma = r->start;
>> }
>>
>> For the drivers that we convert to DT before we convert them to
>> dmaengine,
>> and not do anything if we convert them to dmaengine first.
>
> Considering the small amount of OMAP drivers really using
> IORESOURCE_DMA, I guess this is fair enough.
>
> Bottom-line, I do not have any more issue removing this of_dma_to_resource.
>
>
> Nico,
> Is that OK for you to repost the patch without this function?
Yes sure, I will repost a V2 soon.
I also try to have an integration in DT selftest.c:
- that will show some possibilities of the API
- that will give some examples of usage (1 or 2 xlate() functions)
- that made me review my code and find a weakness
- that will allow to monitor non-regression (obviously)
=> WIP...
Bye,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 3/16/2012 1:04 PM, Arnd Bergmann wrote:
On Friday 16 March 2012, Cousson, Benoit wrote:
And it seems that other ARM SoCs are using it for the same purpose.
There are at least 230+ IORESOURCE_DMA instances in the kernel today.
These tend to be the ones that don't use dmaengine but either the
ISA dma api or a platform specific variant of that, right?
Also, I think that most of those definitions are for the same few
devices. The number that I see is much lower:
$ git grep -l IORESOURCE_DMA drivers/ sound/ | wc -l
51
Gosh, good point... I've just done a dumb grep, but most of them are in
the device creation inside arch code:-(
Assuming OMAP driver does contains omap in the name, the result is
indeed way smaller.
git grep -l IORESOURCE_DMA drivers/ sound/ | grep omap
drivers/crypto/omap-aes.c
drivers/crypto/omap-sham.c
drivers/spi/spi-omap2-mcspi.c
drivers/tty/serial/omap-serial.c
sound/soc/omap/omap-dmic.c
sound/soc/omap/omap-hdmi.c
We do have 127 DMA requests and only 6 drivers are using them, that's a
pity...
Out of those, a quite number are mips or blackfin or xtensa based, or are
for legacy ISA devices, which leaves 29 drivers for ARM.
For the moment it is still used in a lot of places, and without any
other better API yet, it is still useful. As written it is there only
for simple single DMA controller case.
By maintaining that IORESOURCE_DMA for the moment we can adapt some
driver to DT without having to change the way they are retrieving
information. By using IORESOURCE_IRQ and IORESOURCE_MEM, we had the same
advantage.
The main difference to IORESOURCE_IRQ and IORESOURCE_MEM that I see
is that those are going to start for any forseeable time and are actually
helpful in a lot of ways. We are not going to remove the single number
space for interrupts in the next few years. For DMA, the dmaengine API
has already moved away from the flat number space of the ISA API.
Otherwise how are we supposed to get the DMA channel for non-DT boot
until we have migrated everything to DT? A bunch of ARM SoC are using
IORESOURCE_DMA for the same purpose.
The goal here is really to maintain that during the transition phase
only. As soon as the full DT support is there, we can switch to the of_ API.
Ideally, we should define and use a generic API non dependent of DT.
AFAIK, that does not exist so far.
And since most drivers are not using dmaengine, we do not even have a
common DMA fmwk to define an API on top.
I fully agree that this IORESOURCE_DMA is not scalable for multiple
controller, ugly, and must be avoided like the plague.
But what other options do we have during the transition?
We could use the same binding for the nonstandard controllers, but
keep the dma channel number lookup separate for those, and let them
call of_get_dma_request directly.
Since there are not too many drivers using those controllers with
dma resources today, it's fairly easy to go through those as we write
the driver bindings and just do
err = of_get_dma_request(pdev->dev.of_node, 0,&dma);
if (err) {
struct resource *r = platform_get_resource(pdev,
IORESOURCE_DMA, 0);
if (r)
dma = r->start;
}
For the drivers that we convert to DT before we convert them to dmaengine,
and not do anything if we convert them to dmaengine first.
Considering the small amount of OMAP drivers really using
IORESOURCE_DMA, I guess this is fair enough.
Bottom-line, I do not have any more issue removing this of_dma_to_resource.
Nico,
Is that OK for you to repost the patch without this function?
Thanks,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Friday 16 March 2012, Cousson, Benoit wrote:
> And it seems that other ARM SoCs are using it for the same purpose.
> There are at least 230+ IORESOURCE_DMA instances in the kernel today.
These tend to be the ones that don't use dmaengine but either the
ISA dma api or a platform specific variant of that, right?
Also, I think that most of those definitions are for the same few
devices. The number that I see is much lower:
$ git grep -l IORESOURCE_DMA drivers/ sound/ | wc -l
51
Out of those, a quite number are mips or blackfin or xtensa based, or are
for legacy ISA devices, which leaves 29 drivers for ARM.
> For the moment it is still used in a lot of places, and without any
> other better API yet, it is still useful. As written it is there only
> for simple single DMA controller case.
>
> By maintaining that IORESOURCE_DMA for the moment we can adapt some
> driver to DT without having to change the way they are retrieving
> information. By using IORESOURCE_IRQ and IORESOURCE_MEM, we had the same
> advantage.
The main difference to IORESOURCE_IRQ and IORESOURCE_MEM that I see
is that those are going to start for any forseeable time and are actually
helpful in a lot of ways. We are not going to remove the single number
space for interrupts in the next few years. For DMA, the dmaengine API
has already moved away from the flat number space of the ISA API.
> Otherwise how are we supposed to get the DMA channel for non-DT boot
> until we have migrated everything to DT? A bunch of ARM SoC are using
> IORESOURCE_DMA for the same purpose.
>
> The goal here is really to maintain that during the transition phase
> only. As soon as the full DT support is there, we can switch to the of_ API.
>
> Ideally, we should define and use a generic API non dependent of DT.
> AFAIK, that does not exist so far.
> And since most drivers are not using dmaengine, we do not even have a
> common DMA fmwk to define an API on top.
>
> I fully agree that this IORESOURCE_DMA is not scalable for multiple
> controller, ugly, and must be avoided like the plague.
> But what other options do we have during the transition?
We could use the same binding for the nonstandard controllers, but
keep the dma channel number lookup separate for those, and let them
call of_get_dma_request directly.
Since there are not too many drivers using those controllers with
dma resources today, it's fairly easy to go through those as we write
the driver bindings and just do
err = of_get_dma_request(pdev->dev.of_node, 0, &dma);
if (err) {
struct resource *r = platform_get_resource(pdev,
IORESOURCE_DMA, 0);
if (r)
dma = r->start;
}
For the drivers that we convert to DT before we convert them to dmaengine,
and not do anything if we convert them to dmaengine first.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 3/16/2012 11:03 AM, Arnd Bergmann wrote:
On Thursday 15 March 2012, Nicolas Ferre wrote:
For legacy reason another API will export the DMA request number into a
Linux resource of type IORESOURCE_DMA.
Can you explain which legacy scenarios you think this is going to help with?
We do have a bunch of drivers that are using IORESOURCE_DMA today to
retrieve the DMA request. Since we do have an unique DMA controller it
was easy and convenient.
And it seems that other ARM SoCs are using it for the same purpose.
There are at least 230+ IORESOURCE_DMA instances in the kernel today.
+/**
+ * of_dma_to_resource - Decode a node's DMA and return it as a resource
+ * @dev: pointer to device tree node
+ * @index: zero-based index of the DMA request
+ * @r: pointer to resource structure to return result into.
+ *
+ * Using a resource to export a DMA request number to a driver should
+ * be used only for legacy purpose and on system when only one DMA controller
+ * is present.
+ * The proper and only scalable way is to use the native of_get_dma_request API
+ * in order retrieve both the DMA controller device node and the DMA request
+ * line for that controller.
+ */
+int of_dma_to_resource(struct device_node *dev, int index, struct resource *r)
+{
I think a better way to discourage the use of IORESOURCE_DMA is to not provide
this function at all, especially given that it's not really well-defined what
it will do when you have more than one cell for the dma channel identifier or
when you have multiple DMA engines, so any driver relying on it is inherently
nonportable.
For the moment it is still used in a lot of places, and without any
other better API yet, it is still useful. As written it is there only
for simple single DMA controller case.
By maintaining that IORESOURCE_DMA for the moment we can adapt some
driver to DT without having to change the way they are retrieving
information. By using IORESOURCE_IRQ and IORESOURCE_MEM, we had the same
advantage.
Otherwise how are we supposed to get the DMA channel for non-DT boot
until we have migrated everything to DT? A bunch of ARM SoC are using
IORESOURCE_DMA for the same purpose.
The goal here is really to maintain that during the transition phase
only. As soon as the full DT support is there, we can switch to the of_ API.
Ideally, we should define and use a generic API non dependent of DT.
AFAIK, that does not exist so far.
And since most drivers are not using dmaengine, we do not even have a
common DMA fmwk to define an API on top.
I fully agree that this IORESOURCE_DMA is not scalable for multiple
controller, ugly, and must be avoided like the plague.
But what other options do we have during the transition?
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thursday 15 March 2012, Nicolas Ferre wrote:
> For legacy reason another API will export the DMA request number into a
> Linux resource of type IORESOURCE_DMA.
Can you explain which legacy scenarios you think this is going to help with?
> +/**
> + * of_dma_to_resource - Decode a node's DMA and return it as a resource
> + * @dev: pointer to device tree node
> + * @index: zero-based index of the DMA request
> + * @r: pointer to resource structure to return result into.
> + *
> + * Using a resource to export a DMA request number to a driver should
> + * be used only for legacy purpose and on system when only one DMA controller
> + * is present.
> + * The proper and only scalable way is to use the native of_get_dma_request
> API
> + * in order retrieve both the DMA controller device node and the DMA request
> + * line for that controller.
> + */
> +int of_dma_to_resource(struct device_node *dev, int index, struct resource
> *r)
> +{
I think a better way to discourage the use of IORESOURCE_DMA is to not provide
this function at all, especially given that it's not really well-defined what
it will do when you have more than one cell for the dma channel identifier or
when you have multiple DMA engines, so any driver relying on it is inherently
nonportable.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> Thank you both for missing my point.
>
> #define OMAP24XX_DMA_SHA1MD5_RX 13 /* S_DMA_12 */
> #define OMAP34XX_DMA_SHA2MD5_RX 13 /* S_DMA_12 */
>
> #define OMAP242X_DMA_EXT_DMAREQ214 /* S_DMA_13 */
> #define OMAP243X_DMA_EXT_DMAREQ314 /* S_DMA_13 */
>
> #define OMAP242X_DMA_EXT_DMAREQ315 /* S_DMA_14 */
> #define OMAP24XX_DMA_SPI3_TX0 15 /* S_DMA_14 */
>
> #define OMAP242X_DMA_EXT_DMAREQ416 /* S_DMA_15 */
> #define OMAP24XX_DMA_SPI3_RX0 16 /* S_DMA_15 */
>
> #define OMAP242X_DMA_EAC_BT_DL_RD 25 /* S_DMA_24 */
> #define OMAP243X_DMA_EXT_DMAREQ425 /* S_DMA_24 */
> #define OMAP34XX_DMA_I2C3_TX25 /* S_DMA_24 */
>
> Notice the overlap between the different SoCs for the same number on the
> same DMA controller.
>
> This shouldn't cause problems when all users are within the SoC specific
> file, but those EXT ones would probably be platform specific, and so you
> immediately have a dependence between the platform and the SoC there.
>
> That dependence can be completely eliminated by other matching schemes
> which are supportable via the DMA engine API.
Ok, so I guess you are worried about the case where you have one .dtsi
include file for the soc and another .dtsi file for the platform, and
you want to generically encode e.g. DMA_EXT_DMAREQ3 in the platform file
so you don't need to write a new .dts file for each combination of that
platform wiht a different soc. Does that describe where you see the
issue?
If so, I would recommend not using a flat numbering scheme for omap,
but have something slightly more complex like a lot of the other
dmagenine drivers do. This binding does not make any assumption about
the meaning of the values, so you can do e.g. three cells for
each channel with a definition like this:
Cell 1: channel class
Possible values are:
0 -- raw channel number
1 -- external channel
2 -- spi
3 -- i2c
4 -- mmc
to be extended when necessary
Cell 2: instance of this channel
When Cell 1 is zero, this is just the channel number local to the controller,
for other values it defines the instance, e.g. ext0, ext1, ext2, ...
Cell 3: DMA direction
The following values are defined:
0 -- invalid
1 -- read
2 -- write
3 -- read/write
This specific binding for the omap dma would let you put either a simple channel
into the device tree, or have a high-level description in there. On an OMAP243X,
these two would have the same meaning and a user could put either one in there:
dma-request = <&dmaengine 0 14 1 /* physical channels 14 (read) */
&dmaengine 0 15 2>; /* and 15 (write) */
dma-request = <&dmaengine 1 3 1 /* external DMA 3 (read) and 4
(write) */
&dmaengine 1 4 2>;
The cell layout above is just a made-up example, you can basically put
everything
in there that you would put into the arguments to the dma match function. I
believe
this is not limited to numbers but can also include phandles and strings. The
mapping
from dma classes to physical channels could either be hardcoded in the source
for
the dmaengine driver, or get passed in properties of the dmaengine's
device_node.
A completely different way to get around the same problem is to define a tree
of virtual dmaengine device nodes, still within the same binding:
dmaengine-phys: dmaengine {
compatible = "ti,omap2430-dmaengine", "ti,omap2-dmaengine";
reg = <0x4a056000 0x1000>;
#address-cells = <1>; /* physical dma channel number space */
#size-cells = <0>;
#dma-cells = 1;
dmaengine-ext: dmaengine@2 {
ranges = <0 2>, <1 3>, <2 7>, <3 14>, <4 25>,
<5 25> <6 64>;
#dma-cells = 1;
};
};
This way, a device driver could either refer to the physical dmaengine device
and pass
a physical number like
dma-requests = <&dmaengine-phys 14>;
or to the same effect refer to a child of node of that engine passing a
local number like
dma-requests = <&dmaengine-ext 3>;
The dmaengine driver in this case can simply use of_translate_address() to
get from channel 3 to 14 using the ranges in the child dmaengine device node.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, 15 Mar 2012, Russell King - ARM Linux wrote: > I really don't like these behind-the-scenes discussions which never then > get followed up in public, and people then start quoting what was "agreed" > as that's the way things must be done. > > It's a bit like folk at the recent Linaro Connect apparantly discussing > my machine registry and deciding that it should be shut down. No one > has yet talked to me about that or even done the curtesy of asking me > what my view is. Let me catch the ball in flight here as the responsible folk was me. For the transition to DT to be effective for new boards on ARM, I suggested that the machine registry would need to be closed at this point i.e. no new additions to it. As far as I remember, Vincent volunteered to talk to you about it, which he apparently did within the same hour. That's all there was to it. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, Mar 15, 2012 at 10:39:12PM +0100, Cousson, Benoit wrote: > On 3/15/2012 9:41 PM, Arnd Bergmann wrote: >> On Thursday 15 March 2012, Russell King - ARM Linux wrote: >>> On Thu, Mar 15, 2012 at 05:30:49PM +0100, Cousson, Benoit wrote: This was done like IRQ on purpose, because an Interrupt ReQuest line and a DMA Request line are really similar for the HW point of view at IP level. >>> >>> I'm not sure about that at all levels. Sure, at hardware level they're >>> the same, but I think the flat numeric namespace for IRQs has been >>> proven to be a problem when there's multiple IRQ controllers in the >>> system. >> >> In the DT bindings, both IRQ and the suggested DMA are not flat number >> spaces, but instead can be of arbitrarly length defined by the controller. >> >>> As far as I'm concerned for DMA stuff, there is currently no real solution >>> for a DT representation; TI have asked me to take over the conversion of >>> OMAP DMA support to the DMA engine API, and I'm not yet convinced that >>> the existing numbering system is the right solution - especially as >>> there's several overlapping numberspaces for OMAP DMA numbers which >>> are SoC specific. >> >> The numbers definitely need to become local to each of the controllers, but >> that is the case pretty much automatically using the proposed binding, >> because each dma request identifier starts with the phandle of the >> controller. > > Indeed, and in the case of the OMAP SDMA controller, it can handle up to > 127 DMA request lines numbered from 0 to 126... So a local number seems > to be a good representation... especially for a number. I'm not sure to > understand the issue with this binding. > > And AFAIK, there is the only one general purpose DMA controller in OMAP > so far. The other ones are private to some IPs like MMC or USB, so they > do not need necessarily need any DT representation. > But anyway, since the controller phandle is mandatory, it will be able > to handle even several instances of this DMA controller without any > issue. Thank you both for missing my point. #define OMAP24XX_DMA_SHA1MD5_RX 13 /* S_DMA_12 */ #define OMAP34XX_DMA_SHA2MD5_RX 13 /* S_DMA_12 */ #define OMAP242X_DMA_EXT_DMAREQ214 /* S_DMA_13 */ #define OMAP243X_DMA_EXT_DMAREQ314 /* S_DMA_13 */ #define OMAP242X_DMA_EXT_DMAREQ315 /* S_DMA_14 */ #define OMAP24XX_DMA_SPI3_TX0 15 /* S_DMA_14 */ #define OMAP242X_DMA_EXT_DMAREQ416 /* S_DMA_15 */ #define OMAP24XX_DMA_SPI3_RX0 16 /* S_DMA_15 */ #define OMAP242X_DMA_EAC_BT_DL_RD 25 /* S_DMA_24 */ #define OMAP243X_DMA_EXT_DMAREQ425 /* S_DMA_24 */ #define OMAP34XX_DMA_I2C3_TX25 /* S_DMA_24 */ Notice the overlap between the different SoCs for the same number on the same DMA controller. This shouldn't cause problems when all users are within the SoC specific file, but those EXT ones would probably be platform specific, and so you immediately have a dependence between the platform and the SoC there. That dependence can be completely eliminated by other matching schemes which are supportable via the DMA engine API. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 3/15/2012 9:41 PM, Arnd Bergmann wrote: On Thursday 15 March 2012, Russell King - ARM Linux wrote: On Thu, Mar 15, 2012 at 05:30:49PM +0100, Cousson, Benoit wrote: This was done like IRQ on purpose, because an Interrupt ReQuest line and a DMA Request line are really similar for the HW point of view at IP level. I'm not sure about that at all levels. Sure, at hardware level they're the same, but I think the flat numeric namespace for IRQs has been proven to be a problem when there's multiple IRQ controllers in the system. In the DT bindings, both IRQ and the suggested DMA are not flat number spaces, but instead can be of arbitrarly length defined by the controller. As far as I'm concerned for DMA stuff, there is currently no real solution for a DT representation; TI have asked me to take over the conversion of OMAP DMA support to the DMA engine API, and I'm not yet convinced that the existing numbering system is the right solution - especially as there's several overlapping numberspaces for OMAP DMA numbers which are SoC specific. The numbers definitely need to become local to each of the controllers, but that is the case pretty much automatically using the proposed binding, because each dma request identifier starts with the phandle of the controller. Indeed, and in the case of the OMAP SDMA controller, it can handle up to 127 DMA request lines numbered from 0 to 126... So a local number seems to be a good representation... especially for a number. I'm not sure to understand the issue with this binding. And AFAIK, there is the only one general purpose DMA controller in OMAP so far. The other ones are private to some IPs like MMC or USB, so they do not need necessarily need any DT representation. But anyway, since the controller phandle is mandatory, it will be able to handle even several instances of this DMA controller without any issue. Regards Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thursday 15 March 2012, Russell King - ARM Linux wrote: > On Thu, Mar 15, 2012 at 05:30:49PM +0100, Cousson, Benoit wrote: > > This was done like IRQ on purpose, because an Interrupt ReQuest line and > > a DMA Request line are really similar for the HW point of view at IP > > level. > > I'm not sure about that at all levels. Sure, at hardware level they're > the same, but I think the flat numeric namespace for IRQs has been > proven to be a problem when there's multiple IRQ controllers in the > system. In the DT bindings, both IRQ and the suggested DMA are not flat number spaces, but instead can be of arbitrarly length defined by the controller. > As far as I'm concerned for DMA stuff, there is currently no real solution > for a DT representation; TI have asked me to take over the conversion of > OMAP DMA support to the DMA engine API, and I'm not yet convinced that > the existing numbering system is the right solution - especially as > there's several overlapping numberspaces for OMAP DMA numbers which > are SoC specific. The numbers definitely need to become local to each of the controllers, but that is the case pretty much automatically using the proposed binding, because each dma request identifier starts with the phandle of the controller. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, Mar 15, 2012 at 05:30:49PM +0100, Cousson, Benoit wrote: > This was done like IRQ on purpose, because an Interrupt ReQuest line and > a DMA Request line are really similar for the HW point of view at IP > level. I'm not sure about that at all levels. Sure, at hardware level they're the same, but I think the flat numeric namespace for IRQs has been proven to be a problem when there's multiple IRQ controllers in the system. > I'm not sure what Thierry have done for pwm, but I thing that having the > same scheme for reg, irq and dma was what we agreed with Grant during > Plumbers. I really don't like these behind-the-scenes discussions which never then get followed up in public, and people then start quoting what was "agreed" as that's the way things must be done. It's a bit like folk at the recent Linaro Connect apparantly discussing my machine registry and deciding that it should be shut down. No one has yet talked to me about that or even done the curtesy of asking me what my view is. As far as I'm concerned for DMA stuff, there is currently no real solution for a DT representation; TI have asked me to take over the conversion of OMAP DMA support to the DMA engine API, and I'm not yet convinced that the existing numbering system is the right solution - especially as there's several overlapping numberspaces for OMAP DMA numbers which are SoC specific. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On 3/15/2012 10:22 AM, Arnd Bergmann wrote:
On Thursday 15 March 2012, Nicolas Ferre wrote:
Add some basic helpers to retrieve a DMA controller device_node and the
DMA request specifications. By making DMA controllers register a generic
translation function, we allow the management of any type of DMA requests
specification.
The void * output of an of_dma_xlate() function that will be implemented
by the DMA controller can carry any type of "dma-request" argument.
The DMA client will search its associated DMA controller in the list and
call the registered of_dam_xlate() function to retrieve the request values.
One simple xlate function is provided for the "single number" type of
request binding.
This implementation is independent from dmaengine so it can also be used
by legacy drivers.
For legacy reason another API will export the DMA request number into a
Linux resource of type IORESOURCE_DMA.
This looks very good. I missed the first version of this patch, but was
thinking of very similar bindings.
+Client drivers should specify the DMA request property using a phandle to
+the controller. If needed, the DMA request identity on that controller is then
+added followed by optional request specifications.
+
+Required property:
+- dma-request: List of phandle + dma-request + request specifications,
+ one group per request "line".
+Optional property:
+- dma-request-names: list of strings in the same order as the dma-request
+ in the dma-request property.
+
+
+Example:
+
+ i2c1: i2c@1 {
+ ...
+ dma-request =<&sdma 2&sdma 3>;
+ dma-request-names = "tx", "rx";
+ ...
+ };
This is slightly different from how the proposed pwm binding works that
Thierry is working on, which uses an arbitrary property name instead of
requiring the use of a specific property but then allowing to give names
in another property.
I don't care much which way it's done, but please try to agree on one
approach that is used for both.
The one you have here is already used by reg and irq, while the other
one is used in gpio.
This was done like IRQ on purpose, because an Interrupt ReQuest line and
a DMA Request line are really similar for the HW point of view at IP level.
I'm not sure what Thierry have done for pwm, but I thing that having the
same scheme for reg, irq and dma was what we agreed with Grant during
Plumbers.
GPIO scheme will be probably good enough as well, but the idea was to be
consistent in the binding for similar information.
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
* Arnd Bergmann wrote:
> On Thursday 15 March 2012, Nicolas Ferre wrote:
[...]
> > + i2c1: i2c@1 {
> > + ...
> > + dma-request = <&sdma 2 &sdma 3>;
> > + dma-request-names = "tx", "rx";
> > + ...
> > + };
>
> This is slightly different from how the proposed pwm binding works that
> Thierry is working on, which uses an arbitrary property name instead of
> requiring the use of a specific property but then allowing to give names
> in another property.
>
> I don't care much which way it's done, but please try to agree on one
> approach that is used for both.
>
> The one you have here is already used by reg and irq, while the other
> one is used in gpio.
I think we can just use pwm as the fixed property name. Or alternatively do
something along the lines of the regulator bindings, where we use "-pwm" as
the suffix for specifying PWM devices. For instance if a named PWM is
requested, the OF support code would look for a -pwm property, while
requesting an unnamed PWM would simply look at the pwm property.
When it comes to the labelling of PWM devices, I don't think both variants
are exclusive. Currently the PWM framework uses name of the user OF device
node for the PWM label. That is, if I have the following in the DTS:
pwm {
...
};
backlight {
compatible = "pwm-backlight";
pwm = <&pwm 0 500>;
...
};
Then the PWM will be labelled "backlight":
$ cat cat /sys/kernel/debug/pwm
platform/tegra-pwm, 4 PWM devices
pwm-0 (backlight ): requested enabled
pwm-1 ((null) ):
pwm-2 ((null) ):
pwm-3 ((null) ):
So if we decide to explicitly allow specifying names, then we can always add
a pwm-names property (or -pwm-names respectively) to use as label and
fallback to the user OF device node name if that property is not present.
Thierry
pgpWjFSQszFmu.pgp
Description: PGP signature
Re: [PATCH] of: Add generic device tree DMA helpers
On Thursday 15 March 2012, Russell King - ARM Linux wrote: > On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: > > On Thursday 15 March 2012, Nicolas Ferre wrote: > > > Add some basic helpers to retrieve a DMA controller device_node and the > > > DMA request specifications. By making DMA controllers register a generic > > > translation function, we allow the management of any type of DMA requests > > > specification. > > > The void * output of an of_dma_xlate() function that will be implemented > > > by the DMA controller can carry any type of "dma-request" argument. > > > > > > The DMA client will search its associated DMA controller in the list and > > > call the registered of_dam_xlate() function to retrieve the request > > > values. > > > > > > One simple xlate function is provided for the "single number" type of > > > request binding. > > > > > > This implementation is independent from dmaengine so it can also be used > > > by legacy drivers. > > > > > > For legacy reason another API will export the DMA request number into a > > > Linux resource of type IORESOURCE_DMA. > > > > This looks very good. I missed the first version of this patch, but was > > thinking of very similar bindings. > > There's one issue which is concerning me - when we switch OMAP to use > DMA engine, it won't use numeric stuff anymore but the DMA engine way > of requesting a channel (match function + data). > > How does that fit into this scheme? I haven't looked at the omap dma driver much, but if it just uses a channel number, then I'd assume the data would be a single u32 cell with that number, and the match function would be trivial. Is that what you are asking about or am I missing the point? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: > On Thursday 15 March 2012, Nicolas Ferre wrote: > > Add some basic helpers to retrieve a DMA controller device_node and the > > DMA request specifications. By making DMA controllers register a generic > > translation function, we allow the management of any type of DMA requests > > specification. > > The void * output of an of_dma_xlate() function that will be implemented > > by the DMA controller can carry any type of "dma-request" argument. > > > > The DMA client will search its associated DMA controller in the list and > > call the registered of_dam_xlate() function to retrieve the request values. > > > > One simple xlate function is provided for the "single number" type of > > request binding. > > > > This implementation is independent from dmaengine so it can also be used > > by legacy drivers. > > > > For legacy reason another API will export the DMA request number into a > > Linux resource of type IORESOURCE_DMA. > > This looks very good. I missed the first version of this patch, but was > thinking of very similar bindings. There's one issue which is concerning me - when we switch OMAP to use DMA engine, it won't use numeric stuff anymore but the DMA engine way of requesting a channel (match function + data). How does that fit into this scheme? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Thursday 15 March 2012, Nicolas Ferre wrote:
> Add some basic helpers to retrieve a DMA controller device_node and the
> DMA request specifications. By making DMA controllers register a generic
> translation function, we allow the management of any type of DMA requests
> specification.
> The void * output of an of_dma_xlate() function that will be implemented
> by the DMA controller can carry any type of "dma-request" argument.
>
> The DMA client will search its associated DMA controller in the list and
> call the registered of_dam_xlate() function to retrieve the request values.
>
> One simple xlate function is provided for the "single number" type of
> request binding.
>
> This implementation is independent from dmaengine so it can also be used
> by legacy drivers.
>
> For legacy reason another API will export the DMA request number into a
> Linux resource of type IORESOURCE_DMA.
This looks very good. I missed the first version of this patch, but was
thinking of very similar bindings.
> +Client drivers should specify the DMA request property using a phandle to
> +the controller. If needed, the DMA request identity on that controller is
> then
> +added followed by optional request specifications.
> +
> +Required property:
> +- dma-request: List of phandle + dma-request + request specifications,
> + one group per request "line".
> +Optional property:
> +- dma-request-names: list of strings in the same order as the dma-request
> + in the dma-request property.
> +
> +
> +Example:
> +
> + i2c1: i2c@1 {
> + ...
> + dma-request = <&sdma 2 &sdma 3>;
> + dma-request-names = "tx", "rx";
> + ...
> + };
This is slightly different from how the proposed pwm binding works that
Thierry is working on, which uses an arbitrary property name instead of
requiring the use of a specific property but then allowing to give names
in another property.
I don't care much which way it's done, but please try to agree on one
approach that is used for both.
The one you have here is already used by reg and irq, while the other
one is used in gpio.
> +/**
> + * of_get_dma_request() - Get the associated DMA request data
> + * @np: device node to get DMA request from
> + * @index: index of the DMA request
> + * @out_data:a output that can be filled in by the of_dma_xlate()
> function
> + *
> + * Returns return value of of_dma_xlate() and fills out_data (if provided).
> + * On error returns the appropriate errno value.
> + */
I would suggest provinding a helper function around this one that directly
calls __dma_request_channel(). AFAICT, you should be able to let the dma
controller provide a generic filter function in struct of_dma that the
driver can use by default.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
