add watchdog timer for mspm0 i2c driver #44
Conversation
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
639cc23 to
f82d96e
Compare
|
Concerns:
|
There was a problem hiding this comment.
As there is no watchdog driver for mspm0 in beo/3.5.99, there is only the counter you can use.
But when moving to beo/4.1.0 are you planning to move to watchdog api? (ti has provided a watchdog driver, no idea how well it works though). If not, then it will require to detail that you need a counter phandle, or else once running it will just crash.
About disabling coredump, it is because the default backend is the "logging" one which indeed takes too much cpu etc... Perhaps detail that in the commit message. (we will switch to in-memory backend when relevant)
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
f82d96e to
ab940d6
Compare
I think I also saw the driver from ti, but wasn't for mspm0.
Will do! |
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
2 times, most recently
from
ec1081b to
a7efd36
Compare
Once this PR merged, I will add it to beo/4.1.0 so we could try the mspm0 watchdog driver there |
drivers/i2c/i2c_mspm0g3xxx.c
Outdated
| @@ -903,7 +982,7 @@ static const struct i2c_driver_api i2c_mspm0g3xxx_driver_api = { | |||
| \ | |||
| I2C_DEVICE_DT_INST_DEFINE(index, i2c_mspm0g3xxx_init, NULL, &i2c_mspm0g3xxx_data_##index, \ | |||
| &i2c_mspm0g3xxx_cfg_##index, POST_KERNEL, \ | |||
| CONFIG_I2C_INIT_PRIORITY, &i2c_mspm0g3xxx_driver_api); \ | |||
| CONFIG_COUNTER_INIT_PRIORITY, &i2c_mspm0g3xxx_driver_api); \ | |||
There was a problem hiding this comment.
@tbursztyka do you think this change will cause an issue?
There was a problem hiding this comment.
No you do need i2c to start after counter, as it uses the counter device at init phase. But indeed I think this change is not the "right one", I mean: there is a cleaner way to do it.
Just a sec, don't merge yet
There was a problem hiding this comment.
Better add a Kconfig.mspm0 option called config I2C_MSPM0G3XXX_INIT_PRIORITY and set it to COUNTER_INIT_PRIORITY by default. And above use CONFIG_I2C_MSPM0G3XXX_INIT_PRIORITY
When building, the final priority is made of such hardcoded value and one computed though DTS dependency tree.
In the final zephyr.map file you'll find something like:
.z_init_POST_KERNEL50_00066_
0x000000000000fab0 0x8 zephyr/drivers/i2c/libdrivers__i2c.a(i2c_mspm0.c.obj)
.z_init_POST_KERNEL60_00050_
0x000000000000fab8 0x8 zephyr/libzephyr.a(emul_pca954x.c.obj)
See the 50_xxxxx_ or 60_xxxxx_ the first number is the hard-coded one, and the second the one coming from DTS.
It's actually a strip on a wooden-leg: without the proper change on the hardcoded value that you did, even if DTS knows the dependency of i2c to the counter: the final priority would be wrong anyway. (counter has a default value of 60...)
Thus why I tried to fix it (never got to finish it, and selling it damn hard because zephyr way of getting changes in...) zephyrproject-rtos#79340 which would generate a unique number of priority without any hard coded value anywhere. Just a damn dependency tree which is the only right way.
There was a problem hiding this comment.
that's a lot of files changed here! but i definitely see your point.
The priority issue should be fixed in the latest commits
There was a problem hiding this comment.
that's a lot of files changed here! but i definitely see your point.
reg-exp changing all the device instantiation macros , hopefully nothing done by hand. I should get back to it when I'll have time, only 2 tests were failing due to one external patch I was waiting for...
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
a7efd36 to
cbfa554
Compare
|
@tbursztyka the last commit fixes a commit message typo |
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
298ff16 to
64d9f2f
Compare
add properties to configure the I2C watchdog reset timeout and and panic error code. Also, add a new kconfig property to define the driver init priority. Since the i2c driver has a dependency to the counter driver, we need to adjust for that. Signed-off-by: Dimitris Karnikis <dika@bang-olufsen.dk>
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
64d9f2f to
b6bb7c2
Compare
|
@sipraga kind reminder :) |
drivers/i2c/i2c_mspm0g3xxx.c
Outdated
| data->target_rx_valid = | ||
| tconfig->callbacks->write_requested(tconfig); | ||
| i2c_mspm0g3xxx_target_stop_watchdog(data); |
There was a problem hiding this comment.
I'm not sure that this is correct? You essentially want to start the watchdog when a transaction starts, this is good as is. But you only want to stop the watchdog once the ACKOverride is getting set (then we can debate whether to stop the watchdog before or after that)
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
ac16f03 to
7c6d1ac
Compare
|
The changes were checked against i2c reads/writes, and the driver always managed to recover |
describe the watchdog timer property that will reset the mcu and the i2c lines (to high) to avoid potential SCL stuck low issues Signed-off-by: Dimitris Karnikis <dika@bang-olufsen.dk>
When the system is crashing, the I2C lines (SDA/SCL) are not being
reset and might stay LOW. To address this issue, we need to ensure that
none of the user i2c driver callbacks (that are doing reads or writes)
are taking longer than CONFIG_I2C_MSPM0G3XXX_WATCHDOG_TIMEOUT. So we
fire a timer before each call. If the callback doesn't finish fast
enough, the watchdog callback will reset the i2c lines, NACK the i2c
message and panic with the CONFIG_MSPM0G3XXX_I2C_WATCHDOG_PANIC_CODE
error code.
The timeout is configured with CONFIG_I2C_MSPM0G3XXX_WATCHDOG_TIMEOUT
The panic error code is configured with
CONFIG_MSPM0G3XXX_I2C_WATCHDOG_PANIC_CODE.
To make use of this reboot feature, the device tree needs to provide
a reference to a hardware timer. Let's say we want to use timer timg7
as a watchdog:
We need to configure that timer in the dts:
```
&timg7 {
status = "okay";
mode = <ONE_SHOT_DOWN>;
prescaler = <0>;
divide-ratio = <RATE_1>;
};
```
and then reference in the i2c block:
```
&i2c0 {
watchdog-timer = <&timg7>;
extras ...
```
Since we are invoking a software panic on the mcu, make sure to disable
the COREDUMP since the default backend is the "logging" one and it takes
too much of the cpu resources, thus delaying the mcu reboot.
This can be done with:
```
CONFIG_DEBUG_COREDUMP=n
```
Of course for now, the implementation is using a counter timer since
we don't have a mspm0 watchdog.
We need to also use I2C_MSPM0G3XXX_INIT_PRIORITY as the driver init
priority.
Signed-off-by: Dimitris Karnikis <dika@bang-olufsen.dk>
dkarnikis
force-pushed
the
dika/a2b-i2c-watchdog
branch
from
7c6d1ac to
9c273d7
Compare
|
Closed in favor of: #45 |
check individual commits:
Example of an i2c call where the slave spends too much time in the i2c callback and the watchdog timer (in the slave) reset the i2c lines and the mcu