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

Fix QSPIF Bus mode mask and quad enable write SR size #10171

Merged
merged 4 commits into from
Apr 1, 2019

Conversation

offirko
Copy link
Contributor

@offirko offirko commented Mar 20, 2019

Fix Bus mode mask, and fix status register write size in some quad enable modes

Fix: #10165
Fix: #10163

Pull request type

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

@offirko
Copy link
Contributor Author

offirko commented Mar 20, 2019

@ARMmbed/mbed-os-maintainers , @ARMmbed/mbed-os-storage , @VairaRamasamy - please review.

@offirko
Copy link
Contributor Author

offirko commented Mar 20, 2019

@VairaRamasamy - can you please test the fix on CY8CPROTO_062_4343W

@@ -874,6 +875,7 @@ int QSPIFBlockDevice::_sfdp_set_quad_enabled(uint8_t *basic_param_table_ptr)
status_reg_setup[1] = 0x2; // Bit 1 of status Reg 2
_read_register_inst = 0x35;
sr_read_size = 1;
sr_write_size = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems to be misaligned (astyle should catch these)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry fixing

@@ -1032,7 +1034,7 @@ int QSPIFBlockDevice::_sfdp_detect_best_bus_read_mode(uint8_t *basic_param_table
}
is_qpi_mode = false;
examined_byte = basic_param_table_ptr[QSPIF_BASIC_PARAM_TABLE_FAST_READ_SUPPORT_BYTE];
if (examined_byte & 0x40) {
if (examined_byte & 0x20) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this mode fix related to sr_write_size - they are both in the same commit - should they be in separate commits ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its two small changes, both are related to quad enabling process... though they in different areas. I can separate them if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be fine, what about commit message (issues have details for these fixes to be tter understand them) ?

@vi
Copy link

vi commented Mar 20, 2019

@vi ... - please review

Maybe meant "@vimalrajr"?

When setting Quad Enable, either SR1, SR2 or CR setup is required.
Either way register size is up to 2 bytes.
@ciarmcom ciarmcom requested review from a team March 20, 2019 14:00
@ciarmcom
Copy link
Member

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

Copy link
Contributor

@cy-vaira cy-vaira left a comment

Choose a reason for hiding this comment

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

@VairaRamasamy - can you please test the fix on CY8CPROTO_062_4343W

@offirko I've tested this on CY8CPROTO_062_4343W and it works! Thanks for the fix.

Copy link
Contributor

@yossi2le yossi2le left a comment

Choose a reason for hiding this comment

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

I would consider adding a return in the default section of the case.
Other than that, it looks good to me.

@offirko
Copy link
Contributor Author

offirko commented Mar 28, 2019

@yossi2le - thanks, I fixed default "Unsupported" behavior to do nothing but log warning and return.

@cmonr
Copy link
Contributor

cmonr commented Mar 28, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 29, 2019

Test run: FAILED

Summary: 2 of 9 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@adbridge
Copy link
Contributor

@offirko please take a look at the failures

@cmonr
Copy link
Contributor

cmonr commented Mar 29, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 29, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@offirko
Copy link
Contributor Author

offirko commented Mar 31, 2019

@NirSonnenschein - failures are on K66F that wasnt available: "[1553892247.32][CONN][ERR] Failed to connect to resource" , can you please rerun CI

@NirSonnenschein
Copy link
Contributor

restarted CI

@mbed-ci
Copy link

mbed-ci commented Mar 31, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@offirko
Copy link
Contributor Author

offirko commented Mar 31, 2019

@NirSonnenschein - hmm same K66F availability issue..
[1554020328.12][GLRM][ERR] Flash response(request_id: 9bc74b62-538d-11e9-8bc0-0242ac110008) failed with message: Resource is not connected

@mbed-ci
Copy link

mbed-ci commented Apr 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 4
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 1, 2019

@NirSonnenschein - hmm same K66F availability issue..

Few targets went offline, fixed today

@0xc0170 0xc0170 merged commit 0403883 into ARMmbed:master Apr 1, 2019
lrusinowicz added a commit to lrusinowicz/mbed-os that referenced this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants