[PATCH 4/6] DMA: PL330: Add device tree support
Rob Herring
robherring2 at gmail.com
Tue Aug 30 23:19:58 EST 2011
Thomas,
On 08/30/2011 07:18 AM, Thomas Abraham wrote:
> Hi Rob,
>
> On 26 August 2011 18:46, Rob Herring <robherring2 at gmail.com
> <mailto:robherring2 at gmail.com>> wrote:
>
> Thomas,
>
> On 08/26/2011 03:40 AM, Thomas Abraham wrote:
> > For PL330 dma controllers instantiated from device tree, the channel
> > lookup is based on phandle of the dma controller and dma request id
> > specified by the client node. During probe, the private data of each
> > channel of the controller is set to point to the device node of the
> > dma controller. The 'chan_id' of the each channel is used as the
> > dma request id.
> >
> > Client driver requesting dma channels specify the phandle of the
> > dma controller and the request id. The pl330 filter function
> > converts the phandle to the device node pointer and matches that
> > with channel's private data. If a match is found, the request id
> > from the client node and the 'chan_id' of the channel is matched.
> > A channel is found if both the values match.
> >
> > Signed-off-by: Thomas Abraham <thomas.abraham at linaro.org
> <mailto:thomas.abraham at linaro.org>>
> > ---
> > .../devicetree/bindings/dma/arm-pl330.txt | 42
> +++++++++++++
> > drivers/dma/pl330.c | 63
> +++++++++++++++++++-
> > 2 files changed, 103 insertions(+), 2 deletions(-)
> > create mode 100644
> Documentation/devicetree/bindings/dma/arm-pl330.txt
> >
> > diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt
> b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> > new file mode 100644
> > index 0000000..46a8307
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> > @@ -0,0 +1,42 @@
> > +* ARM PrimeCell PL330 DMA Controller
> > +
> > +The ARM PrimeCell PL330 DMA controller can move blocks of memory
> contents
> > +between memory and peripherals or memory to memory.
> > +
> > +Required properties:
> > + - compatible: should one or more of the following
> > + - arm,pl330-pdma - For controllers that support mem-to-dev
> and dev-to-mem
> > + transfers.
> > + - arm,pl330-mdma - For controllers that support mem-to-mem
> transfers only.
>
> And if they support both? I would think all controllers can support
> mem-to-mem. If so, the distinction can be made with the number of
> requests.
>
>
> If a controller supports both types of transfer, the device node should
> not claim compatibility for "arm,pl330-pdma" or "arm,pl330-mdma".
> Compatible should be "arm,primecell".
No, every Primecell peripheral has "arm,primecell", so it is not
specific enough for a driver to use.
Is there a case that a controller with peripheral requests cannot
support mem-to-mem transfers? I don't think there is. You could decide
you don't want to for other reasons like you don't have enough free
channels, but that's really a s/w decision, not a h/w description.
>
>
> > + - arm,primecell - should be included for all pl330 dma
> controller nodes.
> > +
> > + - reg: physical base address of the controller and length of
> memory mapped
> > + region.
> > +
> > + - interrupts: interrupt number to the cpu.
> > +
> > + - arm,primecell-periphid: should be 0x00041330.
>
> Should be optional. It's only needed when the h/w value is wrong. This
> is already documented in primecell.txt.
>
>
> Ok. This will be made optional.
>
>
>
> > +
> > + - arm,pl330-peri-reqs: number of actual peripheral requests
> connected to the
> > + dma controller. Maximum value is 32.
>
> Perhaps could be a bitmask for sparsely populated requests. May not
> matter since phandles will define the connections.
>
> Can be optional and not present means 00 requests (mem-to-mem only).
>
>
> As suggested by Russell, this property will be removed and its value
> will be read from the configuration register.
>
Good. Reading a value of 0 requests can still be used to determine the
controller is mem-to-mem only.
>
> > +
> > +Example: (from Samsung's Exynos4 processor dtsi file)
> > +
> > + pdma0: pdma at 12680000 {
> > + compatible = "arm,pl330-pdma", "arm,primecell";
> > + reg = <0x12680000 0x1000>;
> > + interrupts = <99>;
> > + arm,primecell-periphid = <0x00041330>;
> > + arm,pl330-peri-reqs = <30>;
> > + };
> > +
> > +Client drivers (device nodes requiring dma transfers from
> dev-to-mem or
> > +mem-to-dev) should specify the DMA channel numbers using a
> two-value pair
> > +as shown below.
> > +
> > + [property name] = <[phandle of the dma controller] [dma
> request id]>;
> > +
> > + where 'dma request id' is the dma request number which is
> connected
> > + to the client controller.
> > +
> > + Example: tx-dma-channel = <&pdma0 12>;
>
> I like this approach. I looked at this some and some PPC platforms do a
> node for each channel/request, but this is much more simple and similar
> to clock binding approach.
>
> You need to define the property name. Probably just "dma-channel" is
> enough. For peripherals with more than 1, just list them out like when
> you have more than 1 interrupt. The order should be defined as part of
> that device's binding (i.e. 1st channel is tx and 2nd channel is rx).
>
>
> I am little hesitant to do this the way you suggested. A controller
> could have dma request lines connected to multiple dma controllers. So
> the phandle could be different for each dma channel. Also, the client
> drivers specify the property value for each dma channel requested (the
> property value gets assigned to chan->private and then used by the
> filter function to lookup the dma channel). So changing it the way you
> have suggested would make things complex.
>
Perhaps you could make private point to the array entry rather than the
property.
But I don't have s strong opinion either way.
>
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 9732995..984dc18 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -19,6 +19,7 @@
> > #include <linux/amba/pl330.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/scatterlist.h>
> > +#include <linux/of.h>
> >
> > #define NR_DEFAULT_DESC 16
> >
> > @@ -277,6 +278,20 @@ bool pl330_filter(struct dma_chan *chan, void
> *param)
> > if (chan->device->dev->driver != &pl330_driver.drv)
> > return false;
> >
> > +#ifdef CONFIG_OF
> > + if (chan->device->dev->of_node) {
> > + const __be32 *prop_value;
> > + phandle phandle;
> > + struct device_node *node;
> > +
> > + prop_value = ((struct property *)param)->value;
> > + phandle = be32_to_cpup(prop_value++);
> > + node = of_find_node_by_phandle(phandle);
> > + return ((chan->private == node) &&
> > + (chan->chan_id ==
> be32_to_cpup(prop_value)));
> > + }
> > +#endif
> > +
> > peri_id = chan->private;
> > return *peri_id == (unsigned)param;
> > }
> > @@ -777,6 +792,40 @@ static irqreturn_t pl330_irq_handler(int irq,
> void *data)
> > return IRQ_NONE;
> > }
> >
> > +#ifdef CONFIG_OF
> > +static struct dma_pl330_platdata *pl330_parse_dt(struct device *dev)
> > +{
> > + struct dma_pl330_platdata *pdat;
> > + const void *value;
> > +
> > + pdat = devm_kzalloc(dev, sizeof(*pdat), GFP_KERNEL);
> > + if (!pdat)
> > + return NULL;
>
> Ideally, we will get rid of platform_data completely in the future, so I
> don't think filling it in from DT is the right approach.
>
>
> Ok. I will drop the usage of platform data.
>
>
> > +
> > + value = of_get_property(dev->of_node, "arm,pl330-peri-reqs",
> NULL);
> > + if (value)
> > + pdat->nr_valid_peri = be32_to_cpup(value);
>
> Can't you use the u32 helper function here?
>
>
> This will go away now since the number of peripherals connected will be
> read from the configuration register.
>
>
> > +
> > + if (of_device_is_compatible(dev->of_node, "arm,pl330-pdma")) {
> > + dma_cap_set(DMA_SLAVE, pdat->cap_mask);
> > + dma_cap_set(DMA_CYCLIC, pdat->cap_mask);
> > + } else if (of_device_is_compatible(dev->of_node,
> "arm,pl330-mdma")) {
> > + dma_cap_set(DMA_MEMCPY, pdat->cap_mask);
> > + } else if (of_device_is_compatible(dev->of_node,
> "arm,primecell")) {
>
> I don't think the driver should look at this property. This is really
> just for the bus code.
>
>
> The dma capabilities are derived from the compatible value by the
> driver. Sorry, I do not understand your suggestion for this.
>
"arm,primecell" is purely for identifying peripherals with the Primecell
ID registers and in turn used to create amba_device instances. You
cannot imply that it is a DMA controller.
Rob
> Thanks for your help.
>
> Regards,
> Thomas.
>
>
> Rob
>
>
More information about the devicetree-discuss
mailing list