Skip to content

arc: cache: Support region operations, SLC, and entire cache operations #86214

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

arc: cache: Support region operations, SLC, and entire cache operations #86214

wants to merge 1 commit into from
+511 −22

Conversation

yanir10
Copy link
Contributor

@yanir10 yanir10 commented Feb 24, 2025

  • Added configurable support for L1 cache region operations, which offers improved performance over line operations.
  • Added configurable support for SLC (system level cache).
  • Added support for entire cache operations: flush_all, invd_all, flush_and_invd_all.

ruuddw
ruuddw reviewed Feb 27, 2025
View reviewed changes
Copy link
Member

@ruuddw ruuddw left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few small remarks inline.

arch/arc/Kconfig Outdated
@@ -370,6 +370,28 @@ endmenu
config DCACHE_LINE_SIZE
default 32

config DCACHE_REGION_OPERATIONS
bool "DCACHE region operations"
depends on CACHE_MANAGEMENT && DCACHE && CPU_HS4X
Copy link
Member

Choose a reason for hiding this comment

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

There are more ARC cores supporting region operations, let's remove the "CPU_HS4X" restriction but make the feature default n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

arch/arc/Kconfig Outdated
config SLC_LINE_SIZE
int "SLC line size"
depends on SLC
default 64
Copy link
Member

Choose a reason for hiding this comment

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

For the hardware (design-time) the default L2 cache size is 128, not 64. (but ok to leave it 64 if you prefer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

arch/arc/core/cache.c Outdated
@@ -39,6 +40,24 @@ size_t sys_cache_line_size;
#define DC_CTRL_DIRECT_ACCESS 0x0 /* direct access mode */
#define DC_CTRL_INDIRECT_ACCESS 0x20 /* indirect access mode */
#define DC_CTRL_OP_SUCCEEDED 0x4 /* d-cache operation succeeded */
#define DC_CTRL_IM_MASK BIT(6) /* d-cache invalidate mode bit */
Copy link
Member

Choose a reason for hiding this comment

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

I like the BIT(n) style, but now both styles are mixed in this file. Could you please either change the existing defines, or adopt to the earlier style for the new ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to the earlier style

arch/arc/core/cache.c Outdated
}
}

__maybe_unused void slc_flush_and_invalidate_region(void *start_addr_ptr, size_t size)
Copy link
Member

Choose a reason for hiding this comment

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

any reason for this function to not be 'static'? (why here the 'maybe unused', and not for the other ones?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

arch/arc/core/cache.c Outdated

return 0;
}

int arch_dcache_flush_and_invd_range(void *start_addr_ptr, size_t size)
{
#if defined(CONFIG_DCACHE_REGION_OPERATIONS)
Copy link
Member

Choose a reason for hiding this comment

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

This function follows a different flow/structure. I assume it is because there was no (line based) implementation before. ok to leave it like this, but maybe for consistency you could add the line-based variant as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is because there was no line based implementation.
Added "dcache_flush_and_invalidate_lines"

arch/arc/core/cache.c
*/
if (pae_exists()) {
#if defined(CONFIG_DCACHE_REGION_OPERATIONS)
dcache_high_addr_init();
Copy link
Member

Choose a reason for hiding this comment

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

ok to explicitly do this, but I believe the hardware reset value is also 0.

@yanir10 yanir10 force-pushed the arc_cache branch 2 times, most recently from f2a2a7c to 109e5f1 Compare March 2, 2025 08:37
@yanir10 yanir10 requested a review from ruuddw March 2, 2025 08:41
@ruuddw
Copy link
Member

ruuddw commented Mar 11, 2025

Thanks, this looks good now. But when internally discussing I was made aware of a possible issue:
"STAR 5103816 - Region Operations on Regions Smaller Than One Cache Line No Not Take Effect (Fixed in 4.10a)"
When writes are made to the SLC_AUX_RGN_START register to start a region operation, the least significant bits are not ignored as stated in the Programmer's Reference. The result is that the cache line on which the start address maps is only included in the region operation if the start address aligns with the start address of the cache line.
I'm not sure what ARC HS4x version you're using, but maybe add the alignment for the SLC (not that much overhead) to be sure, or at least add a remark in the code referring the problem for other users?

arc: cache: Support region operations, SLC, and entire cache operations
6242dc7 
Added configurable support for L1 cache region operations,
which offers improved performance over line operations.
Added configurable support for SLC (system level cache).
Added support for entire cache operations: flush_all,
invd_all, flush_and_invd_all.

Reviewed-by: Aaron Komisar <[email protected]>
Signed-off-by: Yanir Levin <[email protected]>
ruuddw
ruuddw approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@ruuddw ruuddw left a comment

Choose a reason for hiding this comment

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

Thanks!

@kartben
Copy link
Collaborator

kartben commented Apr 18, 2025

@evgeniy-paltsev @abrodkin can you help review?

@kartben kartben requested a review from Copilot April 18, 2025 19:12
Copilot
Copilot AI reviewed Apr 18, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds configurable support for L1 cache region operations, SLC (system level cache), and entire cache operations, enabling improved cache management.

  • Introduces new auxiliary register definitions for DC region operations.
  • Adds new registers for SLC control, flush, invalidate, region operations, and build ID.
Files not reviewed (1)
  • arch/arc/Kconfig: Language not supported
Comments suppressed due to low confidence (1)

include/zephyr/arch/arc/v2/aux_regs.h:188

  • The hex value for _ARC_V2_SLC_BUILD (0xCE) appears inconsistent with the surrounding SLC register definitions in the 0x900 range. Please verify if this value is correct and intended.
#define _ARC_V2_SLC_BUILD        0xCE

@dcpleung
Copy link
Member

Suggestion: prefix your kconfig with ARC_* as these are ARC specific, at least for now. A short kconfig like CONFIG_SLC is less descriptive and has higher likelihood of conflicting with any future additions in other areas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@ruuddw ruuddw ruuddw approved these changes

@abrodkin abrodkin Awaiting requested review from abrodkin

@dcpleung dcpleung Awaiting requested review from dcpleung

@evgeniy-paltsev evgeniy-paltsev Awaiting requested review from evgeniy-paltsev

@nashif nashif Awaiting requested review from nashif

@tagunil tagunil Awaiting requested review from tagunil

At least 2 approving reviews are required to merge this pull request.

Assignees

@ruuddw ruuddw

@evgeniy-paltsev evgeniy-paltsev

Labels
area: ARC ARC Architecture area: Architectures
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yanir10 @ruuddw @kartben @dcpleung @evgeniy-paltsev @zephyrbot