On Fri, 2012-04-20 at 17:46 +0530, Laxman Dewangan wrote: > Thanks Vinod for quick review. Since I was on vacation, I hadn't noticed Russell has already sent the patches for omap dma support. http://permalink.gmane.org/gmane.linux.ports.arm.omap/75034 It would be nice if both the efforts are coordinated. Btw I like the virtual channel support introduced by Russell > > On Friday 20 April 2012 04:44 PM, Vinod Koul wrote: > > On Fri, 2012-04-20 at 14:38 +0530, Laxman Dewangan wrote: > > + * dma_transfer_mode: Different dma transfer mode. > >> + * DMA_MODE_ONCE: Dma transfer the configured buffer once and at the end of > >> + * transfer, dma stops automatically and generates interrupt > >> + * if enabled. SW need to reprogram dma for next transfer. > >> + * DMA_MODE_CYCLE: Dma keeps transferring the same buffer again and again > >> + * until dma stopped explicitly by SW or another buffer configured. > >> + * After transfer completes, dma again starts transfer from > >> + * beginning of buffer without sw intervention. If any new > >> + * address/size is configured during buffer transfer then > >> + * dma start transfer with new configuration otherwise it > >> + * will keep transferring with old configuration. It also > >> + * generates the interrupt after buffer transfer completes. > > why do you need to define this? use the cyclic api to convey this > > This is not the public definition, only used in dma_driver locally. The > tegra dma support the cyclic mode in two ways; > Cyclic single interrupt mode on which it generates interrupt once full > buffer transfer completes and hw keep transferring the data from start > of buffer without sw intervention. > Cyclic double interrupt mode on which it generates interrupt two times, > once after half buffer and second after full buffer. The hw keep > transferring buffer in cyclic manner. > > I am using these mode based on how cyclic parameter is passed from client. > If period_len is half of buffer len the using cyclic double interrupt > mode and hence configure dma once for two interrupt. > For other cases, I am using the cyclic single interrupt mode. > > > > >> + * DMA_MODE_CYCLE_HALF_NOTIFY: In this mode dma keeps transferring the buffer > >> + * into two folds. This is kind of ping-pong buffer where both > >> + * buffer size should be same. Dma completes the one buffer, > >> + * generates interrupt and keep transferring the next buffer > >> + * whose address start just next to first buffer. At the end of > >> + * second buffer transfer, dma again generates interrupt and > >> + * keep transferring of the data from starting of first buffer. > >> + * If sw wants to change the address/size of the buffer then > >> + * it needs to change only when dma transferring the second > >> + * half of buffer. In dma configuration, it only need to > >> + * configure starting of first buffer and size of first buffer. > >> + * Dma hw assumes that striating address of second buffer is just > >> + * next to end of first buffer and size is same as the first > >> + * buffer. > > isnt this a specifc example of cylci and frankly why should dmaengine > > care about this. This one of the configurations you are passing for a > > cyclic dma operation > > No special configuration is passed. Just based on buf_len and > period_len, the mode get selected. > > >> + */ > >> +enum dma_transfer_mode { > >> + DMA_MODE_NONE, > >> + DMA_MODE_ONCE, > >> + DMA_MODE_CYCLE, > >> + DMA_MODE_CYCLE_HALF_NOTIFY, > >> +}; > >> + > >> +/* List of memory allocated for that channel */ > >> +struct tegra_dma_chan_mem_alloc { > >> + struct list_head node; > >> +}; > > this seems questionable too... > > When we allocate the channel, initially allocate some descriptors with > some initial count. If client has more request and if it is found out of > descriptor then we reallocate the some more descriptors. All these > allocations are dynamically and when Chanel got released, it frees all > allocations. > This structure is to maintain the list of memory pointers which are > allocated. > > here I am allocating chunk of descriptor in one shot, not one by one. If > I allocate descriptor one by one then I will not need this structure. > tried to optimize the malloc call here. > > > >> + * The client's request for data transfer can be broken into multiple > >> + * sub-transfer as per requestor details and hw support. > > typo ^^^^^^^^^ > > Will fix this. > > >> + * tegra_dma_desc: Tegra dma descriptors which manages the client requests. > >> + * This de scripts keep track of transfer status, callbacks, transfer and > > again ^^^^ > Will fix this, thanks for pointing. My spell check did not found it. > > > >> + > >> +static dma_cookie_t tegra_dma_tx_submit(struct dma_async_tx_descriptor *tx); > >> + > >> +static int allocate_tegra_desc(struct tegra_dma_channel *tdc, > >> + int ndma_desc, int nsg_req) > > what does the last arg mean? > > This is number of transfer request. So if client call the prep_slave or > prep_dma_cyclic with multiple segment/period len then this structure > will contain the details of sub transfer per segment/period_len. > And it allocate one main dma_descriptor which have the transfer > descriptor. So one dma_desc which is allocated one per call will contain > list of such transfer requests. > > >> + INIT_LIST_HEAD(&sg_req_list); > >> + > >> + /* Calculate total require size of memory and then allocate */ > >> + dma_desc_size = sizeof(struct tegra_dma_desc) * ndma_desc; > >> + sg_req_size = sizeof(struct tegra_dma_sg_req) * nsg_req; > >> + chan_mem_size = sizeof(struct tegra_dma_chan_mem_alloc); > >> + total_size = chan_mem_size + dma_desc_size + sg_req_size; > > why cant you simply allocate three structs you need? > > So allocation is done for number of dma desc and number of request > structure and the structure which keeps track for allocated pointers. > Here I am calculating total allocation size and then allocating once in > place of allocating them in loop. > > >> +static int tegra_dma_slave_config(struct dma_chan *dc, > >> + struct dma_slave_config *sconfig) > >> +{ > >> + struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc); > >> + > >> + if (!list_empty(&tdc->pending_sg_req)) { > >> + dev_err(tdc2dev(tdc), > >> + "dma requests are pending, cannot take new configuration"); > >> + return -EBUSY; > >> + } > >> + > >> + /* Slave specific configuration is must for channel configuration */ > >> + if (!dc->private) { > > private is deprecated, pls dont use that > > OK, I saw this in the linux-next and hence I used it. So is there any > way to send the client specific data to the dma driver. > I will remove this. > > > >> + > >> + apb_seq = APB_SEQ_WRAP_WORD_1; > >> + > >> + switch (direction) { > >> + case DMA_MEM_TO_DEV: > >> + apb_ptr = tdc->dma_sconfig.dst_addr; > >> + apb_seq |= get_bus_width(tdc->dma_sconfig.dst_addr_width); > >> + csr |= CSR_DIR; > >> + break; > >> + > >> + case DMA_DEV_TO_MEM: > >> + apb_ptr = tdc->dma_sconfig.src_addr; > >> + apb_seq |= get_bus_width(tdc->dma_sconfig.src_addr_width); > >> + break; > > you dont support DMA_MEM_TO_DEV? > > > > Supported, first case ;-) > But MEM_TO_MEM is not supported by this dma controller. > > > > >> + if (!period_len) > >> + period_len = buf_len; > > i am not sure about this assignment here. Why should period length be > > ZERO? > > > > Just in case, if some client send it to ZERO then setting it to buf_len > in place of returning error. > > > >> + > >> + if (buf_len % period_len) { > >> + dev_err(tdc2dev(tdc), > >> + "buf_len %d should be multiple of period_len %d\n", > >> + buf_len, period_len); > >> + return NULL; > >> + } > > I am assuming you are also putting this as a constraint in sound driver. > > > > Yaah, I think sound driver make sure that the buf_len should be integer > multiple period_len. Not supporting if it is not to reduce the complexity. > > >> + > >> + half_buffer_notify = (buf_len == (2 * period_len)) ? true : false; > >> + len = (half_buffer_notify) ? buf_len / 2 : period_len; > >> + if ((len& 3) || (buf_addr& 3) || > >> + (len> tdc->tdma->chip_data.max_dma_count)) { > >> + dev_err(tdc2dev(tdc), > >> + "Dma length/memory address is not correct\n"); > > not supported would be apt > > Fine. I will do it. > > >> +#ifndef LINUX_TEGRA_DMA_H > >> +#define LINUX_TEGRA_DMA_H > >> + > >> +/* > >> + * tegra_dma_burst_size: Burst size of dma. > >> + * @TEGRA_DMA_AUTO: Based on transfer size, select the burst size. > >> + * If it is multple of 32 bytes then burst size will be 32 bytes else > >> + * If it is multiple of 16 bytes then burst size will be 16 bytes else > >> + * If it is multiple of 4 bytes then burst size will be 4 bytes. > >> + * @TEGRA_DMA_BURST_1: Burst size is 1 word/4 bytes. > >> + * @TEGRA_DMA_BURST_4: Burst size is 4 word/16 bytes. > >> + * @TEGRA_DMA_BURST_8: Burst size is 8 words/32 bytes. > >> + */ > >> +enum tegra_dma_burst_size { > >> + TEGRA_DMA_AUTO, > >> + TEGRA_DMA_BURST_1, > >> + TEGRA_DMA_BURST_4, > >> + TEGRA_DMA_BURST_8, > >> +}; > > why should this be global, clinet should pass them as defined in > > dmaengine.h > > The dma_slave_config on the dmaengine have the member as src_maxburst > and I understand that this is for maximum burst only. > and so passing the actual burst size, I defined new enums. Also wanted > to have the auto mode where I can select the BURST size based on the > request length. > if some encoding and understanding is maintain between client and dma > driver then I can get rid of this: > like src_maxburst = 0 is for the selection of burst size based on req > len otherwise use non-zero value of src_maxburst. > > > > >> + * @dma_dev: required DMA master client device. > >> + * @dm_req_id: Peripheral dma requestor ID. > >> + */ > >> +struct tegra_dma_slave { > >> + struct device *client_dev; > >> + enum tegra_dma_requestor dma_req_id; > >> + enum tegra_dma_burst_size burst_size; > > pls remove > > if above is OK then I can remove this. > > >> +}; > >> + > >> +#endif /* LINUX_TEGRA_DMA_H */ > > Please also update the driver to use the cookie helpers in > > drivers/dma/dmaengine.h > > > > I have already used cookie helper. > -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
