RE: [PATCH v2 1/3] ARM: DaVinci: Add support for multiple EDMA CC instances
Dave,
>
> > /* 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) & 0x)
> > +
>
> 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.
>
As EDMA_CTLR_CHAN is used only to pass channel numbers, we can
leave it as it is and I am proposing to use EDMA_CHAN_SLOT
instead of EDMA_CHAN to get channel/slot numbers.
Regards, Sudhakar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v2 1/3] ARM: DaVinci: Add support for multiple EDMA CC instances
Dave,
[...]
>
> 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);
> }
I agree that I was discarding the controller information in
edma_alloc_channel function, but I am taking the controller
as an argument in edma_alloc_slot function.
Regards, Sudhakar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v2 1/3] ARM: DaVinci: Add support for multiple EDMA CC instances
Dave,
I'll incorporate the comments and re-submit the patch.
Thanks, Sudhakar
>
> 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) & 0x)
> > +
>
> 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 {
> > unsignedn_region;
> > unsignedn_slot;
> > unsignedn_tc;
> > + unsignedn_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 */
> > + unsignednum_channels;
> > + unsignednum_region;
> > + unsignednum_slots;
> > + unsignednum_tc;
> > + unsignednum_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
>
>
>
___
Re: [PATCH v2 1/3] ARM: DaVinci: Add support for multiple EDMA CC instances
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) & 0x)
> +
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 {
> unsignedn_region;
> unsignedn_slot;
> unsignedn_tc;
> + unsignedn_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 */
> + unsignednum_channels;
> + unsignednum_region;
> + unsignednum_slots;
> + unsignednum_tc;
> + unsignednum_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
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
