feature-qspi: QSPI HAL addition #5429

0xc0170 · 2017-11-03T14:18:18Z

Destination branch: feature-qspi

Adding new QSPI HAL header file. This should help to use memory-maped devices
as memories, graphical displays.

The API consist of few functions, most important are read/write/write_command functions.

The command format is:

----------------------------------------------
| Instruction | Address | Alt | Dummy | Data |
----------------------------------------------

We define only synch API at the moment. This patch contains only a header file.

We will provide an implementation for a target.

@jeromecoutant @LMESTM @kl-cruz @stevew817 @mmahadevan108 @Archcady @TomoYamanaka @jessexm @jacobjohnson-ON please have a look, might be interested in this, any feedback welcome!

cc @maclobdell @screamerbg @MarceloSalazar @ashok-rao

Adding new QSPI HAL header file. This should help to use memory-maped devices as memories, graphical displays. The API consist of few functions, most important are read/write/write_command functions. The command format is: ``` ---------------------------------------------- | Instruction | Address | Alt | Dummy | Data | ---------------------------------------------- ``` We define only synch API at the moment.

stevew817 · 2017-11-03T14:32:22Z

How does this fit in with #4229 ?

0xc0170 · 2017-11-03T14:54:45Z

How does this fit in with #4229 ?

The API proposed there is slighty different (more oriented to a file system like, program/erase). You can view the file for hal API here https://github.com/kl-cruz/mbed-os/blob/61a510a15af9b780d260b24fea33920229074144/hal/qspi_api.h. #4229 (comment) - I left there a comment regarding HAL. We can continue discussion around it here.

@stevew817 you got any driver for QSPI or even an example for any target that I can have a look at? I could not find any in the codebase, but recall there was one of GG targets that should have QSPI peripheral.

0xc0170 · 2017-11-03T14:57:39Z

For a reader it might help some references that are out there and worth looking at (I forgot to pinned this in the first comment !)

References:

https://os.mbed.com/users/capsavon/code/QSPI_DISCO_F746NG/file/f391cd8f34c1/QSPI_DISCO_F746NG.h
CHibiOS https://github.com/ChibiOS/ChibiOS/blob/dd85bcdf688ac779f157f35f0e23f489d0d466c1/os/ex/Micron/m25q.c
at91 bootloader https://github.com/jonpe960/at91bootstrap/blob/4d6b5153a6b70a5ad0b8bba582808e52ec1fb4f5/include/arch/at91_qspi.h
EALib from EmbeddedArtists https://os.mbed.com/users/embeddedartists/code/EALib/
ST reference manual for QSPI http://www.st.com/content/ccc/resource/technical/document/application_note/group0/b0/7e/46/a8/5e/c1/48/01/DM00227538/files/DM00227538.pdf/jcr:content/translations/en.DM00227538.pdf
Nordic QSPI driver https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.sdk5.v12.2.0%2Fhardware_driver_qspi.html
NXP MCUExpresso QSPI driver https://mcuxpresso.nxp.com/apidoc/2.0/group__qspi.html
Renesas QSPI driver https://www.renesas.com/en-eu/doc/products/renesas-synergy/apn/r11an0114eu0101-synergy-qspi-hal-mod-guide.pdf
Some linux QSPI drivers can be located here https://github.com/torvalds/linux/blob/master/drivers/spi (look at -qspi suffixes), nor quadspi can be found here: https://github.com/torvalds/linux/blob/3a00be19238ca330ce43abd33caac8eff343800c/drivers/mtd/spi-nor/atmel-quadspi.c#L167
Nuttx QSPI driver https://bitbucket.org/nuttx/nuttx/src/f562bf0fbed0fdec668e988306638c16d26c0ad6/include/nuttx/spi/qspi.h?at=master&fileviewer=file-view-default

stevew817 · 2017-11-06T09:12:18Z

@0xc0170 Our QSPI HAL doc lives ere: http://devtools.silabs.com/dl/documentation/doxygen/5.3.2/efm32gg11/html/group__QSPI.html
QSPI is being introduced on EFM32GG11, for which support is getting staged here: https://github.com/SiliconLabs/mbed-os/tree/feature/EFM32GG11

0xc0170 · 2017-11-06T09:34:52Z

@0xc0170 Our QSPI HAL doc lives ere: http://devtools.silabs.com/dl/documentation/doxygen/5.3.2/efm32gg11/html/group__QSPI.html

Thanks Steve, I checked the docs. The API there is similar to the one here, found some diferencies but mostly as it is structured differently (read/write data and config). The write/read config maps to our qspi_command_t, read/write plus read/write configs should be straight-forward to port to this HAL.

LMESTM · 2017-11-06T09:41:29Z

cc @adustm @bcostm

Fixing by adding NONE values for both

0xc0170 · 2017-11-07T14:38:25Z

Updated (see 2 commits for details)

bentcooke · 2017-11-07T15:16:59Z

hal/qspi_api.h

+ * @param obj QSPI object
+ * @param command QSPI command
+ * @param data TX buffer
+ * @param[in,out] in - length TX buffer length in bytes, out - number of bites written


bites->bytes

bentcooke · 2017-11-07T15:17:19Z

hal/qspi_api.h

+ * @param obj QSPI object
+ * @param command QSPI command
+ * @param data RX buffer
+ * @param[in,out] in - length RX buffer length in bytes, out - number of bites read


bites->bytes

will fix asap

bentcooke · 2017-11-07T15:18:14Z

Looks good other than the two minor corrections of bites->bytes in the comments.

This provides a way to return how many bytes have been written/read (as status codes are returned via func ret value)

bentcooke

Looks good!

ashok-rao · 2017-11-08T13:26:07Z

hal/qspi_api.h

+ * @param sclk The clock pin
+ * @param ssel The chip select pin
+ * @param hz The bus frequency
+ * @param mode SPI mode


Is this just the master/slave mode or are we referring to SPI modes 0,1,2,3? Would be nice to add a 1 line comment if it's either.

Will add Clock polarity and phase mode (0 - 3)

ashok-rao · 2017-11-08T13:29:28Z

hal/qspi_api.h

+    QSPI_STATUS_ERROR = -1, /**< Generic error >*/
+    QSPI_STATUS_INVALID_PARAMETER = -2, /**< The parameter is invalid >*/
+    QSPI_STATUS_OK    =  0, /**< Function executed sucessfully  >*/
+} qspi_status_t;


How about some specific error codes like peripheral init fail, read/write fail along with generic error and maybe we can add more as it matures (buffer issues, bus error etc)?

I would expect this might get more errors once we get some implementations. As this error is per function, I find it sufficient for cases like peripheral fail (if init returns ERROR) init phase failed. Or if write() returns error, write failed. If we got something like transfer(read+write) then write/read error would make sense.

Thanks @0xc0170 . OK with me!

SPI mode means Clock polarity and phase mode (0 - 3)

0xc0170 · 2017-11-10T13:47:22Z

/morph build

0xc0170 · 2017-11-10T14:01:11Z

I triggered CI build to move forward with this.

This goes to feature branch feature-qspi, feedback is still open anytime even if this gets integrated to the branch.

mbed-ci · 2017-11-10T15:10:54Z

Build : SUCCESS

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

Triggering tests

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

marcuschangarm · 2017-11-10T15:40:31Z

@0xc0170 this seems like a very manual API. Are there any plans for enabling memory mapped QSPI devices? For example, it would be useful to have an API for setting up an external QSPI flash chip as memory mapped internal flash.

mmahadevan108 · 2017-11-10T16:27:09Z

hal/qspi_api.h

+    struct {
+        qspi_bus_width_t bus_width; /**< Bus width for the address >*/
+        qspi_address_size_t size; /**< Address size >*/
+        uint32_t value; /**< Address value >*/


For memory mapped flash devices, do we only use the "value" element from the command structure?

Can you elaborate ? provide more details ?

Note: This so far does not support memory mapped QSPI. I am about to add a proposal below for it.

mbed-ci · 2017-11-10T17:23:19Z

Test : SUCCESS

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

mbed-ci · 2017-11-10T17:52:14Z

Exporter Build : SUCCESS

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

bcostm · 2017-11-13T10:04:31Z

It looks good. But I looked in the ST HAL QSPI driver and there are other parameters that are used during QSPI peripheral initialization and I think not covered in this API.

  uint32_t FifoThreshold;      /* Specifies the threshold number of bytes in the FIFO (used only in indirect mode)
                                  This parameter can be a value between 1 and 32 */

  uint32_t SampleShifting;     /* Specifies the Sample Shift. The data is sampled 1/2 clock cycle delay later to 
                                  take in account external signal delays. (It should be QSPI_SAMPLE_SHIFTING_NONE in DDR mode)
                                  This parameter can be a value of @ref QSPI_SampleShifting */

  uint32_t FlashSize;          /* Specifies the Flash Size. FlashSize+1 is effectively the number of address bits 
                                  required to address the flash memory. The flash capacity can be up to 4GB 
                                  (addressed using 32 bits) in indirect mode, but the addressable space in 
                                  memory-mapped mode is limited to 256MB
                                  This parameter can be a number between 0 and 31 */

  uint32_t ChipSelectHighTime; /* Specifies the Chip Select High Time. ChipSelectHighTime+1 defines the minimum number 
                                  of clock cycles which the chip select must remain high between commands.
                                  This parameter can be a value of @ref QSPI_ChipSelectHighTime */ 

  uint32_t ClockMode;          /* Specifies the Clock Mode. It indicates the level that clock takes between commands.
                                  This parameter can be a value of @ref QSPI_ClockMode */

  uint32_t FlashID;            /* Specifies the Flash which will be used,
                                  This parameter can be a value of @ref QSPI_Flash_Select */

  uint32_t DualFlash;          /* Specifies the Dual Flash Mode State
                                  This parameter can be a value of @ref QSPI_DualFlash_Mode */

So, how are we going to set them ? Add other parameters in the qspi_init function or create additional methods ?

Same thing for the qspi command. We have many other parameters and I don't know how to manage them with the actual API:

uint32_t DdrMode;            // Two values: Disabled or Enabled
uint32_t DdrHoldHalfCycle;   // Two values: Analog delay or half clock delay
uint32_t SIOOMode;          // Two values: every command or only first command

uint32_t TimeOutPeriod;      // Specifies the number of clock to wait when the FIFO is full before to release the chip select. Range 0..0xFFFF
uint32_t TimeOutActivation;  // Specifies if the time out counter is enabled to release the chip select.

0xc0170 · 2017-11-13T10:44:09Z

@0xc0170 this seems like a very manual API. Are there any plans for enabling memory mapped QSPI devices? For example, it would be useful to have an API for setting up an external QSPI flash chip as memory mapped internal flash.

@marcuschangarm Currently looking at memory mapped mode support (execute in place - XIP mode). There could be two methods that would enable/disable memory mapped mode. I'll propose a change via separate comment and what implications they have.

It looks good. But I looked in the ST HAL QSPI driver and there are other parameters that are used during QSPI peripheral initialization and I think not covered in this API.

@bcostm Thanks for the review.
These configurations are target specific, what we are capturing here is generic QSPI definitions. From the above, they are very peripheral specific and should be set to default values in HAL (if tuning required, then use direct SDK drivers API). If you find any that are common, let us know. I'll recheck them and we can have a look at their portability.

0xc0170 · 2017-11-13T15:29:44Z

The proposal for extending this to cover memory mapped QSPI (execute code in place - XIP) in HAL.

From a driver perspective, all it needs to do is enable/disable memory-mapped mode. This is usually a register in QSPI peripheral that controls this. Based on this, plus QSPI section, it would use QSPI peripheral for read if its within the defined section and it's active.

/** Enable memory-mapped mode
 *
 * Each target defines own memory section
 * that is mapped for this.
 * If a peripheral is in memory-mapped mode, it provides limited set of features. Write is not allowed, only read.
 */
qspi_status_t qspi_enable_xip(qspi_t *obj);

/** Disable memory-mapped mode
 *
 */
 qspi_status_t qspi_disable_xip(qspi_t *obj);

Each target defines QSPI specific memory section. QSPI would be used if an access is done within this defined section and memory-mapped is active.

Based on this proposal it can come as separate addition to this feature branch, or I'll add it here. Waiting for review!

0xc0170 · 2017-11-14T10:07:48Z

I'll send a new pull request with memory mapped support (the latest comment above).

This is ready for merge to the feature-branch (comments are always appreciated)

0xc0170 added the needs: review label Nov 3, 2017

0xc0170 requested review from bulislaw, bentcooke and SenRamakri November 3, 2017 14:18

0xc0170 changed the base branch from master to feature-qspi November 3, 2017 14:18

0xc0170 added scope: feature and removed scope: feature labels Nov 3, 2017

QSPI: fix address/alt variable sizes (can be skipped)

5f20b9c

Fixing by adding NONE values for both

bentcooke reviewed Nov 7, 2017

View reviewed changes

QSPI: change length to be in/out parameter

cb0959b

This provides a way to return how many bytes have been written/read (as status codes are returned via func ret value)

0xc0170 force-pushed the feature-qspi branch from 0afaf07 to cb0959b Compare November 7, 2017 15:57

bentcooke approved these changes Nov 7, 2017

View reviewed changes

ashok-rao reviewed Nov 8, 2017

View reviewed changes

0xc0170 added 2 commits November 8, 2017 13:59

QSPI: improve mode documentation

74c5858

SPI mode means Clock polarity and phase mode (0 - 3)

QSPI: fix command declaration names

13767a2

0xc0170 added needs: CI and removed needs: review labels Nov 10, 2017

mmahadevan108 reviewed Nov 10, 2017

View reviewed changes

0xc0170 removed the needs: CI label Nov 14, 2017

0xc0170 merged this pull request into ARMmbed:feature-qspi Nov 14, 2017

feature-qspi: QSPI HAL addition #5429

feature-qspi: QSPI HAL addition #5429

Uh oh!

Conversation

0xc0170 commented Nov 3, 2017

Uh oh!

stevew817 commented Nov 3, 2017

Uh oh!

0xc0170 commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevew817 commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LMESTM commented Nov 6, 2017

Uh oh!

0xc0170 commented Nov 7, 2017

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!

bentcooke commented Nov 7, 2017

Uh oh!

bentcooke 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!

0xc0170 commented Nov 10, 2017

Uh oh!

0xc0170 commented Nov 10, 2017

Uh oh!

mbed-ci commented Nov 10, 2017

Triggering tests

Uh oh!

marcuschangarm commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Nov 10, 2017

Uh oh!

mbed-ci commented Nov 10, 2017

Uh oh!

bcostm commented Nov 13, 2017

Uh oh!

0xc0170 commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 13, 2017

Uh oh!

0xc0170 commented Nov 14, 2017

Uh oh!

Uh oh!

0xc0170 commented Nov 3, 2017 •

edited

Loading

0xc0170 commented Nov 3, 2017 •

edited

Loading

stevew817 commented Nov 6, 2017 •

edited

Loading

0xc0170 commented Nov 6, 2017 •

edited

Loading

0xc0170 commented Nov 13, 2017 •

edited

Loading