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/
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@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
