Quick reactions to this version ...
Almost all of these changes flow directly from the interface
changes which I've excerpted below, which will help make this
response much shorter than the patch! That's a nice feature
of a patch being reviewed.
Still ... 40 KBytes is big. This patch *might* split naturally
into (a) internal restructuring around a "struct edma", (b) the
interface changes visible to DMA clients, and (c) using those
changes to work with multiple such structs. Smaller patches
are easier to test and review; plus, the MMC and ASoC patches
would depend only on (b).
I'd structure the alloc_channel() a bit differently. You made
it loop through each channel controller ... but that's only
needed when it's not allocating a specific channel. Maybe the
thing to do is more or less
if (search)
call helper to allocate some channel on some CC;
else
allocate specified channel on specified CC;
if (error)
return failure code;
init
return success
Plus, both your alloc_channel() and alloc_slot() are discarding the
controller they may have been told to use ... alloc_slot() should
if (slot >= 0) {
ctlr = EDMA_CTLR(slot);
slot = EDMA_CHAN(slot);
}
On Friday 17 April 2009, Sudhakar Rajashekhara wrote:
> --- a/arch/arm/mach-davinci/include/mach/edma.h
> +++ b/arch/arm/mach-davinci/include/mach/edma.h
> @@ -58,6 +58,11 @@
> #ifndef EDMA_H_
> #define EDMA_H_
>
> +#define EDMA_MAX_DMACH 64
> +#define EDMA_MAX_PARAMENTRY 512
> +#define EDMA_MAX_EVQUE 2 /* FIXME too small */
> +#define EDMA_MAX_CC 2
> +
Those shouldn't need to move outside of the dma.c source file;
there's no reason a DMA client should care about these values.
> /* PaRAM slots are laid out like this */
> struct edmacc_param {
> unsigned int opt;
> @@ -171,6 +176,10 @@ enum sync_dimension {
> ABSYNC = 1
> };
>
> +#define EDMA_CTLR_CHAN(ctlr, chan) (((ctlr) << 16) | (chan))
> +#define EDMA_CTLR(i) ((i) >> 16)
> +#define EDMA_CHAN(i) ((i) & 0xffff)
> +
I can't quite think of a word that can mean "channel or slot"
and is already used in the DMA context. So all I'll do right
now is note that "CHAN" will be misleading when these are
used with slot identifiers, so a better tag would be nice.
> #define EDMA_CHANNEL_ANY -1 /* for edma_alloc_channel() */
> #define EDMA_SLOT_ANY -1 /* for edma_alloc_slot() */
>
> @@ -181,7 +190,7 @@ int edma_alloc_channel(int channel,
> void edma_free_channel(unsigned channel);
>
> /* alloc/free parameter RAM slots */
> -int edma_alloc_slot(int slot);
> +int edma_alloc_slot(unsigned ctlr, int slot);
Right, that's about all that DMA clients should need to see change.
One function, and those three macros. ;)
> void edma_free_slot(unsigned slot);
>
> /* calls that operate on part of a parameter RAM slot */
> @@ -221,9 +230,34 @@ struct edma_soc_info {
> unsigned n_region;
> unsigned n_slot;
> unsigned n_tc;
> + unsigned n_cc;
>
> /* list of channels with no even trigger; terminated by "-1" */
> const s8 *noevent;
> };
>
> +struct edma {
I had in mind that this structure would be internal to
the dma.c source file because, again, clients have no
need to see these details.
> + /* how many dma resources of each type */
> + unsigned num_channels;
> + unsigned num_region;
> + unsigned num_slots;
> + unsigned num_tc;
> + unsigned num_cc;
> +
> + /* list of channels with no even trigger; terminated by "-1" */
> + const s8 *noevent;
> +
> + /* The edma_inuse bit for each PaRAM slot is clear unless the
> + * channel is in use ... by ARM or DSP, for QDMA, or whatever.
> + */
> + DECLARE_BITMAP(edma_inuse, EDMA_MAX_PARAMENTRY);
> +
> +
> + /* The edma_noevent bit for each channel is clear unless
> + * it doesn't trigger DMA events on this platform. It uses a
> + * bit of SOC-specific initialization code.
> + */
> + DECLARE_BITMAP(edma_noevent, EDMA_MAX_DMACH);
I would think that the interrupt callback data would need to be
stored per-CC as well. You still have a global-to-the-driver
"struct interrupt_data { ... } intr_data[EDMA_MAX_DMACH];", so
there's no way to distinguish e.g. the channel 42 IRQ from two
different channel controllers ...
> +};
> +
> #endif
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
