Messages in this thread | | | Date | Sun, 27 Apr 2014 16:29:42 +0900 | From | Pankaj Dubey <> | Subject | Re: [PATCH v2 06/10] ARM: EXYNOS: Add support for mapping PMU base address via DT |
| |
Hi Tomasz,
Thanks for review.
On 04/26/2014 06:43 AM, Tomasz Figa wrote: > Hi, > > On 25.04.2014 14:32, Pankaj Dubey wrote: >> From: Young-Gun Jang <yg1004.jang@samsung.com> >> >> Add support for mapping Samsung Power Management Unit (PMU) base address >> from device tree. Code will use existing samsung pmu binding information. >> This patch also adds two helper functions as "get_exynos_pmuregmap" and >> "get_exynos_pmuaddr". >> "get_exynos_pmuregmap" returns a regmap based PMU register handle where as >> "get_exynos_pmuaddr" returns ioremap virtual address. >> >> Signed-off-by: Young-Gun Jang <yg1004.jang@samsung.com> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> arch/arm/mach-exynos/Kconfig | 2 ++ >> arch/arm/mach-exynos/common.h | 3 ++ >> arch/arm/mach-exynos/exynos.c | 78 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 83 insertions(+) >> >> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig >> index fc8bf18..2f60c90 100644 >> --- a/arch/arm/mach-exynos/Kconfig >> +++ b/arch/arm/mach-exynos/Kconfig >> @@ -26,6 +26,7 @@ config ARCH_EXYNOS4 >> select PINCTRL >> select PM_GENERIC_DOMAINS if PM_RUNTIME >> select S5P_DEV_MFC >> + select MFD_SYSCON >> help >> Samsung EXYNOS4 SoCs based systems >> >> @@ -36,6 +37,7 @@ config ARCH_EXYNOS5 >> select HAVE_ARM_SCU if SMP >> select HAVE_SMP >> select PINCTRL >> + select MFD_SYSCON >> help >> Samsung EXYNOS5 (Cortex-A15) SoC based systems >> >> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h >> index 31c5964..ecfd0fc 100644 >> --- a/arch/arm/mach-exynos/common.h >> +++ b/arch/arm/mach-exynos/common.h >> @@ -57,4 +57,7 @@ struct exynos_pmu_conf { >> >> extern void exynos_sys_powerdown_conf(enum sys_powerdown mode); >> >> +extern struct regmap *get_exynos_pmuregmap(void); >> +extern void __iomem *get_exynos_pmuaddr(void); > > Do you really need these functions? Couldn't all the drivers using PMU simply > call syscon_regmap_lookup_by_phandle() or of_iomap() directly?
Well, currently "get_exynos_pmuregmap" is used in three location under mach-exynos as "exynos.c", "pm.c" and "hotplug.c" so just to avoid duplicate code in all these files I added "get_exynos_pmuregmap" as helper function in "exynos.c". So in my opinion it should be fine. What do you say?
Regarding "get_exynos_pmuaddr", only user of this function is "platsmp.c" so I can move this function in platsmp.c and remove this helper. This will also solve the other issue of early mapping as you raised below.
> >> + >> #endif /* __ARCH_ARM_MACH_EXYNOS_COMMON_H */ >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >> index d6f405f..68f60e1 100644 >> --- a/arch/arm/mach-exynos/exynos.c >> +++ b/arch/arm/mach-exynos/exynos.c >> @@ -19,6 +19,7 @@ >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> #include <linux/pm_domain.h> >> +#include <linux/mfd/syscon.h> >> >> #include <asm/cacheflush.h> >> #include <asm/hardware/cache-l2x0.h> >> @@ -36,6 +37,9 @@ >> #define L2_AUX_VAL 0x7C470001 >> #define L2_AUX_MASK 0xC200ffff >> >> +static struct regmap *exynos_pmu_regmap; >> +static void __iomem *exynos_pmu_base; >> + >> static struct map_desc exynos4_iodesc[] __initdata = { >> { >> .virtual = (unsigned long)S3C_VA_SYS, >> @@ -269,6 +273,46 @@ static int __init exynos_fdt_map_chipid(unsigned long >> node, const char *uname, >> return 1; >> } >> >> +static const struct of_device_id exynos_dt_pmu_match[] = { >> + { .compatible = "samsung,exynos4210-pmu" }, >> + { .compatible = "samsung,exynos4212-pmu" }, >> + { .compatible = "samsung,exynos4412-pmu" }, >> + { .compatible = "samsung,exynos5250-pmu" }, >> + {}, >> +}; >> + >> +static int __init exynos_fdt_map_pmu(unsigned long node, >> + const char *uname, int depth, void *data) >> +{ >> + struct map_desc iodesc; >> + __be32 *reg; >> + unsigned long len; >> + phys_addr_t phys_addr; >> + const struct of_device_id *matches = exynos_dt_pmu_match; >> + >> + for (; matches->compatible[0]; matches++) { >> + if (!of_flat_dt_is_compatible(node, matches->compatible)) >> + continue; >> + reg = of_get_flat_dt_prop(node, "reg", &len); >> + if (reg == NULL || len != (sizeof(unsigned long) * 2)) >> + return 0; >> + >> + phys_addr = be32_to_cpu(reg[0]); >> + iodesc.pfn = __phys_to_pfn(phys_addr); >> + iodesc.length = be32_to_cpu(reg[1]) - 1; >> + iodesc.virtual = (unsigned long)S5P_VA_PMU; >> + iodesc.type = MT_DEVICE; >> + iotable_init(&iodesc, 1); >> + >> + exynos_pmu_base = ioremap(phys_addr, be32_to_cpu(reg[1])); >> + if (WARN_ON(!exynos_pmu_base)) >> + return -EFAULT; >> + return 1; >> + } >> + >> + return 0; >> +} > > Is such early mapping really needed? Couldn't the code using PMU be deferred > to the stage that memory management is available and then of_iomap() used > directly?
OK, not really required, If I move mapping of PMU base address to platsmp.c. As "exynos_pmu_base" is only required in platsmp.c during "exynos_smp_prepare_cpus" function call, I can move mapping of PMU address in platsmp.c itself and there I can use of_iomap.
> >> + >> /* >> * exynos_map_io >> * >> @@ -302,6 +346,7 @@ static void __init exynos_init_io(void) >> debug_ll_io_init(); >> >> of_scan_flat_dt(exynos_fdt_map_chipid, NULL); >> + of_scan_flat_dt(exynos_fdt_map_pmu, NULL); >> >> /* detect cpu id and rev. */ >> s5p_init_cpu(S5P_VA_CHIPID); >> @@ -336,6 +381,38 @@ static int __init exynos4_l2x0_cache_init(void) >> } >> early_initcall(exynos4_l2x0_cache_init); >> >> + >> +inline struct regmap *get_exynos_pmuregmap() >> +{ >> + return exynos_pmu_regmap; >> +} >> + >> +inline void __iomem *get_exynos_pmuaddr() >> +{ >> + return exynos_pmu_base; >> +} > > Hmm, non-static inline inside a C file? Probably should be either static > inline or non-static non-inline. (Assuming that both are really needed, as I > pointed in the comments above.)
OK, will remove "get_exynos_pmuaddr" and make "get_exynos_pmuregmap" as non-static non-inline.
> >> + >> + >> +void __init exynos_map_pmu(void) >> +{ >> + struct device_node *np = NULL; >> + >> + early_syscon_init(); >> + >> + np = of_find_matching_node(NULL, exynos_dt_pmu_match); >> + >> + if (!np) { >> + pr_err("Failed to find PMU node\n"); >> + return; >> + } else { >> + exynos_pmu_regmap = syscon_early_regmap_lookup_by_phandle(np, >> + "samsung,syscon-phandle"); > > Do you need this regmap really here? I believe it should be the code using PMU > registers which calls this function directly to retrieve a handle to the syscon. >
Yes, as "exynos_restart" is accessing one PMU register. So even though all callers (pm.c and hotplug.c) directly retrieve a regmap handle to PMU, we need regmap handle in this file also. So as I mentioned above just to avoid duplicate code in all files we can retrieve regmap handle in "exynos.c" and using helper function other files can use it.
> Best regards, > Tomasz >
-- Best Regards, Pankaj Dubey
| |