lkml.org 
[lkml]   [2009]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: dw_dmac driver questions
Atsushi Nemoto wrote:
> Hi. I'm writing a new DMA driver, using dw_dmac driver in kernel
> 2.6.28 as a reference implementation.
>
> I can see an outline of the dw_dmac driver and framework (well,
> hopefully), but have some questions in details.
>
>
> 1. map/unmap DMA buffers for slave transfer
>
> For slave-DMA, it seems dmac driver is responsible for mapping DMA
> buffers, and client is responsible for unmapping them. Is it right?

No, it's the other way around, unless DMA_COMPL_SKIP_*_UNMAP is set.
But I think the dw_dmac driver wrongly maps the buffers before queuing
them.

> Now dwc_descriptor_complete() unmaps DMA buffer unless
> DMA_COMPL_SKIP_XXX_UNMAP set. These unmapping should be done only for
> non-slave transfers? I.e. I suppose "if (!dwc->dws)" is required
> around dma_unmap_page() pair.

Yes, perhaps. Or we could just forcibly set those flags in
prep_slave_sg().

> I also wonder why dwc_prep_slave_sg() calculates total_len. The
> total_len is stored in first descriptor of the chain and it is only
> referenced for unmapping DMA buffers. But the scatterlist may contain
> uncontinuous buffers, so they can not be unmaped at a time.

Yes, it is indeed a bit pointless to keep the total length around for
scatterlists.

> 2. dwc_is_tx_complete() and dwc->lock
>
> The function dwc_is_tx_complete() calls dwc_scan_descriptors() without
> dwc->lock (and disabling bh). Is it safe? All other callers of
> dwc_scan_descriptors() take dwc->lock and disable bh.

No, that does indeed look unsafe. We should probably take dwc->lock and
disable bh around the call to dwc_scan_descriptors().

> 3. dwc->queue list management
>
> The function dwc_tx_submit() add the descriptor to dwc->queue list if
> active list was not empty. But it does not manage lli.llp list. And
> all descriptors in the queue list will be moved to active list at a
> time. So it seems non-first descriptors in queue list will never
> processed by the hardware.
>
> The dwc_tx_submit() should rewrite lli.llp of the last descriptor in
> queue list (it it had children, the last children of it) by txd.phys
> of newly queued descriptor. Or, only first elements of queue list
> should be moved to active list at a time.
>
> Is my analysis correct?

Yes, I think you're right. lli.llp of the last queued entry should be
updated when adding new entries to the queue.

>
> BTW, I found some redundant code in the driver.
>
> * memset(dw, 0, sizeof *dw) seems redundant while dw is allocated
> kzalloc().

Indeed.

> * platform_get_drvdata(pdev) is called twice in dw_shutdown,
> dw_suspend_late.
>
> Both of them harmless.

Right.

> 4. This is a comment on head of dwc_handle_error().
>
> /*
> * The descriptor currently at the head of the active list is
> * borked. Since we don't have any way to report errors, we'll
> * just have to scream loudly and try to carry on.
> */
>
> But, the bad descriptor can be at any place of the active list, no ?
> For example, if the active list contained two descriptor and latter
> was broken and tasklet was delayed by some reason, the head of the
> list should be good.

Since dwc_scan_descriptors() was just called, all descriptors that were
completed successfully have been removed from the active list. So the
first entry must be the broken one.

Haavard


\
 
 \ /
  Last update: 2009-01-05 15:33    [W:0.073 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site