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.

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

Arch arm mpu regions index number in DTS #12173

Merged
merged 6 commits into from
May 20, 2019

Conversation

ioannisg
Copy link
Member

We can describe even ARM Cortex-M core peripherals in DTS; we do it with NVIC and SysTick already; the MPU could be the next. We create an mpu node, and use DTS to describe the number of HW regions, which is implementation-defined.

We use the DTS-generated macro directly in _get_num_regions(), so we do not need to evaluate the MPU_TYPE register at run-time, all the time.

@ioannisg ioannisg added the RFC Request For Comments: want input from the community label Dec 19, 2018
@ioannisg ioannisg changed the title Arch arm mpu regions indts Arch arm mpu regions index number in DTS Dec 19, 2018
@codecov-io
Copy link

Codecov Report

Merging #12173 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12173   +/-   ##
=======================================
  Coverage   48.06%   48.06%           
=======================================
  Files         284      284           
  Lines       43383    43383           
  Branches    10397    10397           
=======================================
  Hits        20853    20853           
  Misses      18389    18389           
  Partials     4141     4141

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b9cfe7...236fdbe. Read the comment docs.

@ioannisg ioannisg added the area: ARM ARM (32-bit) Architecture label Dec 19, 2018
@ioannisg ioannisg requested a review from agross-oss December 19, 2018 15:03
@ioannisg
Copy link
Member Author

@agross-linaro could you provide comments on this PR?

@pizi-nordic pizi-nordic self-requested a review December 19, 2018 15:35
@ioannisg ioannisg requested a review from anangl December 19, 2018 15:37
@pizi-nordic
Copy link
Collaborator

Why we need that? In all modified architectures you can read the number of MPU regions in runtime.
Do we need this value during compilation?

@ioannisg ioannisg force-pushed the arch_arm_mpu_regions_indts branch 3 times, most recently from 350001f to 556f28b Compare April 24, 2019 20:21
@ioannisg ioannisg removed the RFC Request For Comments: want input from the community label Apr 24, 2019
@ioannisg ioannisg force-pushed the arch_arm_mpu_regions_indts branch 2 times, most recently from 12d60dc to 82a3a7f Compare April 24, 2019 21:43
@@ -22,6 +22,11 @@
reg = <0xe000e010 0x10>;
status = "disabled";
};

mpu: mpu@e000ed90 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this really make sense here, since not all arm soc's have MPUs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The MPU registers can always be accessed (some of them will be RAZ/WI, of course, if the MPU is not present). So this does not harm, IMHO, as we don't really define the "regions" property, here. The "regions" property and the corresponding DT_ macro are, anyway, defined in SOC/Board DTS fixup.

I don't see a better place to define the MPU node, actually. We do the same for NVIC. And the actual "prio-bits" property is defined in the SOC level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I feel like all the SoCs have NVIC which is why we do it, and if one doesn't it should delete the node.

I'd rather see we add MPU nodes to the SoCs that have them instead of at this level.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Rather than adding mpu nodes to armv{6,7,8}-m.dtsi I'd like to see we add them to the SoCs dtsi files that have mpus.

@ioannisg
Copy link
Member Author

Rather than adding mpu nodes to armv{6,7,8}-m.dtsi I'd like to see we add them to the SoCs dtsi files that have mpus.

@galak, I see your point, but in my opinion this is, mostly, a matter of style :)

True, all SoCs have NVIC, and that's, perhaps, why the nvic node is defined in the arm_.dtsi headers. But the actual number of priority bits of NVIC is implementation defined. And that's why we have the following definitions in the SOC .dtsi headers:

&nvic {
	arm,num-irq-priority-bits = <3>;
};

Note that this is the actual and only property we need regarding NVIC - the priority bits; no other NVIC information is passed from DTS to C code! And that one is supplied at SOC level.

So, I'd like to see MPU following the same style: i.e. seen as a "general" feature of Cortex-M CPUs, defined in the ARCH level. But the actual and only property we need regarding MPU - the number of regions - is defined at SOC level if applicable and if required, and passed to C code.

Note that this is also the case with the systick DTS node: it is defined in the arm_.dtsi while it is clearly, an optional peripheral for M0, M0+ and M23.

@galak
Copy link
Collaborator

galak commented May 13, 2019

And we should delete systick from any SoC that doesn't have it.

So we should either add the node per SoC, or delete it from the SoC's that dont have MPUs (if we add it at the arm*.dtsi level).

@ioannisg ioannisg added the Enhancement Changes/Updates/Additions to existing features label May 13, 2019
@ioannisg
Copy link
Member Author

And we should delete systick from any SoC that doesn't have it.

So we should either add the node per SoC, or delete it from the SoC's that dont have MPUs (if we add it at the arm*.dtsi level).

Alright. Makes more sense to add the node per SoC, since it is all optional, anyways. The node is meant for Cortex-M23,33,7 where the number of regions is impl-defined (Cortex-M0+, M3, and M4 have 8 regions, fixed).

Will delete the SysTick from v6-M SoCs that don't have it.

ioannisg added 2 commits May 14, 2019 10:12
Add DTS binding files for the ARM MPU, for both ARM
MPU architecture variants, ARMv7-M and ARMv8-M.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Make _get_num_regions() return a constant representing the
number of HW MPU regions, defined in DTS, if such define
is available. This removes the need of evaluating the
number of regions at run-time. The ASSERT in arm_mpu_init()
is expanded, to cover that case, where the number of
regions is taken from DTS.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg ioannisg force-pushed the arch_arm_mpu_regions_indts branch from 82a3a7f to cd4cc7b Compare May 14, 2019 08:12
@ioannisg ioannisg requested a review from erwango as a code owner May 14, 2019 08:12
ioannisg added 4 commits May 14, 2019 10:16
This commit adds a DTS node for the ARM MPU peripheral in the
device tree of ARMv8-M SoCs (for the secure and the non-secure
DTS descriptions) and updates the fixup files. SoCs:
- nrf9160
- musca_a
- musca_b1

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Unlike Cortex-M3 and Cortex-M4, in Cortex-M7 the number of
MPU regions may vary based on the implementation. This commit
adds a DTS node for the ARM MPU peripheral in the device tree
of Cortex-M7 SoCs and updates the fixup files, so we may extract
the number of MPU regions at build time. SoCs:
- nxp_rt
- same70
- stm32f7

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Atmel SAM D series SoC variants (present in the tree) all have
an ARM Cortex-M0+ core, not a Cortex-M0, so we correct this in
the .dtsi header.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
ARM SysTick peripheral is optional in Cortex-M0 MCUs,
so we delete the respective dts node when the peripheral
is not present.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Devicetree area: Memory Protection area: optimization Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants