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

Introduce RD-1 AE Platform #7065

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Introduce RD-1 AE Platform #7065

merged 3 commits into from
Oct 11, 2024

Conversation

ZiadElhanafy
Copy link
Contributor

1- Add initial support for RD-1 AE platform, this includes:
a- GIC and console initialization functions.
b- Memory layout.
c- Make files.
d- Assembly function get_core_pos_mpidr to compute the linear core position from MPIDR.

2- Accept GIC version 4 if CFG_ARM_GICV3 is enabled


CFG_CORE_SEL1_SPMC = y
CFG_WITH_ARM_TRUSTED_FW = y
CFG_CORE_RESERVED_SHM = n
Copy link
Contributor

Choose a reason for hiding this comment

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

replace = with ?= of $(call force,CFG_xxx,<value>)
Ditto at line 32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#define PLATFORM_CORE_COUNT (PLAT_RD1AE_CHIP_COUNT * \
PLAT_ARM_CLUSTER_COUNT * \
RD1AE_MAX_CPUS_PER_CLUSTER * \
RD1AE_MAX_PE_PER_CPU)
Copy link
Contributor

Choose a reason for hiding this comment

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

PLATFORM_CORE_COUNT seems unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


void boot_primary_init_intc(void)
{
gic_init(GICC_BASE, GICD_BASE);
Copy link
Contributor

Choose a reason for hiding this comment

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

GICR unused? Use gic_init_v3()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is unused, thanks for noticing. Removed #define GICR_BASE

arm64-platform-cflags += -mcpu=$(arm64-platform-cpuarch)
arm64-platform-aflags += -mcpu=$(arm64-platform-cpuarch)
arm64-platform-cxxflags += -mcpu=$(arm64-platform-cpuarch)
platform-flavor-armv9 := 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth creating a new core/arch/arm/cpu/cortex-armv9.mk file for these directives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a neoverse-v2.mk (Actually it is a Neoverse V3AE to be more precise but there is no arm64-platform-cpuarch that I could find yet for it). Would that be okay ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't knw. Do armv9 variants need each a specific toolachin artchitecture directives? On armv8.x, for example, we build all Arm 64bit machine with -mcpu=cortex-a53 even if not targeting a Cortex-A53 core.

Copy link
Contributor Author

@ZiadElhanafy ZiadElhanafy Oct 4, 2024

Choose a reason for hiding this comment

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

That would be fine here too.

My question is, is it okay to create an .mk file as follows ?

core/arch/arm/cpu/neoverse-v2.mk

$(call force,CFG_HWSUPP_MEM_PERM_WXN,y)
$(call force,CFG_HWSUPP_MEM_PERM_PXN,y)
$(call force,CFG_ENABLE_SCTLR_RR,n)
$(call force,CFG_ENABLE_SCTLR_Z,n)

arm64-platform-cpuarch 	:= neoverse-v2
arm64-platform-cflags 	+= -mcpu=$(arm64-platform-cpuarch)
arm64-platform-aflags 	+= -mcpu=$(arm64-platform-cpuarch)
arm64-platform-cxxflags	+= -mcpu=$(arm64-platform-cpuarch)
platform-flavor-armv9 := 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be named core/arch/arm/cpu/neoverse-v2.mk if applicable to all armv9 platforms?
If not, maybe it's better to keep these in your plat-rd1ae/conf.mk file. There will be time for factorization when other armv9 based platforms are integrated.
@jforissier, @jenswi-linaro, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it will not be applicable to all armv9 platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe create core/arch/arm/cpu/neoverse-v2.mk which would include core/arch/arm/cpu/armv9.mk (common part)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sounds good, just to confirm what you intend

1- core/arch/arm/cpu/cortex-armv9.mk would be:

$(call force,CFG_HWSUPP_MEM_PERM_WXN,y)
$(call force,CFG_HWSUPP_MEM_PERM_PXN,y)
$(call force,CFG_ENABLE_SCTLR_RR,n)
$(call force,CFG_ENABLE_SCTLR_Z,n)

arm64-platform-cflags 	+= -mcpu=$(arm64-platform-cpuarch)
arm64-platform-aflags 	+= -mcpu=$(arm64-platform-cpuarch)
arm64-platform-cxxflags	+= -mcpu=$(arm64-platform-cpuarch)
platform-flavor-armv9 := 1

2- core/arch/arm/cpu/neoverse-v2.mk would be:

include core/arch/arm/cpu/cortex-armv9.mk

arm64-platform-cpuarch 	:= neoverse-v2

Is that right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. In (2) it is preferable to swap the two lines. Although not strictly necessary since arm64-platform-cflags is currently a recursive variable (with deferred assignment) (A = ...), we tend to prefer immediate assignments (A := ...) because they are easier to follow and it is how the conditional assignment behaves (A ?= ...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

  • For "core: introduce Arm Cortex-v9 and Neoverse-v2 CPU support":
    Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
  • See comments below on "plat-rd1ae: introduce RD-1 AE platform support"
  • For "core: gic: accept GIC version 4 if CFG_ARM_GICV3 is enabled":
    Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

core/arch/arm/plat-rd1ae/conf.mk Show resolved Hide resolved
core/arch/arm/plat-rd1ae/conf.mk Outdated Show resolved Hide resolved
core/arch/arm/plat-rd1ae/conf.mk Outdated Show resolved Hide resolved
core/arch/arm/plat-rd1ae/conf.mk Outdated Show resolved Hide resolved
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.

For "plat-rd1ae: introduce RD-1 AE platform support":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@@ -0,0 +1,3 @@
arm64-platform-cpuarch := neoverse-v2
Copy link
Contributor

Choose a reason for hiding this comment

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

No arm32 support for EL0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain more why would we need arm32 support here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

OP-TEE supports loading and executing both AArch64 and AArch32 TAs at S-EL0. I don't see a reason to stop it if the hardware supports it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it wouldn't hurt to add it.
Excuse me if it is obvious, but would it be achieved by simply adding the following ?

In neoverse-v2.mk

arm32-platform-cpuarch 	:= neoverse-v2

and in cortex-armv9.mk:

arm32-platform-cflags 	+= -mcpu=$(arm32-platform-cpuarch)
arm32-platform-aflags 	+= -mcpu=$(arm32-platform-cpuarch)
arm32-platform-cxxflags	+= -mcpu=$(arm32-platform-cpuarch)

Or is there something extra that needs to be done ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

platform-cflags-debug-info = -gdwarf-4
platform-aflags-debug-info = -gdwarf-4

CFG_ARM64_core ?= y
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, this should be forced since armv9 doesn't support AArch32 at EL1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jenswi-linaro
Copy link
Contributor

With my comment addressed, please apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

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.

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> with a minor comment.

#define DRAM0_SIZE 0x80000000UL

#define DRAM1_BASE 0x8080000000ULL
#define DRAM1_SIZE 0x80000000ULL
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: would prefer UL(xxx)/ULL(xxx) instead of xxxUL/xxxULL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Introduce cortex-armv9.mk file and use it to support the
Armv9 Neoverse v2 CPU.

Signed-off-by: Ziad Elhanafy <ziad.elhanafy@arm.com>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Add initial support for RD-1 AE platform, this includes:
1- GIC and console initialization functions.
2- Memory layout.
3- Make files.
4- Assembly function `get_core_pos_mpidr` to compute the
   linear core position from MPIDR.

Signed-off-by: Ziad Elhanafy <ziad.elhanafy@arm.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
GIC v4 is backwards compatible with GIC v3, Accept GIC
version 4 if CFG_ARM_GICV3 is enabled.

Signed-off-by: Ziad Elhanafy <ziad.elhanafy@arm.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier jforissier merged commit dd18bd8 into OP-TEE:master Oct 11, 2024
9 checks passed
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