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

chore(CMSIS,Examples,PeriphDrivers): Sync latest ME30 changes #1075

Open
wants to merge 213 commits into
base: main
Choose a base branch
from

Conversation

sihyung-maxim
Copy link
Contributor

Description

It has been awhile since the last time the feat/ME30 branch was synced up to main (May). Most people should be aware that the feat/ME30 contains the latest files, and the ME30 files in main have been out of date.

Some highlights:

  • Add I3C, Boost, RSTZ, Frequency Counter, MPC, SPC, and NSPC registers.
  • Add trustzone support to build system: memory definitions are done by build system.
  • Initial commit for MPC, SPC, and NSPC drivers (needs more testing).
  • CMSIS fixes and changes.

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • (Optional) Link any related GitHub issues using a keyword
  • (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

Jake-Carter and others added 30 commits April 18, 2024 16:03
This commit enable MAX32657 zephyr support.

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
Co-authored-by: lorne-maxim <lorne-maxim@users.noreply.github.com>
…675 (#1001)

Co-authored-by: lorne-maxim <lorne-maxim@users.noreply.github.com>
Co-authored-by: Sihyung Woo <75494566+sihyung-maxim@users.noreply.github.com>
These files are exact copy of MAX32655
Files will be updated in next sections, added here
to demonstrate delta clearly on next steps

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
These files are exact copy of MAX32655
Files will be updated in next sections, added here
to demonstrate delta clearly on next steps

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
feat(PeriphDrivers): Add max32657 basic files
@@ -22,6 +22,12 @@ if(NOT TARGET_REV)
zephyr_compile_definitions(-DTARGET_REV=0x4131)
endif()

if(CONFIG_TRUSTED_EXECUTION_SECURE)
zephyr_compile_definitions(-DIS_SECURE_ENVIRONMENT=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not essential, just a suggestion,
How about to align msdk and zepyhr flags,
On zephyr side CONFIG_TRUSTED_EXECUTION_SECURE" and CONFIG_TRUSTED_EXECUTION_NONSECURE is used to generate secure, non-secure image.

If you replace IS_SECURE_ENVIRONMENT with CONFIG_TRUSTED_EXECUTION_SECURE in msdk files. MSDK will be align with zephyr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since many members of the roadtest and verification teams are used to the current flags, I'll still leave IS_SECURE_ENVIRONMENT defined, but I'll use the CONFIG_TRUSTED_EXECUTION_* going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, in that case I can remove conversion in Libraries/zephyr/MAX/Source/MAX32657/CMakeLists.txt.
Let me I apply some test with final status of this branch on zephyr side.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's removed.

sihyung-maxim and others added 5 commits July 9, 2024 10:08
Since all timers are 32bit, max32657 timer wrapper is updated
accordingly.

Signed-off-by: Anıl Kara <anil.kara@analog.com>
MAX32657 UART interrupt enable register named as inten

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
@ozersa
Copy link
Contributor

ozersa commented Jul 10, 2024

MXC_INFO_MEM_BASE not accessible for non-secure mode, so this function shall be enabled only for secure mode?

image

ozersa and others added 5 commits July 10, 2024 13:57
MSDK team start to use CONFIG_TRUSTED_EXECUTION_SECURE flag instead of
IS_SECURE_ENVIRONMENT

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
@sihyung-maxim
Copy link
Contributor Author

MXC_INFO_MEM_BASE not accessible for non-secure mode, so this function shall be enabled only for secure mode?

image

Yep, nice catch. It's now restricted for secure builds only.

@sihyung-maxim
Copy link
Contributor Author

@ozersa Also not sure how you want to handle the merge conflict. (I'm assuming the current file in the branch is what we want merged in, but I will let you confirm).

# Generate an object file with empty definitions of the secure image symbols at the correct locations.
# The object file needs to be linked with the non-secure image.
ifeq ($(GEN_CMSE_IMPLIB_OBJ),1)
PROJ_LDFLAGS := -Xlinker --cmse-implib -Xlinker --out-implib=$(CURDIR)/build/build_s/secure_implib.o
Copy link
Contributor

Choose a reason for hiding this comment

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

Use += instead of := so you don't erase previous values in PROJ_LDFLAGS here


################################################################################
#
# If 'USE_CUSTOM_MEMORY_SETTINGS = 1':
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest an actual conditional here

ifeq "$(USE_CUSTOM_MEMORY_SETTINGS)" "1"
# ...
endif

ifeq ($(USE_CUSTOM_MEMORY_SETTINGS),1)

ifeq "$(MSECURITY_MODE)" "SECURE"
LINKERFILE=$(abspath ${SECURE_CODE_DIR})/$(TARGET_LC)_s.ld
Copy link
Contributor

Choose a reason for hiding this comment

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

If custom memory settings are used the linkerfile needs to be local inside the project?

ifeq "$(MSECURITY_MODE)" "SECURE"
LINKERFILE=$(CMSIS_ROOT)/Device/Maxim/$(TARGET_UC)/Source/GCC/$(TARGET_LC)_s.ld
else # MSECURITY_MODE=NONSECURE
LINKERFILE=$(CMSIS_ROOT)/Device/Maxim/$(TARGET_UC)/Source/GCC/$(TARGET_LC)_ns.ld
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest defining all LINKERFILEs with ?= so that user overrides still behave as expected

# Project-owner must set these variables in project.mk
# Absolute paths will be used when these are used.
NONSECURE_CODE_DIR ?= $(CMSIS_ROOT)/../../Examples/$(TARGET_UC)/Hello_World_TZ/NonSecure
SECURE_CODE_DIR ?= $(CMSIS_ROOT)/../../Examples/$(TARGET_UC)/Hello_World_TZ/Secure
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest defining these defaults as

NONSECURE_CODE_DIR ?= $(abspath $(dir $(PROJECTMK))/NonSecure)
SECURE_CODE_DIR ?= $(abspath $(dir $(PROJECTMK)))

which will set them relative to the project.mk file, and makes the secure project the "root" project.

Hello_World_TZ        
    .vscode
    main.c
    project.mk
    Makefile
    /NonSecure
        .vscode
        main.c
        Makefile
        project.mk

It's a pain to develop in two island folders, especially if one of them isn't intended to be built by itself. Nested is more friendly

endif # USE_CUSTOM_MEMORY_SETTINGS

# Prepare definitions to store in PROJ_CFLAGS.
S_FLASH_START := $(shell printf "0x%x" $(S_FLASH_START))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for calls to printf. We can't directly depend on OS-specific utilities unless you want to wrap inside of an OS-enforced boundary which I would leave as a very last resort.


# Should only be run from the host "Secure" project
.PHONY: debug_tz_mem_from_host_proj
debug_tz_mem_from_host_proj:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap this rule definition in a check for the right variable definitions to enforce it

# Add more if needed.
}

def string_to_integer_bytes(string_hex_or_units):
Copy link
Contributor

Choose a reason for hiding this comment

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

I can kind of guess what the input to this function is but a comment explaining what it does would be helpful

program_description = '''Generates the linker scripts and partiton_{device}.h with
project-defined memory settings.

NOTE: This script uses binary prefix notation for units when not used by build system
Copy link
Contributor

@Jake-Carter Jake-Carter Jul 16, 2024

Choose a reason for hiding this comment

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

Confused about what this means

NOTE: This script uses binary prefix notation for units when not used by build system
For example: Kilobytes (KB) will be treated as Kibibytes (KiB).

Note: All these arguments will be set with default values according to the {device}.mk file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only because the build system calls this script though, right? This could be misleading since it doesn't parse the makefile

@Jake-Carter
Copy link
Contributor

Build errors. Auto-tester is not triggering on this for some reason.

- CC /home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c: In function 'MXC_FLC_GetWELR':
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c:266:25: error: 'mxc_flc_regs_t' has no member named 'welr0'
  266 |         return &(MXC_FLC->welr0);
      |                         ^~
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c:268:25: error: 'mxc_flc_regs_t' has no member named 'welr1'
  268 |         return &(MXC_FLC->welr1);
      |                         ^~
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c: In function 'MXC_FLC_GetRLR':
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c:286:25: error: 'mxc_flc_regs_t' has no member named 'rlr0'
  286 |         return &(MXC_FLC->rlr0);
      |                         ^~
/home/jhcarter/repos/msdk/Libraries/CMSIS/../PeriphDrivers/Source/FLC/flc_me30.c:288:25: error: 'mxc_flc_regs_t' has no member named 'rlr1'
  288 |         return &(MXC_FLC->rlr1);
      |                         ^~
make[1]: *** [/home/jhcarter/repos/msdk/Libraries/CMSIS/Device/Maxim/GCC/gcc.mk:538: /home/jhcarter/repos/msdk/Libraries/PeriphDrivers/bin/MAX32657/spi-v1_softfp/flc_me30.o] Error 1
make[1]: Leaving directory '/home/jhcarter/repos/msdk/Examples/MAX32657/Hello_World_TZ/Secure'
make: *** [/home/jhcarter/repos/msdk/Libraries/PeriphDrivers/periphdriver.mk:102: /home/jhcarter/repos/msdk/Libraries/PeriphDrivers/bin/MAX32657/spi-v1_softfp/libPeriphDriver_spi-v1_softfp.a] Error 2

Comment on lines 63 to +66
{
if ((addr >= MXC_FLASH_MEM_BASE) && (addr < (MXC_FLASH_MEM_BASE + MXC_FLASH_MEM_SIZE))) {
*result = addr & (MXC_FLASH_MEM_SIZE - 1);
#if CONFIG_TRUSTED_EXECUTION_SECURE
Copy link
Contributor

Choose a reason for hiding this comment

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

Flash controller only available for secure mode, flc_me30.c functions shall handle this.

These parameters require to be defined in bare metal drivers

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
@@ -405,10 +407,12 @@ int MXC_FLC_RevA_ClearFlags(uint32_t mask)
//******************************************************************************
int MXC_FLC_RevA_UnlockInfoBlock(mxc_flc_reva_regs_t *flc, uint32_t address)
{
#if !defined(CONFIG_TRUSTED_EXECUTION_SECURE) || (CONFIG_TRUSTED_EXECUTION_SECURE != 0) || (TARGET_NUM != 32657)
Copy link
Contributor

Choose a reason for hiding this comment

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

Exclamation mark need to be removed.

The logic might be simplified too.

@@ -424,10 +428,12 @@ int MXC_FLC_RevA_UnlockInfoBlock(mxc_flc_reva_regs_t *flc, uint32_t address)
//******************************************************************************
int MXC_FLC_RevA_LockInfoBlock(mxc_flc_reva_regs_t *flc, uint32_t address)
{
#if !defined(CONFIG_TRUSTED_EXECUTION_SECURE) || (CONFIG_TRUSTED_EXECUTION_SECURE != 0) || (TARGET_NUM != 32657)
Copy link
Contributor

Choose a reason for hiding this comment

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

Exclamation mark need to be removed.

The logic might be simplified too.

Copy link
Contributor

@ozersa ozersa left a comment

Choose a reason for hiding this comment

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

Exclamation mark need to be removed.

Remove application flash section restriction from flash driver
Some section of flash can be used for storage purpose

Signed-off-by: Sadik Ozer <sadik.ozer@analog.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Register Change This issue or pull request involves a change to the MSDK registers. Tools This issue or pull request involves a change to the Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants