Re: [PATCH v3 2/4] dmaengine: Add STM32 MDMA driver

[Pierre Yves MORDRET <pierre-yves.mordret@st.com>]

On 08/16/2017 06:47 PM, Vinod Koul wrote:
> On Wed, Jul 26, 2017 at 11:48:20AM +0200, Pierre-Yves MORDRET wrote:
> 
>> +/* MDMA Channel x transfer configuration register */
>> +#define STM32_MDMA_CTCR(x)		(0x50 + 0x40 * (x))
>> +#define STM32_MDMA_CTCR_BWM		BIT(31)
>> +#define STM32_MDMA_CTCR_SWRM		BIT(30)
>> +#define STM32_MDMA_CTCR_TRGM_MSK	GENMASK(29, 28)
>> +#define STM32_MDMA_CTCR_TRGM(n)		(((n) & 0x3) << 28)
>> +#define STM32_MDMA_CTCR_TRGM_GET(n)	(((n) & STM32_MDMA_CTCR_TRGM_MSK) >> 28)
> 
> OK this seems oft repeated here.
> 
> So you are trying to extract the bit values and set the bit value, so why
> not this do generically...
> 
> #define STM32_MDMA_SHIFT(n)		(ffs(n) - 1))
> #define STM32_MDMA_SET(n, mask)		((n) << STM32_MDMA_SHIFT(mask))
> #define STM32_MDMA_GET(n, mask)		(((n) && mask) >> STM32_MDMA_SHIFT(mask))
> 
> Basically, u extract the shift using the mask value and ffs helping out, so
> no need to define these and reduce chances of coding errors...
> 

OK.
but I would prefer if you don't mind
#define STM32_MDMA_SET(n, mask)		(((n) << STM32_MDMA_SHIFT(mask)) & mask)

>> +static int stm32_mdma_get_width(struct stm32_mdma_chan *chan,
>> +				enum dma_slave_buswidth width)
>> +{
>> +	switch (width) {
>> +	case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> +	case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> +	case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> +	case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> +		return ffs(width) - 1;
>> +	default:
>> +		dev_err(chan2dev(chan), "Dma bus width not supported\n");
> 
> please log the width here, helps in debug...
> 

Hum.. just a dev_dbg to log the actual width or within the dev_err ?

>> +static u32 stm32_mdma_get_best_burst(u32 buf_len, u32 tlen, u32 max_burst,
>> +				     enum dma_slave_buswidth width)
>> +{
>> +	u32 best_burst = max_burst;
>> +	u32 burst_len = best_burst * width;
>> +
>> +	while ((burst_len > 0) && (tlen % burst_len)) {
>> +		best_burst = best_burst >> 1;
>> +		burst_len = best_burst * width;
>> +	}
>> +
>> +	return (best_burst > 0) ? best_burst : 1;
> 
> when would best_burst <= 0? DO we really need this check
> 
> 

best_burst < 0 is obviously unlikely but =0 is likely whether no best burst
found. Se we do need this check.

>> +static struct dma_async_tx_descriptor *
>> +stm32_mdma_prep_dma_cyclic(struct dma_chan *c, dma_addr_t buf_addr,
>> +			   size_t buf_len, size_t period_len,
>> +			   enum dma_transfer_direction direction,
>> +			   unsigned long flags)
>> +{
>> +	struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c);
>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>> +	struct dma_slave_config *dma_config = &chan->dma_config;
>> +	struct stm32_mdma_desc *desc;
>> +	dma_addr_t src_addr, dst_addr;
>> +	u32 ccr, ctcr, ctbr, count;
>> +	int i, ret;
>> +
>> +	if (!buf_len || !period_len || period_len > STM32_MDMA_MAX_BLOCK_LEN) {
>> +		dev_err(chan2dev(chan), "Invalid buffer/period len\n");
>> +		return NULL;
>> +	}
>> +
>> +	if (buf_len % period_len) {
>> +		dev_err(chan2dev(chan), "buf_len not multiple of period_len\n");
>> +		return NULL;
>> +	}
>> +
>> +	/*
>> +	 * We allow to take more number of requests till DMA is
>> +	 * not started. The driver will loop over all requests.
>> +	 * Once DMA is started then new requests can be queued only after
>> +	 * terminating the DMA.
>> +	 */
>> +	if (chan->busy) {
>> +		dev_err(chan2dev(chan), "Request not allowed when dma busy\n");
>> +		return NULL;
>> +	}
> 
> is that a HW restriction? Once a txn is completed can't we submit
> subsequent txn..? Can you explain this part please.
> 

Driver can prepare any request Slave SG, Memcpy or Cyclic. But if the channel is
busy to complete a DMA transfer, the request will be put in pending list. This
is only when the DMA transfer is going to be completed the next descriptor is
going to be processed and started.
However for cyclic this is different since when cyclic is ignited the channel
will be busy until its termination. This is why we forbid any DMA preparation
for this channel.
Nonetheless I believe we have a flaw here since we have to forbid
Slave/Memcpy/Cyclic whether a cyclic request is on-going.


>> +	if (len <= STM32_MDMA_MAX_BLOCK_LEN) {
>> +		cbndtr |= STM32_MDMA_CBNDTR_BNDT(len);
>> +		if (len <= STM32_MDMA_MAX_BUF_LEN) {
>> +			/* Setup a buffer transfer */
>> +			tlen = len;
>> +			ccr |= STM32_MDMA_CCR_TCIE | STM32_MDMA_CCR_CTCIE;
>> +			ctcr |= STM32_MDMA_CTCR_TRGM(STM32_MDMA_BUFFER);
>> +			ctcr |= STM32_MDMA_CTCR_TLEN((tlen - 1));
>> +		} else {
>> +			/* Setup a block transfer */
>> +			tlen = STM32_MDMA_MAX_BUF_LEN;
>> +			ccr |= STM32_MDMA_CCR_BTIE | STM32_MDMA_CCR_CTCIE;
>> +			ctcr |= STM32_MDMA_CTCR_TRGM(STM32_MDMA_BLOCK);
>> +			ctcr |= STM32_MDMA_CTCR_TLEN(tlen - 1);
>> +		}
>> +
>> +		/* Set best burst size */
>> +		max_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> 
> that maynot be best.. we should have wider and longer burst for best
> throughput..
> 

Will look at that.

>> +	ret = device_property_read_u32(&pdev->dev, "dma-requests",
>> +				       &nr_requests);
>> +	if (ret) {
>> +		nr_requests = STM32_MDMA_MAX_REQUESTS;
>> +		dev_warn(&pdev->dev, "MDMA defaulting on %i request lines\n",
>> +			 nr_requests);
>> +	}
>> +
>> +	count = of_property_count_u32_elems(of_node, "st,ahb-addr-masks");
> 
> We dont have device_property_xxx for this?

Sorry no. Well didn't figure out one though.

> 
>> +	if (count < 0)
>> +		count = 0;
>> +
>> +	dmadev = devm_kzalloc(&pdev->dev, sizeof(*dmadev) + sizeof(u32) * count,
>> +			      GFP_KERNEL);
>> +	if (!dmadev)
>> +		return -ENOMEM;
>> +
>> +	dmadev->nr_channels = nr_channels;
>> +	dmadev->nr_requests = nr_requests;
>> +	of_property_read_u32_array(of_node, "st,ahb-addr-masks",
>> +				   dmadev->ahb_addr_masks,
>> +				   count);
> 
> i know we have an device api for array reads :)
> and I think that helps in former case..
> 

Correct :) device_property_read_u32_array