[PATCH 1/7] ARM: EXYNOS: add support for EXYNOS5440 SoC

Tomasz Figa wrote:

Hi Kgene,

Hi,

[...]

quoted

 void exynos5_restart(char mode, const char *cmd)
 {
-	__raw_writel(0x1, EXYNOS_SWRESET);
+	u32 val;
+	void __iomem *addr;
+
+	if (of_machine_is_compatible("samsung,exynos5250")) {
+		val = 0x1;
+		addr = EXYNOS_SWRESET;
+	} else if (of_machine_is_compatible("samsung,exynos5440")) {
+		val = (0x10 << 20) | (0x1 << 16);
+		addr = EXYNOS5440_SWRESET;
+	} else {
+		pr_err("%s: cannot support non-DT\n", __func__);
+		return;
+	}

Why soc_is_XXX isn't used here? It should be faster and more correct than
of_machine_is_compatible.

Well...let me check again.

I can imagine the same board available with two different SoCs, for which
of_machine_is_compatible wouldn't work.

I don't think so. Basically, the restart() depends on SoC not board in
addition, each board is supposed to have its own SoC not different SoCs.

[...]

quoted

-static char const *exynos5250_dt_compat[] __initdata = {
+static char const *exynos5_dt_compat[] __initdata = {
 	"samsung,exynos5250",
+	"samsung,exynos5440",
 	NULL
 };

Something doesn't seem right here. How do you distinguish between
MACH_EXYNOS5_DT and MACH_EXYNOS5440_DT if both have the same compatible
matches?

I updated to support MACH_EXYNOS5440_DT with MACH_EXYNOS5_DT.

Those machines doesn't seem to share much definitions, so maybe a separate
mach-exynos5440-dt.c file would be a better approach?

See my updated patch.

quoted

@@ -96,11 +100,23 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5

(Flattened Device Tree)") /* Maintainer: Kukjin Kim
[off-list ref] */
 	.init_irq	= exynos5_init_irq,
 	.smp		= smp_ops(exynos_smp_ops),
-	.map_io		= exynos5250_dt_map_io,
+	.map_io		= exynos5_dt_map_io,
 	.handle_irq	= gic_handle_irq,
-	.init_machine	= exynos5250_dt_machine_init,
+	.init_machine	= exynos5_dt_machine_init,
 	.init_late	= exynos_init_late,
 	.timer		= &exynos4_timer,
-	.dt_compat	= exynos5250_dt_compat,
+	.dt_compat	= exynos5_dt_compat,
+	.restart        = exynos5_restart,
+MACHINE_END
+
+DT_MACHINE_START(EXYNOS5440_DT, "SAMSUNG EXYNOS5440 (Flattened Device
Tree)") +	/* Maintainer: Kukjin Kim [off-list ref] */
+	.init_irq	= exynos5_init_irq,
+	.smp		= smp_ops(exynos_smp_ops),
+	.map_io		= exynos5_dt_map_io,
+	.handle_irq	= gic_handle_irq,
+	.init_machine	= exynos5_dt_machine_init,
+	.timer		= &exynos5_timer,
+	.dt_compat	= exynos5_dt_compat,
 	.restart        = exynos5_restart,

Since restarts for both differ, why not to add separate exynos5440 restart
and use it here?

As I said above, I don't think so.

[...]

quoted

@@ -487,6 +489,9 @@ static void __init exynos4_timer_init(void)
 		exynos4x12_clk_init();
 #endif

+	if (of_machine_is_compatible("samsung,exynos5440"))
+		arch_timer_of_register();
+

Why exynos4_timer_init is being touched here, if exynos5_timer_init is
being added?

I would rather keep exynos4_timer (which is used for all Exynos4 SoCs
and for Exynos5250) as is and define new exynos5250_timer if it needs
completely different initialization...

See updated patch.

[...]

Thanks.

Best regards,
Kgene.
--
Kukjin Kim [off-list ref], Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.