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

PSOC6: move mbed_sdk_init to mbed_overrides.c #10483

Merged
merged 2 commits into from
May 16, 2019
Merged

PSOC6: move mbed_sdk_init to mbed_overrides.c #10483

merged 2 commits into from
May 16, 2019

Conversation

vmedcy
Copy link
Contributor

@vmedcy vmedcy commented Apr 25, 2019

Purposes:

  • Remove MbedOS-specific code from system_psoc6_{cm4,cm0plus}.c
    to simplify updates to new PDL version (startup code is part of PDL).
  • Unify mbed_sdk_init initialization sequence for both CPU cores.
    This change is non-functional, sequence itself is not changed for any
    of the PSoC 6 M4/M0 PSA/non-PSA targets.
  • Do not disable global interrupts during init_cycfg_all, this function is safe to execute with interrupts enabled.

Description

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

Purposes:
* Remove MbedOS-specific code from system_psoc6_{cm4,cm0plus}.c
  to simplify updates to new PDL version (startup code is part of PDL).
* Unify mbed_sdk_init initialization sequence for both CPU cores.
  This change is non-functional, sequence itself is not changed for any
  of the PSoC 6 M4/M0 PSA/non-PSA targets.
@ciarmcom ciarmcom requested a review from a team April 25, 2019 19:00
@ciarmcom
Copy link
Member

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

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Nice unification

{
#if !defined(COMPONENT_SPM_MAILBOX)
/* Disable global interrupts */
__disable_irq();
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use rather critical section here

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 line 63 and 56

Copy link
Contributor

Choose a reason for hiding this comment

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

As this was in the code previously, we could still fix it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now fixed by 737c98f - after further analysis, I can confirm entering critical section is not needed at all, init_cycfg_all is fine to execute with global interrupts enabled. I checked this functionally on CM4/CM0P PSA/non-PSA targets (actually there is no change in PSA case). On CM0+, global interrupts are enabled by ROM boot before Reset_Handler.

Copy link
Contributor

@orenc17 orenc17 left a comment

Choose a reason for hiding this comment

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

PSA-wise this is fine

@vmedcy
Copy link
Contributor Author

vmedcy commented May 3, 2019

@0xc0170: code review feedback is addressed, the needless __enable_irq()/__disable_irq() calls removed from mbed_sdk_init.

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

thanks for the update

Sorry about the delay for this review. Just one question about the enable irq

init_cycfg_all();

/* Enable global interrupts (disabled in CM4 startup assembly) */
__enable_irq();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still needed ? Previously was disable-enable pair, now only enable in sdk_init

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. As mentioned in the code comment, CM4 startup assembly masks the global interrupts in Reset_Handler (this behavior is required to enable correct startup sequence under all scenarios, the code is there since the initial PDL 3.0.0 release).
For example, see startup_psoc6_01_cm4.S:

@mbed-ci
Copy link

mbed-ci commented May 15, 2019

Test run: SUCCESS

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

@adbridge adbridge merged commit d997563 into ARMmbed:master May 16, 2019
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.

6 participants