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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
