Hi Philipp,
On 04/02/2018 04:47 AM, Philipp Tomsich wrote: > > > 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). It does not available to all SoCs, only rk3288-board-spl use this function. Jagan move this function from rk3288-board-spl.c to sys_proto.h because he think both spl and tpl needs it: a982d51 armv7: rk3288: Move configure_l2ctlr to common I move this function back to rk3288 AS-IS and only with checkpatch fix, I don't think I have to add so much explanation for this AS-IS function copy-paste. > >> Signed-off-by: Kever Yang <kever.y...@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.toms...@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. This const-declaration does not belong to this patch, isn't it? > >> >> +#ifdef CONFIG_SPL_BUILD > > Should this really happen both for TPL and SPL? I think we only need do this once, if TPL do it, then SPL no need to do it again. > >> +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... Not really, this write operation is a 'mcr' write to cp15. > >> + 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. Again, this content does not belong to this patch, Thanks, - Kever > >> >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot