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@xxxxxxxxxxx> > >> > > >> > 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@xxxxxxx>? > > 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
