i2c: riic: Always round-up when calculating bus period

Commit Message

Geert Uytterhoeven Nov. 22, 2024, 2:14 p.m. UTC

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 <[e-mail]>
Signed-off-by: Geert Uytterhoeven <[e-mail]>
---
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/[e-mail]/

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/[e-mail]
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch

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;
 	}