QSPI SFDP Flash - Block Device #1

offirko · 2018-07-29T10:17:54Z

No description provided.

offirko · 2018-07-29T10:29:23Z

Hello: @bulislaw , @donatieng , @ARMmbed/mbed-os-maintainers , @dannybenor , @davidsaada .
I'd appreciate your review of QSPIF Block Device

offirko · 2018-07-30T12:00:22Z

@maciejbocianski you're review is welcome

0xc0170 · 2018-07-30T12:00:54Z

mbed_lib.json

+{
+"name": "qspif",
+    "config": {
+        "QSPI_IO0": "NC",


we can use QSPI FLASH pins that are currently under review, then this lib can be removed?

0xc0170 · 2018-07-30T12:01:32Z

TESTS/block_device/qspif/main.cpp

+     TEST_ASSERT_EQUAL(0, err);
+
+     _mutex->lock();
+     printf("read  %0*llx:%llu ", addrwidth, block, block_size);


Assuming these debug prints will be removed later

indeed will be removed later on

0xc0170 · 2018-07-30T12:02:09Z

TESTS/block_device/qspif/main.cpp

+// Mutex is also protecting printouts for clear logs.
+// Mutex is NOT protecting Block Device actions: erase/program/read - which is the purpose of the multithreaded test!
+void basic_erase_program_read_test(
+		QSPIFBlockDevice &blockD,


I would recommend using Mbed OS astyle configuration and run astyle (use the codestyle for this repository)

I have used astyle.. must have missed something. Will fix it

0xc0170 · 2018-07-30T12:03:04Z

QSPIFBlockDevice.h

+                                          size_t tx_length, const char *rx_buffer, size_t rx_length);
+
+    // Send Bus configure_format command to Driver
+    qspi_status_t _qspiConfiureFormat(qspi_bus_width_t inst_width, qspi_bus_width_t address_width,


_qspiConfiureFormat typo -> _qspiConfigureFormat

0xc0170 · 2018-07-30T12:04:35Z

QSPIFBlockDevice.h

+
+
+    // Soft Reset Flash Memory
+    int _resetFlashMem();


use all_small rather than camel case for function and variable names. Same as above , the public functions. Even tough these are private helpers , should follow

0xc0170 · 2018-07-30T12:05:32Z

QSPIFBlockDevice.cpp

+
+#include "QSPIFBlockDevice.h"
+#include <string.h>
+#include <mbed_wait_api.h>


use "" for this header file

donatieng

Thanks for sharing @offirko - I haven't had the time to look into the implementation but here are my comments on the API

donatieng · 2018-07-30T15:49:28Z

QSPIFBlockDevice.h

+     *  @param freq Clock frequency of the QSPI bus (defaults to 40MHz)
+     *
+     */
+    QSPIFBlockDevice(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, PinName csel, int clock_mode,


Can we have use an enum for clock mode?

You're right, though I would have expected the driver/hal to expose such an enum...they haven't.
I'll add an enum.

donatieng · 2018-07-30T15:49:58Z

QSPIFBlockDevice.h

+     *
+     */
+    QSPIFBlockDevice(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, PinName csel, int clock_mode,
+                     int freq = 40000000);


Any reason why you're using a signed int?

Matching BD to the existing QSPI driver API, didn't wont to risk unnecessary casting:
qspi_status_t set_frequency(int hz);

donatieng · 2018-07-30T15:50:40Z

QSPIFBlockDevice.h

+     *                  QSPIF_BD_ERROR_READY_FAILED - Waiting for Memory ready failed or timedout
+     *                  QSPIF_BD_ERROR_PARSING_FAILED - unexpected format or values in one of the SFDP tables
+     */
+    virtual int init();


Return type should be qspif_bd_error

Please note this virtual function and its signature is inherited from existing 'BlockDevice' base class.
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/bd/BlockDevice.h

donatieng · 2018-07-30T15:57:14Z

QSPIFBlockDevice.h

+     *                  QSPIF_BD_ERROR_WREN_FAILED - Write Enable failed
+     *                  QSPIF_BD_ERROR_PARSING_FAILED - unexpected format or values in one of the SFDP tables
+     */
+    virtual int program(const void *buffer, bd_addr_t addr, bd_size_t size);


"Program" sounds a bit odd (as opposed to write). Is that standard terminology?

Same as above, this function is inherited from existing 'BlockDevice' base class.

donatieng · 2018-07-30T15:59:58Z

QSPIFBlockDevice.h

+     *  @return         Size of a minimal erase block, common to all regions, in bytes
+     *  @note Must be a multiple of the program size
+     */
+    virtual bd_size_t get_erase_size() const;


Is that possible that a block erase size won't be a multiple of other ones? (e.g 1 block is 8kB, another one is 6kB for some reason)

SFDP standard dictates erase size to be given as 2^N. So, the different sizes will always be a multiple of other ones.
Still, for this function it is possible for one region, for example, to support minimal size of 32KB erase size, and other regions to support minimal erase size of 4KB - and then they have no common erase size.

donatieng · 2018-07-30T16:00:17Z

QSPIFBlockDevice.h

+     *  @note Must be a multiple of the program size
+     */
+    //virtual bd_size_t get_erase_size(bd_addr_t addr) const;
+    virtual bd_size_t get_erase_size(bd_addr_t addr);


How do I know the start address of the block?

The given address can be any offset within the Flash size. I'll make it clear in the remarks.

donatieng · 2018-07-30T16:00:46Z

QSPIFBlockDevice.h

+    /*   Calls to QSPI Driver APIs  */
+    /********************************/
+    // Send Program => Write command to Driver
+    qspi_status_t _qspi_send_program_command(unsigned int prog_instruction, const void *buffer, bd_addr_t addr,


Shouldn't instructions fit within one byte (uint8_t)?

You're correct, but I was matching the BD code to the existing lower levels QSPI driver where the instructions are 'unsigned int':
qspi_status_t write(unsigned int instruction, unsigned int alt, unsigned int address, const char *tx_buffer, size_t *tx_length);

donatieng · 2018-07-30T16:40:05Z

QSPIFBlockDevice.h

+     *                  QSPIF_BD_ERROR_WREN_FAILED - Write Enable failed
+     *                  QSPIF_BD_ERROR_PARSING_FAILED - unexpected format or values in one of the SFDP tables
+     */
+    virtual int erase(bd_addr_t addr, bd_size_t size);


Erasing is a very long operation - would it make sense to have an asynchronous API as well? Same for writing.

Currently BlockDevice base class does not support Async APIs.
I believe what you suggest would make sense. I think it should be raised as new requirement to add Async BlockDevice async APIs, or maybe add a different base class 'AsyncBlockDevice'.
It would require architecture and design.
Once this API is added at base class, other BlockDevices (SD, SPI, QSPI,..) could adapt.

donatieng · 2018-07-30T16:43:55Z

QSPIFBlockDevice.h

+    bool _is_initialized;
+
+    // mutex is required for sequence of driver commands that can not be disrupted
+    static SingletonPtr<PlatformMutex> _mutex;


Using a singleton mutex across all instances means you cannot use two QSPI flash chips concurrently. Shouldn't "transactions" be a requirement on the driver? (I imagine something like _qspi.lock())

_qspi.lock is implemented in driver level to protect single driver operations: e.g. write .
BD lock is protecting from a sequence of operations that must be carried sequentially: e.g. (a) _set_write_enable , (b) write, (c) _is_mem_ready . The QSPI driver requires these 3 operations to be carried with no interruption.

@donatieng - after further discussions, the BD mutex was moved to instance level to support multiple QSPI flash chips, as you suggested - Thanks!

It looks like the QSPI driver itself has exposed lock/unlock functions:
https://github.com/ARMmbed/mbed-os/blob/feature-qspi/drivers/QSPI.h#L174-L176

Should we actually be using those?

@geky - QSPI's lock is protecting the bus driver. QSPIF BD lock is protecting the Flash device:
e.g.

QSPI's lock will lock 3 times, each of: Set Write Enable, Send Write Command, Register Read
QSPIF BD lock will lock "program" command for that flash device (on other threads) that contains all of the above 3 because another flash command, such as read, can not be handled in between (a second QSPI Flash device can use the bus driver in between the first flash device commands though)

maciejbocianski · 2018-08-01T06:18:09Z

@offirko
Are we going to have API for QSPI read/write mode configuration (1-1-1/1-2-2/...)?
What will allow to choose what read/write speed/width we want to use

list<mode> get_supported_write_modes() from SFDP
list<mode> get_supported_read_modes() from SFDP
bool set_write_mode(mode)
bool set_read_mode(mode)

offirko · 2018-08-01T08:56:29Z

@maciejbocianski

Are we going to have API for QSPI read/write mode configuration (1-1-1/1-2-2/...)?

According to the design we decided not to add this flexibility.
An internal logic selects the "fastest" mode from the supported ones (as described in SFDP Tables)
See Section 3.5 at the design doc link:
https://armh.sharepoint.com/:w:/r/sites/IoT-Project-Morpheus/Shared%20Documents/design-docs/QSPI%20Block%20Device%201.0.1.docx?d=w70bb086fb2be4d0e92c62a42040197dc&csf=1&e=2UlrqT

davidsaada

Looks very good. Some minor changes required.

davidsaada · 2018-08-01T09:05:22Z

QSPIFBlockDevice.cpp

+    // Soft Reset
+    if ( -1 == _reset_flash_mem()) {
+        QSPIF_LOG(QSPIF_DEBUG_INFO, "ERROR: init - Unable to initialize flash memory, tests failed\n");
+        return QSPIF_BD_ERROR_DEVICE_ERROR;


Forgot to unlock the mutex (here, and in all following error cases).

missed this one - Thanks! I'll add an exit point for errors with unlocking

davidsaada · 2018-08-01T09:13:20Z

TESTS/block_device/qspif/main.cpp

@@ -0,0 +1,339 @@
+#include "mbed.h"


Is this needed if use the mbed namespace?

I checked again, both are not required - I'll remove

davidsaada · 2018-08-01T09:19:39Z

TESTS/block_device/qspif/main.cpp

+    rtos::Thread first_qspif_bd_thread;
+    rtos::Thread second_qspif_bd_thread;
+    rtos::Thread third_qspif_bd_thread;
+


Why not use an array and loop here?

I thought 3 would be enough (-:
I'll upgrade to an array

davidsaada · 2018-08-01T09:30:55Z

TESTS/block_device/qspif/main.cpp

+    }
+
+    bd_size_t block_size = blockD.get_erase_size();
+    uint8_t *write_block = new uint8_t[block_size];


I don't know how large this block size can get. If it's large, the test can fail on low memory boards. In this case, would suggest to add std::nothrow to the allocation. This can be used as a good reference.

It can get quite large (Typically 4KB) - I'll add 'std::nothrow' - thanks

offirko · 2018-08-01T13:49:01Z

@0xc0170 , @donatieng , @davidsaada

I related to your comments and pushed changes related to them.
Please re-review - thanks.

davidsaada

Looks good to me.

0xc0170 · 2018-08-02T11:36:04Z

TESTS/block_device/qspif/main.cpp

@@ -0,0 +1,368 @@
+#include "greentea-client/test_env.h"


Add license header here on the top of the file

0xc0170 · 2018-08-02T11:36:24Z

QSPIFBlockDevice.cpp

@@ -0,0 +1,1263 @@
+
+/* mbed Microcontroller Library
+ * Copyright (c) 2016 ARM Limited


0xc0170 · 2018-08-02T11:36:57Z

QSPIFBlockDevice.cpp

+SingletonPtr<PlatformMutex> QSPIFBlockDevice::_mutex;
+
+// Local Function
+int localMathPower(int base, int exp);


static missing here if local only

should be local_math_power

0xc0170 · 2018-08-02T11:37:45Z

QSPIFBlockDevice.cpp

+#define QSPIF_LOG(level,...) {\
+    	if (level <= QSPIF_DEFAULT_DEBUG_LEVEL) {\
+    	  char str[256];\
+      	  sprintf(str, "\n[%s][%s:%d], ", __FILENAME__, __FUNCTION__, __LINE__);\


misaligned line 113 and 115

0xc0170 · 2018-08-02T11:38:15Z

QSPIFBlockDevice.cpp

+};
+
+// Remove Remark To Enalbe Logs
+//#define QSPIF_ENABLE_LOGS 1


Rather than touching sources, use config ? The default one would have this set as disabled?

@0xc0170 , please advise: What is the common practice in mbed-os? I would prefer to enable logs at either Warning or Error level? also, can you pls refer me to a good configuration example?

@geky @davidsaada Do you have anything similar in filesystem somewhere - logging ?

LittleFS adopted custom macros because unfortunately there hasn't been a common practice for logging. (I need to go adopt mbed-trace at some point).

Here's the config for LittleFS, you're welcome to the same. It makes it easy to change if the OS logging APIs change:

config

implementation

@geky - thanks!! , eventually with Martin's help I adopted mbed-trace

0xc0170 · 2018-08-02T11:45:48Z

QSPIFBlockDevice.cpp

+/*********************************************/
+/************* Utility Functions *************/
+/*********************************************/
+int QSPIFBlockDevice::_utils_find_addr_region(int offset)


address is defined as bd_address_t, so why here is int and then cast everywhere in this file?

fixed, thanks

0xc0170 · 2018-08-02T11:46:46Z

QSPIFBlockDevice.cpp

+        bd_size_t size)
+{
+    // Send Read command to device driver
+    size_t bufLen = size;


buf_len , same as read_inst in this scope

0xc0170 · 2018-08-02T11:47:52Z

QSPIFBlockDevice.cpp

+        int dummy_cycles)
+{
+    // Configure QSPI driver Bus format
+    _qspi.configure_format(inst_width, address_width, address_size, alt_width, alt_size, data_width, dummy_cycles);


Why is the return value ignored and always returns OK (line 1242) ?

I saw that driver's 'configure_format' always return OK so I thought not to take its returned value.. Its my mistake I'll take it, just in case the driver changes one day.

0xc0170 · 2018-08-02T11:49:37Z

QSPIFBlockDevice.h

+    unsigned int _erase_type_size_arr[MAX_NUM_OF_ERASE_TYPES];
+
+    // Sector Regions Map
+    int _regions_count; //number of regions


why these are int ? compare to command instructions for instance t hat are unsigned int ?

command instructions are defined as 'unsigned int' in driver level, I didnt want to change it to avoid lots of casting. Here I chose 'int' just because its the simplest, it matches the purpose with no impact.

0xc0170 · 2018-08-02T11:51:57Z

TESTS/block_device/qspif/main.cpp

+        for (int i_ind = 0; i_ind < 15; i_ind++) {
+            printf("%02x", read_block[block_size - 16 + i_ind]);
+        }
+        printf("...\n");


Test should minimize debug output by default (these debug msg are important but only if something in the test goes wrong). Instead of printf, use utest_printf

Will fix - thanks

offirko · 2018-08-02T17:15:45Z

@0xc0170 - I added the changes related to your comments, please review - thanks.

geky · 2018-08-02T18:52:59Z

QSPIFBlockDevice.cpp

@@ -433,7 +406,7 @@ int QSPIFBlockDevice::erase(bd_addr_t addr, bd_size_t inSize)
        }
        wait_ms(QSPIF_DEFAULT_TIMEOUT_MSEC);
        if ( false == _is_mem_ready()) {
-            QSPIF_LOG(QSPIF_DEBUG_ERROR, "ERROR: QSPI After Erase Device not ready - failed\n");
+            tr_error("ERROR: QSPI After Erase Device not ready - failed\n");
            erase_failed = true;
            status = QSPIF_BD_ERROR_READY_FAILED;
            goto Exit_Point;


nit: @0xc0170 do we have a naming convention for labels? I would expect them to be similar to functions.

not specifically but should be similar

I'll fix to match convention

geky

This looks great 👍

QSPI will be a great feature to have

geky · 2018-08-02T19:25:09Z

QSPIFBlockDevice.cpp

+
+    if (QSPI_STATUS_OK != _qsp_set_frequency(freq)) {
+        tr_error("ERROR: QSPI Set Frequency Failed");
+        tr_error("ERROR: QSPI Set Frequency Failed");


Intentionally duplicated?

late night typo (-:

geky · 2018-08-02T19:30:45Z

QSPIFBlockDevice.cpp

+        }
+        tr_debug("/nDEBUG: Read Bus Mode set to 1-1-1, Instruction: 0x%xh", _read_instruction);
+        is_done = true;
+    } while (is_done == false);


I'm not sure what the "is_done" variable is doing, it looks like this is always set to true? Would it be better as a series of goto statements?

Or you could just make this while (false);

Its just an easy way to avoid multiple returns (or multiple gotos).
I believe, when possible:

'goto' should be avoided

function should have minimum 'return' points

I'll change it to 'while (false);' to avoid confusions.

geky · 2018-08-02T19:32:41Z

Out of curiousity, how much code size does the library cost?

offirko · 2018-08-05T08:48:25Z

@geky , Thanks for your remarks!
Code size is 5Kbyte compiled with GCC_ARM

donatieng

Thanks @offirko, that singleton mutex was my main remaining concern so thanks for addressing it :). Great stuff.

0xc0170

Only minor style changes, the rest LGTM

0xc0170 · 2018-08-07T09:42:12Z

QSPIFBlockDevice.cpp

+
+namespace mbed {
+
+enum qspif_default_instructions {


where are these used ? I can't locate any code referencing this enum

@0xc0170 - removed the unused ones -thanks

0xc0170 · 2018-08-07T09:43:36Z

QSPIFBlockDevice.cpp

+
+int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
+{
+


can you remove empty lines like this, 247-248 at least

done for entire file

0xc0170 · 2018-08-07T09:44:01Z

QSPIFBlockDevice.cpp

+    int status = QSPIF_BD_ERROR_OK;
+    uint32_t offset = 0;
+    uint32_t chunk = 0;
+    bd_size_t writtenBytes = 0;


written_bytes

0xc0170 · 2018-08-07T09:52:20Z

QSPIFBlockDevice.cpp

+}
+
+
+qspi_status_t QSPIFBlockDevice::_qspi_send_erase_command(unsigned int eraseInst, bd_addr_t addr, bd_size_t size)


erase_inst

0xc0170 · 2018-08-07T09:53:43Z

README.md

+#include "QSPIFBlockDevice.h"
+
+QSPIFBlockDevice blockD(MBED_CONF_QSPIF_QSPI_IO0,MBED_CONF_QSPIF_QSPI_IO1,MBED_CONF_QSPIF_QSPI_IO2,MBED_CONF_QSPIF_QSPI_IO3,


block_d or block_qspi

0xc0170 · 2018-08-07T09:54:20Z

TESTS/block_device/qspif/main.cpp

+// Mutex is also protecting printouts for clear logs.
+// Mutex is NOT protecting Block Device actions: erase/program/read - which is the purpose of the multithreaded test!
+void basic_erase_program_read_test(
+    QSPIFBlockDevice& blockD,


fine to have all args on the line 46

@0xc0170 - moved all to one line - fixed all comments thanks.

Offir Kochalsky added 6 commits July 25, 2018 11:37

Initial QSPIF BD commit

356cf9f

Added more Tests

0753471

Placing Tests in correct folder

9fd5cc6

TESTS instead TEST

91c94f3

Adding Readme.md documentation

205f957

Code Styling tool applied

62eab25

Corrected target at json F413 and Not L413

6227a45

0xc0170 suggested changes Jul 30, 2018

View reviewed changes

First Changes after PR Remarks

e5b0e7e

donatieng suggested changes Jul 30, 2018

View reviewed changes

Offir Kochalsky added 3 commits July 31, 2018 14:42

Adding PR review changes #2

3a4bef2

Astyle Changes for PR review #2 changes

e22a7d6

DISCO L475 deafult frequency is 8MHz for Low Power

34fe5f4

davidsaada suggested changes Aug 1, 2018

View reviewed changes

davidsaada reviewed Aug 1, 2018

View reviewed changes

Changes following 3rd PR review remarks

10c84a6

davidsaada approved these changes Aug 1, 2018

View reviewed changes

0xc0170 suggested changes Aug 2, 2018

View reviewed changes

Offir Kochalsky added 2 commits August 2, 2018 17:32

Changes after 4th PR remarks

0c47213

Replaced QSPIF_LOG with mbed_trace

84ce69b

geky reviewed Aug 2, 2018

View reviewed changes

geky approved these changes Aug 2, 2018

View reviewed changes

Offir Kochalsky added 4 commits August 5, 2018 11:49

After CR review remarks #5

f19b257

Is mem ready non busy loop

003565c

CR changes #6

373108a

Support Multiple QSPIF Flash Devices on same QSPI bus

7554071

geky approved these changes Aug 6, 2018

View reviewed changes

donatieng approved these changes Aug 6, 2018

View reviewed changes

0xc0170 approved these changes Aug 7, 2018

View reviewed changes

CR changes #7

7b8c0a4

offirko merged commit 98e6694 into master Aug 7, 2018

maciejbocianski mentioned this pull request Aug 21, 2018

Add QSPI ARMmbed/mbed-os#7783

Merged

		@@ -0,0 +1,1263 @@

		/* mbed Microcontroller Library
		* Copyright (c) 2016 ARM Limited


		int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
		{

		}


		qspi_status_t QSPIFBlockDevice::_qspi_send_erase_command(unsigned int eraseInst, bd_addr_t addr, bd_size_t size)

		#include "QSPIFBlockDevice.h"

		QSPIFBlockDevice blockD(MBED_CONF_QSPIF_QSPI_IO0,MBED_CONF_QSPIF_QSPI_IO1,MBED_CONF_QSPIF_QSPI_IO2,MBED_CONF_QSPIF_QSPI_IO3,

QSPI SFDP Flash - Block Device #1

QSPI SFDP Flash - Block Device #1

Uh oh!

Conversation

offirko commented Jul 29, 2018

Uh oh!

offirko commented Jul 29, 2018

Uh oh!

offirko commented Jul 30, 2018

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!

donatieng left a comment

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!

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 commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

offirko commented Aug 1, 2018

Uh oh!

davidsaada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

maciejbocianski commented Aug 1, 2018 •

edited

Loading