This adds an interface to the DMAengine to make it possible to
reconfigure a slave channel at runtime. We add a few foreseen
config parameters to the passed struct, with a void * pointer
for custom per-device or per-platform runtime slave data.
Signed-off-by: Linus Walleij <[email protected]>
---
include/linux/dmaengine.h | 48 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 5204f01..e2601bd 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -114,11 +114,17 @@ enum dma_ctrl_flags {
* @DMA_TERMINATE_ALL: terminate all ongoing transfers
* @DMA_PAUSE: pause ongoing transfers
* @DMA_RESUME: resume paused transfer
+ * @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
+ * that need to runtime reconfigure the slave channels (as opposed to passing
+ * configuration data in statically from the platform). An additional
+ * argument of struct dma_slave_config must be passed in with this
+ * command.
*/
enum dma_ctrl_cmd {
DMA_TERMINATE_ALL,
DMA_PAUSE,
DMA_RESUME,
+ DMA_SLAVE_CONFIG,
};
/**
@@ -199,6 +205,48 @@ struct dma_chan_dev {
atomic_t *idr_ref;
};
+/**
+ * struct dma_slave_config - dma slave channel runtime config
+ * @addr: this is the physical address where DMA slave data should be
+ * read (RX) or written (TX)
+ * @addr_width: this is the width of the source (RX) or target
+ * (TX) register where DMA data shall be read/written, in bytes.
+ * legal values: 1, 2, 4, 8.
+ * @direction: whether the data shall go in or out on this slave
+ * channel, right now.
+ * @maxburst: the maximum number of words (note: words, as in units
+ * of the addr_width member, not bytes) that can be sent in one burst
+ * to the device. Typically something like half the FIFO depth on
+ * I/O peripherals so you don't overflow it.
+ * @private_config: if you need to pass in specialized configuration
+ * at runtime, apart from the generic things supported in this
+ * struct, you provide it in this pointer and dereference it inside
+ * your dmaengine driver to get the proper configuration bits out.
+ *
+ * This struct is passed in as configuration data to a DMA engine
+ * in order to set up a certain channel for DMA transport at runtime.
+ * The DMA device/engine has to provide support for an additional
+ * command in the channel config interface, DMA_SLAVE_CONFIG
+ * and this struct will then be passed in as an argument to the
+ * DMA engine device_control() function.
+ *
+ * The rationale for adding configuration information to this struct
+ * is as follows: if it is likely that most DMA slave controllers in
+ * the world will support the configuration option, then make it
+ * generic. If not, if it is fixed so that it be sent in static from
+ * the platform data, then prefer to do that. Else, if it is neither
+ * fixed, not generic enough (such as bus mastership on some CPU
+ * family and whatnot) then pass it in the private_config member
+ * and dereference it to some per-device struct in your driver.
+ */
+struct dma_slave_config {
+ dma_addr_t addr;
+ u8 addr_width:4;
+ enum dma_data_direction direction;
+ int maxburst;
+ void *private_config;
+};
+
static inline const char *dma_chan_name(struct dma_chan *chan)
{
return dev_name(&chan->dev->device);
--
1.7.0.1
Hi Dan,
any thoughts on this? Personally I really like this abstraction from
my experience with embedded DMA controllers, but I don't have your
wide experience of DMA hardware.
Yours,
Linus Walleij
2010/6/29 Linus Walleij <[email protected]>:
> This adds an interface to the DMAengine to make it possible to
> reconfigure a slave channel at runtime. We add a few foreseen
> config parameters to the passed struct, with a void * pointer
> for custom per-device or per-platform runtime slave data.
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ?include/linux/dmaengine.h | ? 48 +++++++++++++++++++++++++++++++++++++++++++++
> ?1 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 5204f01..e2601bd 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -114,11 +114,17 @@ enum dma_ctrl_flags {
> ?* @DMA_TERMINATE_ALL: terminate all ongoing transfers
> ?* @DMA_PAUSE: pause ongoing transfers
> ?* @DMA_RESUME: resume paused transfer
> + * @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
> + * that need to runtime reconfigure the slave channels (as opposed to passing
> + * configuration data in statically from the platform). An additional
> + * argument of struct dma_slave_config must be passed in with this
> + * command.
> ?*/
> ?enum dma_ctrl_cmd {
> ? ? ? ?DMA_TERMINATE_ALL,
> ? ? ? ?DMA_PAUSE,
> ? ? ? ?DMA_RESUME,
> + ? ? ? DMA_SLAVE_CONFIG,
> ?};
>
> ?/**
> @@ -199,6 +205,48 @@ struct dma_chan_dev {
> ? ? ? ?atomic_t *idr_ref;
> ?};
>
> +/**
> + * struct dma_slave_config - dma slave channel runtime config
> + * @addr: this is the physical address where DMA slave data should be
> + * read (RX) or written (TX)
> + * @addr_width: this is the width of the source (RX) or target
> + * (TX) register where DMA data shall be read/written, in bytes.
> + * legal values: 1, 2, 4, 8.
> + * @direction: whether the data shall go in or out on this slave
> + * channel, right now.
> + * @maxburst: the maximum number of words (note: words, as in units
> + * of the addr_width member, not bytes) that can be sent in one burst
> + * to the device. Typically something like half the FIFO depth on
> + * I/O peripherals so you don't overflow it.
> + * @private_config: if you need to pass in specialized configuration
> + * at runtime, apart from the generic things supported in this
> + * struct, you provide it in this pointer and dereference it inside
> + * your dmaengine driver to get the proper configuration bits out.
> + *
> + * This struct is passed in as configuration data to a DMA engine
> + * in order to set up a certain channel for DMA transport at runtime.
> + * The DMA device/engine has to provide support for an additional
> + * command in the channel config interface, DMA_SLAVE_CONFIG
> + * and this struct will then be passed in as an argument to the
> + * DMA engine device_control() function.
> + *
> + * The rationale for adding configuration information to this struct
> + * is as follows: if it is likely that most DMA slave controllers in
> + * the world will support the configuration option, then make it
> + * generic. If not, if it is fixed so that it be sent in static from
> + * the platform data, then prefer to do that. Else, if it is neither
> + * fixed, not generic enough (such as bus mastership on some CPU
> + * family and whatnot) then pass it in the private_config member
> + * and dereference it to some per-device struct in your driver.
> + */
> +struct dma_slave_config {
> + ? ? ? dma_addr_t addr;
> + ? ? ? u8 addr_width:4;
> + ? ? ? enum dma_data_direction direction;
> + ? ? ? int maxburst;
> + ? ? ? void *private_config;
> +};
> +
> ?static inline const char *dma_chan_name(struct dma_chan *chan)
> ?{
> ? ? ? ?return dev_name(&chan->dev->device);
> --
> 1.7.0.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
Ping on this subject, I have the PL08x driver depending on this
so if it's not OK I have to rewrite that patch soonish.
Also the DMA for PrimeCells use it so I would like to respin that
patch series using this generic control if it is accepted.
As mentions something like this will be needed for us anyway,
so in case it's not OK I'll likely go for more or less the same
struct, just that it is for DMA40 and its clients solely, then on
top of that a PrimeCell interface more or less redefining the
same things again or wrapping it.
Yours,
Linus Walleij
On Wed, Jul 14, 2010 at 4:11 PM, Linus Walleij
<[email protected]> wrote:
> Ping on this subject, I have the PL08x driver depending on this
> so if it's not OK I have to rewrite that patch soonish.
>
> Also the DMA for PrimeCells use it so I would like to respin that
> patch series using this generic control if it is accepted.
>
> As mentions something like this will be needed for us anyway,
> so in case it's not OK I'll likely go for more or less the same
> struct, just that it is for DMA40 and its clients solely, then on
> top of that a PrimeCell interface more or less redefining the
> same things again or wrapping it.
>
I think it looks ok, I just want to get one other dmaslave driver
author to nod that dma_slave_config has the right amount of fields. I
initially put too much into dma_async_tx_descriptor. I'll take this
as is if we don't get feedback, but that would remove my hesitation.
The recently submitted intel_mid driver [1] seems to have a similar structure:
+/**
+ * struct intel_mid_dma_slave - DMA slave structure
+ *
+ * @dma_dev: DMA master client
+ * @tx_reg: physical address of data register used for
+ * memory-to-peripheral transfers
+ * @rx_reg: physical address of data register used for
+ * peripheral-to-memory transfers
+ * @tx_width: tx register width
+ * @rx_width: rx register width
+ * @dirn: DMA trf direction
+
+ * @cfg_hi: Platform-specific initializer for the CFG_HI register
+ * @cfg_lo: Platform-specific initializer for the CFG_LO register
+
+ * @ tx_width: width of src and dstn
+ * @ hs_mode: SW or HW handskaking mode
+ * @ cfg_mode: Mode configuration, DMA mem to mem to dev & mem
+ */
+struct intel_mid_dma_slave {
+ enum dma_data_direction dirn;
+ enum intel_mid_dma_width src_width; /*width of DMA src txn*/
+ enum intel_mid_dma_width dst_width; /*width of DMA dst txn*/
+ enum intel_mid_dma_hs_mode hs_mode; /*handshaking*/
+ enum intel_mid_dma_mode cfg_mode; /*mode configuration*/
+ enum intel_mid_dma_msize src_msize; /*size if src burst*/
+ enum intel_mid_dma_msize dst_msize; /*size of dst burst*/
+ dma_async_tx_callback callback; /*callback function*/
+ void *callback_param; /*param for callback*/
+ unsigned int device_instance; /*0, 1 for periphral instance*/
+};
+
Vinod, would you consider switching to dma_slave_config [2] for this
information, or at a minimum wrapping dma_slave_config with your
intel_mid specific fields?
--
Dan
[1]: http://marc.info/?l=linux-kernel&m=127687775404278&w=2
[2]: http://marc.info/?l=linux-kernel&m=127777055428648&w=2
2010/7/19 Dan Williams <[email protected]>:
> The recently submitted intel_mid driver [1] seems to have a similar structure:
This is very interesting, let's examine this closely as a comparison!
I'm looking at it from the point of a generic slave control mechanism,
though I suspect it is designed to be Intel-specific so keep that in mind.
> +/**
> + * struct intel_mid_dma_slave - DMA slave structure
> + *
> + * @dma_dev: DMA master client
> + * @tx_reg: physical address of data register used for
> + * ? ? memory-to-peripheral transfers
> + * @rx_reg: physical address of data register used for
> + * ? ? peripheral-to-memory transfers
> + * @tx_width: tx register width
> + * @rx_width: rx register width
> + * @dirn: DMA trf direction
> + * @cfg_hi: Platform-specific initializer for the CFG_HI register
> + * @cfg_lo: Platform-specific initializer for the CFG_LO register
> + * @ tx_width: width of src and dstn
> + * @ hs_mode: SW or HW handskaking mode
> + * @ cfg_mode: Mode configuration, DMA mem to mem to dev & mem
> + */
> +struct intel_mid_dma_slave {
> + ? ? ? enum dma_data_direction ? ? ? ? dirn;
> + ? ? ? enum intel_mid_dma_width ? ? ? ?src_width; /*width of DMA src txn*/
> + ? ? ? enum intel_mid_dma_width ? ? ? ?dst_width; /*width of DMA dst txn*/
> + ? ? ? enum intel_mid_dma_hs_mode ? ? ?hs_mode; ?/*handshaking*/
> + ? ? ? enum intel_mid_dma_mode ? ? ? ? cfg_mode; /*mode configuration*/
> + ? ? ? enum intel_mid_dma_msize ? ? ? ?src_msize; /*size if src burst*/
> + ? ? ? enum intel_mid_dma_msize ? ? ? ?dst_msize; /*size of dst burst*/
> + ? ? ? dma_async_tx_callback ? ? ? ? ? callback; /*callback function*/
> + ? ? ? void ? ? ? ? ? ? ? ? ? ? ? ? ? ?*callback_param; /*param for callback*/
> + ? ? ? unsigned int ? ? ? ? ? ?device_instance; /*0, 1 for periphral instance*/
> +};
> +
kerneldoc and struct members seem to be out-of-sync but I
see the general idea.
Of these members handshaking, cfg_mode, hs_mode
and I suspect also device_instance are candidates for
the private config since they cannot be assumed to
exist on all DMA engines. (Also goes for cfg_[lo|hi] from
the kerneldoc)
The callback and callback param are configured
per-transfer in the current API, so I don't see what they
are doing here at all.
Remains: direction, then register address, burstsize and
word width of the source and destination.
(Register address present in kerneldoc but not in struct,
burstsize present in struct but not in kerneldoc, whatever)
I don't have src/dst pairs in my API, because since the
only data provision mechanisms to slaves are sglists,
and these provide either source or destination address
depending on transfer direction.
Also I assume that since sglists will be mem->peripheral
or peripheral->mem, you know what is required on the
memory side of things for word width and burstsize, and
these only affect the device side of things.
So that is why my API is more minimalistic.
If you feel src/dst versions of wordwidth, register address
and burstsize are a must, I can add them, no big thing,
just makes for some nonused parameters, mostly.
Yours,
Linus Walleij
On Mon, Jul 19, 2010 at 3:44 PM, Linus Walleij
<[email protected]> wrote:
> 2010/7/19 Dan Williams <[email protected]>:
>
>> The recently submitted intel_mid driver [1] seems to have a similar structure:
>
> This is very interesting, let's examine this closely as a comparison!
> I'm looking at it from the point of a generic slave control mechanism,
> though I suspect it is designed to be Intel-specific so keep that in mind.
>
>> +/**
>> + * struct intel_mid_dma_slave - DMA slave structure
>> + *
>> + * @dma_dev: DMA master client
>> + * @tx_reg: physical address of data register used for
>> + * ? ? memory-to-peripheral transfers
>> + * @rx_reg: physical address of data register used for
>> + * ? ? peripheral-to-memory transfers
>> + * @tx_width: tx register width
>> + * @rx_width: rx register width
>> + * @dirn: DMA trf direction
>> + * @cfg_hi: Platform-specific initializer for the CFG_HI register
>> + * @cfg_lo: Platform-specific initializer for the CFG_LO register
>> + * @ tx_width: width of src and dstn
>> + * @ hs_mode: SW or HW handskaking mode
>> + * @ cfg_mode: Mode configuration, DMA mem to mem to dev & mem
>> + */
>> +struct intel_mid_dma_slave {
>> + ? ? ? enum dma_data_direction ? ? ? ? dirn;
>> + ? ? ? enum intel_mid_dma_width ? ? ? ?src_width; /*width of DMA src txn*/
>> + ? ? ? enum intel_mid_dma_width ? ? ? ?dst_width; /*width of DMA dst txn*/
>> + ? ? ? enum intel_mid_dma_hs_mode ? ? ?hs_mode; ?/*handshaking*/
>> + ? ? ? enum intel_mid_dma_mode ? ? ? ? cfg_mode; /*mode configuration*/
>> + ? ? ? enum intel_mid_dma_msize ? ? ? ?src_msize; /*size if src burst*/
>> + ? ? ? enum intel_mid_dma_msize ? ? ? ?dst_msize; /*size of dst burst*/
>> + ? ? ? dma_async_tx_callback ? ? ? ? ? callback; /*callback function*/
>> + ? ? ? void ? ? ? ? ? ? ? ? ? ? ? ? ? ?*callback_param; /*param for callback*/
>> + ? ? ? unsigned int ? ? ? ? ? ?device_instance; /*0, 1 for periphral instance*/
>> +};
>> +
>
> kerneldoc and struct members seem to be out-of-sync but I
> see the general idea.
>
> Of these members handshaking, cfg_mode, hs_mode
> and I suspect also device_instance are candidates for
> the private config ?since they cannot be assumed to
> exist on all DMA engines. (Also goes for cfg_[lo|hi] from
> the kerneldoc)
>
> The callback and callback param are configured
> per-transfer in the current API, so I don't see what they
> are doing here at all.
Yeah, I already pointed that out...
>
> Remains: direction, then register address, burstsize and
> word width of the source and destination.
>
> (Register address present in kerneldoc but not in struct,
> burstsize present in struct but not in kerneldoc, whatever)
>
> I don't have src/dst pairs in my API, because since the
> only data provision mechanisms to slaves are sglists,
> and these provide either source or destination address
> depending on transfer direction.
>
> Also I assume that since sglists will be mem->peripheral
> or peripheral->mem, you know what is required on the
> memory side of things for word width and burstsize, and
> these only affect the device side of things.
>
> So that is why my API is more minimalistic.
>
> If you feel src/dst versions of wordwidth, register address
> and burstsize are a must, I can add them, no big thing,
> just makes for some nonused parameters, mostly.
Unused parameters is what I want to avoid, and getting drivers that
can wrap dma_slave_config to actually do so is the final kicker.
Thoughts Vinod?
Thanks,
Dan
> On Mon, Jul 19, 2010 at 3:44 PM, Linus Walleij
> <[email protected]> wrote:
> > 2010/7/19 Dan Williams <[email protected]>:
> >
> >> The recently submitted intel_mid driver [1] seems to have a similar structure:
> >
> > This is very interesting, let's examine this closely as a comparison!
> > I'm looking at it from the point of a generic slave control mechanism,
> > though I suspect it is designed to be Intel-specific so keep that in mind.
> >
> >> +/**
> >> + * struct intel_mid_dma_slave - DMA slave structure
> >> + *
> >> + * @dma_dev: DMA master client
> >> + * @tx_reg: physical address of data register used for
> >> + * ? ? memory-to-peripheral transfers
> >> + * @rx_reg: physical address of data register used for
> >> + * ? ? peripheral-to-memory transfers
> >> + * @tx_width: tx register width
> >> + * @rx_width: rx register width
> >> + * @dirn: DMA trf direction
> >> + * @cfg_hi: Platform-specific initializer for the CFG_HI register
> >> + * @cfg_lo: Platform-specific initializer for the CFG_LO register
> >> + * @ tx_width: width of src and dstn
> >> + * @ hs_mode: SW or HW handskaking mode
> >> + * @ cfg_mode: Mode configuration, DMA mem to mem to dev & mem
> >> + */
> >> +struct intel_mid_dma_slave {
> >> + ? ? ? enum dma_data_direction ? ? ? ? dirn;
> >> + ? ? ? enum intel_mid_dma_width ? ? ? ?src_width; /*width of DMA src txn*/
> >> + ? ? ? enum intel_mid_dma_width ? ? ? ?dst_width; /*width of DMA dst txn*/
> >> + ? ? ? enum intel_mid_dma_hs_mode ? ? ?hs_mode; ?/*handshaking*/
> >> + ? ? ? enum intel_mid_dma_mode ? ? ? ? cfg_mode; /*mode configuration*/
> >> + ? ? ? enum intel_mid_dma_msize ? ? ? ?src_msize; /*size if src burst*/
> >> + ? ? ? enum intel_mid_dma_msize ? ? ? ?dst_msize; /*size of dst burst*/
> >> + ? ? ? dma_async_tx_callback ? ? ? ? ? callback; /*callback function*/
> >> + ? ? ? void ? ? ? ? ? ? ? ? ? ? ? ? ? ?*callback_param; /*param for callback*/
> >> + ? ? ? unsigned int ? ? ? ? ? ?device_instance; /*0, 1 for periphral instance*/
> >> +};
> >> +
> >
> > kerneldoc and struct members seem to be out-of-sync but I
> > see the general idea.
> >
> > Of these members handshaking, cfg_mode, hs_mode
> > and I suspect also device_instance are candidates for
> > the private config ?since they cannot be assumed to
> > exist on all DMA engines. (Also goes for cfg_[lo|hi] from
> > the kerneldoc)
> >
> > The callback and callback param are configured
> > per-transfer in the current API, so I don't see what they
> > are doing here at all.
>
> Yeah, I already pointed that out...
This is my current structure which I was hoping to submit upstream this week
after my testing on various controllers was complete
/**
* struct intel_mid_dma_slave - DMA slave structure
*
* @dirn: DMA trf direction
* @src_width: tx register width
* @dst_width: rx register width
* @hs_mode: HW/SW handshaking mode
* @cfg_mode: DMA data transfer mode (per-per/mem-per/mem-mem)
* @src_msize: Source DMA burst size
* @dst_msize: Dst DMA burst size
* @device_instance: DMA peripheral device instance, we can have multiple
* peripheral device connected to single DMAC
*/
struct intel_mid_dma_slave {
enum dma_data_direction dirn;
enum intel_mid_dma_width src_width; /*width of DMA src txn*/
enum intel_mid_dma_width dst_width; /*width of DMA dst txn*/
enum intel_mid_dma_hs_mode hs_mode; /*handshaking*/
enum intel_mid_dma_mode cfg_mode; /*mode configuration*/
enum intel_mid_dma_msize src_msize; /*size if src burst*/
enum intel_mid_dma_msize dst_msize; /*size of dst burst*/
unsigned int device_instance; /*0, 1 for peripheral instance*/
};
>
> >
> > Remains: direction, then register address, burstsize and
> > word width of the source and destination.
> >
> > (Register address present in kerneldoc but not in struct,
> > burstsize present in struct but not in kerneldoc, whatever)
> >
> > I don't have src/dst pairs in my API, because since the
> > only data provision mechanisms to slaves are sglists,
> > and these provide either source or destination address
> > depending on transfer direction.
> >
> > Also I assume that since sglists will be mem->peripheral
> > or peripheral->mem, you know what is required on the
> > memory side of things for word width and burstsize, and
> > these only affect the device side of things.
> >
> > So that is why my API is more minimalistic.
> >
> > If you feel src/dst versions of wordwidth, register address
> > and burstsize are a must, I can add them, no big thing,
> > just makes for some nonused parameters, mostly.
>
> Unused parameters is what I want to avoid, and getting drivers that
> can wrap dma_slave_config to actually do so is the final kicker.
>
> Thoughts Vinod?
Yes the structures have various common things and would be good to abstract
generic stuff in this and move the rest to private_config.
But since we are talking about a generic DMA slave capabilities structure, I
would recommend changing few.
I am not sure why do you want the I/O addr to be passed here, sorry I
don't follow that logic. If you notice in intel_mid_dma driver the I/O address
is passed by client driver in prep_memcpy in src and dst fields. Since the slave
structure tell you the direction and mode you can interpret that src/dst address
as IO address easily
Addr_width and Maxburst: talks about the I/O width and msize I guess, but what
about mem-width, I have few configuration where we would like to have different
mem width as well.
And why limit the generic API and what about per-per transfers which
intel_mid_dma driver would need to support in future.
I would recommend renaming this field to src_addr_width etc and add dst_addr_xxx
pair.
Yes I can have them in private_config but then again driver has to do a simple
check on which one is src/dst in slave and which one is in privae_config. Again
easily doable but little confusing, I am okay either way
Thanks
Vinod
2010/7/20 Koul, Vinod <[email protected]>:
> /**
> ?* struct intel_mid_dma_slave - DMA slave structure
> ?*
> ?* @dirn: DMA trf direction
> ?* @src_width: tx register width
> ?* @dst_width: rx register width
> ?* @hs_mode: HW/SW handshaking mode
> ?* @cfg_mode: DMA data transfer mode (per-per/mem-per/mem-mem)
> ?* @src_msize: Source DMA burst size
> ?* @dst_msize: Dst DMA burst size
> ?* @device_instance: DMA peripheral device instance, we can have multiple
> ?* ? ? ? ? ? ? ? ? ? ? ?peripheral device connected to single DMAC
> ?*/
> struct intel_mid_dma_slave {
> ? ? ? ?enum dma_data_direction ? ? ? ? dirn;
> ? ? ? ?enum intel_mid_dma_width ? ? ? ?src_width; /*width of DMA src txn*/
> ? ? ? ?enum intel_mid_dma_width ? ? ? ?dst_width; /*width of DMA dst txn*/
> ? ? ? ?enum intel_mid_dma_hs_mode ? ? ?hs_mode; ?/*handshaking*/
> ? ? ? ?enum intel_mid_dma_mode ? ? ? ? cfg_mode; /*mode configuration*/
> ? ? ? ?enum intel_mid_dma_msize ? ? ? ?src_msize; /*size if src burst*/
> ? ? ? ?enum intel_mid_dma_msize ? ? ? ?dst_msize; /*size of dst burst*/
> ? ? ? ?unsigned int ? ? ? ? ? ?device_instance; /*0, 1 for peripheral instance*/
> };
>
> Yes the structures have various common things and would be good to abstract
> generic stuff in this and move the rest to private_config.
> But since we are talking about a generic DMA slave capabilities structure, I
> would recommend changing few.
>
> I am not sure why do you want the I/O addr to be passed here, sorry I
> don't follow that logic. If you notice in intel_mid_dma driver the I/O address
> is passed by client driver in prep_memcpy in src and dst fields. Since the slave
> structure tell you the direction and mode you can interpret that src/dst address
> as IO address easily
? Are you using memcpy() to talk to slaves?
I was assuming all slave communication was to use sglists through
the .device_prep_slave_sg() call. This is currently the design constraint,
memcpy() will by design increase both source and destination address
and also always operate on the memory bus.
(If you need reconfiguration also for memcpy() I think will be a
different issue, I'm only looking at slaves now.)
With only the slave interface the dma_chan struct can be deferred
by the DMA engine into a local struct which has this address configured
from the platform, statically.
The runtime configuration API is exactly about being able to reconfigure
even the source/destination address at runtime. This is why
these are on the interface.
> Addr_width and Maxburst: talks about the I/O width and msize I guess, but what
> about mem-width, I have few configuration where we would like to have different
> mem width as well.
OK then we need that for both src and dst.
> And why limit the generic API
Just trying to see how small we can get it, less things can go wrong.
> and what about per-per transfers which
> intel_mid_dma driver would need to support in future.
Yep per2per will definately need that.
> I would recommend renaming this field to src_addr_width
> etc and add dst_addr_xxx
> pair.
OK if Dan agrees I'll make a try.
> Yes I can have them in private_config but then again driver has to do a simple
> check on which one is src/dst in slave and which one is in privae_config. Again
> easily doable but little confusing, I am okay either way
No if you're already foreseeing that they'll be needed, lets put
them in from the beginning...
Yours,
Linus Walleij
> ? Are you using memcpy() to talk to slaves?
Yes, I don't have sg support yet, that's something I need to add next.
> I was assuming all slave communication was to use sglists through
> the .device_prep_slave_sg() call. This is currently the design constraint,
> memcpy() will by design increase both source and destination address
> and also always operate on the memory bus.
>
> (If you need reconfiguration also for memcpy() I think will be a
> different issue, I'm only looking at slaves now.)
>
> With only the slave interface the dma_chan struct can be deferred
> by the DMA engine into a local struct which has this address configured
> from the platform, statically.
>
> The runtime configuration API is exactly about being able to reconfigure
> even the source/destination address at runtime. This is why
> these are on the interface.
Okay looking at the sg API, now I can understand why you need the address in
this structure, I would also need that in future.
One suggestion since we are giving io address here, how about naming the
variable as io_addr, and we can add comment to be used for sg operations as io
addr if anyone wants to use memcpy() they can ignore this
Thanks
Vinod