Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FLASHIAP component to DISCO_H747 #11619

Merged
merged 4 commits into from Oct 30, 2019

Conversation

@JanneKiiskila
Copy link
Contributor

JanneKiiskila commented Oct 3, 2019

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.

Copy link
Contributor

jeromecoutant left a comment

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 Outdated Show resolved Hide resolved
@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:FlashIAP_DISCO_H747 branch from 5f8c1ec to 45c03bc Oct 3, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 3, 2019

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 ciarmcom requested review from adbridge and ARMmbed/mbed-os-maintainers Oct 3, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 3, 2019

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

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 3, 2019

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).

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

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Oct 3, 2019

Contributor

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

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Oct 9, 2019

Author Contributor

@SeppoTakalo - we should consider this?

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Oct 9, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Oct 3, 2019

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

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 3, 2019

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

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 3, 2019

@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.
@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:FlashIAP_DISCO_H747 branch 3 times, most recently from 21e2203 to 4bf4f93 Oct 4, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 7, 2019

@jeromecoutant are you happy with the updates?

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Oct 7, 2019

@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

@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:FlashIAP_DISCO_H747 branch from 4bf4f93 to 0dbfeea Oct 9, 2019
jeromecoutant added a commit to jeromecoutant/mbed that referenced this pull request Oct 9, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 15, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 15, 2019

@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

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

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

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Oct 18, 2019

@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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 28, 2019

Could we keep only the targets.json file update ?

@JanneKiiskila Shall we?

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 28, 2019

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](#11617) has.
This copies the approach of the STM32F7 flash driver submitted via
PR #10248

With this change the board finally passes all of the device key
tests 10/10 times correctly.
@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:FlashIAP_DISCO_H747 branch from 0dbfeea to a485001 Oct 30, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 30, 2019

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.

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

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 30, 2019

Contributor

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 ...

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Oct 30, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Oct 30, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@LMESTM

LMESTM Oct 30, 2019

Contributor

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
LMESTM approved these changes Oct 30, 2019
Copy link
Contributor

LMESTM left a comment

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

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

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 30, 2019

CI started

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 30, 2019

@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

This comment has been minimized.

Copy link

mbed-ci commented Oct 30, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Oct 30, 2019
@0xc0170 0xc0170 merged commit cbf9f06 into ARMmbed:master Oct 30, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8684 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 31, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.