Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JordanAceto opened this issue Jun 12, 2024 · 3 comments
Closed

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

JordanAceto opened this issue Jun 12, 2024 · 3 comments
Assignees
Labels
enhancement New feature, request, or updating to latest version

Comments

@JordanAceto
Copy link

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:

MXC_GPIO_Config(&gpio_cfg_i2c2);

And the definition of the mxc_gpio_cfg_t struct is here:

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_GPIO_VSSEL_VDDIO, MXC_GPIO_DRVSTR_0 };

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
Copy link
Contributor

Jake-Carter commented Jul 1, 2024

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?

@Jake-Carter Jake-Carter self-assigned this Jul 1, 2024
@Jake-Carter Jake-Carter added the enhancement New feature, request, or updating to latest version label Jul 1, 2024
@JordanAceto
Copy link
Author

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!

@Jake-Carter
Copy link
Contributor

Thanks @JordanAceto, will close out for now

@Jake-Carter Jake-Carter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or updating to latest version
Projects
None yet
Development

No branches or pull requests

2 participants