analogdevicesinc / msdk

Software Development Kit for Analog Device's MAX-series microcontrollers
Apache License 2.0
60 stars 75 forks source link

I2C SDA and SDC pins are hardcoded to use VDDIO as vssel #1047

Open JordanAceto opened 2 weeks ago

JordanAceto commented 2 weeks ago

Hi there,

I am working with the MAX32666 on a FTHR2 board. I configured I2C2 as a master like this (simplified for clarity):

MXC_I2C_Init(MXC_I2C2_BUS0, I2C_CFG_MASTER_MODE, 0);
MXC_I2C_SetFrequency(MXC_I2C2_BUS0, MXC_I2C_STD_MODE);

... and tied the 10k pullups to 3.3v using jumper J4 on the FTHR2.

With an oscilloscope I found that the SDA and SCL lines had a maximum of around 2.2v, not the expected 3.3v.

Running through the debugger I found that during the MXC_I2C_Init() call in i2c_me14.c a mxc_gpio_cfg_t is used to configure the SDA and SCL pins. This is hardcoded to use VDDIO as the .vssel field.

The init call is here: https://github.com/analogdevicesinc/msdk/blob/1430a0c9fcddbd691bd7a26f8ea8ade7e3b678ba/Libraries/PeriphDrivers/Source/I2C/i2c_me14.c#L61

And the definition of the mxc_gpio_cfg_t struct is here: https://github.com/analogdevicesinc/msdk/blob/1430a0c9fcddbd691bd7a26f8ea8ade7e3b678ba/Libraries/PeriphDrivers/Source/SYS/pins_me14.c#L76-L77

In my main function I added this code to re-initialize the SDA and SCL pins to use VDDIOH after the MXC_I2C_Init() call, and this does now bring the high level of both lines to 3.3v:

    const mxc_gpio_cfg_t i2c2_sda_pin = {
        .port = MXC_GPIO1,
        .mask = MXC_GPIO_PIN_15,
        .pad = MXC_GPIO_PAD_NONE,
        .func = MXC_GPIO_FUNC_ALT1,
        .vssel = MXC_GPIO_VSSEL_VDDIOH,
        .drvstr = MXC_GPIO_DRVSTR_0,
    };
    MXC_GPIO_Config(&i2c2_sda_pin);

    const mxc_gpio_cfg_t i2c2_scl_pin = {
        .port = MXC_GPIO1,
        .mask = MXC_GPIO_PIN_14,
        .pad = MXC_GPIO_PAD_NONE,
        .func = MXC_GPIO_FUNC_ALT1,
        .vssel = MXC_GPIO_VSSEL_VDDIOH,
        .drvstr = MXC_GPIO_DRVSTR_0,
    };
    MXC_GPIO_Config(&i2c2_scl_pin);

Question: Is this the intended way to use an I2C bus with 3.3v pullups? Is there some more ergonomic config I could do? It seems easy to miss this and then wonder why your pullups aren't pulling all the way to 3.3v. I'm definitely operating under the assumption that I might have missed something here, but if not there might be an opportunity for a slightly cleaner I2C config.

Thanks!

Jake-Carter commented 1 day ago

Hi @JordanAceto, you're right this is not very well-exposed to the user.

Your current solution is a good workaround. If you wanted something a bit you more compact you could copy the existing struct and just overwrite the VSSEL field:

#include "mxc_pins.h"
// ...
mxc_gpio_cfg_t i2c2_pins_new = gpio_cfg_i2c2;
i2c2_pins_new.vssel = MXC_GPIO_VSSEL_VDDIOH;
MXC_GPIO_Config(&i2c2_pins_new);

In general I think we can offer a compile-time solution for switching the default logic level for mxc_pins. Almost all definitions currently uses VDDIO.

Something like:

// pins_me14.c

#ifndef MXC_PINS_VSSEL
#define MXC_PINS_VSSEL MXC_GPIO_VSSEL_VDDIO
#endif
// ...
const mxc_gpio_cfg_t gpio_cfg_i2c2 = { MXC_GPIO1, (MXC_GPIO_PIN_14 | MXC_GPIO_PIN_15), MXC_GPIO_FUNC_ALT1,
                                       MXC_GPIO_PAD_NONE, MXC_PINS_VSSEL, MXC_GPIO_DRVSTR_0 };
//                                                                 ^

Overwriting it would look like:

# project.mk
PROJ_CFLAGS += -DMXC_PINS_VSSEL=MXC_GPIO_VSSEL_VDDIOH

Not the cleanest string to write, but at least gives you the option. What do you think?

JordanAceto commented 10 hours ago

Jake, that all makes sense, thanks. I think in our case your more compact config will be fine. As I work more with the MSDK the paradigms make more sense. Now that I understand that most everything defaults to VDDIO I won't be surprised next time.

For what we're doing we have a couple different voltage domains and we have some pins on VDDIO and some on VDDIOH, so a compile-time setting that impacts all the pins doesn't help too much. In the end we'll actually have one I2C pulled to 1.8v and another one puled to 3.3v. But this idea might be useful for other people if you think it's worthwhile.

So, from my perspective you could close this with no action if you want, or if you think it's worth the time you could add the compile time solution you presented. Either way thanks for looking at it and thanks for the suggestion for the more compact config!