| Message ID | c59aea77998dfea1b4456c4b33b55ab216fcbf5e.1732284746.git.geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: riic: Always round-up when calculating bus period | expand |
On Fri, Nov 22, 2024 at 03:14:35PM +0100, Geert Uytterhoeven wrote: > Currently, the RIIC driver may run the I2C bus faster than requested, > which may cause subtle failures. E.g. Biju reported a measured bus > speed of 450 kHz instead of the expected maximum of 400 kHz on RZ/G2L. > > The initial calculation of the bus period uses DIV_ROUND_UP(), to make > sure the actual bus speed never becomes faster than the requested bus > speed. However, the subsequent division-by-two steps do not use > round-up, which may lead to a too-small period, hence a too-fast and > possible out-of-spec bus speed. E.g. on RZ/Five, requesting a bus speed > of 100 resp. 400 kHz will yield too-fast target bus speeds of 100806 > resp. 403226 Hz instead of 97656 resp. 390625 Hz. > > Fix this by using DIV_ROUND_UP() in the subsequent divisions, too. > > Tested on RZ/A1H, RZ/A2M, and RZ/Five. > > Fixes: d982d66514192cdb ("i2c: riic: remove clock and frequency restrictions") > Reported-by: Biju Das <biju.das.jz@bp.renesas.com> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Apart from the rounding issue fixed by this patch, I could not find any > bugs in the calculation of the various parameters (based on the formulas > in the documentation). Still, the actual (measured) bus speed may still > be higher than the target bus speed. Hence this patch is not sufficient > to reduce the actual bus speed to safe levels, and I have not yet addded > > Closes: https://lore.kernel.org/TYCPR01MB11269BFE7A9D3DC605BA2A2A9864E2@TYCPR01MB11269.jpnprd01.prod.outlook.com/ Can I add this in the commit log? > On RZ/A1H (RSK+RZA1): > > speed actual duty fall rise cks brl brh > ------ -------- ---- ---- ---- --- --- --- > before: 101600 113 kHz 63 1 4 3 20 10 > after: 99181 110 kHz 64 1 4 3 21 10 > > before: 396726 407 kHz 62 5 5 1 17 9 > after: 396726 407 kHz 62 5 5 1 17 9 > > Note that before commit d982d66514192cdb, the actual values were > within spec, so probably the parameters were hand-tuned with the > help of scope: > 101600 99.2 kHz 63 1 4 3 19 16 > 396726 370 kHz 62 5 5 1 21 9 > > RZ/A2M (RZA2MEVB): > > speed actual duty fall rise cks brl brh > ------ -------- ---- ---- ---- --- --- --- > before: 100609 115 kHz 63 1 4 4 20 10 > after: 98214 111 kHz 64 1 4 4 21 10 > > before: 402439 459 kHz 61 5 5 2 16 9 > after: 392857 446 kHz 62 5 5 2 17 9 > > RZ/Five (RZ/Five SMARC): > > speed actual duty fall rise cks brl brh > ------ -------- ---- ---- ---- --- --- --- > before: 100806 112 kHz 64 0 3 5 15 7 > after: 97656 108 kHz 65 0 3 5 16 7 > > before: 403225 446 kHz 60 3 3 3 12 7 > after: 390625 431 kHz 61 3 3 3 13 7 > > I.e. the actual bus speed is still up to 10% higher than requested. > > The driver assumes the default register settings: > - FER.SCLE = 1 (SCL sync circuit enabled, adds 2 or 3 cycles) > - FER.NFE = 1 (noise circuit enabled) > - MR3.NF = 0 (1 cycle of noise filtered out) > As these are not explicitly set by the driver, I verified that the > assumptions are true on all affected platforms. > > I also tried disabling FER.SCLE and removing the compensation for SCLE > on RZ/Five. For a bus speed of 100 kHz, that gave: > > speed actual duty fall rise cks brl brh > ------ ---------- ---- ---- ---- --- --- --- > before: 97656 108 kHz 65 0 3 5 16 7 > after: 97656 94.7 kHz 63 0 3 5 18 9 > > which looks better, but obviously the SCL sync circuit must add some > value? > > So it looks like the default values provided by i2c_parse_fw_timings() > do not work well for us, and all board DTS files should provide suitable > values explicitly, using the "i2c-scl-rising-time-ns" and > "i2c-scl-falling-time-ns" properties. > Adam submitted something similar for R-Car a while ago[1]. It's a pity that all this description is lost. I love long explanations especially when they come from test results. Can I add it in the commit log? Andi > Thanks for your comments! > > [1] "[PATCH] arm64: dts: renesas: beacon: Fix i2c2 speed calcuation" > https://lore.kernel.org/20210825122757.91133-1-aford173@gmail.com > --- > drivers/i2c/busses/i2c-riic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > index 0c2342f86d3d2fbe..a6aeb40aae04d607 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -352,7 +352,7 @@ static int riic_init_hw(struct riic_dev *riic) > if (brl <= (0x1F + 3)) > break; > > - total_ticks /= 2; > + total_ticks = DIV_ROUND_UP(total_ticks, 2); > rate /= 2; > } > > -- > 2.34.1 >
Hi Andi, On Wed, Dec 11, 2024 at 12:20 PM Andi Shyti <andi.shyti@kernel.org> wrote: > On Fri, Nov 22, 2024 at 03:14:35PM +0100, Geert Uytterhoeven wrote: > > Currently, the RIIC driver may run the I2C bus faster than requested, > > which may cause subtle failures. E.g. Biju reported a measured bus > > speed of 450 kHz instead of the expected maximum of 400 kHz on RZ/G2L. > > > > The initial calculation of the bus period uses DIV_ROUND_UP(), to make > > sure the actual bus speed never becomes faster than the requested bus > > speed. However, the subsequent division-by-two steps do not use > > round-up, which may lead to a too-small period, hence a too-fast and > > possible out-of-spec bus speed. E.g. on RZ/Five, requesting a bus speed > > of 100 resp. 400 kHz will yield too-fast target bus speeds of 100806 > > resp. 403226 Hz instead of 97656 resp. 390625 Hz. > > > > Fix this by using DIV_ROUND_UP() in the subsequent divisions, too. > > > > Tested on RZ/A1H, RZ/A2M, and RZ/Five. > > > > Fixes: d982d66514192cdb ("i2c: riic: remove clock and frequency restrictions") > > Reported-by: Biju Das <biju.das.jz@bp.renesas.com> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > Apart from the rounding issue fixed by this patch, I could not find any > > bugs in the calculation of the various parameters (based on the formulas > > in the documentation). Still, the actual (measured) bus speed may still > > be higher than the target bus speed. Hence this patch is not sufficient > > to reduce the actual bus speed to safe levels, and I have not yet addded > > > > Closes: https://lore.kernel.org/TYCPR01MB11269BFE7A9D3DC605BA2A2A9864E2@TYCPR01MB11269.jpnprd01.prod.outlook.com/ > > Can I add this in the commit log? I'd rather not add the Closes-tag, as this patch does not fully fix the issue. > > On RZ/A1H (RSK+RZA1): [...] > It's a pity that all this description is lost. I love long > explanations especially when they come from test results. > > Can I add it in the commit log? What about starting to add Link: tags pointing to lore to I2C commits, like most other subsystems do? That way people can easily reach any background information (if available), and find the corresponding email thread where to report issues or follow-up information? Thanks! [1] https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/maintainer/configure-git.rst#L33 Gr{oetje,eeting}s, Geert
Hi Geert, [...] > > It's a pity that all this description is lost. I love long > > explanations especially when they come from test results. > > > > Can I add it in the commit log? > > What about starting to add Link: tags pointing to lore to I2C commits, > like most other subsystems do? > That way people can easily reach any background information (if > available), and find the corresponding email thread where to report > issues or follow-up information? To be honest, I'm not a big fan of putting links in commit logs (not even 'Closes:') and was happy not to see any here. If the domain changes for some reason (which always happens sooner or later), we'd just end up with a bunch of broken links in our commits. If others want to have the Link added I can definitely add them. > Thanks! > > [1] https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/maintainer/configure-git.rst#L33 b4 can add the link, as well. If you prefer, then, I will take this patch as it is. Thanks, Andi
Hi Andi, On Wed, Dec 11, 2024 at 4:43 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > > It's a pity that all this description is lost. I love long > > > explanations especially when they come from test results. > > > > > > Can I add it in the commit log? > > > > What about starting to add Link: tags pointing to lore to I2C commits, > > like most other subsystems do? > > That way people can easily reach any background information (if > > available), and find the corresponding email thread where to report > > issues or follow-up information? > > To be honest, I'm not a big fan of putting links in commit logs > (not even 'Closes:') and was happy not to see any here. If the > domain changes for some reason (which always happens sooner or > later), we'd just end up with a bunch of broken links in our > commits. > > If others want to have the Link added I can definitely add them. In general, we assume kernel.org will survive... > > [1] https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/maintainer/configure-git.rst#L33 > > b4 can add the link, as well. > > If you prefer, then, I will take this patch as it is. Please do so; thanks! Gr{oetje,eeting}s, Geert
On Wed, Dec 11, 2024 at 04:46:45PM +0100, Geert Uytterhoeven wrote: > On Wed, Dec 11, 2024 at 4:43 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > > > It's a pity that all this description is lost. I love long > > > > explanations especially when they come from test results. > > > > > > > > Can I add it in the commit log? > > > > > > What about starting to add Link: tags pointing to lore to I2C commits, > > > like most other subsystems do? > > > That way people can easily reach any background information (if > > > available), and find the corresponding email thread where to report > > > issues or follow-up information? > > > > To be honest, I'm not a big fan of putting links in commit logs > > (not even 'Closes:') and was happy not to see any here. If the > > domain changes for some reason (which always happens sooner or > > later), we'd just end up with a bunch of broken links in our > > commits. > > > > If others want to have the Link added I can definitely add them. > > In general, we assume kernel.org will survive... > > > > [1] https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/maintainer/configure-git.rst#L33 > > > > b4 can add the link, as well. > > > > If you prefer, then, I will take this patch as it is. > > Please do so; thanks! merged to i2c/i2c-host-fixes with the Link: tag. Thanks, Andi
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index 0c2342f86d3d2fbe..a6aeb40aae04d607 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -352,7 +352,7 @@ static int riic_init_hw(struct riic_dev *riic) if (brl <= (0x1F + 3)) break; - total_ticks /= 2; + total_ticks = DIV_ROUND_UP(total_ticks, 2); rate /= 2; }
Currently, the RIIC driver may run the I2C bus faster than requested, which may cause subtle failures. E.g. Biju reported a measured bus speed of 450 kHz instead of the expected maximum of 400 kHz on RZ/G2L. The initial calculation of the bus period uses DIV_ROUND_UP(), to make sure the actual bus speed never becomes faster than the requested bus speed. However, the subsequent division-by-two steps do not use round-up, which may lead to a too-small period, hence a too-fast and possible out-of-spec bus speed. E.g. on RZ/Five, requesting a bus speed of 100 resp. 400 kHz will yield too-fast target bus speeds of 100806 resp. 403226 Hz instead of 97656 resp. 390625 Hz. Fix this by using DIV_ROUND_UP() in the subsequent divisions, too. Tested on RZ/A1H, RZ/A2M, and RZ/Five. Fixes: d982d66514192cdb ("i2c: riic: remove clock and frequency restrictions") Reported-by: Biju Das <biju.das.jz@bp.renesas.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Apart from the rounding issue fixed by this patch, I could not find any bugs in the calculation of the various parameters (based on the formulas in the documentation). Still, the actual (measured) bus speed may still be higher than the target bus speed. Hence this patch is not sufficient to reduce the actual bus speed to safe levels, and I have not yet addded Closes: https://lore.kernel.org/TYCPR01MB11269BFE7A9D3DC605BA2A2A9864E2@TYCPR01MB11269.jpnprd01.prod.outlook.com/ On RZ/A1H (RSK+RZA1): speed actual duty fall rise cks brl brh ------ -------- ---- ---- ---- --- --- --- before: 101600 113 kHz 63 1 4 3 20 10 after: 99181 110 kHz 64 1 4 3 21 10 before: 396726 407 kHz 62 5 5 1 17 9 after: 396726 407 kHz 62 5 5 1 17 9 Note that before commit d982d66514192cdb, the actual values were within spec, so probably the parameters were hand-tuned with the help of scope: 101600 99.2 kHz 63 1 4 3 19 16 396726 370 kHz 62 5 5 1 21 9 RZ/A2M (RZA2MEVB): speed actual duty fall rise cks brl brh ------ -------- ---- ---- ---- --- --- --- before: 100609 115 kHz 63 1 4 4 20 10 after: 98214 111 kHz 64 1 4 4 21 10 before: 402439 459 kHz 61 5 5 2 16 9 after: 392857 446 kHz 62 5 5 2 17 9 RZ/Five (RZ/Five SMARC): speed actual duty fall rise cks brl brh ------ -------- ---- ---- ---- --- --- --- before: 100806 112 kHz 64 0 3 5 15 7 after: 97656 108 kHz 65 0 3 5 16 7 before: 403225 446 kHz 60 3 3 3 12 7 after: 390625 431 kHz 61 3 3 3 13 7 I.e. the actual bus speed is still up to 10% higher than requested. The driver assumes the default register settings: - FER.SCLE = 1 (SCL sync circuit enabled, adds 2 or 3 cycles) - FER.NFE = 1 (noise circuit enabled) - MR3.NF = 0 (1 cycle of noise filtered out) As these are not explicitly set by the driver, I verified that the assumptions are true on all affected platforms. I also tried disabling FER.SCLE and removing the compensation for SCLE on RZ/Five. For a bus speed of 100 kHz, that gave: speed actual duty fall rise cks brl brh ------ ---------- ---- ---- ---- --- --- --- before: 97656 108 kHz 65 0 3 5 16 7 after: 97656 94.7 kHz 63 0 3 5 18 9 which looks better, but obviously the SCL sync circuit must add some value? So it looks like the default values provided by i2c_parse_fw_timings() do not work well for us, and all board DTS files should provide suitable values explicitly, using the "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns" properties. Adam submitted something similar for R-Car a while ago[1]. Thanks for your comments! [1] "[PATCH] arm64: dts: renesas: beacon: Fix i2c2 speed calcuation" https://lore.kernel.org/20210825122757.91133-1-aford173@gmail.com --- drivers/i2c/busses/i2c-riic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)