Prev: [PATCH] cifs: remove an potentially confusing, obsolete comment
Next: Problem with tty changes in linux-next
From: Linus Walleij on 6 Jul 2010 08:40
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 <linus.walleij(a)stericsson.com>:
> 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 <linus.walleij(a)stericsson.com>
> ---
> �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 majordomo(a)vger.kernel.org
> More majordomo info at �http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at �http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 14 Jul 2010 19:20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 19 Jul 2010 18:50
2010/7/19 Dan Williams <dan.j.williams(a)intel.com>:

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 19 Jul 2010 19:00
On Mon, Jul 19, 2010 at 3:44 PM, Linus Walleij
<linus.ml.walleij(a)gmail.com> wrote:
> 2010/7/19 Dan Williams <dan.j.williams(a)intel.com>:
>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Koul, Vinod on 19 Jul 2010 23:50
> On Mon, Jul 19, 2010 at 3:44 PM, Linus Walleij
> <linus.ml.walleij(a)gmail.com> wrote:
> > 2010/7/19 Dan Williams <dan.j.williams(a)intel.com>:
> >
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH] cifs: remove an potentially confusing, obsolete comment
Next: Problem with tty changes in linux-next