Reregistration GR-PEACH as mbed support board #5530

TomoYamanaka · 2017-11-20T07:00:42Z

Description

By #5342, since Cortex-A was supported wtih updating CMSIS/RTX, I request that GR-PEACH is registed as mbed board again.
Therefore, I PR the process that depend on GR-EPACH, and more.

Request

I would like to compile the Cortex-A boards that is supported as Mbed.
To do that, it's necessary to change the python file in "tools".
In "tools/build_api.py", please remove the processing done by Disallow building of mbed OS 2 + RTOS, and remove mbed OS 5 support for Cortext-A #4415.
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 · 2017-11-20T11:09:48Z

I would like to compile the Cortex-A boards that is supported as Mbed.
To do that, it's necessary to change the python file in "tools".
In "tools/build_api.py", please remove the processing done by #4415.

This should be part of this PR, or should be done as separate patch and this commited on top of it, to test these changes.
@theotherjimmy Can we just revert the PR 4415 as whole? Please advise

@TomoYamanaka Was this tested with the modifications (reverting locally 4415) ?

TomoYamanaka · 2017-11-20T11:41:46Z

@0xc0170

@TomoYamanaka Was this tested with the modifications (reverting locally 4415) ?

Of course. I tested with modification it. :)

There was a lack in the above description. I needed to modify "tools\toolchains\arm.py" too.
b0fc103#diff-9cf75aea879b6b9cc5026af0b8f708a3R37
I added "Cortex-A" in arm.py because I couldn't compile by ARMCC.

theotherjimmy · 2017-11-20T16:35:56Z

@TomoYamanaka @0xc0170 That PR was designed to be reverted at a later date. Please feel free to revert.

TomoYamanaka · 2017-11-21T06:44:10Z

@theotherjimmy

That PR was designed to be reverted at a later date. Please feel free to revert.

I reverted "Remove Cortex-A mbed OS 5 support".

Also,

There was a lack in the above description. I needed to modify "tools\toolchains\arm.py" too.

I added Cortex-A for core support in ARM toolchain.

0xc0170 · 2017-11-21T14:16:36Z

targets/TARGET_RENESAS/TARGET_RZ_A1H/us_ticker.c

@@ -62,6 +62,7 @@ void us_ticker_init(void) {
    // INTC settings
    InterruptHandlerRegister(US_TICKER_TIMER_IRQn, (void (*)(uint32_t))us_ticker_interrupt);
    GIC_SetPriority(US_TICKER_TIMER_IRQn, 5);
+    GIC_SetConfiguration(US_TICKER_TIMER_IRQn, 3);


The commit msg:

Change the processes that depend on GR-EPACH.

These files are based on the codes of CMSIS5/RTX5 for RZ/A1H incorporated in DS-5.

I modified the support range from Mbed OS 2 to Mbed OS 2 and 5.

In "mbed_rtx.h", I changed the values depend on GR-PEACH like a other targets for Cortex-M.

Is this change related to the first one These files are based on the codes of CMSIS5/RTX5 for RZ/A1H incorporated in DS-5. ? What does this change do? Changes like this should be in a separate commit - what is fixing and how.

By the codes of CMSIS5/RTX5 for RZ/A1H incorporated in DS-5, GIC_SetConfiguration is a newly added function as Cortex-A, this function sets the interrupt configration using GIC.
It seems that this processing has become necessary for interrupt of each driver to work properly.

Thanks, that answers it. Please consider this type of details to be part of the change (using git commit message).

You can amend the commit msg to include there this detail for the reader, it would answer my question.

I amended the commit msg(8863d14). Make a sense?

0xc0170 · 2017-11-21T14:19:23Z

rtos/TARGET_CORTEX/mbed_boot.c

@@ -187,7 +187,11 @@ osThreadAttr_t _main_thread_attr;
 #ifndef MBED_CONF_APP_MAIN_STACK_SIZE
 #define MBED_CONF_APP_MAIN_STACK_SIZE 4096
 #endif
+#if defined(__CORTEX_A9)
+MBED_ALIGN(8) char _main_stack[MBED_CONF_APP_MAIN_STACK_SIZE*4];


why we cant use the default one (*4 here)? This is breaking this config. As a user would set it to 10k for instance, actually would get 40k.

This change refer in the processing of previous mbed-os version.
Reference is here.
I think Cortex-A in general will require more stack space more than Cortex-M.

As we can see, some changes like this would be good to have as a separate commit with description to understand the reason for this change.

The default value should be rather changed. It is set on the line 188 in this file. As we would need to do *4 for any line that uses it in this file.

I think Cortex-A in general will require more stack space more than Cortex-M.

4k by default would not be sufficient to run mbed-os examples ? If we agree that Cortex-A should have 4x more stack by default than cortex-m, then we would need:

#if cortex-a // default stack is 16k #else // default stack is 4k #endif

@bulislaw Please check

We decided to set it to 4K by default for all platforms and make it configurable for applications. Overwriting it silently like that is confusing. Why do we need to do that? Is 4K not enought to run basic Mbed Apps and examples?

In case of this test, Executing with #define THREAD_STACK_SIZE 256 in Cortex-A, program will abort because of the lack of stack.
Even threads that only execute malloc and free, it seems that it will make a difference in the stack used between Cortex-A and Cortex-M.
As a result of check, in Cortex-A, program will work properly by #define THREAD_STACK_SIZE 512 at least.
(Now the value of macro I committed is 2048, I will change to 512)

This is why, Cortex-A needs more stack size than Cortex-M.
Since this value(16K) may be too large, how about change to 8K by following the above size? (Cortex-A is twice Cortex-M)
Could you give me your opinion about the above?

@0xc0170 @bulislaw
I will appreciate if you reply this promptly.

I would leave the MBED_CONF_APP_MAIN_STACK_SIZE as it is for all platforms. The question above was not answered, if 4k is not sufficient for mbed OS app. Did you have any failures with it - tests or examples?
This is application specific, it is intended to be tuned by application.

@0xc0170
Thank you for your commnets.
I understood the specification of MBED_CONF_APP_MAIN_STACK_SIZE.
To be honest, I was worried about the lack of the stack by any chance, there was no problem for mbed test in case of 4K.
Therefore, I will modify to revert the value of macro to 4K soon.

I committed.

0xc0170 · 2017-11-21T14:39:47Z

targets/targets.json

@@ -2356,11 +2356,12 @@
        "core": "Cortex-A9",
        "program_cycle_s": 2,
        "extra_labels": ["RENESAS", "MBRZA1H"],
+        "macros": ["TARGET_FEATURE_EXTENSION_REGISTER_COUNT_GCC=32", "TARGET_FEATURE_EXTENSION_REGISTER_COUNT_IAR=32"],


What does these 2 macro do? Would it be better to put into the config, instead of macro ?

I can see it is used in the rtx assembly files, are these also part of RTX ?

Original was the form of TARGET_FEATURE_EXTENSION_REGISTER_COUNT.
However, since this macro is a built-in environment variable of the ARM compiler,
in the case of GCC or IAR, this macro is became un-active, either process related to VFP does not run.
The macro here should be the target dependent value. In order to run the processing with the register count 32 as same previously,
I prepared the renamed macro, I actived it's processes in GCC and IAR.
Finally there was no a file is included in RTX assembly file, so I defined a macro in targets.json.

Can we do this via config, see an example: https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json#L269. Rather than macro with some value?

As a result of cherry-picking fixes from official CMSIS repo please, the macro that I added in targets.json became unnecessary. So I reverted the change of its file.

bulislaw

I really don't want to change CMSIS files in Mbed OS when we can help it. These should be dicussed with CMSIS folk and broguht there and then cherry-picked on Mbed until we do next code drop.
@JonatanAntoni could you look at the changes to cmis/ directory and see if you would be willing to accept them to CMSIS?
@theotherjimmy can you review changes to tools please

JonatanAntoni · 2017-11-22T10:52:09Z

rtos/TARGET_CORTEX/rtx5/RTX/Source/TOOLCHAIN_GCC/TARGET_CORTEX_A/irq_ca.S

@@ -391,13 +391,13 @@ osRtxContextSave:

                VMRS    R2, FPSCR
                STMDB   R3!, {R2,R12}              // Push FPSCR, maintain 8-byte alignment
-                #if TARGET_FEATURE_EXTENSION_REGISTER_COUNT == 16
+                #if TARGET_FEATURE_EXTENSION_REGISTER_COUNT_GCC == 16


We fixed that issue in the meanwhile. Please check with latest CMSIS release if this works for you.

JonatanAntoni · 2017-11-22T10:52:55Z

rtos/TARGET_CORTEX/rtx5/RTX/Source/TOOLCHAIN_IAR/TARGET_CORTEX_A/irq_ca.S

+                ;ORR     R2, R2, #2
+                ;STRB    R2, [R0, #TCB_SP_FRAME]
+                ;ENDIF
+                ;IF {TARGET_FEATURE_EXTENSION_REGISTER_COUNT_IAR} == 32


We fixed this issue on latest CMSIS release.

JonatanAntoni · 2017-11-22T10:58:21Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

 #include "rtx_core_cm.h"
+#endif


Why do you need to access this internal RTX header?
A user should never need to rely on RTX implementation details like this.

I don't actually think it's needed.

It seems that it implemented by this commit (4b354f8#diff-415fd494349b5414064e0cc0d9f0d596).
In cortex-A, the build error occurs by including "rtx_core_cm.h". Therefore I implemented to include the header file for Cortex-A("rtx_core_ca.h").

Sure rtx_core_cm.h contains the core abstraction for Cortex-M, so this cannot be used for Cortex-A. I strongly recommend not to rely on internal stuff. Actually the include (formerly core_cm.h) was introduced in #e44d94f. Can you point out what you need that's defined in this header?

It's not needed and can be deleted. I'll submit an PR to do it.

JonatanAntoni · 2017-11-22T10:59:27Z

cmsis/TARGET_CORTEX_A/core_ca.h

  {
      //Disable the SPI interrupt
-      GIC_DisableIRQ(i);
+      GIC_DisableIRQ((IRQn_Type)i);
      if (i > 15) {


i starts at 32, thus this condition is always true.
We fixed GIC implementation in latest CMSIS release.

JonatanAntoni · 2017-11-22T11:02:43Z

cmsis/TARGET_CORTEX_A/cmsis_iccarm.h

@@ -581,6 +603,13 @@ void __FPU_Enable(void)
 }


+__STATIC_INLINE
+int __disable_irq_iar() {


Its not the intention of CMSIS to incorporate device and/or compiler specific functions. All functions (API) shall rather be implemented for all compilers in this case. It seems you are looking for a extended version of __disable_irq() that returns the value of CPSR before modification.

I reverted this function by cherry-picking from CMSIS repo, implemented disable_irq() for IAR by utilizing mbed critical function.

bulislaw · 2017-11-22T15:21:59Z

@TomoYamanaka Instead of fixing it yourself, could you cherry pick fixes from official CMSIS repo please? All the other modifications to CMSIS should be submitted and accepted to CMSIS repo as well and then cherry picked on top of Mbed.

theotherjimmy

Could you add "Cortex-A9" to ARMC6's supported core list as well?

TomoYamanaka · 2017-11-24T10:44:21Z

@bulislaw
Now, I'm preparing to cherry-pick them, so can you wait a second?

@theotherjimmy

Could you add "Cortex-A9" to ARMC6's supported core list as well?

I commited this change.

TomoYamanaka · 2017-11-27T07:40:14Z

I cherry-picked fixes from official CMSIS repo please.
And I modifid the macro of "THREAD_STACK_SIZE" using malloc test to 512B.

bulislaw · 2017-11-27T09:57:34Z

I'm sorry that's not cherry picking: 66aea98 could we have actual commits 1 to 1 from the CMSIS repo so we know which fixes we actually have.

bulislaw · 2017-11-27T10:01:34Z

These commit still change CMSIS/RTX:
54a3e56
21d0d31

If they are not longer needed because of the 'cherry-picking' please remove them. If they are still needed can we submit them to CMSIS/RTX repo?

TomoYamanaka · 2017-11-27T10:29:08Z

@bulislaw
Thank you for your kindness.
I will try again.

TomoYamanaka · 2017-11-28T12:30:43Z

@bulislaw

I cherry-picked from CMSIS repo.
Also I changed the method of disable_irq() / enable_irq() in each drivers.

If they are not longer needed because of the 'cherry-picking' please remove them. If they are still needed can we submit them to CMSIS/RTX repo?

I adjusted the contents of CMSIS repo, but in IAR toolchain, there were two changes needed to run GR-PEACH in cmsis_iccarm.h.
6239ec0
31ce59a

Could you sumit them to CMSIS repo ?

bulislaw · 2017-11-28T13:40:23Z

@TomoYamanaka I'm sorry I can't see anything in this history, can you remove all the unnecessary commits rather than overwrite them and change back and forth? The log should be clean as we are reviewing commit by commit and also it'll end up in the Mbed OS history.

Also if your commits will be structured as follows it will build a coherent story and make all our lives easier.
1..a) Changes to PEACH code
a..b) Changes to Mbed OS including reverting disabling the Cortex A
b..c) Cherry-picked changes from CMSIS/RTX
c..d) New changes to CMSIS/RTX that @JonatanAntoni can review and then can be submitted to https://github.com/ARM-software/CMSIS_5/

TomoYamanaka · 2017-11-30T12:21:54Z

I've closed This pull request by mistake, so I will PR again soon.

TomoYamanaka changed the title ~~Retarget GR-PEACH as mbed support board~~ Reregistration GR-PEACH as mbed support board Nov 20, 2017

0xc0170 added the needs: review label Nov 20, 2017

0xc0170 requested a review from theotherjimmy November 21, 2017 14:11

0xc0170 requested changes Nov 21, 2017

View reviewed changes

bulislaw reviewed Nov 22, 2017

View reviewed changes

JonatanAntoni reviewed Nov 22, 2017

View reviewed changes

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

theotherjimmy suggested changes Nov 22, 2017

View reviewed changes

TomoYamanaka force-pushed the master branch from 8053b2d to baf7029 Compare November 24, 2017 03:24

TomoYamanaka force-pushed the master branch from 3953429 to 5f820c9 Compare November 27, 2017 09:04

TomoYamanaka closed this Nov 30, 2017

TomoYamanaka force-pushed the master branch from 31ce59a to b025ea1 Compare November 30, 2017 12:19

sg- removed the needs: work label Nov 30, 2017

TomoYamanaka mentioned this pull request Dec 1, 2017

Reregistration GR-PEACH as mbed support board #5628

Merged

Reregistration GR-PEACH as mbed support board #5530

Reregistration GR-PEACH as mbed support board #5530

Conversation

TomoYamanaka commented Nov 20, 2017

Description

Request

0xc0170 commented Nov 20, 2017

TomoYamanaka commented Nov 20, 2017

theotherjimmy commented Nov 20, 2017

TomoYamanaka commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomoYamanaka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomoYamanaka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw commented Nov 22, 2017

theotherjimmy left a comment • edited Loading

Choose a reason for hiding this comment

TomoYamanaka commented Nov 24, 2017

TomoYamanaka commented Nov 27, 2017

bulislaw commented Nov 27, 2017

bulislaw commented Nov 27, 2017

TomoYamanaka commented Nov 27, 2017

TomoYamanaka commented Nov 28, 2017 • edited Loading

bulislaw commented Nov 28, 2017

TomoYamanaka commented Nov 30, 2017

TomoYamanaka Nov 24, 2017 •

edited

Loading

TomoYamanaka Nov 24, 2017 •

edited

Loading

theotherjimmy left a comment •

edited

Loading

TomoYamanaka commented Nov 28, 2017 •

edited

Loading