From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Thu, 15 Jan 2015 16:59:12 -0700
Subject: [U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration
 options and reserve secure code
In-Reply-To: <1421178360-23778-3-git-send-email-ijc@hellion.org.uk>
References: <1421178290.11796.159.camel@hellion.org.uk>
	<1421178360-23778-3-git-send-email-ijc@hellion.org.uk>
Message-ID: <54B85450.1030504@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 01/13/2015 12:45 PM, Ian Campbell wrote:
> The secure world code is relocated to the MB just below the top of 4G, we
> reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is
> not protected in h/w. See next patch.

> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h

> +#define CONFIG_ARMV7_PSCI			1
> +/* Reserve top 1M for secure RAM */
> +#define CONFIG_ARMV7_SECURE_BASE		0xfff00000
> +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE	0x00100000

I /think/ the assumption in the existing code is that 
CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, 
and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory 
if that symbol is *not* set? That seems like rather a confusing semantic 
given the variable name. Introducing a new define that looks like it's 
simply the size of that region but actually changes the reservation 
semantics makes the situation worse for me.

Wouldn't it be better to have:

CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.

CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the 
secure base is in DRAM or not.

That define would default to unset and you'd get the current behaviour.

If that define was set, then CONFIG_ARMV7_SECURE_BASE through 
CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be 
reserved in RAM?

That way, armv7_update_dt would be more like:

int armv7_update_dt(void *fdt)
{
#if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \
		!defined(CONFIG_ARMV7_SECURE_BASE)
         /* secure code lives in RAM, keep it alive */
#if defined(CONFIG_ARMV7_SECURE_BASE)
	base = CONFIG_ARMV7_SECURE_BASE;
#else
	base = __secure_start;
#endif
         fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start);
#endif

         return fdt_psci(fdt);
}