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.

Sign up for GitHub

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

Jump to bottom

Fvp dram1 #1860

Merged
merged 6 commits into from
Oct 12, 2017
Merged

Fvp dram1 #1860

merged 6 commits into from
Oct 12, 2017

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

@jenswi-linaro jenswi-linaro mentioned this pull request Oct 5, 2017
Merged
etienne-lms
etienne-lms reviewed Oct 6, 2017
View reviewed changes
core/arch/arm/include/mm/core_mmu.h
};

#define __register_memory2(_name, _type, _addr, _size, _section, _id) \
static const struct core_mmu_phys_mem __phys_mem_ ## _id \
__used __section(_section) = \
{ .name = _name, .type = _type, .addr = _addr, .size = _size }

#if __SIZEOF_LONG__ != __SIZEOF_PADDR__
Copy link
Contributor

Choose a reason for hiding this comment

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

looks strange to have 2 macros register_phys_mem() and register_phys_mem_ul().
Can't use something like:

#if __SIZEOF_LONG__ == __SIZEOF_PADDR__
#define __PHYSMEM_LOC(_addr, _size) \
			.addr = (_addr), .size = (_size),
#else
#define __PHYSMEM_LOC(_addr, _size) \
			.lo_addr = (_addr), .lo_size = (_size),
#endif

#define __register_memory2(_name, _type, _addr, _size, _section, _id) \
	static const struct core_mmu_phys_mem __phys_mem_ ## _id \
		__used __section(_section) = \
 		{ .name = _name, .type = _type, __PHYSMEM_LOC(_addr, _size) },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need both the normal and the _ul version. The normal is needed when supplying 64-bit addresses from a proper constant, while the _ul version must be used when initializing from variables provided by the link script.

It's not beautiful and have some sharp corners, but I can't find a way around it.

core/arch/arm/include/kernel/linker.h
@@ -58,15 +58,15 @@ extern const struct core_mmu_phys_mem __end_phys_mem_map_section;
extern const struct core_mmu_phys_mem __start_phys_nsec_ddr_section;
extern const struct core_mmu_phys_mem __end_phys_nsec_ddr_section;

#define VCORE_UNPG_RX_PA ((paddr_t)__vcore_unpg_rx_start)
#define VCORE_UNPG_RX_PA ((unsigned long)__vcore_unpg_rx_start)
Copy link
Contributor

Choose a reason for hiding this comment

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

As these hold physical addresses/sizes, CFG_CORE_LARGE_PHYS_ADDR=y requires 64bit fields here. Why doesn't paddr_t fit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Static initialization based on addresses of variables seems to the a bit fragile. As soon as there's a 64-bit type on a 32-bit system involved the compiler tends to complain. unsigned long should be a good enough compromise for all systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@jenswi-linaro
Copy link
Contributor Author

Ping?

@etienne-lms
Copy link
Contributor

sorry.
Why not always using the UL way for register_phys_mem ?

@jenswi-linaro
Copy link
Contributor Author

The UL (which is short for unsigned long in this context) only handles 32-bit physical addresses on a 32-bit platform. In the FVP case with AArch32 that's not enough as some physical addresses are wider than that. The addresses (or constants) from the link script can always be kept in an unsigned long since they are originally based on pointers or virtual addresses.

@etienne-lms
Copy link
Contributor

Commit: "core: add register_phys_mem_ul()"

  • Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
    I still don't get why we need to register_phys_mem macros. It is not clear which one one shall use over which circumstances.

For all other commits:

  • Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

@jenswi-linaro
Copy link
Contributor Author

Tags applied.

To summarize you can say that register_phys_mem_ul() is needed whenever register_phys_mem() isn't accepted by the compiler, for instance when supplying VCORE_UNPG_RX_PA when compiling for AArch32 and CFG_CORE_LARGE_PHYS_ADDR == y).

@jforissier
Copy link
Contributor

Maybe split the EMSG line to silence the checkpatch warning?

                       EMSG("Non-sec mem (%#" PRIxPA ":%#" PRIxPASZ
                            ") overlaps map (type %d %#" PRIxPA ":%#zx)",
                             start[n].addr, start[n].size,
                             map->type, map->pa, map->size);

@jenswi-linaro
Copy link
Contributor Author

Split the EMSG() line

@jenswi-linaro jenswi-linaro mentioned this pull request Oct 11, 2017
Merged
@lorc lorc mentioned this pull request Oct 11, 2017
Merged
@jforissier
Copy link
Contributor

Can you please rebase manually? (conflict)

@jenswi-linaro
core: lpae: take nsec DDR ranges into account
6592db5 
Takes nsec DDR ranges into account when setting TCR.PS field.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
core: fix gic_init() prototype
090b89c 
Prior to this patch gic_init() incorrectly had paddr_t as type for the
GIC base addresses while the implementation used vaddr_t. The correct
type is vaddr_t which we're changing to here.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Add CFG_CORE_LARGE_PHYS_ADDR for 64bit paddr_t
bdf5355 
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
core: trivial large paddr_t fixes
cb7f218 
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
core: add register_phys_mem_ul()
c73de40 
Adds register_phys_mem_ul() which must be used (for compatibility with
CFG_CORE_LARGE_PHYS_ADDR=y) when input address and size is based on
symbols generated in the link script.

Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
plat-vexpress: fvp: add missing DRAM1
7980b88 
Defines missing DRAM1 base 0x880000000 size 0xa00000000 for FVP.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (FVP)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Rebased

@jforissier jforissier merged commit 3ff067c into OP-TEE:master Oct 12, 2017
@jenswi-linaro jenswi-linaro deleted the fvp_dram1 branch October 12, 2017 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienne-lms etienne-lms etienne-lms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jenswi-linaro @etienne-lms @jforissier