Messages in this thread | | | Date | Tue, 1 Jul 2008 16:41:34 -0700 | From | "Dan Williams" <> | Subject | Re: [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
| |