Skip to content

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Oct 8, 2025

Add SMBus Process Call / Process Call Block APIs for STM32.

This implementation also supports PEC mode (packet error checking) and is dependent on PEC support already being supported in-tree.

For more details, please see Sections 6.5.6 and 6.5.8 of the SMBus 3.1 Specification.
https://smbus.org/specs/SMBus_3_1_20180319.pdf

@cfriedt cfriedt requested review from erwango and etienne-lms October 8, 2025 12:36
@erwango erwango requested a review from benediktibk October 8, 2025 12:38
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Commit "drivers: smbus: stm32: add support for pcall api" LGTM.

etienne-lms
etienne-lms previously approved these changes Oct 13, 2025
@cfriedt
Copy link
Member Author

cfriedt commented Oct 13, 2025

@erwango or @finikorg - PTAL when you have a moment.

@erwango
Copy link
Member

erwango commented Oct 15, 2025

@benediktibk

Copy link
Contributor

@benediktibk benediktibk left a comment

Choose a reason for hiding this comment

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

In the SMBus specification the process call seems to be specified differently:
grafik

There is no count for send and receive with a variable length, just a hard coded word exchange (write and read)?

There seems to be no difference between version 3.1 and 3.2 of the SMBus specification regarding the process call, btw.

* This works because the STM I2C driver processes each message serially.
* The addressing math assumes little-endian.
*/
msgs[3].buf = (uint8_t *)&msgs[4].len;
Copy link
Contributor

Choose a reason for hiding this comment

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

neat trick

Copy link
Member Author

@cfriedt cfriedt Oct 20, 2025

Choose a reason for hiding this comment

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

I think that came from @jgrowdenTT 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, but I can't claim credit :) it's a repeat of the idea in smbus_stm32_block_read

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably @alewycky-tenstorrent in that case 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a little unsafe to blindly trust a size from the wire 🙂

But since the driver already does it...

Copy link
Contributor

@etienne-lms etienne-lms Oct 21, 2025

Choose a reason for hiding this comment

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

Indeed. If caller is expected to set the input value to the size available in the output buffer, it would be worth checking that the received data won't overflow memory. As you said @mathieuchopstm, it's seems something to address driver wide (possibly outside of this P-R).

@etienne-lms
Copy link
Contributor

In the SMBus specification the process call seems to be specified differently:
(...)
There is no count for send and receive with a variable length, just a hard coded word exchange (write and read)?

I see this change implements the block write/read process call (Section "6.5.8 Block Write-Block Read Process Call" of the spec).

Indeed the "Process Call" (single word) this is already implemented by smbus_stm32_pcall() handler but currently does not support PEC. Commit message (and P-R description) should be fixed.

@cfriedt, @jgrowdenTT, did you plan to also implement single word "Process Call"?

@cfriedt
Copy link
Member Author

cfriedt commented Oct 15, 2025

did you plan to also implement single word "Process Call"?

@etienne-lms - I can take a look at it

@cfriedt cfriedt marked this pull request as draft October 15, 2025 15:21
@cfriedt
Copy link
Member Author

cfriedt commented Oct 15, 2025

In the SMBus specification the process call

Sorry - my bad, this was the pcall block API. I'll add the pcall API shortly.

Add SMBus Block Write-Block Read Process Call API for STM32.

This implementation also supports PEC mode (packet error checking) and is
dependent on PEC support already being supported in-tree.

Signed-off-by: James Growden <jgrowden@tenstorrent.com>
Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
@cfriedt cfriedt marked this pull request as ready for review October 19, 2025 23:43
@cfriedt
Copy link
Member Author

cfriedt commented Oct 19, 2025

@etienne-lms @benediktibk @mathieuchopstm @erwango - please take another look

Copy link
Contributor

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM


return result;
num_msgs = smbus_pec_num_msgs(data->config, ARRAY_SIZE(msgs));
smbus_write_prepare_pec(data->config, periph_addr, msgs, ARRAY_SIZE(msgs));
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that the PEC byte is read here. Is this correct?

(Also, am I misunderstanding or is the PEC byte completely ignored?? Shouldn't smbus_read_check_pec be called?)

Copy link
Contributor

Choose a reason for hiding this comment

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

image

That's correct. The transaction/PEC code for the block-write-block-read pcall should work here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

* This works because the STM I2C driver processes each message serially.
* The addressing math assumes little-endian.
*/
msgs[3].buf = (uint8_t *)&msgs[4].len;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a little unsafe to blindly trust a size from the wire 🙂

But since the driver already does it...

Implement smbus_stm32_pcall() with packet error correction.

For more information, please see chapter 6.5.6 of the SMBus specification.
https://smbus.org/specs/SMBus_3_1_20180319.pdf

Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Copy link

* This works because the STM I2C driver processes each message serially.
* The addressing math assumes little-endian.
*/
msgs[3].buf = (uint8_t *)&msgs[4].len;
Copy link
Contributor

@etienne-lms etienne-lms Oct 21, 2025

Choose a reason for hiding this comment

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

Indeed. If caller is expected to set the input value to the size available in the output buffer, it would be worth checking that the received data won't overflow memory. As you said @mathieuchopstm, it's seems something to address driver wide (possibly outside of this P-R).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants