Skip to content

Reregistration GR-PEACH as mbed support board #5628

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

Merged
merged 17 commits into from
Dec 28, 2017
Merged

Conversation

TomoYamanaka
Copy link
Contributor

@TomoYamanaka TomoYamanaka commented Dec 1, 2017

Description

By #5342, since Cortex-A was supported wtih updating CMSIS/RTX, I request that GR-PEACH is registed as mbed board again.

I've closed #5530, because I removed my commits accidently. So, I made this PR again.

The main changes is the below.

  • Changes to PEACH code (4 commits)
  • Changes to Mbed OS including to reverting "disabling the Cortex-A" (5 commits)
  • Cherry-picked changes from CMSIS/RTX repo (3 commits)
  • New changes to CMSIS/RTX (3 commits)

Request

  • I strongly desire to be included in the shortest release package after completing the merge of this PR.

@toyowata
Please help me to go ahead with the above.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 1, 2017

I looked at the changes (changes to target code looks fine to me). Got question regarding changes to CMSIS/RTX.

The last 3 commits - are they going also upstream (are there pull requests or issues reported) ?

3 commits that are being cherry-picked from CMSIS repository, they are already released (what version), or develop branch ? To be aware, what update do we need to get these in.

@bulislaw
Copy link
Member

bulislaw commented Dec 1, 2017

@JonatanAntoni could you review this 3 commits, as if they would be submitted to CMSIS_5 repo:
c6fa97c
c91b39a
aaad6c1

@@ -1788,6 +1788,20 @@ typedef struct RegionStruct {
region.sh_t = NON_SHARED; \
MMU_GetSectionDescriptor(&descriptor_l1, region);

#define section_normal_nc(descriptor_l1, region) region.rg_t = SECTION; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looking at the different macros this one is missing documentation

@TomoYamanaka
Copy link
Contributor Author

@bulislaw @JonatanAntoni
For merge the changes(0421868, fefec98, e2acfc1) of CMSIS/RTX, I has been promoted discussion by giving three PR to CMSIS_5 repo.
Currently, one of three(#286) was merged in CMSIS repo, but rest requests(#287, #288) do not be still merged and discussion is continuing. It seems that the cause and my changing contents obtained a certain understanding, now discussion focus on permanent fix. I think that permanent fix is needed, however, I strongly desire to merge this PR to Mbed OS 5.7 that will mention "cortex-A suppport" as official.

Therefore, Could you please merge my changes of CMSIS/RTX as tentative?
Of course, if permanent fix was completed in CMSIS_5 repo, I will cherry-pick them.

@toyowata
Please help me to go ahead with the above.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 8, 2017

For merge the changes(0421868, fefec98, e2acfc1) of CMSIS/RTX, I has been promoted discussion by giving three PR to CMSIS_5 repo.

Thank you 👍

Currently, one of three(#286) was merged in CMSIS repo, but rest requests(#287, #288) do not be still merged and discussion is continuing. It seems that the cause and my changing contents obtained a certain understanding, now discussion focus on permanent fix. I think that permanent fix is needed, however, I strongly desire to merge this PR to Mbed OS 5.7 that will mention "cortex-A suppport" as official.

Based on the review for those opened 2 PR, and discussion with @JonatanAntoni. There's only one left, 288 was fixed , so that could be done here the same (cosmetic change there the functionality the same). Thus only 287 needs further validation. Please help there to validate.

Thus only one remaining, that is not blocking this PR to my understanding.

What I would like to have one documentation improvement, New changes to CMSIS/RTX (No 1 of total 3 commits) The headline for each commit should provide meaningful information to a reader within 50 characters. If you could rework the commit messages it would be helpful. For instance: New changes to CMSIS/RTX (No 1 of total 3 commits) this could be CMSIS: add section_normal_nc impl or similar then the paragraph below is good as you have it 👍 The rest should follow. Once this is done, let us know

@TomoYamanaka
Copy link
Contributor Author

@0xc0170
Thank you for your comments.
I rebased those commits in the form of changing commit message.

Based on the review for those opened 2 PR, and discussion with @JonatanAntoni. There's only one left, 288 was fixed , so that could be done here the same (cosmetic change there the functionality the same). Thus only 287 needs further validation. Please help there to validate.

286 and 288 were fixed in CMSIS_5 repo so that I cherry-picked those two commits(5f850a2 , 839e4a5).
Also since only 287 needs further validation, I didn't cherry-pick, and changed only the commit message.

Please let me know if there is anything you are unclear.

@toyowata
Copy link
Contributor

@0xc0170 Any additional step required for this PR?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 13, 2017

@TomoYamanaka If you can please rebase, we will trigger tests

@bulislaw Please review

@TomoYamanaka
Copy link
Contributor Author

@0xc0170 @toyowata

I rebased those commits.

@TomoYamanaka
Copy link
Contributor Author

How is going? Is there anything else to do?

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only commit I do not fully understand is c6827c8854604d45ee3b32f49adddddaab252efa - changing linker script files, not enough information in the commit msg to understand why bss section was changed and other sections there

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 18, 2017

/morph build

@mbed-ci
Copy link

mbed-ci commented Dec 18, 2017

Build : FAILURE

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

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 18, 2017

It seems our CI got a license problem (only some devices, seems like CI networking failure rather), will investigate

cc @studavekar @kegilbert

@TomoYamanaka
Copy link
Contributor Author

@0xc0170
I changed the linker script files to implement the dynamic HEAP the same as Cortex-M targets.
Since GR-PEACH's HEAP was a fixed area, I changed the label name(ZI_DATA to RW_IRAM1) and replaced the allocation of STACK/HEAP.

Do you know what I mean?
If there is no problem in this description, I will update the commit message soon.

@TomoYamanaka
Copy link
Contributor Author

@0xc0170
Thank you. I updated the commit msg.

@bulislaw
Copy link
Member

@TomoYamanaka we still have two commits touching CMSIS/RTX code:
a888ca5
4d85759

Where are we with pushing them to the original CMSIS/RTX repo?

@TomoYamanaka
Copy link
Contributor Author

TomoYamanaka commented Dec 19, 2017

@bulislaw

  • a888ca5
    I cherry-picked this commit from CMSIS_5 repo.

  • 4d85759
    I don't cherry-pick this commit from CMSIS_5 repo.
    I aim to merge this PR to Mbed OS 5.7, so I discussed with Arm Mbed team member on the thread and agreed with them to merge this commit as a tentative.

@JonatanAntoni
Copy link
Member

JonatanAntoni commented Dec 19, 2017

@TomoYamanaka,

4d85759 should have been superseded by 17b53ef2, bd0db8d and d43938f.

Cheers,
Jonatan

@TomoYamanaka
Copy link
Contributor Author

@JonatanAntoni

I rebased the commit 4d85759 by cherry-pick those commits(17b53ef2 , d43938f).

@bulislaw
Copy link
Member

@TomoYamanaka

4d85759
I don't cherry-pick this commit from CMSIS_5 repo.
I aim to merge this PR to Mbed OS 5.7, so I discussed with Arm Mbed team member on the thread and agreed with them to merge this commit as a tentative.

Could you point me to the comment?

VladimirUmek and others added 3 commits December 21, 2017 14:09
- Added L1 Cache test cases to CoreValidation.
- Adopted FVP Cortex-A configs to simulate cache states.
…ed#287)

__DEPRECATED conflicts with a predefined macro in GCC C++ mode.
@TomoYamanaka
Copy link
Contributor Author

TomoYamanaka commented Dec 21, 2017

@0xc0170
I rebased those commits. Is this ok with you?

@bulislaw
Copy link
Member

/morph build

@mbed-ci
Copy link

mbed-ci commented Dec 21, 2017

Build : FAILURE

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

@bulislaw
Copy link
Member

Arm compiler licence issues - retry.

/morph build

@mbed-ci
Copy link

mbed-ci commented Dec 22, 2017

Build : SUCCESS

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

Triggering tests

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

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 22, 2017

Restarted uvisor job (flash failure). mbed-os-generic-context - to be resolved, should not be here

@mbed-ci
Copy link

mbed-ci commented Dec 22, 2017

@mbed-ci
Copy link

mbed-ci commented Dec 22, 2017

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 22, 2017

@orenc17A Can you help resolving an error in uvisor CI?

@orenc17
Copy link
Contributor

orenc17 commented Dec 23, 2017

/morph uvisor-test

@toyowata
Copy link
Contributor

@0xc0170 Can you merge this please?

@bulislaw
Copy link
Member

@0xc0170 @theotherjimmy

@cmonr cmonr merged commit 2b718fe into ARMmbed:master Dec 28, 2017
@adbridge
Copy link
Contributor

This relies on previous changes targeted for 5.8 so should also go to that minor release.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 2, 2018

This relies on previous changes targeted for 5.8 so should also go to that minor release.

Please provide reference of offending dependencies.

@adbridge
Copy link
Contributor

adbridge commented Jan 2, 2018

Having done a re-run this is now patching across ok. Not sure why it was failing before unless it was conflicting with another PR which may have also been removed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.