On 25/08/15 08:40, Chen-Yu Tsai wrote:
> 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.
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...
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
