Re: [RFC]DMA API Update


Piotr MieĊ„kowski <piotr.mienkowski at gmail.com...>
 

Hi Baohong,

Thanks for preparing the new DMA API and am very sorry for not replying
earlier. I was busy with other things. I do have some comments to the
proposal.

* the callback data pointer was removed, the reasoning was as follows:

"Currently, this field holds user data. It can be moved to driver
data structure. The callback function can dig out the information
from dev pointer and channel id passed to it."

I believe that's not correct. The callback function will have no way
to dig out the required information from the DMA dev pointer. The
original idea as described in the Jira issue by Tomasz Bursztyka was
different.

"Instead, the callback would provide the pointer of struct
dma_channel_config which triggered it. Then, up to the user to
relevantly design his own data structure to be able to use
CONTAINER_OF() with that pointer."

So we would need to pass a pointer to dma_config. I'm however not
sure if that's really the right way to go. Using CONTAINER_OF() is
an extremely clever idea to save a byte of RAM but also one which
will keep many users scratching their heads. We are talking here
about public API, one which should be straightforward to use and
accessible also to less experienced users. Maybe the original
concept was better?

* Often, when working with DMA, we need to configure DMA channel once
and then perform several transfers where the only difference to the
last setup is the location of the source or the destination buffer.
Yet again I think the original concept was a little bit better as it
split channel configuration in two parts exactly along this lines.
That way we didn't have to go through a somewhat lengthy process of
fully reconfiguring the DMA channel when we wanted to change source
address only. One could argue that it's what linked lists are for
but in practice we do not always know up front what will be the
location of the next buffer so setting up a linked list transfer is
not always an option. I'm not saying that we need to go to the old
dma_api_channel_config/dma_api_transfer_config split. We may try to
think about something else, e.g. have dma_start take something like
the old dma_transfer_config struct as a parameter.

* I know that this comment is coming very late but I was wondering if
we really want the device drivers to use this DMA API? Device
drivers are always written for a specific SoC and their code doesn't
need to be portable between architectures, i.e. Quark DMA driver
will never be used with Atmel DAC driver. When writing Atmel or
Quark DAC driver I would rather access private API of Atmel/Quark
DMA module and not this one. Private APIs correlate well with the
documentation, put no limits on features the hardware module can
offer and add only a thin layer of code. This API has to be
universal and that's good but there is a price to pay. All the
various bit field parameters which we defined will have to be
translated to the device native format. Cutting a bit field and
moving it elsewhere is actually not a very expensive operation but
we have a lot of parameters to process and not all of them will be
straightforward to translate. So this API adds a relatively thick
layer of code. Maybe it would be better if it was targeted
exclusively at user space programs? The only use case involving a
user space program and DMA driver that I can think of is a memory to
memory transfer. Any memory to peripheral or peripheral to memory
transfer will be handled by a respective device driver. Or am I
wrong on this one? Is one driver accessing other driver via a
private and not public API violating some Zephyr design guideline?

Regards,
Piotr

On 01/23/2017 10:53 PM, Liu, Baohong wrote:
Hi everyone,

Based on the feedbacks, the following is the final format.

Data Structure Definitions
-------

/**
* @brief DMA block configuration structure.
*
* source_address is block starting address at source
* source_gather_interval is the address adjustment at gather boundary
* dest_address is block starting address at destination
* dest_scatter_interval is the address adjustment at scatter boundary
* dest_scatter_count is the continuous transfer count between scatter boundaries
* source_gather_count is the continuous transfer count between gather boundaries
* block_size is the number of bytes to be transferred for this block.
* config is a bit field with the following parts:
* source_gather_en [ 0 ] --0-disable, 1-enable
* dest_scatter_en [ 1 ] --0-disable, 1-enable
* source_addr_adj [ 2 : 3 ] --00-increment, 01-decrement, 10-no change
* dest_addr_adj [ 4 : 5 ] --00-increment, 01-decrement, 10-no change
* source_reload_en [ 6 ] --reload source address at the end of block transfer
* 0-disable, 1-enable
* dest_reload_en [ 7 ] --reload destination address at the end of block transfer
* 0-disable, 1-enable
* fifo_mode_control [ 8 : 11 ] --How full of the fifo before transfer start. HW specific.
* flow_control_mode [ 12 ] --0-source request served upon data availability
* 1-wait until destination request happens
* RESERVED [ 13 : 15 ]
*/
struct dma_block_config {
uint32_t source_address;
uint32_t source_gather_interval;
uint32_t dest_address;
uint32_t dest_scatter_interval;
uint16_t dest_scatter_count;
uint16_t source_gather_count;
uint32_t block_size;
struct dma_block_config *next_block;
uint16_t config;
}

/**
* @brief DMA configuration structure.
*
* config is a bit field with the following parts:
* dma_slot [ 0 : 5 ] --which peripheral and direction(HW specific)
* channel_direction [ 6 : 8 ] --000-memory to memory, 001-memory to peripheral,
* 010-peripheral to memory ...
* complete_int_en [ 9 ] --completion interrupt enable
* 0-disable, 1-enable
* block_int_en [ 10 ] --block completion interrupt enable
* 0-disable, 1-enable
* source_int_en [ 11 ] --source completion interrupt enable
* 0-disable, 1-enable
* dest_int_en [ 12 ] --destination completion interrupt enable
* 0-disable, 1-enable
* error_int_en [ 13 ] --error interrupt enable
* 0-disable, 1-enable
* source_handshake [ 14 ] --0-HW, 1-SW
* dest_handshake [ 15 ] --0-HW, 1-SW
* channel_priority [ 16 : 19 ] --DMA channel priority
* source_chaining_en [ 20 ] --enable/disable source block chaining
* 0-disable, 1-enable
* dest_chaining_en [ 21 ] --enable/disable destination block chaining
* 0-disable, 1-enable
* RESERVED [ 22 : 31 ]
* config_size is a bit field with the following parts:
* source_data_size [ 0 : 7 ] --number of bytes
* dest_data_size [ 8 : 15 ] -- number of bytes
* source_burst_length [ 16 : 23 ] -- number of source data units
* dest_burst_length [ 24 : 31 ] -- number of destination data units
* dma_callback is the callback function pointer
*/
struct dma_config {
uint32_t config;
uint32_t config_size;
uint32_t block_count;
struct dma_block_config *head_block;
void (*dma_callback)(struct device *dev, uint32_t channel, int error_code);
}

API Interfaces
-------

/**
* @brief Configure individual channel for DMA transfer.
*
* @param dev Pointer to the device structure for the driver instance.
* @param channel Numeric identification of the channel to configure
* @param config Data structure containing the intended configuration for the
* selected channel
*
* @retval 0 If successful.
* @retval Negative errno code if failure.
*/
static inline int dma_config(struct device *dev, uint32_t channel,
struct dma_config *config)
{
}

/**
* @brief Enables DMA channel and starts the transfer, the channel must be
* configured beforehand.
*
* @param dev Pointer to the device structure for the driver instance.
* @param channel Numeric identification of the channel where the transfer will
* be processed
*
* @retval 0 If successful.
* @retval Negative errno code if failure.
*/
static inline int dma_start(struct device *dev, uint32_t channel)
{
}

/**
* @brief Stops the DMA transfer and disables the channel.
*
* @param dev Pointer to the device structure for the driver instance.
* @param channel Numeric identification of the channel where the transfer was
* being processed
*
* @retval 0 If successful.
* @retval Negative errno code if failure.
*/
static inline int dma_stop(struct device *dev, uint32_t channel)
{
}

All existing data structure definitions and APIs will be deprecated.

-----Original Message-----
From: Liu, Baohong
Sent: Tuesday, January 10, 2017 6:29 PM
To: 'Liu, Baohong' <baohong.liu(a)intel.com>
Subject: [RFC]DMA API Update

Hi everyone,

I would like to propose the update for DMA API.
(https://jira.zephyrproject.org/browse/ZEP-873).

Known Problems.
--------

Structure dma_channel_config consists of quite a few variables of enum type,
two callback pointers, and one callback data pointer.

- The enums can be collapsed into a bit field.
This will reduce the size of the data structure.

- The two callback pointers (for normal and error cases) can be combined.
One new callback parameter will be used to return error code.

- Callback data pointer will be removed.
Currently, this field actually holds user data. So, it can be moved to driver
data
structure. The callback function can dig out the information from dev
pointer and
channel id passed to the callback function.

Proposed Solution
--------

-- The following are the enums we have now.

handshake_interface /* which peripheral and direction*/
hankshake_polarity /* high or low*/
channel_direction /* memory to memory, memory to peripheral ... *.
Source_transfer_width /* source data size */
Destination_transfer_width /* destination data size */
Source_burst_length /* source burst length */
Destination_burst_length /* destination burst length */

All these will be collapsed into a bit field.

Besides the above, three new items will be added into the bit field.
source_handshake /* HW or SW */
dest_handshake /* HW or SW */
channel_priority /* DMA channel priority */

-- For the callback function, there will be three parameters. They are device
pointer, channel_id, and error code. As said above, the error callback will be
removed.

-- The callback_data pointer will be removed.

Final API Format
--------

/**
* @brief DMA configuration structure.
*
* config is a bit field with the following parts:
* handshake_interface [ 0 : 3 ] --which peripheral and
direction(HW specific)
* hankshake_polarity [ 4 ] --high or low
* channel_direction [ 5 : 7 ] --0-memory to memory, 1-
memory to peripheral,
* 2-peripheral to memory
* source_transfer_width [ 8 : 10 ] --source data size(8,16,32,64,128
and 256 bits)
* dest_transfer_width [ 11 : 13 ] --destination data
size(8,16,32,64,128 and 256 bits)
* source_burst_length [ 14 : 16 ] --source burst
length(1,4,8,16,32,64,128 and 256)
* dest_burst_length [ 17 : 19 ] -- destination burst
length(1,4,8,16,32,64,128 and 256)
* source_handshake [ 20 ] --HW or SW
* dest_handshake [ 21 ] --HW or SW
* channel_priority [ 22 : 24 ] --DMA channel priority
* RESERVED [ 25 : 31 ]
* dma_callback is the callback function pointer
*/
struct dma_channel_config {
uint32_t config;
void (*dma_callback)(struct device *dev, uint32_t channel, int
error_code); }

The remaining parts will stay the same.
No change to the structure dma_transfer_config.
No change to the API calls:
dma_channel_config(...)
dma_transfer_config(...)
dma_transfer_start(...)
dma_transfer_stop(...)

Suggestions and comments are welcome.

Join devel@lists.zephyrproject.org to automatically receive all group messages.