| Message ID | 1373642781-32631-6-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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
[ 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
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
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
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
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
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