Add ARC MPU v3 & v6 support #37871

YuguoWH · 2021-08-23T10:37:51Z

Adding ARC MPU v3 and v6 support.
The v3 uses same logic with v2 and the only difference is to support 32 bytes minimal region size.
The v6 support 32 bytes, and it can have up to 32 regions. The regions are divided into 2 banks and switched by set/unset RB bit in MPU_EN aux register
This PR also introduces a new nsim hs board simulator which can validate/test with mpu v6.

YuguoWH · 2021-08-23T10:38:19Z

related issue #37309

abrodkin

Maybe move common functionality into version-agnostic file and then do specific things in v2 & v6 files to reduce duplication of code?

Also see some minor comments.

abrodkin · 2021-08-23T11:14:08Z

include/arch/arc/arch.h

Maybe

#if (CONFIG_ARC_MPU_VER == 4) || (CONFIG_ARC_MPU_VER == 6)

?

include/arch/arc/v2/linker.ld

abrodkin · 2021-08-23T11:29:10Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

arch/arc/core/mpu/arc_mpu_v2_internal.h

abrodkin · 2021-08-23T11:32:48Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

ZEPHYR_ARCH_ARC_CORE_MPU_ARC_MPU_V6_INTERNAL_H_ (see v6)

abrodkin · 2021-08-23T11:35:24Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

Why mention MPUv2 in code for MPUv6?

abrodkin · 2021-08-23T11:35:42Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

evgeniy-paltsev

Hi Yuguo,

please find some comments (mostly minor).

evgeniy-paltsev · 2021-08-23T22:37:26Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

return true;

evgeniy-paltsev · 2021-08-23T22:37:44Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

return false;

evgeniy-paltsev · 2021-08-23T22:39:31Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:42:43Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

Hmm, is it really about MPUv2?

evgeniy-paltsev · 2021-08-23T22:43:49Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:44:08Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:44:29Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:45:07Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:46:11Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

This one fits in 100 symbol per line limit so let's keep it in one line.

evgeniy-paltsev · 2021-08-23T22:47:46Z

soc/arc/snps_nsim/Kconfig

Hmm, Is it correct? We don't have MPU in this NSIM SoC configuration.

abrodkin

With a couple of style nitpicks LGTM.

include/arch/arc/arch.h

arch/arc/core/mpu/arc_mpu_v6_internal.h

abrodkin · 2021-08-24T14:39:46Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

Frankly I don't like these cryptic numbers. I guess these are some groups of bits shifted left?
Look at BIT_MASK() in https://github.com/zephyrproject-rtos/zephyr/blob/main/include/sys/util_macro.h#L68. Then we'll get something like BIT_MASK(xxx) << yyy which is much more readable.

abrodkin · 2021-08-24T14:41:38Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

Maybe BIT(x) and ~BIT(x) correspondigly?

abrodkin · 2021-08-24T14:42:29Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

evgeniy-paltsev · 2021-08-24T23:05:18Z

arch/arc/core/mpu/arc_mpu_common_internal.h

As we use r_index only in loop probably it's better to define it in loop itself:

for (int r_index = 0; r_index < get_num_regions(); r_index++) {

evgeniy-paltsev · 2021-08-24T23:08:19Z

arch/arc/core/mpu/arc_mpu_common_internal.h

I don't think we need to split definition and declaration here. Let's combine it:

/* comment */ int r_index = num_regions - mpu_config.num_regions;

evgeniy-paltsev · 2021-08-24T23:23:23Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

We don't exceed the Zephyr line limit here (100 symbols per line), please keep it in one line.

Please check rest of the code - we have a lot of places where we can put the whole expression in one line. It improves code readability a lot.

evgeniy-paltsev · 2021-08-24T23:24:15Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

evgeniy-paltsev · 2021-08-24T23:24:25Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

evgeniy-paltsev · 2021-08-24T23:25:06Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

Should we move those magic numbers to some defines / constants?

I guess it would be enough to also use much simpler understood BIT_MASK(2) & BIT_MASK(3) << 9 here.

Still It would be better to have all this shift values named. For example we can use "AUX_MPU_REGxx_FIELDxx_SHIFT"

evgeniy-paltsev · 2021-08-24T23:28:29Z

arch/arc/core/mpu/arc_mpu_common_internal.h

As we use i only in loop probably it's better to define it in loop itself:

for (uint32_t i = 0U; i < r_index; i++) {

evgeniy-paltsev · 2021-08-24T23:28:38Z

arch/arc/core/mpu/arc_mpu_common_internal.h

abrodkin

In commit titles it's better to say "add support of" instead of "to".
Also some comments below.

abrodkin · 2021-08-26T10:27:27Z

arch/arc/core/mpu/arc_mpu_v2_internal.h

So you don't like #define AUX_MPU_RDP_ATTR_MASK (BIT_MASK(6) << 3)? ;)

abrodkin · 2021-08-26T10:28:36Z

arch/arc/core/mpu/arc_mpu_v2_internal.h

And here #define AUX_MPU_RDP_SIZE_MASK (BIT_MASK(3) << 9 | BIT_MASK(2))?

arch/arc/core/mpu/arc_mpu_v6_internal.h

abrodkin · 2021-08-26T10:44:59Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

I guess it would be enough to also use much simpler understood BIT_MASK(2) & BIT_MASK(3) << 9 here.

evgeniy-paltsev · 2021-08-26T11:56:05Z

@YuguoWH could you please also add the nSIM board with v6 MPU so we can easily test it in our verification / manually?

boards/arc/nsim/nsim_hs_mpuv6_defconfig

abrodkin

With 2 nitpicks LGTM

abrodkin · 2021-08-27T09:10:16Z

arch/arc/core/mpu/arc_mpu_v2_internal.h

Why not use BIT_MASK here as well as in the next line?

then it would be:
#define AUX_MPU_RDP_REGION_SIZE(size) (((size - 1) & BIT_MASK(2)) | \ (((size - 1) & (BIT_MASK(3) << 2)) << 7))
I think it looks more complex...

abrodkin · 2021-08-27T09:10:38Z

arch/arc/core/mpu/arc_mpu_v6_internal.h

evgeniy-paltsev · 2021-08-27T09:26:17Z

@YuguoWH looks like it need to be rebased on top of cd87366

Add support of ARC mpu version 3 which can have region size down to 32 bytes Signed-off-by: Yuguo Zou <[email protected]>

YuguoWH · 2021-08-27T09:57:52Z

Hi @evgeniy-paltsev , PR rebased, thanks.

Add support of ARC mpu v6 * minimal region size down to 32 bytes * maximal region number up to 32 * not support uncacheable region and volatile uncached region * clean up mpu code for better readablity Signed-off-by: Yuguo Zou <[email protected]>

soc/arc/snps_nsim/Kconfig.defconfig.hs_mpuv6

We add support of mpu v6 therefore it is needed to have a board to validate that feature. This commit add a new HS nsim simulator which supports mpu v6. Signed-off-by: Yuguo Zou <[email protected]>

abrodkin · 2021-08-27T14:56:05Z

@nashif could you please pick this one up for v2.7?

ruuddw · 2021-08-30T11:45:36Z

soc/arc/snps_nsim/Kconfig

+config SOC_NSIM_HS_MPUV6
+	bool "Synopsys ARC HS with MPU v6 in nSIM"
+	select CPU_HAS_MPU
+	select CPU_HAS_FPU


Why is FPU enabled?

I keep it same with other NSIM_HS boards, so FPU is enabled

YuguoWH requested review from abrodkin and evgeniy-paltsev August 23, 2021 10:37

YuguoWH requested a review from ruuddw as a code owner August 23, 2021 10:37

github-actions bot added area: API Changes to public APIs area: ARC ARC Architecture labels Aug 23, 2021

YuguoWH force-pushed the topic-mpu-v6 branch from 7630b82 to e1e2706 Compare August 23, 2021 10:46

abrodkin requested changes Aug 23, 2021

View reviewed changes

zephyrbot added the platform: Synopsys Synopsys label Aug 23, 2021

zephyrbot requested a review from IRISZZW August 23, 2021 13:10

zephyrbot assigned ruuddw Aug 23, 2021

evgeniy-paltsev requested changes Aug 23, 2021

View reviewed changes

YuguoWH force-pushed the topic-mpu-v6 branch 5 times, most recently from 5e0f2ae to 36ad8bd Compare August 24, 2021 12:56

abrodkin approved these changes Aug 24, 2021

View reviewed changes

abrodkin added this to the v2.7.0 milestone Aug 24, 2021

ruuddw approved these changes Aug 24, 2021

View reviewed changes

evgeniy-paltsev requested changes Aug 24, 2021

View reviewed changes

YuguoWH force-pushed the topic-mpu-v6 branch 2 times, most recently from 489f934 to 43ac849 Compare August 26, 2021 07:12

abrodkin requested changes Aug 26, 2021

View reviewed changes

YuguoWH force-pushed the topic-mpu-v6 branch from 43ac849 to ae43ee7 Compare August 27, 2021 07:25

YuguoWH requested review from nashif and tejlmand as code owners August 27, 2021 07:25

github-actions bot added area: Boards area: Build System labels Aug 27, 2021

github-actions bot added the area: Devicetree label Aug 27, 2021

YuguoWH commented Aug 27, 2021

View reviewed changes

boards/arc/nsim/nsim_hs_mpuv6_defconfig Outdated Show resolved Hide resolved

YuguoWH force-pushed the topic-mpu-v6 branch 2 times, most recently from f943954 to 8cdf600 Compare August 27, 2021 08:59

abrodkin approved these changes Aug 27, 2021

View reviewed changes

arch: arc: add support of mpu v3

c24d162

Add support of ARC mpu version 3 which can have region size down to 32 bytes Signed-off-by: Yuguo Zou <[email protected]>

YuguoWH force-pushed the topic-mpu-v6 branch from 8cdf600 to a867f4f Compare August 27, 2021 09:56

arch: arc: add support of mpu v6

cb5a423

Add support of ARC mpu v6 * minimal region size down to 32 bytes * maximal region number up to 32 * not support uncacheable region and volatile uncached region * clean up mpu code for better readablity Signed-off-by: Yuguo Zou <[email protected]>

YuguoWH force-pushed the topic-mpu-v6 branch from a867f4f to 5ae2eae Compare August 27, 2021 10:02

evgeniy-paltsev reviewed Aug 27, 2021

View reviewed changes

soc/arc/snps_nsim/Kconfig.defconfig.hs_mpuv6 Outdated Show resolved Hide resolved

boards: arc: add a nsim_hs_mpuv6 board simulator

67bbdac

We add support of mpu v6 therefore it is needed to have a board to validate that feature. This commit add a new HS nsim simulator which supports mpu v6. Signed-off-by: Yuguo Zou <[email protected]>

YuguoWH force-pushed the topic-mpu-v6 branch from 5ae2eae to 67bbdac Compare August 27, 2021 12:11

abrodkin removed the request for review from IRISZZW August 27, 2021 14:54

evgeniy-paltsev requested review from IRISZZW and evgeniy-paltsev August 27, 2021 14:55

evgeniy-paltsev approved these changes Aug 27, 2021

View reviewed changes

nashif approved these changes Aug 27, 2021

View reviewed changes

nashif merged commit 7d8d4fd into zephyrproject-rtos:main Aug 27, 2021

abrodkin mentioned this pull request Aug 27, 2021

ARC: add MPU v6 (and others) support #37309

Closed

YuguoWH deleted the topic-mpu-v6 branch August 30, 2021 06:02

ruuddw reviewed Aug 30, 2021

View reviewed changes

Add ARC MPU v3 & v6 support #37871

Add ARC MPU v3 & v6 support #37871

Uh oh!

Conversation

YuguoWH commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YuguoWH commented Aug 23, 2021

Uh oh!

abrodkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniy-paltsev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrodkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrodkin left a comment

Choose a reason for hiding this comment

YuguoWH commented Aug 23, 2021 •

edited

Loading