[U-Boot] [PATCH] sunxi: Enable non-secure access to RTC on sun6i (A31s)

[Marc Zyngier]

On Tue, 25 Aug 2015 10:49:19 +0800
Chen-Yu Tsai <wens at csie.org> wrote:

Hi,

Thanks for putting this patch together. A few comments below:

> On the A31s the RTC is by default secured. Thus when u-boot
> loads the kernel in non-secure world, the RTC is unavailable. The
> SoC has a TrustZone Protection Controller, which can be used to
> enable non-secure access to the RTC.
> 
> On the A31 the TZPC doesn't seem to do anything, i.e. changes to
> its register contents do not affect access to the RTC.

Does it mean that the RTC is still inaccessible? Or that it has always
been accessible?

> 
> Signed-off-by: Chen-Yu Tsai <wens at csie.org>
> ---
>  arch/arm/cpu/armv7/sunxi/Makefile      |  1 +
>  arch/arm/cpu/armv7/sunxi/board.c       |  5 +++++
>  arch/arm/cpu/armv7/sunxi/tzpc.c        | 18 ++++++++++++++++++
>  arch/arm/include/asm/arch-sunxi/tzpc.h | 23 +++++++++++++++++++++++
>  4 files changed, 47 insertions(+)
>  create mode 100644 arch/arm/cpu/armv7/sunxi/tzpc.c
>  create mode 100644 arch/arm/include/asm/arch-sunxi/tzpc.h
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
> index 76c7e55..459d5d8 100644
> --- a/arch/arm/cpu/armv7/sunxi/Makefile
> +++ b/arch/arm/cpu/armv7/sunxi/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_MACH_SUN6I)	+= clock_sun6i.o
>  obj-$(CONFIG_MACH_SUN7I)	+= clock_sun4i.o
>  obj-$(CONFIG_MACH_SUN8I)	+= clock_sun6i.o
>  obj-$(CONFIG_MACH_SUN9I)	+= clock_sun9i.o
> +obj-$(CONFIG_MACH_SUN6I)	+= tzpc.o
>  
>  obj-$(CONFIG_AXP152_POWER)	+= pmic_bus.o
>  obj-$(CONFIG_AXP209_POWER)	+= pmic_bus.o
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
> index f01846e..b40198b 100644
> --- a/arch/arm/cpu/armv7/sunxi/board.c
> +++ b/arch/arm/cpu/armv7/sunxi/board.c
> @@ -23,6 +23,7 @@
>  #include <asm/arch/gpio.h>
>  #include <asm/arch/sys_proto.h>
>  #include <asm/arch/timer.h>
> +#include <asm/arch/tzpc.h>
>  #include <asm/arch/mmc.h>
>  
>  #include <linux/compiler.h>
> @@ -115,6 +116,10 @@ void s_init(void)
>  		"orr r0, r0, #1 << 6\n"
>  		"mcr p15, 0, r0, c1, c0, 1\n");
>  #endif
> +#if defined CONFIG_MACH_SUN6I
> +	/* Enable non-secure access to the RTC */
> +	tzpc_init();
> +#endif
>  
>  	clock_init();
>  	timer_init();
> diff --git a/arch/arm/cpu/armv7/sunxi/tzpc.c b/arch/arm/cpu/armv7/sunxi/tzpc.c
> new file mode 100644
> index 0000000..5c9c69b
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/sunxi/tzpc.c
> @@ -0,0 +1,18 @@
> +/*
> + * (C) Copyright 2015 Chen-Yu Tsai <wens at csie.org>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <asm/io.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/tzpc.h>
> +
> +/* Configure Trust Zone Protection Controller */
> +void tzpc_init(void)
> +{
> +	struct sunxi_tzpc *tzpc = (struct sunxi_tzpc *)SUNXI_TZPC_BASE;
> +
> +	/* Enable non-secure access to the RTC */
> +	writel(SUNXI_TZPC_DECPORT0_RTC, &tzpc->decport0_set);

Irk. Seem blow.

> +}
> diff --git a/arch/arm/include/asm/arch-sunxi/tzpc.h b/arch/arm/include/asm/arch-sunxi/tzpc.h
> new file mode 100644
> index 0000000..ba4d43b
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-sunxi/tzpc.h
> @@ -0,0 +1,23 @@
> +/*
> + * (C) Copyright 2015 Chen-Yu Tsai <wens at csie.org>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef _SUNXI_TZPC_H
> +#define _SUNXI_TZPC_H

Why is this sunxi specific? This seems to describe a standard ARM BP147
(http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dto0015a/index.html),
so I'd expect the various offsets and the file location to reflect this.

Another platform (FSL 2085) seems to use the same IP, so it would make
sense to clean that part of the code too.

> +
> +#ifndef __ASSEMBLY__
> +struct sunxi_tzpc {
> +	u32 r0size;		/* 0x00 Size of secure RAM region */
> +	u32 decport0_status;	/* 0x04 Status of decode protection port 0 */
> +	u32 decport0_set;	/* 0x08 Set decode protection port 0 */
> +	u32 decport0_clear;	/* 0x0c Clear decode protection port 0 */
> +};

I'm not overly fond of this way to describe a set of contiguous
register. It tends to be fairly fragile (the compiler is free to
insert some padding) and prevents the use of offsets from assembly code.

> +#endif
> +
> +#define SUNXI_TZPC_DECPORT0_RTC	(1 << 1)
> +
> +void tzpc_init(void);

And this won't help if you have assembly code either.

> +
> +#endif /* _SUNXI_TZPC_H */

If I can suggest something, it would be to make this something generic,
with a standard config option describing the base, a set of offsets
from the base (as #defines), and then a set of accessors accessible from
C code.

LS2085 could also be cleaned up to reuse the same offsets (instead of
hardcoding the register addresses).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.