Differentiate alt and dummy cycles in QSPIF #11297

kyle-cypress · 2019-08-23T00:14:33Z

Description

Proposed fix for #11295

Propagate separate alt cycle and dummy cycle counts from QSPIFBlockDevice
down to the qspi driver, so that drivers which handle the two separately have
enough information to do so.

Submitting the pull request now to obtain feedback on the change. Greentea test logs will be attached soon.

Pull request type

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

Reviewers

@ARMmbed/team-cypress

Release Notes

ciarmcom · 2019-08-23T01:00:35Z

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

0xc0170 · 2019-08-27T12:50:35Z

@ARMmbed/mbed-os-storage Please review

VeijoPesonen

Looks good to me. But I would like to see someone else also to review this as I'm not that familiar with the subject yet.

bulislaw

@davidsaada @bentcooke can you guys have a look please

davidsaada · 2019-08-29T08:03:51Z

@offirko mind taking a quick look?

offirko · 2019-08-29T09:41:28Z

The code updates seems to be good.
When the initial code was developed, QSPI driver was available for STM boards only.
I would recommend running QSPIF tests on STM devices (L476, L475, F769..) and others to make sure the change doesn't break their behavior.

0xc0170 · 2019-08-29T09:49:38Z

@ARMmbed/team-st-mcd would you be able to quickly review this if all is good ? I'll run Ci in the meantime

0xc0170 · 2019-08-29T09:49:49Z

CI started

jeromecoutant · 2019-08-29T10:29:59Z

Hi

I would recommend running QSPIF tests on STM devices (L476, L475, F769..) and others to make sure the change doesn't break their behavior.

I requested to ST CI features-storage-tests-blockdevice-general_block_device test
with all STM targets supporting QSPIF

mbed-ci · 2019-08-29T12:27:29Z

Test run: SUCCESS

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

0xc0170 · 2019-08-29T13:13:14Z

I requested to ST CI features-storage-tests-blockdevice-general_block_device test

Waiting for the confirmation

jeromecoutant · 2019-08-29T13:22:45Z

First tests not very good :-(

target	platform_name	test suite	test case	passed	result	elapsed_time (sec)
DISCO_F746NG-ARMC6	DISCO_F746NG	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing Init block device	1	OK	0.09
DISCO_F746NG-ARMC6	DISCO_F746NG	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing read write random blocks	0	ERROR	0.0
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing BlockDevice erase functionality	1	OK	0.52
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing Deinit block device	1	OK	0.09
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing Init block device	1	OK	0.1
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing contiguous erase, write and read	1	OK	4.6
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing multi threads erase program read	1	OK	5.21
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing program read small data sizes	1	OK	0.44
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing read write random blocks	1	OK	1.22
DISCO_L475VG_IOT01A-ARMC6	DISCO_L475VG_IOT01A	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing unaligned erase blocks	1	OK	0.17
DISCO_F469NI-ARMC6	DISCO_F469NI	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing Init block device	1	OK	0.09
DISCO_F469NI-ARMC6	DISCO_F469NI	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing read write random blocks	0	ERROR	0.0
DISCO_F413ZH-ARMC6	DISCO_F413ZH	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing Init block device	1	OK	0.1
DISCO_F413ZH-ARMC6	DISCO_F413ZH	mbed-os-features-storage-tests-blockdevice-general_block_device	QSPIF Testing read write random blocks	0	ERROR	0.0

0xc0170 · 2019-08-29T16:52:08Z

@kyle-cypress unfortunately, this needs work

kyle-cypress · 2019-08-29T17:58:49Z

We believe we have found the issue. It appears that commit 05899e9 introduced a latent bug in the STM QSPI API which is only triggered when command->alt.disabled == false. However, we do not have a way to test this locally. I have pushed d202815 with a proposed fix. @0xc0170 , @jeromecoutant , are one of you able to trigger a new CI pass?

0xc0170 · 2019-08-30T06:00:12Z

@ARMmbed/team-st-mcd can you retest please?

jeromecoutant · 2019-08-30T06:12:24Z

already started :-)

0xc0170 · 2019-08-30T12:42:42Z

I am proposing to move this to 5.14.1 , as there are few other changes needed for 5.14 that can't go into the patch release.

I'll remove the label, we need more testing allowed for this one.

kyle-cypress · 2019-09-18T22:05:48Z

Pushed up a new changeset which should address the build failures. The cause of the failures is that the previous version of the commit was an early proof of concept, but several other drivers needed to be updated in order for the build to pass.

kyle-cypress · 2019-09-18T22:08:52Z

@bulislaw @0xc0170 - any suggestion on a way forward for this?

This needs to be tested on various targets and memories as it is affecting a driver. @kyle-cypress What targets can you test this? We can help or provide a way to test this. What would be helpful to get to the root of the issue for other targets?

Let me check if PR check includes QSPI test , I could not find quickly some PR that would have it, as this touches the area, it should be tested.

We are only able to test this on Cypress targets. I think the best approach would be if someone from your side is able to debug with the other failing targets and identify root causes of the failures, then we can adjust our proposed changeset as necessary.

0xc0170 · 2019-09-19T07:09:43Z

We are only able to test this on Cypress targets. I think the best approach would be if someone from your side is able to debug with the other failing targets and identify root causes of the failures, then we can adjust our proposed changeset as necessary.

Yes, that would be the idea. I'll talk to our @ARMmbed/mbed-os-test team how to help with testing

0xc0170 · 2019-09-19T07:13:51Z

CI restarted

mbed-ci · 2019-09-19T08:03:31Z

Test run: SUCCESS

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

jamesbeyond · 2019-09-19T10:06:51Z

FYI @maciejbocianski

0xc0170 · 2019-09-26T14:23:22Z

Allow for arbitrary QSPI alt sizes (WIP)

Can we split this from this PR? I would like to review this separately and proceed. It's not related to the block device, just HAL/driver. Please describe why alt should be any number and how it changes QSPI apps (blockdevices and others). Can this be done backward compatible (functional change) ?

kyle-cypress · 2019-09-26T16:20:59Z

Allow for arbitrary QSPI alt sizes (WIP)

Can we split this from this PR? I would like to review this separately and proceed. It's not related to the block device, just HAL/driver. Please describe why alt should be any number and how it changes QSPI apps (blockdevices and others). Can this be done backward compatible (functional change) ?

This this is to illustrate the proposal in #11297 (comment) , as requested in #11297 (comment).
Alt should be any number because the spec allows it and there are devices enabled in mbed which make use of this. The example mentioned above by @matthew-macovsky-cypress is Micron's N25Q128A:

As its datasheet indicates (page 39), in all read modes that support mode bits only a single mode cycle is used - i.e. 1, 2, or 4 mode bits depending on the read mode.

If you agree that this change makes sense in principle, I will split off a new PR (on which this one would then depend) to track discussion of the specifics.

maciejbocianski

I'm getting quite different results comparing to @jeromecoutant results.
DISCO_F413ZH passes with and without fix while 'DISCO_L475VG_IOT01A' is failing with fix.

maciejbocianski · 2019-09-30T07:39:01Z

targets/TARGET_STM/qspi_api.c

+            // Align alt value to the end of the most significant byte
+            st_command->AlternateBytes = command->alt.value << leftover_bits;
+        } else {
+            rounded_size -= 1;


I think this line shouldn't be here. It cause that rounded_size falls below 0 in some cases

I agree. Addressed in 9b32c0f pushed to #11602 , and rebased this PR on #11602 again

The QSPI spec allows alt to be any size that is a multiple of the number of data lines. For example, Micron's N25Q128A uses only a single alt cycle for all read modes (1, 2, or 4 bits depending on how many data lines are in use).

kyle-cypress · 2019-09-30T22:21:57Z

Split out #11602 and #11603 . Rebased this PR onto #11602 to enable testing and debugging to continue on the combination (#11603 is a general improvement which should not be required for testing)

Remove an extraneous decrement operation in cases where the alt bits size is a multiple of 8.

Propagate separate alt cycle and dummy cycle counts from QSPIFBlockDevice down to the qspi driver, so that drivers which handle the two separately have enough information to do so.

jeromecoutant

Tested with ARMC6 with:

DISCO_L496AG
DISCO_L476VG
DISCO_F769NI
DISCO_F469NI
DISCO_F746NG
DISCO_F413ZH
DISCO_L475VG_IOT01A

@LMESTM @MarceloSalazar

maciejbocianski · 2019-10-07T08:20:11Z

Also tested on NRF52840_DK and EFM32GG11_STK3701

| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing BlockDevice erase functionality  | 1      | 0      | OK     | 0.52               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing Deinit block device              | 1      | 0      | OK     | 0.09               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing Init block device                | 1      | 0      | OK     | 0.08               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing contiguous erase, write and read | 1      | 0      | OK     | 1.56               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing multi threads erase program read | 1      | 0      | OK     | 5.57               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing program read small data sizes    | 1      | 0      | OK     | 0.46               |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing read write random blocks         | 1      | 0      | OK     | 1.3                |
| NRF52840_DK-ARMC6 | NRF52840_DK   | features-storage-tests-blockdevice-general_block_device | QSPIF Testing unaligned erase blocks           | 1      | 0      | OK     | 0.18               |


| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing BlockDevice erase functionality  | 1      | 0      | OK     | 0.19               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing Deinit block device              | 1      | 0      | OK     | 0.02               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing Init block device                | 1      | 0      | OK     | 0.04               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing contiguous erase, write and read | 1      | 0      | OK     | 1.24               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing multi threads erase program read | 1      | 0      | OK     | 5.42               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing program read small data sizes    | 1      | 0      | OK     | 0.41               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing read write random blocks         | 1      | 0      | OK     | 1.12               |
| EFM32GG11_STK3701-ARMC6 | EFM32GG11_STK3701 | features-storage-tests-blockdevice-general_block_device | QSPIF Testing unaligned erase blocks           | 1      | 0      | OK     | 0.08               |

maclobdell · 2019-10-08T19:57:05Z

@ARMmbed/mbed-os-maintainers Any blockers on this one? Can it be merged yet?

adbridge · 2019-10-11T10:49:00Z

CI started

mbed-ci · 2019-10-11T12:57:28Z

Test run: SUCCESS

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

kyle-cypress force-pushed the pr/qspi-dummy-cycles branch from ca69263 to d659461 Compare August 23, 2019 00:35

ciarmcom requested a review from a team August 23, 2019 01:00

ciarmcom added the needs: review label Aug 23, 2019

0xc0170 added the release-version: 5.14.0-rc1 label Aug 23, 2019

0xc0170 requested review from a team and removed request for a team August 23, 2019 08:24

0xc0170 approved these changes Aug 26, 2019

View reviewed changes

VeijoPesonen approved these changes Aug 28, 2019

View reviewed changes

bulislaw reviewed Aug 28, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 29, 2019

0xc0170 added needs: review and removed needs: work labels Aug 30, 2019

0xc0170 removed the release-version: 5.14.0-rc1 label Aug 30, 2019

kyle-cypress force-pushed the pr/qspi-dummy-cycles branch from 0f39b2e to 56f8e01 Compare September 18, 2019 22:00

maciejbocianski suggested changes Sep 30, 2019

View reviewed changes

Allow for arbitrary QSPI alt sizes

baf375f

The QSPI spec allows alt to be any size that is a multiple of the number of data lines. For example, Micron's N25Q128A uses only a single alt cycle for all read modes (1, 2, or 4 bits depending on how many data lines are in use).

kyle-cypress force-pushed the pr/qspi-dummy-cycles branch from 56f8e01 to 7d3dd3e Compare September 30, 2019 22:21

Kyle Kearney and others added 2 commits September 30, 2019 16:00

Fix possible negative QSPI alt count on STM

9b32c0f

Remove an extraneous decrement operation in cases where the alt bits size is a multiple of 8.

Differentiate alt and dummy cycles in QSPIF

6bba46e

Propagate separate alt cycle and dummy cycle counts from QSPIFBlockDevice down to the qspi driver, so that drivers which handle the two separately have enough information to do so.

kyle-cypress force-pushed the pr/qspi-dummy-cycles branch from 7d3dd3e to 6bba46e Compare September 30, 2019 23:18

jeromecoutant approved these changes Oct 2, 2019

View reviewed changes

SeppoTakalo approved these changes Oct 7, 2019

View reviewed changes

adbridge added needs: CI and removed needs: work labels Oct 11, 2019

adbridge added the release-version: 5.14.1 label Oct 11, 2019

adbridge added ready for merge and removed needs: CI labels Oct 11, 2019

adbridge merged commit 489c30f into ARMmbed:master Oct 11, 2019

adbridge removed the ready for merge label Oct 11, 2019

kyle-cypress mentioned this pull request Oct 17, 2019

Allow for arbitrary QSPI alt sizes #11602

Merged

Differentiate alt and dummy cycles in QSPIF #11297

Differentiate alt and dummy cycles in QSPIF #11297

Uh oh!

Conversation

kyle-cypress commented Aug 23, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Aug 23, 2019

Uh oh!

0xc0170 commented Aug 27, 2019

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

davidsaada commented Aug 29, 2019

Uh oh!

offirko commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

jeromecoutant commented Aug 29, 2019

Uh oh!

mbed-ci commented Aug 29, 2019

Test run: SUCCESS

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

jeromecoutant commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

kyle-cypress commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 30, 2019

Uh oh!

jeromecoutant commented Aug 30, 2019

Uh oh!

0xc0170 commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyle-cypress commented Sep 18, 2019

Uh oh!

kyle-cypress commented Sep 18, 2019

Uh oh!

0xc0170 commented Sep 19, 2019

Uh oh!

0xc0170 commented Sep 19, 2019

Uh oh!

mbed-ci commented Sep 19, 2019

Test run: SUCCESS

Uh oh!

jamesbeyond commented Sep 19, 2019

Uh oh!

0xc0170 commented Sep 26, 2019

Uh oh!

kyle-cypress commented Sep 26, 2019

Uh oh!

maciejbocianski left a comment

Choose a reason for hiding this comment

Uh oh!

maciejbocianski Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

kyle-cypress Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

kyle-cypress commented Sep 30, 2019

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Oct 7, 2019

0xc0170 commented Aug 30, 2019 •

edited

Loading