From 5fa80692740f016605f49f225e5b25f85fd3a5f1 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Mon, 29 Jun 2020 15:58:14 +0200 Subject: [PATCH 1/6] platform: intel: clock: Explicit change active frequency index 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 --- src/platform/intel/cavs/lib/clk.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/platform/intel/cavs/lib/clk.c b/src/platform/intel/cavs/lib/clk.c index f2e4433b0348..2b4271cd503e 100644 --- a/src/platform/intel/cavs/lib/clk.c +++ b/src/platform/intel/cavs/lib/clk.c @@ -61,12 +61,6 @@ static inline void select_cpu_clock(int freq_idx, bool release_unused) } #endif -static int clock_platform_set_cpu_freq(int clock, int freq_idx) -{ - select_cpu_clock(freq_idx, true); - return 0; -} - #if CONFIG_CAVS_USE_LPRO_IN_WAITI /* Store clock source that was active before going to waiti, * so it can be restored on wake up. @@ -106,8 +100,6 @@ void platform_clock_on_waiti(void) { int freq_idx = get_cpu_current_freq_idx(); - *cache_to_uncache(&active_freq_idx) = freq_idx; - if (freq_idx != CPU_LPRO_FREQ_IDX) { /* LPRO requests are fast, but requests for other ROs * can take a lot of time. That's why it's better to @@ -118,8 +110,26 @@ void platform_clock_on_waiti(void) set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX); } } + +static inline void clock_platform_update_active_freq_idx(int freq_idx) +{ + int *uncached_freq_idx = cache_to_uncache(&active_freq_idx); + + *uncached_freq_idx = freq_idx; +} +#else +static inline void clock_platform_update_active_freq_idx(int freq_idx) +{ +} #endif +static int clock_platform_set_cpu_freq(int clock, int freq_idx) +{ + select_cpu_clock(freq_idx, true); + clock_platform_update_active_freq_idx(freq_idx); + return 0; +} + void platform_clock_init(struct sof *sof) { int i; From ba178e2641a19466ea29309d6f4a5dd51eb8d2bd Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Wed, 1 Jul 2020 13:24:03 +0200 Subject: [PATCH 2/6] platform: intel: clock: Merge clock change to single function call 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 --- src/platform/intel/cavs/lib/clk.c | 62 ++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/src/platform/intel/cavs/lib/clk.c b/src/platform/intel/cavs/lib/clk.c index 2b4271cd503e..f4e7d73cfbeb 100644 --- a/src/platform/intel/cavs/lib/clk.c +++ b/src/platform/intel/cavs/lib/clk.c @@ -15,7 +15,7 @@ static SHARED_DATA struct clock_info platform_clocks_info[NUM_CLOCKS]; #if CAVS_VERSION == CAVS_VERSION_1_5 -static inline void select_cpu_clock(int freq_idx, bool release_unused) +static inline void select_cpu_clock_hw(int freq_idx, bool release_unused) { uint32_t enc = cpu_freq_enc[freq_idx]; @@ -25,7 +25,7 @@ static inline void select_cpu_clock(int freq_idx, bool release_unused) enc); } #else -static inline void select_cpu_clock(int freq_idx, bool release_unused) +static inline void select_cpu_clock_hw(int freq_idx, bool release_unused) { uint32_t enc = cpu_freq_enc[freq_idx]; uint32_t status_mask = cpu_freq_status_mask[freq_idx]; @@ -61,6 +61,29 @@ static inline void select_cpu_clock(int freq_idx, bool release_unused) } #endif +static inline void select_cpu_clock(int freq_idx, bool release_unused) +{ + struct clock_info *clk_info = clocks_get(); + int flags[PLATFORM_CORE_COUNT]; + int i; + + /* lock clock for all cores */ + for (i = 0; i < PLATFORM_CORE_COUNT; i++) + spin_lock_irq(&clk_info[CLK_CPU(i)].lock, flags[i]); + + /* change clock */ + select_cpu_clock_hw(freq_idx, release_unused); + for (i = 0; i < PLATFORM_CORE_COUNT; i++) + clk_info[CLK_CPU(i)].current_freq_idx = freq_idx; + + /* unlock clock for all cores */ + for (i = PLATFORM_CORE_COUNT - 1; i >= 0; i--) + spin_unlock_irq(&clk_info[CLK_CPU(i)].lock, flags[i]); + + platform_shared_commit(clk_info, + sizeof(*clk_info) * PLATFORM_CORE_COUNT); +} + #if CONFIG_CAVS_USE_LPRO_IN_WAITI /* Store clock source that was active before going to waiti, * so it can be restored on wake up. @@ -74,16 +97,16 @@ static inline int get_cpu_current_freq_idx(void) return clk_info->current_freq_idx; } -static inline void set_cpu_current_freq_idx(int freq_idx) +/* + * This is call from public API, so overwrite currently used frequency index + * in 'active' state with new value. This value will be used in each wakeup. + */ +static inline void set_cpu_current_freq_idx(int freq_idx, bool release_unused) { - int i; - struct clock_info *clk_info = clocks_get(); - - for (i = 0; i < PLATFORM_CORE_COUNT; i++) - clk_info[i].current_freq_idx = freq_idx; + int *uncached_freq_idx = cache_to_uncache(&active_freq_idx); - platform_shared_commit(clk_info, - sizeof(*clk_info) * PLATFORM_CORE_COUNT); + select_cpu_clock(freq_idx, release_unused); + *uncached_freq_idx = freq_idx; } void platform_clock_on_wakeup(void) @@ -92,7 +115,6 @@ void platform_clock_on_wakeup(void) if (freq_idx != get_cpu_current_freq_idx()) { select_cpu_clock(freq_idx, true); - set_cpu_current_freq_idx(freq_idx); } } @@ -107,26 +129,24 @@ void platform_clock_on_waiti(void) * so they can be switched without delay on wake up. */ select_cpu_clock(CPU_LPRO_FREQ_IDX, false); - set_cpu_current_freq_idx(CPU_LPRO_FREQ_IDX); } } -static inline void clock_platform_update_active_freq_idx(int freq_idx) -{ - int *uncached_freq_idx = cache_to_uncache(&active_freq_idx); - - *uncached_freq_idx = freq_idx; -} #else -static inline void clock_platform_update_active_freq_idx(int freq_idx) +/* + * This is call from public API, there is no clock switch for core waiti, so + * value of value frequency index in 'active' state doesn't need to be saved in + * any additional variable and restored in future. + */ +static inline void set_cpu_current_freq_idx(int freq_idx, bool release_unused) { + select_cpu_clock(freq_idx, true); } #endif static int clock_platform_set_cpu_freq(int clock, int freq_idx) { - select_cpu_clock(freq_idx, true); - clock_platform_update_active_freq_idx(freq_idx); + set_cpu_current_freq_idx(freq_idx, true); return 0; } From cc6a1bcccf2b9294046451b8896fb76d14c44aaa Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Tue, 30 Jun 2020 15:23:57 +0200 Subject: [PATCH 3/6] platform: intel: cpu: Unify active_cores_mask Set bit for PLATFORM_MASTER_CORE_ID and remove exception handlind from arch_cpu_is_core_enabled. Signed-off-by: Karol Trzcinski --- src/arch/xtensa/lib/cpu.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/arch/xtensa/lib/cpu.c b/src/arch/xtensa/lib/cpu.c index d6e0cfc65782..e63250280c8d 100644 --- a/src/arch/xtensa/lib/cpu.c +++ b/src/arch/xtensa/lib/cpu.c @@ -31,7 +31,7 @@ extern struct core_context *core_ctx_ptr[PLATFORM_CORE_COUNT]; extern struct xtos_core_data *core_data_ptr[PLATFORM_CORE_COUNT]; -static uint32_t active_cores_mask; +static uint32_t active_cores_mask = BIT(PLATFORM_MASTER_CORE_ID); static int dsp_sref[PLATFORM_CORE_COUNT]; /**< simple ref counter */ #if CONFIG_NO_SLAVE_CORE_ROM @@ -131,8 +131,7 @@ void arch_cpu_disable_core(int id) int arch_cpu_is_core_enabled(int id) { - return id == PLATFORM_MASTER_CORE_ID || - active_cores_mask & (1 << id); + return active_cores_mask & BIT(id); } void cpu_alloc_core_context(int core) From fedec980b1d3566d56ef6a498eec6f8a3d855ff5 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Tue, 30 Jun 2020 13:44:04 +0200 Subject: [PATCH 4/6] cpu: Export mask with enabled cores 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 --- src/arch/host/include/arch/lib/cpu.h | 5 +++++ src/arch/xtensa/include/arch/lib/cpu.h | 4 ++++ src/arch/xtensa/lib/cpu.c | 5 +++++ src/include/sof/lib/cpu.h | 5 +++++ 4 files changed, 19 insertions(+) diff --git a/src/arch/host/include/arch/lib/cpu.h b/src/arch/host/include/arch/lib/cpu.h index 39d9485d4787..c6e9ee74693b 100644 --- a/src/arch/host/include/arch/lib/cpu.h +++ b/src/arch/host/include/arch/lib/cpu.h @@ -24,6 +24,11 @@ static inline int arch_cpu_is_core_enabled(int id) return 0; } +static inline int arch_cpu_enabled_cores(void) +{ + return 1; +} + static inline int arch_cpu_get_id(void) { return 0; diff --git a/src/arch/xtensa/include/arch/lib/cpu.h b/src/arch/xtensa/include/arch/lib/cpu.h index 9878f75c0f28..b72c697f3a57 100644 --- a/src/arch/xtensa/include/arch/lib/cpu.h +++ b/src/arch/xtensa/include/arch/lib/cpu.h @@ -25,6 +25,8 @@ void arch_cpu_disable_core(int id); int arch_cpu_is_core_enabled(int id); +int arch_cpu_enabled_cores(void); + #else static inline int arch_cpu_enable_core(int id) { return 0; } @@ -33,6 +35,8 @@ static inline void arch_cpu_disable_core(int id) { } static inline int arch_cpu_is_core_enabled(int id) { return 1; } +static inline int arch_cpu_enabled_cores(void) { return 1; } + #endif static inline int arch_cpu_get_id(void) diff --git a/src/arch/xtensa/lib/cpu.c b/src/arch/xtensa/lib/cpu.c index e63250280c8d..bdd869448996 100644 --- a/src/arch/xtensa/lib/cpu.c +++ b/src/arch/xtensa/lib/cpu.c @@ -134,6 +134,11 @@ int arch_cpu_is_core_enabled(int id) return active_cores_mask & BIT(id); } +int arch_cpu_enabled_cores(void) +{ + return active_cores_mask; +} + void cpu_alloc_core_context(int core) { struct core_context *core_ctx; diff --git a/src/include/sof/lib/cpu.h b/src/include/sof/lib/cpu.h index 217db04343b0..8b3d566dd9ce 100644 --- a/src/include/sof/lib/cpu.h +++ b/src/include/sof/lib/cpu.h @@ -55,6 +55,11 @@ static inline int cpu_is_core_enabled(int id) return arch_cpu_is_core_enabled(id); } +static inline int cpu_enabled_cores(void) +{ + return arch_cpu_enabled_cores(); +} + #endif #endif /* __SOF_LIB_CPU_H__ */ From 4aa34989e3be11fb0a50829487d45e307ea06ac3 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Tue, 7 Jul 2020 20:49:22 +0200 Subject: [PATCH 5/6] clock: Add function to set low power mode 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 --- src/include/sof/lib/clk.h | 8 +++++++ src/lib/clk.c | 8 +++++++ src/platform/intel/cavs/lib/clk.c | 36 +++++++++++++++++++------------ 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/include/sof/lib/clk.h b/src/include/sof/lib/clk.h index 0d80dcd78843..04febf6f2b04 100644 --- a/src/include/sof/lib/clk.h +++ b/src/include/sof/lib/clk.h @@ -12,6 +12,7 @@ #include #include #include +#include #include struct timer; @@ -40,13 +41,20 @@ struct clock_info { uint32_t notification_id; uint32_t notification_mask; spinlock_t lock; + + /* persistent change clock value in active state */ int (*set_freq)(int clock, int freq_idx); + + /* temporary change clock - don't modify default clock settings */ + void (*low_power_mode)(int clock, bool enable); }; uint32_t clock_get_freq(int clock); void clock_set_freq(int clock, uint32_t hz); +void clock_low_power_mode(int clock, bool enable); + uint64_t clock_ms_to_ticks(int clock, uint64_t ms); void platform_timer_set_delta(struct timer *timer, uint64_t ns); diff --git a/src/lib/clk.c b/src/lib/clk.c index 0f23e4e34b49..433502838a38 100644 --- a/src/lib/clk.c +++ b/src/lib/clk.c @@ -85,6 +85,14 @@ void clock_set_freq(int clock, uint32_t hz) spin_unlock_irq(&clk_info->lock, flags); } +void clock_low_power_mode(int clock, bool enable) +{ + struct clock_info *clk_info = clocks_get() + clock; + + if (clk_info->low_power_mode) + clk_info->low_power_mode(clock, enable); +} + uint64_t clock_ms_to_ticks(int clock, uint64_t ms) { struct clock_info *clk_info = clocks_get() + clock; diff --git a/src/platform/intel/cavs/lib/clk.c b/src/platform/intel/cavs/lib/clk.c index f4e7d73cfbeb..f7fec82698fa 100644 --- a/src/platform/intel/cavs/lib/clk.c +++ b/src/platform/intel/cavs/lib/clk.c @@ -90,9 +90,9 @@ static inline void select_cpu_clock(int freq_idx, bool release_unused) */ static SHARED_DATA int active_freq_idx = CPU_DEFAULT_IDX; -static inline int get_cpu_current_freq_idx(void) +static inline int get_current_freq_idx(int clock) { - struct clock_info *clk_info = clocks_get() + CLK_CPU(cpu_get_id()); + struct clock_info *clk_info = clocks_get() + clock; return clk_info->current_freq_idx; } @@ -109,27 +109,30 @@ static inline void set_cpu_current_freq_idx(int freq_idx, bool release_unused) *uncached_freq_idx = freq_idx; } -void platform_clock_on_wakeup(void) +static void platform_clock_low_power_mode(int clock, bool enable) { + int current_freq_idx = get_current_freq_idx(clock); int freq_idx = *cache_to_uncache(&active_freq_idx); - if (freq_idx != get_cpu_current_freq_idx()) { - select_cpu_clock(freq_idx, true); - } -} - -void platform_clock_on_waiti(void) -{ - int freq_idx = get_cpu_current_freq_idx(); - - if (freq_idx != CPU_LPRO_FREQ_IDX) { + if (enable && current_freq_idx != CPU_LPRO_FREQ_IDX) /* LPRO requests are fast, but requests for other ROs * can take a lot of time. That's why it's better to * not release active clock just for waiti, * so they can be switched without delay on wake up. */ select_cpu_clock(CPU_LPRO_FREQ_IDX, false); - } + else if (!enable && current_freq_idx != freq_idx) + select_cpu_clock(freq_idx, false); +} + +void platform_clock_on_wakeup(void) +{ + clock_low_power_mode(CLK_CPU(cpu_get_id()), false); +} + +void platform_clock_on_waiti(void) +{ + clock_low_power_mode(CLK_CPU(cpu_get_id()), true); } #else @@ -142,6 +145,10 @@ static inline void set_cpu_current_freq_idx(int freq_idx, bool release_unused) { select_cpu_clock(freq_idx, true); } + +static void platform_clock_low_power_mode(int clock, bool enable) +{ +} #endif static int clock_platform_set_cpu_freq(int clock, int freq_idx) @@ -166,6 +173,7 @@ void platform_clock_init(struct sof *sof) .notification_id = NOTIFIER_ID_CPU_FREQ, .notification_mask = NOTIFIER_TARGET_CORE_MASK(i), .set_freq = clock_platform_set_cpu_freq, + .low_power_mode = platform_clock_low_power_mode, }; spinlock_init(&sof->clocks[i].lock); From 50b9cc4307cad0b1214b6f864429fb9ee57452d5 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Tue, 7 Jul 2020 20:59:37 +0200 Subject: [PATCH 6/6] platform: pm: Add HP_CLK management to pm_runtime Clock manegement is important part for power consumption optimization, what is a task of this module. Signed-off-by: Karol Trzcinski --- src/include/sof/lib/pm_runtime.h | 1 + .../intel/cavs/include/cavs/lib/pm_runtime.h | 1 + src/platform/intel/cavs/lib/clk.c | 5 +- src/platform/intel/cavs/lib/pm_runtime.c | 47 +++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/include/sof/lib/pm_runtime.h b/src/include/sof/lib/pm_runtime.h index 46081b7feda5..b6ae8a9961fa 100644 --- a/src/include/sof/lib/pm_runtime.h +++ b/src/include/sof/lib/pm_runtime.h @@ -40,6 +40,7 @@ enum pm_runtime_context { DMIC_POW, /**< DMIC Power */ DW_DMAC_CLK, /**< DW DMAC Clock */ CORE_MEMORY_POW, /**< Core Memory power */ + CORE_HP_CLK, /**< High Performance Clock*/ PM_RUNTIME_DSP /**< DSP */ }; diff --git a/src/platform/intel/cavs/include/cavs/lib/pm_runtime.h b/src/platform/intel/cavs/include/cavs/lib/pm_runtime.h index 94e590c83271..a1521893c3b9 100644 --- a/src/platform/intel/cavs/include/cavs/lib/pm_runtime.h +++ b/src/platform/intel/cavs/include/cavs/lib/pm_runtime.h @@ -22,6 +22,7 @@ struct pm_runtime_data; struct cavs_pm_runtime_data { int dsp_d0_sref; /**< simple ref counter, accessed by core 0 only */ int host_dma_l1_sref; /**< ref counter for Host DMA accesses */ + uint32_t sleep_core_mask; /**< represents cores in waiti state */ }; #endif diff --git a/src/platform/intel/cavs/lib/clk.c b/src/platform/intel/cavs/lib/clk.c index f7fec82698fa..8df72de1c647 100644 --- a/src/platform/intel/cavs/lib/clk.c +++ b/src/platform/intel/cavs/lib/clk.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -127,12 +128,12 @@ static void platform_clock_low_power_mode(int clock, bool enable) void platform_clock_on_wakeup(void) { - clock_low_power_mode(CLK_CPU(cpu_get_id()), false); + pm_runtime_get(CORE_HP_CLK, cpu_get_id()); } void platform_clock_on_waiti(void) { - clock_low_power_mode(CLK_CPU(cpu_get_id()), true); + pm_runtime_put(CORE_HP_CLK, cpu_get_id()); } #else diff --git a/src/platform/intel/cavs/lib/pm_runtime.c b/src/platform/intel/cavs/lib/pm_runtime.c index ea56e4722ff9..a7e5a30c8a1c 100644 --- a/src/platform/intel/cavs/lib/pm_runtime.c +++ b/src/platform/intel/cavs/lib/pm_runtime.c @@ -337,6 +337,47 @@ static inline void cavs_pm_runtime_core_en_memory(uint32_t index) #endif } +static inline void cavs_pm_runtime_core_dis_hp_clk(uint32_t index) +{ + int all_active_cores_sleep; + int enabled_cores = cpu_enabled_cores(); + struct pm_runtime_data *prd = pm_runtime_data_get(); + struct cavs_pm_runtime_data *pprd = prd->platform_data; + uint32_t flags; + + spin_lock_irq(&prd->lock, flags); + + pprd->sleep_core_mask |= BIT(index); + + all_active_cores_sleep = + (enabled_cores & pprd->sleep_core_mask) == enabled_cores; + + if (all_active_cores_sleep) + clock_low_power_mode(CLK_CPU(index), true); + + platform_shared_commit(prd, sizeof(*prd)); + platform_shared_commit(pprd, sizeof(*pprd)); + + spin_unlock_irq(&prd->lock, flags); +} + +static inline void cavs_pm_runtime_core_en_hp_clk(uint32_t index) +{ + struct pm_runtime_data *prd = pm_runtime_data_get(); + struct cavs_pm_runtime_data *pprd = prd->platform_data; + uint32_t flags; + + spin_lock_irq(&prd->lock, flags); + + pprd->sleep_core_mask &= ~BIT(index); + clock_low_power_mode(CLK_CPU(index), false); + + platform_shared_commit(prd, sizeof(*prd)); + platform_shared_commit(pprd, sizeof(*pprd)); + + spin_unlock_irq(&prd->lock, flags); +} + static inline void cavs_pm_runtime_dis_dsp_pg(uint32_t index) { #if CAVS_VERSION >= CAVS_VERSION_1_8 @@ -434,6 +475,9 @@ void platform_pm_runtime_get(enum pm_runtime_context context, uint32_t index, case CORE_MEMORY_POW: cavs_pm_runtime_core_en_memory(index); break; + case CORE_HP_CLK: + cavs_pm_runtime_core_en_hp_clk(index); + break; case PM_RUNTIME_DSP: cavs_pm_runtime_dis_dsp_pg(index); break; @@ -471,6 +515,9 @@ void platform_pm_runtime_put(enum pm_runtime_context context, uint32_t index, case CORE_MEMORY_POW: cavs_pm_runtime_core_dis_memory(index); break; + case CORE_HP_CLK: + cavs_pm_runtime_core_dis_hp_clk(index); + break; case PM_RUNTIME_DSP: cavs_pm_runtime_en_dsp_pg(index); break;