Dear Olof and Tomasz, On 04/11/2014 05:39 PM, Tomasz Figa wrote: > On 11.04.2014 08:32, Chanwoo Choi wrote: >> 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.c...@samsung.com> >>>> Signed-off-by: Kyungmin Park <kyungmin.p...@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. > > There is no direct relation between dts file names and compatible string > (although usually they correspond). You don't need exynos3.dtsi (at least > until another SoC from this family shows up). > >> >> Do you prefer to add SoC version as following? >> + "samsung,exynos3", >> + "samsung,exynos3250", >> >> or ? >> + "samsung,exynos3", > > This is actually a good question. If adding exynos3 anyway, it probably > wouldn't hurt to add exynos3250 anyway, to avoid adding it in future if some > SoC specific quirks show up, especially when both of compatible strings need > to be documented anyway. > >> >>> >>>> "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(). > > First of all, there is still a lot of code in mach-exynos/ using the > soc_is_xx() macros, so having some SoCs use them and other SoCs use > of_machine_is_compatible() wouldn't make the code cleaner. > > For now, I wouldn't mind adding soc_is_exynos3250(), but in general such code > surrounded with if (soc_is_xx()) blocks should be reworked to use something > better, for example function pointers, as Olof suggested.
I thought 'function pointers' method instead of soc_is_xxx() macro as following two case: I need more detailed explanation/example of "for example function pointers, as Olof suggested." sentence. [case 1] Each Exynos SoC has other function pointers according to compatible name of DT. For example, arch/arm/mach-exynos/firmware.c static const struct firmware_ops exynos_firmware_ops = { .do_idle = exynos_do_idle, .set_cpu_boot_addr = exynos_set_cpu_boot_addr, .cpu_boot = exynos_cpu_boot, }; static const struct firmware_ops exynos3250_firmware_ops = { .do_idle = exynos_do_idle, .set_cpu_boot_addr = exynos4212_set_cpu_boot_addr, .cpu_boot = exynos3250_cpu_boot, }; static const struct firmware_ops exynos4212_firmware_ops = { .do_idle = exynos_do_idle, .set_cpu_boot_addr = exynos4212_set_cpu_boot_addr, .cpu_boot = exynos4212_cpu_boot, }; struct secure_firmware { char *name; const struct firmware_ops *ops; } exynos_secure_firmware[] __initconst = { { "samsung,secure-firmware", &exynos_firmware_ops }, { "samsung,exynos3250-secure-firmware", &exynos3250_firmware_ops }, { "samsung,exynos4212-secure-firmware", &exynos4212_firmware_ops }, }; [case 2] Delete all the soc_is_xxx() macro and then use only get_samsung_soc_id() function as following: switch (get_samsung_soc_id()) { case EXYNOS3250_SOC_ID: // ... break; case EXYNOS4210_CPU_ID: // ... break; case EXYNOS4212_CPU_ID: // ... break; case EXYNOS4412_CPU_ID: // ... break; case EXYNOS5250_SOC_ID: // ... break; case EXYNOS5420_SOC_ID: // ... break; case EXYNOS5440_SOC_ID: // ... break; }; Best Regards, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/