MSPM0G1507: MSPM0 SDK smbus library issue and suggestions

Part Number: MSPM0G1507
Other Parts Discussed in Thread: MSPM0G3507, BQ25798, BQ4050

Hi Ti SDK enginners,

I have several questions regarding on your latest SDK.

1.undocumented issue resolution

Recently we upgraded our SDK to 1.30.00.03 and found regression issue. If an I2C device isn't connected, the MCU will struck.

I think that I found the root of cause that in 1.30.00.03, smbus_phy.c:

        case DL_I2C_IIDX_CONTROLLER_NACK:
            ret_state = SMBus_NWK_controllerProcessNACK(smbus);
            __BKPT(0); /* Placeholder for debugging purposes */
        break;

Probably the __BKPT(0) locks the MCU if debugger isn't attached. And this can explain why if a debugger attaches to it, I see a breakpoint stop on where I didn't put (actually in disassembly only, since we were using library, no source code symbol link to that)

I am happy to see this problem was solved in the latest 2.00.00.03. But Isn't disclosed in the SDK release page. Can you please attach brief description of this (regression) issue in SDK release in the future?

2. Unsure the intention of this piece of code, is it a bug?

smbus_phy.c:

SMBus_PHY_controllerStartRx(){
...
    while (timeout &&
               ( (DL_I2C_getControllerStatus(smbus->phy.SMBus_Phy_i2cBase) |
                       DL_I2C_CONTROLLER_STATUS_BUSY) ==
                       DL_I2C_CONTROLLER_STATUS_BUSY ) )
    {
        timeout--;
    }
...
}

In my instinct, should bitfield be check like (get_flags() & bitfield_mask == bitfield_mask) to determine if bitfield_mask is set?

But why this code seems to work? Do you actually want to make sure all other bitfields must be 0? The code makes me confusion.
Can you explain the intention of this piece?

3.suggestion for makefile

Also, I am glad to see that in the 2.00.00.03 SDK, Some of the build option are provided as below.

#ifndef SMB_TARGET_SUPPORTS_HOST_NOTIFY
#define SMB_TARGET_SUPPORTS_HOST_NOTIFY         (true)
#endif
...
#ifndef SMB_CONTROLLER_SUPPORTS_HOST_NOTIFY
#define SMB_CONTROLLER_SUPPORTS_HOST_NOTIFY     (true)
#endif
However, I think if you can provide further external build variable injection method, it will be better. Here is my suggestion:
in build makefile (e.g., mspm0_sdk_2_00_00_03/source/ti/smbus/lib/ticlang/m0p/Makefile)
Put a place holder like $USER_CFLAGS in the beginning of your CFLAGS
CFLAGS = $USER_CFLAGS -I$(TICLANG_ARMCOMPILER)/include .......
AFLAGS = $USER_AFLAGS rc
ASMFLAGS = $USER_ASMFLAGS -I$(TICLANG_ARMCOMPILER)/include .....
Then we can invoke (or include) your makefile inside our make system and set the option through following ways:
SDK_INSTALL_DIR ?= $(MSPM0_SDK_DIR)
TICLANG_ARMCOMPILER ?= $(TI_TOOL_DIR)
GCC_ARMCOMPILER ?= $(GCC_TOOL_DIR)

override NAME=$(SDK_LOCAL_BUILD_SMBUS_DIR)/smbus
override OBJ_DIR=$(SDK_LOCAL_BUILD_SMBUS_DIR)/obj
override SRC_DIR=$(SDK_INSTALL_DIR)/source/ti/smbus

USER_CFLAGS = -DSMB_TARGET_SUPPORTS_HOST_NOTIFY=false -DSMB_CONTROLLER_SUPPORTS_HOST_NOTIFY=false
include  $(SRC_DIR)/lib/ticlang/m0p/Makefile
This method make us easy to maintain the SDK code "as-is" from your release, not changing them manually. It can help us to track the change and turn on/off those features independently across different projects. Because we want to "share" the SDK folder for all projects, not multiple copies with different modifications. This is hard to maintenance and easy to forget to recover the change when updating the SDK.
BTW, why you define -D__MSPM0G3507__ when building the smbus library? Is it required, or it can be removed.

4.target and controller shares the same init

I saw that the smbus module references external function SYSCFG_DL_SMB_I2C_init() to initialize the underlying I2C peripheral. However, due to the namespace, if we use both smbus host and target in the same project (yes, we have two legs of I2C, we are designing a special I2C smbus gateway), it cannot achieve our goal.

I recommend to change your SMBus_controllerInit() and SMBus_targetInit() function prototype, to allow passing a function pointer void *p(void) to it. Then, in your internal implementation, you may call it through this way

SMBus_PHY_targetEnable(){
...
if(smbus->phy.init_fun){
    smbus->phy.init_fun();
}
....

  • Hi Tiger,

    1.undocumented issue resolution

    Bug fixes are usually included in the MSPM0 SDK Release notes that accompany the SDK. There is a note in the release notes on the removal of these breakpoints. Just for clarity, are you looking for something with more details than this? 

    2. Unsure the intention of this piece of code, is it a bug?

    This looks wrong to me as well. I think it should be a bitwise AND rather than an OR in this case. I am filing a bug on this internally to get it updated. I expect it will work only because you eventually exit that loop no matter what due to the timeout countdown. 

    3.suggestion for makefile

    I'm not too familiar with our makefile, but I will forward this to our software team for their input. 

    4.target and controller shares the same init

    Yes, the current way this is written it assumes the existence of that SYSCFG function in order to enable a whole peripheral reset. Passing in the function does seem to me like it would be a more dynamic solution. 

    Best Regards,
    Brandon Fisher

  • Just want to follow up with #2:

    For older version of SDK, it didn't have timeout. But still success. That's why weird to me. Can you further check if some bits actually link together or achieve similar thing?

  • Tiger,

    For older version of SDK, it didn't have timeout. But still success

    When you say success, you mean you are eventually exiting that loop right? What are your stop settings when you were performing the transaction? I suspect that without a stop (i.e. repeated start), you might have issues, as the BUSBY flag would not be cleared, so you might get stuck there. With a stop the BUSBSY should clear, so as long as you don't have an error you should break the loop. 

    Can you further check if some bits actually link together or achieve similar thing?

    I'm not sure I understand your meaning here. Are you asking if there is another way of writing this wait statement that is better? Or for some details about the busy/idle flags in the I2C state machine and their sources? 

    Best Regards,
    Brandon Fisher

  • Hi Brandon,

    Yes, our program didn't encounter issue on 1.2.0.5. Only several chip that request explicit STOP condition that I cannot bypass (start, write, stop, start, read, stop), for bq4050 and bq25798, the library works for me. BTW, I didn't use your smbus.c 's API, I mimic your APIs and created our own based on smbus_nwk. But I don't think that our solution actually bypass that issue.

    2nd, I was meaning, what if i2c->MASTER.MSR all other bits are 0 eventually? Since you | I2C_MSR_BUSY_MASK, so actually that bit doesn't matter at all.

    Only: 0 | I2C_MSR_BUSY_MASK == I2C_MSR_BUSY_MASK

    OR I2C_MSR_BUSY_MASK | I2C_MSR_BUSY_MASK == I2C_MSR_BUSY_MASK