Repository navigation
Fix clock change in waiti for multicore access - #3113
Merged
lgirdwood merged 6 commits intoJul 9, 2020
Merged
Conversation
ktrzcinx
requested review from
dbaluta,
jajanusz,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
tlauda
as code owners
June 30, 2020 06:16
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
4 times, most recently
from
June 30, 2020 08:13
ea2f1fc to
8f93ad4
Compare
lgirdwood
requested changes
Jun 30, 2020
mmaka1
reviewed
Jun 30, 2020
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
2 times, most recently
from
June 30, 2020 13:53
1e15c0b to
0f792e2
Compare
mmaka1
requested changes
Jun 30, 2020
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
2 times, most recently
from
July 1, 2020 09:54
b7e55b3 to
3af0bd4
Compare
|
@ktrzcinx Some checkpatch issues regarding typos and comment misalignment are valid. |
mmaka1
changed the title
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
from
July 1, 2020 11:26
3af0bd4 to
9767f55
Compare
mmaka1
approved these changes
Jul 3, 2020
lgirdwood
requested changes
Jul 6, 2020
Member
There was a problem hiding this comment.
We should return the old value not the new value. See https://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Atomic-Builtins.html. This also needs fixed in atomic_add and atomic_dec.
Member
There was a problem hiding this comment.
This is really runtime PM.
pm_runtime_get(PM_HP_CLOCK, cpu_get_id());Which nicely abstracts the clocking PM. Likewise
pm_runtime_put(PM_HP_CLOCK, cpu_get_id());will cjheck if mask is 0 and switch off HP clock.
Member
Author
There was a problem hiding this comment.
But to maintain this from pm_runtime, I will need to change clock API (set_freq() will need new parameter defining if clock change is temporary or persistentclock_low_power_mode())
Closed
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
from
July 8, 2020 09:23
9767f55 to
db2a7a9
Compare
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
from
July 8, 2020 10:10
db2a7a9 to
8aae9f5
Compare
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
3 times, most recently
from
July 8, 2020 10:38
b1edc00 to
d568160
Compare
In situation with two or more active cores, saving active frequency index in platform_clock_on_waiti suffer from race condition. Changing state in *set_cpu_freq() method is much more safe. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Closed
Member
|
Blocking on #3151. |
Member
Author
lgirdwood
requested changes
Jul 8, 2020
lgirdwood
left a comment
lgirdwood
left a comment
Member
There was a problem hiding this comment.
Are we missing the atomic OR / AND ? or are these no longer needed ?
Member
Author
OR / AND operations are already inside spin_lock / unlock block, so I guess that there atomic operations are not needed (but it wouldn't be also not an error to use it) |
It was needed to call two functions and update one global variable to change clock frequency, what is quite error prone solution. After merging it to one function, it is easier to keep clock description synchronized. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Set bit for PLATFORM_MASTER_CORE_ID and remove exception handlind from arch_cpu_is_core_enabled. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Core enable/disable is related with power management and maintenance of clock trees, so such an mask should be possible to read. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
This function will be called from power manager to change current clock value for sleep time. Detailed implemnentation may be clock instance dependednt. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Clock manegement is important part for power consumption optimization, what is a task of this module. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
ktrzcinx
force-pushed
the
fix-clock-change-in-waiti
branch
from
July 8, 2020 14:13
d568160 to
50b9cc4
Compare
lgirdwood
approved these changes
Jul 9, 2020
Member
|
CI jenkins known issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was race condition in situation with multicore environment:
current_freq_idxtoactive_freq_idx(HPRO) and then changecurrent_freq_idxto LPROcurrent_freq_idxtoactive_freq_idx(LPRO) and then changecurrent_freq_idxto LPROactive_freq_idx- LPRO instead HPROMoreover
sleep_core_maskmust be introduced because in a case with multiple cores connected to single clock source, then clock rate may be reduced only when each of them is in wait state.