Hi Chanwoo,

On 14.04.2014 07:13, Chanwoo Choi wrote:
On 04/11/2014 05:39 PM, Tomasz Figa wrote:
On 11.04.2014 08:32, Chanwoo Choi wrote:
On 04/11/2014 10:46 AM, Olof Johansson wrote:
On Thu, Apr 10, 2014 at 06:37:12PM +0900, Chanwoo Choi wrote:
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 },
};


This is probably the right solution. Another would be to detect which firmware ops to use by matching root node with particular SoC compatible strings.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to