From: Andrea Merello <andrea.merello@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Couret Charles-Antoine <charles-antoine.couret@essensium.com>,
"Ardelean, Alexandru" <alexandru.Ardelean@analog.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: Mon, 16 Sep 2019 09:39:00 +0200 [thread overview]
Message-ID: <CAN8YU5NVWubV0HeXDHZ=fJThaKTC5R2AoNtX-BsDdifVkiJZnA@mail.gmail.com> (raw)
In-Reply-To: <20190915114939.13fc6d44@archlinux>
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.
> 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
> >
>
next prev parent reply other threads:[~2019-09-16 7:39 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 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
2019-09-16 7:39 ` Andrea Merello [this message]
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 publicly 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='CAN8YU5NVWubV0HeXDHZ=fJThaKTC5R2AoNtX-BsDdifVkiJZnA@mail.gmail.com' \
--to=andrea.merello@gmail.com \
--cc=Michael.Hennerich@analog.com \
--cc=alexandru.Ardelean@analog.com \
--cc=charles-antoine.couret@essensium.com \
--cc=jic23@kernel.org \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).