On Tue, Aug 25, 2015 at 3:34 PM, Marc Zyngier <marc.zyng...@arm.com> wrote:
> On Tue, 25 Aug 2015 10:49:19 +0800
> Chen-Yu Tsai <w...@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.
>>
>> Signed-off-by: Chen-Yu Tsai <w...@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 <w...@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 <w...@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.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
