pm: device_runtime: Async improvements

[ceolin]

• Avoid unnecessary state transitions if a pending put has not started. In this case, just update the internal states without triggering any device action.
• Add a parameter in the async put to delay the operation.

[ceolin]

@erwango Could you please take a look in the stm32 related changes ? It seemed to me that COUNTER_RTC_STM32_SUBSECONDS is not a hard requirement to enable PM, but I am not completely sure. Please check if that change makes sense please.

[erwango]

It seemed to me that COUNTER_RTC_STM32_SUBSECONDS is not a hard requirement to enable PM, but I am not completely sure.

Well, I guess it is if you want reliable tick count. @niedzwiecki-dawid can provide more details.

[niedzwiecki-dawid]

RTC freq without COUNTER_RTC_STM32_SUBSECONDS is 1Hz. System timer will miss every timeout after entering low power mode, but it will eventually wake up and announce a correct number of ticks. Personally, I would call it hard dependency.

However the change itself looks good, because COUNTER_RTC_STM32_SUBSECONDS depends on RTC being enabled.

I think adding the second build assert that RTC is enabled would make sense.

[ceolin]

RTC freq without COUNTER_RTC_STM32_SUBSECONDS is 1Hz. System timer will miss every timeout after entering low power mode, but it will eventually wake up and announce a correct number of ticks. Personally, I would call it hard dependency.

The other solution is make it a dependency to enable PM in these targets. I let it open to you folks to decide what is preferable here. Note that one can make it work only declaring power states with residency timer bigger than the timer resolution. The system will save less power but won't miss scheduled events.

However the change itself looks good, because COUNTER_RTC_STM32_SUBSECONDS depends on RTC being enabled.

I think adding the second build assert that RTC is enabled would make sense.

[JordanYates]

Love the idea, it's a much cleaner solution than my OOT hack to get equivalent functionality.

I would like to see a test that exercises the case where get is called while the suspending work is running. For example put a sleep in the transition function, then call get while it is sleeping.

[erwango]

Can you explain when you faced this issue ?
In theory this shouldn't be required as rtc node should be enabled when CONFIG_PM=y.
It might not be the case when running generic PM tests for instance. But in that case we should ensure these tests are only ran on PM pre configured targets (with the required node enabled).

So I'd prefer you remove this, and we fix the board configuration issue.

[ceolin]

this is broken in HEAD. Just try to enable PM for these targets e.g:

ceolin@vault:/mnt/nvme/zephyrproject/zephyr$ west build -p -b mikroe_mini_m4_for_stm32 samples/hello_world/ -- -DCONFIG_PM=y
-- west build: making build dir /mnt/nvme/zephyrproject/zephyr/build pristine
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: /mnt/nvme/zephyrproject/zephyr/samples/hello_world
-- CMake version: 3.25.1
-- Found Python3: /usr/bin/python3 (found suitable version "3.11.2", minimum required is "3.8") found components: Interpreter 
-- Cache files will be written to: /home/ceolin/.cache/zephyr
-- Zephyr version: 3.5.99 (/mnt/nvme/zephyrproject/zephyr)
-- Found west (found suitable version "1.1.0", minimum required is "0.14.0")
-- Board: mikroe_mini_m4_for_stm32
-- Found host-tools: zephyr 0.16.3 (/mnt/nvme/zephyrproject/zephyr-sdk-0.16.3)
-- Found toolchain: zephyr 0.16.3 (/mnt/nvme/zephyrproject/zephyr-sdk-0.16.3)
-- Found Dtc: /mnt/nvme/zephyrproject/zephyr-sdk-0.16.3/sysroots/x86_64-pokysdk-linux/usr/bin/dtc (found suitable version "1.6.0", minimum required is "1.4.6") 
-- Found BOARD.dts: /mnt/nvme/zephyrproject/zephyr/boards/arm/mikroe_mini_m4_for_stm32/mikroe_mini_m4_for_stm32.dts
-- Generated zephyr.dts: /mnt/nvme/zephyrproject/zephyr/build/zephyr/zephyr.dts
-- Generated devicetree_generated.h: /mnt/nvme/zephyrproject/zephyr/build/zephyr/include/generated/devicetree_generated.h
-- Including generated dts.cmake file: /mnt/nvme/zephyrproject/zephyr/build/zephyr/dts.cmake

warning: COUNTER_RTC_STM32_SUBSECONDS (defined at drivers/counter/Kconfig.stm32_rtc:47) has direct dependencies !SOC_SERIES_STM32F1X && COUNTER_RTC_STM32 && COUNTER with value n, but is currently being y-selected by the following symbols:
 - PM (defined at soc/arm/silabs_exx32/efr32bg22/Kconfig.defconfig.series:18, soc/arm/silabs_exx32/efr32bg27/Kconfig.defconfig.series:18, soc/arm/silabs_exx32/efr32mg24/Kconfig.defconfig.series:19, soc/arm/st_stm32/stm32f4/Kconfig.defconfig.series:20, subsys/pm/Kconfig:13), with value y, direct dependencies (SOC_SERIES_EFR32BG22 && SOC_FAMILY_EXX32) || (SOC_SERIES_EFR32BG27 && SOC_FAMILY_EXX32) || (SOC_SERIES_EFR32MG24 && SOC_FAMILY_EXX32) || SOC_SERIES_STM32F4X || (SYS_CLOCK_EXISTS && HAS_PM) (value: y), and select condition SOC_SERIES_STM32F4X (value: y)
Parsing /mnt/nvme/zephyrproject/zephyr/Kconfig
Loaded configuration '/mnt/nvme/zephyrproject/zephyr/boards/arm/mikroe_mini_m4_for_stm32/mikroe_mini_m4_for_stm32_defconfig'
Merged configuration '/mnt/nvme/zephyrproject/zephyr/samples/hello_world/prj.conf'
Merged configuration '/mnt/nvme/zephyrproject/zephyr/build/zephyr/misc/generated/extra_kconfig_options.conf'

error: Aborting due to Kconfig warnings

CMake Error at /mnt/nvme/zephyrproject/zephyr/cmake/modules/kconfig.cmake:355 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /mnt/nvme/zephyrproject/zephyr/cmake/modules/zephyr_default.cmake:129 (include)
  /mnt/nvme/zephyrproject/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /mnt/nvme/zephyrproject/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:92 (include_boilerplate)
  CMakeLists.txt:5 (find_package)

[erwango]

Being fixed by #66359

[ceolin]

Love the idea, it's a much cleaner solution than my OOT hack to get equivalent functionality.

I would like to see a test that exercises the case where get is called while the suspending work is running. For example put a sleep in the transition function, then call get while it is sleeping.

This is already tested.

	test_driver_pm_async(test_dev);

	/* usage: 1, -1, suspend: yes (queued) */
	ret = pm_device_runtime_put_async(test_dev);
	zassert_equal(ret, 0);

	(void)pm_device_state_get(test_dev, &state);
	zassert_equal(state, PM_DEVICE_STATE_SUSPENDING);

	/* let suspension start */
	k_yield();

	/* create and start get_runner thread
	 * get_runner thread is used to test synchronous path while asynchronous
	 * is ongoing. It is important to set its priority >= to the system work
	 * queue to make sure sync path run by the thread is forced to wait.
	 */
	k_thread_create(&get_runner_td, get_runner_stack,
			K_THREAD_STACK_SIZEOF(get_runner_stack), get_runner,
			NULL, NULL, NULL, CONFIG_SYSTEM_WORKQUEUE_PRIORITY, 0,
			K_NO_WAIT);
	k_yield();

	/* let driver suspend to finish and wait until get_runner finishes
	 * resuming the driver
	 */
	test_driver_pm_done(test_dev);
	k_thread_join(&get_runner_td, K_FOREVER);

	(void)pm_device_state_get(test_dev, &state);
	zassert_equal(state, PM_DEVICE_STATE_ACTIVE);

The action callback in the driver blocks waiting a semaphore that is only given when test_driver_pm_done(test_dev); is called.

[erwango]

LGTM aside the stm32 change which is being resolved in a different way.

[erwango]

Nice. I see a welcome use of the new pm_device_runtime_put_async in console in order to avoid useless uart device suspension while there are more chars to be sent.

[ceolin]

LGTM aside the stm32 change which is being resolved in a different way.

@erwango what is the proposal ? Make this dependency a hard requirement for PM in these targets ? That change is not related with this pr and we could track that problem in a different issue / pr

[erwango]

@erwango what is the proposal ? Make this dependency a hard requirement for PM in these targets ? That change is not related with this pr and we could track that problem in a different issue / pr

#66359 was one part of it.
Then indeed make this dependency a hard requirement for PM in these targets. In any case, the change you proposed is not fitting exactly the need and should not be merged as is. We can handle this in a different PR.

[erwango]

Changing my mind on the STM32 change. It is not ideal but is a lesser evil compared to other solutions.