QSPI : Define default pins at drivers level #11444

jeromecoutant · 2019-09-09T16:21:22Z

Description

I had to make some tests with an existing MBED target and an external QSPI,
and it was not so easy, many patches were needed in several part.

This proposition is then to be able to make it easier.

QSPI and QSPIF tests are now possible only with mbed_app.json update:

Example:

        "NUCLEO_L4R5ZI": {
            "target.device_has_add": ["QSPI"],
            "target.components": ["QSPIF"],
            "drivers.qspi_io0": "PE_12",
            "drivers.qspi_io1": "PB_0",
            "drivers.qspi_io2": "PE_14",
            "drivers.qspi_io3": "PE_15",
            "drivers.qspi_sck": "PB_10",
            "drivers.qspi_csn": "PA_2",
            "target.extra_labels_add": ["MX25R6435F"]
        },

Addition of new board with some already defined QSPI memory is now very easy!

There is no change for existing targets with embedded QSPI.

Pull request type

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

Reviewers

Release Notes

ciarmcom · 2019-09-09T17:00:26Z

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-hal @ARMmbed/mbed-os-core @ARMmbed/mbed-os-test @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen · 2019-09-10T08:02:17Z

TESTS/mbed_hal/qspi/flash_configs/flash_configs.h

@@ -17,6 +17,16 @@
 #ifndef MBED_FLASH_CONFIGS_H
 #define MBED_FLASH_CONFIGS_H

+#define MX25R6435F 1


Why this would need to be always enabled?

0xc0170 · 2019-09-10T08:14:36Z

drivers/mbed_lib.json

@@ -12,6 +12,24 @@
        "spi_count_max": {
            "help": "The maximum number of SPI peripherals used at the same time. Determines RAM allocated for SPI peripheral management. If null, limit determined by hardware.",
            "value": null
+        },
+        "qspi_io0": {


We have QSPI_FLASH1_IO0 and now introducing QSPI_FLASH1_IO0 ?

Why are not these in PinNames and defined for all boards having QSPI? There are QSPI pins or not? They should provide default pins, that could be overriden in tests/apps via config. Is this what is it adding?

Goal is to use an external QSPI,
and add pins value in mbed_app.json file

I understand now. This is a new approach here in this drivers config. This brings QSPI definitions even for a board that does not have it. Shouldn' t this be by default null, not even created if not used , and a target would override it and set?

@maciejbocianski what do you think?

Goal of this approach is to be full compatible with current implementation.
There is no impact with current targets with and without QSPI pins

The only impact having QSPI defined always even if QSPI is not available?

4 new define added and not used in the mbed_config.h file

maciejbocianski · 2019-09-10T08:57:33Z

TESTS/mbed_hal/qspi/main.cpp

+#define QPIN_2 static_cast<PinName>(MBED_CONF_DRIVERS_QSPI_IO2)
+#define QPIN_3 static_cast<PinName>(MBED_CONF_DRIVERS_QSPI_IO3)
+#define QSCK   static_cast<PinName>(MBED_CONF_DRIVERS_QSPI_SCK)
+#define QCSN   static_cast<PinName>(MBED_CONF_DRIVERS_QSPI_CSN)


It was designed that tests-mbed_hal-qspi uses default qspi interface available on dev boards.
This should look rather like below to maintain hal test working:

#ifdef MBED_CONF_DRIVERS_QSPI_IO0 #define QPIN_0 static_cast<PinName>(MBED_CONF_DRIVERS_QSPI_IO0) #else #define QPIN_0 static_cast<PinName>(QSPI_FLASH1_IO0) #endif

I don't see the difference as MBED_CONF_DRIVERS_QSPI_IO0 is QSPI_FLASH1_IO0...

jeromecoutant · 2019-09-10T09:18:40Z

I split the proposition in few commits with more comments in order to be more explicit.

0xc0170 · 2019-09-10T11:57:58Z

@ARMmbed/mbed-os-hal Please review if drivers config should be extended to provide QSPI pins.

@maciejbocianski What is the current way to provide these pins, besides defining them in PinNames.h (using on board flash) but what about external flash with our tests?

adbridge · 2019-09-27T15:17:38Z

@bulislaw @evedon could you please review on behalf of the hal team? (@donatieng fyi)

jeromecoutant · 2019-10-02T11:19:04Z

Hi
Some QSPI development are still ongoing. Please merge this PR in order to make mbed easy to use.
Thx
@MarceloSalazar

VeijoPesonen

Looks good to me.

bulislaw

Hi Jerome, I think I'm missing something. The tests has been updated to use the new config MBED_CONF_DRIVERS_QSPI_XXX, but it's not set by default to anything and the code wasn't change to link the existing (built in QSPI chips) accessible via QSPI_FLASH1_XXX to the new config.

The way users would use the code (not test) doesn't change and it's described here: https://github.com/ARMmbed/mbed-os/blob/master/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h#L50 no matter if they use builtin or external chip.

For tests things should still work by default with built in QSPI, but it should be possible to reconfigure to use external, I agree here. What i'm missing is how this change works with default built in memories. So where MBED_CONF_DRIVERS_QSPI_IO0 defaults to QSPI_FLASH1_IO0.

bulislaw · 2019-10-03T11:21:04Z

drivers/mbed_lib.json

@@ -12,6 +12,24 @@
        "spi_count_max": {
            "help": "The maximum number of SPI peripherals used at the same time. Determines RAM allocated for SPI peripheral management. If null, limit determined by hardware.",
            "value": null
+        },
+        "qspi_io0": {


Help text is missing.

jeromecoutant · 2019-10-03T12:38:02Z

@bulislaw
QSPIF test was updated, QSPI test is still difficult to execute.

jeromecoutant · 2019-10-03T12:54:30Z

Help added in the json file

Rebase also done

bulislaw · 2019-10-03T13:31:08Z

components/storage/blockdevice/COMPONENT_QSPIF/mbed_lib.json

-        "QSPI_IO3": "QSPI_FLASH1_IO3",
-        "QSPI_SCK": "QSPI_FLASH1_SCK",
-        "QSPI_CSN": "QSPI_FLASH1_CSN",
+        "QSPI_IO0": "MBED_CONF_DRIVERS_QSPI_IO0",


@jeromecoutant what I'm saying is these new configs like MBED_CONF_DRIVERS_QSPI_IO0 are not defined by default as far as i can tell, while the old ones like QSPI_FLASH1_IO0 were defined as part of pinmap. Am I missing something?

MBED_CONF_DRIVERS_QSPI_IO0 is set by default to QSPI_FLASH1_IO0,
so no impact for QSPIF

Difference, now, is that when user adds some external QSPI memory, he only has to update his mbed_app.json file. Then he could execute QSPIF AND QSPI tests.

Thx

Looks it work
mbed_config.h:

// Automatically generated configuration file. // DO NOT EDIT, content will be overwritten. #ifndef __MBED_CONFIG_DATA__ #define __MBED_CONFIG_DATA__ // Configuration parameters ... #define MBED_CONF_DRIVERS_QSPI_CSN QSPI_FLASH1_CSN // set by library:drivers #define MBED_CONF_DRIVERS_QSPI_IO0 QSPI_FLASH1_IO0 // set by library:drivers #define MBED_CONF_DRIVERS_QSPI_IO1 QSPI_FLASH1_IO1 // set by library:drivers #define MBED_CONF_DRIVERS_QSPI_IO2 QSPI_FLASH1_IO2 // set by library:drivers #define MBED_CONF_DRIVERS_QSPI_IO3 QSPI_FLASH1_IO3 // set by library:drivers #define MBED_CONF_DRIVERS_QSPI_SCK QSPI_FLASH1_SCK // set by library:drivers ...

adbridge · 2019-10-07T10:43:26Z

CI started

mbed-ci · 2019-10-07T11:51:53Z

Test run: SUCCESS

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

0xc0170 · 2019-10-14T12:48:20Z

astyle error - please fix,

Now, these pins can be used everywhere (MBED_CONF_DRIVERS_QSPI_xxx) and be redefined in local mbed_app.json (no more need to patch PinNames.h file)

This makes new board with the same QSPI memory addition.

0xc0170 · 2019-10-18T12:53:22Z

CI started

Please @maciejbocianski review

mbed-ci · 2019-10-18T16:51:06Z

Test run: SUCCESS

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

0xc0170 · 2019-10-21T07:40:51Z

As soon as @maciejbocianski approves, this can go in.

maciejbocianski · 2019-10-21T13:10:05Z

@0xc0170
The idea for qspi pins was QSPI_FLASHn_IO0 for on board flashes
and QSPIn_IO0 for qspi pins routed out.
So there is always need of manual configuration when testing external qspi flash

maciejbocianski

LGTM

0xc0170 · 2019-10-21T13:19:31Z

The idea for qspi pins was QSPI_FLASHn_IO0 for on board flashes
and QSPIn_IO0 for qspi pins routed out.
So there is always need of manual configuration when testing external qspi flash

IS this captured in our docs, for instance in QSPI driver docs? If not , it should !

isaev-d · 2020-11-03T17:58:34Z

@jeromecoutant you save my evening.

ciarmcom requested review from a team September 9, 2019 17:00

ciarmcom added the needs: review label Sep 9, 2019

VeijoPesonen reviewed Sep 10, 2019

View reviewed changes

0xc0170 reviewed Sep 10, 2019

View reviewed changes

maciejbocianski reviewed Sep 10, 2019

View reviewed changes

jeromecoutant force-pushed the PR_QSPI_EXTERNAL branch from bb825ec to 5d86560 Compare September 10, 2019 09:17

0xc0170 requested a review from a team September 10, 2019 11:52

adbridge requested a review from bulislaw September 27, 2019 15:18

VeijoPesonen approved these changes Oct 3, 2019

View reviewed changes

bulislaw reviewed Oct 3, 2019

View reviewed changes

jeromecoutant force-pushed the PR_QSPI_EXTERNAL branch from 5d86560 to 75f3afb Compare October 3, 2019 12:53

bulislaw reviewed Oct 3, 2019

View reviewed changes

adbridge added needs: CI and removed needs: review labels Oct 7, 2019

0xc0170 added needs: work and removed needs: CI labels Oct 14, 2019

jeromecoutant added 5 commits October 15, 2019 17:36

QSPI : Define default pins at drivers level

bec463d

Now, these pins can be used everywhere (MBED_CONF_DRIVERS_QSPI_xxx) and be redefined in local mbed_app.json (no more need to patch PinNames.h file)

QSPIF component should use defined QSPI pins

03837a7

QSPI test should use json defined QSPI pins

5c1c558

QSPIF frequency setting depends on QSPI memory not target.

f117d35

This makes new board with the same QSPI memory addition.

QSPI HAL test: flash configuration file selection update

23e6840

jeromecoutant force-pushed the PR_QSPI_EXTERNAL branch from 7e99230 to 23e6840 Compare October 15, 2019 15:39

0xc0170 added needs: CI and removed needs: work labels Oct 18, 2019

0xc0170 requested review from maciejbocianski and removed request for a team October 18, 2019 12:52

0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Oct 21, 2019

maciejbocianski approved these changes Oct 21, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Oct 21, 2019

0xc0170 approved these changes Oct 21, 2019

View reviewed changes

0xc0170 merged commit fe12608 into ARMmbed:master Oct 21, 2019

jeromecoutant deleted the PR_QSPI_EXTERNAL branch October 21, 2019 13:34

miteshdedhia7 mentioned this pull request Oct 21, 2019

Issues with SFDP parsing of the Sector Map Parameter Header and Table parsing #11722

Closed

0xc0170 removed the ready for merge label Oct 24, 2019

0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

This was referenced Mar 6, 2020

Issue with SFDP parsing of the Sector Map Parameter Table #12574

Closed

Issue with features-storage-tests-blockdevice-general_block_device #12575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QSPI : Define default pins at drivers level #11444

QSPI : Define default pins at drivers level #11444

jeromecoutant commented Sep 9, 2019 •

edited

Loading

ciarmcom commented Sep 9, 2019

VeijoPesonen Sep 10, 2019

0xc0170 Sep 10, 2019

jeromecoutant Sep 10, 2019

0xc0170 Sep 10, 2019

jeromecoutant Sep 10, 2019

0xc0170 Sep 10, 2019

jeromecoutant Sep 10, 2019

maciejbocianski Sep 10, 2019

jeromecoutant Sep 10, 2019

jeromecoutant commented Sep 10, 2019

0xc0170 commented Sep 10, 2019

adbridge commented Sep 27, 2019

jeromecoutant commented Oct 2, 2019

VeijoPesonen left a comment

bulislaw left a comment

bulislaw Oct 3, 2019

jeromecoutant commented Oct 3, 2019

jeromecoutant commented Oct 3, 2019

bulislaw Oct 3, 2019

jeromecoutant Oct 3, 2019

maciejbocianski Oct 21, 2019

adbridge commented Oct 7, 2019

mbed-ci commented Oct 7, 2019

0xc0170 commented Oct 14, 2019

0xc0170 commented Oct 18, 2019

mbed-ci commented Oct 18, 2019

0xc0170 commented Oct 21, 2019

maciejbocianski commented Oct 21, 2019 •

edited

Loading

maciejbocianski left a comment

0xc0170 commented Oct 21, 2019

isaev-d commented Nov 3, 2020

QSPI : Define default pins at drivers level #11444

QSPI : Define default pins at drivers level #11444

Conversation

jeromecoutant commented Sep 9, 2019 • edited Loading

Description

Pull request type

Reviewers

Release Notes

ciarmcom commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecoutant commented Sep 10, 2019

0xc0170 commented Sep 10, 2019

adbridge commented Sep 27, 2019

jeromecoutant commented Oct 2, 2019

VeijoPesonen left a comment

Choose a reason for hiding this comment

bulislaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecoutant commented Oct 3, 2019

jeromecoutant commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbridge commented Oct 7, 2019

mbed-ci commented Oct 7, 2019

0xc0170 commented Oct 14, 2019

0xc0170 commented Oct 18, 2019

mbed-ci commented Oct 18, 2019

0xc0170 commented Oct 21, 2019

maciejbocianski commented Oct 21, 2019 • edited Loading

maciejbocianski left a comment

Choose a reason for hiding this comment

0xc0170 commented Oct 21, 2019

isaev-d commented Nov 3, 2020

jeromecoutant commented Sep 9, 2019 •

edited

Loading

maciejbocianski commented Oct 21, 2019 •

edited

Loading