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

MP1 support #283

Merged
merged 20 commits into from
Feb 24, 2022
Merged

MP1 support #283

merged 20 commits into from
Feb 24, 2022

Conversation

Inujel
Copy link
Contributor

@Inujel Inujel commented Feb 4, 2022

This is still very much a work in progress.

I create the PR because I'd like to discuss a few points before going further 🙂.

Could you please give me some feedback about the comments below?
Thank you very much!

Repository owner deleted a comment from Inujel Feb 4, 2022
Repository owner deleted a comment from Inujel Feb 4, 2022
cmake/FindCMSIS.cmake Outdated Show resolved Hide resolved
Repository owner deleted a comment from Inujel Feb 4, 2022
@Inujel
Copy link
Contributor Author

Inujel commented Feb 9, 2022

It looks like for the MP1, CMSIS is dependant on HAL : system_stm32mp1xx.c includes stm32mp1xx_hal.h

CMSIS cannot be used without HAL for the MP1. I don't think that stm32-cmake should do anything special about it.
The CMSIS test cannot succeed. I remove MP1 from the default list and add a warning about it.

if(NOT TEST_FAMILIES)
    set(TEST_FAMILIES F0 F1 F2 F3 F4 F7 G0 G4 H7 L0 L1 L4 L5 U5 WB WL)
endif()

if("MP1" IN_LIST TEST_FAMILIES)
    message(WARNING "CMSIS for MP1 devices requires HAL, this test is expected to fail for the MP1 family")
endif()

@Inujel
Copy link
Contributor Author

Inujel commented Feb 11, 2022

Hi @atsju

It's ready to be reviewed.

cmake/FindBSP.cmake Show resolved Hide resolved
cmake/FindCMSIS.cmake Outdated Show resolved Hide resolved
Comment on lines 235 to 238
# There are stm32mp15?axx.s and stm32mp15?cxx.s files but no stm32mp15?dxx.s nor stm32mp15?fxx.s. No idea why.
# I think that stm32mp15xx.s should be compatible with all stm32mp15 devices anyway.
# This might need refinement if devices other stm32mp15 are released.
list(APPEND STARTUP_NAMES startup_stm32mp15xx.s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you did not manage it like described in our previous discussion. I think you did not get how MP1.cmake is supposed to work (I know it's complex as I added several families in the past). F3.cmake might be my best guess to look at. And look at how stm32_get_chip_type is working

Let me know if you want to dig or if you want me to have a look. It's not easy for me either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I remember changing it. I guess I screwed up something when committing 😅 .
This is fixed.

I admit that I did not understand everything about types and families.
I will look into stm32_get_chip_type, but can you tell me what feels wrong to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

as discussed previously the correct startup could be used instead the generic one.

@@ -205,6 +210,8 @@ function(stm32_get_memory_info)
set(FLASH "32K")
elseif(SIZE_CODE STREQUAL "8")
set(FLASH "64K")
elseif(SIZE_CODE STREQUAL "A")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not completely correct. all size codes are incorrect for MP1 family. It must be managed explicitly and not just removing a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no flash on the MP1 family. The only size code is "A" and I though that's what it meant. Don't you agree?
I did not put that there just to remove a warning.

To me, this part just don't apply for the MP1 family.

Copy link
Collaborator

Choose a reason for hiding this comment

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

in STM32G473RE the E is the size code.
Please also have a look at datasheet. From my point of view, MP1 has no such thing as a size_code.
maybe you should do same as L1 family (see stm32l1_get_memory_info) to return always 0.
All these information makes me think a modification is needed.
image

cmake/FindCMSIS.cmake Outdated Show resolved Hide resolved
cmake/stm32/mp1.cmake Outdated Show resolved Hide resolved
cmake/stm32/utilities.cmake Outdated Show resolved Hide resolved
tests/fetch/CMakeLists.txt Show resolved Hide resolved
tests/cmsis/CMakeLists.txt Show resolved Hide resolved
cmake/FindCMSIS.cmake Outdated Show resolved Hide resolved
@atsju
Copy link
Collaborator

atsju commented Feb 19, 2022

We are almost done. Seems very promising.

@Inujel
Copy link
Contributor Author

Inujel commented Feb 21, 2022

I added BSP support for the discovery and evaluation boards. However the bsp test currently fails.
I think it's the case for all multicore MCUs because the test target depends on BSP::STM32::${BOARD} instead of BSP::STM32::${BOARD}${CORE_C}.

CMake Error at CMakeLists.txt:59 (add_executable):
  Target "bsp-test-STM32WB15CC_Nucleo" links to target
  "BSP::STM32::STM32WB15CC_Nucleo" but the target was not found.  Perhaps a
  find_package() call is missing for an IMPORTED target, or an ALIAS target
  is missing?

I propose we fix this in a separate PR.

@atsju
Copy link
Collaborator

atsju commented Feb 21, 2022

I propose we fix this in a separate PR.

Please just clean the "size code" thing and fix the startup script.
For the BSP test we can indeed wait a bit more.

@Inujel
Copy link
Contributor Author

Inujel commented Feb 21, 2022

There's still something wrong in the fetch test but I can't reproduce it on my machine. I'm searching...

Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

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

For me this is good to merge. If you feel like BSP will be fixed later then remove it from the tests so that it doesn't fail .
I let you fix the fetch also.

@Hish15 we will need your review for this

After merge will open an issue for BSP test not working and CMSIS test that needs ST correction.

cmake/stm32/devices.cmake Outdated Show resolved Hide resolved
cmake/stm32/mp1.cmake Outdated Show resolved Hide resolved
cmake/FindHAL.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@Hish15 Hish15 left a comment

Choose a reason for hiding this comment

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

Good job!

@atsju
Copy link
Collaborator

atsju commented Feb 24, 2022

Yes well done! This was not an easy task and you were not the first trying out.
Also you asked good questions and had excellent proposals. Thank you.

@atsju atsju merged commit 293cef4 into ObKo:master Feb 24, 2022
This was referenced Feb 24, 2022
@Inujel
Copy link
Contributor Author

Inujel commented Feb 24, 2022

Thanks for your kind words!

@Inujel Inujel deleted the jj-mp1 branch February 24, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants