Hi,
On 25-08-15 09:40, Chen-Yu Tsai wrote:
On Tue, Aug 25, 2015 at 3:34 PM, Marc Zyngier marc.zyngier@arm.com wrote:
On Tue, 25 Aug 2015 10:49:19 +0800 Chen-Yu Tsai wens@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?
Means its always accessible.
Ah, so what you're saying is that on A31 the RTC is always accessible and on A31s it depends on the TZPC settings ?
That does not make sense since both are the same die used in a different package, so chances are this does not depend on A31 vs A31s but on a difference in revision of the die.
It does not matter, we can just flip the bit in the TZPC everywhere to make sure the kernel has access to the RTC.
Regards,
Hans
Signed-off-by: Chen-Yu Tsai wens@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@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@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.h...), 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.
I guess we could make this generic. Seems samsung uses this as well.
+#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.
Seems this is how we are doing things in U-boot... I'm OK either way.
+#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).
OK. Will look into it.
ChenYu