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

[Marc Zyngier]

On 25/08/15 08:40, Chen-Yu Tsai wrote:
> 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.

As I was pointed out over IRC, the memory map is subtly different from
the original BP147 (hey, being awkward is be a good thing, isn't it?),
which makes it difficult to have common code for this.

So let's not bother having a generic BP147 setup for now - at least not
for sunxi. It could still be done for FSL and Samsung, but that'd be
unfair to ask you to do this.

Thanks,

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