[U-Boot] [U-Boot, 01/36] rockchip: rk3288: move configure_l2ctlr back to rk3288

[Philipp Tomsich]

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 at rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich at 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.

>
>