hal-qspi test #7325

maciejbocianski · 2018-06-26T08:49:03Z

Description

This PR adds:

QSPI hal test
temporary fix for qspi_free return value for STM QSPI implementation
qspi pin names for DISCO_L475VG_IOT01A target
fix for QSPI custom command sending for NRF target
fix for qspi R/W opcodes mapping for NRF target

Supported targets: DISCO_L475VG_IOT01A, NRF52840_DK

Known issues:

STM: lack of qspi_free implementation
STM: bug with to early write finish indication on DISCO_L475VG_IOT01A target (for more details see STM_DISCO_L475VG_IOT01A_WRITE_4IO_BUG_WORKAROUND)
STM: HAL_QSPI_Init is failing on ARM compiler with HAL_BUSY error
STM: malloc is failing on DISCO_L475VG_IOT01A target on GCC (every dynamic memory allocation returns NULL)

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

jamesbeyond · 2018-06-27T09:19:28Z

targets/TARGET_STM/qspi_api.c

@@ -192,7 +192,8 @@ qspi_status_t qspi_init(qspi_t *obj, PinName io0, PinName io1, PinName io2, PinN
 qspi_status_t qspi_free(qspi_t *obj)
 {
    // TODO
-    return QSPI_STATUS_ERROR;
+    //return QSPI_STATUS_ERROR;
+    return QSPI_STATUS_OK;


What's the reason, this function can't be fully implemented but required a temperate fix?

I think it's temporary fix. It should be implemented

@adustm Please review

offirko · 2018-06-27T09:35:29Z

TESTS/mbed_hal/qspi/qspi_test_utils.cpp

+{
+    uint8_t reg_data[QSPI_STATUS_REGISTER_SIZE];
+
+    reg_data[0] = STATUS_BIT_QE;


SFDP standard defines Quad Enable procedure depending on QER bits value.
This procedure of matches QER setup 010b where QE is bit 6 of status register.
Do we require a more general approach that handles Quad Enable for all QER value scenarios?

Good point, could be done in next step.
But not sure if we really need such flexibility as SFDP provides in test layer.
The idea of QSPI tests was to check basic features, common for all targets/memory chips,
just to be sure that HAL API implementations works
@0xc0170 @SenRamakri

@offirko This is utils functions for the driver test that has no SFDP knowledge. SFDP will be part of the device block driver (tested there). What would you suggest here?

AFAIU (regardless of SFDP), QE bit is frequently bit number 6 of Status Register - but not for all devices. 'quad_enable' will pass successfully because it verifies that bit 6 value has changed, but if further tests expect that quad_enable will enable QPI or FAST Quad mode they might fail for certain devices. We can perhaps start with this hard coded value and later on move to it to per-target configuration if required.

SenRamakri · 2018-06-27T20:04:12Z

TESTS/mbed_hal/qspi/qspi_test_utils.cpp

+    return ((reg[0] & STATUS_BIT_WEL) == 0 ? QSPI_STATUS_OK : QSPI_STATUS_ERROR);
+}
+
+WEAK void log_security_register(Qspi &qspi)


What's the purpose of these log_xx_register functions? Why are they weak? Usually we don't have to print lot of data from tests, may be you need to confine them to debug builds only.

Yes, it was for debugging purpose, I'll remove it.
All weak function can be overridden if specific target require different handling

maciejbocianski · 2018-06-29T15:55:01Z

PR has been rebased and updated:

log_xx_register functions removed

cmonr · 2018-06-30T01:12:12Z

If all of the reviewers could re-review:
@0xc0170 @SenRamakri @jamesbeyond @offirko @kegilbert

maciejbocianski · 2018-07-04T17:03:53Z

Test code was updated. Changes was introduced as background for new memory chips support.
Please review again @0xc0170 @SenRamakri @jamesbeyond @offirko @kegilbert

offirko

Is there a 4_4_4 QPI read mode test?
Thanks

offirko · 2018-06-28T12:53:12Z

TESTS/mbed_hal/qspi/qspi_test_utils.cpp

+{
+    uint8_t reg_data[QSPI_STATUS_REGISTER_SIZE];
+
+    reg_data[0] = STATUS_BIT_QE;


AFAIU (regardless of SFDP), QE bit is frequently bit number 6 of Status Register - but not for all devices. 'quad_enable' will pass successfully because it verifies that bit 6 value has changed, but if further tests expect that quad_enable will enable QPI or FAST Quad mode they might fail for certain devices. We can perhaps start with this hard coded value and later on move to it to per-target configuration if required.

maciejbocianski · 2018-07-05T07:01:47Z

@offirko
Currently 4_4_4 is not supported, but the test will evolve as new targets/flash chips support will be added, so there is no problem to add it in future when we will have stable version of test.

BTW adding new configs is very easy since everything is parameterized

maciejbocianski · 2018-07-05T07:16:48Z

@offirko
It has been already done (see a3f76bc) all per-target configuration like: quad_enable, quad_disable, dual_enable, dual_disable, fast_mode_enable was moved to memory config files

jamesbeyond · 2018-07-05T17:55:21Z

Can we have CI on this one, please ? @cmonr

cmonr · 2018-07-05T18:32:08Z

/morph build

mbed-ci · 2018-07-05T19:29:04Z

Build : ABORTED

Build number : 2535
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7325/

cmonr · 2018-07-05T20:05:29Z

Trying that again.
/morph build

mbed-ci · 2018-07-05T23:12:12Z

Build : FAILURE

Build number : 2541
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7325/

maciejbocianski · 2018-07-06T05:34:04Z

Test failed on some QSPI targets since only selected QSPI enabled targets support QSPI test.
Additional preprocessor guard has been added

maciejbocianski · 2018-07-06T05:52:58Z

@cmonr @0xc0170 @adbridge
morph can be re-triggered

maciejbocianski · 2018-07-17T10:16:05Z

feature-qspi branch ant this PR have been rebased

0xc0170 · 2018-07-17T10:25:03Z

/morph build

adustm · 2018-07-17T13:36:55Z

Hello @maciejbocianski
Could you give me more details about the failure in case you remove the STM_WRITE_4IO_BUG_WORKAROUND ?
I don't see the difference with and without it.
Kind regards

cmonr · 2018-07-17T13:51:03Z

/morph build

mbed-ci · 2018-07-17T15:45:31Z

Build : ABORTED

Build number : 2624
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7325/

cmonr · 2018-07-17T16:49:11Z

Not sure why this ended up timing out, but please take a look at the build errors.

QSPI feature was mistakenly moved form target nrf52840 to nrf52832 while rebasing. This change fixes it

maciejbocianski · 2018-07-17T21:20:05Z

There was mistake while rebasing. QSPI flag was mistakenly moved from nrf52840 to 'nrf52832` config.
Thus all nrf52832 based targets builds failed.
Fix was added: af90f2c

cmonr · 2018-07-18T02:48:02Z

@maciejbocianski Been there. It happens.

/morpg build

cmonr · 2018-07-18T02:48:17Z

/morph build

mbed-ci · 2018-07-18T03:47:06Z

Build : SUCCESS

Build number : 2627
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7325/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-07-18T05:28:52Z

Test : SUCCESS

Build number : 2374
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7325/2374

mbed-ci · 2018-07-18T05:29:27Z

Exporter Build : SUCCESS

Build number : 2266
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7325/

maciejbocianski · 2018-07-24T05:36:19Z

@adustm
after rebasing feature_qspi branch to master, problems with malloc and with write delay seems to be not present anymore

adustm · 2018-07-24T08:47:04Z

great news. The only remaining problem I can see is the ARM toolchain

---------------------+---------------------+-------------------------------------------------------+--------+--------+---------+--------------------+
 platform_name       | test suite          | test case                                             | passed | failed | result  | elapsed_time (sec) |
---------------------+---------------------+-------------------------------------------------------+--------+--------+---------+--------------------+
 DISCO_L475VG_IOT01A | tests-mbed_hal-qspi | qspi frequency setting test                           | 0      | 0      | SKIPPED | 0.0                |
 DISCO_L475VG_IOT01A | tests-mbed_hal-qspi | qspi init/free test                                   | 0      | 1      | FAIL    | 0.08               |
 DISCO_L475VG_IOT01A | tests-mbed_hal-qspi | qspi memory id test                                   | 1      | 0      | OK      | 0.1                |
 DISCO_L475VG_IOT01A | tests-mbed_hal-qspi | qspi write(1-1-1)/x1  read(1-1-1)/x1  repeat/x1  test | 0      | 0      | SKIPPED | 0.0                |

I can't find the fix for that at the moment (the issue disappears when opened with a debugger)

maciejbocianski · 2018-07-24T09:48:30Z

On my side problem with ARM compiler also disappeared (after update to master)

adustm · 2018-07-24T09:55:06Z

On my side problem with ARM compiler also disappeared (after update to master)
@maciejbocianski did you rebase again or are you using feature-qspi branch that was rebased a few days ago ?

maciejbocianski · 2018-07-24T10:16:34Z

@adustm there was only one rebase a few days ago, and I'm using it (pure https://github.com/ARMmbed/mbed-os/tree/feature-qspi plus yours PR #7581)

adustm · 2018-07-24T12:54:58Z

Hello,
I'm using the same version as you. It's a matter of chance that yours is working.
I've found the bug.
qspi object is a local variable in qspi_init_free_test function (then located in the stack that contains whatever values).
2 variables in qspi struct shall be initalized before the call to HAL_QSPI_Init function, inside qspi_init function. Otherwise you can be blocked at HAL_LOCK(hqspi) in stm32l4xx_hal_qspi.c, line 276
which was my case

 qspi_status_t qspi_init(qspi_t *obj, PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, PinName ssel, uint32_t hz, uint8_t mode)
 {
     // Enable interface clock for QSPI
      __HAL_RCC_QSPI_CLK_ENABLE();
 
     // Reset QSPI
     __HAL_RCC_QSPI_FORCE_RESET();
     __HAL_RCC_QSPI_RELEASE_RESET();
 
+    // Reset handle internal state
+    obj->handle.State = HAL_QSPI_STATE_RESET;
+    obj->handle.Lock = HAL_UNLOCKED;
+

PR#7586 has just arrived ;)

Kind regards
Armelle

0xc0170 requested a review from SenRamakri June 26, 2018 08:53

0xc0170 added the needs: review label Jun 26, 2018

cmonr requested a review from kegilbert June 27, 2018 02:08

bulislaw requested review from jamesbeyond and offirko June 27, 2018 08:34

jamesbeyond reviewed Jun 27, 2018

View reviewed changes

offirko reviewed Jun 27, 2018

View reviewed changes

SenRamakri reviewed Jun 27, 2018

View reviewed changes

0xc0170 force-pushed the feature-qspi branch from cc7834a to 1eb51cf Compare June 29, 2018 08:20

maciejbocianski force-pushed the qspi_tests branch 2 times, most recently from c11c8f8 to c9b3b45 Compare June 29, 2018 15:51

jamesbeyond approved these changes Jul 4, 2018

View reviewed changes

offirko reviewed Jul 5, 2018

View reviewed changes

offirko approved these changes Jul 5, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Jul 5, 2018

cmonr added needs: work and removed needs: CI labels Jul 5, 2018

maciejbocianski force-pushed the qspi_tests branch from f13565f to 24750d6 Compare July 17, 2018 10:13

cmonr added needs: work and removed needs: review labels Jul 17, 2018

nrf52x: fix QSPI enable flag

af90f2c

QSPI feature was mistakenly moved form target nrf52840 to nrf52832 while rebasing. This change fixes it

cmonr added needs: CI and removed needs: work labels Jul 18, 2018

cmonr added ready for merge and removed needs: CI labels Jul 18, 2018

cmonr merged commit 0715a46 into ARMmbed:feature-qspi Jul 18, 2018

0xc0170 removed the ready for merge label Jul 18, 2018

maciejbocianski mentioned this pull request Jul 23, 2018

hal-qspi_test: remove STM workaround #7580

Merged

adustm mentioned this pull request Jul 24, 2018

Add reset internal state before call to HAL_QspiInit function #7586

Merged

hal-qspi test #7325

hal-qspi test #7325

Uh oh!

Conversation

maciejbocianski commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Jun 30, 2018

Uh oh!

maciejbocianski commented Jul 4, 2018

Uh oh!

offirko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejbocianski commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maciejbocianski commented Jul 5, 2018

Uh oh!

jamesbeyond commented Jul 5, 2018

Uh oh!

cmonr commented Jul 5, 2018

Uh oh!

mbed-ci commented Jul 5, 2018

Uh oh!

cmonr commented Jul 5, 2018

Uh oh!

mbed-ci commented Jul 5, 2018

Uh oh!

maciejbocianski commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maciejbocianski commented Jul 6, 2018

Uh oh!

maciejbocianski commented Jul 17, 2018

Uh oh!

0xc0170 commented Jul 17, 2018

Uh oh!

adustm commented Jul 17, 2018

Uh oh!

cmonr commented Jul 17, 2018

Uh oh!

mbed-ci commented Jul 17, 2018

Uh oh!

cmonr commented Jul 17, 2018

Uh oh!

maciejbocianski commented Jul 17, 2018

Uh oh!

cmonr commented Jul 18, 2018

Uh oh!

cmonr commented Jul 18, 2018

Uh oh!

mbed-ci commented Jul 18, 2018

Triggering tests

Uh oh!

maciejbocianski commented Jun 26, 2018 •

edited

Loading

maciejbocianski Jun 28, 2018 •

edited

Loading

maciejbocianski commented Jun 29, 2018 •

edited

Loading

maciejbocianski commented Jul 5, 2018 •

edited

Loading

maciejbocianski commented Jul 6, 2018 •

edited

Loading