Patchwork [RFC,5/8] dma: mpc512x: use symbolic specifiers for DMA channels

login
register
mail settings
Submitter Gerhard Sittig
Date July 12, 2013, 3:26 p.m.
Message ID <1373642781-32631-6-git-send-email-gsi@denx.de>
Download mbox | patch
Permalink /patch/258769/
State Superseded
Delegated to: Anatolij Gustschin
Headers show

Comments

Gerhard Sittig - July 12, 2013, 3:26 p.m.
for the DMA controller of the MPC512x SoC, DMA channels directly
correspond to specific peripherals, since requester lines directly map
to channels while no additional flexibility or mapping is involved

introduce a dt-bindings header file for MPC512x DMA channels, and make
the shared DT specs in the mpc5121.dtsi as well as the DMA engine driver
use those names instead of numbers

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 arch/powerpc/boot/dts/mpc5121.dtsi    |    6 +++++-
 drivers/dma/mpc512x_dma.c             |    6 ++++--
 include/dt-bindings/dma/mpc512x-dma.h |   21 +++++++++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)
 create mode 100644 include/dt-bindings/dma/mpc512x-dma.h
Arnd Bergmann - July 13, 2013, 7:17 a.m.
On Friday 12 July 2013, Gerhard Sittig wrote:
> +++ b/include/dt-bindings/dma/mpc512x-dma.h
> @@ -0,0 +1,21 @@
> +/*
> + * This header file provides symbolic specifiers for DMA channels
> + * within the MPC512x SoC's DMA controller.  Since requester lines
> + * directly map to channel numbers and no additional flexibility
> + * is involved, DMA channels can be considered directly associated
> + * with individual peripherals.
> + *
> + * This header file gets shared among DT bindings which provide
> + * hardware specs, and driver code which implements supporting logic.
> + */
> +
> +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> +
> +#define MPC512x_DMACHAN_SCLPC          26
> +#define MPC512x_DMACHAN_SDHC           30
> +#define MPC512x_DMACHAN_MDDRC          32
> +
> +#define MPC512x_DMACHAN_MAX            64
> +

I think these should not be in the header and should not bve part of the
binding either. They are specific to an SoC that happens to be using this
DMA controller but would be completely different for any other SoC with
the same DMA engine. These belong into the dma descriptors of the slave
drivers and don't need symbolic names.

	Arnd
Gerhard Sittig - July 13, 2013, 2:14 p.m.
[ MPC8308 knowledge required, see below ]

On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
> 
> On Friday 12 July 2013, Gerhard Sittig wrote:
> > +++ b/include/dt-bindings/dma/mpc512x-dma.h
> > @@ -0,0 +1,21 @@
> > +/*
> > + * This header file provides symbolic specifiers for DMA channels
> > + * within the MPC512x SoC's DMA controller.  Since requester lines
> > + * directly map to channel numbers and no additional flexibility
> > + * is involved, DMA channels can be considered directly associated
> > + * with individual peripherals.
> > + *
> > + * This header file gets shared among DT bindings which provide
> > + * hardware specs, and driver code which implements supporting logic.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> > +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> > +
> > +#define MPC512x_DMACHAN_SCLPC          26
> > +#define MPC512x_DMACHAN_SDHC           30
> > +#define MPC512x_DMACHAN_MDDRC          32
> > +
> > +#define MPC512x_DMACHAN_MAX            64
> > +
> 
> I think these should not be in the header and should not bve part of the
> binding either. They are specific to an SoC that happens to be using this
> DMA controller but would be completely different for any other SoC with
> the same DMA engine. These belong into the dma descriptors of the slave
> drivers and don't need symbolic names.

Thank you for the feedback.

OK, so not adding the dt-bindings header leads to no change in
the DTS nodes, which in turn collapses 5/8 into something local
to the .c driver source (introduce an enum and replace a few
magic numbers with names), and obsoletes 4/8 as a prerequisite.
This will further reduce the patch set's size.

Your feedback made me re-visit the driver source and notice that
it is shared among the MPC512x as well as the MPC8308 hardware.
The latter only has 16 channels, and appears to _not_ have its
channels dedicated to peripherals.  It's even uncertain to me
whether it can cope with peripherals at all and how so.

I scanned chapter 12 (DMA controller) in the MPC8308 reference
manual (rev 0 as of 2010-04) several times and could not find any
hint about peripherals, request lines, or anything else related
to flow control.  Searching in the whole RM won't give a hint
either.  Does this suggest that the MPC8308 DMA controller's
channels are "free" in their assignment to transfer tasks?  Or
are they "memory transfers only"?  Or do they happily accept any
XLB address (internal and external RAM, IMMR and IP bus space)
but don't apply flow control, i.e. expect either peripherals to
already hold the RX data, or peripherals to keep up with being
fed random amounts of TX data?  I tend to read the doc as the
latter.

Can somebody with MPC8308 knowledge confirm my suspicion that the
MPC8308 DMA channels aren't associated with peripherals, and thus
always need the software initiated start condition (_if_ they are
used for data transfer to/from peripherals)?  Can somebody with
access to an MPC8308 based board test later versions of the
series, to verify we don't break DMA operation on that platform?

Regardless of whether MPC8308 can't handle peripheral access, or
doesn't differ from memory transfers there, the patch series
needs an update.  Part 1/8 in its current form is either wrong or
incomplete, works for MPC512x but not for all hardware that this
driver is responsible for.


virtually yours
Gerhard Sittig
Arnd Bergmann - July 14, 2013, 8:50 a.m.
On Saturday 13 July 2013, Gerhard Sittig wrote:
> [ MPC8308 knowledge required, see below ]
> 
> On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
> > 
> > On Friday 12 July 2013, Gerhard Sittig wrote:
> > > +++ b/include/dt-bindings/dma/mpc512x-dma.h
> > > @@ -0,0 +1,21 @@
> > > +/*
> > > + * This header file provides symbolic specifiers for DMA channels
> > > + * within the MPC512x SoC's DMA controller.  Since requester lines
> > > + * directly map to channel numbers and no additional flexibility
> > > + * is involved, DMA channels can be considered directly associated
> > > + * with individual peripherals.
> > > + *
> > > + * This header file gets shared among DT bindings which provide
> > > + * hardware specs, and driver code which implements supporting logic.
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> > > +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> > > +
> > > +#define MPC512x_DMACHAN_SCLPC          26
> > > +#define MPC512x_DMACHAN_SDHC           30
> > > +#define MPC512x_DMACHAN_MDDRC          32
> > > +
> > > +#define MPC512x_DMACHAN_MAX            64
> > > +
> > 
> > I think these should not be in the header and should not bve part of the
> > binding either. They are specific to an SoC that happens to be using this
> > DMA controller but would be completely different for any other SoC with
> > the same DMA engine. These belong into the dma descriptors of the slave
> > drivers and don't need symbolic names.
> 
> Thank you for the feedback.
> 
> OK, so not adding the dt-bindings header leads to no change in
> the DTS nodes, which in turn collapses 5/8 into something local
> to the .c driver source (introduce an enum and replace a few
> magic numbers with names), and obsoletes 4/8 as a prerequisite.
> This will further reduce the patch set's size.

Actually I think you will need extra changes: The dma-engine driver
should not require knowledge of any channel-specific settings.
I did not notice you had them until you mentioned the above, but
from what I can tell, you need a few flags in the dma-specifier
to replace code like

                /* only start explicitly on MDDRC channel */
-               if (cid == 32)
+               if (cid == MPC512x_DMACHAN_MDDRC)
                        mdesc->tcd->start = 1;

with

		mdesc->tcd->start = dmaspec->explicit_start;

or something along these lines, where dmaspec is a data structure
derived from the fields in the DT dma specifier of the child
node.		

> I scanned chapter 12 (DMA controller) in the MPC8308 reference
> manual (rev 0 as of 2010-04) several times and could not find any
> hint about peripherals, request lines, or anything else related
> to flow control.  Searching in the whole RM won't give a hint
> either.  Does this suggest that the MPC8308 DMA controller's
> channels are "free" in their assignment to transfer tasks?  Or
> are they "memory transfers only"?  Or do they happily accept any
> XLB address (internal and external RAM, IMMR and IP bus space)
> but don't apply flow control, i.e. expect either peripherals to
> already hold the RX data, or peripherals to keep up with being
> fed random amounts of TX data?  I tend to read the doc as the
> latter.

It sounds to me that they are memory-to-memory only, which means
you probably want to allow #dma-cells=<0> as a special case to
describe an instance that has no slave API support.

	Arnd
Lars-Peter Clausen - July 14, 2013, 9:53 a.m.
On 07/14/2013 10:50 AM, Arnd Bergmann wrote:
> On Saturday 13 July 2013, Gerhard Sittig wrote:
>> [ MPC8308 knowledge required, see below ]
>>
>> On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
>>>
>>> On Friday 12 July 2013, Gerhard Sittig wrote:
>>>> +++ b/include/dt-bindings/dma/mpc512x-dma.h
>>>> @@ -0,0 +1,21 @@
>>>> +/*
>>>> + * This header file provides symbolic specifiers for DMA channels
>>>> + * within the MPC512x SoC's DMA controller.  Since requester lines
>>>> + * directly map to channel numbers and no additional flexibility
>>>> + * is involved, DMA channels can be considered directly associated
>>>> + * with individual peripherals.
>>>> + *
>>>> + * This header file gets shared among DT bindings which provide
>>>> + * hardware specs, and driver code which implements supporting logic.
>>>> + */
>>>> +
>>>> +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
>>>> +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
>>>> +
>>>> +#define MPC512x_DMACHAN_SCLPC          26
>>>> +#define MPC512x_DMACHAN_SDHC           30
>>>> +#define MPC512x_DMACHAN_MDDRC          32
>>>> +
>>>> +#define MPC512x_DMACHAN_MAX            64
>>>> +
>>>
>>> I think these should not be in the header and should not bve part of the
>>> binding either. They are specific to an SoC that happens to be using this
>>> DMA controller but would be completely different for any other SoC with
>>> the same DMA engine. These belong into the dma descriptors of the slave
>>> drivers and don't need symbolic names.
>>
>> Thank you for the feedback.
>>
>> OK, so not adding the dt-bindings header leads to no change in
>> the DTS nodes, which in turn collapses 5/8 into something local
>> to the .c driver source (introduce an enum and replace a few
>> magic numbers with names), and obsoletes 4/8 as a prerequisite.
>> This will further reduce the patch set's size.
> 
> Actually I think you will need extra changes: The dma-engine driver
> should not require knowledge of any channel-specific settings.
> I did not notice you had them until you mentioned the above, but
> from what I can tell, you need a few flags in the dma-specifier
> to replace code like
> 
>                 /* only start explicitly on MDDRC channel */
> -               if (cid == 32)
> +               if (cid == MPC512x_DMACHAN_MDDRC)
>                         mdesc->tcd->start = 1;
> 
> with
> 
> 		mdesc->tcd->start = dmaspec->explicit_start;
> 
> or something along these lines, where dmaspec is a data structure
> derived from the fields in the DT dma specifier of the child
> node.		
> 

I think the MDDRC channel is used for mem-to-mem transfers. So there is no
peripheral that is going to request it by handle. If the channel that is
used for mem-to-mem transfers differs between different instances of the
controller (e.g. on different SoCs) it should probably be a property of the
dma controllers DT node.

- Lars
Gerhard Sittig - July 14, 2013, 11:02 a.m.
On Sun, Jul 14, 2013 at 10:50 +0200, Arnd Bergmann wrote:
> 
> On Saturday 13 July 2013, Gerhard Sittig wrote:
> > 
> > [ ... ]
> > 
> > Thank you for the feedback.
> > 
> > OK, so not adding the dt-bindings header leads to no change in
> > the DTS nodes, which in turn collapses 5/8 into something local
> > to the .c driver source (introduce an enum and replace a few
> > magic numbers with names), and obsoletes 4/8 as a prerequisite.
> > This will further reduce the patch set's size.
> 
> Actually I think you will need extra changes: The dma-engine driver
> should not require knowledge of any channel-specific settings.
> I did not notice you had them until you mentioned the above, but
> from what I can tell, you need a few flags in the dma-specifier
> to replace code like
> 
>                 /* only start explicitly on MDDRC channel */
> -               if (cid == 32)
> +               if (cid == MPC512x_DMACHAN_MDDRC)
>                         mdesc->tcd->start = 1;
> 
> with
> 
> 		mdesc->tcd->start = dmaspec->explicit_start;
> 
> or something along these lines, where dmaspec is a data structure
> derived from the fields in the DT dma specifier of the child
> node.		

The above change applies to the mpc512x_dma.c file, this is the
very driver source which implements the DMA engine API, and deals
with the specific details of the SoC.  So I consider this the
most appropriate location to handle requirements of specific
channels in the DMA controller's driver, while the generic DMA
engine subsystem just finds the provider's API.

The driver takes care of _one_ DMA controller which has several
channels while these channels in turn might have _different_
characteristics.

Most channels of the MPC512x SoC's DMA controller have request
lines for peripherals to apply flow control, while the 'MDDRC'
channel is dedicated to mem-to-mem transfers and requires a
software initiated start in the absence of an external request
line.

The channels of the MPC8308 SoC's DMA controller appear to not
have any request lines, which makes them perfectly fit for
mem-to-mem transfers and always requires software initiated
start.  Yet this does not absolutely prevent their use for
peripheral access, _provided_ that the peripheral's FIFO is deep
enough or the data volume is appropriately low (or wire speed is
rather high, whatever obsoletes flow control).

I'm in the process of preparing v2 of the series, which keeps
compatibility with the MPC8308 and approaches things slightly
differently than v1 although in the same spirit.


Regarding the device tree binding, I was under the impression
that backwards compatibility is a must.  Reading channel counts
from DT nodes instead of encoding them in the driver would have
been nice upon introduction of OF support, but would break
compatibility these days.


> > I scanned chapter 12 (DMA controller) in the MPC8308 reference
> > manual (rev 0 as of 2010-04) several times and could not find any
> > hint about peripherals, request lines, or anything else related
> > to flow control.  Searching in the whole RM won't give a hint
> > either.  Does this suggest that the MPC8308 DMA controller's
> > channels are "free" in their assignment to transfer tasks?  Or
> > are they "memory transfers only"?  Or do they happily accept any
> > XLB address (internal and external RAM, IMMR and IP bus space)
> > but don't apply flow control, i.e. expect either peripherals to
> > already hold the RX data, or peripherals to keep up with being
> > fed random amounts of TX data?  I tend to read the doc as the
> > latter.
> 
> It sounds to me that they are memory-to-memory only, which means
> you probably want to allow #dma-cells=<0> as a special case to
> describe an instance that has no slave API support.

This would impact the current OF registration, which
unconditionally assumes one-cell specs.  Before changing this to
one-cell for MPC512x and zero-zell for MPC8308, I'd love to learn
whether slave support on MPC8308 is inapplicable or whether it's
doable yet constraints apply (ATM I assume the latter, would not
want to enforce non-availability just to re-introduce it later).


virtually yours
Gerhard Sittig

Patch

diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
index bd14c00..384e692 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -9,6 +9,8 @@ 
  * option) any later version.
  */
 
+#include <dt-bindings/dma/mpc512x-dma.h>
+
 /dts-v1/;
 
 / {
@@ -152,7 +154,7 @@ 
 			compatible = "fsl,mpc5121-sdhc";
 			reg = <0x1500 0x100>;
 			interrupts = <8 0x8>;
-			dmas = <&dma0 30>;
+			dmas = <&dma0 MPC512x_DMACHAN_SDHC>;
 			dma-names = "rx-tx";
 		};
 
@@ -262,6 +264,8 @@ 
 		lpc@10000 {
 			compatible = "fsl,mpc5121-lpc";
 			reg = <0x10000 0x200>;
+			dmas = <&dma0 MPC512x_DMACHAN_SCLPC>;
+			dma-names = "rx-tx";
 		};
 
 		pata@10200 {
diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 0053ff8..8f6d545 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -29,6 +29,8 @@ 
  * file called COPYING.
  */
 
+#include <dt-bindings/dma/mpc512x-dma.h>
+
 #include <linux/module.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
@@ -257,7 +259,7 @@  static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		prev->tcd->dlast_sga = mdesc->tcd_paddr;
 		prev->tcd->e_sg = 1;
 		/* only start explicitly on MDDRC channel */
-		if (cid == 32)
+		if (cid == MPC512x_DMACHAN_MDDRC)
 			mdesc->tcd->start = 1;
 
 		prev = mdesc;
@@ -276,7 +278,7 @@  static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		/* peripherals involved, use external request */
 		out_8(&mdma->regs->dmaserq, cid);
 		break;
-	case 32:
+	case MPC512x_DMACHAN_MDDRC:
 		/* memory transfer, software provided start signal */
 		out_8(&mdma->regs->dmassrt, cid);
 		break;
diff --git a/include/dt-bindings/dma/mpc512x-dma.h b/include/dt-bindings/dma/mpc512x-dma.h
new file mode 100644
index 0000000..56b06d1
--- /dev/null
+++ b/include/dt-bindings/dma/mpc512x-dma.h
@@ -0,0 +1,21 @@ 
+/*
+ * This header file provides symbolic specifiers for DMA channels
+ * within the MPC512x SoC's DMA controller.  Since requester lines
+ * directly map to channel numbers and no additional flexibility
+ * is involved, DMA channels can be considered directly associated
+ * with individual peripherals.
+ *
+ * This header file gets shared among DT bindings which provide
+ * hardware specs, and driver code which implements supporting logic.
+ */
+
+#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
+#define _DT_BINDINGS_DMA_MPC512x_DMA_H
+
+#define MPC512x_DMACHAN_SCLPC		26
+#define MPC512x_DMACHAN_SDHC		30
+#define MPC512x_DMACHAN_MDDRC		32
+
+#define MPC512x_DMACHAN_MAX		64
+
+#endif