PM runtime dynamic clock switching implementation by keyonjie · Pull Request #3205 · thesofproject/sof

keyonjie · 2020-07-17T09:10:53Z

This is to implement the pm runtime dynamic clock switching which is needed for the TGL 007 release, necessary for making WoV works with LPRO in S0Ix and HPRO for normal usage to meet the bandwidth requirement for DSM and other high MCPS scenarios. The LPRO_ONLY mode is also preserved for debug usage.

lgirdwood

This seems like a generic feature. We should have a CONFIG_DYNAMIC_CLOCK_SWITCHING that should enabled this. If CONFIG_DYNAMIC_CLOCK_SWITCHING is false then we use a static clock (default HP clock otherwise LPRO if selected).

lgirdwood · 2020-07-21T14:31:35Z

It's probably best to put this depends CONFIG_DEBUG sine it's debug only.
I would also say "Force use of Low Power Oscillator"

It's probably best to put this depends CONFIG_DEBUG sine it's debug only.

Okay, let me add this dependency. @mmaka1 @lbetlej are you OK for this?

I would also say "Force use of Low Power Oscillator"

for Dynamic switching mode, the LPRO is also used, so anyway LPRO will always be used/enabled, LPRO_ONLY is more accurate here?

I'd not couple CONFIG_DEBUG and LPRO_ONLY since the former is used to compile extra debugability which may not fit into smaller MCPS budget. On the other hand, it is still useful to create special non-debug build with LPRO_ONLY turned on and run simple pipeline test for quick verification of MCPS consumption by base components.

lgirdwood · 2020-07-21T14:35:50Z

It looks like most of these function bodies are conditionally compiled code, this may be better if you had two version of each function here, one for static clock (LPRO or HP clock) the other for dynamic clock.

We don't have HPRO only mode, so basically

by default clock switching is used, at least we need to switch to LPRO when in D0i3, and switch back to HPRO when performing D0i3 -> D0.

if USE_LPRO_IN_WAITI is selected, it will switch to LPRO during WFI, no matter in D0 or in D0i3.

LPRO only for debug purpose only, where only LPRO is used and therefore no clock switching is needed for all scenarios including 1) and 2).

The preserving of the LPRO_ONLY mode increases the complexity of this conditionally compiling, that's one of the reason why I wanted to remove the debug mode initially, @mmaka1 @lbetlej

We want to keep LPRO_ONLY mode. It would improve readability if there are three ver. of the function compiled conditionally as @lgirdwood noted.

We want to keep LPRO_ONLY mode. It would improve readability if there are three ver. of the function compiled conditionally as @lgirdwood noted.

OK, let me change to use 3 separated versions.

keyonjie · 2020-07-22T02:05:48Z

This seems like a generic feature. We should have a CONFIG_DYNAMIC_CLOCK_SWITCHING that should enabled this. If CONFIG_DYNAMIC_CLOCK_SWITCHING is false then we use a static clock (default HP clock otherwise LPRO if selected).

Thanks for reviewing. I thought about adding this, but then I realized that this item is not selectable(it should be always Y) since we have to use LPRO in D0i3. That's why we don't need adding CONFIG_DYNAMIC_CLOCK_SWITCHING item.

mmaka1 · 2020-07-28T19:26:59Z

Clock switching may base on pm_runtime settings in short term. However, the pm_runtime calls should be prepared for unbalanced pm_gate ipc requests, by replacing ref-counter with a bitmap (one bit per client, could be reduced to a single bool/bit for host->pm_gate use only in short term).

Clock switching may base on pm_runtime settings in short term. However, the pm_runtime calls should be prepared for unbalanced pm_gate ipc requests, by replacing ref-counter with a bitmap (one bit per client, could be reduced to a single bool/bit for host->pm_gate use only in short term).

Thanks for comments, @mmaka1 . I just created a new PR to handle that issue, please help to review:
#3252

This looks over complex. Is there any reason why we dont store the clock status in uncache memory by default ? You are also checking PM and cached states twice which is not atomic.

@keyonjie I'm still seeing the "complex" version here ? Have you pushed the new version ?

also far easier to flow if you do

freq_idx = uncache(&active_freq_idx); pm_is_active = pm_runtime_is_active(PM_blah, MASTER_CORE); if (pm_is_active) { if (freq_idx != LPRO) do_this; } else { if (freq_idx != HPRO) do_that; }

also far easier to flow if you do

freq_idx = uncache(&active_freq_idx); pm_is_active = pm_runtime_is_active(PM_blah, MASTER_CORE); if (pm_is_active) { if (freq_idx != LPRO) do_this; } else { if (freq_idx != HPRO) do_that; }

sure, let me add these variables for easier understanding.

BTW, have you got my changes that adding the hold of lock for the atomic issue you were worrying about, is that OK to you?

yes, GH all working now.

@lgirdwood BTW, I will prefer this as it is simple enough for understanding after 2 new variables introduced:

/* perform clock switching if the PM active status not aligned with the clock frequency */ if (!pm_is_active && freq_idx != CPU_LPRO_FREQ_IDX) set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX, true); else if (pm_is_active && freq_idx != CPU_HPRO_FREQ_IDX) set_cpu_current_freq_idx(CPU_HPRO_FREQ_IDX, true);

But you are checking the same logic twice, someone using a code scanning tool will just come along an fix it.

mmaka1 · 2020-07-30T09:17:51Z

SOFCI TEST

mmaka1 · 2020-07-31T15:43:28Z

This version of on_waiti/on_wakeup is removed by the previous commit and restored by this one. Should those two ones be squashed to avoid intermediate commit that has some regression?

good point, let me squash them.

keyonjie · 2020-08-11T06:23:20Z

@mmaka1 @lgirdwood ping for review, I think this is the last debt for the TGL 007 release.

lgirdwood · 2020-08-11T12:50:07Z

This looks over complex. Is there any reason why we dont store the clock status in uncache memory by default ? You are also checking PM and cached states twice which is not atomic.

keyonjie · 2020-08-11T13:54:05Z

@lgirdwood looks I can't reply your last comment and requested change directly, so type it here.

This looks over complex. Is there any reason why we dont store the clock status in uncache memory by default ? You are also checking PM and cached states twice which is not atomic.

For the complex, I think you are referring to old version code (don't know why github shows you the old one).
For the cached memory, I think usually DSP cores will access cached memory (while DMA access the uncached ones), so cached one is default? We need to access the uncached memory here to make sure they are consistent for different DSP cores.
For the PM and cached states check, as here is the WFI context so this will be executed atomically, no?

lgirdwood

Am I still seeing the old version of the patches ?

lgirdwood · 2020-08-13T14:01:02Z

@keyonjie I'm still seeing the "complex" version here ? Have you pushed the new version ?

keyonjie · 2020-08-13T15:01:39Z

Am I still seeing the old version of the patches ?

I think so. the commit 9b079b224a7a007 actually looks like this:
https://github.com/thesofproject/sof/commit/9b079b224a7a007da5fd358357e6dcaa3fc0946b.patch

No those lines as you quoted below:

#if !(CONFIG_CAVS_LPRO_ONLY)
	/* perform the possible dynamic clock switching */

lgirdwood · 2020-08-14T14:11:53Z

@keyonjie ok, it looks like we are both looking at the same file. My comment still stands here.

+void platform_clock_on_waiti(void)
 {
-	pm_runtime_get(CORE_HP_CLK, cpu_get_id());
+	/* perform the possible dynamic clock switching */
+	if (*cache_to_uncache(&active_freq_idx) != CPU_LPRO_FREQ_IDX &&
+	    !pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID))
+		set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX, true);
+	else if (*cache_to_uncache(&active_freq_idx) != CPU_HPRO_FREQ_IDX &&
+		 pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID))
+		set_cpu_current_freq_idx(CPU_HPRO_FREQ_IDX, true);
+
+	/* check if waiti HPRO->LPRO switching is needed */
+	pm_runtime_put(CORE_HP_CLK, cpu_get_id());
 }

This races as active_freq_idx can be changed in between first and second tests likewise with the pm_runtime_is_active() conditions.

keyonjie · 2020-08-14T15:25:02Z

@keyonjie ok, it looks like we are both looking at the same file. My comment still stands here.

+void platform_clock_on_waiti(void)
 {
-	pm_runtime_get(CORE_HP_CLK, cpu_get_id());
+	/* perform the possible dynamic clock switching */
+	if (*cache_to_uncache(&active_freq_idx) != CPU_LPRO_FREQ_IDX &&
+	    !pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID))
+		set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX, true);
+	else if (*cache_to_uncache(&active_freq_idx) != CPU_HPRO_FREQ_IDX &&
+		 pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID))
+		set_cpu_current_freq_idx(CPU_HPRO_FREQ_IDX, true);
+
+	/* check if waiti HPRO->LPRO switching is needed */
+	pm_runtime_put(CORE_HP_CLK, cpu_get_id());
 }

This races as active_freq_idx can be changed in between first and second tests likewise with the pm_runtime_is_active() conditions.

Got your point now, let me use the pm_runtime lock to make this atomic.

keyonjie · 2020-08-17T01:57:57Z

SOFCI TEST

Today how we are using CONFIG_CAVS_LPRO is actually using LPRO only, rename to make the naming accurate. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

only The config item CONFIG_CAVS_LPRO (CONFIG_CAVS_LPRO_ONLY now) should be used for debugging only at the moment, remove it from Chrome configs. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

LPRO_ONLY is not set When LPRO_ONLY is set, the item CAVS_USE_LPRO_IN_WAITI is not configurable, and no clock switching is possible. Change to enable CAVS_USE_LPRO_IN_WAITI selection only when LPRO_ONLY is not set. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Define CPU_LPRO_FREQ_IDX, CPU_HPRO_FREQ_IDX and CPU_DEFAULT_IDX for all cAVS platforms. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2020-08-17T10:35:56Z

@lgirdwood updated now.

lgirdwood · 2020-08-17T10:43:43Z

You are checking pm_is_active twice.

if (pm_is_active) { /* set LPRO clock if not already enabled */ if (freq_idx != CPU_LPRO_FREQ_IDX) set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX, true); } else { /* set HPRO clock if not already enabled */ if (req_idx != CPU_HPRO_FREQ_IDX) set_cpu_current_freq_idx(CPU_HPRO_FREQ_IDX, true); }

OK, changed now.

Let's select WAKEUP_HOOK for all cAVS platforms, and just do nothing inside if that's the true. 1. When CONFIG_CAVS_LPRO_ONLY=y, use LPRO only, no clock switching. 2. When CONFIG_CAVS_LPRO_ONLY=n, dynamic clock switching will be enabled, and the clock switching will be performed during platform_wait_for_interrupt() only. In platform_clock_on_waiti(), perform dynamic clock switching if needed (active_freq_idx will be updated to denote the active non-wfi clock frequecy index), and then put CORE_HP_CLK to perform possible HPRO->LPRO switching if CONFIG_CAVS_USE_LPRO_IN_WAITI is selected. In platform_clock_on_wakeup(), when CONFIG_CAVS_USE_LPRO_IN_WAITI is selected, get CORE_HP_CLK to perform switching back to HPRO if HPRO is the saved non-wfi active clock, and no more switching needed. Otherwise, need to restore the active cpu freq_idx manually. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2020-08-18T07:27:59Z

Thanks for reviewing, @mmaka1 and @lgirdwood
CI result is good, let's perform merging of this.

keyonjie requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners July 17, 2020 09:10

keyonjie requested a review from mrajwa July 17, 2020 09:11

keyonjie mentioned this pull request Jul 17, 2020

Dynamic clk switch #3150

Closed

keyonjie force-pushed the lpro branch from 7498824 to 68d51fa Compare July 20, 2020 13:55

keyonjie changed the title ~~LPRO renaming to LPRO_ONLY~~ PM runtime dynamic clock switching implment Jul 20, 2020

keyonjie requested a review from slawblauciak July 20, 2020 14:00

keyonjie changed the title ~~PM runtime dynamic clock switching implment~~ PM runtime dynamic clock switching implement Jul 20, 2020

keyonjie force-pushed the lpro branch 6 times, most recently from acd5df6 to ad6204b Compare July 21, 2020 00:23

keyonjie changed the title ~~PM runtime dynamic clock switching implement~~ PM runtime dynamic clock switching implementation Jul 21, 2020

lgirdwood requested changes Jul 21, 2020

View reviewed changes

keyonjie mentioned this pull request Jul 24, 2020

cavs: clk: add timeout to wait loop to avoid dead loop #3192

Closed

slawblauciak mentioned this pull request Jul 28, 2020

[BUG][TGL][DSM]Sound from speaker0,0 is distortion when playback and capture on speaker at the same time #3073

Closed

mmaka1 reviewed Jul 28, 2020

View reviewed changes

mmaka1 reviewed Jul 31, 2020

View reviewed changes

keyonjie force-pushed the lpro branch from ad6204b to 9b079b2 Compare August 10, 2020 07:18

keyonjie requested review from lgirdwood and mmaka1 August 10, 2020 07:19

lgirdwood approved these changes Aug 11, 2020

View reviewed changes

lgirdwood requested changes Aug 11, 2020

View reviewed changes

bkokoszx mentioned this pull request Aug 12, 2020

[BUG][Smart_Amp] ipc timeout in smart_amp pipelines test #3249

Closed

keyonjie mentioned this pull request Aug 13, 2020

Support extended manifest config data for clock config thesofproject/linux#2297

Closed

mmaka1 approved these changes Aug 13, 2020

View reviewed changes

keyonjie requested a review from lgirdwood August 13, 2020 11:04

lgirdwood requested changes Aug 13, 2020

View reviewed changes

keyonjie force-pushed the lpro branch from 9b079b2 to 1d4e3db Compare August 14, 2020 15:36

keyonjie requested a review from lgirdwood August 14, 2020 15:43

keyonjie added 4 commits August 17, 2020 18:13

platform: rename CONFIG_CAVS_LPRO to CONFIG_CAVS_LPRO_ONLY

59c3315

Today how we are using CONFIG_CAVS_LPRO is actually using LPRO only, rename to make the naming accurate. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

config: override: chrome: not enable CONFIG_CAVS_LPRO as it is for debug

59aa292

only The config item CONFIG_CAVS_LPRO (CONFIG_CAVS_LPRO_ONLY now) should be used for debugging only at the moment, remove it from Chrome configs. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

cAVS: clk: configure default CPU clocks

ceeabab

Define CPU_LPRO_FREQ_IDX, CPU_HPRO_FREQ_IDX and CPU_DEFAULT_IDX for all cAVS platforms. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie force-pushed the lpro branch from 1d4e3db to 73b104e Compare August 17, 2020 10:27

lgirdwood reviewed Aug 17, 2020

View reviewed changes

keyonjie force-pushed the lpro branch from 73b104e to f47228a Compare August 17, 2020 12:06

lgirdwood approved these changes Aug 17, 2020

View reviewed changes

lgirdwood merged commit b4a6cea into thesofproject:master Aug 18, 2020

Conversation

keyonjie commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Jul 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmaka1 commented Jul 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Aug 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Aug 11, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Aug 13, 2020

Uh oh!

lgirdwood commented Aug 14, 2020

Uh oh!

keyonjie commented Aug 14, 2020

Uh oh!

keyonjie commented Aug 17, 2020

Uh oh!

keyonjie commented Aug 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Aug 18, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

keyonjie commented Jul 17, 2020 •

edited

Loading