Fix stm32wl i2c driver LPM

[Martdur]

Description

This PR introduce a fix for this issue :#37414 by adding a timing reinitialization when PM resume

Type of change

• Bug fix

Test steps

• Choose a i2c sensor sample (Choose the one you have). For example samples/sensor/bme280.
• Add CONFIG_PM=y
• Build for the nucleo_wl55jc
• Check that the sample work well with and without CONFIG_PM

[github-actions]

Hello @Martdur, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[erwango]

Please squash your commits and provide a commit with a clear message. See urls provided by bot in upper message.

[erwango]

Why not calling already available i2c_stm32_runtime_configure ?
Why not calling it from i2c_stm32_activate ?

[Martdur]

• Found that was no necessary to repeat all operation from i2c_stm32_runtime_configure
• i2c_stm32_activate Is called else where, my thinking was that timing reinit wasn't needed at every call of i2c_stm32_activate .

Do you think it's still relevant to create the i2c_stm32_reinit_timing function or just call i2c_stm32_runtime_configure inside i2c_stm32_activate is enough ?

[erwango]

Do you think it's still relevant to create the i2c_stm32_reinit_timing function or just call i2c_stm32_runtime_configure inside i2c_stm32_activate is enough ?

I see 2 benefits:

• smaller code increase
• validated code reuse (as for instance your proposal would have missed the clock_off call)

So, if my proposal is functional, I'd suggest to make use of it.

This being said, I recall that correctly reinitializing I2C when existing Stop2 used to require significant changes such as #60998. See #59194 (comment).
Btw, maybe @adrienbruant you'd be able to confirm if current PR is sufficient (which may be the case since several changes have been introduced in PM subsystem since that time) ?

[aurel32]

i2c_stm32_runtime_configure is a relatively big function, that at the end just write one uint32_t value to a I2C register using LL_I2C_SetTiming. An alternative would be to just store this value in i2c_stm32_data, and always write the register before the transaction, within the block protected from suspending the device or going to stop modes.

That would also avoid configuring registers in i2c_stm32_runtime_configure that anyway are going to be lost when the device goes into stop 2.

On the drawback, it's a much bigger code change, so maybe it's an optimization to keep for later.

[erwango]

That would also avoid configuring registers in i2c_stm32_runtime_configure that anyway are going to be lost when the device goes into stop 2.

Note that this is specific to WL

i2c_stm32_runtime_configure is a relatively big function, that at the end just write one uint32_t value to a I2C register using LL_I2C_SetTiming. An alternative would be to just store this value in i2c_stm32_data, and always write the register before the transaction, within the block protected from suspending the device or going to stop modes.

This is a fair point. I'm fine with this proposal as well.

[adrienbruant]

In our branch, we use the PM_NOTIFIER introduced by #60998 to restore from STOP2. Our restore routine is not as well thought out as we call i2c_stm32_configure_clk_and_registers(), a looong function basically call init again.

@erwango Can you point to the changes introduced in the PM subsystem? This might work for us, in the sense that the peripheral is now restored from the pessimist assumption that it needs to be fully reconfigured. Isn't that a suboptimal solution for STOP1 and lighter sleep modes?

EDIT: I referenced a custom function from our repo

[Martdur]

i2c_stm32_runtime_configure is a relatively big function, that at the end just write one uint32_t value to a I2C register using LL_I2C_SetTiming. An alternative would be to just store this value in i2c_stm32_data,

@aurel32 Can you point me out how to do so ?

As I understand changes include:

• Create new uint32_t i2c_clock var inside i2c_stm32_data
• Store the value at initialisation using dev->data.i2c_clock = i2c_clock
• Use LL_I2C_SetTiming(dev->data.i2c_clock) inside i2c_stm32_activate

[aurel32]

In our branch, we use the PM_NOTIFIER introduced by #60998 to restore from STOP2. Our restore routine is not as well thought out as we call i2c_stm32_configure_clk_and_registers(), a looong function.

@erwango Can you point to the changes introduced in the PM subsystem? This might work for us, in the sense that the peripheral is now restored from the pessimist assumption that it needs to be fully reconfigured. Isn't that a suboptimal solution for STOP1 and lighter sleep modes?

Yes, this is suboptimal, but this is a first step before mitigations can be applied:

• make the reconfiguration less costly as suggested above
• add PM runtime support (similar to drivers: spi: stm32: add runtime PM support #72991 on the SPI side) to disable the device as soon as it is not needed anymore, that way there is no need to save it around a STOP transition. As a bonus is reduces the runtime power consumption.

[ajarmouni-st]

@Martdur you have some CI compliance issues (https://github.com/zephyrproject-rtos/zephyr/actions/runs/9112793159/job/25053821771?pr=72805).
Code can be formatted using clang-format -i my_source_file.c & you can check for compliance issues locally before pushing again using ./scripts/ci/check_compliance.py --commits HEAD~n..HEAD (with n being the number of commits in your PR)

[hubpav]

I have been testing this pull request on a fleet of ~100 devices in production for the past two weeks, and it works very well. Huge thanks to @Martdur - delivering our project on a tight schedule without your commit would be hard.

[erwango]

I have been testing this pull request on a fleet of ~100 devices in production for the past two weeks, and it works very well. Huge thanks to @Martdur - delivering our project on a tight schedule without your commit would be hard.

Thanks for this feedback. Unfortunately, this can't be merged as is and basically the way to go is #72805 (comment)

[Martdur]

Thanks for this feedback. Unfortunately, this can't be merged as is and basically the way to go is #72805 (comment)
@erwango The last modification that I brought doesn't satisfy what aurel32 said ?