cpu/sam0_common: i2c baudrate calculation fails if CLOCK_CORECLOCK > 51 MHz

benpicco commented 4 years ago

Description

The I2C baudrate is always derived from CLOCK_CORECLOCK, even if a different .gclk_src is selected. If CLOCK_CORECLOCK is too high, the resulting baudrate will is too big and a check in i2c_init() silently fails, leaving the I2C uninitialized.

Any resulting access to the i2c bus will fail.

Steps to reproduce the issue

in examples/default:

USEMODULE += i2c_scan
make BOARD=same54-xpro flash
run i2c_scan 0

Expected results

2019-08-20 14:01:01,264 - INFO #  > i2c_scan 0
2019-08-20 14:01:01,270 - INFO # Scanning I2C device 0...
2019-08-20 14:01:01,279 - INFO # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-08-20 14:01:01,283 - INFO #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-08-20 14:01:01,286 - INFO # 0x00 R R R R R R R R R R R R R R - -
2019-08-20 14:01:01,287 - INFO # 0x10 - - - - - - - - - - - - - - - -
2019-08-20 14:01:01,288 - INFO # 0x20 - - - - - - - - X - - - - - - -
2019-08-20 14:01:01,296 - INFO # 0x30 X - - - - - X - - - - - - - - -
2019-08-20 14:01:01,298 - INFO # 0x40 - - - - - - - - - - - - - - - -
2019-08-20 14:01:01,304 - INFO # 0x50 X - - - - - X - X - - - - - X -
2019-08-20 14:01:01,309 - INFO # 0x60 X - - - - - - - - - - - - - - -
2019-08-20 14:01:01,313 - INFO # 0x70 - - - - - - - - R R R R R R R R

Actual results

2019-08-20 13:56:22,893 - INFO # > i2c_scan 0
2019-08-20 13:56:22,895 - INFO # Scanning I2C device 0...
2019-08-20 13:56:22,901 - INFO # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-08-20 13:56:22,906 - INFO #      0 1 2 3 4 5 6 7 8 9 a b c d e f

the program hangs indefinitely in an -EAGAIN loop.

Versions

RIOT master

dylad commented 4 years ago

Indeed, this is true for SAMD51. The baud calculation should be slightly rework. We should also use the SERCOM GCLK input clock instead of CLOCK_CORECLOCK (which is a legacy from old SAMD21 inplementation). I can have a deeper look after the summit if you want.

benpicco commented 4 years ago

I think it helpful to look at how Zephyr is handling it. But it looks like we need to use a slower GCLK.

dylad commented 4 years ago

One quick workaround could be to use GCLK5 (already used by xtimer) and replace the hardcoded CLOCK_CORECLOCK. Since the issue only affects SAME54 (for now) this could be the way to go for now. In a second time, I think we could put some work into a new way to handle clock init / clock management (like a struct array parse during boot ?) in order to replace all hardcoded stuff among SAM0 drivers.

RIOT-OS / RIOT