soc: arm: stm32: Updated SRAM Region Definitions #14600

Hobbs1210 · 2019-03-16T05:01:20Z

Support for STM32F7XX MCU's that have 512k of SRAM.

zephyrbot · 2019-03-16T05:02:22Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

codecov-io · 2019-03-16T05:53:34Z

Codecov Report

Merging #14600 into master will decrease coverage by 0.96%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #14600      +/-   ##
==========================================
- Coverage   52.93%   51.96%   -0.97%     
==========================================
  Files         309      309              
  Lines       45251    45576     +325     
  Branches    10447    10554     +107     
==========================================
- Hits        23953    23683     -270     
- Misses      16533    17083     +550     
- Partials     4765     4810      +45

Impacted Files	Coverage Δ
subsys/testsuite/ztest/include/ztest_assert.h	`38.88% <0%> (-38.89%)`	⬇️
subsys/net/ip/net_pkt.c	`44.92% <0%> (-26.51%)`	⬇️
subsys/net/ip/udp.c	`47.66% <0%> (-21.79%)`	⬇️
include/spinlock.h	`57.14% <0%> (-17.86%)`	⬇️
drivers/net/loopback.c	`77.27% <0%> (-13.64%)`	⬇️
kernel/mutex.c	`83.82% <0%> (-13.28%)`	⬇️
subsys/net/ip/ipv4.c	`51.9% <0%> (-12.96%)`	⬇️
subsys/net/ip/net_context.c	`49.41% <0%> (-9.47%)`	⬇️
kernel/timeout.c	`75.92% <0%> (-9.26%)`	⬇️
kernel/include/kernel_structs.h	`90.9% <0%> (-9.1%)`	⬇️
... and 60 more

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 cf91f8c...3dafacd. Read the comment docs.

soc/arm/st_stm32/common/arm_mpu_mem_cfg.h

ioannisg

Commit title prefix should, probably be: "soc: arm: stm32:"
It's not really an ARM architecture patch

ioannisg · 2019-03-16T11:43:17Z

adding this to 1.15, unless it is a bug affecting current boards/SOCs. If so, please indicate and re-scope.

ioannisg

So, the patch is now OK to me, however, the commit history looks confusing.
Could you please clean this up? I.e. commit this diff in a single commit, and force-push to this pull-request, so the PR is a single commit. Refer to the Zephyr contribution guidelines if needed :)

Then we can approve the PR and add it to the queue for merging to master.

erwango

As requested by @ioannisg, please restructure your change into a single commit and fix commit message issues reported by @zephyrbot

Hobbs1210 · 2019-03-24T02:31:03Z

Fixed per request

psidhu · 2019-03-25T16:12:13Z

@erwango might know better than me, but shouldn't you split this up into two REGION_256K's?

For example, even the 128k support uses two REGION_64K's to split the RAM up instead of a single REGION_128K.

ioannisg · 2019-03-25T18:57:09Z

@erwango might know better than me, but shouldn't you split this up into two REGION_256K's?

For example, even the 128k support uses two REGION_64K's to split the RAM up instead of a single REGION_128K.

Yes, it looks like this file needs rework.
So, the rule should be the following:
SRAM sizes that are power-of-two shall use a single SRAM MPU region (Region 0).
If not a power-of-two, we shall use as few MPU regions as possible.
@Hobbs1210 could you please, enhance this PR so it fixes all issues?

Hobbs1210 · 2019-03-25T23:24:00Z

@erwango might know better than me, but shouldn't you split this up into two REGION_256K's?
For example, even the 128k support uses two REGION_64K's to split the RAM up instead of a single REGION_128K.

Yes, it looks like this file needs rework.
So, the rule should be the following:
SRAM sizes that are power-of-two shall use a single SRAM MPU region (Region 0).
If not a power-of-two, we shall use as few MPU regions as possible.
@Hobbs1210 could you please, enhance this PR so it fixes all issues?

@ioannisg please review the latest push. please let me know if there are any further changes required.

erwango · 2019-03-26T14:02:20Z

@Hobbs1210, you'll need to also fix soc/arm/st_stm32/common/arm_mpu_regions.c which was implemented on the assumption there would always be 2 RAM zones.

Btw, this was implemented this way to have same number of available zones, whether RAM size is a power of 2 or not. This indeed can be optimized and save one MPU region when size is directly a power of 2. Which leaves one region free in this case.
@ioannisg, is there anything we can do with this available MPU region?

Hobbs1210 · 2019-03-30T00:00:16Z

@Hobbs1210, you'll need to also fix soc/arm/st_stm32/common/arm_mpu_regions.c which was implemented on the assumption there would always be 2 RAM zones.

Btw, this was implemented this way to have same number of available zones, whether RAM size is a power of 2 or not. This indeed can be optimized and save one MPU region when size is directly a power of 2. Which leaves one region free in this case.
@ioannisg, is there anything we can do with this available MPU region?

@erwango please review my latest push.

erwango

Please add a one liner comment in commit message to get git check happy

ioannisg · 2019-04-11T14:22:52Z

@ioannisg, is there anything we can do with this available MPU region?

Yes, we can use it for application memory partitions. The more we have the better.

erwango · 2019-04-11T14:51:20Z

Yes, we can use it for application memory partitions. The more we have the better.

What bothers me is you'll have one extra partition depending on the memory size, so a 256K config will enjoy one more partition than a 384K..

This updates the SRAM region definition for stm32 Signed-off-by: Habib Zaid <[email protected]>

ioannisg

thanks

Hobbs1210 requested a review from erwango as a code owner March 16, 2019 05:01

ioannisg suggested changes Mar 16, 2019

View reviewed changes

soc/arm/st_stm32/common/arm_mpu_mem_cfg.h Outdated Show resolved Hide resolved

ioannisg reviewed Mar 16, 2019

View reviewed changes

ioannisg added this to the v1.15.0 milestone Mar 16, 2019

Hobbs1210 changed the title ~~arch: arm: mpu: add support for 512k SRAM~~ soc: arm: stm32: add support for 512k SRAM Mar 16, 2019

aescolar added the platform: STM32 ST Micro STM32 label Mar 17, 2019

ioannisg reviewed Mar 18, 2019

View reviewed changes

ioannisg assigned erwango Mar 18, 2019

erwango requested changes Mar 18, 2019

View reviewed changes

Hobbs1210 force-pushed the 512k-SRAM branch from d4a2c69 to 39a8c3e Compare March 24, 2019 01:19

Hobbs1210 force-pushed the 512k-SRAM branch from 39a8c3e to e029860 Compare March 25, 2019 23:20

Hobbs1210 force-pushed the 512k-SRAM branch from e029860 to 3dafacd Compare March 28, 2019 00:13

erwango requested changes Apr 11, 2019

View reviewed changes

Hobbs1210 force-pushed the 512k-SRAM branch from 3dafacd to f75f8a8 Compare April 14, 2019 22:37

Hobbs1210 changed the title ~~soc: arm: stm32: add support for 512k SRAM~~ soc: arm: stm32: Updated SRAM Region Definitions Apr 14, 2019

Hobbs1210 force-pushed the 512k-SRAM branch from f75f8a8 to 221f53e Compare April 15, 2019 00:02

soc: arm: stm32: Updated SRAM Region Definitions

57cc05b

This updates the SRAM region definition for stm32 Signed-off-by: Habib Zaid <[email protected]>

Hobbs1210 force-pushed the 512k-SRAM branch from 221f53e to 57cc05b Compare April 15, 2019 00:34

erwango approved these changes Apr 24, 2019

View reviewed changes

ioannisg approved these changes Apr 24, 2019

View reviewed changes

ioannisg requested a review from galak April 24, 2019 16:06

galak merged commit 32367c0 into zephyrproject-rtos:master Apr 24, 2019

soc: arm: stm32: Updated SRAM Region Definitions #14600

soc: arm: stm32: Updated SRAM Region Definitions #14600

Uh oh!

Conversation

Hobbs1210 commented Mar 16, 2019

Uh oh!

zephyrbot commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg commented Mar 16, 2019

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

Hobbs1210 commented Mar 24, 2019

Uh oh!

psidhu commented Mar 25, 2019

Uh oh!

ioannisg commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hobbs1210 commented Mar 25, 2019

Uh oh!

erwango commented Mar 26, 2019

Uh oh!

Hobbs1210 commented Mar 30, 2019

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg commented Apr 11, 2019

Uh oh!

erwango commented Apr 11, 2019

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zephyrbot commented Mar 16, 2019 •

edited

Loading

codecov-io commented Mar 16, 2019 •

edited

Loading

ioannisg commented Mar 25, 2019 •

edited

Loading