Re: [PATCH v5 2/2] iio: adc: ad4130: add AD4130 driver

[Jonathan Cameron]

On Thu, 23 Jun 2022 19:33:45 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Thu, Jun 23, 2022 at 6:14 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> > On 6/23/22 18:39, Andy Shevchenko wrote:  
> > > On Thu, Jun 23, 2022 at 5:27 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:  
> > >> On 6/20/22 21:29, Andy Shevchenko wrote:  
> > >>> On Mon, Jun 20, 2022 at 6:27 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:  
> 
> ...
> 
> > >>>> +       /*
> > >>>> +        * DMA (thus cache coherency maintenance) requires the
> > >>>> +        * transfer buffers to live in their own cache lines.
> > >>>> +        */  
> > >>>
> > >>> This is a good comment, but what fields does it apply to?  
> > >>
> > >> Whatever is below it, grouped together. This is not hard to
> > >> understand.  
> > >
> > > It's hard to understand what exactly is DMA-aware here. I see only one
> > > buffer that is aligned properly for DMA, the rest are not, except the
> > > case if all of them are going in one DMA transaction. Is this the case
> > > here?
> > >  
> > >>>> +       u8                      reset_buf[AD4130_RESET_BUF_SIZE] __aligned(IIO_DMA_MINALIGN);  
> > >
> > > This is aligned.
> > >  
> > >>>> +       u8                      reg_write_tx_buf[4];  
> > >
> > > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0).
> > >  
> > >>>> +       u8                      reg_read_tx_buf[1];  
> > >
> > > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0 + 4).
> > >  
> > >>>> +       u8                      reg_read_rx_buf[3];  
> > >
> > > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0 + 4 + 1).
> > > And this is Rx.
> > >  
> > >>>> +       u8                      fifo_tx_buf[2];  
> > >
> > > Here is Tx again which is most likely is not aligned...
> > >  
> > >>>> +       u8                      fifo_rx_buf[AD4130_FIFO_SIZE *
> > >>>> +                                           AD4130_FIFO_MAX_SAMPLE_SIZE];
> > >>>> +};  
> > >  
> >
> > This has been mentioned before by Jonathan as a reply to V6 of my
> > AD74413R driver.
> >  
> >  > I'm surprised I didn't mention this before but you only need to  
> > ensure  > that any memory used for DMA is not in a cacheline with memory
> > used  
> >  > for other things that might change concurrently.  
> >
> > To my understanding, as long as the DMA buffers will all be accessed by
> > the same DMA-compatible SPI controller, you only need to align them so
> > they're not in the same cacheline with memory that will not be accessed
> > by the SPI controller.  
> 
> SPI is synchronous by nature, what will happen if the Tx and Rx
> buffers are sharing the same cache line? Anybody to shed a light here?
> 
> (I.o.w. I'm not sure that we don't need to split the Rx and Tx buffers
> of the same transfer.)

My understanding is that any device that stamps on itself is considered broken
and needs to do it's own bounce buffering.  We just need to ensure no
CPU writes hit stuff in the same cacheline whilst DMA is in progress.

A clarification to the comment to say that it covers all the buffers at the
end of the structure would be a good addition.

Jonathan