Re: [PATCH 0/4] Fixes for ad7949

["Ardelean, Alexandru" <alexandru.Ardelean@analog.com>]

On Mon, 2019-09-16 at 07:48 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Mon, 2019-09-16 at 09:39 +0200, Andrea Merello wrote:
> > Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron
> > <jic23@kernel.org> ha scritto:
> > > On Fri, 13 Sep 2019 16:00:29 +0200
> > > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
> > > 
> > > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > > > > > [External]
> > > > > > 
> > > > > > This patch series fixes ad7949 driver incorrectly read data, simplify the
> > > > > > code, and enforces device timing constraints.
> > > > > > 
> > > > > > This has been tested on a UltraZed SOM + a custom carrier equipped with
> > > > > > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> > > > > > kernel and then rebased on linux-next kernel.
> > > > > > 
> > > > > Thanks for the patches.
> > > > > Added Charles-Antoine to also take a look.
> > > > > Apologies for not thinking of adding him sooner.
> > > > > 
> > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> > > > > Jonathan will likely also take a look.
> > > > > 
> > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling
> > > > > code")
> > > > > now and see about the rest.
> > > > > The rest are a bit more open to discussion, so a v2 may happen.
> > > > 
> > > > Hi,
> > > > 
> > > > Don't worry. Due to the fact I don't have on my mail client access to
> > > > the whole discussions, I'm making a complete answer there based on the
> > > > archive of the mailing list. Sorry for that.
> > > > 
> > > > 
> > > > For the patch 1, I approve it too. This part of code is useless because
> > > > the feature was removed. RIP my code. :D
> > > > 
> > > > For the patch 2, the cache information was added due to comment from
> > > > Jonathan Cameron when I developed the driver. The comment was:
> > > > 
> > > > > Look very carefully at the requirements for a buffer being passed
> > > > > to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> > > > > way to do that easily is to put a cacheline aligned buffer in your
> > > > > ad7949_adc_chip structure.
> > > 
> > > The short version of this is best illustrated with an example.
> > > This only applies systems where the DMA engines are not coherent
> > > (i.e. a change made by a DMA engine is not automatically updated to
> > >  all other places a copy is held in caches in the system, we have to
> > >  do it by hand).
> > > 
> > > We have a structure like
> > > struct bob {
> > >         int initial_data;
> > >         u8 buffer[8];
> > >         int magic_flags
> > > };
> > > 
> > > When a DMA transfer is setup involving 'buffer', the DMA engine may take
> > > up to a cacheline (typically 64 bytes) including buffer, make a copy of it
> > > and assume that the only bit of hardware working in this cacheline is itself.
> > > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'.
> > > Whilst that DMA is going on, a CPU can write something in magic flags.
> > > That something might be important but unrelated to the DMA transfer going
> > > on.
> > > 
> > > The DMA finishes, having put new data in the buffer element of the copy
> > > of the cacheline local to . It's guaranteed to not change it's copy of the
> > > cacheline (in this case containing the whole of bob).   However, it's version
> > > of magic_flags is out of date so when we flush the caches at the end of the
> > > non coherent DMA transfer (to force the CPU to read it from main memory and
> > > get the new values in buffer), the value of magic_flags can be reset to the
> > > version the DMA engine has.
> > > 
> > > So, upshot is to avoid any potential of such problems, DMA buffers 'must'
> > > always be in a cacheline containing nothing that might be changed by
> > > other activities.  This can mean it is safe to put both TX and RX buffers
> > > in the same cacheline as we won't touch either during an SPI transfer.
> > > 
> > > > > Lots of examples to copy, but it's also worth making sure you understand
> > > > > why this is necessary.
> > > > 
> > > > For the endianess thing, it shouldn't be required to make an explicit
> > > > conversion into the driver. According to the spi.h documentation:
> > > > 
> > > > > * In-memory data values are always in native CPU byte order, translated
> > > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> > > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long
> > > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> > > > So from my point of view the SPI subsystem always converts to the right
> > > > endianess. We don't have to take care about it.
> > > 
> > > Correct, though as I commented on that patch, that's not always 'possible'
> > > and not all drivers set the word length 'correctly'.
> > 
> > Thank you both for the explanations about DMA and SPI endianess :)
> > 
> > So indeed 2/4 seems OK to me, and it doesn't need any further
> > endianess-related fix.
> 
> Yep.
> With these explanations:
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 

this is for patch 2/4

> > 
> > > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018
> > > also touches on a lot of this.
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > > 
> > > > For patch 3, I didn't use delay_usecs fiels due to the timings
> > > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A
> > > > BUSY INDICATOR" mode. During the delay, the chip select line must be
> > > > released which is not the case when we use delay_usecs field. So I add
> > > > the delay instruction after the write step to be compliant with these
> > > > timings.
> > > > 
> > > > 
> > > > For patch 4, I explained a bit in the other thread. Maybe we have a
> > > > difference of behaviour due to the choice of the timings "modes"?
> > > > 
> > > > 
> > > > BTW, from my point of view the datasheet is not totally clear about the
> > > > timings and what is mandatory or not in the expected behaviour.
> > > > 
> > > > Regards,
> > > > 
> > > > Charles-Antoine Couret
> > > >