Tomasz Figa wrote:
>
> Hi Kgene,
>
Hi,
[...]
> > 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.
[...]
> > -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.
> > @@ -96,11 +100,23 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5
> > (Flattened Device Tree)") /* Maintainer: Kukjin Kim
> > <kgene.kim@xxxxxxxxxxx> */
> > .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 <kgene.kim@xxxxxxxxxxx> */
> > + .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.
[...]
> > @@ -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 <kgene.kim@xxxxxxxxxxx>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
