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

[Chen-Yu Tsai]

On Tue, Aug 25, 2015 at 3:34 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> 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?

Means its always 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.

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