Added Support for Toshiba TMPM46B #5902

ghost · 2018-01-23T10:49:47Z

Description

Add mbed support for TOSHIBA's TMPM46B board.

Status

READY

Migrations

NO

Test Status

Pass: Mbed SDK automated test suite and greentea with ARM, GCC_ARM and IAR toolchains.

M46B_GreenteaAutomated_TestResults.txt
M46B_SingleAutomated_TestResults.txt

mbed-ci · 2018-01-23T10:49:48Z

Automatic CI verification build not done, please verify manually.

0xc0170

Can you share tests results for all 3 toolchains?

0xc0170 · 2018-01-24T13:51:48Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
+{
+    uint32_t Data[length];
+    int timeout = 10;


how is t his number calculated ? why 10?

The entropy seed generator (ESG) is a circuit that generates a 512-bit entropy seed using a ring oscillator. When the ESG core ends the operation, it outputs an interrupt. Interrupt generation can be checked by polling of the interrupt flag. Then an entropy seed can be read from 16 blocks (ESGBLK00 to 15)
Timeout 10 is used for 10 iteration to confirm the ESG operation is completed.

Sometimes function execution completes before generating interrupts and read wrong value.

Timeout will help to return from function, even interrupts are not generated within 10 iteration.

We will also try to change the approach.

@hanno-arm Please review trng addition

0xc0170 · 2018-01-24T13:52:36Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/sleep.c

+
+void hal_deepsleep(void)
+{
+    hal_sleep();


why deepsleep = sleep here?

TMPM46BF10FG does not support the low power consumption mode configured with the SLEEPDEEP bit in the Cortex-M4 core.
Setting the bit of the system control register is prohibited.

OK Thanks, if this is captured in git log or can be here in the code, so others wont be surprised?

Hi @0xc0170, added the reason as code comments. Thank you.

0xc0170 · 2018-01-24T13:53:23Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/pwmout_api.c

+#include "tmpm46b_tmrb.h"
+
+#define MAX_COUNTER_16B         0xFFFF
+TMRB_InitTypeDef m_tmrb;


these 2 variables static?

Updated as static.

0xc0170 · 2018-01-24T13:54:39Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/i2c_api.c

+    20, 24, 32, 48, 80, 144, 272, 528
+};  // SCK Divider value table
+
+I2C_clock_setting_t clk;


why these 2 are not static as well?

Updated as static.

0xc0170 · 2018-01-29T09:49:07Z

Can you share tests results for all 3 toolchains?

I might have missed the test results shared above. The failure MBED 11 was resolved recently, should be also green !

Travis failed, this was also fixed. Can you please rebase from master, to get both fixes in?

ghost · 2018-01-29T13:38:53Z

I might have missed the test results shared above. The failure MBED 11 was resolved recently, should be also green !

We checked MBED 11 and result is OK.

Travis failed, this was also fixed. Can you please rebase from master, to get both fixes in?

Rebased from master.

hanno-becker · 2018-01-29T14:44:40Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+
+int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
+{
+    uint32_t Data[length];


The length parameter is the maximum amount of entropy in bytes that may be written to output, while Data has four times that size.

hanno-becker · 2018-01-29T14:45:22Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+        // Get the calculation result
+        if(CompleteFlag == 1){
+            CompleteFlag = 0;
+            ESG_GetResult(Data); 


This might overflow the Data buffer if length < 16.

hanno-becker · 2018-01-29T14:48:21Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+            CompleteFlag = 0;
+            ESG_GetResult(Data); 
+            for(i = 0U; i < length; i++){
+                output[i] = Data[i];


At most 16 bytes have been written to Data, but this loop reads its entire contents of user-specified size length, filling the remainder of output with non-random data.

hanno-becker

The entropy gathering function trng_get_bytes needs to be rewritten. The internal data buffer should have a static size of 64 bytes - the chunk size in which the TRNG returns the entropy - and only the amount of entropy gathered should be reported back to the user. Since it is not mandatory to return as many bytes as the user has requested, this leaves the option of either looping or always returning at most 16 bytes. Also, the stack buffer temporarily holding the entropy must be zeroized before the function returns.

ghost · 2018-01-30T11:28:36Z

Hi @hanno-arm, rewritten the entropy gathering function trng_get_bytes based on your comments.

hanno-becker · 2018-01-30T13:07:43Z

Hi @ganesh-ramachandran, thank you for your rework! Could you point me to the technical reference manual for the board?

ghost · 2018-01-31T04:44:13Z

Could you point me to the technical reference manual for the board?

Hi @hanno-arm, Please find the datasheet and schematics at https://os.mbed.com/platforms/TMPM46B/#technical-reference

ciarmcom · 2018-02-07T12:50:25Z

ARM Internal Ref: IOTSSL-2073

0xc0170 · 2018-02-09T14:45:34Z

@ganesh-ramachandran As TRNG is waiting for mbedtls approval, it might be faster to leave TRNG here and send PR adding target support separately? Or leave it as it is.

hanno-becker · 2018-02-09T15:22:34Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+            ESG_SetLatchTiming(ESG_LATCH_TIMING_1);
+            // Get latchtiming value
+            latchtiming = ESG_GetLatchTiming();
+            if (Fintming >= 512U * (latchtiming + 1U) + 3U) {


@ganesh-ramachandran Could you explain the rationale behind this check? As far as I see, it passes if and only if latchtiming is zero ( == ESG_LATCH_TIMING_1), so why not check for this directly?

Yes, we understood and removed the extra checking.

hanno-becker · 2018-02-09T15:28:00Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+    } else {
+        // Do nothing 
+    }
+}


Unfortunately, the current API doesn't allow trng_init to have a return value to signal failure (that has been resolved in Mbed TLS 2.7.0 and will hopefully soon be merged into Mbed OS). Still, I'd like to seek for a solution which allows to distinguish initialization failure from success, in order to not have the TRNG driver potentially output non-random data because of unnoticed failure during initialization.

Suggestion: Currently, the TRNG context trng_s consists of an unused dummy field only. This can be changed to a boolean flag indicating successful initialization, which is set on a successful call to trng_init, cleared on a call to trng_free, and checked for in trng_get_bytes.

Thank you for the suggestion. we have added the errors handling during TRNG initialization to prevent accidental output of non-random data.

hanno-becker · 2018-02-09T15:34:01Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+    *output_length = 0;
+    (void)obj;
+
+    while (TSB_ESG->ST == ESG_BUSY_SET) {


Why are we polling on TSB_ESB->ST here instead of CompleteFlag, which is what the ESG's IRQ handler sets?

TSB_ESB->ST is the status register and it indicate the process is complete or not.
While polling IRQ handler flag(CompleteFlag), interrupt will generate only once.
Hence if application will try to call two times the trng_get_bytes() function it will freeze the application. To avoid this scenario we poll TSB_ESB->ST and reconfirm IRQ handler flag (CompleteFlag) is set or not.

Now we changed to polling on IRQ handler flag(CompleteFlag), because we confirmed the upper layer file(mbed_trng.c) always calls trng_get_bytes() function in between TRNG init and TRNG free.

hanno-becker · 2018-02-09T15:38:21Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+        CompleteFlag = 0;
+        // Get the calculation result
+        ESG_GetResult((uint32_t*)random); //512-bit entropy
+        for (i = 0; (i < 64) && (*output_length < length); i++) {


Please use i < length here instead of *output_length < length, and set *output_length only once at the end of the loop.

Yes, modified as per your suggestion.

hanno-becker

Suggested a way of handling errors during TRNG initialization to prevent accidental output of non-random data, and requested clarification of some parts of the driver code.

0xc0170 · 2018-03-22T12:59:21Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/us_ticker.c

+}
+
+// initialize us_ticker
+void us_ticker_init(void)


I've noticed in the init interrupts for HAL. you are not using NVIC_SetVector - dynamic vectors?

See https://docs.mbed.com/docs/mbed-os-handbook/en/5.2/advanced/porting_guide/ (Bootstrap section)

@0xc0170, Added NVIC_SetVector - dynamic vectors

0xc0170 · 2018-03-22T12:59:39Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/us_ticker.c

+const ticker_info_t* us_ticker_get_info()
+{
+    static const ticker_info_t info = {
+			1875000,//1875000,


are there tabs as this line is misaligned with line below?

0xc0170 · 2018-03-22T13:00:04Z

targets/TARGET_TOSHIBA/TARGET_TMPM46B/trng_api.c

+    // Interrupt clearing
+    ret = ESG_ClrInt();
+    if (ret == ERROR)
+    {


{ one the same line (line 74 as well here)

cmonr · 2018-03-23T17:16:55Z

Still waiting on @c1728p9 for feedback on @0xc0170 comment.

0xc0170 · 2018-03-23T18:32:49Z

Still waiting on @c1728p9 for feedback on @0xc0170 comment.

Should not block this? It's more for the ongoing specification in sleep.

0xc0170 · 2018-03-28T12:03:03Z

/morph build

mbed-ci · 2018-03-28T13:14:43Z

Build : SUCCESS

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

Triggering tests

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

mazimkhan · 2018-03-28T13:35:17Z

@ganesh-ramachandran thanks for incorporating the comments. Approved.

mbed-ci · 2018-03-28T17:10:38Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-29T00:53:35Z

Test : SUCCESS

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

0xc0170 added needs: review scope: new-target labels Jan 23, 2018

0xc0170 reviewed Jan 24, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 29, 2018

0xc0170 requested a review from hanno-becker January 29, 2018 13:49

0xc0170 added tracking component: security labels Jan 29, 2018

hanno-becker reviewed Jan 29, 2018

View reviewed changes

hanno-becker suggested changes Jan 29, 2018

View reviewed changes

cmonr added needs: review and removed needs: work labels Feb 1, 2018

ciarmcom added the mirrored label Feb 7, 2018

hanno-becker reviewed Feb 9, 2018

View reviewed changes

hanno-becker suggested changes Feb 9, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 12, 2018

0xc0170 reviewed Mar 22, 2018

View reviewed changes

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

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

0xc0170 previously approved these changes Mar 23, 2018

View reviewed changes

Ganesh Ramachandran added 5 commits March 26, 2018 15:39

Added Support for Toshiba TMPM46B

4e7e9e9

Modified ESG reset process

d7905ab

CI build - case sensitive include header resolved

ee1bf13

Dynamic vector relocation and formatting changes

609441a

Low power consumption mode for deepsleep

a7d8eae

ghost dismissed 0xc0170’s stale review via a7d8eae March 26, 2018 10:19

0xc0170 mentioned this pull request Mar 28, 2018

Add support for target MTB_ADV_WISE_1530. #6423

Closed

5 tasks

0xc0170 approved these changes Mar 28, 2018

View reviewed changes

mazimkhan approved these changes Mar 28, 2018

View reviewed changes

theotherjimmy added ready for merge and removed needs: review labels Mar 29, 2018

theotherjimmy merged commit 0615111 into ARMmbed:master Mar 29, 2018

theotherjimmy added release-version: 5.8.2 and removed ready for merge labels Mar 29, 2018

ciarmcom added the closed_in_jira label Mar 29, 2018

Added Support for Toshiba TMPM46B #5902

Added Support for Toshiba TMPM46B #5902

Uh oh!

Conversation

ghost commented Jan 23, 2018

Description

Status

Migrations

Test Status

Uh oh!

mbed-ci commented Jan 23, 2018

Uh oh!

0xc0170 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!

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 Jan 29, 2018

Uh oh!

ghost commented Jan 29, 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!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 30, 2018

Uh oh!

hanno-becker commented Jan 30, 2018

Uh oh!

ghost commented Jan 31, 2018

Uh oh!

ciarmcom commented Feb 7, 2018

Uh oh!

0xc0170 commented Feb 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker Feb 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

hanno-becker Feb 9, 2018 •

edited

Loading