Hi Vinod,
On Mon, May 18, 2015 at 02:16:14PM +0530, Vinod Koul wrote:
> On Mon, May 11, 2015 at 03:27:32PM +0200, Maxime Ripard wrote:
> > +
> > +/** Normal DMA register values **/
> > +
> > +/* Normal DMA source/destination data request type values */
> > +#define NDMA_DRQ_TYPE_SDRAM 0x16
> > +#define NDMA_DRQ_TYPE_LIMIT (0x1F + 1)
> > +
> > +/** Normal DMA register layout **/
> > +
> > +/* Normal DMA configuration register layout */
> > +#define NDMA_CFG_LOADING BIT(31)
> > +#define NDMA_CFG_CONT_MODE BIT(30)
> > +#define NDMA_CFG_WAIT_STATE(n) ((n) << 27)
> > +#define NDMA_CFG_DEST_DATA_WIDTH(width) ((width) << 25)
> > +#define NDMA_CFG_DEST_BURST_LENGTH(len) ((len) << 23)
> > +#define NDMA_CFG_DEST_NON_SECURE BIT(22)
> > +#define NDMA_CFG_DEST_FIXED_ADDR BIT(21)
> > +#define NDMA_CFG_DEST_DRQ_TYPE(type) ((type) << 16)
> > +#define NDMA_CFG_BYTE_COUNT_MODE_REMAIN BIT(15)
> > +#define NDMA_CFG_SRC_DATA_WIDTH(width) ((width) << 9)
> > +#define NDMA_CFG_SRC_BURST_LENGTH(len) ((len) << 7)
> > +#define NDMA_CFG_SRC_NON_SECURE BIT(6)
> > +#define NDMA_CFG_SRC_FIXED_ADDR BIT(5)
> > +#define NDMA_CFG_SRC_DRQ_TYPE(type) ((type) << 0)
> > +
> > +/** Dedicated DMA register values **/
> > +
> > +/* Dedicated DMA source/destination address mode values */
> > +#define DDMA_ADDR_MODE_LINEAR 0
> > +#define DDMA_ADDR_MODE_IO 1
> > +#define DDMA_ADDR_MODE_HORIZONTAL_PAGE 2
> > +#define DDMA_ADDR_MODE_VERTICAL_PAGE 3
> > +
> > +/* Dedicated DMA source/destination data request type values */
> > +#define DDMA_DRQ_TYPE_SDRAM 0x1
> > +#define DDMA_DRQ_TYPE_LIMIT (0x1F + 1)
> > +
> > +/** Dedicated DMA register layout **/
> > +
> > +/* Dedicated DMA configuration register layout */
> > +#define DDMA_CFG_LOADING BIT(31)
> > +#define DDMA_CFG_BUSY BIT(30)
> > +#define DDMA_CFG_CONT_MODE BIT(29)
> > +#define DDMA_CFG_DEST_NON_SECURE BIT(28)
> > +#define DDMA_CFG_DEST_DATA_WIDTH(width) ((width) << 25)
> > +#define DDMA_CFG_DEST_BURST_LENGTH(len) ((len) << 23)
> > +#define DDMA_CFG_DEST_ADDR_MODE(mode) ((mode) << 21)
> > +#define DDMA_CFG_DEST_DRQ_TYPE(type) ((type) << 16)
> > +#define DDMA_CFG_BYTE_COUNT_MODE_REMAIN BIT(15)
> > +#define DDMA_CFG_SRC_NON_SECURE BIT(12)
> > +#define DDMA_CFG_SRC_DATA_WIDTH(width) ((width) << 9)
> > +#define DDMA_CFG_SRC_BURST_LENGTH(len) ((len) << 7)
> > +#define DDMA_CFG_SRC_ADDR_MODE(mode) ((mode) << 5)
> > +#define DDMA_CFG_SRC_DRQ_TYPE(type) ((type) << 0)
> > +
> > +/* Dedicated DMA parameter register layout */
> > +#define DDMA_PARA_DEST_DATA_BLK_SIZE(n) (((n) - 1) << 24)
> > +#define DDMA_PARA_DEST_WAIT_CYCLES(n) (((n) - 1) << 16)
> > +#define DDMA_PARA_SRC_DATA_BLK_SIZE(n) (((n) - 1) << 8)
> > +#define DDMA_PARA_SRC_WAIT_CYCLES(n) (((n) - 1) << 0)
> > +
> > +/** DMA register offsets **/
> > +
> > +/* General register offsets */
> > +#define DMA_IRQ_ENABLE_REG 0x0
> > +#define DMA_IRQ_PENDING_STATUS_REG 0x4
> > +
> > +/* Normal DMA register offsets */
> > +#define NDMA_CHANNEL_REG_BASE(n) (0x100 + (n) * 0x20)
> > +#define NDMA_CFG_REG 0x0
> > +#define NDMA_SRC_ADDR_REG 0x4
> > +#define NDMA_DEST_ADDR_REG 0x8
> > +#define NDMA_BYTE_COUNT_REG 0xC
> > +
> > +/* Dedicated DMA register offsets */
> > +#define DDMA_CHANNEL_REG_BASE(n) (0x300 + (n) * 0x20)
> > +#define DDMA_CFG_REG 0x0
> > +#define DDMA_SRC_ADDR_REG 0x4
> > +#define DDMA_DEST_ADDR_REG 0x8
> > +#define DDMA_BYTE_COUNT_REG 0xC
> > +#define DDMA_PARA_REG 0x18
>
> the defines here NDMA_, DDMA_, DMA_ are too generic names and not specific to
> this driver, they can cause collisions in future with others, so lets make
> them future proof please
Good point, I'll change that.
> > +static int choose_optimal_buswidth(dma_addr_t addr)
> > +{
> > + /* On 32 bit aligned addresses, we can use a 32 bit bus width */
> > + if (addr % 4 == 0)
> > + return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + /* On 16 bit aligned addresses, we can use a 16 bit bus width */
> > + else if (addr % 2 == 0)
> > + return DMA_SLAVE_BUSWIDTH_2_BYTES;
> > +
> > + /* Worst-case scenario, we need to do byte aligned reads */
> > + return DMA_SLAVE_BUSWIDTH_1_BYTE;
>
> am not sure if that is right way, dmaengine drivers shouldn't assume and use
> a parameter as they will conflict with client FIFO settings and make whole
> data garbage
It's only used for memcpy, but I guess that since we set copy_align to
32 bits, we can assume that we're going to use a bus width of 32
bits..
> > +static struct sun4i_dma_promise *
> > +generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
> > + size_t len, struct dma_slave_config *sconfig)
> > +{
> > + struct sun4i_dma_promise *promise;
> > + int ret;
> > +
> > + promise = kzalloc(sizeof(*promise), GFP_NOWAIT);
> > + if (!promise)
> > + return NULL;
> > +
> > + promise->src = src;
> > + promise->dst = dest;
> > + promise->len = len;
> > + promise->cfg = NDMA_CFG_LOADING | NDMA_CFG_BYTE_COUNT_MODE_REMAIN;
> > +
> > + /* Use sensible default values if client is using undefined ones */
> > + if (sconfig->src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > + sconfig->src_addr_width = sconfig->dst_addr_width;
> > + if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> > + sconfig->dst_addr_width = sconfig->src_addr_width;
> > + if (sconfig->src_maxburst == 0)
> > + sconfig->src_maxburst = sconfig->dst_maxburst;
> > + if (sconfig->dst_maxburst == 0)
> > + sconfig->dst_maxburst = sconfig->src_maxburst;
>
> oh no, we certainly don't want to do that, explicit failure, fixing and
> matching with clients is the right approach here
Ok.
> > +static struct dma_async_tx_descriptor *
> > +sun4i_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf, size_t len,
> > + size_t period_len, enum dma_transfer_direction dir,
> > + unsigned long flags)
> > +{
> > + struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
> > + struct dma_slave_config *sconfig = &vchan->cfg;
> > + struct sun4i_dma_promise *promise;
> > + struct sun4i_dma_contract *contract;
> > + dma_addr_t src, dest;
> > + u32 endpoints;
> > + int nr_periods, offset, plength, i;
> > +
> > + if (!is_slave_direction(dir)) {
> > + dev_err(chan2dev(chan), "Invalid DMA direction\n");
> > + return NULL;
> > + }
> > +
> > + if (vchan->is_dedicated) {
> > + /*
> > + * As we are using this just for audio data, we need to use
> > + * normal DMA. There is nothing stopping us from supporting
> > + * dedicated DMA here as well, so if a client comes up and
> > + * requires it, it will be simple to implement it.
> > + */
> > + dev_err(chan2dev(chan),
> > + "Cyclic transfers are only supported on Normal DMA\n");
> > + return NULL;
> > + }
> > +
> > + contract = generate_dma_contract();
> > + if (!contract)
> > + return NULL;
> > +
> > + contract->is_cyclic = 1;
> > +
> > + /* Figure out the endpoints and the address we need */
> > + if (dir == DMA_MEM_TO_DEV) {
> > + src = buf;
> > + dest = sconfig->dst_addr;
> > + endpoints = NDMA_CFG_SRC_DRQ_TYPE(NDMA_DRQ_TYPE_SDRAM) |
> > + NDMA_CFG_DEST_DRQ_TYPE(vchan->endpoint) |
> > + NDMA_CFG_DEST_FIXED_ADDR;
> > + } else {
> > + src = sconfig->src_addr;
> > + dest = buf;
> > + endpoints = NDMA_CFG_SRC_DRQ_TYPE(vchan->endpoint) |
> > + NDMA_CFG_SRC_FIXED_ADDR |
> > + NDMA_CFG_DEST_DRQ_TYPE(NDMA_DRQ_TYPE_SDRAM);
> > + }
> > +
> > + /*
> > + * We will be using half done interrupts to make two periods
> > + * out of a promise, so we need to program the DMA engine less
> > + * often
> > + */
> > + nr_periods = DIV_ROUND_UP(len / period_len, 2);
>
> and why is that.. why don't you use actual period count here?
I must admit I don't really know on this one.
Emilio?
> > +static struct dma_async_tx_descriptor *
> > +sun4i_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> > + unsigned int sg_len, enum dma_transfer_direction dir,
> > + unsigned long flags, void *context)
> > +{
> > + struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
> > + struct dma_slave_config *sconfig = &vchan->cfg;
> > + struct sun4i_dma_promise *promise;
> > + struct sun4i_dma_contract *contract;
> > + struct scatterlist *sg;
> > + dma_addr_t srcaddr, dstaddr;
> > + u32 endpoints, para;
> > + int i;
> > +
> > + if (!sgl)
> > + return NULL;
> > +
> > + if (!is_slave_direction(dir)) {
> > + dev_err(chan2dev(chan), "Invalid DMA direction\n");
> > + return NULL;
> > + }
> > +
> > + contract = generate_dma_contract();
> > + if (!contract)
> > + return NULL;
> > +
> > + /* Figure out endpoints */
> > + if (vchan->is_dedicated && dir == DMA_MEM_TO_DEV) {
> > + endpoints = DDMA_CFG_SRC_DRQ_TYPE(DDMA_DRQ_TYPE_SDRAM) |
> > + DDMA_CFG_SRC_ADDR_MODE(DDMA_ADDR_MODE_LINEAR) |
> > + DDMA_CFG_DEST_DRQ_TYPE(vchan->endpoint) |
> > + DDMA_CFG_DEST_ADDR_MODE(DDMA_ADDR_MODE_IO);
> > + } else if (!vchan->is_dedicated && dir == DMA_MEM_TO_DEV) {
> > + endpoints = NDMA_CFG_SRC_DRQ_TYPE(NDMA_DRQ_TYPE_SDRAM) |
> > + NDMA_CFG_DEST_DRQ_TYPE(vchan->endpoint) |
> > + NDMA_CFG_DEST_FIXED_ADDR;
> > + } else if (vchan->is_dedicated) {
> > + endpoints = DDMA_CFG_SRC_DRQ_TYPE(vchan->endpoint) |
> > + DDMA_CFG_SRC_ADDR_MODE(DDMA_ADDR_MODE_IO) |
> > + DDMA_CFG_DEST_DRQ_TYPE(DDMA_DRQ_TYPE_SDRAM) |
> > + DDMA_CFG_DEST_ADDR_MODE(DDMA_ADDR_MODE_LINEAR);
> > + } else {
> > + endpoints = NDMA_CFG_SRC_DRQ_TYPE(vchan->endpoint) |
> > + NDMA_CFG_SRC_FIXED_ADDR |
> > + NDMA_CFG_DEST_DRQ_TYPE(NDMA_DRQ_TYPE_SDRAM);
> > + }
>
> I would like this to be reworked a bit with mapping of each endpoint
> configuration to setting like dedicated and dir, that way adding future
> value gets simpler by adding one more case rather than dense bring here
I don't really follow you on this one. You mean adding a
macro/function to generate the endpoints value?
> > +
> > + for_each_sg(sgl, sg, sg_len, i) {
> > + /* Figure out addresses */
> > + if (dir == DMA_MEM_TO_DEV) {
> > + srcaddr = sg_dma_address(sg);
> > + dstaddr = sconfig->dst_addr;
> > + } else {
> > + srcaddr = sconfig->src_addr;
> > + dstaddr = sg_dma_address(sg);
> > + }
> > +
> > + /*
> > + * These are the magic DMA engine timings that keep SPI going.
> > + * I haven't seen any interface on DMAEngine to configure
> > + * timings, and so far they seem to work for everything we
> > + * support, so I've kept them here. I don't know if other
> > + * devices need different timings because, as usual, we only
> > + * have the "para" bitfield meanings, but no comment on what
> > + * the values should be when doing a certain operation :|
> > + */
> > + para = DDMA_MAGIC_SPI_PARAMETERS;
>
> Is there no documentation available from vendor on what this means ...?
There's only a partial documentation on that engine. The current
driver and these opaque parameters, whatever they mean, seem to work
fine with SPI and the audio codec so far, so this seems like a good
enough value.
But we might have to change this in the future when we will use more
devices with this DMA controller.
> > +static enum dma_status sun4i_dma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *state)
> > +{
> > + struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
> > + struct sun4i_dma_pchan *pchan = vchan->pchan;
> > + struct sun4i_dma_contract *contract;
> > + struct sun4i_dma_promise *promise;
> > + struct virt_dma_desc *vd;
> > + unsigned long flags;
> > + enum dma_status ret;
> > + size_t bytes = 0;
> > +
> > + ret = dma_cookie_status(chan, cookie, state);
> > + if (ret == DMA_COMPLETE)
> > + return ret;
>
> Pls check if state is valid before progressing ahead
Ok.
> > +
> > + spin_lock_irqsave(&vchan->vc.lock, flags);
> > + vd = vchan_find_desc(&vchan->vc, cookie);
> > + if (!vd)
> > + goto exit;
> > + contract = to_sun4i_dma_contract(vd);
> > +
> > + list_for_each_entry(promise, &contract->demands, list)
> > + bytes += promise->len;
>
> one thing which might help here is to keep this value in vd or perhaps
> pre-compute this during the prepare
Except that you don't really know where you are in the transfer, and
most likely you'll be in the middle of the transfer. The best we could
do is using the interrupts to increment the count at each chunk
transferred, but that doesn't seem very efficient.
We'll still have to go through the LLI chunks, but that's true that we
can probably rework this logic.
> > +static int sun4i_dma_remove(struct platform_device *pdev)
> > +{
> > + struct sun4i_dma_dev *priv = platform_get_drvdata(pdev);
> > +
> > + /* Disable IRQ so no more work is scheduled */
> > + disable_irq(priv->irq);
> > +
> > + of_dma_controller_free(pdev->dev.of_node);
> > + dma_async_device_unregister(&priv->slave);
> > +
> > + clk_disable_unprepare(priv->clk);
>
> you still have irq enabled here, expilcit call to free_irq helps that as well
> as ensuring all the tasklets are executed before you remove the device
Which tasklets? We don't use any in our driver.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
