Skip to content

platform: cavs: remove hw init in platform_init() on sof side #7896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 3 commits into from
Jul 13, 2023
Merged

platform: cavs: remove hw init in platform_init() on sof side #7896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e356b1b
platform: cavs: don't program general owner and io registers
RanderWang Jul 6, 2023
3acdf10
platform: cavs: remove hw clk setting in platform_init
RanderWang Jul 6, 2023
87290fe
cavs: remove CONFIG_CAVS_LPRO_ONLY and clean up the code
RanderWang Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/platform/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,14 +354,6 @@ config CAVS_LPS
This option is required to support S0ix/D0ix mode
on cAVS platforms.

config CAVS_LPRO_ONLY
bool "Use low power ring oscillator always"
default n
depends on CAVS
help
Select if you want to use only the 120MHz LPRO as the DSP clock source.
This option is for debugging only at the moment, choose n if unclear.

config CAVS_USE_LPRO_IN_WAITI
bool "Use low power ring oscillator in WFI"
default n
Expand Down
30 changes: 1 addition & 29 deletions src/platform/intel/cavs/lib/clk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,36 +86,8 @@ static inline void select_cpu_clock(int freq_idx, bool release_unused)
clock_unlock(key);
}

/* LPRO_ONLY mode */
#if CONFIG_CAVS_LPRO_ONLY

/*
* 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);
}

static void platform_clock_low_power_mode(int clock, bool enable)
{
/* do nothing for LPRO_ONLY mode */
}

void platform_clock_on_waiti(void)
{
/* do nothing for LPRO_ONLY mode */
}

void platform_clock_on_wakeup(void)
{
/* do nothing for LPRO_ONLY mode */
}

/* USE_LPRO_IN_WAITI mode */
#elif CONFIG_CAVS_USE_LPRO_IN_WAITI
#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.
Expand Down
54 changes: 1 addition & 53 deletions src/platform/intel/cavs/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,9 @@ const struct ext_man_windows xsram_window
#endif

#if CONFIG_TIGERLAKE
#if CONFIG_CAVS_LPRO_ONLY
#define CAVS_DEFAULT_RO SHIM_CLKCTL_RLROSCC
#define CAVS_DEFAULT_RO_FOR_MEM SHIM_CLKCTL_OCS_LP_RING
#else

@lyakh lyakh Jul 11, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So this commit was wrong? It seems to suggest that LPRO was also available for cAVS 2.0 034c54a

@RanderWang RanderWang Jul 12, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I deleted CONFIG_CAVS_LPRO_ONLY which is for debugging only at the moment, choose n if unclear

@lyakh lyakh Jul 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yeah, that was just a renaming in 59c3315 so from those commits it looks like this was a debug option also available for TGL. It's ok if we remove it because we don't need or don't want it any more, but at least I don't see that it was only available for CFL

@RanderWang RanderWang Jul 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lyakh I did involved in this feature and you may recall it. In 2019, we found a HW hang on HDA platform only on CFL and debugged it for almost a month, finally confirmed by HW team that we need to use LPO for HW restraint. But we don't need it for CML and successor HW platform.

@lyakh lyakh Jul 13, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, good, maybe it would just be a bit clearer if we said something like "remove this debug option because we don't need it any more" in the commit message - just by looking at the patch nothing in it explains why it's only relevant up to CFL

#define CAVS_DEFAULT_RO SHIM_CLKCTL_RHROSCC
#define CAVS_DEFAULT_RO_FOR_MEM SHIM_CLKCTL_OCS_HP_RING
#endif
#endif

#if CONFIG_DW_GPIO

Expand Down Expand Up @@ -304,7 +299,7 @@ int platform_boot_complete(uint32_t boot_message)
{
struct ipc_cmd_hdr header;

#if CONFIG_TIGERLAKE && !CONFIG_CAVS_LPRO_ONLY
#if CONFIG_TIGERLAKE
/* TGL specific HW recommended flow */
pm_runtime_get(PM_RUNTIME_DSP, PWRD_BY_HPRO | (CONFIG_CORE_COUNT - 1));
#endif
Expand All @@ -327,26 +322,6 @@ int platform_boot_complete(uint32_t boot_message)

#endif

/* init HW */
static void platform_init_hw(void)
{
io_reg_write(DSP_INIT_GENO,
GENO_MDIVOSEL | GENO_DIOPTOSEL);

io_reg_write(DSP_INIT_IOPO,
IOPO_DMIC_FLAG | IOPO_I2S_FLAG);

#ifndef __ZEPHYR__
io_reg_write(DSP_INIT_ALHO,
ALHO_ASO_FLAG | ALHO_CSO_FLAG);

io_reg_write(DSP_INIT_LPGPDMA(0),
LPGPDMA_CHOSEL_FLAG | LPGPDMA_CTLOSEL_FLAG);
io_reg_write(DSP_INIT_LPGPDMA(1),
LPGPDMA_CHOSEL_FLAG | LPGPDMA_CTLOSEL_FLAG);
#endif
}

/* Runs on the primary core only */
int platform_init(struct sof *sof)
{
Expand Down Expand Up @@ -377,9 +352,6 @@ int platform_init(struct sof *sof)
*/
pm_runtime_disable(PM_RUNTIME_DSP, 0);

trace_point(TRACE_BOOT_PLATFORM_ENTRY);
platform_init_hw();

trace_point(TRACE_BOOT_PLATFORM_IRQ);
platform_interrupt_init();

Expand Down Expand Up @@ -412,35 +384,11 @@ int platform_init(struct sof *sof)
trace_point(TRACE_BOOT_PLATFORM_CPU_FREQ);

#if CONFIG_TIGERLAKE
Comment thread
lgirdwood marked this conversation as resolved.
/* initialize PM for boot */

/* request configured ring oscillator and wait for status ready */
shim_write(SHIM_CLKCTL, shim_read(SHIM_CLKCTL) | CAVS_DEFAULT_RO);
while (!(shim_read(SHIM_CLKSTS) & CAVS_DEFAULT_RO))
idelay(16);

shim_write(SHIM_CLKCTL,
CAVS_DEFAULT_RO | /* Request configured RING Osc */
CAVS_DEFAULT_RO_FOR_MEM | /* Select configured
* RING Oscillator Clk for memory
*/
SHIM_CLKCTL_HMCS_DIV2 | /* HP mem clock div by 2 */
SHIM_CLKCTL_LMCS_DIV4 | /* LP mem clock div by 4 */
SHIM_CLKCTL_TCPLCG_DIS_ALL); /* Allow Local Clk Gating */

/* prevent LP GPDMA 0&1 clock gating */
shim_write(SHIM_GPDMA_CLKCTL(0), SHIM_CLKCTL_LPGPDMAFDCGB);
shim_write(SHIM_GPDMA_CLKCTL(1), SHIM_CLKCTL_LPGPDMAFDCGB);

/* prevent DSP Common power gating */
pm_runtime_get(PM_RUNTIME_DSP, PLATFORM_PRIMARY_CORE_ID);

#if CONFIG_DSP_RESIDENCY_COUNTERS
#if CONFIG_CAVS_LPRO_ONLY
init_dsp_r_state(r1_r_state);
#else
init_dsp_r_state(r0_r_state);
#endif /* CONFIG_CAVS_LPRO_ONLY */
#endif /* CONFIG_DSP_RESIDENCY_COUNTERS */
#endif /* CONFIG_TIGERLAKE */

Expand Down
4 changes: 0 additions & 4 deletions src/platform/tigerlake/include/platform/lib/clk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@

#define CPU_LOWEST_FREQ_IDX CPU_WOVCRO_FREQ_IDX

#if CONFIG_CAVS_LPRO_ONLY
#define CPU_DEFAULT_IDX CPU_LPRO_FREQ_IDX
#else
#define CPU_DEFAULT_IDX CPU_HPRO_FREQ_IDX
#endif
Comment thread
RanderWang marked this conversation as resolved.
Outdated

#define SSP_DEFAULT_IDX 1

Expand Down