Re: [PATCH 4/6] DMA: PL330: Add device tree support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thomas,

On 08/30/2011 07:18 AM, Thomas Abraham wrote:
> Hi Rob,
> 
> On 26 August 2011 18:46, Rob Herring <robherring2@xxxxxxxxx
> <mailto:robherring2@xxxxxxxxx>> 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@xxxxxxxxxx
>     <mailto:thomas.abraham@xxxxxxxxxx>>
>     > ---
>     >  .../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@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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux