Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Couret Charles-Antoine <charles-antoine.couret@essensium.com>
Cc: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>,
	"andrea.merello@gmail.com" <andrea.merello@gmail.com>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"pmeerw@pmeerw.net" <pmeerw@pmeerw.net>,
	"knaack.h@gmx.de" <knaack.h@gmx.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 0/4] Fixes for ad7949
Date: Sun, 15 Sep 2019 11:49:39 +0100
Message-ID: <20190915114939.13fc6d44@archlinux> (raw)
In-Reply-To: <2b7ab6c3-6ff6-69b5-bbd4-f8a1966b6b57@essensium.com>

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'.

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
> 


  reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 14:43 Andrea Merello
2019-09-12 14:43 ` [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code Andrea Merello
2019-09-13  6:37   ` Ardelean, Alexandru
2019-09-15 10:26     ` Jonathan Cameron
2019-09-12 14:43 ` [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len Andrea Merello
2019-09-13  6:46   ` Ardelean, Alexandru
2019-09-13  7:56     ` Andrea Merello
2019-09-13  8:28       ` Ardelean, Alexandru
2019-09-15 10:36       ` Jonathan Cameron
2019-09-16  7:51         ` Ardelean, Alexandru
2019-09-21 17:16           ` Jonathan Cameron
2019-09-12 14:43 ` [PATCH 3/4] iio: ad7949: fix SPI xfer delays Andrea Merello
2019-09-13  6:59   ` Ardelean, Alexandru
2019-09-13  8:23     ` Andrea Merello
2019-09-13  8:43       ` Ardelean, Alexandru
2019-09-12 14:43 ` [PATCH 4/4] iio: ad7949: fix channels mixups Andrea Merello
2019-09-13  7:19   ` Ardelean, Alexandru
2019-09-13  8:30     ` Andrea Merello
2019-09-13 11:30       ` Couret Charles-Antoine
2019-09-13 11:40         ` Andrea Merello
2019-09-20  7:45         ` Andrea Merello
2019-09-21 17:12           ` Jonathan Cameron
2019-09-23  8:21             ` Andrea Merello
2019-10-05  9:55               ` Jonathan Cameron
     [not found]                 ` <CAN8YU5PRO5Y5EeEj2SZGm5XfuKSB1rtS7nKdu6wWxXYDOfexqw@mail.gmail.com>
2019-10-22  8:56                   ` Jonathan Cameron
2019-11-04 14:12                     ` Andrea Merello
2019-11-09 11:58                       ` Jonathan Cameron
2019-11-12 15:09                       ` Couret Charles-Antoine
2019-12-02 14:13                         ` [v2] " Andrea Merello
2019-12-02 15:36                           ` Couret Charles-Antoine
2019-12-04 11:06                             ` Jonathan Cameron
2019-12-04 11:13                               ` Couret Charles-Antoine
2019-12-06 16:45                                 ` Jonathan Cameron
2019-09-13  7:24 ` [PATCH 0/4] Fixes for ad7949 Ardelean, Alexandru
2019-09-13 14:00   ` Couret Charles-Antoine
2019-09-15 10:49     ` Jonathan Cameron [this message]
2019-09-16  7:39       ` Andrea Merello
2019-09-16  7:48         ` Ardelean, Alexandru
2019-09-16  7:50           ` Ardelean, Alexandru
2019-09-16  7:34     ` Andrea Merello

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190915114939.13fc6d44@archlinux \
    --to=jic23@kernel.org \
    --cc=Michael.Hennerich@analog.com \
    --cc=alexandru.Ardelean@analog.com \
    --cc=andrea.merello@gmail.com \
    --cc=charles-antoine.couret@essensium.com \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git