lkml.org 
[lkml]   [2008]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFC] DMA engine driver for Marvell XOR engine
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


\
 
 \ /
  Last update: 2008-07-02 01:45    [W:0.091 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site