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

Remove ownership/acquire in QSPI/OSPI driver #14437

Merged
merged 2 commits into from
Mar 26, 2021
Merged

Remove ownership/acquire in QSPI/OSPI driver #14437

merged 2 commits into from
Mar 26, 2021

Conversation

gpsimenos
Copy link
Contributor

@gpsimenos gpsimenos commented Mar 17, 2021

Summary of changes

This PR removes the concept of peripheral acquisition and ownership in the QSPI driver. Its implementation is currently incomplete and undocumented, and has caused problems in some cases (see 11230 12678 12680).

Impact of changes

In cases where multiple instances of QSPI/OSPI are used with the same underlying peripheral, the QSPI/OSPI driver will no longer re-initialize the underlying peripheral to the appropriate configuration when switching instances. The application must call init when switching instances.

Migration actions required

Documentation


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


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.

OSPI driver should be changed in the same time?

@ciarmcom ciarmcom requested review from a team March 17, 2021 14:00
@ciarmcom
Copy link
Member

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

@gpsimenos gpsimenos changed the title Remove ownership/acquire in QSPI driver Remove ownership/acquire in QSPI/OSPI driver Mar 22, 2021
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.

this shall be a candidate for major. It changes the implementation detail (private function acquire) and fixes the behavior).

This change should contain docs for the release notes.

@@ -229,8 +229,6 @@ class OSPI : private NonCopyable<OSPI> {

ospi_t _ospi;

bool acquire(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was declared but not defined anywhere?

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 it appears so

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2021

this shall be a candidate for major. It changes the implementation detail (private function acquire) and fixes the behavior).

If no one objects, lets change to feature update and document the fix in Impact of changes.

@gpsimenos
Copy link
Contributor Author

I see no objections so far, I have added the impact and fix in the description. Should we go ahead?

@gpsimenos
Copy link
Contributor Author

@adbridge Let's start CI

@adbridge
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 25, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️

@mbed-ci
Copy link

mbed-ci commented Mar 25, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-test
jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci
Copy link

mbed-ci commented Mar 26, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️

@gpsimenos
Copy link
Contributor Author

@adbridge Let's merge!

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.

None yet

8 participants