| Message ID | 1490879826-16754-10-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pankaj, On 2017-03-30 15:17, Pankaj Dubey wrote: > Various Exynos SoC has different CPU related information, such as CPU > boot register, programming sequence making CPU up/down. Currently this > is handled by adding lots of soc_is_exynosMMM checks in the code, in > an attempt to remove the dependency of such helper functions specific to > each SoC, let's separate this information pertaining to CPU by introducing > a new "struct exynos_cpu_info". This struct will contain differences > associated with CPU on various Exynos SoC. This can be matched by using > generic API "soc_device_match". > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 135 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c > index cb6d199..ff369b9 100644 > --- a/arch/arm/mach-exynos/platsmp.c > +++ b/arch/arm/mach-exynos/platsmp.c > @@ -19,6 +19,7 @@ > #include <linux/smp.h> > #include <linux/io.h> > #include <linux/of_address.h> > +#include <linux/sys_soc.h> > #include <linux/soc/samsung/exynos-regs-pmu.h> > > #include <asm/cacheflush.h> > @@ -33,6 +34,16 @@ > > extern void exynos4_secondary_startup(void); > > +/* > + * struct exynos_cpu_info - Exynos CPU related info/operations > + * @cpu_boot_reg: computes cpu boot address for requested cpu > + */ > +struct exynos_cpu_info { > + void __iomem* (*cpu_boot_reg)(u32 cpu); > +}; > + > +static const struct exynos_cpu_info *cpu_info; > + > #ifdef CONFIG_HOTPLUG_CPU > static inline void cpu_leave_lowpower(u32 core_id) > { > @@ -168,27 +179,57 @@ int exynos_cluster_power_state(int cluster) > S5P_CORE_LOCAL_PWR_EN); > } > > -static void __iomem *cpu_boot_reg_base(void) > +static void __iomem *exynos4210_rev11_cpu_boot_reg(u32 cpu) > { > - if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1) > - return pmu_base_addr + S5P_INFORM5; > - return sysram_base_addr; > + void __iomem *boot_reg = pmu_base_addr; > + > + if (!boot_reg) > + return IOMEM_ERR_PTR(-ENODEV); > + > + boot_reg += S5P_INFORM5; > + > + return boot_reg; > } > > -static inline void __iomem *cpu_boot_reg(int cpu) > +static void __iomem *exynos4412_cpu_boot_reg(u32 cpu) > { > - void __iomem *boot_reg; > + void __iomem *boot_reg = sysram_base_addr; > > - boot_reg = cpu_boot_reg_base(); > if (!boot_reg) > return IOMEM_ERR_PTR(-ENODEV); > - if (soc_is_exynos4412()) > - boot_reg += 4*cpu; > - else if (soc_is_exynos5420() || soc_is_exynos5800()) > - boot_reg += 4; > + > + boot_reg += 4*cpu; > + > return boot_reg; > } > > +static void __iomem *exynos5420_cpu_boot_reg(u32 cpu) > +{ > + void __iomem *boot_reg = sysram_base_addr; > + > + if (!sysram_base_addr) > + return IOMEM_ERR_PTR(-ENODEV); > + > + boot_reg += 4; > + > + return boot_reg; > +} > + > +static void __iomem *exynos_common_cpu_boot_reg(u32 cpu) > +{ > + if (!sysram_base_addr) > + return IOMEM_ERR_PTR(-ENODEV); > + > + return sysram_base_addr; > +} > + > +static inline void __iomem *cpu_boot_reg(int cpu) > +{ > + if (cpu_info && cpu_info->cpu_boot_reg) > + return cpu_info->cpu_boot_reg(cpu); > + return NULL; > +} > + > /* > * Set wake up by local power mode and execute software reset for given core. > * > @@ -296,13 +337,84 @@ int exynos_get_boot_addr(u32 core_id, unsigned long *boot_addr) > return ret; > } > > +static const struct exynos_cpu_info exynos3250_cpu_info = { > + .cpu_boot_reg = exynos_common_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos5420_cpu_info = { > + .cpu_boot_reg = exynos5420_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos4210_rev11_cpu_info = { > + .cpu_boot_reg = exynos4210_rev11_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos4412_cpu_info = { > + .cpu_boot_reg = exynos4412_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos_common_cpu_info = { > + .cpu_boot_reg = exynos_common_cpu_boot_reg, > +}; > + > +static const struct soc_device_attribute exynos_soc_revision[] = { > + { > + .soc_id = "EXYNOS4210", > + .revision = "11", > + .data = &exynos4210_rev11_cpu_info > + }, { > + .soc_id = "EXYNOS4210", > + .revision = "10", > + .data = &exynos_common_cpu_info > + } > +}; > + > +static const struct of_device_id exynos_pmu_of_device_ids[] __initconst = { exynos_pmuc_of_device_ids is a bit too easy to confuse with drivers/soc/samsung/exynos_*pmu.c drivers and its device tree nodes. IMHO exynos_soc_of_device_ids is a bit more appropriate here. > + { > + .compatible = "samsung,exynos3250", > + .data = &exynos3250_cpu_info > + }, { > + .compatible = "samsung,exynos4212", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos4412", > + .data = &exynos4412_cpu_info > + }, { > + .compatible = "samsung,exynos5250", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5260", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5410", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5420", > + .data = &exynos5420_cpu_info > + }, { > + .compatible = "samsung,exynos5440", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5800", > + .data = &exynos5420_cpu_info > + }, > + { /*sentinel*/ }, > +}; > + > static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > { > unsigned long timeout; > + const struct soc_device_attribute *match; > u32 mpidr = cpu_logical_map(cpu); > u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > int ret = -ENOSYS; > > + if (of_machine_is_compatible("samsung,exynos4210")) { > + match = soc_device_match(exynos_soc_revision); > + if (match) > + cpu_info = (const struct exynos_cpu_info *) match->data; > + } > + > /* > * Set synchronisation state between this boot processor > * and the secondary one > @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > > static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) > { > + const struct of_device_id *match; > + struct device_node *np; > + > + if (!of_machine_is_compatible("samsung,exynos4210")) { > + np = of_find_matching_node_and_match(NULL, > + exynos_pmu_of_device_ids, &match); > + if (!np) > + pr_err("failed to find supported CPU\n"); > + else > + cpu_info = (const struct exynos_cpu_info *) match->data; > + } > + This approach nukes on Exynos4210 booted with maxcpus=1, because exynos_boot_secondary() is not called in such case: Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 805 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc5-00013-g7aa5680b6c53-dirty #1185 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: c0b08640 task.stack: c0b00000 PC is at exynos_set_boot_addr+0x64/0x84 LR is at exynos_enter_coupled_lowpower+0x1c/0x74 pc : [<c0117890>] lr : [<c052f654>] psr: 60000093 sp : c0b01f00 ip : 02800000 fp : c0b2f7e4 r10: c0b86e44 r9 : 00000001 r8 : dbb97e10 r7 : 00000001 r6 : dbb97e10 r5 : 00000001 r4 : 40116c50 r3 : 00000000 r2 : 00000001 r1 : 40116c50 r0 : 00000000 Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 4000404a DAC: 00000051 Process swapper/0 (pid: 0, stack limit = 0xc0b00210) Stack: (0xc0b01f00 to 0xc0b02000) 1f00: c01165f8 c0b86e4c c0b05508 c052f654 c052f638 c0b2f844 c0b05508 90442e5c 1f20: 00000000 c052d528 00041fc9 00000000 00000000 00000000 00000001 00000001 1f40: c0b05508 dbb97e10 c0b2f7e4 dbb97e10 dae522c0 c0b86e44 c0b86e44 c052f3b0 1f60: 00000000 c0b05424 00000001 dae522e8 c0b05424 c0b05480 c0b05424 c0a66e08 1f80: c0b0548c c0b10b0e c0b2f7e4 dbb97e10 00000000 c0152eb0 000000bb ffffffff 1fa0: c0b47000 c0a34a38 dbfffb40 412fc091 00000001 c01531cc c0b08640 c0a00c00 1fc0: ffffffff ffffffff 00000000 c0a00680 00000000 c0a34a38 00000000 c0b473d4 1fe0: c0b05418 c0a34a34 c0b09870 4000406a 00000000 4000807c 00000000 00000000 [<c0117890>] (exynos_set_boot_addr) from [<c052f654>] (exynos_enter_coupled_lowpower+0x1c/0x74) [<c052f654>] (exynos_enter_coupled_lowpower) from [<c052d528>] (cpuidle_enter_state+0x64/0x264) [<c052d528>] (cpuidle_enter_state) from [<c052f3b0>] (cpuidle_enter_state_coupled+0x394/0x3e4) [<c052f3b0>] (cpuidle_enter_state_coupled) from [<c0152eb0>] (do_idle+0x178/0x200) [<c0152eb0>] (do_idle) from [<c01531cc>] (cpu_startup_entry+0x18/0x1c) [<c01531cc>] (cpu_startup_entry) from [<c0a00c00>] (start_kernel+0x330/0x398) [<c0a00c00>] (start_kernel) from [<4000807c>] (0x4000807c) Code: e1a00005 e12fff33 e3700a01 8a000004 (e5804000) ---[ end trace 46ced8ec429feb82 ]--- Kernel panic - not syncing: Attempted to kill the idle task! > exynos_sysram_init(); > > exynos_set_delayed_reset_assertion(true); Best regards
Hi Marek, On Monday 03 April 2017 01:27 PM, Marek Szyprowski wrote: > Hi Pankaj, > > On 2017-03-30 15:17, Pankaj Dubey wrote: >> Various Exynos SoC has different CPU related information, such as CPU >> boot register, programming sequence making CPU up/down. Currently this >> is handled by adding lots of soc_is_exynosMMM checks in the code, in >> an attempt to remove the dependency of such helper functions specific to >> each SoC, let's separate this information pertaining to CPU by >> introducing >> a new "struct exynos_cpu_info". This struct will contain differences >> associated with CPU on various Exynos SoC. This can be matched by using >> generic API "soc_device_match". >> >> + >> +static const struct of_device_id exynos_pmu_of_device_ids[] >> __initconst = { > > exynos_pmuc_of_device_ids is a bit too easy to confuse with > drivers/soc/samsung/exynos_*pmu.c drivers and its device tree nodes. > IMHO exynos_soc_of_device_ids is a bit more appropriate here. > Yes you are right, I will update this name. >> + { >> + .compatible = "samsung,exynos3250", >> + .data = &exynos3250_cpu_info >> + }, { >> + .compatible = "samsung,exynos4212", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos4412", >> + .data = &exynos4412_cpu_info >> + }, { >> + .compatible = "samsung,exynos5250", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5260", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5410", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5420", >> + .data = &exynos5420_cpu_info >> + }, { >> + .compatible = "samsung,exynos5440", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5800", >> + .data = &exynos5420_cpu_info >> + }, >> + { /*sentinel*/ }, >> +}; >> + >> static int exynos_boot_secondary(unsigned int cpu, struct >> task_struct *idle) >> { >> unsigned long timeout; >> + const struct soc_device_attribute *match; >> u32 mpidr = cpu_logical_map(cpu); >> u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); >> int ret = -ENOSYS; >> + if (of_machine_is_compatible("samsung,exynos4210")) { >> + match = soc_device_match(exynos_soc_revision); >> + if (match) >> + cpu_info = (const struct exynos_cpu_info *) match->data; >> + } >> + >> /* >> * Set synchronisation state between this boot processor >> * and the secondary one >> @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int >> cpu, struct task_struct *idle) >> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >> { >> + const struct of_device_id *match; >> + struct device_node *np; >> + >> + if (!of_machine_is_compatible("samsung,exynos4210")) { >> + np = of_find_matching_node_and_match(NULL, >> + exynos_pmu_of_device_ids, &match); >> + if (!np) >> + pr_err("failed to find supported CPU\n"); >> + else >> + cpu_info = (const struct exynos_cpu_info *) match->data; >> + } >> + > > This approach nukes on Exynos4210 booted with maxcpus=1, because > exynos_boot_secondary() > is not called in such case: argh :( If you see I have handled Exynos4210 case in a different manner, because as per code-flow I can see following order of sequence: 1: exynos_smp_prepare_cpus 2: exynos_chipid_early_init 3: exynos_boot_secondary We can't handle differentiation of Exynos4210 case in smp_prepare_cpus only based on compatible property of root node, as it has different revision boards and chipid is not getting initialized before smp_prepare_cpus, even though I have marked it as early_initcall, I am not sure how I can make chipid initialization much early than this? Any idea? So I decided to handle it in exynos_boot_secondary and populate its cpu_info based on match found via "soc_device_match" API. But as you tested and mentioned exynos_boot_secondary will not be called if nr_cpus is capped to 1 and this will lead to crash, I missed this point. I feel I am forced to add such work-around as there is need of some I/Ps such as chipid to be probed or initialized at very early stage, but currently I am not seeing any such initcall or feature (early probe or something similar) exist to make this work Let me think again how we can make it work.. any suggestion will be helpful for me :) Thanks, Pankaj Dubey > > Unable to handle kernel NULL pointer dereference at virtual address > 00000000 > pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 805 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 0 PID: 0 Comm: swapper/0 Not tainted > 4.11.0-rc5-00013-g7aa5680b6c53-dirty #1185 > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > task: c0b08640 task.stack: c0b00000 > PC is at exynos_set_boot_addr+0x64/0x84 > LR is at exynos_enter_coupled_lowpower+0x1c/0x74 > pc : [<c0117890>] lr : [<c052f654>] psr: 60000093 > sp : c0b01f00 ip : 02800000 fp : c0b2f7e4 > r10: c0b86e44 r9 : 00000001 r8 : dbb97e10 > r7 : 00000001 r6 : dbb97e10 r5 : 00000001 r4 : 40116c50 > r3 : 00000000 r2 : 00000001 r1 : 40116c50 r0 : 00000000 > Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 4000404a DAC: 00000051 > Process swapper/0 (pid: 0, stack limit = 0xc0b00210) > Stack: (0xc0b01f00 to 0xc0b02000) > 1f00: c01165f8 c0b86e4c c0b05508 c052f654 c052f638 c0b2f844 c0b05508 > 90442e5c > 1f20: 00000000 c052d528 00041fc9 00000000 00000000 00000000 00000001 > 00000001 > 1f40: c0b05508 dbb97e10 c0b2f7e4 dbb97e10 dae522c0 c0b86e44 c0b86e44 > c052f3b0 > 1f60: 00000000 c0b05424 00000001 dae522e8 c0b05424 c0b05480 c0b05424 > c0a66e08 > 1f80: c0b0548c c0b10b0e c0b2f7e4 dbb97e10 00000000 c0152eb0 000000bb > ffffffff > 1fa0: c0b47000 c0a34a38 dbfffb40 412fc091 00000001 c01531cc c0b08640 > c0a00c00 > 1fc0: ffffffff ffffffff 00000000 c0a00680 00000000 c0a34a38 00000000 > c0b473d4 > 1fe0: c0b05418 c0a34a34 c0b09870 4000406a 00000000 4000807c 00000000 > 00000000 > [<c0117890>] (exynos_set_boot_addr) from [<c052f654>] > (exynos_enter_coupled_lowpower+0x1c/0x74) > [<c052f654>] (exynos_enter_coupled_lowpower) from [<c052d528>] > (cpuidle_enter_state+0x64/0x264) > [<c052d528>] (cpuidle_enter_state) from [<c052f3b0>] > (cpuidle_enter_state_coupled+0x394/0x3e4) > [<c052f3b0>] (cpuidle_enter_state_coupled) from [<c0152eb0>] > (do_idle+0x178/0x200) > [<c0152eb0>] (do_idle) from [<c01531cc>] (cpu_startup_entry+0x18/0x1c) > [<c01531cc>] (cpu_startup_entry) from [<c0a00c00>] > (start_kernel+0x330/0x398) > [<c0a00c00>] (start_kernel) from [<4000807c>] (0x4000807c) > Code: e1a00005 e12fff33 e3700a01 8a000004 (e5804000) > ---[ end trace 46ced8ec429feb82 ]--- > Kernel panic - not syncing: Attempted to kill the idle task! > >> exynos_sysram_init(); >> exynos_set_delayed_reset_assertion(true); > > Best regards
On Thu, Mar 30, 2017 at 3:17 PM, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Various Exynos SoC has different CPU related information, such as CPU > boot register, programming sequence making CPU up/down. Currently this > is handled by adding lots of soc_is_exynosMMM checks in the code, in > an attempt to remove the dependency of such helper functions specific to > each SoC, let's separate this information pertaining to CPU by introducing > a new "struct exynos_cpu_info". This struct will contain differences > associated with CPU on various Exynos SoC. This can be matched by using > generic API "soc_device_match". > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 135 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c > index cb6d199..ff369b9 100644 > --- a/arch/arm/mach-exynos/platsmp.c > +++ b/arch/arm/mach-exynos/platsmp.c > @@ -19,6 +19,7 @@ > #include <linux/smp.h> > #include <linux/io.h> > #include <linux/of_address.h> > +#include <linux/sys_soc.h> > #include <linux/soc/samsung/exynos-regs-pmu.h> > > #include <asm/cacheflush.h> > @@ -33,6 +34,16 @@ > > extern void exynos4_secondary_startup(void); > > +/* > + * struct exynos_cpu_info - Exynos CPU related info/operations > + * @cpu_boot_reg: computes cpu boot address for requested cpu > + */ > +struct exynos_cpu_info { > + void __iomem* (*cpu_boot_reg)(u32 cpu); > +}; Beside Marek comments, I think we are getting too many structures for differentiating Exynos. Actually all of them describe the same - difference between Exynos revisions. Having many separate structures means that you have to initialize all of them in different places in different (probably) order. The good benefit is however making them local (static) so the scope is limited... but anyway I dislike the duplication. How about combining all of them into one (except the firmware which has its own register function): struct exynos_revision_data { void __iomem* (*boot_vector_addr)(void); void __iomem* (*boot_vector_flag)(void); void (*enter_aftr)(void); void __iomem* (*cpu_boot_reg)(u32 cpu); void (*cpu_power_down)(u32 cpu); void (*cpu_power_up)(u32 cpu); }; Best regards, Krzysztof > + > +static const struct exynos_cpu_info *cpu_info; > + > #ifdef CONFIG_HOTPLUG_CPU > static inline void cpu_leave_lowpower(u32 core_id) > { > @@ -168,27 +179,57 @@ int exynos_cluster_power_state(int cluster) > S5P_CORE_LOCAL_PWR_EN); > } > > -static void __iomem *cpu_boot_reg_base(void) > +static void __iomem *exynos4210_rev11_cpu_boot_reg(u32 cpu) > { > - if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1) > - return pmu_base_addr + S5P_INFORM5; > - return sysram_base_addr; > + void __iomem *boot_reg = pmu_base_addr; > + > + if (!boot_reg) > + return IOMEM_ERR_PTR(-ENODEV); > + > + boot_reg += S5P_INFORM5; > + > + return boot_reg; > } > > -static inline void __iomem *cpu_boot_reg(int cpu) > +static void __iomem *exynos4412_cpu_boot_reg(u32 cpu) > { > - void __iomem *boot_reg; > + void __iomem *boot_reg = sysram_base_addr; > > - boot_reg = cpu_boot_reg_base(); > if (!boot_reg) > return IOMEM_ERR_PTR(-ENODEV); > - if (soc_is_exynos4412()) > - boot_reg += 4*cpu; > - else if (soc_is_exynos5420() || soc_is_exynos5800()) > - boot_reg += 4; > + > + boot_reg += 4*cpu; > + > return boot_reg; > } > > +static void __iomem *exynos5420_cpu_boot_reg(u32 cpu) > +{ > + void __iomem *boot_reg = sysram_base_addr; > + > + if (!sysram_base_addr) > + return IOMEM_ERR_PTR(-ENODEV); > + > + boot_reg += 4; > + > + return boot_reg; > +} > + > +static void __iomem *exynos_common_cpu_boot_reg(u32 cpu) > +{ > + if (!sysram_base_addr) > + return IOMEM_ERR_PTR(-ENODEV); > + > + return sysram_base_addr; > +} > + > +static inline void __iomem *cpu_boot_reg(int cpu) > +{ > + if (cpu_info && cpu_info->cpu_boot_reg) > + return cpu_info->cpu_boot_reg(cpu); > + return NULL; > +} > + > /* > * Set wake up by local power mode and execute software reset for given core. > * > @@ -296,13 +337,84 @@ int exynos_get_boot_addr(u32 core_id, unsigned long *boot_addr) > return ret; > } > > +static const struct exynos_cpu_info exynos3250_cpu_info = { > + .cpu_boot_reg = exynos_common_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos5420_cpu_info = { > + .cpu_boot_reg = exynos5420_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos4210_rev11_cpu_info = { > + .cpu_boot_reg = exynos4210_rev11_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos4412_cpu_info = { > + .cpu_boot_reg = exynos4412_cpu_boot_reg, > +}; > + > +static const struct exynos_cpu_info exynos_common_cpu_info = { > + .cpu_boot_reg = exynos_common_cpu_boot_reg, > +}; > + > +static const struct soc_device_attribute exynos_soc_revision[] = { > + { > + .soc_id = "EXYNOS4210", > + .revision = "11", > + .data = &exynos4210_rev11_cpu_info > + }, { > + .soc_id = "EXYNOS4210", > + .revision = "10", > + .data = &exynos_common_cpu_info > + } > +}; > + > +static const struct of_device_id exynos_pmu_of_device_ids[] __initconst = { > + { > + .compatible = "samsung,exynos3250", > + .data = &exynos3250_cpu_info > + }, { > + .compatible = "samsung,exynos4212", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos4412", > + .data = &exynos4412_cpu_info > + }, { > + .compatible = "samsung,exynos5250", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5260", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5410", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5420", > + .data = &exynos5420_cpu_info > + }, { > + .compatible = "samsung,exynos5440", > + .data = &exynos_common_cpu_info > + }, { > + .compatible = "samsung,exynos5800", > + .data = &exynos5420_cpu_info > + }, > + { /*sentinel*/ }, > +}; > + > static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > { > unsigned long timeout; > + const struct soc_device_attribute *match; > u32 mpidr = cpu_logical_map(cpu); > u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > int ret = -ENOSYS; > > + if (of_machine_is_compatible("samsung,exynos4210")) { > + match = soc_device_match(exynos_soc_revision); > + if (match) > + cpu_info = (const struct exynos_cpu_info *) match->data; > + } > + > /* > * Set synchronisation state between this boot processor > * and the secondary one > @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > > static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) > { > + const struct of_device_id *match; > + struct device_node *np; > + > + if (!of_machine_is_compatible("samsung,exynos4210")) { > + np = of_find_matching_node_and_match(NULL, > + exynos_pmu_of_device_ids, &match); > + if (!np) > + pr_err("failed to find supported CPU\n"); > + else > + cpu_info = (const struct exynos_cpu_info *) match->data; > + } > + > exynos_sysram_init(); > > exynos_set_delayed_reset_assertion(true); > -- > 2.7.4 >
On Thu, Mar 30, 2017 at 3:17 PM, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Various Exynos SoC has different CPU related information, such as CPU > boot register, programming sequence making CPU up/down. Currently this > is handled by adding lots of soc_is_exynosMMM checks in the code, in > an attempt to remove the dependency of such helper functions specific to > each SoC, let's separate this information pertaining to CPU by introducing > a new "struct exynos_cpu_info". This struct will contain differences > associated with CPU on various Exynos SoC. This can be matched by using > generic API "soc_device_match". > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 135 insertions(+), 11 deletions(-) (...) > static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > { > unsigned long timeout; > + const struct soc_device_attribute *match; > u32 mpidr = cpu_logical_map(cpu); > u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > int ret = -ENOSYS; > > + if (of_machine_is_compatible("samsung,exynos4210")) { > + match = soc_device_match(exynos_soc_revision); > + if (match) > + cpu_info = (const struct exynos_cpu_info *) match->data; > + } > + > /* > * Set synchronisation state between this boot processor > * and the secondary one > @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) > > static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) > { > + const struct of_device_id *match; > + struct device_node *np; > + > + if (!of_machine_is_compatible("samsung,exynos4210")) { This differentiation is unnatural. It looks like the chipid soc_device_match driver is not simplifying things but complicating. Maybe the solution I mentioned in previous mails (initialize these in proper order from some of machine init calls) would simplify it... Best regards, Krzysztof
On Friday 07 April 2017 04:11 PM, Krzysztof Kozlowski wrote: > On Thu, Mar 30, 2017 at 3:17 PM, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: >> Various Exynos SoC has different CPU related information, such as CPU >> boot register, programming sequence making CPU up/down. Currently this >> is handled by adding lots of soc_is_exynosMMM checks in the code, in >> an attempt to remove the dependency of such helper functions specific to >> each SoC, let's separate this information pertaining to CPU by introducing >> a new "struct exynos_cpu_info". This struct will contain differences >> associated with CPU on various Exynos SoC. This can be matched by using >> generic API "soc_device_match". >> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 135 insertions(+), 11 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c >> index cb6d199..ff369b9 100644 >> --- a/arch/arm/mach-exynos/platsmp.c >> +++ b/arch/arm/mach-exynos/platsmp.c >> @@ -19,6 +19,7 @@ >> #include <linux/smp.h> >> #include <linux/io.h> >> #include <linux/of_address.h> >> +#include <linux/sys_soc.h> >> #include <linux/soc/samsung/exynos-regs-pmu.h> >> >> #include <asm/cacheflush.h> >> @@ -33,6 +34,16 @@ >> >> extern void exynos4_secondary_startup(void); >> >> +/* >> + * struct exynos_cpu_info - Exynos CPU related info/operations >> + * @cpu_boot_reg: computes cpu boot address for requested cpu >> + */ >> +struct exynos_cpu_info { >> + void __iomem* (*cpu_boot_reg)(u32 cpu); >> +}; > > Beside Marek comments, I think we are getting too many structures for > differentiating Exynos. Actually all of them describe the same - > difference between Exynos revisions. Having many separate structures > means that you have to initialize all of them in different places in > different (probably) order. The good benefit is however making them > local (static) so the scope is limited... but anyway I dislike the > duplication. > OK, regarding duplication, only the way they are initialized is getting duplicated. But again, my long term goal was to remove dependency of pm.c and suspend.c from arm/mach-exynos/{exynos.c,platsmp.c} or common.h in mach-exynos. So that we could move these files as a Exynos Power Management driver in drivers/soc/ where we already moved pm_domain.c, as you see these three files pm.c, suspend.c and pm_domain.c are heavily dependent on PMU driver, and handles Power Management states. So I feel keeping CPU specific data separate from PM specific data makes sense and will help in moving these files as a platform driver one day? Surely I will work out and see how I can minimize duplication. Thanks, Pankaj Dubey > How about combining all of them into one (except the firmware which > has its own register function): > > struct exynos_revision_data { > void __iomem* (*boot_vector_addr)(void); > void __iomem* (*boot_vector_flag)(void); > void (*enter_aftr)(void); > void __iomem* (*cpu_boot_reg)(u32 cpu); > void (*cpu_power_down)(u32 cpu); > void (*cpu_power_up)(u32 cpu); > }; > > Best regards, > Krzysztof > > >> + >> +static const struct exynos_cpu_info *cpu_info; >> + >> #ifdef CONFIG_HOTPLUG_CPU >> static inline void cpu_leave_lowpower(u32 core_id) >> { >> @@ -168,27 +179,57 @@ int exynos_cluster_power_state(int cluster) >> S5P_CORE_LOCAL_PWR_EN); >> } >> >> -static void __iomem *cpu_boot_reg_base(void) >> +static void __iomem *exynos4210_rev11_cpu_boot_reg(u32 cpu) >> { >> - if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1) >> - return pmu_base_addr + S5P_INFORM5; >> - return sysram_base_addr; >> + void __iomem *boot_reg = pmu_base_addr; >> + >> + if (!boot_reg) >> + return IOMEM_ERR_PTR(-ENODEV); >> + >> + boot_reg += S5P_INFORM5; >> + >> + return boot_reg; >> } >> >> -static inline void __iomem *cpu_boot_reg(int cpu) >> +static void __iomem *exynos4412_cpu_boot_reg(u32 cpu) >> { >> - void __iomem *boot_reg; >> + void __iomem *boot_reg = sysram_base_addr; >> >> - boot_reg = cpu_boot_reg_base(); >> if (!boot_reg) >> return IOMEM_ERR_PTR(-ENODEV); >> - if (soc_is_exynos4412()) >> - boot_reg += 4*cpu; >> - else if (soc_is_exynos5420() || soc_is_exynos5800()) >> - boot_reg += 4; >> + >> + boot_reg += 4*cpu; >> + >> return boot_reg; >> } >> >> +static void __iomem *exynos5420_cpu_boot_reg(u32 cpu) >> +{ >> + void __iomem *boot_reg = sysram_base_addr; >> + >> + if (!sysram_base_addr) >> + return IOMEM_ERR_PTR(-ENODEV); >> + >> + boot_reg += 4; >> + >> + return boot_reg; >> +} >> + >> +static void __iomem *exynos_common_cpu_boot_reg(u32 cpu) >> +{ >> + if (!sysram_base_addr) >> + return IOMEM_ERR_PTR(-ENODEV); >> + >> + return sysram_base_addr; >> +} >> + >> +static inline void __iomem *cpu_boot_reg(int cpu) >> +{ >> + if (cpu_info && cpu_info->cpu_boot_reg) >> + return cpu_info->cpu_boot_reg(cpu); >> + return NULL; >> +} >> + >> /* >> * Set wake up by local power mode and execute software reset for given core. >> * >> @@ -296,13 +337,84 @@ int exynos_get_boot_addr(u32 core_id, unsigned long *boot_addr) >> return ret; >> } >> >> +static const struct exynos_cpu_info exynos3250_cpu_info = { >> + .cpu_boot_reg = exynos_common_cpu_boot_reg, >> +}; >> + >> +static const struct exynos_cpu_info exynos5420_cpu_info = { >> + .cpu_boot_reg = exynos5420_cpu_boot_reg, >> +}; >> + >> +static const struct exynos_cpu_info exynos4210_rev11_cpu_info = { >> + .cpu_boot_reg = exynos4210_rev11_cpu_boot_reg, >> +}; >> + >> +static const struct exynos_cpu_info exynos4412_cpu_info = { >> + .cpu_boot_reg = exynos4412_cpu_boot_reg, >> +}; >> + >> +static const struct exynos_cpu_info exynos_common_cpu_info = { >> + .cpu_boot_reg = exynos_common_cpu_boot_reg, >> +}; >> + >> +static const struct soc_device_attribute exynos_soc_revision[] = { >> + { >> + .soc_id = "EXYNOS4210", >> + .revision = "11", >> + .data = &exynos4210_rev11_cpu_info >> + }, { >> + .soc_id = "EXYNOS4210", >> + .revision = "10", >> + .data = &exynos_common_cpu_info >> + } >> +}; >> + >> +static const struct of_device_id exynos_pmu_of_device_ids[] __initconst = { >> + { >> + .compatible = "samsung,exynos3250", >> + .data = &exynos3250_cpu_info >> + }, { >> + .compatible = "samsung,exynos4212", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos4412", >> + .data = &exynos4412_cpu_info >> + }, { >> + .compatible = "samsung,exynos5250", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5260", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5410", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5420", >> + .data = &exynos5420_cpu_info >> + }, { >> + .compatible = "samsung,exynos5440", >> + .data = &exynos_common_cpu_info >> + }, { >> + .compatible = "samsung,exynos5800", >> + .data = &exynos5420_cpu_info >> + }, >> + { /*sentinel*/ }, >> +}; >> + >> static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) >> { >> unsigned long timeout; >> + const struct soc_device_attribute *match; >> u32 mpidr = cpu_logical_map(cpu); >> u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); >> int ret = -ENOSYS; >> >> + if (of_machine_is_compatible("samsung,exynos4210")) { >> + match = soc_device_match(exynos_soc_revision); >> + if (match) >> + cpu_info = (const struct exynos_cpu_info *) match->data; >> + } >> + >> /* >> * Set synchronisation state between this boot processor >> * and the secondary one >> @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) >> >> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >> { >> + const struct of_device_id *match; >> + struct device_node *np; >> + >> + if (!of_machine_is_compatible("samsung,exynos4210")) { >> + np = of_find_matching_node_and_match(NULL, >> + exynos_pmu_of_device_ids, &match); >> + if (!np) >> + pr_err("failed to find supported CPU\n"); >> + else >> + cpu_info = (const struct exynos_cpu_info *) match->data; >> + } >> + >> exynos_sysram_init(); >> >> exynos_set_delayed_reset_assertion(true); >> -- >> 2.7.4 >> > > >
On Fri, Apr 7, 2017 at 4:00 PM, pankaj.dubey <pankaj.dubey@samsung.com> wrote: > > > On Friday 07 April 2017 04:11 PM, Krzysztof Kozlowski wrote: >> On Thu, Mar 30, 2017 at 3:17 PM, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: >>> Various Exynos SoC has different CPU related information, such as CPU >>> boot register, programming sequence making CPU up/down. Currently this >>> is handled by adding lots of soc_is_exynosMMM checks in the code, in >>> an attempt to remove the dependency of such helper functions specific to >>> each SoC, let's separate this information pertaining to CPU by introducing >>> a new "struct exynos_cpu_info". This struct will contain differences >>> associated with CPU on various Exynos SoC. This can be matched by using >>> generic API "soc_device_match". >>> >>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >>> --- >>> arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 135 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c >>> index cb6d199..ff369b9 100644 >>> --- a/arch/arm/mach-exynos/platsmp.c >>> +++ b/arch/arm/mach-exynos/platsmp.c >>> @@ -19,6 +19,7 @@ >>> #include <linux/smp.h> >>> #include <linux/io.h> >>> #include <linux/of_address.h> >>> +#include <linux/sys_soc.h> >>> #include <linux/soc/samsung/exynos-regs-pmu.h> >>> >>> #include <asm/cacheflush.h> >>> @@ -33,6 +34,16 @@ >>> >>> extern void exynos4_secondary_startup(void); >>> >>> +/* >>> + * struct exynos_cpu_info - Exynos CPU related info/operations >>> + * @cpu_boot_reg: computes cpu boot address for requested cpu >>> + */ >>> +struct exynos_cpu_info { >>> + void __iomem* (*cpu_boot_reg)(u32 cpu); >>> +}; >> >> Beside Marek comments, I think we are getting too many structures for >> differentiating Exynos. Actually all of them describe the same - >> difference between Exynos revisions. Having many separate structures >> means that you have to initialize all of them in different places in >> different (probably) order. The good benefit is however making them >> local (static) so the scope is limited... but anyway I dislike the >> duplication. >> > > OK, regarding duplication, only the way they are initialized is getting > duplicated. But again, my long term goal was to remove dependency of > pm.c and suspend.c from arm/mach-exynos/{exynos.c,platsmp.c} or common.h > in mach-exynos. So that we could move these files as a Exynos Power > Management driver in drivers/soc/ where we already moved pm_domain.c, as > you see these three files pm.c, suspend.c and pm_domain.c are heavily > dependent on PMU driver, and handles Power Management states. > > So I feel keeping CPU specific data separate from PM specific data makes > sense and will help in moving these files as a platform driver one day? > > Surely I will work out and see how I can minimize duplication. Hm, in that case it would make sense. However I am not quite sure what is the benefit of moving this from arm/mach to drivers/soc. This code will not be re-used on any other platform, unlike existing drivers/soc/samsung drivers which are used also on ARMv8. Best regards, Krzysztof
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index cb6d199..ff369b9 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -19,6 +19,7 @@ #include <linux/smp.h> #include <linux/io.h> #include <linux/of_address.h> +#include <linux/sys_soc.h> #include <linux/soc/samsung/exynos-regs-pmu.h> #include <asm/cacheflush.h> @@ -33,6 +34,16 @@ extern void exynos4_secondary_startup(void); +/* + * struct exynos_cpu_info - Exynos CPU related info/operations + * @cpu_boot_reg: computes cpu boot address for requested cpu + */ +struct exynos_cpu_info { + void __iomem* (*cpu_boot_reg)(u32 cpu); +}; + +static const struct exynos_cpu_info *cpu_info; + #ifdef CONFIG_HOTPLUG_CPU static inline void cpu_leave_lowpower(u32 core_id) { @@ -168,27 +179,57 @@ int exynos_cluster_power_state(int cluster) S5P_CORE_LOCAL_PWR_EN); } -static void __iomem *cpu_boot_reg_base(void) +static void __iomem *exynos4210_rev11_cpu_boot_reg(u32 cpu) { - if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1) - return pmu_base_addr + S5P_INFORM5; - return sysram_base_addr; + void __iomem *boot_reg = pmu_base_addr; + + if (!boot_reg) + return IOMEM_ERR_PTR(-ENODEV); + + boot_reg += S5P_INFORM5; + + return boot_reg; } -static inline void __iomem *cpu_boot_reg(int cpu) +static void __iomem *exynos4412_cpu_boot_reg(u32 cpu) { - void __iomem *boot_reg; + void __iomem *boot_reg = sysram_base_addr; - boot_reg = cpu_boot_reg_base(); if (!boot_reg) return IOMEM_ERR_PTR(-ENODEV); - if (soc_is_exynos4412()) - boot_reg += 4*cpu; - else if (soc_is_exynos5420() || soc_is_exynos5800()) - boot_reg += 4; + + boot_reg += 4*cpu; + return boot_reg; } +static void __iomem *exynos5420_cpu_boot_reg(u32 cpu) +{ + void __iomem *boot_reg = sysram_base_addr; + + if (!sysram_base_addr) + return IOMEM_ERR_PTR(-ENODEV); + + boot_reg += 4; + + return boot_reg; +} + +static void __iomem *exynos_common_cpu_boot_reg(u32 cpu) +{ + if (!sysram_base_addr) + return IOMEM_ERR_PTR(-ENODEV); + + return sysram_base_addr; +} + +static inline void __iomem *cpu_boot_reg(int cpu) +{ + if (cpu_info && cpu_info->cpu_boot_reg) + return cpu_info->cpu_boot_reg(cpu); + return NULL; +} + /* * Set wake up by local power mode and execute software reset for given core. * @@ -296,13 +337,84 @@ int exynos_get_boot_addr(u32 core_id, unsigned long *boot_addr) return ret; } +static const struct exynos_cpu_info exynos3250_cpu_info = { + .cpu_boot_reg = exynos_common_cpu_boot_reg, +}; + +static const struct exynos_cpu_info exynos5420_cpu_info = { + .cpu_boot_reg = exynos5420_cpu_boot_reg, +}; + +static const struct exynos_cpu_info exynos4210_rev11_cpu_info = { + .cpu_boot_reg = exynos4210_rev11_cpu_boot_reg, +}; + +static const struct exynos_cpu_info exynos4412_cpu_info = { + .cpu_boot_reg = exynos4412_cpu_boot_reg, +}; + +static const struct exynos_cpu_info exynos_common_cpu_info = { + .cpu_boot_reg = exynos_common_cpu_boot_reg, +}; + +static const struct soc_device_attribute exynos_soc_revision[] = { + { + .soc_id = "EXYNOS4210", + .revision = "11", + .data = &exynos4210_rev11_cpu_info + }, { + .soc_id = "EXYNOS4210", + .revision = "10", + .data = &exynos_common_cpu_info + } +}; + +static const struct of_device_id exynos_pmu_of_device_ids[] __initconst = { + { + .compatible = "samsung,exynos3250", + .data = &exynos3250_cpu_info + }, { + .compatible = "samsung,exynos4212", + .data = &exynos_common_cpu_info + }, { + .compatible = "samsung,exynos4412", + .data = &exynos4412_cpu_info + }, { + .compatible = "samsung,exynos5250", + .data = &exynos_common_cpu_info + }, { + .compatible = "samsung,exynos5260", + .data = &exynos_common_cpu_info + }, { + .compatible = "samsung,exynos5410", + .data = &exynos_common_cpu_info + }, { + .compatible = "samsung,exynos5420", + .data = &exynos5420_cpu_info + }, { + .compatible = "samsung,exynos5440", + .data = &exynos_common_cpu_info + }, { + .compatible = "samsung,exynos5800", + .data = &exynos5420_cpu_info + }, + { /*sentinel*/ }, +}; + static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) { unsigned long timeout; + const struct soc_device_attribute *match; u32 mpidr = cpu_logical_map(cpu); u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); int ret = -ENOSYS; + if (of_machine_is_compatible("samsung,exynos4210")) { + match = soc_device_match(exynos_soc_revision); + if (match) + cpu_info = (const struct exynos_cpu_info *) match->data; + } + /* * Set synchronisation state between this boot processor * and the secondary one @@ -387,6 +499,18 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) { + const struct of_device_id *match; + struct device_node *np; + + if (!of_machine_is_compatible("samsung,exynos4210")) { + np = of_find_matching_node_and_match(NULL, + exynos_pmu_of_device_ids, &match); + if (!np) + pr_err("failed to find supported CPU\n"); + else + cpu_info = (const struct exynos_cpu_info *) match->data; + } + exynos_sysram_init(); exynos_set_delayed_reset_assertion(true);
Various Exynos SoC has different CPU related information, such as CPU boot register, programming sequence making CPU up/down. Currently this is handled by adding lots of soc_is_exynosMMM checks in the code, in an attempt to remove the dependency of such helper functions specific to each SoC, let's separate this information pertaining to CPU by introducing a new "struct exynos_cpu_info". This struct will contain differences associated with CPU on various Exynos SoC. This can be matched by using generic API "soc_device_match". Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> --- arch/arm/mach-exynos/platsmp.c | 146 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 135 insertions(+), 11 deletions(-)