Re: [PATCH v4 3/4] i2c: imx: use readb_relaxed and writeb_relaxed

[Stefan Eichenberger <eichest@gmail.com>]

On Wed, Oct 02, 2024 at 01:36:04PM +0000, Arnd Bergmann wrote:
> On Wed, Oct 2, 2024, at 13:08, Stefan Eichenberger wrote:
> > On Wed, Oct 02, 2024 at 11:51:22AM +0000, Arnd Bergmann wrote:
> >> On Wed, Oct 2, 2024, at 11:19, Stefan Eichenberger wrote:
> >> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >> >
> >> > Use the relaxed version of readb and writeb to reduce overhead. It is
> >> > safe to use the relaxed version because we either do not rely on dma
> >> > completion, or we use a dma callback to ensure that the dma transfer is
> >> > complete before we continue.
> >> 
> >> I would still consider this a bug in general, you should
> >> never default to the unsafe variants.
> >> 
> >> If there is a codepath that needs the barrierless version,
> >> please add imx_i2c_write_reg_relaxed()/imx_i2c_read_reg_relaxed()
> >> helpers that use those only in the places where it makes
> >> a measurable difference, with a comment that explains
> >> the usage.
> >
> > I added the patch because of the following dicussion:
> > https://lore.kernel.org/linux-i2c/ZpVWXlR6j2i0ZtVQ@lizhi-Precision-Tower-5810/
> >
> > I can't determine if the relaxed version improves performance. The
> > 'normal' version worked well for our use case too. Therefore, dropping
> > the change would be acceptable for us. Another potential solution could
> > be to use the relaxed version only inside the ISR. Would that be an
> > acceptable solution? What is your impression, Frank Li
> > <Frank.Li@nxp.com>?
> 
> I'm pretty sure that Frank meant to use readb_relaxed()/writeb_relaxed()
> inside of the FIFO access loop, not for everything else. This
> makes a lot of sense, since the FIFO read in particular is
> clearly performance sensitive and already serialized by the
> implied control dependency.
> 
> If you can read multiple bytes, the best interface to use
> would in fact be readsb() or possibly readsl() to read
> four bytes with each access.
> 
> It appears that you did not implement the suggestion to
> read the entire FIFO though, so you can probably just skip
> the _relaxed() change entirely.

This makes sense, it appears this was a misunderstanding. If no one
objects, I will drop the patch in the next version. Thank you for the
clarification.

Regards,
Stefan