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

["Dan Williams" <dan.j.williams@intel.com>]

On Tue, Jul 1, 2008 at 9:07 AM, saeed <saeed@marvell.com> wrote:
>> > +   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?

The admittedly ugly do { } while () loop in iop-adma.c assumed that
num_descs_in_pool is always > 1, and guarantees that idx is equal to
the count of allocated descriptors.  Since you changed it to a simple
while() loop then you should also replace idx with ->slots_allocated
in the rest of the routine i.e.:

     return mv_chan->slots_allocated ? : -ENOMEM;

>> 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?
>

I agree, keep it this way for now and then we can look to unify all
the drivers' self test routines into a common dmaengine routine for
2.6.28.

>>
>> > +   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

Yes, this is an architecture specific aspect of the iop-adma driver.
On an iop the channel may be a memcpy only channel, or an xor / memset
channel.  Hence the need for two separate tests.

--
Dan