Message-ID: <4F9153AF.7020901@nvidia.com>
Date:	Fri, 20 Apr 2012 17:46:47 +0530
From:	Laxman Dewangan <ldewangan@...dia.com>
To:	Vinod Koul <vinod.koul@...ux.intel.com>
CC:	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	Stephen Warren <swarren@...dia.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH V1] dmaengine: tegra: add dma driver

Thanks Vinod for quick review.

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists