Skip to content
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 Marvell ARMADA A7K A8K platform support #1807

Merged
merged 3 commits into from
Oct 16, 2017

Conversation

kevin-peng-hao
Copy link

Added Marvell platform with initial support for ARMADA A7K & A8K.

Only tested 64-bit mode with default configurations:

  1. build command
    make PLATFORM=marvell-armada7080 CFG_ARM64_core=y
  2. Passed xtest

Signed-off-by: Kevin Peng kevinp@marvell.com

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Mainly minor style issues + few issues to fix.

Note you can run scripts/checkpatch.pl to your check commit(s) against basic stylish issues.

#include <marvell_sec_perf.h>
#include <io.h>
#include <mm/core_memprot.h>
#include <initcall.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: prefer sorting include in alphabetical order

#define SIZE_1M (0x100000)
#define SIZE_2M (0x200000)
#define SIZE_4M (0x400000)
#define SIZE_8M (0x800000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: useless parentheses .
Minor: maybe those SIZE_xM coud nicely fit in lib/libutils/ext/include/util.h

for (i = 0; i < MAX_RANGE_NUM; i++) {
tmp = read32(MCU_TZ_RANGE_LOW_REG(i));
TZ_GET_VALID(tmp, valid);
if (!(valid))
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parentheses (here and there)

}

if (!_IS_ALIGNED(addr, size)) {
EMSG("ERR: region size(0x%x) not align with addr(0x%x)"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor:

  • prefer the , at the end of the previous line.
  • use "... 0x%" PRIx32 rather than " ... 0x%x", here and below.

TZ_SET_AREA_LEN_CODE(data, sizecode);
TZ_SET_START_ADDR_L(data, addr);

if (valid_range == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor: prefer !valid_range

{
/* max supported granule for armada is 8TB
* but 2GB is far enough here
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: comment indentation.


#ifdef ARM64
#ifdef CFG_WITH_PAGER
#error "Pager not supported for ARM64"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: ARM64 now supports pager. But maybe your platform does not yet or does not need to.

Copy link
Author

Choose a reason for hiding this comment

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

It happened our platform supports 64-bit while pager not. So these codes are OK so far.
But these are the codes reference from the porting_guidelines. So the guide may need updates. :)

@@ -20,6 +20,8 @@ for these platforms.
| HiKey Board (HiSilicon Kirin 620) |`Linaro <op-tee@linaro.org>`|
| HiKey960 Board (HiSilicon Kirin 960) |`Linaro <op-tee@linaro.org>`|
| HiSilicon D02 |`Linaro <op-tee@linaro.org>`|
| Marvell Armada 70x0 |`Kevin Peng <kevinp@marvell.com>`|
| Marvell Armada 80x0 |`Kevin Peng <kevinp@marvell.com>`|
Copy link
Contributor

Choose a reason for hiding this comment

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

As Armada 80x0 is not yet supported, maybe wait for its support to declare the board.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry that the name "armada7080" makes confusions. It is supposed to include the two platforms, armada70x0 & armada 80x0, as they share the same codes. I shall make it clear.

* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Add some inclusion protection, i.e:

#ifndef PLAT_MARVELL_SEC_PERF_H
#define PLAT_MARVELL_SEC_PERF_H
...
#endif

#include <platform_config.h>
#include <tee_api_types.h>

TEE_Result init_sec_perf(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: this function does not need to be exported as routine is local to hal_sec_perf.c (use of service_init(init_sec_perf);)

Copy link
Author

Choose a reason for hiding this comment

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

That's right, then this head file is not necessary either.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

LGTM, I only have two very minor comments (please see below).
Since we're now trying to use Shippable for CI, could you please rebase your code onto the latest optee_os master, and update .shippable.yml? At the same time, please squash the fixup commits. With this done, you may add my:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

#include <arm.h>

.globl plat_my_core_pos
/* -----------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: the ------- lines are not welcome. This multi-line comments should be like so:

     /*
      *  unsigned int plat_my_core_pos(void)
      *  This function uses the plat_marvell_calc_core_pos()
      *  definition to get the index of the calling CPU.
      */

(Same for the comment below)

* unsigned int plat_marvell_calc_core_pos(uint64_t mpidr)
* Helper function to calculate the core position.
* With this function: CorePos = (ClusterId * 2) +
* CoreId
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: CoreId can be on the previous line.

@kevin-peng-hao kevin-peng-hao force-pushed the add_marvell_platform branch 2 times, most recently from 0376de6 to 1cf6cec Compare September 19, 2017 05:45
@kevin-peng-hao
Copy link
Author

@jforissier Hi Jerome, why checkpatch checked so many commits that are not related to this Pull Request:

Running checkpatch on each patch in pull request:
3037280 core: introduce get_core_pos_mpidr()
b95ac3d core: mmu: export map_memarea_sections
1b181fb core: arm: psci: pass nsec ctx to psci
df34b18 core: arm: kernel: make thread_core_local public
86e50a6 core: arm: psci: add suspend resume common functions
49a3c15 core: arm: imx: move psci code to pm
ed74b27 core: imx7: fix comments
e621e4e core: imx: simplify code
f51f270 core: arm: imx: get mmdc type
b3c4f4f core: arm: sm: add psci power state macros
eedc47b core: arm: imx7d: remove soc_is_imx7d/s functions
1295874 core: arm: imx7d: add psci suspend support
13b3ee9 core: print rwx flags for each MMU region when a user TA aborts
3099912 scripts/symbolize.py: print ELF sections after MMU region information
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#22:

region 1: va 0x103000 pa 0xe100000 size 0x2e000 flags r-x .ta_head .text .rodata

While I don't see my commit checked?

@jforissier
Copy link
Contributor

@kevin-peng-hao because the CI script (.shippable.yml) is buggy ;) The checkpatch part was adapted from the same in .travis.yml but obviously I got something wrong. I'll fix that.

You commit passes checkpatch (I've run it manually), so all is fine. @etienne-lms do you want to give your A-b or R-b, since you commented on this PR?

@jforissier
Copy link
Contributor

#1823

@kevin-peng-hao
Copy link
Author

@jforissier ok, got it. Thanks.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

stylish comments...

#include <asm.S>
#include <arm.h>

.globl plat_my_core_pos
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: prefer tabs than space for indentation.
Here and in other source files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace plat_my_core_pos with get_core_pos.

#define PHY_2_VIR(addr) ((vaddr_t)phys_to_virt((addr), MEM_AREA_IO_SEC))

#define MCU_MC_CONTROL_0_REG PHY_2_VIR(MCU_BASE + 0x044)
#define TRUSTZONE_LOCK (0x1 << 31)
Copy link
Contributor

Choose a reason for hiding this comment

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

BIT(31)

#define ABORT_PERM 0x3

#define MAX_RANGE_NUM (16)
#define INVALID_SIZE_CODE (0xff)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parentheses


#define RA_ADDR (TZDRAM_BASE)
#define RA_SIZE (TZDRAM_SIZE)
#define RA_PERM (ABORT_PERM)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parentheses

} while (0)

#define RANGE_CODE_TO_SIZE_K(code, sizek) \
((sizek) = ((4) << (code))) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove last \
minor: fits in a single line.

i, MCU_TZ_RANGE_LOW_REG(i), tmp);
DMSG("AddrL: 0x%08" PRIx32, addr_read);
RANGE_CODE_TO_SIZE_K(sizecode_read, sizek);
sizem = (sizek >> 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parentheses

DMSG("AddrL: 0x%08" PRIx32, addr_read);
RANGE_CODE_TO_SIZE_K(sizecode_read, sizek);
sizem = (sizek >> 10);
DMSG("Size: %" PRId32 "K, %" PRId32 "M", sizek, sizem);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: s/PRId32/PRIu32/

#define SIZE_2M 0x200000
#define SIZE_4M 0x400000
#define SIZE_8M 0x800000
#define SIZE_2G 0x10000000
Copy link
Contributor

Choose a reason for hiding this comment

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

style: use tabulations for indentation.

#define MCU_BASE (0xF0020000)
#define MCU_REG_SIZE (0x1000)
#define MC_SCR_REGISTER (0xF06F0204)
#define MC_SCR_REG_SIZE (0x1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parentheses

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review. I think all fixed.


#define CONSOLE_UART_BASE PLAT_MARVELL_BOOT_UART_BASE

/* Location of trusted dram on the base fvp */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update fvp with something else.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

fix SIZE_2G


.globl get_core_pos
/*
* unsigned int plat_my_core_pos(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: comment not consistent. Maybe better remove it.

#define SIZE_2M 0x200000
#define SIZE_4M 0x400000
#define SIZE_8M 0x800000
#define SIZE_2G 0x10000000
Copy link
Contributor

Choose a reason for hiding this comment

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

#define SIZE_2G	0x80000000

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, stupid mistake.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

minor coding style comments...
To get nice consistency in optee_os source tree.

#error "Unknown platform flavor"
#endif

#define CFG_TEE_RAM_VA_SIZE (4 * 1024 * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using SIZE_4M here ?
You could consider using SIZE_4K instead of the 2 #define xxx_SIZE 0x1000 above.

Copy link
Author

Choose a reason for hiding this comment

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

I could not use SIZE_4M for CFG_TEE_RAM_VA_SIZE. Looks like kern.ls do not recognize "UL"

Copy link
Contributor

Choose a reason for hiding this comment

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

Try to use UINTPTR_C() instead of "UL"

Copy link
Author

Choose a reason for hiding this comment

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

It's not only caused by "UL", but also the different definitions of ROUND_UP in util.h and kernel/kern.ld.S.

b plat_marvell_calc_core_pos
END_FUNC get_core_pos

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: indentation below

Copy link
Author

Choose a reason for hiding this comment

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

Er...sorry, I don't quite follow

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a extra spaces to remove in the 4 lines below.

 /*
-   *  unsigned int plat_marvell_calc_core_pos(uint64_t mpidr)
-   *  Helper function to calculate the core position. 
-   *  With this function: CorePos = (ClusterId * 2) + CoreId
-   */
+  *  unsigned int get_core_pos_mpidr(uint32_t mpidr)
+  *  Helper function to calculate the core position. 
+  *  With this function: CorePos = (ClusterId * 2) + CoreId
+  */

(note: 32bit argument, use the generic label get_core_pos_mpidr as @jenswi-linaro recommends)


.globl get_core_pos

FUNC get_core_pos , :
Copy link
Contributor

Choose a reason for hiding this comment

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

With #1817 this function isn't needed

* Helper function to calculate the core position.
* With this function: CorePos = (ClusterId * 2) + CoreId
*/
FUNC plat_marvell_calc_core_pos , :
Copy link
Contributor

Choose a reason for hiding this comment

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

With #1817 this function should be named get_core_pos_mpidr()

#define TZ_GET_VALID(data, ret) ((ret) = ((data) & (0x1)))
#define TZ_SET_VALID(data, val) \
do { \
(data) &= (~(0x1)); \
Copy link
Contributor

Choose a reason for hiding this comment

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

This macro will probably only work as intended if data is of the same width as the type int

void console_init(void)
{
serial8250_uart_init(&console_data, CONSOLE_UART_BASE
, CONSOLE_UART_CLK_IN_HZ, CONSOLE_BAUDRATE);
Copy link
Contributor

Choose a reason for hiding this comment

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

The first , should be at the end of previous line.

@@ -0,0 +1,6 @@
global-incdirs-y += .
srcs-y += main.c
ifdef PLATFORM_FLAVOR_armada7k8k
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer

ifeq ($(PLATFORM_FLAVOR_armada7k8k),y)

@@ -30,6 +30,13 @@
#include <compiler.h>
#include <stdint.h>

#define SIZE_4K 0x1000
Copy link
Contributor

Choose a reason for hiding this comment

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

All these constants should be of an unsigned type, same width as size_t would be a good choice.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry but how to address this

Copy link
Contributor

Choose a reason for hiding this comment

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

We maybe something like #define SIZE_2G 0x80000000UL

@@ -30,6 +30,13 @@
#include <compiler.h>
#include <stdint.h>

#define SIZE_4K 0x1000
Copy link
Contributor

Choose a reason for hiding this comment

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

We maybe something like #define SIZE_2G 0x80000000UL

{
/* max supported granule for armada is 8TB
* but 2GB is far enough here
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

-	/* max supported granule for armada is 8TB
-	   * but 2GB is far enough here
-	   */
+	/*
+	 * Max supported granule for armada is 8TB
+	 * but 2GB is far enough here
+	 */

b plat_marvell_calc_core_pos
END_FUNC get_core_pos

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a extra spaces to remove in the 4 lines below.

 /*
-   *  unsigned int plat_marvell_calc_core_pos(uint64_t mpidr)
-   *  Helper function to calculate the core position. 
-   *  With this function: CorePos = (ClusterId * 2) + CoreId
-   */
+  *  unsigned int get_core_pos_mpidr(uint32_t mpidr)
+  *  Helper function to calculate the core position. 
+  *  With this function: CorePos = (ClusterId * 2) + CoreId
+  */

(note: 32bit argument, use the generic label get_core_pos_mpidr as @jenswi-linaro recommends)

@kevin-peng-hao
Copy link
Author

@jforissier @jenswi-linaro @etienne-lms
Hi guys, may I squash the patches and add your x-by if you have no more review comments. :)
BTW, is this patch able to catch the next release? I guess maybe not since the next release should come very shortly.

@jenswi-linaro
Copy link
Contributor

With the checkpatch errors/warnings fixed:
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

@jforissier
Copy link
Contributor

@kevin-peng-hao yes, please do squash the patches, fix the two checkpatch warnings/errors [1], and add Jens' A-b.
@etienne-lms want to ack this, too?
This patch can definitely reach the next release IMO.

[1]:

WARNING: adding a line without newline at end of file
#99: FILE: core/arch/arm/plat-marvell/armada7k8k/armada7k8k_core_pos_a64.S:41:
+END_FUNC get_core_pos_mpidr

ERROR: Macros with complex values should be enclosed in parentheses
#171: FILE: core/arch/arm/plat-marvell/armada7k8k/hal_sec_perf.c:65:
+#define TZ_SET_VALID(data)		(data) |= 0x1

@kevin-peng-hao
Copy link
Author

Hi @etienne-lms , can I add you ack in this patch?

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

A few minor comments. Thanks.

#ifndef PLATFORM_CONFIG_H
#define PLATFORM_CONFIG_H

#include <util.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not include <util.h> here because nothing in this file requires it.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's required 'cause SIZE_XX are referenced.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly needed because you're just #defining things. So the user of those things could include <util.h> instead of having it included here.
But, thinking again about it, I think it's a bad idea ;) so you can leave the include here.

@@ -44,12 +51,14 @@

#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

#ifndef ASM
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to a separate commit.

#define SIZE_2M UINTPTR_C(0x200000)
#define SIZE_4M UINTPTR_C(0x400000)
#define SIZE_8M UINTPTR_C(0x800000)
#define SIZE_2G UINTPTR_C(0x80000000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to a separate commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks ok to me. But is it the right format for asm/C to get the (unsigned long int) sized value ?

Copy link
Author

Choose a reason for hiding this comment

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

ASM seems not recognize UL or L...etc. I cannot figure out what's the difference between UL and LU. But UINTPTR_C distinguishes ASM and C, so I used it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "LU" is a typo that slipped trough because the compiler seems to accept it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, UL and LU are equivalent and are defined by the C99 standard in section 6.4.4.1:

integer-suffix:
    unsigned-suffix long-suffix opt
    unsigned-suffix long-long-suffix
    long-suffix unsigned-suffix opt
    long-long-suffix unsigned-suffix opt
unsigned-suffix: one of
    u U
long-suffix: one of
    l L
long-long-suffix: one of
    ll LL

.cpu_resume = pm_do_nothing,
.system_off = pm_do_nothing,
.system_reset = pm_do_nothing,
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

How can that happen since plat-marvell/conf.mk has $(call force,CFG_WITH_ARM_TRUSTED_FW,y)?

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Few minor comments.

Agree with @jforissier, changes in util.h should be in a specific change, before the plat-marvell/ changes.

FUNC get_core_pos_mpidr , :
and x1, x0, #MPIDR_CPU_MASK
and x0, x0, #MPIDR_CLUSTER_MASK
add x0, x1, x0, LSR #7
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get rid of this file by settings $(call force,CFG_CORE_CLUSTER_SHIFT,1) in your conk.mk, see core/arch/arm/kernel/misc_a64.S.

#define RA_PERM ABORT_PERM

#define TZ_IS_VALID(data) ((data) & (0x1))
#define TZ_SET_VALID(data) (data) |= 0x1
Copy link
Contributor

Choose a reason for hiding this comment

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

add parenthesis.

@@ -0,0 +1 @@
#include "../kernel/kern.ld.S"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: you can remove this file (since #1873)

@@ -0,0 +1 @@
include core/arch/arm/kernel/link.mk
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: you can remove this file (since #1873)

/* Round up the even multiple of size, size has to be a multiple of 2 */
#define ROUNDUP(v, size) (((v) + ((__typeof__(v))(size) - 1)) & \
~((__typeof__(v))(size) - 1))

/* Round down the even multiple of size, size has to be a multiple of 2 */
#define ROUNDDOWN(v, size) ((v) & ~((__typeof__(v))(size) - 1))
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe define an ASM version for these ROUNDDOWN/ROUNDUP.

#define ROUNDUP(x, y)           ((((x) + (y) - 1) / (y)) * (y))
#define ROUNDDOWN(x, y)         (((x) / (y)) * (y))

@jforissier
Copy link
Contributor

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

@jforissier
Copy link
Contributor

@kevin-peng-hao you have modified plat-ls/main.c by mistake and I didn't notice. My R-b applies with that change made to plat-marvell/main.c, of course.

@kevin-peng-hao
Copy link
Author

@jforissier Yeah, sure. My mistake, sorry for that.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

One last question for the 1st commit (SIZE_xx macros)
Otherwise looks ok to me.

#define SIZE_2M UINTPTR_C(0x200000)
#define SIZE_4M UINTPTR_C(0x400000)
#define SIZE_8M UINTPTR_C(0x800000)
#define SIZE_2G UINTPTR_C(0x80000000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks ok to me. But is it the right format for asm/C to get the (unsigned long int) sized value ?

@jforissier
Copy link
Contributor

@kevin-peng-hao please add the XXX-by: tags to the commit comments so that I can merge this. Thanks.

Kevin Peng added 3 commits October 16, 2017 16:50
Add some useful SIZE_XX definitions like 4k, 1M, etc...

Signed-off-by: Kevin Peng <kevinp@marvell.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
Add ROUNDDOWN and ROUNDUP definitions for ASM version which are
different from C versions

Signed-off-by: Kevin Peng <kevinp@marvell.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
Only tested 64-bit mode with default configurations:

1. build command
    make PLATFORM=marvell-armada7080 CFG_ARM64_core=y
2. Passed xtest

Signed-off-by: Kevin Peng <kevinp@marvell.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
@kevin-peng-hao
Copy link
Author

Ok, thank you guys very much.

@jforissier jforissier merged commit 8ae8c73 into OP-TEE:master Oct 16, 2017
@kevin-peng-hao kevin-peng-hao deleted the add_marvell_platform branch October 26, 2017 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants