RE: [PATCH/RFC] DMA engine driver for Marvell XOR engine

[saeed]

> > +	if (flags & DMA_PREP_INTERRUPT)
> > +		hw_desc->desc_command = (1 << 31);
> > +	else
> > +		hw_desc->desc_command = 0;
> > +
> > +	hw_desc->desc_command = (1 << 31);
> > +}
> 
> It looks like in mv_desc_init() either the last line or if-else section
> needs to be removed.
right, the if-else will be removed
> 
> > +
> > +static inline u32 mv_desc_get_dest_addr(struct mv_xor_desc_slot
> *desc,
> > +					struct mv_xor_chan *chan)
> > +{
> > +	struct mv_xor_desc *hw_desc = desc->hw_desc;
> > +	return hw_desc->phy_dest_addr;
> > +}
> > +
> > +static inline u32 mv_desc_get_src_addr(struct mv_xor_desc_slot *desc,
> > +				      struct mv_xor_chan *chan,
> > +				      int src_idx)
> > +{
> > +	struct mv_xor_desc *hw_desc = desc->hw_desc;
> > +	return hw_desc->phy_src_addr[src_idx];
> > +}
> > +
> > +
> > +static inline void mv_desc_set_byte_count(struct mv_xor_desc_slot
> *desc,
> > +					  struct mv_xor_chan *chan,
> > +					  u32 byte_count)
> > +{
> > +	struct mv_xor_desc *hw_desc = desc->hw_desc;
> > +	hw_desc->byte_count = byte_count;
> > +}
> 
> Parameter 'chan' is not used in mv_desc_get_dest_addr(),
> mv_desc_get_src_addr() 
> and mv_desc_set_byte_count()
ok, it will be removed
> 
> > +static void mv_xor_check_threshold(struct mv_xor_chan *mv_chan)
> > +{
> > +	if (mv_chan->pending >= MV_XOR_THRESHOLD) {
> > +		mv_chan->pending = 0;
> > +		mv_chan_activate(mv_chan);
> > +	}
> > +}
> 
> Is it needed to use both mv_xor_check_threshold() and
> mv_xor_issue_pending()
> in the driver? 
> What about replacing mv_xor_check_threshold() with
> mv_xor_issue_pending()
> and using the threshold in mv_xor_issue_pending() 
> (especially that MV_XOR_THRESHOLD is set to 1)?
you're right, as the threshold is 1, those functions doing exactly the 
same job.
> 
> > +	int num_descs_in_pool = plat_data->pool_size/MV_XOR_SLOT_SIZE;
> > +
> > +	/* Allocate descriptor slots */
> > +	do {
> > +		idx = mv_chan->slots_allocated;
> > +		if (idx == num_descs_in_pool)
> > +			break;
> 
> This break condition is actually redundant to the do-while loop
> condition.
> What about replacing do-while with simpler while loop?
I did that, but know I found some problem with this code which was 
copied from the iop-adma. what bothers me that if we exit the loop from 
the break, then we end with idx=mv_chan->slots_allocated=num_descs_in_pool,
 but, if we exit from the while condition, then we end with 
idx=mv_chan->slots_allocated - 1 = num_descs_in_pool - 1
Dan, can you comment?
> MV_XOR_SLOT_SIZE];
> > +
> > +	tx = mv_xor_prep_dma_memcpy(dma_chan, dest_dma, src_dma,
> > +				    MV_XOR_TEST_SIZE, 0);
> > +	cookie = mv_xor_tx_submit(tx);
> 
> It would be more generic solution in both _self_test() functions
> to use dma_device API and async_tx API rather than
> direct calls like mv_xor_alloc_chan_resources(),
> mv_xor_prep_dma_memcpy(), 
> mv_xor_tx_submit(), mv_xor_issue_pending()
> (i.e. replace mv_xor_alloc_chan_resources 
> with device->common.device_alloc_chan_resources, etc.)
again, this is copy&paste from iop-adma, I suggest to keep it this way, 
and to do what you suggest in seperate patch set. and I think that the 
test code better be removed from the low level drivers to the DMA Engine 
layer.
agree?

> 
> > +	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
> > +		ret = mv_xor_memcpy_self_test(adev);
> > +		dev_dbg(&pdev->dev, "memcpy self test returned %d\n",
> ret);
> > +		if (ret)
> > +			goto err_free_dma;
> > +	}
> > +
> > +	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask) ||
> > +		dma_has_cap(DMA_MEMSET, dma_dev->cap_mask)) {
> > +		ret = mv_xor_xor_self_test(adev);
> > +		dev_dbg(&pdev->dev, "xor self test returned %d\n", ret);
> > +		if (ret)
> > +			goto err_free_dma;
> > +	}
> > +	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
> > +		ret = mv_xor_memcpy_self_test(adev);
> > +		dev_dbg(&pdev->dev, "memcpy self test returned %d\n",
> ret);
> > +		if (ret)
> > +			goto err_free_dma;
> > +	}
> > +
> > +	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask) ||
> > +		dma_has_cap(DMA_MEMSET, dma_dev->cap_mask)) {
> > +		ret = mv_xor_xor_self_test(adev);
> > +		dev_dbg(&pdev->dev, "xor self test returned %d\n", ret);
> > +		if (ret)
> > +			goto err_free_dma;
> > +	}
> 
> What is the reason for running exact the same memcpy/xor self_test
> procedure two times?
> It would be helpful if there was a comment on that in this place.
no reason, I did that for debug and forgot to remove, I also removed the 
|| dma_has_cap (MEMSET) from the xor self test; another stupid copy&paste
> 
> > +
Here is the updated patch:
---------------- >8