Re: [PATCH V3 2/2] dmaengine: Add driver for IMG MDC

[Vinod Koul <vinod.koul@intel.com>]

On Thu, Dec 11, 2014 at 02:59:17PM -0800, Andrew Bresticker wrote:
> Add support for the IMG Multi-threaded DMA Controller (MDC) found on
> certain IMG SoCs.  Currently this driver supports the variant present
> on the MIPS-based Pistachio SoC.a

Overall looks okay. I also need some review by DT folks on the bindings

> +static void mdc_list_desc_config(struct mdc_chan *mchan,
> +				 struct mdc_hw_list_desc *ldesc,
> +				 enum dma_transfer_direction dir,
> +				 dma_addr_t src, dma_addr_t dst, size_t len)
> +{
> +	struct mdc_dma *mdma = mchan->mdma;
> +	unsigned int max_burst, burst_size;
> +
> +	ldesc->gen_conf = MDC_GENERAL_CONFIG_IEN | MDC_GENERAL_CONFIG_LIST_IEN |
> +		MDC_GENERAL_CONFIG_LEVEL_INT | MDC_GENERAL_CONFIG_PHYSICAL_W |
> +		MDC_GENERAL_CONFIG_PHYSICAL_R;
> +	ldesc->readport_conf =
> +		(mchan->thread << MDC_READ_PORT_CONFIG_STHREAD_SHIFT) |
> +		(mchan->thread << MDC_READ_PORT_CONFIG_RTHREAD_SHIFT) |
> +		(mchan->thread << MDC_READ_PORT_CONFIG_WTHREAD_SHIFT);
> +	ldesc->read_addr = src;
> +	ldesc->write_addr = dst;
> +	ldesc->xfer_size = len - 1;
> +	ldesc->node_addr = 0;
> +	ldesc->cmds_done = 0;
> +	ldesc->ctrl_status = MDC_CONTROL_AND_STATUS_LIST_EN |
> +		MDC_CONTROL_AND_STATUS_EN;
> +	ldesc->next_desc = NULL;
> +
> +	if (IS_ALIGNED(dst, mdma->bus_width) &&
> +	    IS_ALIGNED(src, mdma->bus_width))
> +		max_burst = mdma->bus_width * mdma->max_burst_mult;
> +	else
> +		max_burst = mdma->bus_width * (mdma->max_burst_mult - 1);
> +
> +	if (dir == DMA_MEM_TO_DEV) {
> +		ldesc->gen_conf |= MDC_GENERAL_CONFIG_INC_R;
> +		ldesc->readport_conf |= MDC_READ_PORT_CONFIG_DREQ_ENABLE;
> +		mdc_set_read_width(ldesc, mdma->bus_width);
> +		mdc_set_write_width(ldesc, mchan->config.dst_addr_width);
> +		burst_size = min(max_burst, mchan->config.dst_maxburst *
> +				 mchan->config.dst_addr_width);
why is this calculation done for burst size? Shouldn't we take the
config.dst_maxburst value configured by client?


> +static struct dma_async_tx_descriptor *mdc_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 mdc_chan *mchan = to_mdc_chan(chan);
> +	struct mdc_dma *mdma = mchan->mdma;
> +	struct mdc_tx_desc *mdesc;
> +	struct scatterlist *sg;
> +	struct mdc_hw_list_desc *curr, *prev = NULL;
> +	dma_addr_t curr_phys, prev_phys;
> +	unsigned int i;
> +
> +	if (!sgl)
> +		return NULL;
> +
> +	if (!is_slave_direction(dir))
> +		return NULL;
> +
> +	if (mdc_check_slave_width(mchan, dir) < 0)
> +		return NULL;
> +
> +	mdesc = kzalloc(sizeof(*mdesc), GFP_NOWAIT);
> +	if (!mdesc)
> +		return NULL;
> +	mdesc->chan = mchan;
> +
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		dma_addr_t buf = sg_dma_address(sg);
> +		size_t buf_len = sg_dma_len(sg);
> +
> +		while (buf_len > 0) {
> +			size_t xfer_size;
> +
> +			curr = dma_pool_alloc(mdma->desc_pool, GFP_NOWAIT,
> +					      &curr_phys);
> +			if (!curr)
> +				goto free_desc;
> +
> +			if (!prev) {
> +				mdesc->list_phys = curr_phys;
> +				mdesc->list = curr;
> +			} else {
> +				prev->node_addr = curr_phys;
> +				prev->next_desc = curr;
> +			}
> +
> +			xfer_size = min_t(size_t, mdma->max_xfer_size,
> +					  buf_len);
> +
> +			if (dir == DMA_MEM_TO_DEV) {
> +				mdc_list_desc_config(mchan, curr, dir, buf,
> +						     mchan->config.dst_addr,
> +						     xfer_size);
> +			} else {
> +				mdc_list_desc_config(mchan, curr, dir,
> +						     mchan->config.src_addr,
> +						     buf, xfer_size);
> +			}
> +
> +			prev = curr;
> +			prev_phys = curr_phys;
> +
> +			mdesc->list_len++;
> +			mdesc->list_xfer_size += xfer_size;
> +			buf += xfer_size;
> +			buf_len -= xfer_size;
i see this pattern is repeat in all the .prepare calls, can we make it bit
generic and use that in the these three calls..

> +	dma_cap_zero(mdma->dma_dev.cap_mask);
> +	dma_cap_set(DMA_SLAVE, mdma->dma_dev.cap_mask);
> +	dma_cap_set(DMA_PRIVATE, mdma->dma_dev.cap_mask);
> +	dma_cap_set(DMA_CYCLIC, mdma->dma_dev.cap_mask);
and you dont seen to support pause/resume, though not a blocker but is it
not supported in HW or driver doesn't?

-- 
~Vinod