LPC176X: Fix flash program size #6413

toyowata · 2018-03-21T10:08:01Z

Description

This patch fix flash write issue when program size is more than page size (= 1024 bytes). See detail - Fixes #6165
Source data always use aligned data in heap memory.

Test result

[LPC1768]

mbedgt: test suite report:
+-----------------+---------------+-----------------------------+--------+--------------------+-------------+
| target          | platform_name | test suite                  | result | elapsed_time (sec) | copy_method |
+-----------------+---------------+-----------------------------+--------+--------------------+-------------+
| LPC1768-GCC_ARM | LPC1768       | tests-mbed_drivers-flashiap | OK     | 25.52              | default     |
+-----------------+---------------+-----------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 1 OK
mbedgt: test case report:
+-----------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
| target          | platform_name | test suite                  | test case                         | passed | failed | result | elapsed_time (sec) |
+-----------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
| LPC1768-GCC_ARM | LPC1768       | tests-mbed_drivers-flashiap | FlashIAP - init                   | 1      | 0      | OK     | 0.04               |
| LPC1768-GCC_ARM | LPC1768       | tests-mbed_drivers-flashiap | FlashIAP - program                | 1      | 0      | OK     | 0.29               |
| LPC1768-GCC_ARM | LPC1768       | tests-mbed_drivers-flashiap | FlashIAP - program across sectors | 1      | 0      | OK     | 0.26               |
| LPC1768-GCC_ARM | LPC1768       | tests-mbed_drivers-flashiap | FlashIAP - program errors         | 1      | 0      | OK     | 0.05               |
+-----------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 4 OK
mbedgt: completed in 26.94 sec

[ARCH_PRO]

mbedgt: test suite report:
+------------------+---------------+-----------------------------+--------+--------------------+-------------+
| target           | platform_name | test suite                  | result | elapsed_time (sec) | copy_method |
+------------------+---------------+-----------------------------+--------+--------------------+-------------+
| ARCH_PRO-GCC_ARM | ARCH_PRO      | tests-mbed_drivers-flashiap | OK     | 23.3               | default     |
+------------------+---------------+-----------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 1 OK
mbedgt: test case report:
+------------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
| target           | platform_name | test suite                  | test case                         | passed | failed | result | elapsed_time (sec) |
+------------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
| ARCH_PRO-GCC_ARM | ARCH_PRO      | tests-mbed_drivers-flashiap | FlashIAP - init                   | 1      | 0      | OK     | 0.04               |
| ARCH_PRO-GCC_ARM | ARCH_PRO      | tests-mbed_drivers-flashiap | FlashIAP - program                | 1      | 0      | OK     | 0.29               |
| ARCH_PRO-GCC_ARM | ARCH_PRO      | tests-mbed_drivers-flashiap | FlashIAP - program across sectors | 1      | 0      | OK     | 0.25               |
| ARCH_PRO-GCC_ARM | ARCH_PRO      | tests-mbed_drivers-flashiap | FlashIAP - program errors         | 1      | 0      | OK     | 0.05               |
+------------------+---------------+-----------------------------+-----------------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 4 OK
mbedgt: completed in 24.81 sec

I also confirmed to pass the test case here.

Pull request type

This patch fix flash write issue when program size is more than page size (= 1024 bytes). See detail - ARMmbed#6165 Source data always use aligned data in heap memory.

davidsaada · 2018-03-21T10:19:54Z

This looks fine to me. Just wonder whether you allocated the size for the full buffer (instead of copy_size) in order not to copy the data inside the critical section. If this was your intention, then got no problem here.

0xc0170 · 2018-03-21T13:43:51Z

This looks fine to me. Just wonder whether you allocated the size for the full buffer (instead of copy_size) in order not to copy the data inside the critical section. If this was your intention, then got no problem here.

I also wonder. Is there a reason these targets copies buffer to heap?

Good to have this fixed 👍

0xc0170 · 2018-03-21T13:44:17Z

/morph build

mbed-ci · 2018-03-21T14:42:39Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-03-21T20:26:13Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-22T01:07:29Z

Test : FAILURE

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

0xc0170 · 2018-03-22T13:10:14Z

/morph test

mbed-ci · 2018-03-22T19:50:22Z

Test : FAILURE

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

cmonr · 2018-03-22T19:55:18Z

@davidsaada, could I get your help here? I'm not sure if the above test failure was caused by the PR, or by some spurious condition with the CI.

The "NVStore: Multiple thread test" failure only occurred with an ARCH_PRO board compiled with IAR.

davidsaada · 2018-03-22T21:41:28Z

@cmonr Reason for that failure is the combination of @toyowata 's solution for this problem and the nvstore test. This PR allocates heap memory for the entire source buffer. This leads to an exhaustion of heap memory in IAR, which seems to be the least efficient of all toolchains in regards to memory allocations. Really don't know what to suggest - already minimized this test's parameters to reduce memory consumption, but seems that this PR pushes the test over the border again.

cmonr · 2018-03-23T02:56:36Z

Thanks for the insight @davidsaada! Looks like @ccli8 has created a PR to fix the test. If you could take a quick look at it, it would be much appreciated.

davidsaada · 2018-03-23T08:17:57Z

@cmonr @toyowata A solution such as the one @ccli8 created in his PR (reducing page size) would be the perfect solution here, if possible for ARCH_PRO as well. ARCH_PRO large page size causes a lot of issues in NVStore, both in memory consumption (as NVStore also allocates a page buffer) and in flash granularity (only allows storing 32 records in a sector, as page size is 1KB and sector size is 32KB).

0xc0170 · 2018-03-23T10:03:18Z

targets/TARGET_NXP/TARGET_LPC176X/device/flash_api.c

    // always malloc outside critical section
-    uint8_t *alignedData = malloc(size);
+    alignedData = malloc(size);


it could be as @davidsaada proposed - smaller program page size if it is defined, or what I was initially thinking, was allocating only copySize buffer on heap (because that one is used for flashing). It increases the runtime but making it possible to work for any app (1k of heap should be available). ?

Not sure it would help in this case. I suspect that the extra allocation of 1024 bytes already pushes this test over the limit. However, it's worth the try.

@0xc0170 @davidsaada In the current code, the reason of the malloc usage is to handle unaligned the data buffer here:

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NXP/TARGET_LPC176X/device/flash_api.c#L132-L137

However, there is an alignment check logic here before calling the HAL driver:
https://github.com/ARMmbed/mbed-os/blob/master/drivers/FlashIAP.cpp#L101

I think therefore, the malloc in the flash_program_page() is not required and it should just check the size of flash data.

I am going to modify this and create new commit for review next week.

@toyowata You can still check the alignment for the (unlikely) case this code is not called from the driver (or if the driver logic is changed for some reason), and only then perform the malloc + memcpy. In the likely aligned case, you can point to the user buffer.

cmonr · 2018-03-23T17:03:37Z

PR fix has been brought in. Feel free to rebase and address @0xc0170's comment when possible.

davidsaada · 2018-03-23T20:18:25Z

PR fix has been brought in. Feel free to rebase and address @0xc0170's comment when possible.

@cmonr Unless I'm missing big time here, I think the PR you brought in should only be used as a reference for the right fix on the ARCH_PRO board (if possible, not sure about it), as it solves the same problem - but on another board.

cmonr · 2018-03-23T23:44:59Z

OH! Ha, you're right @davidsaada. That completely excaped me. The timing was so right that I thought it was the fix.

* Add source address word alignment check * malloc and memcpy are called only if data is unaligned * malloc size is now copySize (program page size), rather than whole buffer to be written

toyowata · 2018-03-27T14:07:16Z

@0xc0170 @davidsaada
I added new commit. Can you please review?

davidsaada · 2018-03-27T14:07:51Z

Looks very good now.

cmonr · 2018-03-27T15:45:48Z

/morph build

mbed-ci · 2018-03-27T17:19:14Z

Build : FAILURE

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

0xc0170 · 2018-03-27T17:25:21Z

/morph build

mbed-ci · 2018-03-27T18:48:51Z

Build : FAILURE

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

0xc0170 · 2018-03-28T11:55:10Z

License server failure, it should be resolved now, restarting

/morph build

mbed-ci · 2018-03-28T13:27:45Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-03-28T18:22:00Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-29T02:06:57Z

Test : SUCCESS

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

LPC176X: Fix flash program size

d76d511

This patch fix flash write issue when program size is more than page size (= 1024 bytes). See detail - ARMmbed#6165 Source data always use aligned data in heap memory.

toyowata mentioned this pull request Mar 21, 2018

Flash programming in ARCH_PRO board doesn't work with size > page_size #6165

Closed

toyowata requested review from davidsaada and 0xc0170 March 21, 2018 10:09

davidsaada approved these changes Mar 21, 2018

View reviewed changes

0xc0170 approved these changes Mar 21, 2018

View reviewed changes

0xc0170 added the needs: CI label Mar 21, 2018

cmonr added the needs: preceding PR label Mar 23, 2018

cmonr mentioned this pull request Mar 23, 2018

Nuvoton: Fix NVSTORE test failed #6437

Merged

0xc0170 requested changes Mar 23, 2018

View reviewed changes

0xc0170 removed the needs: CI label Mar 23, 2018

cmonr added needs: work and removed needs: preceding PR labels Mar 23, 2018

Add alignment check in the flash_program_page

3acdc81

* Add source address word alignment check * malloc and memcpy are called only if data is unaligned * malloc size is now copySize (program page size), rather than whole buffer to be written

davidsaada approved these changes Mar 27, 2018

View reviewed changes

0xc0170 approved these changes Mar 27, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Mar 27, 2018

cmonr merged commit f895392 into ARMmbed:master Mar 29, 2018

cmonr added release-version: 5.8.2 and removed needs: CI labels Mar 29, 2018

toyowata deleted the lpc1768_flash_fix branch December 17, 2018 05:02

LPC176X: Fix flash program size #6413

LPC176X: Fix flash program size #6413

Uh oh!

Conversation

toyowata commented Mar 21, 2018 • edited by cmonr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test result

Pull request type

Uh oh!

davidsaada commented Mar 21, 2018

Uh oh!

0xc0170 commented Mar 21, 2018

Uh oh!

0xc0170 commented Mar 21, 2018

Uh oh!

mbed-ci commented Mar 21, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 21, 2018

Uh oh!

mbed-ci commented Mar 22, 2018

Uh oh!

0xc0170 commented Mar 22, 2018

Uh oh!

mbed-ci commented Mar 22, 2018

Uh oh!

cmonr commented Mar 22, 2018

Uh oh!

davidsaada commented Mar 22, 2018

Uh oh!

cmonr commented Mar 23, 2018

Uh oh!

davidsaada commented Mar 23, 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!

cmonr commented Mar 23, 2018

Uh oh!

davidsaada commented Mar 23, 2018

Uh oh!

cmonr commented Mar 23, 2018

Uh oh!

toyowata commented Mar 27, 2018

Uh oh!

davidsaada commented Mar 27, 2018

Uh oh!

cmonr commented Mar 27, 2018

Uh oh!

mbed-ci commented Mar 27, 2018

Uh oh!

0xc0170 commented Mar 27, 2018

Uh oh!

mbed-ci commented Mar 27, 2018

Uh oh!

0xc0170 commented Mar 28, 2018

Uh oh!

mbed-ci commented Mar 28, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 28, 2018

Uh oh!

mbed-ci commented Mar 29, 2018

Uh oh!

Uh oh!

toyowata commented Mar 21, 2018 •

edited by cmonr

Loading