| Message ID | 1522142971-20739-2-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | rockchip: clean up board file for rockchip SoCs | expand |
> The configure_l2ctlr() is used only by rk3288, do not need to > locate in sys_proto.h > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > 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(-) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
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. > >
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@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. 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 > >> >> >
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 +#ifdef CONFIG_SPL_BUILD +static void configure_l2ctlr(void) +{ + u32 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 + 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);
The configure_l2ctlr() is used only by rk3288, do not need to locate in sys_proto.h Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- 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(-)