On Sun, 24 Nov 2019 20:11:01 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> On Sat, Nov 23, 2019 at 05:12:04PM +0000, Jonathan Cameron wrote:
> > On Thu, 21 Nov 2019 22:00:07 +0100
> > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> >
> > > This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
> > > and so has a slightly different protocol. Only the actual hardware
> > > access is different. The spi protocol is different enough to not be able
> > > to map the differences via a regmap.
> > >
> > > Also generalize the entry in MAINTAINER to cover the newly introduced
> > > file.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > looks good with the exception of the now overly protected DMA buffers.
> >
> > See inline. As that's all I'm seeing that needs fixing I'll just
> > fix this up whilst applying.
> >
> > I'd like the series to sit a little longer on the list though to give
> > devicetree maintainers time to look at the bindings.
>
> ok.
>
> > > +struct ltc2496_driverdata {
> > > + /* this must be the first member */
> > > + struct ltc2497core_driverdata common_ddata;
> > > + struct spi_device *spi;
> > > +
> > > + /*
> > > + * DMA (thus cache coherency maintenance) requires the
> > > + * transfer buffers to live in their own cache lines.
> > > + */
> > > + unsigned char rxbuf[3] ____cacheline_aligned;
> > > + unsigned char txbuf[3] ____cacheline_aligned;
> > Ah. I've not explained this clearly enough. Upshot is you only need
> > to ensure that the buffers used for dma are not shared with any other
> > usage. the __cacheline_aligned marker pads the structure to ensure
> > the element so marked is at the start of a new cacheline. This means
> > there is no sharing with non DMA related elements which may be accidentally
> > reset when the DMA transfer ends.
> >
> > Imagine we had:
> > struct bob {
> > int a; //used for all sorts of fun things not related to dma and not
> > //protected from happening concurrently with dma.
> > unsigned char rx_buf[3];
> > unsigned char tx_buf[3]
> > };
> >
> > The buffers are used for DMA. The DMA engine takes a copy of the cacheline
> > to start doing it's magic.
> >
> > Along comes other activity and writes to 'a'.
> >
> > DMA completes, then engine pushes the cacheline back to the memory including
> > writing back what it had as a copy of a. Thus the update to 'a' is lost.
> >
> > Now the guarantee we make use of is that DMA engines are not allowed to
> > copy cachelines that do not contain the buffers they are using (all sorts
> > of things would break if they were).
> >
> > However, there is no need to separate rx_buf and tx_buf as they are being
> > used by the same DMA engine and nothing else is going to update them whilst
> > they are in use.
>
> Yeah, I thought about that when adding the second annotation but then
> forgot to mention that in my cover letter.
>
> So I assume you will just drop the 2nd ____cacheline_aligned? That's
> fine for me; thanks.
Yup.
Jonathan
>
> Best regards
> Uwe
>
