| Message ID | 20241002112020.23913-4-eichest@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: imx: prevent rescheduling in non-dma mode | expand |
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. Arnd
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>? Regards, Stefan
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. Arnd
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
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 8e9cdeff278a8..f70522a83f823 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -313,13 +313,13 @@ static inline int is_vf610_i2c(struct imx_i2c_struct *i2c_imx) static inline void imx_i2c_write_reg(unsigned int val, struct imx_i2c_struct *i2c_imx, unsigned int reg) { - writeb(val, i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); + writeb_relaxed(val, i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); } static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx, unsigned int reg) { - return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); + return readb_relaxed(i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); } static void i2c_imx_clear_irq(struct imx_i2c_struct *i2c_imx, unsigned int bits)