Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[David Brownell]

On Thursday 20 November 2008, Mike Frysinger wrote:
> On Thu, Nov 20, 2008 at 15:24, David Brownell wrote:
> > On Monday 17 November 2008, Bryan Wu wrote:
> >>                         /* set transfer mode, and enable SPI */
> >>                         dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
> >>
> >> +                       /* invalidate caches, if needed */
> >> +                       if (bfin_addr_dcachable((unsigned long) drv_data->rx))
> >> +                               invalidate_dcache_range((unsigned long) drv_data->rx,
> >> +                                                       (unsigned long) (drv_data->rx +
> >> +                                                       drv_data->len));
> >
> > Could you explain why you're not using dma_map_*() calls
> > or the rx_dma (and tx_dma) addresses the caller may pass?
> 
> i'm not familiar with said API.  i worked with what was there already.

I see.


> > As a rule, you should use the standard kernel interfaces
> > for such stuff instead of platform-specific calls like
> > those two.  There are a LOT more developers who can find
> > and fix bugs that way.
> 
> could you elaborate on the common calls that would replace these ?

See Documentation/DMA-(mapping,API}.txt ... the "mapping"
document's section on what memory may be used for DMA is
not otherwise replicated in the description of the "generic"
versions of those API calls.

Basically, dma_map_single(), dma_unmap_single() ... and
remember that the caller may have done the mappings for
you already.


> > Also, this patch affects the "not full duplex" branch of
> > this routine.  DMA here seems unusually convoluted ... but
> > if you didn't invalidate the cache (RX path) before
> > flushing it (TX path) and instead did it the other way
> > aroound, would you actually *need* separate branches?
> 
> it's convoluted because the hardware sucks.  it cant do full duplex
> with DMA.  full duplex only works in the non-DMA case.

Ah, ok.  Sucky hardware -- been there, done that, still doing.  ;)

It'd be nice if one of patches snuck in a comment on that
point:  "Full duplex only works for non-DMA transfers."
Same rationale:  you may know this hardware inside out,
but the next person won't.

- Dave



> -mike
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/