Skip to content
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

Jump to bottom

soc: intel_adsp/ace: use functions to do CPU power control #59799

Merged
merged 1 commit into from
Jul 10, 2023
+98 −6
Merged

soc: intel_adsp/ace: use functions to do CPU power control #59799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 46 additions & 0 deletions soc/xtensa/intel_adsp/ace/include/intel_ace15_mtpm/adsp_power.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/

#include <stdint.h>
#include <zephyr/toolchain.h>
#include <zephyr/sys/util_macro.h>
#include <adsp_shim.h>

#ifndef ZEPHYR_SOC_INTEL_ADSP_POWER_H_
Expand Down Expand Up @@ -43,4 +46,47 @@ struct ace_pwrsts {

#define ACE_PWRSTS ((volatile struct ace_pwrsts *) &ACE_DfPMCCU.dfpwrsts)

/**
* @brief Power up a specific CPU.
*
* This sets the "not power gating" bit in the power control
* register to disable power gating to CPU, thus powering up
* the CPU.
*
* @param cpu_num CPU to be powered up.
*/
static ALWAYS_INLINE void soc_cpu_power_up(int cpu_num)
{
ACE_PWRCTL->wpdsphpxpg |= BIT(cpu_num);
}

/**
* @brief Power down a specific CPU.
*
* This clears the "not power gating" bit in the power control
* register to enable power gating to CPU, thus powering down
* the CPU.
*
* @param cpu_num CPU to be powered down.
*/
static ALWAYS_INLINE void soc_cpu_power_down(int cpu_num)
{
ACE_PWRCTL->wpdsphpxpg &= ~BIT(cpu_num);
}

/**
* @brief Test if a CPU is currently powered.
*
* This queries the power status register to see if the CPU
* is currently powered.
*
* @param cpu_num CPU to be queried.
* @return True if CPU is powered, false if now.
*/
static ALWAYS_INLINE bool soc_cpu_is_powered(int cpu_num)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't these names a bit too generic? I mean no ace_ prefix/namespace or anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could change these if you insist. I made these generic so we don't need to rename stuff if we ever need to share code with another audio DSP architecture in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could change these if you insist. I made these generic so we don't need to rename stuff if we ever need to share code with another audio DSP architecture in the future.

Thanks, I was wondering if this was by design. If this is generic by design then great but then leave a short comment that it's meant to be. There is zero comment right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the function description (doxygen). Hopefully this would be enough.

Unless we have a porting guide, it's hard to convey the idea of "generic function name to ease future platform addition" in the comment without it sounding out of place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, the doxygen looks useful but that's not what I had in mind.

it's hard to convey the idea of "generic function name to ease future platform addition" in the comment

I think you just did, brilliantly :-)

without it sounding out of place.

I don't see why it would sound out of place.

It's not important, nevermind.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opt for including this in the commit message.

{
return (ACE_PWRSTS->dsphpxpgs & BIT(cpu_num)) == BIT(cpu_num);
}


#endif /* ZEPHYR_SOC_INTEL_ADSP_POWER_H_ */
46 changes: 46 additions & 0 deletions soc/xtensa/intel_adsp/ace/include/intel_ace20_lnl/adsp_power.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
#ifndef ZEPHYR_SOC_INTEL_ADSP_POWER_H_
#define ZEPHYR_SOC_INTEL_ADSP_POWER_H_

#include <stdint.h>
#include <zephyr/toolchain.h>
#include <zephyr/sys/util_macro.h>

/* Value used as delay when waiting for hw register state change. */
#define HW_STATE_CHECK_DELAY 64

Expand Down Expand Up @@ -42,4 +46,46 @@ struct ace_pwrsts {

#define ACE_PWRSTS ((volatile struct ace_pwrsts *)PWRSTS_REG)

/**
* @brief Power up a specific CPU.
*
* This sets the "not power gating" bit in the power control
* register to disable power gating to CPU, thus powering up
* the CPU.
*
* @param cpu_num CPU to be powered up.
*/
static ALWAYS_INLINE void soc_cpu_power_up(int cpu_num)
{
ACE_PWRCTL->wpdsphpxpg |= BIT(cpu_num);
}

/**
* @brief Power down a specific CPU.
*
* This clears the "not power gating" bit in the power control
* register to enable power gating to CPU, thus powering down
* the CPU.
*
* @param cpu_num CPU to be powered down.
*/
static ALWAYS_INLINE void soc_cpu_power_down(int cpu_num)
{
ACE_PWRCTL->wpdsphpxpg &= ~BIT(cpu_num);
}

/**
* @brief Test if a CPU is currently powered.
*
* This queries the power status register to see if the CPU
* is currently powered.
*
* @param cpu_num CPU to be queried.
* @return True if CPU is powered, false if now.
*/
static ALWAYS_INLINE bool soc_cpu_is_powered(int cpu_num)
{
return (ACE_PWRSTS->dsphpxpgs & BIT(cpu_num)) == BIT(cpu_num);
}

#endif /* ZEPHYR_SOC_INTEL_ADSP_POWER_H_ */
6 changes: 3 additions & 3 deletions soc/xtensa/intel_adsp/ace/multiprocessing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ void soc_start_core(int cpu_num)
#endif

sys_cache_data_flush_range(rom_jump_vector, sizeof(*rom_jump_vector));
ACE_PWRCTL->wpdsphpxpg |= BIT(cpu_num);
soc_cpu_power_up(cpu_num);

while ((ACE_PWRSTS->dsphpxpgs & BIT(cpu_num)) == 0) {
while (!soc_cpu_is_powered(cpu_num)) {
k_busy_wait(HW_STATE_CHECK_DELAY);
}

Expand Down Expand Up @@ -194,7 +194,7 @@ int soc_adsp_halt_cpu(int id)
return -EINVAL;
}

ACE_PWRCTL->wpdsphpxpg &= ~BIT(id);
soc_cpu_power_down(id);
return 0;
}
#endif
6 changes: 3 additions & 3 deletions soc/xtensa/intel_adsp/ace/power.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ __weak void pm_state_set(enum pm_state state, uint8_t substate_id)
} else if (state == PM_STATE_RUNTIME_IDLE) {
DSPCS.bootctl[cpu].bctl &= ~DSPBR_BCTL_WAITIPPG;
DSPCS.bootctl[cpu].bctl &= ~DSPBR_BCTL_WAITIPCG;
ACE_PWRCTL->wpdsphpxpg &= ~BIT(cpu);
soc_cpu_power_down(cpu);
if (cpu == 0) {
uint32_t battr = DSPCS.bootctl[cpu].battr & (~LPSCTL_BATTR_MASK);

Expand Down Expand Up @@ -337,9 +337,9 @@ __weak void pm_state_exit_post_ops(enum pm_state state, uint8_t substate_id)
return;
}

ACE_PWRCTL->wpdsphpxpg |= BIT(cpu);
soc_cpu_power_up(cpu);

while ((ACE_PWRSTS->dsphpxpgs & BIT(cpu)) == 0) {
while (!soc_cpu_is_powered(cpu)) {
k_busy_wait(HW_STATE_CHECK_DELAY);
}

Expand Down
Loading