Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Gerhard Sittig <gsi@denx.de>]

[ 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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de