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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
