drivers: add i2c driver support for MSPM0#94627
drivers: add i2c driver support for MSPM0#94627d-philpot wants to merge 3 commits intozephyrproject-rtos:mainfrom
Conversation
There was a problem hiding this comment.
This driver needs lot of re-factor. In summary,
- ISR context is handling all the data path, which needs to be abstracted.
- Target and Controller support needs to be split.
- Multi target support needs to be compile time based on devicetree target nodes.
- Current usage of data section is high and needs to be optimized based on rodata / data place-holding.
Current code base is not compilable as well. So IMO it makes sense to put in DRAFT until it's ready for review.
teburd
left a comment
There was a problem hiding this comment.
There's some confusing things going on, maybe because the HAL is handling interrupts? It's unclear, but those should be handled by Zephyr not by the HAL if that's the case.
drivers/i2c/i2c_mspm0.c
Outdated
| DL_I2C_startControllerTransfer(config->base, data->addr, DL_I2C_CONTROLLER_DIRECTION_TX, | ||
| data->msg.len); | ||
|
|
||
| while (!(DL_I2C_getControllerStatus(config->base) & DL_I2C_CONTROLLER_STATUS_IDLE)) { |
There was a problem hiding this comment.
Tight polling loops like this aren't inherently wrong but typically there's an interrupt signal available.
Would recommend changing the driver, if possible, to use an interrupt (event driven) state machine that is kicked off by the transfer function, and completed when the state machine signals back to the thread that had called transfer blocking on a k_sem_take by giving a semaphore.
If you really want the tight poll loop here instead, or there's no other option, use a WAIT_FOR() macro with a timeout such that even given a faulty i2c transfer the caller isn't blocked forever and instead may get an error.
There was a problem hiding this comment.
Looking into refactoring transfer to use semaphores
| while (!(DL_I2C_getControllerStatus(config->base) & DL_I2C_CONTROLLER_STATUS_IDLE)) { | ||
| } | ||
|
|
||
| if (data->state == I2C_MSPM0_TIMEOUT) { |
There was a problem hiding this comment.
And I maybe spoke too soon if there's a hardware timeout available.
There was a problem hiding this comment.
There is a configurable SCL low hardware timeout available
| reg: | ||
| required: true | ||
|
|
||
| interrupts: |
There was a problem hiding this comment.
I don't see the interrupts being used?
| #address-cells = <1>; | ||
| #size-cells = <0>; | ||
| reg = <0x400f2000 0x2000>; | ||
| interrupts = <26 0>; |
There was a problem hiding this comment.
It doesn't seem like the driver supports interrupts
There was a problem hiding this comment.
Interrupts are used for controller and target logic
The code base is compliable. Target support is split and optional from controller via kconfig define. Multi target support is compile time based on Kconfig defines |
1f122c6 to
b708dc5
Compare
|
@teburd Ping for re-review. Refactored transmit logic to use semaphores instead of busy wait looping |
tbursztyka
left a comment
There was a problem hiding this comment.
ISR handler would gain in clarity being split: irq types handled in dedicated inline functions and as noted already: target stuff being on their own as well.
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
| /* Driverlib includes */ | ||
| #include <ti/driverlib/dl_i2c.h> | ||
|
|
||
| #define TI_MSPM0_CONTROLLER_INTERRUPTS \ |
There was a problem hiding this comment.
Can you add an "I2C" prefix in the name?
| DL_I2C_INTERRUPT_CONTROLLER_RXFIFO_TRIGGER | DL_I2C_INTERRUPT_CONTROLLER_STOP | \ | ||
| DL_I2C_INTERRUPT_CONTROLLER_TX_DONE | DL_I2C_INTERRUPT_TIMEOUT_A) | ||
|
|
||
| #define TI_MSPM0_TARGET_INTERRUPTS \ |
There was a problem hiding this comment.
Can you add an "I2C" prefix in the name?
|
I am trying to use the I2C shell to scan, read, and write on the LP_MSPM0G3519 board as well as a custom board. It simply hangs for me with this driver implementation. ULPCLK frequency is 40 MHz. I2C controller is configured for 100 kHz. |
|
|
||
| data->state = I2C_MSPM0_IDLE; | ||
|
|
||
| sem_give: |
There was a problem hiding this comment.
This generates warning: label 'sem_give' defined but not used warning when CONFIG_I2C_SCL_LOW_TIMEOUT is 0.
There was a problem hiding this comment.
Changing the value of the timeout as a default to 1 second removes this issue and follows best practices. I believe no further action is necessary
drivers/i2c/i2c_mspm0.c
Outdated
| #include <soc.h> | ||
|
|
||
| #include <zephyr/logging/log.h> | ||
| LOG_MODULE_REGISTER(i2c_mspm0, CONFIG_I2C_LOG_LEVEL); |
There was a problem hiding this comment.
Can you please move this to be after the header includes?
drivers/i2c/i2c_mspm0.c
Outdated
| I2C_MSPM0_TARGET_TX_INPROGRESS, | ||
| I2C_MSPM0_TARGET_RX_INPROGRESS, | ||
| I2C_MSPM0_TIMEOUT, | ||
| I2C_MSPM0_ERROR |
There was a problem hiding this comment.
Nit...
| I2C_MSPM0_ERROR | |
| I2C_MSPM0_ERROR, |
@lsd-he360 Can you provide more detail on your setup? Have you provided sufficient pull ups? I am able to scan for devices, read and write |
@d-philpot, did you have to modify the LP_MSPM0G3519 board or did it just work out of the gate for you? On the custom board, we have 10K pull-ups on SCL/SDA lines. FWIW, we have the ability to make another MCU (RPI PICO for now) the controller, and it works okay with the RPI PICO I2C driver. One other thing I wanted to mention is that if I reverse the order of powerenable and reset calls in https://github.com/msp-ti/zephyr/blob/mspm0_dev_stable/drivers/i2c/i2c_mspm0.c |
@d-philpot we have also scoped the I2C lines and no data or clock come out with the new implementation. This makes it seem unlikely that the pull-up resistors are the culprits. I'm curious on your setup and how the code was tested. Can you provide more details on that? |
drivers/i2c/i2c_mspm0.c
Outdated
| transaction_msg.buf = transaction_buf; | ||
| transaction_msg.len = transaction_len; | ||
|
|
||
| k_sem_take(data->i2c_busy_sem, K_FOREVER); |
There was a problem hiding this comment.
I think you could hang here forever if you are doing an i2c_write_read for example
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/i2c.h#L1495
In the second iteration of the for loop (for the second message), you would attempt to take the sem, but it is not reelased/given until you are out of the loop on line 420.
@d-philpot, the TRM does recommend doing a RESET and then ENABLE just like the code I linked did.
|
|
@lsd-he360 @mshawkyVT I am not performing any additional modification the LP_MSPM0G3519 board. I have tested this implementation with various sensors and an application running the I2C shell. How are you configuring the pins in the overlay? The pins need to be set for input, open drain, and bias disable. |
I am just using the pin configs from here: |
|
@lsd-he360 Updated PR w.r.t reset consideration. Also, can you provide more details on how your setup is failing? Do the transfers timeout? I've just tested it now with a LED driver sensor and see no issues reading or writing. I am glad to hop on a call if you would like. |
|
|
@teburd @parthitce @ssekar15 @tbursztyka ping for review |
It was hanging with your older implementation where there were two issues:
I can give it a try again with your most recent changes |
drivers/i2c/i2c_mspm0.c
Outdated
| #define I2C_TI_MSPM0_CONTROLLER_INTERRUPTS \ | ||
| (DL_I2C_INTERRUPT_CONTROLLER_ARBITRATION_LOST | DL_I2C_INTERRUPT_CONTROLLER_NACK | \ | ||
| DL_I2C_INTERRUPT_CONTROLLER_RXFIFO_TRIGGER | DL_I2C_INTERRUPT_CONTROLLER_STOP | \ | ||
| DL_I2C_INTERRUPT_CONTROLLER_TX_DONE | DL_I2C_INTERRUPT_TIMEOUT_A) | ||
|
|
||
| #define I2C_TI_MSPM0_TARGET_INTERRUPTS \ | ||
| (DL_I2C_INTERRUPT_TARGET_RX_DONE | DL_I2C_INTERRUPT_TARGET_TXFIFO_EMPTY | \ | ||
| DL_I2C_INTERRUPT_TARGET_START | DL_I2C_INTERRUPT_TARGET_STOP | \ | ||
| DL_I2C_INTERRUPT_TIMEOUT_A) |
There was a problem hiding this comment.
Aligned at 4 tab indent to line 80
There was a problem hiding this comment.
Scratch that, aligned with 8-tab indent but using spaces after coding content as per clang-format suggestions
drivers/i2c/i2c_mspm0.c
Outdated
| uint32_t transfer_count; | ||
| uint32_t transfer_len; | ||
| uint8_t *msg_buf; | ||
| #if (IS_ENABLED(CONFIG_I2C_TARGET)) |
There was a problem hiding this comment.
these should all be ifdef CONFIG_I2C_TARGET. IS_ENABLED is for use in code like if (IS_ENABLED(...
drivers/i2c/i2c_mspm0.c
Outdated
| bool is_target; | ||
| }; | ||
|
|
||
| #if (CONFIG_I2C_SCL_LOW_TIMEOUT != 0) |
There was a problem hiding this comment.
nit: no need for the parenthesis
drivers/i2c/i2c_mspm0.c
Outdated
| ret = -EINVAL; | ||
| goto out; |
There was a problem hiding this comment.
just return -EINVAL? why the goto, there's nothing to unwind, same everywhere in this function
There was a problem hiding this comment.
Noted. Change made with goto out removed
drivers/i2c/i2c_mspm0.c
Outdated
| * This function works out to be ceil(clockRate/(desiredSpeed*10)) - 1 | ||
| */ | ||
| period = clock_rate / (desired_speed * 10); | ||
| period -= (clock_rate % (desired_speed * 10) == 0) ? 1 : 0; |
There was a problem hiding this comment.
bit confused, why is the -1 only on the modulo condition? can't wrap my head around it
There was a problem hiding this comment.
I cleaned this up to be more clearly a ceiling round
drivers/i2c/i2c_mspm0.c
Outdated
| static const struct mspm0_sys_clock mspm0_i2c_clockSys##index = \ | ||
| MSPM0_CLOCK_SUBSYS_FN(index); \ | ||
| \ | ||
| I2C_MSPM0_CONFIG_IRQ_FUNC_DECLARE(index); \ |
There was a problem hiding this comment.
just use I2C_MSPM0_CONFIG_IRQ_FUNC here then you don't have to declare it in the first place
There was a problem hiding this comment.
Fixed in the latest version. DECLARE removed
drivers/i2c/i2c_mspm0.c
Outdated
| #define MERGE_BUF_SIZE(index) \ | ||
| COND_CODE_1(DT_NODE_HAS_PROP(DT_NODELABEL(i2c##index), merge_buf_size), \ | ||
| (DT_PROP(DT_NODELABEL(i2c##index), merge_buf_size)), (0)) |
There was a problem hiding this comment.
index here should only be used with INST macros, here you are adding wrong assumptions, imagine if you had i2c0 i2c1 i2c2 labels and i2c1 is disabled, then i2c2 would get index 1 and this would start picking properties from the wrong devices in very hard to debug way
DT_INST_NODE_HAS_PROP(index, merge_buf_size) (if I understand what you are trying to do correctly)
drivers/i2c/i2c_mspm0.c
Outdated
| IF_ENABLED(USES_MERGE_BUF(index), \ | ||
| (static uint8_t mspm0_i2c_msg_buf_##index[MERGE_BUF_SIZE(index)];)); \ |
There was a problem hiding this comment.
I suggest indenting this as if it was an #IF, as it stand it's hard to track, I"d do
IF_ENABLED(USES_MERGE_BUF(index), ( \
static uint8_t mspm0_i2c_msg_buf_##index[MERGE_BUF_SIZE(index)]; \
)); \
\
static const struct i2c_mspm0_config i2c_mspm0_cfg_##index = { \
drivers/i2c/i2c_mspm0.c
Outdated
| .merge_buf_size = MERGE_BUF_SIZE(index), \ | ||
| IF_ENABLED(USES_MERGE_BUF(index), \ | ||
| (.merge_buf = mspm0_i2c_msg_buf_##index,)) .pinctrl = \ | ||
| PINCTRL_DT_INST_DEV_CONFIG_GET(index), \ | ||
| .irq_config_func = i2c_mspm0_irq_config_func_##index, \ | ||
| .i2c_clock_config = { \ | ||
| .clockSel = MSPM0_CLOCK_PERIPH_REG_MASK( \ | ||
| DT_INST_CLOCKS_CELL(index, clk)), \ | ||
| .divideRatio = DL_I2C_CLOCK_DIVIDE_1, \ | ||
| }}; \ | ||
| \ |
There was a problem hiding this comment.
the indentation here is all over the place
drivers/i2c/i2c_mspm0.c
Outdated
| static K_SEM_DEFINE(i2c_busy_sem##index, 1, 1); \ | ||
| static K_SEM_DEFINE(device_sync_sem##index, 0, 1); \ | ||
| static struct i2c_mspm0_data i2c_mspm0_data_##index = { \ | ||
| .i2c_busy_sem = &i2c_busy_sem##index, \ | ||
| .device_sync_sem = &device_sync_sem##index, \ | ||
| }; \ |
There was a problem hiding this comment.
drop this whole block, allocate the semaphore as directly on the structure (not as a pointer), initialize them in runtime, doing the init statically here waste loads of flash for the static initialization and few bytes of RAM
There was a problem hiding this comment.
Semaphores are now initialized at runtime as you suggested. I learned something on this one!
There was a problem hiding this comment.
Can you please change the commit message header to contain: manifest: hal_ti: <msg>?
There was a problem hiding this comment.
This commit was not needed in the new push as the hal_ti has been upstreamed
|
@d-philpot Any updates? I would like to use mspm0 i2c for some BeagleBoard stuff. |
|
Hi Zephyr community, Dylan has had to step down from zephyr development at TI, for the time being I'll be taking back up the reigns for this PR. I've gone ahead and tried to address all comments as best I can before re-pushing. We also have some i2c testing framework on the TI side now and I have confidence in behavior of merging and timeout features on both target and controller side. Compliance checks are passing for me locally, but if anything shows up below I'll be quick to fix it |
Can you run some of the in tree tests and post results? The i2c_ram test in particular covers the API well. I do not have mspm0. That would help build confidence quickly for me that the PR is in reasonable shape and I'd look mostly for structural issues then. |
Hi @teburd, I don't have an I2C Ram chipset on hand locally, but I can elaborate on the testing that we do between two M0's At 100k, 400k, and 1MHz, we do:
|
|
Latest push fixed some whitespace fixes based on compliance checks. Got clarity from Discord on the use of 8 tab indents |
There was a problem hiding this comment.
I have tested i2c target api on mspm0l1117 running greybus and pocketbeagle 2 running linux with gb-i2c-host driver
drivers/i2c/i2c_mspm0.c
Outdated
| static int i2c_mspm0_reset_controller(const struct device *dev) | ||
| { | ||
| const struct i2c_mspm0_config *config = dev->config; | ||
| int ret = 0; |
There was a problem hiding this comment.
doesn't look like this is used? just return 0 at the end
There was a problem hiding this comment.
Good point. Addressed in latest push.
drivers/i2c/i2c_mspm0.c
Outdated
| bool additionalMessages = (i + 1 < num_msgs); | ||
| bool allowMerge = !(msgs[i].flags & I2C_MSG_STOP) && !(msgs[i + 1].flags & I2C_MSG_RESTART); | ||
| bool sameDir = ((msgs[i].flags & I2C_MSG_READ) == (msgs[i + 1].flags & I2C_MSG_READ)); | ||
| bool merge_next = additionalMessages && allowMerge && sameDir; | ||
|
|
||
| return merge_next; |
There was a problem hiding this comment.
wrong camelcase, should be additional_messages allow_merge etc... also you can drop the outer parenthesis on the first and third varaiable, merge_next you can just return the value directly
There was a problem hiding this comment.
noted. Changed to camel_case to follow style guide. Thanks for catching this!
Includes driver and dts binding changes for I2C support Signed-off-by: Jackson Farley <j-farley@ti.com> Signed-off-by: Dylan Philpot <d-philpot@ti.com>
Adds devicetree entries for different I2C peripherals on MSPM0 devices. Signed-off-by: Dylan Philpot <d-philpot@ti.com>
Added i2c pin information to MSPM0 launchpads Signed-off-by: Dylan Philpot <d-philpot@ti.com> Signed-off-by: Jackson Farley <j-farley@ti.com>
|




Adds I2C controller and target support for MSPM0. Includes driver changes, dts and board updates.