| Message ID | 1522142971-20739-3-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 |
> All rockchip SoCs can use ARM arch timer, let's enable it in > common header file > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > include/configs/rk3368_common.h | 2 -- > include/configs/rk3399_common.h | 2 -- > include/configs/rockchip-common.h | 4 ++++ > 3 files changed, 4 insertions(+), 4 deletions(-) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
On Tue, 27 Mar 2018, Kever Yang wrote: > All rockchip SoCs can use ARM arch timer, let's enable it in > common header file Please provide a commit message that is more descriptive of what actually happens... i.e. that COUNTER_FREQUENCY gets moved to a common header. It would be great to document why this will always remain 24M. > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> See below for requested changes. > --- > > include/configs/rk3368_common.h | 2 -- > include/configs/rk3399_common.h | 2 -- > include/configs/rockchip-common.h | 4 ++++ > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/configs/rk3368_common.h b/include/configs/rk3368_common.h > index 10f643f..a7fe4ca 100644 > --- a/include/configs/rk3368_common.h > +++ b/include/configs/rk3368_common.h > @@ -22,8 +22,6 @@ > #define CONFIG_SYS_CBSIZE 1024 > #define CONFIG_SKIP_LOWLEVEL_INIT > > -#define COUNTER_FREQUENCY 24000000 > - > #define CONFIG_SYS_NS16550_MEM32 > > #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 > diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h > index d700bf2..fe8c675 100644 > --- a/include/configs/rk3399_common.h > +++ b/include/configs/rk3399_common.h > @@ -17,8 +17,6 @@ > #define CONFIG_SPL_SPI_LOAD > #endif > > -#define COUNTER_FREQUENCY 24000000 > - > #define CONFIG_SYS_NS16550_MEM32 > > #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 > diff --git a/include/configs/rockchip-common.h b/include/configs/rockchip-common.h > index 26d41b5..24651ce 100644 > --- a/include/configs/rockchip-common.h > +++ b/include/configs/rockchip-common.h > @@ -8,6 +8,10 @@ > #define _ROCKCHIP_COMMON_H_ > #include <linux/sizes.h> > > +#define COUNTER_FREQUENCY 24000000 Is this really safe for all past, current and future SOCs (after all: you are putting this into 'rockchip-common.h'? > +#define CONFIG_SYS_ARCH_TIMER I don't agree with putting this here, as the CONFIG_SYS_ARCH_TIMER definition is only used on ARMv7, but this file is also included by ARMv8 SOCs. > +#define CONFIG_SYS_HZ_CLOCK 24000000 You might want to have this refer back to COUNTER_FREQUENCY. > + > #ifndef CONFIG_SPL_BUILD > > /* First try to boot from SD (index 0), then eMMC (index 1) */ >
On 04/02/2018 04:51 AM, Philipp Tomsich wrote: > > > On Tue, 27 Mar 2018, Kever Yang wrote: > >> All rockchip SoCs can use ARM arch timer, let's enable it in >> common header file > > Please provide a commit message that is more descriptive of what > actually happens... i.e. that COUNTER_FREQUENCY gets moved to a common > header. Well, will add this info. > It would be great to document why this will always remain 24M. All the setting in header file for Rockchip is 24M, doesn't it already a common code and we should re-use it in common file? > >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > > See below for requested changes. > >> --- >> >> include/configs/rk3368_common.h | 2 -- >> include/configs/rk3399_common.h | 2 -- >> include/configs/rockchip-common.h | 4 ++++ >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/include/configs/rk3368_common.h >> b/include/configs/rk3368_common.h >> index 10f643f..a7fe4ca 100644 >> --- a/include/configs/rk3368_common.h >> +++ b/include/configs/rk3368_common.h >> @@ -22,8 +22,6 @@ >> #define CONFIG_SYS_CBSIZE 1024 >> #define CONFIG_SKIP_LOWLEVEL_INIT >> >> -#define COUNTER_FREQUENCY 24000000 >> - >> #define CONFIG_SYS_NS16550_MEM32 >> >> #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 >> diff --git a/include/configs/rk3399_common.h >> b/include/configs/rk3399_common.h >> index d700bf2..fe8c675 100644 >> --- a/include/configs/rk3399_common.h >> +++ b/include/configs/rk3399_common.h >> @@ -17,8 +17,6 @@ >> #define CONFIG_SPL_SPI_LOAD >> #endif >> >> -#define COUNTER_FREQUENCY 24000000 >> - >> #define CONFIG_SYS_NS16550_MEM32 >> >> #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 >> diff --git a/include/configs/rockchip-common.h >> b/include/configs/rockchip-common.h >> index 26d41b5..24651ce 100644 >> --- a/include/configs/rockchip-common.h >> +++ b/include/configs/rockchip-common.h >> @@ -8,6 +8,10 @@ >> #define _ROCKCHIP_COMMON_H_ >> #include <linux/sizes.h> >> >> +#define COUNTER_FREQUENCY 24000000 > > Is this really safe for all past, current and future SOCs (after all: > you are putting this into 'rockchip-common.h'? Rockchip timer always have a option of 24M, it may not default in SoC value, we need to select to use 24M in this case. > >> +#define CONFIG_SYS_ARCH_TIMER > > I don't agree with putting this here, as the CONFIG_SYS_ARCH_TIMER > definition is only used on ARMv7, but this file is also included by > ARMv8 SOCs. Does this setting break anything in ARMv8? I don't think we need to add a rockchip_common_armv7.h, at least we can add macro for the definition is used for CONFIG_ARM64 or not. Thanks, - Kever > >> +#define CONFIG_SYS_HZ_CLOCK 24000000 > > You might want to have this refer back to COUNTER_FREQUENCY. > >> + >> #ifndef CONFIG_SPL_BUILD >> >> /* First try to boot from SD (index 0), then eMMC (index 1) */ >> >
diff --git a/include/configs/rk3368_common.h b/include/configs/rk3368_common.h index 10f643f..a7fe4ca 100644 --- a/include/configs/rk3368_common.h +++ b/include/configs/rk3368_common.h @@ -22,8 +22,6 @@ #define CONFIG_SYS_CBSIZE 1024 #define CONFIG_SKIP_LOWLEVEL_INIT -#define COUNTER_FREQUENCY 24000000 - #define CONFIG_SYS_NS16550_MEM32 #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h index d700bf2..fe8c675 100644 --- a/include/configs/rk3399_common.h +++ b/include/configs/rk3399_common.h @@ -17,8 +17,6 @@ #define CONFIG_SPL_SPI_LOAD #endif -#define COUNTER_FREQUENCY 24000000 - #define CONFIG_SYS_NS16550_MEM32 #define CONFIG_SYS_INIT_SP_ADDR 0x00300000 diff --git a/include/configs/rockchip-common.h b/include/configs/rockchip-common.h index 26d41b5..24651ce 100644 --- a/include/configs/rockchip-common.h +++ b/include/configs/rockchip-common.h @@ -8,6 +8,10 @@ #define _ROCKCHIP_COMMON_H_ #include <linux/sizes.h> +#define COUNTER_FREQUENCY 24000000 +#define CONFIG_SYS_ARCH_TIMER +#define CONFIG_SYS_HZ_CLOCK 24000000 + #ifndef CONFIG_SPL_BUILD /* First try to boot from SD (index 0), then eMMC (index 1) */
All rockchip SoCs can use ARM arch timer, let's enable it in common header file Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- include/configs/rk3368_common.h | 2 -- include/configs/rk3399_common.h | 2 -- include/configs/rockchip-common.h | 4 ++++ 3 files changed, 4 insertions(+), 4 deletions(-)