On Tue, 27 Mar 2018, Kever Yang wrote:
The configure_l2ctlr() is used only by rk3288, do not need to locate in sys_proto.h
Please elaborate on what the function does and why it is not needed by any of the other SOCs (after all: it has been available to all SOCs so far).
Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
This should be a standalone patch and doesn't need to be part of the series it is in. This series has way too many different things happening at once and needs to be broken up into individual series that do one well-defined thing each.
See below for requested changes.
arch/arm/include/asm/arch-rockchip/sys_proto.h | 22 ---------------------- arch/arm/mach-rockchip/rk3288/rk3288.c | 26 +++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/arch/arm/include/asm/arch-rockchip/sys_proto.h b/arch/arm/include/asm/arch-rockchip/sys_proto.h index e428d59..3617ac2 100644 --- a/arch/arm/include/asm/arch-rockchip/sys_proto.h +++ b/arch/arm/include/asm/arch-rockchip/sys_proto.h @@ -7,27 +7,5 @@ #ifndef _ASM_ARCH_SYS_PROTO_H #define _ASM_ARCH_SYS_PROTO_H
-#ifdef CONFIG_ROCKCHIP_RK3288 -#include <asm/armv7.h>
-static void configure_l2ctlr(void) -{
- uint32_t l2ctlr;
- l2ctlr = read_l2ctlr();
- l2ctlr &= 0xfffc0000; /* clear bit0~bit17 */
- /*
- Data RAM write latency: 2 cycles
- Data RAM read latency: 2 cycles
- Data RAM setup latency: 1 cycle
- Tag RAM write latency: 1 cycle
- Tag RAM read latency: 1 cycle
- Tag RAM setup latency: 1 cycle
- */
- l2ctlr |= (1 << 3 | 1 << 0);
- write_l2ctlr(l2ctlr);
-} -#endif /* CONFIG_ROCKCHIP_RK3288 */
#endif /* _ASM_ARCH_SYS_PROTO_H */ diff --git a/arch/arm/mach-rockchip/rk3288/rk3288.c b/arch/arm/mach-rockchip/rk3288/rk3288.c index acc3b79..1e1c6be 100644 --- a/arch/arm/mach-rockchip/rk3288/rk3288.c +++ b/arch/arm/mach-rockchip/rk3288/rk3288.c @@ -3,15 +3,39 @@
- SPDX-License-Identifier: GPL-2.0+
*/ +#include <asm/armv7.h> #include <asm/io.h> #include <asm/arch/hardware.h>
#define GRF_SOC_CON2 0xff77024c
Please make this a const-declaration in the function it is needed in.
+#ifdef CONFIG_SPL_BUILD
Should this really happen both for TPL and SPL?
+static void configure_l2ctlr(void) +{
- u32 l2ctlr;
- l2ctlr = read_l2ctlr();
- l2ctlr &= 0xfffc0000; /* clear bit0~bit17 */
What are bits 0...17?
- /*
* Data RAM write latency: 2 cycles* Data RAM read latency: 2 cycles* Data RAM setup latency: 1 cycle* Tag RAM write latency: 1 cycle* Tag RAM read latency: 1 cycle* Tag RAM setup latency: 1 cycle*/
Please add a symbolic way to assemble these (i.e. something that makes it easy for the casual reader to see what values you are writing to which bitfields).
- l2ctlr |= (1 << 3 | 1 << 0);
From the "clear bit0 ~ bit17" and this, I assume you actually want to do a
clrsetbits_le32...
- write_l2ctlr(l2ctlr);
+} +#endif
int arch_cpu_init(void) { /* We do some SoC one time setting here. */
+#ifdef CONFIG_SPL_BUILD
- configure_l2ctlr();
+#else /* Use rkpwm by default */ rk_setreg(GRF_SOC_CON2, 1 << 0);
Please use a symbolic way to write the (1 << 0), wo it is easy for the casual reader to see what gets enabled/disabled here.