Skip to content

drivers: smbus: stm32: support packet-error-checking (pec) #96691

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+515 −36
Open

drivers: smbus: stm32: support packet-error-checking (pec) #96691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2703e2f
drivers: smbus: provide packet-error-checking (pec) routines
cfriedt Sep 28, 2025
4483c38
tests: drivers: smbus: add packet error correction (pec) testsuite
cfriedt Oct 3, 2025
f665447
drivers: smbus: stm32: support packet-error-checking (pec)
cfriedt Sep 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions drivers/smbus/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ config SMBUS_INIT_PRIORITY
help
SMBus device driver initialization priority.

config SMBUS_SOFT_PEC
bool "SMBus software PEC support"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this option configurable by the project? It seems rather a driver private config.
... hmmm, i see at least the test sample needs to enable it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's configurable by the project. Technically could be used by any driver or other code.

select CRC
help
Enable software Packet Error Checking (PEC) support.

These generic functions can be used by SMBus drivers for transceivers that do not support
PEC in hardware.

module = SMBUS
module-str = smbus
Expand Down Expand Up @@ -83,6 +91,7 @@ menuconfig SMBUS_STM32
depends on DT_HAS_ST_STM32_SMBUS_ENABLED
depends on I2C_STM32
select PINCTRL
select SMBUS_SOFT_PEC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there are two ways to look at this:

  • PEC is a feature, which is implemented in the STM32 SMBus driver
  • PEC is a capability which is required by the SMBus driver

With the select here it is more the latter one, although it should actually be the first one? But this then would also mean that everything has to be ifdef-ed in the driver itself, so I'm not sure if it is actually a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slowly I start to understand it, you actually need this config-flag so that the utility functions for PEC (which themselves require CRC) are only pulled in when necessary?

Copy link
Member Author

@cfriedt cfriedt Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the select here it is more the latter one, although it should actually be the first one?

"Should" is potentially debatable. E.g. intel_pch_smbus.c implicitly supports PEC.

zephyr/drivers/smbus/intel_pch_smbus.c

Line 332 in d3e645e

if (auxs & PCH_SMBUS_AUXS_CRC_ERROR) {

The stm32 driver is the only other smbus driver in-tree.

It's almost completely platform-independent, actually, with the exception of i2c_stm32_smbalert_enable(), i2c_stm32_smbalert_disable(), and i2c_stm32_set_smbus_mode().

Personally, I would feel better if there was a uniform API, so that PEC can at least be configured by the application, but it isn't disabled by default here (the Zephyr convention), so I can see your point.

everything has to be ifdef-ed in the driver itself, so I'm not sure if it is actually a good idea.

SMBUS_SOFT_PEC could use depends on CRC instead of select, and the stm32 driver could use conditional compilation throughout.

I'm ok to go either way.

@benediktibk , @erwango , @etienne-lms - do you have preferences?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything has to be ifdef-ed in the driver itself, so I'm not sure if it is actually a good idea.

SMBUS_SOFT_PEC could use depends on CRC instead of select, and the stm32 driver could use conditional compilation throughout.

I'm ok to go either way.

I'd go for SMBUS_SOFT_PEC select'ing CRC and the consumer drivers select'ing SMBUS_SOFT_PEC as done in PR right now. This avoids duplicating the select CRC in every driver's Kconfig and is just more sensical IMO.

help
Enable STM32 SMBus driver.

Expand Down
231 changes: 195 additions & 36 deletions drivers/smbus/smbus_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ static int smbus_stm32_configure(const struct device *dev, uint32_t config_value
const struct smbus_stm32_config *config = dev->config;
struct smbus_stm32_data *data = dev->data;

if (config_value & SMBUS_MODE_PEC) {
LOG_ERR("%s: not implemented", dev->name);
return -EINVAL;
}

if (config_value & SMBUS_MODE_HOST_NOTIFY) {
LOG_ERR("%s: not available", dev->name);
return -EINVAL;
Expand Down Expand Up @@ -152,62 +147,200 @@ static int smbus_stm32_quick(const struct device *dev, uint16_t periph_addr,

static int smbus_stm32_byte_write(const struct device *dev, uint16_t periph_addr, uint8_t command)
{
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
.flags = I2C_MSG_WRITE,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_WRITE,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

return i2c_write(config->i2c_dev, &command, sizeof(command), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
smbus_write_prepare_pec(data->config, periph_addr, msgs, num_msgs);
return i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
}

static int smbus_stm32_byte_read(const struct device *dev, uint16_t periph_addr, uint8_t *byte)
{
int ret;
uint8_t pec = 0;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = byte,
.len = sizeof(*byte),
.flags = I2C_MSG_READ,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_READ,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

return i2c_read(config->i2c_dev, byte, sizeof(*byte), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
ret = i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
if (ret < 0) {
return ret;
}

ret = smbus_read_check_pec(data->config, periph_addr, msgs, num_msgs);
if (ret < 0) {
return ret;
}

return 0;
}

static int smbus_stm32_byte_data_write(const struct device *dev, uint16_t periph_addr,
uint8_t command, uint8_t byte)
{
const struct smbus_stm32_config *config = dev->config;
uint8_t buffer[] = {
command,
byte,
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
.flags = I2C_MSG_WRITE,
},
{
.buf = &byte,
.len = sizeof(byte),
.flags = I2C_MSG_WRITE,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_WRITE,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

return i2c_write(config->i2c_dev, buffer, ARRAY_SIZE(buffer), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
smbus_write_prepare_pec(data->config, periph_addr, msgs, num_msgs);
return i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
}

static int smbus_stm32_byte_data_read(const struct device *dev, uint16_t periph_addr,
uint8_t command, uint8_t *byte)
{
int ret;
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
.flags = I2C_MSG_WRITE,
},
{
.buf = byte,
.len = sizeof(*byte),
.flags = I2C_MSG_READ | I2C_MSG_RESTART,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_READ,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

return i2c_write_read(config->i2c_dev, periph_addr, &command, sizeof(command), byte,
sizeof(*byte));
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
ret = i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
if (ret < 0) {
return ret;
}

ret = smbus_read_check_pec(data->config, periph_addr, msgs, num_msgs);
if (ret < 0) {
return ret;
}

return 0;
}

static int smbus_stm32_word_data_write(const struct device *dev, uint16_t periph_addr,
uint8_t command, uint16_t word)
{
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
.flags = I2C_MSG_WRITE,
},
{
.buf = (uint8_t *)&word,
.len = sizeof(word),
.flags = I2C_MSG_WRITE,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_WRITE,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;
uint8_t buffer[sizeof(command) + sizeof(word)];

buffer[0] = command;
sys_put_le16(word, buffer + 1);

return i2c_write(config->i2c_dev, buffer, ARRAY_SIZE(buffer), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
smbus_write_prepare_pec(data->config, periph_addr, msgs, num_msgs);
return i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
}

static int smbus_stm32_word_data_read(const struct device *dev, uint16_t periph_addr,
uint8_t command, uint16_t *word)
{
int ret;
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg messages[] = {
{
.buf = &command,
.len = sizeof(command),
.flags = I2C_MSG_WRITE,
},
{
.buf = (uint8_t *)word,
.len = sizeof(*word),
.flags = I2C_MSG_READ | I2C_MSG_RESTART,
},
{
.buf = &pec,
.len = sizeof(pec),
.flags = I2C_MSG_READ,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;
int result;

result = i2c_write_read(config->i2c_dev, periph_addr, &command, sizeof(command), word,
sizeof(*word));
*word = sys_le16_to_cpu(*word);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(messages));
ret = i2c_transfer(config->i2c_dev, messages, num_msgs, periph_addr);
if (ret < 0) {
return ret;
}

return result;
ret = smbus_read_check_pec(data->config, periph_addr, messages, num_msgs);
if (ret < 0) {
return ret;
}

return 0;
}

static int smbus_stm32_pcall(const struct device *dev, uint16_t periph_addr, uint8_t command,
Expand All @@ -230,8 +363,9 @@ static int smbus_stm32_pcall(const struct device *dev, uint16_t periph_addr, uin
static int smbus_stm32_block_write(const struct device *dev, uint16_t periph_addr, uint8_t command,
uint8_t count, uint8_t *buf)
{
const struct smbus_stm32_config *config = dev->config;
struct i2c_msg messages[] = {
uint8_t pec;
uint8_t num_msgs;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
Expand All @@ -246,18 +380,28 @@ static int smbus_stm32_block_write(const struct device *dev, uint16_t periph_add
.buf = buf,
.len = count,
.flags = I2C_MSG_WRITE,
}
},
{
.buf = &pec,
.len = 1,
.flags = I2C_MSG_WRITE,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

return i2c_transfer(config->i2c_dev, messages, ARRAY_SIZE(messages), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
smbus_write_prepare_pec(data->config, periph_addr, msgs, ARRAY_SIZE(msgs));
return i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
}

static int smbus_stm32_block_read(const struct device *dev, uint16_t periph_addr, uint8_t command,
uint8_t *count, uint8_t *buf)
{
const struct smbus_stm32_config *config = dev->config;

struct i2c_msg messages[] = {
int ret;
uint8_t num_msgs;
uint8_t received_pec;
struct i2c_msg msgs[] = {
{
.buf = &command,
.len = sizeof(command),
Expand All @@ -272,20 +416,35 @@ static int smbus_stm32_block_read(const struct device *dev, uint16_t periph_addr
.buf = buf,
.len = 0, /* written by previous message! */
.flags = I2C_MSG_READ,
}
},
{
.buf = &received_pec,
.len = 1,
.flags = I2C_MSG_READ,
},
};
struct smbus_stm32_data *data = dev->data;
const struct smbus_stm32_config *config = dev->config;

/* Count is read in msg 1 and stored in the len of msg 2.
* This works because the STM I2C driver processes each message serially.
* The addressing math assumes little-endian.
*/
messages[1].buf = (uint8_t *)&messages[2].len;
msgs[1].buf = (uint8_t *)&msgs[2].len;

int res = i2c_transfer(config->i2c_dev, messages, ARRAY_SIZE(messages), periph_addr);
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
ret = i2c_transfer(config->i2c_dev, msgs, num_msgs, periph_addr);
if (ret < 0) {
return ret;
}

*count = messages[2].len;
*count = msgs[2].len;
ret = smbus_read_check_pec(data->config, periph_addr, msgs, num_msgs);
if (ret < 0) {
return ret;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to investigate capturing error statistics, in this block, in a subsequent PR.

Also mentioned here
https://github.com/zephyrproject-rtos/zephyr/pull/96691/files#r2413732281

}

return res;
return 0;
}

static DEVICE_API(smbus, smbus_stm32_api) = {
Expand Down
Loading