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

[Kever Yang]

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.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.

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
>
>>
>>
>