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

STM32: Add support for STM32H7 dual core #17100

Merged
merged 17 commits into from
Jul 4, 2019

Conversation

erwango
Copy link
Member

@erwango erwango commented Jun 26, 2019

Enable support of Zephyr on STM32H747I-DISCO board.
This board is dual core and this work enable running zephyr in both cores.

For now, the minimum work is done to enable dual core operation by sharing the resources.
No communication is enabled between cores except use of hardware semaphore for synchronization
and critical resources access.

Depends on zephyrproject-rtos/hal_stm32#15

@erwango erwango requested a review from nashif June 26, 2019 16:01
@erwango erwango added the platform: STM32 ST Micro STM32 label Jun 26, 2019
@erwango erwango requested a review from galak June 26, 2019 16:03
@erwango
Copy link
Member Author

erwango commented Jun 26, 2019

@arnop2

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jun 26, 2019

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.

@erwango erwango force-pushed the dev_stm32h7_basic_modules branch 2 times, most recently from a91284b to f3e3890 Compare June 27, 2019 09:35
@zephyrbot zephyrbot added the area: Samples Samples label Jun 27, 2019
@erwango
Copy link
Member Author

erwango commented Jun 27, 2019

stm32h747i_disco_m4 doesn't enable CONFIG_SERIAL.

How come samples/nfc/nfc_hello (filters on CONFIG_SERIAL) and samples/mpu/mpu_test (filters on CONFIG_SERIAL_SUPPORT_INTERRUPT) are compiled on that target ?

EDIT: While target does not enable CONFIG_SERIAL, is it actually enabled in samples .config file when building (then build fails because no uart device is enabled).

EDIT: Adding following in target .yaml, but notsure it's the right solution:

testing:
  ignore_tags:
    - mpu
    - nfc

@erwango erwango force-pushed the dev_stm32h7_basic_modules branch from f3e3890 to dbcea37 Compare June 27, 2019 13:59
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

some recommended doc changes


The discovery kit enables a wide diversity of applications taking benefit
from audio, multi-sensor support, graphics, security, security, video,
and high-speed connectivity features. Important board features include:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and high-speed connectivity features. Important board features include:
and high-speed connectivity features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

from audio, multi-sensor support, graphics, security, security, video,
and high-speed connectivity features. Important board features include:

It embeds STM32H747XI SoC: a high-performance and DSP with DP-FPU, Arm Cortex-M7 + Cortex-M4 MCU,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It embeds STM32H747XI SoC: a high-performance and DSP with DP-FPU, Arm Cortex-M7 + Cortex-M4 MCU,
The board includes an STM32H747XI SoC with a high-performance DSP, DP-FPU, Arm Cortex-M7 + Cortex-M4 MCU,

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


It embeds STM32H747XI SoC: a high-performance and DSP with DP-FPU, Arm Cortex-M7 + Cortex-M4 MCU,
with 2MBytes of Flash memory, 1MB RAM, 480 MHz CPU, Art Accelerator, L1 cache, external memory interface,
large set of peripherals, SMPS, MIPI-DSI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
large set of peripherals, SMPS, MIPI-DSI.
large set of peripherals, SMPS, and MIPI-DSI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Other hardware features are not yet supported on Zephyr porting.

The default configuration per core can be found in the defconfig files:
``boards/arm/stm32h747i_disco/stm32h747i_disco_defconfig_m7``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``boards/arm/stm32h747i_disco/stm32h747i_disco_defconfig_m7``
``boards/arm/stm32h747i_disco/stm32h747i_disco_defconfig_m7`` and

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


The STM32H747I Discovery kit has up to 8 UARTs.
Default configuration assigns USART1 and UART8 to the CPU1. The Zephyr console
output is assigned to UART1 which connected to the onboard ST-LINK/V. Virtual
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the right fix is here, but should this be ST-LINK/Virtual

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be ST-LINK/V3.0. Txs

- Compilation: Clock configuration is only accessible to M7 core. M4 core only
have access to bus clock activation, deactivation
- Static pre-compilation assignment: Peripherals such as UART are assigned in device
tree before compilation. it is up to the user to ensure same peripheral is not
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tree before compilation. it is up to the user to ensure same peripheral is not
tree before compilation. The user must ensure peripherals are not

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

have access to bus clock activation, deactivation
- Static pre-compilation assignment: Peripherals such as UART are assigned in device
tree before compilation. it is up to the user to ensure same peripheral is not
assigned to both cores at the same time
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assigned to both cores at the same time
assigned to both cores at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

option bytes configuration.
It is advised to use `STM32CubeProgrammer`_ to check and update option bytes
configuration and flash ``stm32h747i_disco_m7`` and ``stm32h747i_disco_m4`` targets.
By default:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd start a new paragraph here by adding a blank line before By default:

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@erwango erwango force-pushed the dev_stm32h7_basic_modules branch 2 times, most recently from 1ea437a to daa2e79 Compare June 28, 2019 07:26
@erwango erwango requested a review from dbkinder June 28, 2019 08:54
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/clock_control/clock_stm32_ll_h7.c Outdated Show resolved Hide resolved
soc/arm/st_stm32/stm32h7/Kconfig.defconfig.stm32h747xx Outdated Show resolved Hide resolved
soc/arm/st_stm32/stm32h7/soc.h Outdated Show resolved Hide resolved
/*HW semaphore Clock enable*/
LL_AHB4_GRP1_EnableClock(LL_AHB4_GRP1_PERIPH_HSEM);

#if defined(CONFIG_STM32H7_BOOT_CM4_CM7)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (IS_ENABLED(CONFIG_STM32H7_BOOT_CM4_CM7)) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was my first try, but then, I need to define stm32h7_m4_boot_stop also when CONFIG_STM32H7_BOOT_CM4_CM7 is not enabled, so I preferred to keep it this way

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for me

drivers/clock_control/clock_stm32_ll_h7.c Show resolved Hide resolved
drivers/clock_control/clock_stm32_ll_h7.c Show resolved Hide resolved
if UART_CONSOLE

config UART_1
default y if BOARD_STM32H747I_DISCO_M7
Copy link
Collaborator

Choose a reason for hiding this comment

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

As peripherals are shared between cortex contexts i would set configs to n by default for peripheral to ensure that is not used by M4 and M7

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is that I need one UART defined on one side to enable serial testing (and things that require serial).

But, I agree with your concern and this is actually why I preferred to put devices activation in this file common to both cores rather than in _m{4.7}_defconfig files.
What I can add is the following to make the intention clearer (won't add nothing in term of "mechanical" check though :

config UART_1
	default y if BOARD_STM32H747I_DISCO_M7
	default n if BOARD_STM32H747I_DISCO_M4

And the same should be done for each other peripheral device.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for me

@erwango erwango force-pushed the dev_stm32h7_basic_modules branch 2 times, most recently from ec26e70 to ce62b3a Compare July 1, 2019 15:05
/*HW semaphore Clock enable*/
LL_AHB4_GRP1_EnableClock(LL_AHB4_GRP1_PERIPH_HSEM);

#if defined(CONFIG_STM32H7_BOOT_CM4_CM7)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for me

if UART_CONSOLE

config UART_1
default y if BOARD_STM32H747I_DISCO_M7
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for me

boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/stm32h747i_disco/doc/index.rst Outdated Show resolved Hide resolved
@erwango erwango force-pushed the dev_stm32h7_basic_modules branch from ce62b3a to 0fb1ccf Compare July 2, 2019 08:58
In two stm32 drivers, fix leading spaces warning.


Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
@erwango erwango force-pushed the dev_stm32h7_basic_modules branch from 0fb1ccf to 2765e93 Compare July 2, 2019 09:01
@erwango erwango requested a review from dbkinder July 2, 2019 11:16
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

doc changes LGTM, thanks!

erwango added 16 commits July 3, 2019 09:09
Now STM32H7 are available in Zephyr.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Following introduction of STM32H7 series, update STM32 module
Kconfig with stm32cube H7 additions.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Enable basic support to STM32H7, in single core configuration (M7).

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Initiate stm32h7 device tree description, with stm32h747 single core
configuration.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Provide basic clock control driver for STM32H7.
Bus clock activation is done through CM7 and CM4 common registers
so we don't have to care to the CPU Id before accessing.
Accesses are not protected for now. Only possible configuration
is system clock source set to HSE driven PLL.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add bare minimum to enable EXTI on STM32H7,
in single core configuration.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add GPIO support on STM32H7.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add needful to enable uart on STM32H7.
This mostly impact dts but as well soc for fixup.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add UART headers for STM32H7 series

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add C-M7 target for board stm32h747i_disco.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add support for C-M4 core on STM32H7 series.
It is enabled in Dual core context with 2 alternatives boot methods:
* Boot CM4 CM7: Both core boot at reset, then CM4 enters Stop mode.
CM7 performs system configuration then finally wakes up CM4
* Boot CM7, CM4 Gated: Only CM7 boots at reset. Once done with
system configuration it triggers (requires option byte update)

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
This change adds sram1 plus m4 core package level dtsi.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
On STM32H7, in Dual core configuration, we restrict configuration
access to CM7 core. CM4 can access to API but not the init part of
the driver.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Protect gpio_configure function in dual core context.
This operation is not needed for other fuctions of the api:
* init
* read
* write

Protecting gpio_configure also protects access to
interrupt_controller IP.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add m4 target to stm32h747i_disco.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Provide doc for stm32h747i_disco.
Includes basic description for building and flashing
individual cores.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
@erwango erwango force-pushed the dev_stm32h7_basic_modules branch from 2765e93 to 0089f0c Compare July 3, 2019 07:10
config BOARD_STM32H747I_DISCO_M7
bool "STM32H747I Discovery Development Board"
depends on SOC_STM32H747XX
select CPU_CORTEX_M7
Copy link
Member

Choose a reason for hiding this comment

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

Weird that Board options select Cortex cores.
could this be done in the SOC level instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was my initial attempt, but at the end, this only introduces and intermediate SOC_STM32H747XX_M7, that selects CPU_CORTEX_M7.
When electing the cortex at this level, I can remove an intermediate step which makes things clearer. Still, we're able to do cortex specific settings in soc/arm/st_stm32/stm32h7/Kconfig.soc

config BOARD_STM32H747I_DISCO_M4
bool "STM32H747I Discovery Development Board"
depends on SOC_STM32H747XX
select CPU_CORTEX_M4
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Looks good, I only notice that CPU core options are selected at the board level.
That's not what we have done in the other multi-core SOCs (but I might be wrong).

I think it would be cleaner, if at the SOC level we have SOC options for the two cores, i.e. xxx_M4 and xxx_M7 and then we have two different BOARDs, anyways, because we need to build two different images.

@erwango
Copy link
Member Author

erwango commented Jul 3, 2019

@ioannisg

I think it would be cleaner, if at the SOC level we have SOC options for the two cores, i.e. xxx_M4 and xxx_M7

As answered above, this would introduce an intermediate symbol with no added value.
And I don't see limitation or risk with the current method

we have two different BOARDs, anyways, because we need to build two different images.

I understand you propose to introduce 2 boards directories.
I don't think this is the good direction: even if we have 2 targets to build, they share a lot of common configuration: boot type, clocks settings, boards support package, pinmux ..
Having 2 targets in the same board directory allow to share some Kconfig file which force configuration coherency.
Also, even if we miss a good tool to handle peripheral assignment, having all information at one place eases manual check that resources are assigned to only one core at the same time.

@ioannisg
Copy link
Member

ioannisg commented Jul 3, 2019

@erwango, got me a bit wrong, here, we shoudln't have 2 BOARD directories, we should have two BOARD .defconfigs (we do actually), as we do for other dual-cores.

So, I am only worried about consistency here. Check the 2 dual-core NXP lpc platforms; they have two SOC symbols defined, for each core, and the two BOARD symbols depend on either SOC Kconfig. I think the ARM Musca_x does the same. We should have one way to do this for any multi-core platform....

@erwango
Copy link
Member Author

erwango commented Jul 3, 2019

@ioannisg

@erwango, got me a bit wrong, here, we shoudln't have 2 BOARD directories, we should have two BOARD .defconfigs (we do actually), as we do for other dual-cores.

Ok. And we have 2 _defconfig, but only one .defconfig. This is the same than lpx as I can check. So we're aligned here.

So, I am only worried about consistency here. Check the 2 dual-core NXP lpc platforms; they have two SOC symbols defined, for each core, and the two BOARD symbols depend on either SOC Kconfig. I think the ARM Musca_x does the same. We should have one way to do this for any multi-core platform.

From what I see, ARM musca defines 2 boards, so does cypress_062_wifi_bt dual core board, let put these aside.
I see your point about consistency. But then, looking at lpc5Xxxx implementations, I don't particularly agree on the way things are done (specially in soc files Kconfig.defconfig.lpcxxxx_mx some kind of peripheral assignment is done, which limits the flexibility at board level and performs driver deactivation at soc level which seems error prone for an end user).
Anyway, I'm keeping your suggestion in mind and I'll see if solves issues when we'll have to add mode resources or mode boards/soc derivates based on this porting.

@galak
Copy link
Collaborator

galak commented Jul 3, 2019

From what I see, ARM musca defines 2 boards, so does cypress_062_wifi_bt dual core board, let put these aside.
I see your point about consistency. But then, looking at lpc5Xxxx implementations, I don't particularly agree on the way things are done (specially in soc files Kconfig.defconfig.lpcxxxx_mx some kind of peripheral assignment is done, which limits the flexibility at board level and performs driver deactivation at soc level which seems error prone for an end user).
Anyway, I'm keeping your suggestion in mind and I'll see if solves issues when we'll have to add mode resources or mode boards/soc derivates based on this porting.

Musca is 2 different actual boards. The cypress_062_wifi_bt should be reworked but we've never gotten around to it.

@nashif nashif merged commit 79e4d0a into zephyrproject-rtos:master Jul 4, 2019
@erwango erwango deleted the dev_stm32h7_basic_modules branch September 3, 2019 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants