-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Fix stm32wl i2c driver LPM #72805
base: main
Are you sure you want to change the base?
Fix stm32wl i2c driver LPM #72805
Conversation
Hello @Martdur, and thank you very much for your first pull request to the Zephyr project! |
Please squash your commits and provide a commit with a clear message. See urls provided by bot in upper message. |
drivers/i2c/i2c_ll_stm32.c
Outdated
@@ -435,6 +471,9 @@ static int i2c_stm32_pm_action(const struct device *dev, enum pm_device_action a | |||
switch (action) { | |||
case PM_DEVICE_ACTION_RESUME: | |||
err = i2c_stm32_activate(dev); | |||
#if defined(CONFIG_SOC_SERIES_STM32WLX) | |||
i2c_stm32_reinit_timing(dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not calling already available i2c_stm32_runtime_configure
?
Why not calling it from i2c_stm32_activate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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 ofi2c_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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 oneuint32_t
value to a I2C register usingLL_I2C_SetTiming
. An alternative would be to just store this value ini2c_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 insidei2c_stm32_data
- Store the value at initialisation using
dev->data.i2c_clock = i2c_clock
- Use
LL_I2C_SetTiming(dev->data.i2c_clock)
insidei2c_stm32_activate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
1da796d
to
bcf2830
Compare
@Martdur you have some CI compliance issues (https://github.com/zephyrproject-rtos/zephyr/actions/runs/9112793159/job/25053821771?pr=72805). |
bcf2830
to
8d49453
Compare
8d49453
to
1a9ddf2
Compare
Fixes the power managment of the i2c driver for the stm32wl (#[37414]). Initialisation parameters for the stm32wl were lost when going out of power mode STOP2 leaving the i2c driver in a unitialize state. Re-initialising i2c timing when pm device resume solves the issue. Signed-off-by: Martin Durietz <martin.durietz@gmail.com> specified code for WL devices
1a9ddf2
to
1779190
Compare
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) |
|
I understand this PR can be further optimized from a power consumption perspective, but this commit fixes something that is broken today. How about splitting this work into two steps - fix the main branch first and optimize it with the correct PM approach? |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Keep it open |
Description
This PR introduce a fix for this issue :#37414 by adding a timing reinitialization when PM resume
Type of change
Test steps
samples/sensor/bme280
.CONFIG_PM=y
nucleo_wl55jc
CONFIG_PM