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

Registration GR-LYCHEE board as a new mbed board #5857

Merged
merged 8 commits into from Jan 26, 2018

Conversation

Projects
None yet
6 participants
@TomoYamanaka
Contributor

TomoYamanaka commented Jan 16, 2018

Description

I register GR-LYCHEE as a Renesas new Mbed board, based on existing GR-PEACH(RZ/A1H) support.
GR-LYCHEE incorporates RZ/A1LU that is RZ/A1 chip series.

Relation PRs

#5628
#5813

TomoYamanaka added some commits Jan 16, 2018

Add GR_LYCHEE as a new target board in terget.json, build_travis.py a…
…nd tests.py

I added GR-LYCHEE's configuration in targets.json file. Also, I added GR_LYCHEE as a Renesas new target board in build_travis.py and tests.py.
Enable exporter for GR_LYCHEE is a Renesas new target board
I added GR_LYCHEE as a new target board in DS-5, e2studio and IAR export.
Add startup processing having CMSIS5/RTX5 been available on GR-LYCHEE
For supporting to CMSIS5/RTX5, I added the start-up processing of 3 toolchains (ARMCC, GCC_ARM, IAR) and the register definition of RZ/A1LU specific.
In addition, I added the linker script files to implement the dynamic HEAP the same as GR-PEACH(RZ/A1H).
Add the definition for GR-LYCHEE in mbed_rtx.h
In mbed_rtx.h file, I added the definition for GR-LYCHEE to use the "Dynamic Heap" processing when the target is GR_LYCHEE.
Support TRNG function for GR-LYCHEE
I supported the TRNG function when target is GR-LYCHEE.
GR-LYCHEE generates TRNG by acquiring the random number of Wifi module(ESP32) incorporated in it using I2C.
@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jan 19, 2018

How is going? Is there anything to do for this PR?

} CANName;


#define STDIO_UART_TX USBTX

This comment has been minimized.

@0xc0170

0xc0170 Jan 19, 2018

Member

Is there a reason why this is not part of PinNames? I noticed this in some platforms but not certain about having a macro instead of in PInNames

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Jan 19, 2018

Contributor

I don't know a clear reason. This have been defined in "PeripheralNames.h" for a long time in GR-PEACH, and I created this the same as GR-PEACH.
Many targets is still defined in PeripheralNames.h, should I move to PInNames.h?

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

It could be moved. I only wondered if there is anything else that I was missing

@@ -0,0 +1,6 @@
#ifndef RESERVED_PINS_H

This comment has been minimized.

@0xc0170

0xc0170 Jan 19, 2018

Member

Should this contain license header on top?

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Jan 19, 2018

Contributor

I modified this content in commit 4bc79b0.

#include "MBRZA1LU.h"
#include "irq_ctrl.h"

void NVIC_SetVector(IRQn_Type IRQn, uint32_t vector) {

This comment has been minimized.

@0xc0170

0xc0170 Jan 19, 2018

Member

{ new line for both functions

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Jan 19, 2018

Contributor

I modified this content in commit e8378ef .

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 19, 2018

@hanno-arm please review trng addition (targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp file)

@ciarmcom ciarmcom added the mirrored label Jan 19, 2018

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Jan 19, 2018

ARM Internal Ref: IOTSSL-2030

TomoYamanaka added some commits Jan 19, 2018

Add license header on top in reserved_pins.h
I modified the lack of license header in the below header files.
- targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_RZ_A1H/TARGET_MBED_MBRZA1H/reserved_pins.h
- targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/TARGET_MBED_MBRZA1LU/reserved_pins.h
Modify the arrangement of "{" that shows the function start
Regarding "{" that show the function start, I modified the the arrangement from right of function to new line.
#ifndef MBED_CMSIS_H
#define MBED_CMSIS_H

#include "MBRZA1LU.h"

This comment has been minimized.

@0xc0170

0xc0170 Jan 19, 2018

Member

should this include cmsis_nvic.h ? I just found the bug report for RZ A1H, and pointed me this direction (Isent just now the fix for it)

This comment has been minimized.

@TomoYamanaka

TomoYamanaka Jan 19, 2018

Contributor

I modified this content in commit 1469fcd.

RZ_A1LU: cmsis nvic include fix
To get cmsis nvic definitions, I added the process that include "cmsis_nvic.h" in cmsis.h.
Relation PR is #5890.
@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jan 24, 2018

@0xc0170
Thank you for your approval. Could you please trigger CI test?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 24, 2018

CI triggered while mbedtls reviews TRNG implementation

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 24, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 25, 2018

known issue with one target that is being retested currently, we will restart CI

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 25, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jan 25, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 26, 2018

/morph uvisor-test

@cmonr cmonr merged commit da6532e into ARMmbed:master Jan 26, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented Jan 26, 2018

I know that it is not optimal that reviewing sometimes takes a long time due to other obligations, but please do not merge PR's that you have requested approval for before the review has been done.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

@hanno-arm We're still going to holding off on releasing this PR until we get feedback for your review.

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented Jan 26, 2018

@cmonr Could you indicate until when you must have received the review, so I can plan and see if and when I can fit it in? Apologies again for the delay.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

@hanno-arm No worries. The changes in this PR are minimal, and shouldn't take too much time. It would be wonderful if you could squeeze it in next week.

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented Jan 29, 2018

@0xc0170 @cmonr Could you point me to the technical reference manual for the board?

if (output_length != NULL) {
*output_length = idx;
}

This comment has been minimized.

@hanno-arm

hanno-arm Jan 29, 2018

Contributor

The stack buffer recv_data holding the chunks of entropy should be zeroized before the function returns, even if it's only 4 bytes maximum.

send_data[0] = 0;
ret = mI2c.write(ESP32_I2C_ADDR, send_data, 1);
if (ret == 0) {
mI2c.read(ESP32_I2C_ADDR, recv_data, sizeof(recv_data));

This comment has been minimized.

@hanno-arm

hanno-arm Jan 29, 2018

Contributor

This call may fail, but the return value is not checked.

@hanno-arm

I reviewed the TRNG addition and cannot approve it for the following reasons:

  1. All entropy must be zeroized before the gathering function returns.
  2. The return value for the I2C read call is not checked. This way, it could happen that predictable data is treated as the TRNG output.
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

@hanno-arm Thank you for the review. For the short term, we'll introduce a PR to disable the TRNG on the board.

@TomoYamanaka, please address the above comments in a new PR.

TomoYamanaka added a commit to TomoYamanaka/mbed-os that referenced this pull request Jan 30, 2018

[RZ_A1LU] Fix TRNG function
Related to the review of ARMmbed#5857, I fixed the TRNG function for GR-LYCHEE.
- I modified to zeroize "recv_data" before the function return.
- I added the processing that check the return value of I2C.read function. If return value is error, "output"  is zeroized before  function return.
@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jan 30, 2018

@cmonr @hanno-arm

I submitted a PR that addressed the above comments. So Please review PR #5970 .

TomoYamanaka added a commit to TomoYamanaka/mbed-os that referenced this pull request Jan 31, 2018

[RZ_A1LU] Fix TRNG function
Related to the review of ARMmbed#5857, I fixed the TRNG function for GR-LYCHEE.
- I modified to zeroize "recv_data" before the function return.
- I added the processing that check the return value of I2C.read function. If return value is error, "output" is zeroized before function return.
- In trng_get_bytes_esp32 function, there is a time lag in the period from ESP32 reset to start working, error may occur when "Write" is called. Thus, I added a retry counter due to address this concern. There is not this counter for "Read" since it is called after "Write".

TomoYamanaka added a commit to TomoYamanaka/mbed-os that referenced this pull request Feb 6, 2018

[RZ_A1LU] Fix TRNG function
Related to the review of ARMmbed#5857, I fixed the TRNG function for GR-LYCHEE.
- I modified to zeroize "recv_data" before the function return.
- I added the processing that check the return value of I2C.read function. If return value is error, "output" is zeroized before function return.
- In trng_get_bytes_esp32 function, there is a time lag in the period from ESP32 reset to start working, error may occur when "Write" is called. Thus, I added a retry counter due to address this concern. There is not this counter for "Read" since it is called after "Write".

adbridge added a commit that referenced this pull request Feb 9, 2018

Fix TRNG function
Related to the review of #5857, I fixed the TRNG function for GR-LYCHEE.
- I modified to zeroize "recv_data" before the function return.
- I added the processing that check the return value of I2C.read function. If return value is error, "output" is zeroized before function return.
- In trng_get_bytes_esp32 function, there is a time lag in the period from ESP32 reset to start working, error may occur when "Write" is called. Thus, I added a retry counter due to address this concern. There is not this counter for "Read" since it is called after "Write".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment