On Sat, Dec 10, 2016 at 04:57:55PM +0530, pankaj.dubey wrote:
> Hi Krzysztof,
>
> On Saturday 10 December 2016 04:25 PM, Krzysztof Kozlowski wrote:
> > On Fri, Dec 09, 2016 at 04:01:17PM +0530, Pankaj Dubey wrote:
> >> We can safely move exynos_pm_init into pm.c as late_initcall and remove
> >> init_late hook from exynos.c. This will remove extern declarations from
> >> common.h and move PM specific operations in pm.c rather being scattered
> >> across many files.
> >>
> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> >> ---
> >> arch/arm/mach-exynos/common.h | 6 ------
> >> arch/arm/mach-exynos/exynos.c | 10 ----------
> >> arch/arm/mach-exynos/suspend.c | 13 ++++++++++---
> >> 3 files changed, 10 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> >> index fb12d11..cfd55ba 100644
> >> --- a/arch/arm/mach-exynos/common.h
> >> +++ b/arch/arm/mach-exynos/common.h
> >> @@ -134,12 +134,6 @@ void exynos_clear_boot_flag(unsigned int cpu, unsigned int mode);
> >>
> >> extern u32 exynos_get_eint_wake_mask(void);
> >>
> >> -#ifdef CONFIG_PM_SLEEP
> >> -extern void __init exynos_pm_init(void);
> >> -#else
> >> -static inline void exynos_pm_init(void) {}
> >> -#endif
> >> -
> >> extern void exynos_cpu_resume(void);
> >> extern void exynos_cpu_resume_ns(void);
> >>
> >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> >> index fa08ef9..040ea66 100644
> >> --- a/arch/arm/mach-exynos/exynos.c
> >> +++ b/arch/arm/mach-exynos/exynos.c
> >> @@ -58,15 +58,6 @@ void __init exynos_sysram_init(void)
> >> }
> >> }
> >>
> >> -static void __init exynos_init_late(void)
> >> -{
> >> - if (of_machine_is_compatible("samsung,exynos5440"))
> >> - /* to be supported later */
> >> - return;
> >> -
> >> - exynos_pm_init();
> >> -}
> >> -
> >> static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname,
> >> int depth, void *data)
> >> {
> >> @@ -216,7 +207,6 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
> >> .init_early = exynos_firmware_init,
> >> .init_irq = exynos_init_irq,
> >> .init_machine = exynos_dt_machine_init,
> >> - .init_late = exynos_init_late,
> >> .dt_compat = exynos_dt_compat,
> >> .dt_fixup = exynos_dt_fixup,
> >> MACHINE_END
> >> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> >> index 73df9f3..f318b08 100644
> >> --- a/arch/arm/mach-exynos/suspend.c
> >> +++ b/arch/arm/mach-exynos/suspend.c
> >> @@ -698,21 +698,25 @@ static const struct of_device_id exynos_pmu_of_device_ids[] __initconst = {
> >>
> >> static struct syscore_ops exynos_pm_syscore_ops;
> >>
> >> -void __init exynos_pm_init(void)
> >> +static int __init exynos_pm_init(void)
> >> {
> >> const struct of_device_id *match;
> >> struct device_node *np;
> >> u32 tmp;
> >>
> >> + if (of_machine_is_compatible("samsung,exynos5440"))
> >> + /* to be supported later */
> >> + return 0;
> >> +
> >> np = of_find_matching_node_and_match(NULL, exynos_pmu_of_device_ids, &match);
> >> if (!np) {
> >> pr_err("Failed to find PMU node\n");
> >> - return;
> >> + return -ENODEV;
> >> }
> >>
> >> if (WARN_ON(!of_find_property(np, "interrupt-controller", NULL))) {
> >> pr_warn("Outdated DT detected, suspend/resume will NOT work\n");
> >> - return;
> >> + return -ENODEV;
> >> }
> >>
> >> pm_data = (const struct exynos_pm_data *) match->data;
> >> @@ -727,4 +731,7 @@ void __init exynos_pm_init(void)
> >>
> >> register_syscore_ops(&exynos_pm_syscore_ops);
> >> suspend_set_ops(&exynos_suspend_ops);
> >> +
> >> + return 0;
> >> }
> >> +late_initcall(exynos_pm_init);
> >
> > No. This does not look like multiplatform friendly. Also, basically you are
> > reverting 559ba237999d7 without clear explanation of revert itself.
> >
>
> Thanks for review.
>
> I could not understand why this change is not multi-platform friendly,
> would you please elaborate more on this.
This late_initcall will be executed on every platform, including
non-exynos one. There is a of_find_matching_node_and_match() check so it
should be safe but all other platforms will see this ugly error "Failed
to find PMU node".
> Well I missed to check history of this file before sending this patch,
> as I could not sense any issue as such, we are calling exynos_pm_init
> from exynos_init_late which is infact gets called as part of
> late_initcall itself. I have tested this with multi_v7_defconfig.
>
> When Thomasz submitted this patch 559ba237999d7 basically there were two
> arch_initcalls as "exynos_pm_drvinit" and "exynos_pm_drvinit", the
> second one he renamed to exynos_pm_init. At the same time he removed
> arch_initcall and made provision so that it can be called from
> exynos_init_late. Probably he did because there were two arch_initcalls.
> Still I am not sure why he did not opt to convert one of them from
> arch_initcall to late_initcall.. how this change affects multiplatform?
>
> As far as intention of this patch, slowly I wanted to reduce dependency
> of common.h from pm.c and suspend.c so that one day all these
> functionalities which are tightly coupled with machine files can be
> loosen and these files can reside along with pmu driver in
> "drivers/soc/samsung/".
I fully support such goal.
Best regards,
Krzysztof
--
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
