Re: [PATCH] ad7877: keep dma rx buffers in seperate cache lines

[Oskar Schirmer]

On Thu, May 06, 2010 at 14:46:04 -0400, Mike Frysinger wrote:
> On Thu, May 6, 2010 at 06:37, Oskar Schirmer wrote:
> > Âstruct ser_req {
> > + Â Â Â u16 Â Â Â Â Â Â Â Â Â Â sample;
> > +    char          Â__padalign[L1_CACHE_BYTES - sizeof(u16)];
> > +
> > Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â reset;
> > Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â ref_on;
> > Â Â Â Âu16 Â Â Â Â Â Â Â Â Â Â command;
> > - Â Â Â u16 Â Â Â Â Â Â Â Â Â Â sample;
> >    Âstruct spi_message   Âmsg;
> >    Âstruct spi_transfer   xfer[6];
> > Â};
> 
> are you sure this is necessary ?  ser_req is only ever used with
> spi_sync() and it's allocated/released on the fly, so how could
> anything be reading that memory between the start of the transmission
> and the return to adi7877 ?

msg is handed over to spi_sync, it contains the addresses
which will be used to programme the DMA: the spi master
transfer function will read these fields to start DMA.

E.g., drivers/spi/atmel_spi.c, assures cache coherency
in function atmel_spi_dma_map_xfer, one xfer at a time.
Multiple transfers are worked on in a loop in atmel_spi_transfer,
so when coherency for transfer #1 is ensured, addresses
for transfer #2 are read from the msg and xfer structs,
caching lines which have been just before invalidated.

And, citing Documentation/DMA-API.txt, Part Id:
"mapped region must begin exactly on a cache line
boundary and end exactly on one", which is our case.

> 
> > Âstruct ad7877 {
> > + Â Â Â u16 Â Â Â Â Â Â Â Â Â Â conversion_data[AD7877_NR_SENSE];
> > +    char          Â__padalign[L1_CACHE_BYTES
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â - AD7877_NR_SENSE * sizeof(u16)];
> > +
> >    Âstruct input_dev    Â*input;
> >    Âchar          Âphys[32];
> >
> > @@ -182,8 +188,6 @@ struct ad7877 {
> > Â Â Â Âu8 Â Â Â Â Â Â Â Â Â Â Âaveraging;
> > Â Â Â Âu8 Â Â Â Â Â Â Â Â Â Â Âpen_down_acc_interval;
> >
> > - Â Â Â u16 Â Â Â Â Â Â Â Â Â Â conversion_data[AD7877_NR_SENSE];
> > -
> >    Âstruct spi_transfer   xfer[AD7877_NR_SENSE + 2];
> >    Âstruct spi_message   Âmsg;
> 
> i can see the spi_message inside of this struct being a problem
> because the spi transfer is doing asynchronously with spi_async().
> however, i would add a comment right above these two fields with a
> short explanation as to why they're at the start and why the pad
> exists so someone down the line doesnt move it.

The code says "pad to align according to L1 cache, and
keep away other stuff by exactly the amount so it is
off the line". I'ld guess a comment would repeat just
this, so it is superfluous. But if opinions differ on
this topic, we can have a comment added, sure.

  Oskar
-- 
oskar schirmer, emlix gmbh, http://www.emlix.com
fon +49 551 30664-0, fax -11, bahnhofsallee 1b, 37081 gÃttingen, germany
sitz der gesellschaft: gÃttingen, amtsgericht gÃttingen hr b 3160
geschÃftsfÃhrer: dr. uwe kracke, ust-idnr.: de 205 198 055

emlix - your embedded linux partner
--
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/