Skip to content

DISCO_H747I dualcore support #11605

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

Merged
merged 4 commits into from
Oct 16, 2019

Conversation

ABOSTM
Copy link
Contributor

@ABOSTM ABOSTM commented Oct 1, 2019

Description

Add 2 targets for DISCO_H747I dualcore:

  • DISCO_H747I -> for CM7 core
  • DISCO_H747I_CM4 -> for CM4 core

Current restrictions:

  • TICKLESS deactivated
  • DeepSleep not supported (DeepSleep wrapped to sleep)

Warning: use of the same IP (example I2C1) by both core at the same time is not prevented,
but is strongly not recommended.
Some Hardware Semaphore are use for common IP, to manage concurrent access by both cores: Flash, GPIO, RCC.

Warning: Drag and drop of binary to DISCO_H747I disk will flash CM7.
In order to flash CM4, one can use STM32 CubeProgrammer tool.
Example of CubeProgrammer command line to flash CM4:
PATH="/c/Program Files/STMicroelectronics/STM32Cube/STM32CubeProgrammer/bin/":$PATH
STM32_Programmer_CLI -c port=SWD mode=UR -w BUILD/DISCO_H747I_CM4/ARM/<binary file name> 0x8100000

Tests executed:

  • On CM7 (with a blinky binary on CM4), all tests on all 3 toolchains: ARM, GCC_ARM and IAR
  • On CM4 (with a blinky binary on CM7), all tests on toolchain IAR and GCC_ARM
    With toolchain ARM: blinky on both core simultaneously

All tests are passed except:

  • tests-mbed_hal-sleep_manager
  • tests-mbed_hal-sleep

This is normal because DeepSleep is not yet supported.

@jeromecoutant
Copy link
Collaborator

@ARMmbed/team-st-mcd
@MarceloSalazar
@facchinm

Copy link
Collaborator

@jeromecoutant jeromecoutant left a comment

Choose a reason for hiding this comment

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

Regression tests on other targets done.

As explained in the description, CM4 support can not be executed with automatic CI,
but I saw manual tests executed and passed!

Thx @ABOSTM !

@ciarmcom ciarmcom requested review from a team October 1, 2019 09:00
@ciarmcom
Copy link
Member

ciarmcom commented Oct 1, 2019

@ABOSTM, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-crypto please review.

@ciarmcom ciarmcom requested a review from a team October 1, 2019 09:00
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this is sort of a basic support only? It's up to the user to put whatever code on both cores and figure out how do they talk to each other?

@@ -103,11 +103,19 @@ void mbedtls_aes_free(mbedtls_aes_context *ctx)
if (ctx == NULL) {
return;
}
#if defined(DUAL_CORE)
Copy link
Member

Choose a reason for hiding this comment

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

@Patater please review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right this is up to the user to put whatever on both cores, and to figure out how they could talk to each other.
The modification in this specific file aes_alt.c concerns the potential concurent access (by CM4 and CM7) to common resource (RCC registers). This is done through Hardware Semaphore.

{PA_2, PWM_2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 3, 0)}, // TIM2_CH3 // Connected to ETH_MDIO
{PA_2_ALT0, PWM_5, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF2_TIM5, 3, 0)}, // TIM5_CH3 // Connected to ETH_MDIO
{PA_2_ALT1, PWM_15, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF4_TIM15, 1, 0)}, // TIM15_CH1 // Connected to ETH_MDIO
// {PA_0, PWM_2, STM_PIN_DATA_EXT(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF1_TIM2, 1, 0)}, // TIM2_CH1 // Connected to PMOD\#1- USART2_CTS_NSS
Copy link
Member

Choose a reason for hiding this comment

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

Dead code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes and no...
We used to keep all the lines in all targets .
As the PinMap table is weak, idea was to give the availability for customer to change easily the default choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From pin configuration point of view, PA_0 can be connected to TIM2_CH1.
Nevertheless TIM2 and TIM5 are reserved for us_ticker.
That is why thoses lines are commented out: it is nomore usable for PWM purpose.
but we keep those lines as reminder to show the capability.
It is done the same way for example on target NUCLEO_L4R5ZI_P.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

Would it benefit from a readme file in the target directory with description how to build and flash both images?

@jeromecoutant
Copy link
Collaborator

Would it benefit from a readme file in the target directory with description how to build and flash both images?

Good idea, I will do it asap (in a second time).
Thx

@adbridge
Copy link
Contributor

adbridge commented Oct 7, 2019

@jeromecoutant is this still waiting for the readme update ?

@jeromecoutant
Copy link
Collaborator

No...
I will do it in a second time, please start CI

@adbridge
Copy link
Contributor

adbridge commented Oct 7, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 7, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@jeromecoutant
Copy link
Collaborator

Can you start again exporter CI ?

Copy link
Contributor

@mark-edgeworth mark-edgeworth left a comment

Choose a reason for hiding this comment

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

Reviewed only from a tools PoV. Looks ok.

Add 2 targets for DISCO_H747I dualcore:
* DISCO_H747I      -> for CM7 core
* DISCO_H747I_CM4  -> for CM4 core

Current restrictions:
* TICKLESS deactivated
* DeepSleep not supported (DeepSleep wrapped to sleep)

Warning: use of the same IP (example I2C1) by both core at the same time is not prevented,
but is strongly not recommended.
Some Hardware Semaphore are use for common IP, to manage concurrent access by both cores: Flash, GPIO, RCC.

Warning: Drag and drop of binary to DISCO_H747I will flash CM7.
         In order to flash CM4, one can use STM32 CubeProgrammer tool.
@ABOSTM ABOSTM force-pushed the DISCO_H747I_DUALCORE_SUPPORT branch from 6c728de to 6397a1d Compare October 14, 2019 16:06
@ABOSTM
Copy link
Contributor Author

ABOSTM commented Oct 14, 2019

Merge conflict solved.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 14, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 15, 2019

I'll review today and merge later.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 15, 2019

Would it benefit from a readme file in the target directory with description how to build and flash both images?

Was this added, @jeromecoutant @ABOSTM ? Github does not show me any readme file here

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 15, 2019

I can't find docs for DUAL_CORE - what does it do ? When should I use one? (if I edit these changed files, how do I know where I should use this ?)

Readme might answer my questions

@jeromecoutant
Copy link
Collaborator

DUAL_CORE is set in MCU CMSIS file.

@MarceloSalazar
Copy link

@0xc0170 can we please merge this for 5.14.1?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 16, 2019

@0xc0170 can we please merge this for 5.14.1?

The code freeze was the previous week, I set this to 5.14.2 as the next patch release.

@jeromecoutant
Copy link
Collaborator

Would it benefit from a readme file in the target directory with description how to build and flash both images?

Good idea, I will do it asap (in a second time).
Thx

@bulislaw
Sorry for delay... #12415

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.

9 participants