os/pm, os/driver/pm: Add pm_timer module #6129

ritesh55555 · 2024-04-03T11:29:58Z

This commit adds pm_timer module in os/pm to use multiple timers for PM operation and PM driver also.
It defines a structure for multiple wakeup timers. All the timers are stored in a list and just before system sleeps, the timer with nearest future expiration time is started.

os/include/tinyara/fs/ioctl.h

os/drivers/pm/pm.c

os/board/rtl8730e/src/component/bluetooth/example/ble_scatternet/scatternet.c

os/include/tinyara/pm/pm.h

os/pm/pm_activity.c

os/pm/pm_timer/pm_timer.h

os/pm/pm_timer/pm_timer_initialize.c

os/pm/pm_timer/pm_timer_create.c

os/pm/pm_timer/pm_timer.h

os/include/tinyara/pm/pm.h

os/drivers/pm/pm.c

os/include/tinyara/fs/ioctl.h

edwakuwaku · 2024-04-03T15:49:14Z

Hi @ritesh55555 ,
Initially, I thought the timer resource manager should be held under /dev/timer, as timer is not only specific to PM module.
In os/board/rtl8730e/src/rtl8730e_boot.c, we initialize the timers w.r.t the timer driver (ie. /dev/timer), . Could you please let me know how are we going to link this pm timer with the lower level timer driver control?
If you check this PR #6064, the final setup for timer is during up_set_pm_timer in os/arch/arm/src/amebasmart/amebasmart_pmhelpers.c. In the ioctl for pm driver, other than state control and dvfs, others are all related to timer operation.
My question is, how do we realise the relation between /dev/timer and /dev/pm?

ritesh55555 · 2024-04-04T01:58:28Z

Hi @ritesh55555 , Initially, I thought the timer resource manager should be held under /dev/timer, as timer is not only specific to PM module. In os/board/rtl8730e/src/rtl8730e_boot.c, we initialize the timers w.r.t the timer driver (ie. /dev/timer), . Could you please let me know how are we going to link this pm timer with the lower level timer driver control? If you check this PR #6064, the final setup for timer is during up_set_pm_timer in os/arch/arm/src/amebasmart/amebasmart_pmhelpers.c. In the ioctl for pm driver, other than state control and dvfs, others are all related to timer operation. My question is, how do we realise the relation between /dev/timer and /dev/pm?

the new modification still uses g_pm_timer, pm_set_timer() and pm_init_handler() as before . The PM LOCK TIMER is same as before. the moment app will request pm_lock with timer, it will set pm_set_timer(PM_LOCK_TIMER, timer_interval) and do the up_set_pm_timer() and start the timer as before.
But we wont be using pm_unlock with timer , that has been removed becuase that didnot make sense.
Here we can add multiple wake up timers (struct pm_wakeup_timer_s) in a list , so whenever board is going to sleep , just before that , we can get the right time , find its delay and then do pm_set_timer(PM_WAKEUP_TIMER, delay).

So /dev/pm uses the os/pm apis , and the final setup for timer is same as before. During up_set_pm_timer in amebasmart_pmhelpers.c .
The only difference is we have multiple timers (struct pm_wakeup_timer_s) in a list in increasing order of their expiration time. Just before sleep, pm module will call an api that will set the appropriate wakeup timer. These all will use one hardware timer.

i hope this explains!

os/board/rtl8730e/src/component/bluetooth/example/ble_scatternet/scatternet.c

ritesh55555 · 2024-04-04T08:16:49Z

@edwakuwaku Hi currently for pm_lock with timer , we dont have a way to stop the timer . Suppose we want to stop it inbetween by calling pm_unlock. It will only pm_relax() but it will not stop the timer that was started by pm_lock timer. Then during expiration, it will call pm_relax() again and it will crash.
Can we have an api in amebasmart_pmhelpers to stop the timers. currently there is only up_set_pm_timer() which only starts. I think we need another api to stop the timers also.

edwakuwaku · 2024-04-04T08:27:48Z

@edwakuwaku Hi currently for pm_lock with timer , we dont have a way to stop the timer . Suppose we want to stop it inbetween by calling pm_unlock. It will only pm_relax() but it will not stop the timer that was started by pm_lock timer. Then during expiration, it will call pm_relax() again and it will crash. Can we have an api in amebasmart_pmhelpers to stop the timers. currently there is only up_set_pm_timer() which only starts. I think we need another api to stop the timers also.

Can you try

void up_stop_pm_timer(void) {
    gtimer_stop(&g_timer1);
    RTIM_INTClear(TIMx[TIMER1]);
}

edwakuwaku · 2024-04-04T08:35:19Z

@edwakuwaku Hi currently for pm_lock with timer , we dont have a way to stop the timer . Suppose we want to stop it inbetween by calling pm_unlock. It will only pm_relax() but it will not stop the timer that was started by pm_lock timer. Then during expiration, it will call pm_relax() again and it will crash. Can we have an api in amebasmart_pmhelpers to stop the timers. currently there is only up_set_pm_timer() which only starts. I think we need another api to stop the timers also.

Let me confirm with my understanding, for the stop it in between before pm_unlock, I assume that you do not need this timer to trigger wakeup or handle some other operation. Thus you would like to stop it, so that it will not trigger callback to do pm_relax at the same state more than once, is that correct?

ritesh55555 · 2024-04-04T09:03:30Z

@edwakuwaku Hi currently for pm_lock with timer , we dont have a way to stop the timer . Suppose we want to stop it inbetween by calling pm_unlock. It will only pm_relax() but it will not stop the timer that was started by pm_lock timer. Then during expiration, it will call pm_relax() again and it will crash. Can we have an api in amebasmart_pmhelpers to stop the timers. currently there is only up_set_pm_timer() which only starts. I think we need another api to stop the timers also.

Let me confirm with my understanding, for the stop it in between before pm_unlock, I assume that you do not need this timer to trigger wakeup or handle some other operation. Thus you would like to stop it, so that it will not trigger callback to do pm_relax at the same state more than once, is that correct?

yes , i just want to stop it . No callback needed.

os/drivers/pm/pm.c

os/pm/pm_timer/pm_timer.h

os/include/tinyara/pm/pm.h

os/pm/pm_changestate.c

os/pm/pm_timer/pm_timer_create.c

os/pm/pm_timer/pm_timer_set.c

Taejun-Kwon · 2024-04-11T12:40:14Z

os/drivers/pm/pm.c

+ * Data
+ ****************************************************************************/
+static const struct file_operations pm_fops = {
+	0,                          /* open */


how it works without open? lock, reference count and something else are unnecessary?

os/drivers/pm/pm.c

os/pm/pm_timer/pm_timer_create.c

os/pm/pm_timer/pm_timer_set.c

os/include/tinyara/pm/pm.h

os/pm/pm_sleep.c

ewoodev · 2024-05-08T05:52:48Z

os/include/tinyara/pm/pm.h

+	int pid;                          		      /* id of process that created pm timer */
+	uint8_t flags;                    		      /* See pm timer definations above*/
+	int delay;		          		      /* refers to time of sleep expected */
+	void (*callback)(struct pm_wakeup_timer_s *timer);    /* function to be executed when timer expires */


it is some strange..
please check this

ewoodev · 2024-05-08T06:02:47Z

os/pm/Kconfig

@@ -45,6 +45,15 @@ config PM_METRICS_DURATION

 endif

+config PM_MAX_WAKEUP_TIMER


how about PM_MAX_STATIC_TIMER ?

And this pr is having both pm_wakeup_timer and pm_timer.
How about unifying the terms into one?

os/pm/pm_timer/pm_timer_delete.c

os/pm/pm_timer/pm_timer_create.c

os/pm/pm_timer/pm_timer_activity.c

os/pm/pm_timer/pm_timer_delete.c

1. this commit adds a semaphore variable to the timer structure. This will allow the system to lock the thread before and after pm sleep. Here the "thread" refers to the thread that called pm_sleep api through pm driver. 2. This commit adds a callback api to the timer structure. Whenever a pm timer expires, this api will be called.

ewoodev · 2024-05-13T01:30:50Z

apps/shell/tash_command.c

@@ -561,12 +561,11 @@ int tash_execute_cmd(char **args, int argc)
 			if (tash_cmds_info.cmd[cmd_idx].exec_type == TASH_EXECMD_SYNC) {
 				/* function call to execute SYNC command */
 #ifdef CONFIG_PM
-				pm_arg_t pm_arg = {PM_IDLE_DOMAIN, PM_NORMAL};


Please seperate commit

ewoodev · 2024-05-13T01:43:29Z

os/drivers/pm/pm.c

+	case PMIOC_SLEEP:
+		if (arg > 0) {
+			/* Converting from milliseconds to ticks */
+			arg = ((arg * CLOCKS_PER_SEC) /  1000);


Do we not need to provide an API in milliseconds for kernel functions that use pm_sleep rather than driver?

I think it is better to do this in the pm_sleep() function. And if necessary, please change the name to pm_msleep().

ewoodev · 2024-05-13T01:47:05Z

os/include/tinyara/pm/pm.h

-	uint8_t timer_type;		/* Bits here are set according to the timer that is to be used */
-	uint32_t timer_interval;	/* The interval for whichever timer is to be used */
-	clock_t last_wifi_alive_send_time;       /* Time tick of the last wifi keep alive signal sent time */
+struct pm_wakeup_timer_s {	


We use pm_timer in all APIs, But only in this structure data type, we call it pm_wakeup_timer.

Please change it to pm_timer as well.

Comment that our pm_timer means waking up the main core on time.

ewoodev · 2024-05-13T01:54:38Z

os/pm/pm_timer/pm_timer_activity.c

+	if (curr->delay > CONFIG_PM_MIN_SLEEP_TIME) {
+		pmvdbg("Setting timer and Board will wake up after %d millisecond\n", curr->delay);
+		/* Converting ticks to nanoseconds */
+		up_set_pm_timer((curr->delay * 1000000) / CLOCKS_PER_SEC);


Please use TICK2NSEC(curr->delay)

ewoodev · 2024-05-13T01:57:58Z

os/pm/pm_timedStay.c

+	/* PM transition will be relaxed here */
+	if (pid_timerMap[pid] != NULL) {
+		pm_relax(PM_IDLE_DOMAIN, (enum pm_state_e)state);
+		wd_delete(pid_timerMap[pid]);


Please change PIDHASH(pid)

ewoodev · 2024-05-13T02:01:37Z

os/pm/pm_timedStay.c

+
+	/* Check if there is already a wdog lock timer running for 
+	 * the process */
+	if (pid_timerMap[pid] != NULL) {


If there is already a timer, it is recommended that we do not clear the timer and start again after wd_cancel.

And if timer_interval is entered as 0, please call wd_cancel and wd_delete only(not start).

I would be failed when a new thread with the same pid hash comes in.

ewoodev · 2024-05-13T09:59:09Z

os/pm/pm_sleep.c

+ *
+ ************************************************************************/
+
+int pm_msleep(int timer_interval)


please change file name to pm_msleep()

os/pm/pm_timer/pm_timer_delete.c

os/pm/pm_timer/pm_timer.h

os/pm/pm_timedStay.c

ewoodev · 2024-05-13T10:26:23Z

os/pm/pm_timedStay.c

+/* This array maps the pid to their respective wdog timer. 
+ * Assumption: when a thread does a timed lock , only the same thread 
+ * can unlock the "timed lock" before it expire */
+static WDOG_ID pid_timerMap[CONFIG_MAX_TASKS];


Please check coding rule.

g_pid_timer_map[]

- This commit adds pm_timedstay api that can perform pm state suspend for a given time duration.It also updates pm driver code.

- this commit updates the pm suspend and pm resume code for SYNC cmd because of the new changes in the pm driver.

sunghan-chang reviewed Apr 3, 2024

View reviewed changes

edwakuwaku reviewed Apr 3, 2024

View reviewed changes

os/drivers/pm/pm.c Outdated Show resolved Hide resolved

edwakuwaku reviewed Apr 3, 2024

View reviewed changes

os/drivers/pm/pm.c Outdated Show resolved Hide resolved

edwakuwaku reviewed Apr 3, 2024

View reviewed changes

os/include/tinyara/fs/ioctl.h Outdated Show resolved Hide resolved

ritesh55555 force-pushed the pm_driver_timer branch 8 times, most recently from 49f5138 to 20f6028 Compare April 4, 2024 06:31

abhishek-samsung reviewed Apr 4, 2024

View reviewed changes

os/board/rtl8730e/src/component/bluetooth/example/ble_scatternet/scatternet.c Outdated Show resolved Hide resolved

ritesh55555 force-pushed the pm_driver_timer branch from 20f6028 to 8bf9929 Compare April 4, 2024 07:22

ritesh55555 force-pushed the pm_driver_timer branch 4 times, most recently from 54c997e to 97f64b1 Compare April 8, 2024 05:10

ewoodev suggested changes Apr 8, 2024

View reviewed changes

Taejun-Kwon reviewed Apr 11, 2024

View reviewed changes

ritesh55555 force-pushed the pm_driver_timer branch 4 times, most recently from d3f8c52 to 29ad25f Compare April 12, 2024 07:19

ritesh55555 force-pushed the pm_driver_timer branch 4 times, most recently from 9cd8cde to 7a9907c Compare May 8, 2024 03:18

ewoodev reviewed May 8, 2024

View reviewed changes

ritesh55555 force-pushed the pm_driver_timer branch 3 times, most recently from c0eb0c5 to c4e911e Compare May 8, 2024 06:53

ewoodev reviewed May 8, 2024

View reviewed changes

os/pm/pm_timer/pm_timer_delete.c Outdated Show resolved Hide resolved

os/pm/pm_timer/pm_timer_create.c Outdated Show resolved Hide resolved

edwakuwaku reviewed May 8, 2024

View reviewed changes

os/pm/pm_timer/pm_timer_activity.c Outdated Show resolved Hide resolved

os/pm/pm_timer/pm_timer_delete.c Show resolved Hide resolved

ritesh55555 force-pushed the pm_driver_timer branch 2 times, most recently from a89212e to 433556c Compare May 8, 2024 08:41

ritesh55555 force-pushed the pm_driver_timer branch 2 times, most recently from ceaf664 to 31b6d56 Compare May 8, 2024 10:35

ewoodev reviewed May 13, 2024

View reviewed changes

ritesh55555 force-pushed the pm_driver_timer branch 2 times, most recently from 9e43040 to 44244c1 Compare May 13, 2024 09:52

ewoodev reviewed May 13, 2024

View reviewed changes

ritesh55555 and others added 2 commits May 13, 2024 16:28

os/pm/pm_timedstay: suspend pm state for given time interval

8e47c15

- This commit adds pm_timedstay api that can perform pm state suspend for a given time duration.It also updates pm driver code.

apps/shell/tash_command.c: Update pm suspend for SYNC cmd

99f1e09

- this commit updates the pm suspend and pm resume code for SYNC cmd because of the new changes in the pm driver.

ritesh55555 force-pushed the pm_driver_timer branch from 3048e30 to 99f1e09 Compare May 13, 2024 11:02

ewoodev approved these changes May 13, 2024

View reviewed changes

sunghan-chang approved these changes May 13, 2024

View reviewed changes

sunghan-chang merged commit 6a341e0 into Samsung:master May 13, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os/pm, os/driver/pm: Add pm_timer module #6129

os/pm, os/driver/pm: Add pm_timer module #6129

ritesh55555 commented Apr 3, 2024 •

edited

edwakuwaku commented Apr 3, 2024 •

edited

ritesh55555 commented Apr 4, 2024 •

edited

ritesh55555 commented Apr 4, 2024

edwakuwaku commented Apr 4, 2024

edwakuwaku commented Apr 4, 2024 •

edited

ritesh55555 commented Apr 4, 2024

Taejun-Kwon Apr 11, 2024

ewoodev May 8, 2024

ewoodev May 8, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

ewoodev May 13, 2024

		@@ -45,6 +45,15 @@ config PM_METRICS_DURATION

		endif

		config PM_MAX_WAKEUP_TIMER

os/pm, os/driver/pm: Add pm_timer module #6129

os/pm, os/driver/pm: Add pm_timer module #6129

Conversation

ritesh55555 commented Apr 3, 2024 • edited

edwakuwaku commented Apr 3, 2024 • edited

ritesh55555 commented Apr 4, 2024 • edited

ritesh55555 commented Apr 4, 2024

edwakuwaku commented Apr 4, 2024

edwakuwaku commented Apr 4, 2024 • edited

ritesh55555 commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritesh55555 commented Apr 3, 2024 •

edited

edwakuwaku commented Apr 3, 2024 •

edited

ritesh55555 commented Apr 4, 2024 •

edited

edwakuwaku commented Apr 4, 2024 •

edited