Re: [PATCH 01/27] ARM: EXYNOS: Add Exynos3250 SoC ID

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi,

On 04/11/2014 10:46 AM, Olof Johansson wrote:
> On Thu, Apr 10, 2014 at 06:37:12PM +0900, Chanwoo Choi wrote:
>> This patch add Exynos3250's SoC ID. Exynos 3250 is System-On-Chip(SoC) that
>> is based on the 32-bit RISC processor for Smartphone. Exynos3250 uses Cortex-A7
>> dual cores and has a target speed of 1.0GHz.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  arch/arm/mach-exynos/Kconfig             | 22 ++++++++++++++++++++++
>>  arch/arm/mach-exynos/exynos.c            |  1 +
>>  arch/arm/plat-samsung/include/plat/cpu.h | 10 ++++++++++
>>  3 files changed, 33 insertions(+)
>>
>> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
>> index fc8bf18..6da8a68 100644
>> --- a/arch/arm/mach-exynos/Kconfig
>> +++ b/arch/arm/mach-exynos/Kconfig
>> @@ -11,6 +11,17 @@ if ARCH_EXYNOS
>>  
>>  menu "SAMSUNG EXYNOS SoCs Support"
>>  
>> +config ARCH_EXYNOS3
>> +	bool "SAMSUNG EXYNOS3"
>> +	select ARM_AMBA
>> +	select CLKSRC_OF
>> +	select HAVE_ARM_SCU if SMP
>> +	select HAVE_SMP
>> +	select PINCTRL
>> +	select PM_GENERIC_DOMAINS if PM_RUNTIME
>> +	help
>> +	  Samsung EXYNOS3 SoCs based systems
>> +
>>  config ARCH_EXYNOS4
>>  	bool "SAMSUNG EXYNOS4"
>>  	default y
>> @@ -41,6 +52,17 @@ config ARCH_EXYNOS5
>>  
>>  comment "EXYNOS SoCs"
>>  
>> +config SOC_EXYNOS3250
>> +	bool "SAMSUNG EXYNOS3250"
>> +	default y
>> +	depends on ARCH_EXYNOS3
>> +	select ARCH_HAS_BANDGAP
>> +	select ARM_CPU_SUSPEND if PM
>> +	select PINCTRL_EXYNOS
>> +	select SAMSUNG_DMADEV
>> +	help
>> +	  Enable EXYNOS3250 CPU support
>> +
>>  config CPU_EXYNOS4210
>>  	bool "SAMSUNG EXYNOS4210"
>>  	default y
>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
>> index b32a907..b134868 100644
>> --- a/arch/arm/mach-exynos/exynos.c
>> +++ b/arch/arm/mach-exynos/exynos.c
>> @@ -370,6 +370,7 @@ static void __init exynos_dt_machine_init(void)
>>  }
>>  
>>  static char const *exynos_dt_compat[] __initconst = {
>> +	"samsung,exynos3250",
> 
> Please consider samsung,exynos3 instead, so you don't have to update this table
> for every SoC. We've talked about this before..

This patchset included only exynos3250.dtsi without exynos3.dtsi.
So, I added only "samsung,exynos3250" compatible name.

Do you prefer to add SoC version as following?
+       "samsung,exynos3",
+       "samsung,exynos3250",

or ?
+       "samsung,exynos3",

> 
>>  	"samsung,exynos4",
>>  	"samsung,exynos4210",
>>  	"samsung,exynos4212",
>> diff --git a/arch/arm/plat-samsung/include/plat/cpu.h b/arch/arm/plat-samsung/include/plat/cpu.h
>> index 5992b8d..3d808f6b 100644
>> --- a/arch/arm/plat-samsung/include/plat/cpu.h
>> +++ b/arch/arm/plat-samsung/include/plat/cpu.h
>> @@ -43,6 +43,9 @@ extern unsigned long samsung_cpu_id;
>>  #define S5PV210_CPU_ID		0x43110000
>>  #define S5PV210_CPU_MASK	0xFFFFF000
>>  
>> +#define EXYNOS3250_SOC_ID       0xE3472000
>> +#define EXYNOS3_SOC_MASK        0xFFFFF000
>> +
>>  #define EXYNOS4210_CPU_ID	0x43210000
>>  #define EXYNOS4212_CPU_ID	0x43220000
>>  #define EXYNOS4412_CPU_ID	0xE4412200
>> @@ -68,6 +71,7 @@ IS_SAMSUNG_CPU(s5p6440, S5P6440_CPU_ID, S5P64XX_CPU_MASK)
>>  IS_SAMSUNG_CPU(s5p6450, S5P6450_CPU_ID, S5P64XX_CPU_MASK)
>>  IS_SAMSUNG_CPU(s5pc100, S5PC100_CPU_ID, S5PC100_CPU_MASK)
>>  IS_SAMSUNG_CPU(s5pv210, S5PV210_CPU_ID, S5PV210_CPU_MASK)
>> +IS_SAMSUNG_CPU(exynos3250, EXYNOS3250_SOC_ID, EXYNOS3_SOC_MASK)
>>  IS_SAMSUNG_CPU(exynos4210, EXYNOS4210_CPU_ID, EXYNOS4_CPU_MASK)
>>  IS_SAMSUNG_CPU(exynos4212, EXYNOS4212_CPU_ID, EXYNOS4_CPU_MASK)
>>  IS_SAMSUNG_CPU(exynos4412, EXYNOS4412_CPU_ID, EXYNOS4_CPU_MASK)
>> @@ -126,6 +130,12 @@ IS_SAMSUNG_CPU(exynos5440, EXYNOS5440_SOC_ID, EXYNOS5_SOC_MASK)
>>  # define soc_is_s5pv210()	0
>>  #endif
>>  
>> +#if defined(CONFIG_SOC_EXYNOS3250)
>> +# define soc_is_exynos3250()    is_samsung_exynos3250()
>> +#else
>> +# define soc_is_exynos3250()    0
>> +#endif
> 
> In general, I think we have too much code littered with soc_is_<foo>() going
> on, so please try to avoid adding more for this SoC. Especially in cases where
> you just want to bail out of certain features where we might already have
> function pointers to control if a function is called or not, such as the
> firmware interfaces.
> 

Do you prefer dt helper function such as following function instead of new soc_is_xx() ?
- of_machine_is_compatible("samsung,exynos3250")

If you are OK, I'll use of_machine_is_compatible() instead of soc_is_xx().

Best Regards,
Chanwoo Choi