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@xxxxxxxxxxxxxx>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxx
> ---
> 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
>
