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

QSPI driver double call qspi_init() #12678

Closed
dustin-crossman opened this issue Mar 23, 2020 · 8 comments
Closed

QSPI driver double call qspi_init() #12678

dustin-crossman opened this issue Mar 23, 2020 · 8 comments

Comments

@dustin-crossman
Copy link
Contributor

dustin-crossman commented Mar 23, 2020

Description of defect

In QSPI::_initialize mbed::QSPI will call the target specific qspi_init() once. Later, if QSPI::_acquire() gets called (which it does in QSPI::read(), QSPI::write(), etc) qspi_init() will be called again. A better description of the issue is here: #11230 (Also note that there is still no documentation regarding this).

There seems to be 2 ways to resolve this:

  1. Completely remove _acquire() and the drivers concept of ownership completely.
  2. Maintain the drivers concept of ownership and explicitly document the implications and requirements in the QSPI porting guide.

In the meantime I believe there is a way to partially mitigate the issue:
In QSPI::_intialize (and _initialize_direct) the _initialized field is being set to true but the _owner field is not. Since _owner is false upon the first read/write call, _acquire() and therefore _initialize() will be called again even if no other QSPI object acquired ownership in the meantime. I think this is a bug regardless of the approach taken to fix this issue and is an easy fix.

I'm happy to put in a PR for the mitigation but I'd also like to see this resolved properly one way or the other.

Target(s) affected by this defect ?

All

Toolchain(s) (name and version) displaying this defect ?

All

What version of Mbed-os are you using (tag or sha) ?

539cf3f

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

Any use of QSPI driver will reproduce this defect.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 23, 2020

cc @ARMmbed/mbed-os-hal

In QSPI::_intialize (and _initialize_direct) the _initialized field is being set to true but the _owner field is not. Since _owner is false upon the first read/write call, _acquire() and therefore _initialize() will be called again even if no other QSPI object acquired ownership in the meantime. I think this is a bug regardless of the approach taken to fix this issue and is an easy fix.

Lets fix this.

#11230

That was a question but looks like a bug that should be opened and fixed.

@ciarmcom
Copy link
Member

@dustin-crossman thank you for raising this issue.Please take a look at the following comments:

We cannot automatically identify a release based on the version of Mbed OS that you have provided.
Please provide either a single valid sha of the form #abcde12 or #3b8265d70af32261311a06e423ca33434d8d80de
or a single valid release tag of the form mbed-os-x.y.z .
E.g. 'Latest' is not a unique single snapshot.
Please update the issue header with the missing information.
NOTE: If there are fields which are not applicable then please just add 'n/a' or 'None'.This indicates to us that at least all the fields have been considered.

@dustin-crossman
Copy link
Contributor Author

@0xc0170 Ok. I put in a PR for the partial fix: #12680
Should we reopen that old issue or continue here?

@ciarmcom
Copy link
Member

ciarmcom commented Mar 24, 2020

Internal Jira reference: https://jira.arm.com/browse/IOTOSM-464

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

@adbridge See ^^ duplication

@ARMmbed ARMmbed deleted a comment from ciarmcom Mar 24, 2020
@adbridge
Copy link
Contributor

adbridge commented Mar 24, 2020

MBOTRIAGE-2603 closed as duplicate and link deleted

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2020

@0xc0170 Ok. I put in a PR for the partial fix: #12680

It was merged. This can be closed or what's left @dustin-crossman ?

@dustin-crossman
Copy link
Contributor Author

@0xc0170 #12680 only partially fixes the double init problem. It would be good to figure out a proper resolution to the issue, especially since this isn't the first time it's been reported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants