intel_adsp: ace: Refactor HPSRAM power-down mechanism for Intel ADSP ACE platforms #78342
Conversation
|
The changes will remain in draft until they are fully tested. |
There was a problem hiding this comment.
yes, the current power_down() -> m_ace_hpsram_power_change() seem to be... more complex than they need to, and this looks like a good direction to simplify them, but (1) we need to make sure that while simplifying we don't lose required flexibility, and (2) I'd propose to first fix the current issue and then optimise the power-down flow
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
0233828
to
ce954ac
Compare
|
Update: rebase + commit refactoring the disabling of lpsram (change made on the occasion of debugging, removes the definitions of registers from this header). |
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
ce954ac
to
9649f96
Compare
zephyrbot
added
area: Xtensa
zephyrbot
requested review from
andyross,
ceolin,
jxstelter,
kv2019i,
lgirdwood,
marcinszkudlinski,
nashif and
softwarecki
|
The changes are ready for review. Earlier tests did not show any problems on MTL and LNL. However, a strange problem appeared on PTL, so I added the DNM label. |
There was a problem hiding this comment.
In general it does look cleaner to just count banks instead of bit-toggling a mask, but what about multiple segments? I know that currently they aren't really supported but if we hard-fix 1 segment, we should document that? Also not sure if we really need that register in DT? Have you submitted a SOF PR to test this? If not - please do!
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
9649f96
to
f97015d
Compare
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
493d759
to
d5349e7
Compare
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
d5349e7
to
52dfb96
Compare
…down This commit addresses an issue on platforms with an MMU where a LoadStoreTLBMissCause exception occurs when accessing hardware registers during the power-down process. The exception arises when attempting to access the IPC register after HPSRAM has been powered down, leading to a double exception: LoadStoreTLBMissCause followed by InstrPIFDataErrorCause. To resolve this, we preload the IPC register before shutting down LPSRAM. This change prevents the double exception by ensuring that the page table entries are correctly managed in the TLB before HPSRAM is powered down and allowing the power-down sequence to complete successfully. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
This commit improves the readability of the power_down.S assembly file by standardizing the indentation of the preprocessor definitions. No functional changes have been made; this is purely a cosmetic update to the code formatting. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
This commit introduces the L2 Memory Capabilities (hsbcap) register node to the Devicetree specifications for Intel ADSP ACE platforms. The hsbcap register provides information on the general capabilities associated with the L2 memory, which is critical for system configuration and resource management. The hsbcap node has been added to the Devicetree source files for ACE 1.5 (MTPM), ACE 2.0 (LNL), and ACE 3.0 (PTL) platforms. In addition, the DFL2MM_REG macro in adsp_memory.h has been updated to use the Devicetree node label for hsbcap, ensuring a consistent and maintainable approach to accessing this register across the codebase. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
Introduce a new assembly macro, m_ace_hpsram_power_down_entire, which utilizes Zephyr Devicetree macros to power down the entire HPSRAM on Intel ADSP ACE platforms. This macro dynamically retrieves the HPSRAM bank count and control register address from the Devicetree, streamlining the power-down process. The macro is designed to iterate over all HPSRAM banks and issue a power down command to each, ensuring a complete shutdown of the HPSRAM when required by the system's power management policy. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
…macro Refactor the power_down function to utilize the newly introduced m_ace_hpsram_power_down_entire macro for shutting down the entire HPSRAM. This change simplifies the power-down process by replacing the previous segment-based power gating mask approach with a single boolean flag that indicates whether the entire HPSRAM should be disabled. The function signature of power_down has been updated to accept the new boolean flag, and the corresponding call sites have been modified to pass the flag based on the CONFIG_ADSP_POWER_DOWN_HPSRAM Kconfig option. Additionally, the assembly code has been cleaned up to remove the now-obsolete hpsram_mask array and related logic. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
Remove the m_ace_hpsram_power_change macro from asm_memory_management.h as it is no longer used after refactoring the power_down function to utilize the new m_ace_hpsram_power_down_entire macro. This cleanup helps to reduce code complexity and maintainability. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
Replace hardcoded register addresses and values in asm_memory_management.h with Devicetree (DT) macros for LPSRAM power-down operations. This change ensures that register addresses and bank counts are dynamically obtained from the Devicetree, improving code portability and reducing the risk of errors due to manual updates. - Removed hardcoded LSPGCTL address definitions. - Updated m_ace_lpsram_power_down_entire macro to use DT_NODELABEL to fetch LPSRAM bank count and control register address - Adjusted bit field extraction logic to align with the updated register information from the Devicetree. This commit aligns with the ongoing effort to utilize Devicetree for hardware abstraction and to facilitate easier maintenance and updates to the codebase. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
tmleman
force-pushed
the
topic/upstream/pr/intel/ace/power_down_hpsram
branch
from
52dfb96
to
05e45e6
Compare
| movi \ay, 1 /* Power down command */ | ||
|
|
||
| /* Calculate the address of the HSxPGCTL register */ | ||
| movi \az, DT_REG_ADDR(DT_NODELABEL(hsbpm)) |
There was a problem hiding this comment.
Since we discussed the use of movi before, do I understand it correctly, that up to this point movis are good, but not inside the loop, where memory banks are already being switched off? Maybe a follow-up PR to add a comment here about that
| * if (b_disable_hpsram) { | ||
| * ace_hpsram_power_down_entire(); | ||
| * } | ||
| */ |
There was a problem hiding this comment.
also for a follow-up PR, maybe add a comment, saying that LPSRAM isn't used to run code, so we aren't running there now
|
We are only merging bug fixes and documentation updates to the 4.0 release. Is this something that can wait after the 4.0 release? |
@mmahadevan108 I think we can wait until 4.1 |
This pull request introduces a series of changes to the HPSRAM power-down mechanism for Intel ADSP ACE platforms, improving code maintainability and readability. The updates include the introduction of a new assembly macro, cleanup of obsolete code, and refactoring of the
power_downfunction to align with the new approach.Key Changes:
hsbcap) register node to the Devicetree specifications for ACE platforms, providing essential information for system configuration and resource management.m_ace_hpsram_power_down_entireusing Zephyr Devicetree macros to dynamically retrieve HPSRAM bank count and control register address, simplifying the power-down process.power_downFunction: Updates thepower_downfunction to use the newm_ace_hpsram_power_down_entiremacro, replacing the previous segment-based power gating mask approach with a single boolean flag.m_ace_hpsram_power_changemacro fromasm_memory_management.h, streamlining the codebase.