Add FLASHIAP component to DISCO_H747 #11619

JanneKiiskila · 2019-10-03T10:15:30Z

Description

FLASHIAP component/capability is needed for DeviceKey functionality
and also for Pelion enablement - firmware update uses this feature, too.

Related to issue 11617.

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

Reviewers

@ARMmbed/team-st-mcd @jeromecoutant @adbridge

Release Notes

Enable FLASHIAP component on ST DISCO_H747I.

jeromecoutant

target	platform_name	test suite	result	elapsed_time (sec)	copy_method
DISCO_H747I-ARMC6	DISCO_H747I	tests-mbed_drivers-flashiap	OK	38.6	default
DISCO_H747I-ARMC6	DISCO_H747I	features-device_key-tests-device_key-functionality	FAIL	44.45	default
DISCO_H747I-ARMC6	DISCO_H747I	features-storage-tests-kvstore-direct_access_devicekey_test	FAIL	28.1	default

targets/targets.json

JanneKiiskila · 2019-10-03T10:55:32Z

Thanks for running the tests. I think we need now additional commits here to get the tests to pass. We need these functionality to enable the Pelion Device Management Client.

ciarmcom · 2019-10-03T11:00:29Z

@JanneKiiskila, thank you for your changes.
@adbridge @ARMmbed/mbed-os-maintainers please review.

JanneKiiskila · 2019-10-03T12:43:46Z

From my point of view this can be merged in, it definitely takes us forward (but something more is still needed for DeviceKey, but that can be a separate PR anyways).

jeromecoutant · 2019-10-03T14:14:48Z

features/storage/kvstore/conf/global/mbed_lib.json

@@ -20,6 +20,9 @@
        "NUCLEO_F429ZI": {
            "storage_type": "TDB_INTERNAL"
        },
+        "DISCO_H747I": {
+            "storage_type": "TDB_INTERNAL"


Maybe it will be easier to set the default value to TDB_INTERNAL ?
instead of overriding each target one by one ?

@SeppoTakalo - we should consider this?

Yes, apparently many modules are assuming that KVStore "just works", so we need to probably default to TDB_INTERNAL and silently steal some flash away from the app.

But of course it needs to be configurable, so that location can be overwritten.

jeromecoutant · 2019-10-03T14:25:52Z

Hi
For me test result is still FAIL even with TDB_INTERNAL patch

JanneKiiskila · 2019-10-03T14:47:28Z

Yep, tests are still failing. The tests are passing with other boards, so there's definitely something to look into here.

JanneKiiskila · 2019-10-03T14:48:37Z

@SeppoTakalo - @jeromecoutant is right, does not make sense adding TDB_INTERNAL to these one-by-one, it should be a generic "default" choice, which you may then override if you so want. Question is - where does it actually put that TDB_INTERNAL storage in the flash? How does it define the place?

Update:

Additional commit puts the default place.
However, we do have some DeviceKey tests, which force it to the END of the flash.
Which in this case will not work, more details in issue 11617.

adbridge · 2019-10-07T14:32:02Z

@jeromecoutant are you happy with the updates?

jeromecoutant · 2019-10-07T14:49:07Z

@JanneKiiskila Correct me is I am wrong.
Current status with this PR is:

FLASHIAP is now working
DEVICE_KEY tests are FAILED

You would like to merge this first patch, and follow the device key issue with #11617

features/storage/kvstore/conf/tdb_internal/mbed_lib.json

0xc0170 · 2019-10-15T14:25:30Z

@JanneKiiskila Correct me is I am wrong.
Current status with this PR is:

FLASHIAP is now working
DEVICE_KEY tests are FAILED
You would like to merge this first patch, and follow the device key issue with #11617

Was this answered, can we progress here?

JanneKiiskila · 2019-10-15T14:41:18Z

@jeromecoutant - any ETA on a possible PR from STM, I think you have done thus plus a bunch of other work, too (like Ethernet etc.)?

jeromecoutant · 2019-10-15T14:46:57Z

Yes I shared with you some dev for STM32H7,
but I couldn't pass yet any device key or Ethernet tests... :-(
Need to go back on this asap..

0xc0170 · 2019-10-18T13:38:10Z

@ARMmbed/team-st-mcd Can we proceed with this PR as it is? It's not clear to me.

jeromecoutant · 2019-10-18T13:42:59Z

@ARMmbed/team-st-mcd Can we proceed with this PR as it is? It's not clear to me.

Could we keep only the targets.json file update ?
The other updates are more linked to #11617

0xc0170 · 2019-10-28T14:11:03Z

Could we keep only the targets.json file update ?

@JanneKiiskila Shall we?

JanneKiiskila · 2019-10-28T14:41:32Z

Yes, we can do that. I would want to add though the MBED_ROM_START and SIZE and the device_name to targets. json, too.

FLASHIAP component/capability is needed for DeviceKey functionality and also for Pelion enablement - firmware update uses this feature, too.

DeviceKey needs the definition of the default storage place, define it to be TDB_INTERNAL (as for the other boards).

Place it at the end of the bank1, last two erase sectors. As erase sector is 128 kB, default size must be double of that. This can't be in bank2 as that is apparently dedicated to the M4 core. Memory map is available in; https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32H7/TARGET_STM32H747xI/flash_data.h Memory map does not have this information, but issue [11617](ARMmbed#11617) has.

This copies the approach of the STM32F7 flash driver submitted via PR ARMmbed#10248 With this change the board finally passes all of the device key tests 10/10 times correctly.

JanneKiiskila · 2019-10-30T13:27:16Z

Rebased, force pushed. The other changes are actually also required, without them the KVStore tests will not even run. With the cache fix (kudos to @kjbracey-arm) - we actually get the tests to pass now 100%, repeatedly.

LMESTM · 2019-10-30T13:43:01Z

features/storage/kvstore/conf/tdb_internal/mbed_lib.json

@@ -15,6 +15,10 @@
            "internal_size": "0x8000",
            "internal_base_address": "0x00028000"
        },
+        "DISCO_H747I": {
+            "internal_size": "256*1024",
+            "internal_base_address": "0x080C0000"


I don't like so much having this value HARD CODED.
Couldn't this be an address like
" MBED_APP_START + MBED_APP_SIZE - 256*1024 + 1 "
?
Note: I'm okay to move on wit this first implementation and try to make a later modification that makes it more dynamic ...

I wonder wha the MBED_APP_SIZE is actually here? If it's 1024-(2*128), then it would be OK. We can do that change for sure.

But at least the offsets for example, you can't give as computed things - they had to be in real numbers.

The offset you're referring should be "internal_size" right ? Basically we're allocating a region of size "internal_size" at the end of application flash memory, isn't it ? so if we get the end address, we can deduce the rest of it at least for default value. Of course application can overload to somewhere else in memory and over-write the 2 parameters.

LMESTM

LGTM - with 1 comment that could be addressed at a later stage

0xc0170 · 2019-10-30T13:45:07Z

CI started

JanneKiiskila · 2019-10-30T14:34:50Z

@SeppoTakalo - we might want to tweak also the enablement to be via the mbed_lib.json, in which case this target definition/default could be removed, too.

mbed-ci · 2019-10-30T14:53:13Z

Test run: SUCCESS

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

JanneKiiskila · 2019-10-31T08:23:36Z

Flash tests also pass;

mbedgt: test suite report:
| target              | platform_name | test suite                                                       | result | elapsed_time (sec) | copy_method |
|---------------------|---------------|------------------------------------------------------------------|--------|--------------------|-------------|
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-features-storage-tests-blockdevice-flashsim_block_device | OK     | 10.3               | default     |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | OK     | 23.5               | default     |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | OK     | 14.1               | default     |
mbedgt: test suite results: 3 OK
mbedgt: test case report:
| target              | platform_name | test suite                                                       | test case                              | passed | failed | result | elapsed_time (sec) |
|---------------------|---------------|------------------------------------------------------------------|----------------------------------------|--------|--------|--------|--------------------|
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-features-storage-tests-blockdevice-flashsim_block_device | FlashSimBlockDevice functionality test | 1      | 0      | OK     | 0.06               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | FlashIAP - init                        | 1      | 0      | OK     | 0.09               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | FlashIAP - program                     | 1      | 0      | OK     | 2.46               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | FlashIAP - program across sectors      | 1      | 0      | OK     | 1.96               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | FlashIAP - program errors              | 1      | 0      | OK     | 0.1                |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_drivers-flashiap                              | FlashIAP - timing                      | 1      | 0      | OK     | 7.9                |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | Flash - clock and cache test           | 1      | 0      | OK     | 0.08               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | Flash - erase sector                   | 1      | 0      | OK     | 1.03               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | Flash - init                           | 1      | 0      | OK     | 0.06               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | Flash - mapping alignment              | 1      | 0      | OK     | 0.05               |
| DISCO_H747I-GCC_ARM | DISCO_H747I   | mbed-os-tests-mbed_hal-flash                                     | Flash - program page                   | 1      | 0      | OK     | 1.95               |
mbedgt: test case results: 11 OK
mbedgt: completed in 48.22 sec

JanneKiiskila mentioned this pull request Oct 3, 2019

STM32H747I - DeviceKey is not enabled #11617

Closed

jeromecoutant reviewed Oct 3, 2019

View reviewed changes

targets/targets.json Outdated Show resolved Hide resolved

JanneKiiskila force-pushed the FlashIAP_DISCO_H747 branch from 5f8c1ec to 45c03bc Compare October 3, 2019 10:53

ciarmcom requested review from adbridge and a team October 3, 2019 11:00

ciarmcom added the needs: review label Oct 3, 2019

jeromecoutant reviewed Oct 3, 2019

View reviewed changes

JanneKiiskila force-pushed the FlashIAP_DISCO_H747 branch 3 times, most recently from 21e2203 to 4bf4f93 Compare October 4, 2019 15:39

jeromecoutant reviewed Oct 9, 2019

View reviewed changes

features/storage/kvstore/conf/tdb_internal/mbed_lib.json Outdated Show resolved Hide resolved

JanneKiiskila force-pushed the FlashIAP_DISCO_H747 branch from 4bf4f93 to 0dbfeea Compare October 9, 2019 09:00

jeromecoutant added a commit to jeromecoutant/mbed that referenced this pull request Oct 9, 2019

ARMmbed#11619

3fa6a6e

0xc0170 approved these changes Oct 18, 2019

View reviewed changes

Add FLASHIAP component to DISCO_H747

8e5f962

FLASHIAP component/capability is needed for DeviceKey functionality and also for Pelion enablement - firmware update uses this feature, too.

Janne Kiiskila added 3 commits October 30, 2019 15:25

DISCO_H747I - define default storage (TDB_INTERNAL)

655274a

DeviceKey needs the definition of the default storage place, define it to be TDB_INTERNAL (as for the other boards).

Fix for the H747 flash driver / cache cleaning

a485001

This copies the approach of the STM32F7 flash driver submitted via PR ARMmbed#10248 With this change the board finally passes all of the device key tests 10/10 times correctly.

JanneKiiskila force-pushed the FlashIAP_DISCO_H747 branch from 0dbfeea to a485001 Compare October 30, 2019 13:26

LMESTM reviewed Oct 30, 2019

View reviewed changes

LMESTM approved these changes Oct 30, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Oct 30, 2019

0xc0170 added the release-version: 5.14.2 label Oct 30, 2019

0xc0170 removed the needs: CI label Oct 30, 2019

0xc0170 merged commit cbf9f06 into ARMmbed:master Oct 30, 2019

jeromecoutant mentioned this pull request Oct 31, 2019

STM32H7: code and feature alignment for both NUCLEO and DISCO targets #11789

Merged

jeromecoutant mentioned this pull request Nov 18, 2019

STM32H7 FLASH API issue with M4 core #11811

Merged

JanneKiiskila deleted the FlashIAP_DISCO_H747 branch January 21, 2022 12:46

Add FLASHIAP component to DISCO_H747 #11619

Add FLASHIAP component to DISCO_H747 #11619

Uh oh!

Conversation

JanneKiiskila commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewers

Release Notes

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JanneKiiskila commented Oct 3, 2019

Uh oh!

ciarmcom commented Oct 3, 2019

Uh oh!

JanneKiiskila commented Oct 3, 2019

Uh oh!

jeromecoutant Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

JanneKiiskila Oct 9, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo Oct 9, 2019

Choose a reason for hiding this comment

Uh oh!

jeromecoutant commented Oct 3, 2019

Uh oh!

JanneKiiskila commented Oct 3, 2019

Uh oh!

JanneKiiskila commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Oct 7, 2019

Uh oh!

jeromecoutant commented Oct 7, 2019

Uh oh!

Uh oh!

0xc0170 commented Oct 15, 2019

Uh oh!

JanneKiiskila commented Oct 15, 2019

Uh oh!

jeromecoutant commented Oct 15, 2019

Uh oh!

0xc0170 commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromecoutant commented Oct 18, 2019

Uh oh!

0xc0170 commented Oct 28, 2019

Uh oh!

JanneKiiskila commented Oct 28, 2019

Uh oh!

JanneKiiskila commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LMESTM Oct 30, 2019 • 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!

JanneKiiskila Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

JanneKiiskila Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

LMESTM Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

LMESTM left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 30, 2019

Uh oh!

JanneKiiskila commented Oct 30, 2019

Uh oh!

mbed-ci commented Oct 30, 2019

JanneKiiskila commented Oct 3, 2019 •

edited

Loading

JanneKiiskila commented Oct 3, 2019 •

edited

Loading

0xc0170 commented Oct 18, 2019 •

edited

Loading

JanneKiiskila commented Oct 30, 2019 •

edited

Loading

LMESTM Oct 30, 2019 •

edited

Loading