tgl: enable WOVCRO clock #3418

abonislawski · 2020-09-15T15:10:40Z

First patch will introduce CPU_WAITI_FREQ_IDX so a specific clock for WAITI can be configured per platform.
Second patch added releasing clock for waiti switches, it will fix #3448
Last patch will enable WOVCRO for TGL and allow to save significant amount of SOC power in low power S0ix WoV.

Patch is backward-compatible, if WOVCRO is not available (some old TGL steppings) then LPRO will be used for WAITI

keyonjie · 2020-09-18T10:07:27Z

@abonislawski Thanks for enabling WOVCRO.

My concern is that switching to WOVCRO everywhere we intent to switch to LPRO looks too aggressive to me, can we force this for WoV on S0Ix only, and make the WFI context low power clock configurable? e.g. we already have USE_LPRO_IN_WAITI, maybe we can add a new item (e.g. USE_WOVCRO_IN_WAITI) under it and unselect it by default?

lgirdwood · 2020-09-18T11:03:44Z

SOFCI TEST

abonislawski · 2020-09-21T06:14:32Z

@keyonjie its not everywhere, its in WAITI only.
low_power_mode is still switching to LPRO

but we can limit it to S0ix only if needed, any pros/cons?

Anyway WOVCRO needs to be enabled by default for our releases

src/platform/intel/cavs/platform.c

keyonjie · 2020-09-21T08:35:51Z

@keyonjie its not everywhere, its in WAITI only.
low_power_mode is still switching to LPRO

Oh, then it's bad, how can we switch to WOVCRO then? The "pm_runtime_put(CORE_HP_CLK, cpu_get_id())" is invoked after the HPRO->WOVCRO switching? won't it switch to LPRO for WoV on S0Ix?

	if (pm_is_active) {
		/* set HPRO clock if not already enabled */
		if (freq_idx != CPU_HPRO_FREQ_IDX)
			set_cpu_current_freq_idx(CPU_HPRO_FREQ_IDX, true);
	} else {
		/* set lowest waiti clock if not already enabled */
		if (freq_idx != CPU_WAITI_FREQ_IDX)
			set_cpu_current_freq_idx(CPU_WAITI_FREQ_IDX, true);
	}

	spin_unlock_irq(&prd->lock, flags);

	/* check if waiti clock switching is needed */
	pm_runtime_put(CORE_HP_CLK, cpu_get_id());

but we can limit it to S0ix only if needed, any pros/cons?

I am suggesting adding config items, to make it configurable. Please be noticed that the CONFIG_CAVS_USE_LPRO_IN_WAITI can be unslected so you might need to change the platform_clock_on_waiti() of the "#else" version also.

Anyway WOVCRO needs to be enabled by default for our releases

abonislawski · 2020-09-21T09:28:09Z

Oh, then it's bad, how can we switch to WOVCRO then? The "pm_runtime_put(CORE_HP_CLK, cpu_get_id())" is invoked after the HPRO->WOVCRO switching? won't it switch to LPRO for WoV on S0Ix?

I need to investigate it because it looks like low_power_mode is a good place for WOVCRO switching. Power measurements gave great results so WOVCRO was working, need to check if this low_power_mode is working correctly then.

I am suggesting adding config items, to make it configurable. Please be noticed that the CONFIG_CAVS_USE_LPRO_IN_WAITI can be unslected so you might need to change the platform_clock_on_waiti() of the "#else" version also.

The CAVS approach is to use WOVCRO as lowest clock in WAITI stage because of power savings, so I think we should rename CONFIG_CAVS_USE_LPRO_IN_WAITI to something more universal for LPRO/WOVCRO and another layer for LPRO-or-WOVCRO switching is not necessary.

zrombel · 2020-09-21T11:04:17Z

This PR force us to upgrade our CI TGL RVP platforms to B0 stepping. At this point we are using A2 stepping platforms thus TGL smoke tests will FAIL. We plan to finish platform upgrade at the end of WW40.

abonislawski · 2020-09-23T08:15:29Z

Updated PR with clock switching moved to "low_power_mode".
The names are very confusing because the true clock switching for CONFIG_CAVS_USE_LPRO_IN_WAITI takes place in platform_clock_low_power_mode() instead of just platform_clock_on_waiti() as the name suggests.

Also added releasing clock for waiti switches, it will fix #3448
This issue is not introduced with WOVCRO but can be seen only with WOVCRO because of logic in platform_clock_on_waiti()

abonislawski · 2020-09-25T08:37:04Z

@keyonjie good news for old steppings, added backward-compatible version (LPRO automatically selected) because it was reported to us that so many boards are still Ax :(

lgirdwood · 2020-09-25T10:51:12Z

@abonislawski can you check CI could be a server issue ?

abonislawski · 2020-09-25T11:49:57Z

@abonislawski can you check CI could be a server issue ?

I think CI issue

keyonjie · 2020-09-28T06:48:43Z

@abonislawski sorry, with this new WoVCRO clock introduced, the clock switching logic is becoming more complex and error-prone, we should be very careful when implement this, several items are not clear to me:

the clock switching logic, e.g. we have 3 clocks (clock 0/1/2) in total, and all clock switching are performed at _waiti()/_wakeup(), basically:

_waiti(): possible High -> Low switching, scenarios including:

a. no switching needed. e.g. only 1 clock is supported/configured for the specific platform, or no any low power mode(e.g. LP for WAITI, or LP for D0I3) is configured. Another scenario like LPRO -> LPRO so no switching needed.
b. clock 2 (HPRO) -> clock 1 (LPRO). e.g. LP for waiti or LP for D0I3(but no clock 0 /WOVCRO existed) low power mode is selected.
c. clock 2 -> clock 0 (WoVCRO). LP for D0I3 low power mode is selected and WoVCRO existed. Need to make sure we only do a single switching, neither 'clock 2 -> clock 1 -> clock 0' nor 'clock 2 -> clock 0 -> clock 1' switching should happen.

_wakeup(): possible Low -> High switching, scenarios including:

a. clock 0 -> clock 1. xxx
b. clock 0 -> clock 2. yyy, neither 'clock 0 -> clock 1 -> clock 2' nor 'clock 0 -> clock 2 -> clock 1' switching should happen.
c. clock 1 -> clock 2. zzz

make it clear about what is configurable and what isn't. e.g. LP low power for waiti (CONFIG_CAVS_USE_LPRO_IN_WAITI at the moment), LP low power mode for D0Ix (extra configure item needed? suppose we will always be required to switch to low power mode for D0Ix entry if possible, maybe no needed here), WOVCRO on D0Ix (a new configure item needed, at least for debugging purpose, it can be default 'Y', bug can be selected as 'N' to try only LPRO for D0Ix WoV).

So in short let's make this more clear in one shot, to avoid possible mysterious errors happen in the future.

lgirdwood · 2020-09-28T09:47:10Z

src/include/sof/lib/clk.h

Ok, I'm reading the comments about things becoming more complex so I think we can simplify by adding an inline comment to each "idx" and "notification" member in this struct. This should start to make things simpler for all reviewers to follow.

Should this index be a common parameter? Seems to be specific for cAVS platforms and left uninitialized/unused for others.

lgirdwood · 2020-09-28T09:48:18Z

src/platform/intel/cavs/lib/clk.c

Can we add another comment here explaining the new logic.

This comment was only about delay which isnt important for us, the remaining logic is pretty simple just as 'if' in code, if clock > LPRO then change to LPRO

src/platform/intel/cavs/lib/clk.c

keyonjie · 2020-11-09T08:30:47Z

@abonislawski ping for update.

abonislawski · 2020-11-13T12:24:47Z

PR updated and rebased.
@keyonjie no logic changes for clock switching, in short: LPRO replaced by WOVCRO (if WOVCRO available)

lgirdwood

We've lost some context by removing a comment, we should really change the comment to match the new logic.

lgirdwood · 2020-11-16T11:32:38Z

src/platform/intel/cavs/lib/clk.c

src/platform/tigerlake/lib/clk.c

lgirdwood · 2020-11-23T10:20:19Z

Travis looks like a CI timeout unrelated to this PR.

keyonjie · 2020-11-24T02:19:54Z

@abonislawski we should add a kconfig item to enable using this WOVCRO (e.g. disable it by default), then on most platforms where no WoV feature is needed, we use LPRO.

abonislawski · 2020-11-25T13:49:24Z

Marked as WIP, will add and squash changes from tgl011 when done

keyonjie · 2020-12-03T02:49:19Z

@abonislawski time to proceed and get this merged to master now, feel free to squash changes I made on the 011 release branch.

keyonjie · 2020-12-21T09:43:52Z

@abonislawski ping.

lgirdwood · 2021-01-04T16:25:46Z

@abonislawski @keyonjie any updates - I assume the CI is OK ?

abonislawski · 2021-01-05T16:55:01Z

@lgirdwood I will update this as soon as tgl-011 issues are fixed

lgirdwood · 2021-01-08T11:09:55Z

@abonislawski thanks - this is v1.7 blocker now. I will rerun CI to make sure everything looks good with recent updates.

lgirdwood · 2021-01-08T11:10:03Z

SOFCI TEST

abonislawski · 2021-01-11T14:14:28Z

@lgirdwood PR updated

lgirdwood

LGTM, but I'm not an expert on the WOVCRO. @keyonjie @mwasko @mmaka1 any comments ?
@abonislawski I assume this is same version used on production ?

abonislawski · 2021-01-12T08:13:47Z

@lgirdwood yes, wovcro is already on tgl-010 and 011 releases, in 011 it was limited to s0ix only and this PR is updated with this change

keyonjie · 2021-01-12T09:13:14Z

@abonislawski can you help to squash and add my commit dfdfa6b?
Better to make sure it is exactly same with tgl-011-drop-stable for this part, which has been verified work on TGL chrome.

abonislawski · 2021-01-12T09:54:06Z

@keyonjie it is already done with limiting wovcro to s0ix, this is only a few lines of code so you can easily compare that it will work exactly the same.
Cannot use your commit without any changes because it is ignoring compatibility check in platform_clock_init().

lgirdwood · 2021-01-12T20:52:55Z

@abonislawski @keyonjie let me know when good to merge.

keyonjie

@abonislawski yes, functional exactly same with 011 base, can you change the naming 'waiti_freq_idx' to avoid confusion, others looks good to me.

keyonjie · 2021-01-15T03:15:20Z

src/include/sof/lib/clk.h

@abonislawski better to change the naming to e.g. lowest_freq_idx to avoid confusion, as we actually use LPRO for the waiti context.

@keyonjie updated with lowest_freq_idx

It will allow to define cpu lowest clock per platform instead of fixed one (LPRO) Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

This patch will release previous clock in low power mode because there are some rare problems with not releasing it Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

This will allow to save significant amount of SOC power in low power S0ix WoV Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

keyonjie

Looks good now, thanks.

abonislawski requested review from jajanusz and mmaka1 September 15, 2020 15:10

abonislawski requested review from dbaluta, lbetlej, lgirdwood and plbossart as code owners September 15, 2020 15:10

abonislawski force-pushed the tgl_wovcro branch from 2ce0ae9 to f6ffaaf Compare September 15, 2020 15:21

lgirdwood added this to the v1.7 milestone Sep 18, 2020

keyonjie reviewed Sep 21, 2020

View reviewed changes

src/platform/intel/cavs/platform.c Outdated Show resolved Hide resolved

abonislawski force-pushed the tgl_wovcro branch from f6ffaaf to de24eca Compare September 23, 2020 08:06

abonislawski force-pushed the tgl_wovcro branch from de24eca to 168cb74 Compare September 25, 2020 08:15

lgirdwood requested changes Sep 28, 2020

View reviewed changes

abonislawski force-pushed the tgl_wovcro branch from 168cb74 to 7b6fb2b Compare November 13, 2020 12:09

lgirdwood requested changes Nov 16, 2020

View reviewed changes

src/platform/intel/cavs/lib/clk.c Outdated

Copy link

Member

lgirdwood Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

keyonjie reviewed Nov 23, 2020

View reviewed changes

src/platform/tigerlake/lib/clk.c Outdated Show resolved Hide resolved

mengdonglin mentioned this pull request Nov 24, 2020

[BUG] [TGL] HDA controller reset fails in reboot tests #3633

Closed

keqiaozhang mentioned this pull request Nov 24, 2020

[BUG][TGL] SSP regression on TGL platform w/ Maxim98373+RT5682 codec #3640

Closed

abonislawski changed the title ~~tgl: enable WOVCRO clock~~ [WIP] tgl: enable WOVCRO clock Nov 25, 2020

abonislawski force-pushed the tgl_wovcro branch 2 times, most recently from e2752f8 to e6e13ad Compare January 11, 2021 14:11

abonislawski changed the title ~~[WIP] tgl: enable WOVCRO clock~~ tgl: enable WOVCRO clock Jan 11, 2021

lgirdwood approved these changes Jan 11, 2021

View reviewed changes

keyonjie reviewed Jan 15, 2021

View reviewed changes

abonislawski added 3 commits January 15, 2021 12:05

clk: add LOWEST_FREQ_IDX to make clock switching more flexible

3cdf5bb

It will allow to define cpu lowest clock per platform instead of fixed one (LPRO) Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

clk: release clock in low power mode

5d37b9d

This patch will release previous clock in low power mode because there are some rare problems with not releasing it Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

tgl: enable WOVCRO clock

56ad96d

This will allow to save significant amount of SOC power in low power S0ix WoV Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

abonislawski force-pushed the tgl_wovcro branch from e6e13ad to 56ad96d Compare January 15, 2021 11:17

keyonjie approved these changes Jan 15, 2021

View reviewed changes

lgirdwood merged commit fadd8e7 into thesofproject:master Jan 15, 2021

tgl: enable WOVCRO clock #3418

tgl: enable WOVCRO clock #3418

Uh oh!

Conversation

abonislawski commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Sep 18, 2020

Uh oh!

lgirdwood commented Sep 18, 2020

Uh oh!

abonislawski commented Sep 21, 2020

Uh oh!

Uh oh!

keyonjie commented Sep 21, 2020

Uh oh!

abonislawski commented Sep 21, 2020

Uh oh!

zrombel commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abonislawski commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abonislawski commented Sep 25, 2020

Uh oh!

lgirdwood commented Sep 25, 2020

Uh oh!

abonislawski commented Sep 25, 2020

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keyonjie commented Nov 9, 2020

Uh oh!

abonislawski commented Nov 13, 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!

Uh oh!

lgirdwood commented Nov 23, 2020

Uh oh!

keyonjie commented Nov 24, 2020

Uh oh!

abonislawski commented Nov 25, 2020

Uh oh!

keyonjie commented Dec 3, 2020

Uh oh!

keyonjie commented Dec 21, 2020

Uh oh!

lgirdwood commented Jan 4, 2021

Uh oh!

abonislawski commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jan 8, 2021

Uh oh!

lgirdwood commented Jan 8, 2021

Uh oh!

abonislawski commented Jan 11, 2021

Uh oh!

lgirdwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

abonislawski commented Sep 15, 2020 •

edited

Loading

zrombel commented Sep 21, 2020 •

edited

Loading

abonislawski commented Sep 23, 2020 •

edited

Loading

keyonjie commented Sep 28, 2020 •

edited

Loading

abonislawski commented Jan 5, 2021 •

edited

Loading

lgirdwood left a comment •

edited

Loading