Add QSPI #7783

maciejbocianski · 2018-08-14T05:56:47Z

Description

Bring new QSPI HAL API to master.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Feature
[ ] Breaking change

@bulislaw @donatieng @jamesbeyond @0xc0170 @offirko

offirko · 2018-08-15T13:31:24Z

TESTS/mbed_hal/qspi/flash_configs/MX25R6435F_config.h

@@ -0,0 +1,192 @@
+/* mbed Microcontroller Library


Is it common practice to use specific commercial manufacturers setups in mbed Tests?

I don't think it's common, actually it's first time. But how to figure out what frequency or what dummy cycle count is supported?

@OPpuolitaival Please review

I don't know how the configuration works but this sounds wrong place to put board specific configuration. We should have some mechanism existing for these configs

Actually it's memory chip specific configuration.The configuration covers: max supported frequency, dummy_cycles_count for different write/read modes, mechanisms for enabling/disabling dual/quad/fast modes (which flag in which register to be set).

Here is example of adding QSPI support to new target by vendor, also contain new memory chip description file (link)

Alternatives:

put config elsewhere (but where?)

use SFDP as is in QSPI SFDP Flash - Block Device link, but it's high level mechanism, not sure if it's adequate here

reduce test to subset of common features supported by all memory chips (not a good idea, HAL tests should cover max of features available in specific API)

@maciejbocianski - I have no advise on this subject, sorry.
Other than that looks great to me - thanks.

offirko · 2018-08-15T13:43:34Z

TESTS/mbed_hal/qspi/flash_configs/N25Q128A_config.h

+    return QSPI_STATUS_OK
+
+
+#define QUAD_ENABLE()                                                           \


I see currently those are TODOs, is there a plan to test QUAD?

Sure dual/quad will be tested, currently we have problems to enable dual/quad mode on targets with N25Q128A memory chip

offirko · 2018-08-15T13:58:40Z

TESTS/mbed_hal/qspi/qspi_test_utils.h

+    sUnknown
+};
+
+struct QspiCommand {


I know struct and class are both legit, but I would prefer to see here a Class

updated, now QspiCommand is a class with privat _cmd

offirko · 2018-08-15T14:00:39Z

TESTS/mbed_hal/qspi/qspi_test_utils.h

+
+    qspi_command_t * get();
+
+    qspi_command_t cmd;


Another reason to make it a class - If a class would have been used, then mbed-os coding style would have required this member to be '_cmd' - later on in the cpp code its hard to tell whether cmd is a member/global/member

updated, now QspiCommand is a class with privat _cmd

offirko · 2018-08-15T14:04:24Z

drivers/QSPI.h

@@ -0,0 +1,222 @@
+/* mbed Microcontroller Library
+ * Copyright (c) 2006-2015 ARM Limited


typo I think license date should be updated

offirko · 2018-08-15T14:17:36Z

drivers/QSPI.h

+     *
+     *  @param inst_width Bus width used by instruction phase(Valid values are 1,2,4)
+     *  @param address_width Bus width used by address phase(Valid values are 1,2,4)
+     *  @param address_size Size in bits used by address phase(Valid values are 8,16,24,32)


same here, better specifies valid ENUM values and not the integer representation which are different

@maciejbocianski Can you remove those integers. It's obvious what type these parameters are

offirko · 2018-08-15T14:21:21Z

drivers/QSPI.h

+
+    /** Acquire exclusive access to this SPI bus
+     */
+    virtual void lock(void);


Why do we want driver lock/unlock to be public? It could potentially be misused to block other SPI users.

Not sure what was the design @0xc0170 @SenRamakri , but public lock/unlock could be utilized to synchronize between two QSPI driver instances controlling two parallel flash chips connected to the same data lines

Good question. I would have assumed this is made protected at least.

@SenRamakri use case for having locks public?

offirko · 2018-08-15T14:47:23Z

drivers/QSPI.cpp

+bool QSPI::_initialize() 
+{
+    if (_mode != 0 && _mode != 1)
+        return QSPI_STATUS_INVALID_PARAMETER;


QSPI_STATUS_INVALID_PARAMETER value (-2) is returned as boolean.
Possibly a problem can occur if "acquire" is called and you return without changing _initialized to false.
also single line 'if' should also be within {..}

AStyle to be run on this feature branch additions

offirko · 2018-08-15T14:54:51Z

drivers/QSPI.h

+
+    /** Configure the data transmission format
+     *
+     *  @param inst_width Bus width used by instruction phase(Valid values are 1,2,4)


Better use the ENUM for Valid values.
The integer representation for the valid values is 0,1,2 :
typedef enum qspi_bus_width {
QSPI_CFG_BUS_SINGLE,
QSPI_CFG_BUS_DUAL,
QSPI_CFG_BUS_QUAD,
} qspi_bus_width_t;

offirko · 2018-08-15T14:58:51Z

drivers/QSPI.cpp

+    _initialized = false;
+
+    //Go ahead init the device here with the default config
+    _initialize();


ctor can not return potential failure in 'initialize' - maybe we should at least add an error log print?

offirko · 2018-08-15T15:11:01Z

@maciejbocianski - Some of my remarks appear twice due to a problem I had in my network during the review - I apologize

stevew817 · 2018-08-18T21:11:33Z

TESTS/mbed_hal/qspi/qspi_test_utils.cpp

+    for (uint32_t j = 0; j < reg_size; j++) {
+        utest_printf("register byte %u data: ", j);
+        for(int i = 0; i < 8; i++) {
+            utest_printf("%s ", ((reg[j] & (1 << i)) & 0xFF) == 0 ? "0" : "1");


Doesn't it make more sense to print the register contents MSB first? I.e. do a (1 << (7-i)) instead.

bulislaw · 2018-08-20T12:16:27Z

Guys we need to accelerate merging this, we have some dependencies. @donatieng @dannybenor

0xc0170

Waiting for the update (@offirko comments addressed)

AnotherButler · 2018-08-20T19:33:12Z

Does this docs PR for the driver need updating, or is it still accurate?
ARMmbed/mbed-os-5-docs#354

maciejbocianski · 2018-08-21T08:13:48Z

Update done all comments were addressed. @offirko please review
astyle fixes on QSPI API/driver/tests added

@donatieng @bulislaw @0xc0170 API has changed QSPI::lock/unlock now are protected

donatieng

Thanks, looks good, let's get this one in!

jamesbeyond · 2018-08-21T10:32:49Z

Hi @AnotherButler,
The ARMmbed/mbed-os-5-docs#354 is about QSPI drivers API, it looks good, I think @SenRamakri may need to review that again. At least some doxygen links is not valid anymore.

The porting guide is Here, we will update that, and we'll submit a new PR on the handbook.

AnotherButler · 2018-08-21T21:49:02Z

@jamesbeyond Thanks

@kegilbert Could you please review the linked PR in place of @SenRamakri as he's out?

maciejbocianski · 2018-08-22T11:20:38Z

@bulislaw @AnotherButler @jamesbeyond @donatieng
Also QSPI porting guide has been updated
ARMmbed/mbed-os-5-docs#676

0xc0170 · 2018-08-22T15:05:52Z

/morph build

cmonr · 2018-08-22T15:33:15Z

CI related.
/morph build

cmonr · 2018-08-22T16:08:08Z

/morph build

mbed-ci · 2018-08-22T16:33:01Z

Build : FAILURE

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

mbed-ci · 2018-08-22T18:30:30Z

Build : FAILURE

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

mbed-ci · 2018-08-22T18:30:30Z

Build : FAILURE

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

kegilbert · 2018-08-22T22:47:53Z

Covering for Senthil for the docs portion, left a few comments on the Handbook PRs but overall looks pretty good.

cmonr · 2018-08-23T04:25:48Z

/morph build

maciejbocianski · 2018-08-23T05:32:03Z

Failed while compiling mbed-os-example-block device K64F IAR

/SPIFBlockDevice.o ./spif-driver/SPIFBlockDevice.cpp
[Warning] SPIFBlockDevice.h@74,56: [Pe611]: overloaded virtual function "BlockDevice::get_erase_size" is only partially overridden in class "SPIFBlockDevice"
[Error] SPIFBlockDevice.cpp@156,52: [Pe020]: identifier "UINT64_MAX" is undefined
[Error] SPIFBlockDevice.cpp@240,17: [Pe020]: identifier "UINT64_MAX" is undefined
[Warning] SPIFBlockDevice.cpp@449,18: [Pe068]: integer conversion resulted in a change of sign
[Error] SPIFBlockDevice.cpp@557,67: [Pe020]: identifier "UINT64_MAX" is undefined
[Error] SPIFBlockDevice.cpp@877,71: [Pe020]: identifier "UINT64_MAX" is undefined
[Error] SPIFBlockDevice.cpp@922,72: [Pe020]: identifier "UINT64_MAX" is undefined
[Error] SPIFBlockDevice.cpp@942,0: [Pe020]: identifier "UINT64_MAX" is undefined

mbed-ci · 2018-08-23T05:40:54Z

Build : SUCCESS

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

Triggering tests

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

0xc0170 · 2018-08-23T08:07:18Z

/morph export-build

mbed-ci · 2018-08-23T10:10:55Z

Exporter Build : FAILURE

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

0xc0170 · 2018-08-23T11:09:10Z

Restarting . Error initialize storage for one target, eclipse.

/morph export-build

mbed-ci · 2018-08-23T13:02:07Z

Exporter Build : FAILURE

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

cmonr · 2018-08-23T17:15:56Z

Pausing Test CI until 5.9.6 PR is merged.
Will restart CI shortly after.

cmonr · 2018-08-23T21:27:36Z

Network licensing issues.
/morph export-build

Testing gap while release PR is worked on.
/morph test

mbed-ci · 2018-08-23T23:48:25Z

Exporter Build : SUCCESS

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

cmonr · 2018-08-24T05:52:33Z

/morph test

mbed-ci · 2018-08-24T08:31:55Z

Test : SUCCESS

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

0xc0170 · 2018-08-24T08:48:27Z

This PR is ready now for integration.

@AnotherButler Can you review API docs please (there are comments above but not touching the API docs review, so assuming it has not yet been done) ? If there are any edits needed, can you send them as subsequent PR please (this PR is required by few other dependencies so would like to have this in today to continue other work related).

mbed-ci · 2018-08-24T10:16:30Z

Exporter Build : FAILURE

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

maciejbocianski mentioned this pull request Aug 14, 2018

Qspi flash pin names standardization #7639

Closed

adbridge added the needs: review label Aug 14, 2018

adbridge requested review from bulislaw, 0xc0170, donatieng, jamesbeyond and offirko August 14, 2018 11:36

offirko reviewed Aug 15, 2018

View reviewed changes

maciejbocianski mentioned this pull request Aug 17, 2018

standardise QSPI pin names #7817

Merged

cmonr requested a review from a team August 17, 2018 15:41

cmonr added the release-version: 5.10.0-rc1 label Aug 17, 2018

stevew817 reviewed Aug 18, 2018

View reviewed changes

stevew817 mentioned this pull request Aug 18, 2018

Silicon Labs QSPI HAL implementation #7825

Merged

0xc0170 changed the title ~~Merge feature-qspi to master~~ Add QSPI feature Aug 20, 2018

0xc0170 requested changes Aug 20, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 20, 2018

maciejbocianski changed the title ~~Add QSPI feature~~ merge QSPI feature branch Aug 21, 2018

offirko approved these changes Aug 21, 2018

View reviewed changes

jamesbeyond approved these changes Aug 21, 2018

View reviewed changes

donatieng approved these changes Aug 21, 2018

View reviewed changes

0xc0170 approved these changes Aug 22, 2018

View reviewed changes

maciejbocianski mentioned this pull request Aug 22, 2018

Update QSPI porting guide ARMmbed/mbed-os-5-docs#676

Merged

0xc0170 removed the needs: work label Aug 22, 2018

cmonr added the risk: G label Aug 23, 2018

0xc0170 added ready for merge and removed needs: CI labels Aug 24, 2018

0xc0170 merged commit 812c6d5 into ARMmbed:master Aug 24, 2018

0xc0170 removed the ready for merge label Aug 24, 2018

0xc0170 changed the title ~~merge QSPI feature branch~~ Add QSPI Aug 24, 2018

cmonr removed the risk: G label Aug 25, 2018

		@@ -0,0 +1,222 @@
		/* mbed Microcontroller Library
		* Copyright (c) 2006-2015 ARM Limited

Add QSPI #7783

Add QSPI #7783

Uh oh!

Conversation

maciejbocianski commented Aug 14, 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!

maciejbocianski Aug 21, 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!

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!

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!

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!

offirko commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Aug 20, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

AnotherButler commented Aug 20, 2018

Uh oh!

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

Uh oh!

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

jamesbeyond commented Aug 21, 2018

Uh oh!

AnotherButler commented Aug 21, 2018

Uh oh!

maciejbocianski commented Aug 22, 2018

Uh oh!

0xc0170 commented Aug 22, 2018

Uh oh!

maciejbocianski commented Aug 14, 2018 •

edited

Loading

maciejbocianski Aug 21, 2018 •

edited

Loading

maciejbocianski commented Aug 21, 2018 •

edited

Loading