Dynamic clk switch by keyonjie · Pull Request #3150 · thesofproject/sof

keyonjie · 2020-07-08T09:20:26Z

cavs: discard CAVS_LPRO and set CAVS_USE_LPRO_IN_WAITI by default

With dynamic clock switching applied, we don't need to configure
CAVS_LPRO explicitly, instead, we will use HPRO by default, and switch
to use LPRO in low power scenarios (e.g. WFI, D0I3 with KWD). Switching
to use LPRO in waiti or not is configured by CAVS_USE_LPRO_IN_WAITI
item, and use LPRO in waiti by default for power saving.

The commit also refine the clock switching logic as below:
1. use the active_freq_idx to save the active (non-WFI) clock index.
2. refine helper platform_set_active_clock() to switch the active clock
and update the active_freq_idx together.
3. for WFI related clock switching, don't update the active_freq_idx,
but just do the real switching and switching back.
4. implement platform_clock_waiti_entry() and
platform_clock_waiti_exit() for the clock switching, and update the
active clock before each WFI entry.

This is verified works on TGL platforms, we also need testing on JSL
with it.

Only set DMA L1 Exit at post copy to reduce the power consumption, this will benefit the KWD scenario as doing the HDA host DMA L1 Exit even no real data is being copied will stop the system to enter s0ix low power stage. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Refine to use the consistent activation threshold for different formats, use a unified coefficient 0.05, and update the threshold at .params(). These refinement will fix the KWD trigger volume inconsistent for different sample format issue. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Current implementation doesn't fit processing restrictions, so we need to disable system agent in order to avoid FW hang. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

[WIP] It looks like under certain circumstances the number of clients of scheduler can go negative. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch fixes two SSP pipelines to be scheduled in timer domain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch removes the constrain to copy at most one period of data one each copy-period. This will help to compensate copy delays introduced by LPRO Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch fixes trace messages related to delays so they print correct number of ticks Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch disables LPS from chrome defconfig Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This commit changes SSP bclk rate from 9600000 to 12288000 in order to fix glitch issue. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

mmaka1 · 2020-07-08T11:20:40Z

-#define CAVS_DEFAULT_RO		SHIM_CLKCTL_RHROSCC
-#define CAVS_DEFAULT_RO_FOR_MEM	SHIM_CLKCTL_OCS_HP_RING
-#endif
+#define CAVS_ROS	(SHIM_CLKCTL_RHROSCC | SHIM_CLKCTL_RLROSCC)


Why is CONFIG_CAVS_LPRO support removed here?
You could request LPRO in #if and request both in #else to get the same result with CAVS_LPRO removed from the TGL config and still support LPRO-only configuration.

I can't imagine the usage of the scale down scenario, to avoid confusion, I just removed it.

@keyonjie it looks like we really only have 2 options right? #if CONFIG_CAVS_LPRO is enabled, we do not enable HPRO but LPRO is enabled whether this config is selected or not. So, we should keep the #if CONFIG_CAVS_LPRO support here

mmaka1 · 2020-07-08T11:23:16Z

-					     * RING Oscillator Clk for memory
-					     */
+		   CAVS_ROS | /* Request configured RING Osc */
+		   CAVS_ROS_FOR_MEM | /* Select configured


Could you explain why single RO selection is changed to selecting both ROs for memory?

It was only a temp to combine the configuration of the DEFAULT RO for both CAVS_LPRO selected and deselected.

mmaka1 · 2020-07-08T11:25:30Z

Why are CNL and JSL changed on TGL specific branch? Such changes should be handled by separate commits.

I thought even it is TGL specific branch, we should at least make CNL/JSL passed building, no?

mmaka1 · 2020-07-08T11:41:07Z

The description is no longer valid and should be changed.
But are not we able to support down scaled LPRO-only configuration? Could you explain why it is completely removed?

@keyonjie I'll second @mmaka1 that (at least for a while) the ability to select LPRO-only should remain as a debug option. Default configuration shall be dynamic clock switching. I'm OK with dynamic clock switching as the only option on https://github.com/thesofproject/sof/tree/tgl-007-drop-stable and a default one but I'd expect that up-streamed code to master will preserve ability to select LPRO-only support.

OK, let me preserve it and rename it to CAVS_LPRO_ONLY.

@lbetlej @mmaka1 the refining is here: #3205

mmaka1 · 2020-07-08T11:50:06Z

Pls explain why is this piece of code duplicated from platform_clock_on_wakeup() (which is called on waiti exit) and put inside the idle task?

I didn't know how the platform_clock_on_wakeup() is used, so I didn't want to touch or change it. But yes, if they(both entry() and exit()) can be put inside the waiti, it will be better.

mmaka1 · 2020-07-08T11:56:58Z

Note that the old function is being fixed by #3113 on master branch. Is there any rationale behind changing the name while this new piece of deferred clock setup is added?

I didn't notice what #3113 is changing, the reason I am changing the naming is just to make the _xx_entry() and _xx_exit() symmetric and are invoked in the platform_wait_for_interrupt() together.

slawblauciak · 2020-07-08T12:42:32Z

Shouldn't you check for !pm_runtime_is_active here? If pm_runtime_is_active returns true and active clock is already set to HPRO, I believe we'd land in that else if block and put the DSP back in LPRO wrongly

keyonjie · 2020-07-08T13:09:34Z

On 7/8/20 8:42 PM, Sławomir Błauciak wrote: @slawblauciak commented on this pull request. > { - int freq_idx = get_cpu_current_freq_idx(); + int freq_idx; + int target_idx; + + /* update the active clock according to the pm runtime state */ + if (pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID) && + *cache_to_uncache(&active_freq_idx) != CPU_HPRO_FREQ_IDX) + platform_set_active_clock(CPU_HPRO_FREQ_IDX); + else if (*cache_to_uncache(&active_freq_idx) != CPU_LPRO_FREQ_IDX) Shouldn't you check for !pm_runtime_is_active here? If pm_runtime_is_active returns true and active clock is already set to HPRO, I believe we'd land in that else if block and put the DSP back in LPRO wrongly

Correct, thanks for spotting this, will update soon.

…

This patch will introduce additional client override flag to pm_runtime_get Extra flag is not necessary but if used it will prevent from powering down a core without the same flag This will save a lot of time if quick changes are needed eg. in dynamic hpro-lpro switching which on TGL will also require to power up core 3 Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

This patch will align with HW recommended flow while clock switching at platform init Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

This change utilizes the PM gate requests to switch between low power and high performance ring oscillator clocks as needed. This allows for the DSP to properly enter D0ix modes, even if normally operated using the high performance clock. Signed-off-by: Slawomir Blauciak <slawomir.blauciak@linux.intel.com>

mmaka1 · 2020-07-08T13:39:29Z

 		pm_runtime_enable(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID);

+#if !CONFIG_CAVS_LPRO
+	if (pm_runtime_is_active(PM_RUNTIME_DSP, PLATFORM_MASTER_CORE_ID))


If there is a risk of having unbalanced pm_gate IPC requests, pm_runtime_disable/enable calls should be protected with client bitmap, not a simple ref-counter (see how the put/get on the DSP resource is protected).

Set HPRO as default clock, so firmware can dynamically switch between LPRO and HPRO, instead of having it constantly set to LPRO. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

We need to enable both LP and HP oscillators at platform init, and switch them dynamically. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

With dynamic clock switching applied, we don't need to configure CAVS_LPRO explicitly, instead, we will use HPRO by default, and switch to use LPRO in low power scenarios (e.g. WFI, D0I3 with KWD). Switching to use LPRO in waiti or not is configured by CAVS_USE_LPRO_IN_WAITI item, and use LPRO in waiti by default for power saving. The commit also refine the clock switching logic as below: 1. use the active_freq_idx to save the active (non-WFI) clock index. 2. refine helper platform_set_active_clock() to switch the active clock and update the active_freq_idx together. 3. for WFI related clock switching, don't update the active_freq_idx, but just do the real switching and switching back. 4. implement platform_clock_waiti_entry() and platform_clock_waiti_exit() for the clock switching, and update the active clock before each WFI entry. This is verified works on TGL platforms, we also need testing on JSL with it. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie

Thanks for review, @mmaka1 some really good points. As I replied inline, this may be not the final commits which should go into the master branch (at least it can be squashed with @slawblauciak 's dynamic switching implementation). I just debugged and found can fix the dynamic clock switching issue, and no plan to refine and submit them to the master, please feel free to refine or squash them before submitting them to the master.

keyonjie · 2020-07-08T13:37:04Z

I thought even it is TGL specific branch, we should at least make CNL/JSL passed building, no?

keyonjie · 2020-07-08T13:46:33Z

-#define CAVS_DEFAULT_RO		SHIM_CLKCTL_RHROSCC
-#define CAVS_DEFAULT_RO_FOR_MEM	SHIM_CLKCTL_OCS_HP_RING
-#endif
+#define CAVS_ROS	(SHIM_CLKCTL_RHROSCC | SHIM_CLKCTL_RLROSCC)


I can't imagine the usage of the scale down scenario, to avoid confusion, I just removed it.

keyonjie · 2020-07-08T13:48:12Z

-					     * RING Oscillator Clk for memory
-					     */
+		   CAVS_ROS | /* Request configured RING Osc */
+		   CAVS_ROS_FOR_MEM | /* Select configured


It was only a temp to combine the configuration of the DEFAULT RO for both CAVS_LPRO selected and deselected.

keyonjie · 2020-07-08T13:51:07Z

I didn't notice what #3113 is changing, the reason I am changing the naming is just to make the _xx_entry() and _xx_exit() symmetric and are invoked in the platform_wait_for_interrupt() together.

keyonjie · 2020-07-08T13:53:31Z

I didn't know how the platform_clock_on_wakeup() is used, so I didn't want to touch or change it. But yes, if they(both entry() and exit()) can be put inside the waiti, it will be better.

ranj063 · 2020-07-08T16:22:01Z

+		platform_set_active_clock(CPU_HPRO_FREQ_IDX);
+	else
+		platform_set_active_clock(CPU_LPRO_FREQ_IDX);
+#endif


@mmaka1 @slawblauciak we also send the PM_GATE IPC when the DSP enters D0i3 in S0. The only way for the FW to know if it is sent during S0 or S0ix is by checking whether the DMA_TRACE is requested to be turned off or not. Do you think it makes a difference whether the switching to LPRO is happening during S0 as well?

keyonjie · 2020-07-24T06:55:52Z

tgl-007 release was done, so close this.

keyonjie and others added 8 commits July 7, 2020 17:28

chrome: tgl: disable system agent

a13c8d0

Current implementation doesn't fit processing restrictions, so we need to disable system agent in order to avoid FW hang. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

scheduler: make sure we don't get negative number of clients.

7dffd9f

[WIP] It looks like under certain circumstances the number of clients of scheduler can go negative. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

topology: fix two playback pipelines to be timer driven

8b98add

This patch fixes two SSP pipelines to be scheduled in timer domain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

dai: do not limit the amount of copy_bytes

321c789

This patch removes the constrain to copy at most one period of data one each copy-period. This will help to compensate copy delays introduced by LPRO Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

traces: fix trace messages related to delay reports

e92e9bc

This patch fixes trace messages related to delays so they print correct number of ticks Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

config: disable LPS from TGL chrome defconfig

33cfa90

This patch disables LPS from chrome defconfig Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

keyonjie requested review from bardliao, dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart, tlauda and xiulipan as code owners July 8, 2020 09:20

keyonjie requested review from mrajwa and slawblauciak July 8, 2020 09:20

slawblauciak approved these changes Jul 8, 2020

View reviewed changes

jajanusz force-pushed the tgl-007-drop-stable branch from 3d5df6e to 33cfa90 Compare July 8, 2020 10:33

smart_amp: fix smart amplifier tplg

5793b17

This commit changes SSP bclk rate from 9600000 to 12288000 in order to fix glitch issue. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

mmaka1 reviewed Jul 8, 2020

View reviewed changes

slawblauciak reviewed Jul 8, 2020

View reviewed changes

keyonjie force-pushed the dynamic_clk_switch branch from 2afb9af to e93ee5e Compare July 8, 2020 13:16

abonislawski and others added 3 commits July 8, 2020 21:38

tgl: align with HW recommended flow at platform init

f0b5b45

This patch will align with HW recommended flow while clock switching at platform init Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

mmaka1 reviewed Jul 8, 2020

View reviewed changes

jajanusz and others added 3 commits July 8, 2020 21:39

chrome: change default clock to HPRO

250ce25

Set HPRO as default clock, so firmware can dynamically switch between LPRO and HPRO, instead of having it constantly set to LPRO. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

cavs: platform: enable both LP and HP Oscillators at init

677938a

We need to enable both LP and HP oscillators at platform init, and switch them dynamically. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie commented Jul 8, 2020

View reviewed changes

keyonjie force-pushed the dynamic_clk_switch branch from e93ee5e to c847315 Compare July 8, 2020 13:59

ranj063 reviewed Jul 8, 2020

View reviewed changes

jajanusz force-pushed the tgl-007-drop-stable branch 2 times, most recently from be6c347 to fb801c8 Compare July 14, 2020 20:06

keyonjie requested a review from libinyang as a code owner July 14, 2020 20:06

keyonjie closed this Jul 24, 2020

Conversation

keyonjie commented Jul 8, 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!

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!

keyonjie commented Jul 8, 2020 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie 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!

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 24, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants