zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.08k stars 6.19k forks source link

STM32WL I2C doesn't work with LPM #37414

Open honestech74 opened 2 years ago

honestech74 commented 2 years ago

Describe the bug When enabled the low power management using CONFIG_PM, STM32WL I2C doesn't work with J-Link but sometimes works with STM32CubeProgrammer using openocd (not stm32cubeprogrammer). There is the same issue #37352. Also It's not working when using low power STOP mode manually without CONFIG_PM.

To Reproduce Steps to reproduce the behavior:

  1. Update the board.cmake (boards/arm/nucleo_wl55jc/board.cmake) for nucleo_wl55jc board to use J-Link
    
    # SPDX-License-Identifier: Apache-2.0

board_runner_args(jlink "--device=STM32WL55JC" "--speed=4000" "--reset-after-load") board_runner_args(stm32cubeprogrammer "--port=swd" "--reset=hw") include(${ZEPHYR_BASE}/boards/common/jlink.board.cmake) include(${ZEPHYR_BASE}/boards/common/openocd.board.cmake) include(${ZEPHYR_BASE}/boards/common/stm32cubeprogrammer.board.cmake)

2. Prepare the BME280 sensor or some I2C sensor. If you will use another sensor, please build with your sensor sample.
3. Add `CONFIG_PM` to `samples/sensor/bme280`
4. Build and flash to test the I2C using BME280 sensor.

west build -b nucleo_wl55jc -p auto samples/sensor/bme280 west flash

3. Verify you are using J-Link in console logs like this

-- west flash: using runner jlink -- runners.jlink: JLink version: 7.22b -- runners.jlink: Flashing file: /home/honestech/zephyrproject/zephyr/build/zephyr/zephyr.hex

4. See logs after reset the board

**Expected behavior**
When disabled `CONFIG_PM`, it works well.
When enabled `CONFIG_PM`, it doesn't work at all. Even if the log shows the zero values, you can't see any I2C signals on I2C lines. I have checked the I2C line using the logic analyzer.

**Log**

Booting Zephyr OS build zephyr-v2.6.0-1680-gc4079e4be29a Found device "ENVIRONMENTAL_SENSOR", getting sensor data temp: 0.000000; press: 0.000000; humidity: 0.000000 temp: 0.000000; press: 0.000000; humidity: 0.000000 temp: 0.000000; press: 0.000000; humidity: 0.000000



**Environment (please complete the following information):**
 - OS: Ubuntu 18.04 LTS
 - Toolchain: 0.12.4
 - Commit SHA: `c4079e4be2`
aurel32 commented 2 years ago

As a workaround, you might want to set CONFIG_I2C_STM32_INTERRUPT=n, this solution works fine on the STM32L4.

The correct solution for the STM32L4 is to prevent the SoC going to STOP2 mode where I2C can't function (well except I2C3) when waiting for the I2C interrupt. The I2C peripherals still need the HSI clock in STOP0 or STOP1 mode.

honestech74 commented 2 years ago

@aurel32 You are right, I have removed the STOP2 mode from board dts, and then I2C works in LPM. In fact, I am using I2C2 and it will lose the clock in STOP2 mode.

What I want is to work back after wake up from STOP2 mode. So I have called the I2C driver init(i2c_stm32_init()) after wake up, then it works well. @erwango I think there might be some way to re-init enabled clocks or call driver init functions after wake up from suspend in power management. E.g. can we add something to enable clocks in power.c. I have no clue what's the best way.

__weak void pm_power_state_exit_post_ops(struct pm_state_info info)
{
    switch (info.state) {
    case PM_STATE_SUSPEND_TO_IDLE:
        LL_LPM_DisableSleepOnExit();
        LL_LPM_EnableSleep();
        /* need to restore the clock */
        stm32_clock_control_init(NULL);

        /* Add something */
#ifdef CONFIG_I2C_STM32
        // call i2c_stm32_init or enable i2c clocks due to dts
#endif

        break;
    case PM_STATE_SOFT_OFF:
        /* Nothing to do. */
        break;
    default:
        LOG_DBG("Unsupported power substate-id %u", info.state);
        break;
    }

    /*
     * System is now in active mode.
     * Reenable interrupts which were disabled
     * when OS started idling code.
     */
    irq_unlock(0);
}
aurel32 commented 2 years ago

I am not sure it needs to be re-inited when exiting STOP2 mode. In my case (again STM32L4), the I2C module survives the STOP2 mode as long as there is no I2C transaction running when entering STOP2. That's why disabling CONFIG_I2C_STM32_INTERRUPT works, it makes the CPU busy wait instead of entering LPM.

Now I guess you indeed need to re-init the I2C module if the SoC entered STOP2 mode with a transaction running, which probably totally messes up its state.

honestech74 commented 2 years ago

That's why disabling CONFIG_I2C_STM32_INTERRUPT works, it makes the CPU busy wait instead of entering LPM.

I haven't any issues with CONFIG_I2C_STM32_INTERRUPT, but agreed with it.

the I2C module survives the STOP2 mode as long as there is no I2C transaction running when entering STOP2.

I have no transaction when entering STOP2 mode. Now I have got running by configure the I2C2 again after wake up from STOP2 like this:

    const struct device *i2c_dev = DEVICE_DT_GET(DT_NODELABEL(i2c2));
    i2c_configure(i2c_dev, I2C_MODE_MASTER | I2C_SPEED_STANDARD << I2C_SPEED_SHIFT);

But it's the hard coded configuration for I2C, there is not easy way to convert the I2C clock-frequency of dts to I2C driver configuration like i2c_map_dt_bitrate in i2c-priv.h. It makes me to add some public function in I2C driver and also not easy to add this to LPM. Does anyone have any idea to configure I2C runtime using device tree configuration like i2c_configure(i2c_dev, DT_PROP(DT_NODELABEL(i2c), clock_frequency))? Or can we move the i2c_map_dt_bitrate to i2c.h?

aurel32 commented 2 years ago

I have no transaction when entering STOP2 mode.

Do you mean you have a defined a custom power management policy? For me with the default PM residency policy, the SoC automatically enters the STOP2 mode during an I2C transaction.

Now I have got running by configure the I2C2 again after wake up from STOP2 like this:

  const struct device *i2c_dev = DEVICE_DT_GET(DT_NODELABEL(i2c2));
  i2c_configure(i2c_dev, I2C_MODE_MASTER | I2C_SPEED_STANDARD << I2C_SPEED_SHIFT);

But it's the hard coded configuration for I2C, there is not easy way to convert the I2C clock-frequency of dts to I2C driver configuration like i2c_map_dt_bitrate in i2c-priv.h. It makes me to add some public function in I2C driver and also not easy to add this to LPM. Does anyone have any idea to configure I2C runtime using device tree configuration like i2c_configure(i2c_dev, DT_PROP(DT_NODELABEL(i2c), clock_frequency))? Or can we move the i2c_map_dt_bitrate to i2c.h?

Not sure I can't really help you here, on the STM32L4 there is no need to reconfigure the I2C device when leaving the STOP2 mode. Maybe have a look at the STM32WL datasheet to have a better idea about what has to be restarted? I am afraid that i2c_configure does a lot more operations than really needed in your case.

honestech74 commented 2 years ago

Do you mean you have a defined a custom power management policy? For me with the default PM residency policy, the SoC automatically enters the STOP2 mode during an I2C transaction.

I have to disabled LPM because there are similar issues with other peripherals like ADC. So I have entered STOP mode using STM32 LL library, not using Zephyr PM :-(. After digging the driver, I knew, should call stm32_i2c_configure_timing. I think it should be included to LPM or driver.

[UPDATE] I got the I2C ARLO error when back to running mode from STOP2. If reconfigure the i2c timing using stm32_i2c_configure_timing, it's fixed but not sure if it's the best way.

FRASTM commented 2 years ago

It seems there is no i2c_stm32_pm_control function yet. Depending on the stm32 family : "the I2C must be disabled before entering Stop mode_" . On wakeUp from stop, if the HSI clock is restored, also for the I2C clock source, no specific code should be required for the I2C (registers are saved).

Note that keeping I2C clock is still possible for checking : "All U(S)ARTs, LPUARTs and I 2 Cs have the capability to enable the HSI16 oscillator even when the MCU is in Stop mode (if HSI16 is selected as the clock source for that peripheral)."

erwango commented 2 years ago

Moving from bug to enhancement as PM is not fully implemented yet.

zephyrbot commented 4 months ago

Hi @erwango,

This issue, marked as an Enhancement, was opened a while ago and did not get any traction. Please confirm the issue is correctly assigned and re-assign it otherwise.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@honestech74 you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!

Martdur commented 2 months ago

As suggested @honestech74 re-configure the i2c timings on PM_DEVICE_ACTION_RESUME. It would be nice to introduce the change in a PR.

What i've done so far (don't know if it's good practice):

#if defined(CONFIG_SOC_SERIES_STM32WLX)
static int i2c_stm32_reinit_timing(const struct device *dev)
{
    int ret;
    const struct i2c_stm32_config *cfg = dev->config;
    const struct device *const clk = DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE);
    uint32_t i2c_clock = 0U;

    if (IS_ENABLED(STM32_I2C_DOMAIN_CLOCK_SUPPORT) && (cfg->pclk_len > 1)) {
        if (clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1],
                       &i2c_clock) < 0) {
            LOG_ERR("Failed call clock_control_get_rate(pclken[1])");
            return -EIO;
        }
    } else {
        if (clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[0],
                       &i2c_clock) < 0) {
            LOG_ERR("Failed call clock_control_get_rate(pclken[0])");
            return -EIO;
        }
    }

#ifdef CONFIG_PM_DEVICE_RUNTIME
    ret = clock_control_on(clk, (clock_control_subsys_t)&cfg->pclken[0]);
    if (ret < 0) {
        LOG_ERR("failure Enabling I2C clock");
        return ret;
    }
#endif

    ret = stm32_i2c_configure_timing(dev, i2c_clock);

    return ret;
}
#endif`

And call it in i2c_stm32_pm_action on PM_DEVICE_ACTION_RESUME

erwango commented 2 months ago

@Djammall Sounds like correct, don't hesitate to contribute it and we can discuss the details directly in the PR