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 support for ADI EV-COG-AD3029LZ platform #5137

Merged
merged 3 commits into from Nov 17, 2017

Conversation

@dave-wu
Contributor

dave-wu commented Sep 19, 2017

Description

Adding support to the EV-COG-AD3029LZ platform. Re-submitted as a new pull request to replace PR #5097.

Status

Ready.

Migrations

NO

Related PRs

Replacement of PR #5097.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 29, 2017

@dave-wu I can see that similar things that came up from another target, are in this one as well, can you confirm? in case yes, please update us once they are addressed

/* RNG Device memory */
static uint8_t RngDevMem[ADI_RNG_MEMORY_SIZE];
void trng_init(trng_t *obj)

This comment has been minimized.

@0xc0170

0xc0170 Oct 5, 2017

Member

@RonEld @yanesca Please review. I believe this is similar implementation to the other device being added

@dave-wu Is that correct?

This comment has been minimized.

@RonEld

RonEld Oct 8, 2017

Contributor

@0xc0170 Please label this with the tls component, so we could track it.
I have same comments as other device being added, once the other device comments will be resolved, and merged here, I will review here as well

targets/TARGET_Analog_Devices/TARGET_ADUCM302X/TARGET_ADUCM3029/api/us_ticker.c Outdated
if (largecnt >= 65536u) {
largecnt -= 65536u;
} else
largecnt = 0;

This comment has been minimized.

@0xc0170
@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

Build : SUCCESS

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

Triggering tests

/test mbed-os

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Oct 12, 2017

Added test results for the platform.
cog3029-20171012.zip

@yanesca

This comment has been minimized.

Contributor

yanesca commented Oct 18, 2017

@0xc0170 Could you please add the 'tracking' label to this PR?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

@dave-wu As soon as the other target is green (approved and all CI are green), I'll review this one as I see those 2 really similar so we dont need to duplicate the review and changes (the requested changes should also be done for this target). Let us know if all has been resolved for this target.

@0xc0170 0xc0170 added the tracking label Oct 18, 2017

@ciarmcom ciarmcom added the mirrored label Oct 20, 2017

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Oct 20, 2017

ARM Internal Ref: IOTSSL-1833

targets/TARGET_Analog_Devices/TARGET_ADUCM302X/TARGET_ADUCM3029/api/trng_api.c Outdated
output[i] = (uint8_t)(nRandomNum & 0xFF);
// Clear nRandomNum on the stack before exiting
nRandomNum = 0;

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

To avoid compiler optimising this memory scrubbing nRandomNum should be volatile.

adi_rng_EnableBuffering(RNGhDevice, false);
// Enable the TRNG
adi_rng_Enable(RNGhDevice, true);

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

adi_rng_EnableBuffering and adi_rng_Enable can fail based on definition in adi_rng.h. Wouldn't it be good idea to check status here. Also, initialise memset( (void *)obj, 0, sizeof( trng_t ) ); so that uninitialised object can be checked in other functions using the obj.

This comment has been minimized.

@hanno-arm

hanno-arm Nov 1, 2017

Contributor

@mazimkhan That's a general problem with the interfaces I'd say, since trng_init is not allowed to fail (and there's no xxx_setup function as in other modules). An ugly way to get around this would be to extend

struct trng_s {
    ADI_RNG_HANDLE  RNGhDevice;
};

by some flag indicating that something went wrong during initialization, and checking for that in trng_get_bytes.

This comment has been minimized.

@RonEld

RonEld Nov 14, 2017

Contributor

@hanno-arm But if the adi_xxx functions fail, we should assertr in this case, or something of the sort. I don't think we should continue operation

This comment has been minimized.

@hanno-arm

hanno-arm Nov 14, 2017

Contributor

@RonEld I agree, but the interface doesn't allow to signal the error. I don't know if an assert would be acceptable here and how it would affect overall system operation @0xc0170.

This comment has been minimized.

@0xc0170

0xc0170 Nov 14, 2017

Member

@dave-wu do you agree to error() if init fails ? If yes, please add it as requested above

void trng_free(trng_t *obj)
{
ADI_RNG_HANDLE RNGhDevice = obj->RNGhDevice;

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Would there be a merit in checking is obj is properly initialised.

int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
{
ADI_RNG_HANDLE RNGhDevice = obj->RNGhDevice;

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Would there be a merit in checking is obj if properly initialised. It is very hw dependent. Hence up to you. However, it is good to document the behaviour when obj is not valid or if initialisation is not suppose to fail.

for (i = 0; i < length; i++) {
// Loop until the device has data to be read
do {
result = adi_rng_GetRdyStatus(RNGhDevice, &bRNGRdy);

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Should we check adi_rng_GetStuckStatus as well and abort the loop if oscillator is stuck.

This comment has been minimized.

@hanno-arm

hanno-arm Nov 1, 2017

Contributor

Related discussion in #5144. From what I saw, the spec doesn't say anything about whether RNRDY being set entails STUCK being unset, so I'd also prefer to check the STUCK flag here.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

@dave-wu Could you please address @mazimkhan review comments?

@adbridge adbridge added needs: work and removed needs: review labels Oct 31, 2017

*phDevice = (ADI_RNG_HANDLE) NULL;
#ifdef ADI_DEBUG
if (nDeviceNum >= NUM_RNG_DEVICES)

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Why are these check inside debug flag?

This comment has been minimized.

@dave-wu

dave-wu Nov 14, 2017

Contributor

I think these are for internal testing by the driver developers only.

pDevice->pData->CBFunc = NULL;
/* clear any pending interrupts. Both bits are write 1 to clear */
pDevice->pRNG->STAT = BITM_RNG_STAT_RNRDY | BITM_RNG_STAT_STUCK;

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

BITM_RNG_STAT_STUCK is enabled here but not checked in trng_get_bytes.

This comment has been minimized.

@dave-wu

dave-wu Nov 14, 2017

Contributor

It is now.

ADI_INT_STATUS_ALLOC();
#ifdef ADI_DEBUG
if (ADI_RNG_INVALID_HANDLE(pDevice)) {

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Why guard these vital checks inside debug flag?

This comment has been minimized.

@RonEld

RonEld Nov 14, 2017

Contributor

@dave-wu Is this also for internal testing by the drivers developers? Please check, it seems to me that this check is important not only for debug

This comment has been minimized.

@dave-wu

dave-wu Nov 15, 2017

Contributor

@RonEld Had another look and I think you are right, these should be enabled as they check for invalid handles and they are the conditions that return the prescribed error codes. I will need to discuss with the driver development guys to get them to fix this in their next release. If you are OK with it I'll leave them for now and add in the fix when the new BSP becomes available.

This comment has been minimized.

@RonEld

RonEld Nov 15, 2017

Contributor

As long as you are aware of this, and that you will have to make a new release, I'm fine

ADI_RNG_DEV_TYPE *pDevice = (ADI_RNG_DEV_TYPE*)hDevice;
ADI_INT_STATUS_ALLOC();
#ifdef ADI_DEBUG

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Why guard these vital checks inside debug flag?

nEvent = ADI_RNG_EVENT_STUCK;
candidate &= (uint16_t)~BITM_RNG_STAT_STUCK;
} else {
break;

This comment has been minimized.

@mazimkhan

mazimkhan Oct 31, 2017

Contributor

Is it robust to ignore other events silently on an unrecognised event.

This comment has been minimized.

@dave-wu

dave-wu Nov 14, 2017

Contributor

This has been discussed in the PR thread for the 4050LZ. The STUCK flag is now checked in the trng_get_bytes to make sure no hardware errors have occurred.

@hanno-arm

Thanks for adressing the comments, @dave-wu.

I have reviewed the TRNG driver code from trng_api.c, adi_rng.c and adi_rng.h and that part of the PR looks good to me.

Two comments:

  1. As @mazimkhan mentioned, trng_init calls potentially failing function but dicards their return value. This is a general problem with the trng_xxx interface, though, which other drivers are also suffering from, and I'm ok with leaving it as is.
  2. The functions in question actually never fail because the relevant checks are guarded by a debug-flag. While I share @mazimkhan's question as to whether they aren't enabled unconditionally, it doesn't constitute a problem for this PR because all parameters are hardcoded in trng_init and satisfy the checks.
@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 12, 2017

@0xc0170 Could you please look at merging this as well? All the changes that I have done for the 4050LZ PR that are relevant to this platform have also been applied for this PR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 13, 2017

@0xc0170 Could you please look at merging this as well? All the changes that I have done for the 4050LZ PR that are relevant to this platform have also been applied for this PR.

Yes, we will review this , thanks for the update

@0xc0170 0xc0170 added needs: review and removed needs: work labels Nov 13, 2017

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 13, 2017

@0xc0170 Just did a commit to resolve the conflicts with the master.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 14, 2017

@dave-wu Still a conflict here. You can rebase to resolve conflicts (so each commit would be on top of the latest master).

@hanno-arm

This comment has been minimized.

Contributor

hanno-arm commented Nov 14, 2017

Please note that this is pending another review from @Patater or @mazimkhan.

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 14, 2017

@0xc0170 This PR is on the master branch of my copy of mbed-os, not sure how I can rebase that. Is there an easier way to resolve these conflicts? From the looks of it the conflicts are minimal and straight forward to resolve, so can it be easily resolved in the merge instead of doing rebasing?

P.S. I just made another update to target.json, to base on the latest target.json from the master branch.

@RonEld

RonEld approved these changes Nov 15, 2017

I approve this, with the condition that a new release is mandatory, but the issue is not blocking, at the moment

@RonEld

This comment has been minimized.

Contributor

RonEld commented Nov 15, 2017

@dave-wu In addition, you should open a github issue so the handle checking won't be missed

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 15, 2017

@RonEld Will do. Thanks.

@0xc0170 0xc0170 removed the needs: work label Nov 15, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 15, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 15, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 15, 2017

@dave-wu Can you please look at the failure ? It reports that SystemCoreClock is undeclared (should be defined in system__ header file and externally linked).

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 15, 2017

@0xc0170 SystemCoreClock is declared in system_aducm3029.c. From the build log it seems this file was not copied by the build script for some reason. Does the build exclude the startup files?

"core": "Cortex-M3",
"supported_toolchains": ["ARM", "GCC_ARM", "IAR"],
"macros": ["__ADUCM3029__", "EV_COG_AD3029LZ"],
"extra_labels": ["Analog_Devices", "ADUCM302X", "ADUCM3029", "EV_COG_AD3029LZ", "FLASH_CMSIS_ALGO"],

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 15, 2017

Contributor
"EV_COG_AD3029LZ",

is not needed here, as it matches the name of the target exactly. The target is already a label so adding it again, with extra_labels, is redundant.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 15, 2017

@studavekar I can't reproduce the build errors locally. I'm going to give it another go. If it fails again, could you look into it?
/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 15, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 15, 2017

It looks like #5342 was merged to master since you last rebased. Merging with the current master prevents this PR from building. I was able to reproduce the issue after updating my master branch.

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 16, 2017

@theotherjimmy Any resolution on this build issue?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

@dave-wu You should be able to reproduce the issue if you merge with master your branch or rather rebase to get the changes from master (do it on another branch so won't change this one. I can see you are working on your master).

Looking at the error. the file os_systick.c cant find the definition for SystemCoreClock . As it includes cmsis.h this is how it pulls it , I would check the include paths are correct and the definition can be seen via cmsis.h header file.

@dave-wu

This comment has been minimized.

Contributor

dave-wu commented Nov 16, 2017

@0xc0170 Just added extern SystemCoreClock to system_aducm3029.h which gets included in cmsis.h and it builds now. Could you please retest?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 16, 2017

Build : SUCCESS

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

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 0xc0170 changed the title from Added support for ADI EV-COG-AD3029LZ platform. to Add support for ADI EV-COG-AD3029LZ platform Nov 17, 2017

@0xc0170 0xc0170 merged commit b025ea1 into ARMmbed:master Nov 17, 2017

6 checks passed

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment