During consolidation work on Exynos platform for SCU, it has been found
that across many ARM platforms Cortex-A9 boards, SCU enabling code is
duplicated.
Subsequently it felt that [1] by introducing generic API support in arm/smp_scu.c
file will help in removing all these duplicate work from all such platforms.
This patch series introduces support for parsting SCU device node to enable SCU
and get SCU base address. If a platform does not support SCU device node in DT, the
API will fallback to scu_a9_get_base to obtain SCU base address.
[1]: https://www.spinics.net/lists/arm-kernel/msg541830.html
Arnd helped in consolidating various ARM platforms duplicating SCU code as below:
a) of_iomap: berlin, exynos, mvebu, realview, rockchip, socfpga, sti, ux500, vexpress
b) ioremap(scu_a9_get_base()): bcm63xx, bcm, hisi, zx
c) iotable_init, scu_a9_get_base(): imx, omap4, tegra, zynq
d) ioremap(constant): shmobile, spear13xx
Based on his suggestion on how to move all these platform to use generic API
I have modified EXYNOS, berlin, realview, socfpga, STi, ux500, vexpress, BCM,
tegra, rockchip, imx, zynq, hisi, mvebu and ZX platform files to adopt generic API for
SCU enabling to obtaining SCU base address.
Only fourth case shmobile and spear13xx will be left to adopt to this new API after this,
which I felt some details information about these platforms and I leave it to the maintainers
of these platforms.
NOTE: This patch series has been tested on EXYNOS platform using Exynos4210 based Origen board
for SMP boot.
Rest platforms are *ONLY* Compiled Tested.
I request maintainers/developers having h/w of these platforms to test this series on their
platforms.
Pankaj Dubey (16):
ARM: scu: Provide support for parsing SCU device node to enable SCU
ARM: EXYNOS: use generic API to enable SCU
ARM: berlin: use generic API for enabling SCU
ARM: realview: use generic API for enabling SCU
ARM: socfpga: use generic API for enabling SCU
ARM: STi: use generic API for enabling SCU
ARM: ux500: use generic API for enabling SCU
ARM: vexpress: use generic API for enabling SCU
ARM: BCM: use generic API for enabling SCU
ARM: tegra: use generic API for enabling SCU
ARM: rockchip: use generic API for enabling SCU
ARM: imx: use generic API for enabling SCU
ARM: zynq: use generic API for enabling SCU
ARM: hisi: use generic API for enabling SCU
ARM: mvebu: use generic API for enabling SCU
ARM: zx: use generic API for enabling SCU
arch/arm/include/asm/smp_scu.h | 4 +++
arch/arm/kernel/smp_scu.c | 56 +++++++++++++++++++++++++++++++++++++
arch/arm/mach-bcm/bcm63xx_smp.c | 18 ++----------
arch/arm/mach-bcm/platsmp.c | 46 +-----------------------------
arch/arm/mach-berlin/platsmp.c | 17 ++++-------
arch/arm/mach-exynos/common.h | 1 -
arch/arm/mach-exynos/platsmp.c | 30 +++-----------------
arch/arm/mach-exynos/pm.c | 4 +--
arch/arm/mach-exynos/suspend.c | 14 +++-------
arch/arm/mach-hisi/platsmp.c | 24 ++++------------
arch/arm/mach-imx/common.h | 5 ----
arch/arm/mach-imx/mach-imx6q.c | 8 +-----
arch/arm/mach-imx/platsmp.c | 32 ++++-----------------
arch/arm/mach-imx/pm-imx6.c | 3 +-
arch/arm/mach-mvebu/board-v7.c | 8 ++----
arch/arm/mach-realview/platsmp-dt.c | 29 ++++++-------------
arch/arm/mach-rockchip/platsmp.c | 12 ++------
arch/arm/mach-socfpga/platsmp.c | 11 +-------
arch/arm/mach-sti/platsmp.c | 9 +-----
arch/arm/mach-tegra/platsmp.c | 2 +-
arch/arm/mach-ux500/platsmp.c | 20 +------------
arch/arm/mach-vexpress/platsmp.c | 13 +--------
arch/arm/mach-zx/platsmp.c | 6 ++--
arch/arm/mach-zynq/common.c | 32 +--------------------
arch/arm/mach-zynq/common.h | 2 --
arch/arm/mach-zynq/platsmp.c | 2 ++
26 files changed, 115 insertions(+), 293 deletions(-)
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
This patch also fixes build failure in case !SMP caused
by commit SHA ID: 94210b1abb2 which is already merged in
krzk/for-next branch
CC: Krzysztof Kozlowski <[email protected]>
CC: [email protected]
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-exynos/common.h | 1 -
arch/arm/mach-exynos/platsmp.c | 30 ++++--------------------------
arch/arm/mach-exynos/pm.c | 4 ++--
arch/arm/mach-exynos/suspend.c | 14 ++++----------
4 files changed, 10 insertions(+), 39 deletions(-)
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index fb12d11..d19064b 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -156,7 +156,6 @@ extern void exynos_cpu_restore_register(void);
extern void exynos_pm_central_suspend(void);
extern int exynos_pm_central_resume(void);
extern void exynos_enter_aftr(void);
-extern int exynos_scu_enable(void);
extern struct cpuidle_exynos_data cpuidle_coupled_exynos_data;
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 94405c7..2e5ecc1 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -168,27 +168,6 @@ int exynos_cluster_power_state(int cluster)
S5P_CORE_LOCAL_PWR_EN);
}
-/**
- * exynos_scu_enable : enables SCU for Cortex-A9 based system
- * returns 0 on success else non-zero error code
- */
-int exynos_scu_enable(void)
-{
- struct device_node *np;
- void __iomem *scu_base;
-
- np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- scu_base = of_iomap(np, 0);
- of_node_put(np);
- if (!scu_base) {
- pr_err("%s failed to map scu_base\n", __func__);
- return -ENOMEM;
- }
- scu_enable(scu_base);
- iounmap(scu_base);
- return 0;
-}
-
static void __iomem *cpu_boot_reg_base(void)
{
if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1)
@@ -409,11 +388,10 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
exynos_set_delayed_reset_assertion(true);
- if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
- /* if exynos_scu_enable fails, return */
- if (exynos_scu_enable())
- return;
- }
+ /* if of_scu_enable fails, return */
+ if (scu_a9_has_base() && of_scu_enable())
+ return;
+
/*
* Write the address of secondary startup into the
* system-wide flags register. The boot monitor waits
diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index c0b46c3..9678438 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -174,8 +174,8 @@ void exynos_enter_aftr(void)
cpu_suspend(0, exynos_aftr_finisher);
- if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
- exynos_scu_enable();
+ if (scu_a9_has_base()) {
+ of_scu_enable();
if (call_firmware_op(resume) == -ENOSYS)
exynos_cpu_restore_register();
}
diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index 73df9f3..5414282 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -451,19 +451,16 @@ static void exynos_pm_release_retention(void)
static void exynos_pm_resume(void)
{
- u32 cpuid = read_cpuid_part();
-
if (exynos_pm_central_resume())
goto early_wakeup;
/* For release retention */
exynos_pm_release_retention();
- if (cpuid == ARM_CPU_PART_CORTEX_A9)
- exynos_scu_enable();
+ if (scu_a9_has_base())
+ of_scu_enable();
- if (call_firmware_op(resume) == -ENOSYS
- && cpuid == ARM_CPU_PART_CORTEX_A9)
+ if (call_firmware_op(resume) == -ENOSYS && scu_a9_has_base())
exynos_cpu_restore_register();
early_wakeup:
@@ -475,8 +472,6 @@ static void exynos_pm_resume(void)
static void exynos3250_pm_resume(void)
{
- u32 cpuid = read_cpuid_part();
-
if (exynos_pm_central_resume())
goto early_wakeup;
@@ -485,8 +480,7 @@ static void exynos3250_pm_resume(void)
pmu_raw_writel(S5P_USE_STANDBY_WFI_ALL, S5P_CENTRAL_SEQ_OPTION);
- if (call_firmware_op(resume) == -ENOSYS
- && cpuid == ARM_CPU_PART_CORTEX_A9)
+ if (call_firmware_op(resume) == -ENOSYS && scu_a9_has_base())
exynos_cpu_restore_register();
early_wakeup:
--
2.7.4
Many platforms are duplicating code for enabling SCU, lets add
common code to enable SCU by parsing SCU device node so the duplication
in each platform can be avoided.
CC: Krzysztof Kozlowski <[email protected]>
CC: Jisheng Zhang <[email protected]>
CC: Russell King <[email protected]>
CC: Dinh Nguyen <[email protected]>
CC: Patrice Chotard <[email protected]>
CC: Linus Walleij <[email protected]>
CC: Liviu Dudau <[email protected]>
CC: Ray Jui <[email protected]>
CC: Stephen Warren <[email protected]>
CC: Heiko Stuebner <[email protected]>
CC: Shawn Guo <[email protected]>
CC: Michal Simek <[email protected]>
CC: Wei Xu <[email protected]>
CC: Andrew Lunn <[email protected]>
CC: Jun Nie <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/include/asm/smp_scu.h | 4 +++
arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index bfe163c..fdeec07 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
#if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
void scu_enable(void __iomem *scu_base);
+void __iomem *of_scu_get_base(void);
+int of_scu_enable(void);
#else
static inline void scu_enable(void __iomem *scu_base) {}
+static inline void __iomem *of_scu_get_base(void) {return NULL; }
+static inline int of_scu_enable(void) {return 0; }
#endif
#endif
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 72f9241..d0ac3ed 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -10,6 +10,7 @@
*/
#include <linux/init.h>
#include <linux/io.h>
+#include <linux/of_address.h>
#include <asm/smp_plat.h>
#include <asm/smp_scu.h>
@@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
*/
flush_cache_all();
}
+
+static const struct of_device_id scu_match[] = {
+ { .compatible = "arm,cortex-a9-scu", },
+ { .compatible = "arm,cortex-a5-scu", },
+ { }
+};
+
+/*
+ * Helper API to get SCU base address
+ * In case platform DT do not have SCU node, or iomap fails
+ * this call will fallback and will try to map via call to
+ * scu_a9_get_base.
+ * This will return ownership of scu_base to the caller
+ */
+void __iomem *of_scu_get_base(void)
+{
+ unsigned long base = 0;
+ struct device_node *np;
+ void __iomem *scu_base;
+
+ np = of_find_matching_node(NULL, scu_match);
+ scu_base = of_iomap(np, 0);
+ of_node_put(np);
+ if (!scu_base) {
+ pr_err("%s failed to map scu_base via DT\n", __func__);
+ if (scu_a9_has_base()) {
+ base = scu_a9_get_base();
+ scu_base = ioremap(base, SZ_4K);
+ }
+ if (!scu_base) {
+ pr_err("%s failed to map scu_base\n", __func__);
+ return IOMEM_ERR_PTR(-ENOMEM);
+ }
+ }
+ return scu_base;
+}
+
+/*
+ * Enable SCU via mapping scu_base DT
+ * If scu_base mapped successfully scu will be enabled and in case of
+ * failure if will return non-zero error code
+ */
+int of_scu_enable(void)
+{
+ void __iomem *scu_base;
+
+ scu_base = of_scu_get_base();
+ if (!IS_ERR(scu_base)) {
+ scu_enable(scu_base);
+ iounmap(scu_base);
+ return 0;
+ }
+ return PTR_ERR(scu_base);
+}
+
#endif
/*
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
Also this patch removes computation of number of cores
from SCU, as for DT platform it will be taken care from
DT CPU device nodes.
CC: Russell King <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-realview/platsmp-dt.c | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-realview/platsmp-dt.c b/arch/arm/mach-realview/platsmp-dt.c
index 70ca99e..b2dbf77 100644
--- a/arch/arm/mach-realview/platsmp-dt.c
+++ b/arch/arm/mach-realview/platsmp-dt.c
@@ -23,8 +23,6 @@
static const struct of_device_id realview_scu_match[] = {
{ .compatible = "arm,arm11mp-scu", },
- { .compatible = "arm,cortex-a9-scu", },
- { .compatible = "arm,cortex-a5-scu", },
{ }
};
@@ -41,27 +39,18 @@ static void __init realview_smp_prepare_cpus(unsigned int max_cpus)
struct device_node *np;
void __iomem *scu_base;
struct regmap *map;
- unsigned int ncores;
int i;
- np = of_find_matching_node(NULL, realview_scu_match);
- if (!np) {
- pr_err("PLATSMP: No SCU base address\n");
- return;
+ if (of_scu_enable()) {
+ np = of_find_matching_node(NULL, realview_scu_match);
+ scu_base = of_iomap(np, 0);
+ of_node_put(np);
+ if (!scu_base) {
+ pr_err("PLATSMP: No SCU remap\n");
+ return;
+ }
+ scu_enable(scu_base);
}
- scu_base = of_iomap(np, 0);
- of_node_put(np);
- if (!scu_base) {
- pr_err("PLATSMP: No SCU remap\n");
- return;
- }
-
- scu_enable(scu_base);
- ncores = scu_get_core_count(scu_base);
- pr_info("SCU: %d cores detected\n", ncores);
- for (i = 0; i < ncores; i++)
- set_cpu_possible(i, true);
- iounmap(scu_base);
/* The syscon contains the magic SMP start address registers */
np = of_find_matching_node(NULL, realview_syscon_match);
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Linus Walleij <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-ux500/platsmp.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/arch/arm/mach-ux500/platsmp.c b/arch/arm/mach-ux500/platsmp.c
index 8f2f615..e1927ae 100644
--- a/arch/arm/mach-ux500/platsmp.c
+++ b/arch/arm/mach-ux500/platsmp.c
@@ -66,28 +66,10 @@ static void wakeup_secondary(void)
static void __init ux500_smp_prepare_cpus(unsigned int max_cpus)
{
- struct device_node *np;
- static void __iomem *scu_base;
- unsigned int ncores;
- int i;
-
- np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- if (!np) {
- pr_err("No SCU base address\n");
- return;
- }
- scu_base = of_iomap(np, 0);
- of_node_put(np);
- if (!scu_base) {
+ if (of_scu_enable()) {
pr_err("No SCU remap\n");
return;
}
-
- scu_enable(scu_base);
- ncores = scu_get_core_count(scu_base);
- for (i = 0; i < ncores; i++)
- set_cpu_possible(i, true);
- iounmap(scu_base);
}
static int ux500_boot_secondary(unsigned int cpu, struct task_struct *idle)
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Dinh Nguyen <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-socfpga/platsmp.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c
index 0794574..d3f0a07 100644
--- a/arch/arm/mach-socfpga/platsmp.c
+++ b/arch/arm/mach-socfpga/platsmp.c
@@ -79,19 +79,10 @@ static int socfpga_a10_boot_secondary(unsigned int cpu, struct task_struct *idle
static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus)
{
- struct device_node *np;
- void __iomem *socfpga_scu_base_addr;
-
- np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- if (!np) {
+ if (of_scu_enable()) {
pr_err("%s: missing scu\n", __func__);
return;
}
-
- socfpga_scu_base_addr = of_iomap(np, 0);
- if (!socfpga_scu_base_addr)
- return;
- scu_enable(socfpga_scu_base_addr);
}
#ifdef CONFIG_HOTPLUG_CPU
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Stephen Warren <[email protected]>
CC: [email protected]
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-tegra/platsmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 75620ae..3467617 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -179,7 +179,7 @@ static void __init tegra_smp_prepare_cpus(unsigned int max_cpus)
cpumask_set_cpu(0, &tegra_cpu_init_mask);
if (scu_a9_has_base())
- scu_enable(IO_ADDRESS(scu_a9_get_base()));
+ of_scu_enable();
}
const struct smp_operations tegra_smp_ops __initconst = {
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
At the same time this patch cleans up mach-imx platform files by
removing static mapping of SCU and dropping imx_scu_map_io function.
CC: Shawn Guo <[email protected]>
CC: Sascha Hauer <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-imx/common.h | 5 -----
arch/arm/mach-imx/mach-imx6q.c | 8 +-------
arch/arm/mach-imx/platsmp.c | 32 +++++---------------------------
arch/arm/mach-imx/pm-imx6.c | 3 ++-
4 files changed, 8 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index c4436d9..9787d5f 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -87,11 +87,6 @@ u32 imx_get_cpu_arg(int cpu);
void imx_set_cpu_arg(int cpu, u32 arg);
#ifdef CONFIG_SMP
void v7_secondary_startup(void);
-void imx_scu_map_io(void);
-void imx_smp_prepare(void);
-#else
-static inline void imx_scu_map_io(void) {}
-static inline void imx_smp_prepare(void) {}
#endif
void imx_src_init(void);
void imx_gpc_pre_suspend(bool arm_power_off);
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 45801b2..1c6cc9f 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -383,12 +383,6 @@ static void __init imx6q_init_late(void)
}
}
-static void __init imx6q_map_io(void)
-{
- debug_ll_io_init();
- imx_scu_map_io();
-}
-
static void __init imx6q_init_irq(void)
{
imx_gpc_check_dt();
@@ -410,7 +404,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad/DualLite (Device Tree)")
.l2c_aux_val = 0,
.l2c_aux_mask = ~0,
.smp = smp_ops(imx_smp_ops),
- .map_io = imx6q_map_io,
+ .map_io = debug_ll_io_init,
.init_irq = imx6q_init_irq,
.init_machine = imx6q_init_machine,
.init_late = imx6q_init_late,
diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 711dbbd..c032369 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -26,26 +26,6 @@
u32 g_diag_reg;
static void __iomem *scu_base;
-static struct map_desc scu_io_desc __initdata = {
- /* .virtual and .pfn are run-time assigned */
- .length = SZ_4K,
- .type = MT_DEVICE,
-};
-
-void __init imx_scu_map_io(void)
-{
- unsigned long base;
-
- /* Get SCU base */
- asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (base));
-
- scu_io_desc.virtual = IMX_IO_P2V(base);
- scu_io_desc.pfn = __phys_to_pfn(base);
- iotable_init(&scu_io_desc, 1);
-
- scu_base = IMX_IO_ADDRESS(base);
-}
-
static int imx_boot_secondary(unsigned int cpu, struct task_struct *idle)
{
imx_set_cpu_jump(cpu, v7_secondary_startup);
@@ -61,20 +41,18 @@ static void __init imx_smp_init_cpus(void)
{
int i, ncores;
- ncores = scu_get_core_count(scu_base);
+ if (!IS_ERR(scu_base))
+ ncores = scu_get_core_count(scu_base);
for (i = ncores; i < NR_CPUS; i++)
set_cpu_possible(i, false);
}
-void imx_smp_prepare(void)
-{
- scu_enable(scu_base);
-}
-
static void __init imx_smp_prepare_cpus(unsigned int max_cpus)
{
- imx_smp_prepare();
+ scu_base = of_scu_get_base();
+ if (!IS_ERR(scu_base))
+ scu_enable(scu_base);
/*
* The diagnostic register holds the errata bits. Mostly bootloader
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 1515e49..859aacb 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -26,6 +26,7 @@
#include <asm/fncpy.h>
#include <asm/proc-fns.h>
#include <asm/suspend.h>
+#include <asm/smp_scu.h>
#include <asm/tlb.h>
#include "common.h"
@@ -393,7 +394,7 @@ static int imx6q_pm_enter(suspend_state_t state)
/* Zzz ... */
cpu_suspend(0, imx6q_suspend_finish);
if (cpu_is_imx6q() || cpu_is_imx6dl())
- imx_smp_prepare();
+ of_scu_enable();
imx_anatop_post_resume();
imx_gpc_post_resume();
imx6_enable_rbc(false);
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Liviu Dudau <[email protected]>
CC: Sudeep Holla <[email protected]>
CC: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-vexpress/platsmp.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
index 8b8d072..17dee2b 100644
--- a/arch/arm/mach-vexpress/platsmp.c
+++ b/arch/arm/mach-vexpress/platsmp.c
@@ -41,20 +41,9 @@ bool __init vexpress_smp_init_ops(void)
return false;
}
-static const struct of_device_id vexpress_smp_dt_scu_match[] __initconst = {
- { .compatible = "arm,cortex-a5-scu", },
- { .compatible = "arm,cortex-a9-scu", },
- {}
-};
-
static void __init vexpress_smp_dt_prepare_cpus(unsigned int max_cpus)
{
- struct device_node *scu = of_find_matching_node(NULL,
- vexpress_smp_dt_scu_match);
-
- if (scu)
- scu_enable(of_iomap(scu, 0));
-
+ of_scu_enable();
/*
* Write the address of secondary startup into the
* system-wide flags register. The boot monitor waits
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Wei Xu <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-hisi/platsmp.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-hisi/platsmp.c b/arch/arm/mach-hisi/platsmp.c
index e1d6764..425a291 100644
--- a/arch/arm/mach-hisi/platsmp.c
+++ b/arch/arm/mach-hisi/platsmp.c
@@ -39,29 +39,14 @@ int hi3xxx_get_cpu_jump(int cpu)
return readl_relaxed(ctrl_base + ((cpu - 1) << 2));
}
-static void __init hisi_enable_scu_a9(void)
-{
- unsigned long base = 0;
- void __iomem *scu_base = NULL;
-
- if (scu_a9_has_base()) {
- base = scu_a9_get_base();
- scu_base = ioremap(base, SZ_4K);
- if (!scu_base) {
- pr_err("ioremap(scu_base) failed\n");
- return;
- }
- scu_enable(scu_base);
- iounmap(scu_base);
- }
-}
-
static void __init hi3xxx_smp_prepare_cpus(unsigned int max_cpus)
{
struct device_node *np = NULL;
u32 offset = 0;
- hisi_enable_scu_a9();
+ if (scu_a9_has_base())
+ of_scu_enable();
+
if (!ctrl_base) {
np = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
if (!np) {
@@ -100,7 +85,8 @@ static const struct smp_operations hi3xxx_smp_ops __initconst = {
static void __init hisi_common_smp_prepare_cpus(unsigned int max_cpus)
{
- hisi_enable_scu_a9();
+ if (scu_a9_has_base())
+ of_scu_enable();
}
static void hix5hd2_set_scu_boot_addr(phys_addr_t start_addr, phys_addr_t jump_addr)
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Jun Nie <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-zx/platsmp.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-zx/platsmp.c b/arch/arm/mach-zx/platsmp.c
index 0297f92..2788c34 100644
--- a/arch/arm/mach-zx/platsmp.c
+++ b/arch/arm/mach-zx/platsmp.c
@@ -44,13 +44,11 @@ static void __iomem *scu_base;
void __init zx_smp_prepare_cpus(unsigned int max_cpus)
{
struct device_node *np;
- unsigned long base = 0;
void __iomem *aonsysctrl_base;
void __iomem *sys_iram;
- base = scu_a9_get_base();
- scu_base = ioremap(base, SZ_256);
- if (!scu_base) {
+ scu_base = of_scu_get_base();
+ if (IS_ERR(scu_base)) {
pr_err("%s: failed to map scu\n", __func__);
return;
}
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
At the same time this patch cleans up mach-zynq platform files by
removing static mapping of SCU and dropping zynq_scu_map_io and zynq_map_io
functions.
CC: Michal Simek <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-zynq/common.c | 32 +-------------------------------
arch/arm/mach-zynq/common.h | 2 --
arch/arm/mach-zynq/platsmp.c | 2 ++
3 files changed, 3 insertions(+), 33 deletions(-)
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index d12002c..3986b2b 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -38,7 +38,6 @@
#include <asm/mach-types.h>
#include <asm/page.h>
#include <asm/pgtable.h>
-#include <asm/smp_scu.h>
#include <asm/system_info.h>
#include <asm/hardware/cache-l2x0.h>
@@ -48,8 +47,6 @@
#define ZYNQ_DEVCFG_PS_VERSION_SHIFT 28
#define ZYNQ_DEVCFG_PS_VERSION_MASK 0xF
-void __iomem *zynq_scu_base;
-
/**
* zynq_memory_init - Initialize special memory
*
@@ -153,33 +150,6 @@ static void __init zynq_timer_init(void)
clocksource_probe();
}
-static struct map_desc zynq_cortex_a9_scu_map __initdata = {
- .length = SZ_256,
- .type = MT_DEVICE,
-};
-
-static void __init zynq_scu_map_io(void)
-{
- unsigned long base;
-
- base = scu_a9_get_base();
- zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
- /* Expected address is in vmalloc area that's why simple assign here */
- zynq_cortex_a9_scu_map.virtual = base;
- iotable_init(&zynq_cortex_a9_scu_map, 1);
- zynq_scu_base = (void __iomem *)base;
- BUG_ON(!zynq_scu_base);
-}
-
-/**
- * zynq_map_io - Create memory mappings needed for early I/O.
- */
-static void __init zynq_map_io(void)
-{
- debug_ll_io_init();
- zynq_scu_map_io();
-}
-
static void __init zynq_irq_init(void)
{
zynq_early_slcr_init();
@@ -196,7 +166,7 @@ DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
.l2c_aux_val = 0x00400000,
.l2c_aux_mask = 0xffbfffff,
.smp = smp_ops(zynq_smp_ops),
- .map_io = zynq_map_io,
+ .map_io = debug_ll_io_init,
.init_irq = zynq_irq_init,
.init_machine = zynq_init_machine,
.init_late = zynq_init_late,
diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
index e771933..7c2f008 100644
--- a/arch/arm/mach-zynq/common.h
+++ b/arch/arm/mach-zynq/common.h
@@ -33,8 +33,6 @@ extern int zynq_cpun_start(u32 address, int cpu);
extern const struct smp_operations zynq_smp_ops;
#endif
-extern void __iomem *zynq_scu_base;
-
void zynq_pm_late_init(void);
static inline void zynq_core_pm_init(void)
diff --git a/arch/arm/mach-zynq/platsmp.c b/arch/arm/mach-zynq/platsmp.c
index 7cd9865..2d09119 100644
--- a/arch/arm/mach-zynq/platsmp.c
+++ b/arch/arm/mach-zynq/platsmp.c
@@ -33,6 +33,7 @@
* be called from zynq_cpun_start() because it is not in __init section.
*/
static int ncores;
+static void __iomem *zynq_scu_base;
int zynq_cpun_start(u32 address, int cpu)
{
@@ -108,6 +109,7 @@ static void __init zynq_smp_init_cpus(void)
static void __init zynq_smp_prepare_cpus(unsigned int max_cpus)
{
+ zynq_scu_base = of_scu_get_base();
scu_enable(zynq_scu_base);
}
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Jason Cooper <[email protected]>
CC: Andrew Lunn <[email protected]>
CC: Gregory Clement <[email protected]>
CC: Sebastian Hesselbarth <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-mvebu/board-v7.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index ccca951..d7014a3 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -41,13 +41,9 @@ static void __iomem *scu_base;
*/
static void __init mvebu_scu_enable(void)
{
- struct device_node *np =
- of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- if (np) {
- scu_base = of_iomap(np, 0);
+ scu_base = of_scu_get_base();
+ if (!IS_ERR(scu_base))
scu_enable(scu_base);
- of_node_put(np);
- }
}
void __iomem *mvebu_get_scu_base(void)
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Heiko Stuebner <[email protected]>
CC: [email protected]
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-rockchip/platsmp.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index 4d827a0..31169cf 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -282,21 +282,15 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
if (has_pmu && rockchip_smp_prepare_pmu())
return;
- if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
+ if (scu_a9_has_base()) {
if (rockchip_smp_prepare_sram(node))
return;
/* enable the SCU power domain */
pmu_set_power_domain(PMU_PWRDN_SCU, true);
- node = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- if (!node) {
- pr_err("%s: missing scu\n", __func__);
- return;
- }
-
- scu_base_addr = of_iomap(node, 0);
- if (!scu_base_addr) {
+ scu_base_addr = of_scu_get_base();
+ if (IS_ERR(scu_base_addr)) {
pr_err("%s: could not map scu registers\n", __func__);
return;
}
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Patrice Chotard <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-sti/platsmp.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
index ea5a227..0bc7ff8 100644
--- a/arch/arm/mach-sti/platsmp.c
+++ b/arch/arm/mach-sti/platsmp.c
@@ -99,19 +99,12 @@ static int sti_boot_secondary(unsigned int cpu, struct task_struct *idle)
static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
{
struct device_node *np;
- void __iomem *scu_base;
u32 __iomem *cpu_strt_ptr;
u32 release_phys;
int cpu;
unsigned long entry_pa = virt_to_phys(sti_secondary_startup);
- np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
-
- if (np) {
- scu_base = of_iomap(np, 0);
- scu_enable(scu_base);
- of_node_put(np);
- }
+ of_scu_enable();
if (max_cpus <= 1)
return;
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Florian Fainelli <[email protected]>
CC: Ray Jui <[email protected]>
CC: Scott Branden <[email protected]>
CC: [email protected]
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-bcm/bcm63xx_smp.c | 18 ++--------------
arch/arm/mach-bcm/platsmp.c | 46 +----------------------------------------
2 files changed, 3 insertions(+), 61 deletions(-)
diff --git a/arch/arm/mach-bcm/bcm63xx_smp.c b/arch/arm/mach-bcm/bcm63xx_smp.c
index 9b6727e..a4c6ecd 100644
--- a/arch/arm/mach-bcm/bcm63xx_smp.c
+++ b/arch/arm/mach-bcm/bcm63xx_smp.c
@@ -20,9 +20,6 @@
#include "bcm63xx_smp.h"
-/* Size of mapped Cortex A9 SCU address space */
-#define CORTEX_A9_SCU_SIZE 0x58
-
/*
* Enable the Cortex A9 Snoop Control Unit
*
@@ -35,7 +32,6 @@
*/
static int __init scu_a9_enable(void)
{
- unsigned long config_base;
void __iomem *scu_base;
unsigned int i, ncores;
@@ -44,19 +40,9 @@ static int __init scu_a9_enable(void)
return -ENXIO;
}
- /* Config base address register value is zero for uniprocessor */
- config_base = scu_a9_get_base();
- if (!config_base) {
- pr_err("hardware reports only one core\n");
- return -ENOENT;
- }
-
- scu_base = ioremap((phys_addr_t)config_base, CORTEX_A9_SCU_SIZE);
- if (!scu_base) {
- pr_err("failed to remap config base (%lu/%u) for SCU\n",
- config_base, CORTEX_A9_SCU_SIZE);
+ scu_base = of_scu_get_base();
+ if (IS_ERR(scu_base))
return -ENOMEM;
- }
scu_enable(scu_base);
diff --git a/arch/arm/mach-bcm/platsmp.c b/arch/arm/mach-bcm/platsmp.c
index 3ac3a9b..743599a 100644
--- a/arch/arm/mach-bcm/platsmp.c
+++ b/arch/arm/mach-bcm/platsmp.c
@@ -28,9 +28,6 @@
#include <asm/smp_plat.h>
#include <asm/smp_scu.h>
-/* Size of mapped Cortex A9 SCU address space */
-#define CORTEX_A9_SCU_SIZE 0x58
-
#define SECONDARY_TIMEOUT_NS NSEC_PER_MSEC /* 1 msec (in nanoseconds) */
#define BOOT_ADDR_CPUID_MASK 0x3
@@ -38,47 +35,6 @@
#define OF_SECONDARY_BOOT "secondary-boot-reg"
#define MPIDR_CPUID_BITMASK 0x3
-/*
- * Enable the Cortex A9 Snoop Control Unit
- *
- * By the time this is called we already know there are multiple
- * cores present. We assume we're running on a Cortex A9 processor,
- * so any trouble getting the base address register or getting the
- * SCU base is a problem.
- *
- * Return 0 if successful or an error code otherwise.
- */
-static int __init scu_a9_enable(void)
-{
- unsigned long config_base;
- void __iomem *scu_base;
-
- if (!scu_a9_has_base()) {
- pr_err("no configuration base address register!\n");
- return -ENXIO;
- }
-
- /* Config base address register value is zero for uniprocessor */
- config_base = scu_a9_get_base();
- if (!config_base) {
- pr_err("hardware reports only one core\n");
- return -ENOENT;
- }
-
- scu_base = ioremap((phys_addr_t)config_base, CORTEX_A9_SCU_SIZE);
- if (!scu_base) {
- pr_err("failed to remap config base (%lu/%u) for SCU\n",
- config_base, CORTEX_A9_SCU_SIZE);
- return -ENOMEM;
- }
-
- scu_enable(scu_base);
-
- iounmap(scu_base); /* That's the last we'll need of this */
-
- return 0;
-}
-
static u32 secondary_boot_addr_for(unsigned int cpu)
{
u32 secondary_boot_addr = 0;
@@ -134,7 +90,7 @@ static void __init bcm_smp_prepare_cpus(unsigned int max_cpus)
const cpumask_t only_cpu_0 = { CPU_BITS_CPU0 };
/* Enable the SCU on Cortex A9 based SoCs */
- if (scu_a9_enable()) {
+ if (of_scu_enable()) {
/* Update the CPU present map to reflect uniprocessor mode */
pr_warn("failed to enable A9 SCU - disabling SMP\n");
init_cpu_present(&only_cpu_0);
--
2.7.4
Now as we have of_scu_enable which takes care of mapping
scu base from DT, lets use it.
CC: Jisheng Zhang <[email protected]>
CC: Sebastian Hesselbarth <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
---
arch/arm/mach-berlin/platsmp.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-berlin/platsmp.c b/arch/arm/mach-berlin/platsmp.c
index 93f9068..25a6ca5 100644
--- a/arch/arm/mach-berlin/platsmp.c
+++ b/arch/arm/mach-berlin/platsmp.c
@@ -60,26 +60,21 @@ static int berlin_boot_secondary(unsigned int cpu, struct task_struct *idle)
static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
{
struct device_node *np;
- void __iomem *scu_base;
void __iomem *vectors_base;
- np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- scu_base = of_iomap(np, 0);
- of_node_put(np);
- if (!scu_base)
- return;
-
np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
cpu_ctrl = of_iomap(np, 0);
of_node_put(np);
if (!cpu_ctrl)
- goto unmap_scu;
+ return;
vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
if (!vectors_base)
- goto unmap_scu;
+ return;
+
+ if (of_scu_enable())
+ return;
- scu_enable(scu_base);
flush_cache_all();
/*
@@ -95,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
iounmap(vectors_base);
-unmap_scu:
- iounmap(scu_base);
}
#ifdef CONFIG_HOTPLUG_CPU
--
2.7.4
Le 13/11/2016 ? 21:02, Pankaj Dubey a ?crit :
> Now as we have of_scu_enable which takes care of mapping
> scu base from DT, lets use it.
>
> CC: Florian Fainelli <[email protected]>
> CC: Ray Jui <[email protected]>
> CC: Scott Branden <[email protected]>
> CC: [email protected]
> Signed-off-by: Pankaj Dubey <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Let me know if I need to pick this and submit via ARM SoC pull requests,
thanks!
--
Florian
Hi Pankaj,
On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
>
> CC: Krzysztof Kozlowski <[email protected]>
> CC: Jisheng Zhang <[email protected]>
> CC: Russell King <[email protected]>
> CC: Dinh Nguyen <[email protected]>
> CC: Patrice Chotard <[email protected]>
> CC: Linus Walleij <[email protected]>
> CC: Liviu Dudau <[email protected]>
> CC: Ray Jui <[email protected]>
> CC: Stephen Warren <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Shawn Guo <[email protected]>
> CC: Michal Simek <[email protected]>
> CC: Wei Xu <[email protected]>
> CC: Andrew Lunn <[email protected]>
> CC: Jun Nie <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Pankaj Dubey <[email protected]>
> ---
> arch/arm/include/asm/smp_scu.h | 4 +++
> arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>
> #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
> #else
> static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
> #endif
>
> #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
> */
> #include <linux/init.h>
> #include <linux/io.h>
> +#include <linux/of_address.h>
>
> #include <asm/smp_plat.h>
> #include <asm/smp_scu.h>
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> */
> flush_cache_all();
> }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);
could we check np before calling of_iomap()?
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);
For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
what does it mean, but it may confuse normal users. In current version,
berlin doesn't complain like this for non-ca9 SoCs
> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
ditto
> + return IOMEM_ERR_PTR(-ENOMEM);
> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);
> +}
> +
> #endif
>
> /*
On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
> Hi Pankaj,
>
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
>
> > Many platforms are duplicating code for enabling SCU, lets add
> > common code to enable SCU by parsing SCU device node so the duplication
> > in each platform can be avoided.
> >
> > CC: Krzysztof Kozlowski <[email protected]>
> > CC: Jisheng Zhang <[email protected]>
> > CC: Russell King <[email protected]>
> > CC: Dinh Nguyen <[email protected]>
> > CC: Patrice Chotard <[email protected]>
> > CC: Linus Walleij <[email protected]>
> > CC: Liviu Dudau <[email protected]>
> > CC: Ray Jui <[email protected]>
> > CC: Stephen Warren <[email protected]>
> > CC: Heiko Stuebner <[email protected]>
> > CC: Shawn Guo <[email protected]>
> > CC: Michal Simek <[email protected]>
> > CC: Wei Xu <[email protected]>
> > CC: Andrew Lunn <[email protected]>
> > CC: Jun Nie <[email protected]>
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Pankaj Dubey <[email protected]>
> > ---
> > arch/arm/include/asm/smp_scu.h | 4 +++
> > arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 60 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> > index bfe163c..fdeec07 100644
> > --- a/arch/arm/include/asm/smp_scu.h
> > +++ b/arch/arm/include/asm/smp_scu.h
> > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> >
> > #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> > void scu_enable(void __iomem *scu_base);
> > +void __iomem *of_scu_get_base(void);
> > +int of_scu_enable(void);
> > #else
> > static inline void scu_enable(void __iomem *scu_base) {}
> > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > +static inline int of_scu_enable(void) {return 0; }
> > #endif
> >
> > #endif
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 72f9241..d0ac3ed 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -10,6 +10,7 @@
> > */
> > #include <linux/init.h>
> > #include <linux/io.h>
> > +#include <linux/of_address.h>
> >
> > #include <asm/smp_plat.h>
> > #include <asm/smp_scu.h>
> > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> > */
> > flush_cache_all();
> > }
> > +
> > +static const struct of_device_id scu_match[] = {
> > + { .compatible = "arm,cortex-a9-scu", },
> > + { .compatible = "arm,cortex-a5-scu", },
> > + { }
> > +};
> > +
> > +/*
> > + * Helper API to get SCU base address
> > + * In case platform DT do not have SCU node, or iomap fails
> > + * this call will fallback and will try to map via call to
> > + * scu_a9_get_base.
> > + * This will return ownership of scu_base to the caller
> > + */
> > +void __iomem *of_scu_get_base(void)
> > +{
> > + unsigned long base = 0;
> > + struct device_node *np;
> > + void __iomem *scu_base;
> > +
> > + np = of_find_matching_node(NULL, scu_match);
>
> could we check np before calling of_iomap()?
>
> > + scu_base = of_iomap(np, 0);
> > + of_node_put(np);
> > + if (!scu_base) {
> > + pr_err("%s failed to map scu_base via DT\n", __func__);
>
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs
oops, I just realized that the non-ca9 berlin arm SoC version isn't upstreamed.
Below is the draft version I planed. Basically speaking, the code tries to
find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
worry about SCU. Is there any elegant solution for my situation?
Thanks,
Jisheng
------------8<-------------------
--- a/arch/arm/mach-berlin/platsmp.c
+++ b/arch/arm/mach-berlin/platsmp.c
@@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
void __iomem *vectors_base;
np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
- scu_base = of_iomap(np, 0);
- of_node_put(np);
- if (!scu_base)
- return;
+ if (np) {
+ scu_base = of_iomap(np, 0);
+ of_node_put(np);
+ if (!scu_base)
+ return;
+ scu_enable(scu_base);
+ iounmap(scu_base);
+ }
np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
cpu_ctrl = of_iomap(np, 0);
of_node_put(np);
if (!cpu_ctrl)
- goto unmap_scu;
+ return;
vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
if (!vectors_base)
- goto unmap_scu;
+ return;
- scu_enable(scu_base);
flush_cache_all();
/*
@@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
iounmap(vectors_base);
-unmap_scu:
- iounmap(scu_base);
}
static struct smp_operations berlin_smp_ops __initdata = {
>
> > + if (scu_a9_has_base()) {
> > + base = scu_a9_get_base();
> > + scu_base = ioremap(base, SZ_4K);
> > + }
> > + if (!scu_base) {
> > + pr_err("%s failed to map scu_base\n", __func__);
>
> ditto
>
> > + return IOMEM_ERR_PTR(-ENOMEM);
> > + }
> > + }
> > + return scu_base;
> > +}
> > +
> > +/*
> > + * Enable SCU via mapping scu_base DT
> > + * If scu_base mapped successfully scu will be enabled and in case of
> > + * failure if will return non-zero error code
> > + */
> > +int of_scu_enable(void)
> > +{
> > + void __iomem *scu_base;
> > +
> > + scu_base = of_scu_get_base();
> > + if (!IS_ERR(scu_base)) {
> > + scu_enable(scu_base);
> > + iounmap(scu_base);
> > + return 0;
> > + }
> > + return PTR_ERR(scu_base);
> > +}
> > +
> > #endif
> >
> > /*
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Jisheng,
On Monday 14 November 2016 11:42 AM, Jisheng Zhang wrote:
> Hi Pankaj,
>
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
<snip>
>> +
>> + np = of_find_matching_node(NULL, scu_match);
>
> could we check np before calling of_iomap()?
>
of_iomap takes care of that, and will return NULL if np is NULL.
So additional check of np is not required here.
>> + scu_base = of_iomap(np, 0);
>> + of_node_put(np);
>> + if (!scu_base) {
>> + pr_err("%s failed to map scu_base via DT\n", __func__);
>
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs
>
OK, let me see other reviewer's comment on this. Then we will decide if
this error message is required or can be omitted.
Thanks,
Pankaj Dubey
Hi Jisheng,
On Monday 14 November 2016 12:24 PM, Jisheng Zhang wrote:
>
> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
>
>> Hi Pankaj,
>>
<snip>
>>> + * Helper API to get SCU base address
>>> + * In case platform DT do not have SCU node, or iomap fails
>>> + * this call will fallback and will try to map via call to
>>> + * scu_a9_get_base.
>>> + * This will return ownership of scu_base to the caller
>>> + */
>>> +void __iomem *of_scu_get_base(void)
>>> +{
>>> + unsigned long base = 0;
>>> + struct device_node *np;
>>> + void __iomem *scu_base;
>>> +
>>> + np = of_find_matching_node(NULL, scu_match);
>>
>> could we check np before calling of_iomap()?
>>
>>> + scu_base = of_iomap(np, 0);
>>> + of_node_put(np);
>>> + if (!scu_base) {
>>> + pr_err("%s failed to map scu_base via DT\n", __func__);
>>
>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>> what does it mean, but it may confuse normal users. In current version,
>> berlin doesn't complain like this for non-ca9 SoCs
>
> oops, I just realized that the non-ca9 berlin arm SoC version isn't upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?
>
To adopt new generic API I have submitted a patch for Berlin (along with
other various platforms) here:
https://patchwork.kernel.org/patch/9426457/
Please review and if possible test and let me know feedback.
Thanks,
Pankaj Dubey
> Thanks,
> Jisheng
>
>
> ------------8<-------------------
> --- a/arch/arm/mach-berlin/platsmp.c
> +++ b/arch/arm/mach-berlin/platsmp.c
> @@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
> void __iomem *vectors_base;
>
> np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> - scu_base = of_iomap(np, 0);
> - of_node_put(np);
> - if (!scu_base)
> - return;
> + if (np) {
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base)
> + return;
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + }
>
> np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
> cpu_ctrl = of_iomap(np, 0);
> of_node_put(np);
> if (!cpu_ctrl)
> - goto unmap_scu;
> + return;
>
> vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
> if (!vectors_base)
> - goto unmap_scu;
> + return;
>
> - scu_enable(scu_base);
> flush_cache_all();
>
> /*
> @@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
> writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
>
> iounmap(vectors_base);
> -unmap_scu:
> - iounmap(scu_base);
> }
>
> static struct smp_operations berlin_smp_ops __initdata = {
>
>
>>
>>> + if (scu_a9_has_base()) {
>>> + base = scu_a9_get_base();
>>> + scu_base = ioremap(base, SZ_4K);
>>> + }
>>> + if (!scu_base) {
>>> + pr_err("%s failed to map scu_base\n", __func__);
>>
>> ditto
>>
>>> + return IOMEM_ERR_PTR(-ENOMEM);
>>> + }
>>> + }
>>> + return scu_base;
>>> +}
>>> +
>>> +/*
>>> + * Enable SCU via mapping scu_base DT
>>> + * If scu_base mapped successfully scu will be enabled and in case of
>>> + * failure if will return non-zero error code
>>> + */
>>> +int of_scu_enable(void)
>>> +{
>>> + void __iomem *scu_base;
>>> +
>>> + scu_base = of_scu_get_base();
>>> + if (!IS_ERR(scu_base)) {
>>> + scu_enable(scu_base);
>>> + iounmap(scu_base);
>>> + return 0;
>>> + }
>>> + return PTR_ERR(scu_base);
>>> +}
>>> +
>>> #endif
>>>
>>> /*
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
>
Hi Pankaj,
On Mon, 14 Nov 2016 14:54:59 +0800 Jisheng Zhang wrote:
> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
>
> > Hi Pankaj,
> >
> > On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> >
> > > Many platforms are duplicating code for enabling SCU, lets add
> > > common code to enable SCU by parsing SCU device node so the duplication
> > > in each platform can be avoided.
> > >
> > > CC: Krzysztof Kozlowski <[email protected]>
> > > CC: Jisheng Zhang <[email protected]>
> > > CC: Russell King <[email protected]>
> > > CC: Dinh Nguyen <[email protected]>
> > > CC: Patrice Chotard <[email protected]>
> > > CC: Linus Walleij <[email protected]>
> > > CC: Liviu Dudau <[email protected]>
> > > CC: Ray Jui <[email protected]>
> > > CC: Stephen Warren <[email protected]>
> > > CC: Heiko Stuebner <[email protected]>
> > > CC: Shawn Guo <[email protected]>
> > > CC: Michal Simek <[email protected]>
> > > CC: Wei Xu <[email protected]>
> > > CC: Andrew Lunn <[email protected]>
> > > CC: Jun Nie <[email protected]>
> > > Suggested-by: Arnd Bergmann <[email protected]>
> > > Signed-off-by: Pankaj Dubey <[email protected]>
> > > ---
> > > arch/arm/include/asm/smp_scu.h | 4 +++
> > > arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 60 insertions(+)
> > >
> > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> > > index bfe163c..fdeec07 100644
> > > --- a/arch/arm/include/asm/smp_scu.h
> > > +++ b/arch/arm/include/asm/smp_scu.h
> > > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > >
> > > #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> > > void scu_enable(void __iomem *scu_base);
> > > +void __iomem *of_scu_get_base(void);
> > > +int of_scu_enable(void);
> > > #else
> > > static inline void scu_enable(void __iomem *scu_base) {}
> > > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > > +static inline int of_scu_enable(void) {return 0; }
> > > #endif
> > >
> > > #endif
> > > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > > index 72f9241..d0ac3ed 100644
> > > --- a/arch/arm/kernel/smp_scu.c
> > > +++ b/arch/arm/kernel/smp_scu.c
> > > @@ -10,6 +10,7 @@
> > > */
> > > #include <linux/init.h>
> > > #include <linux/io.h>
> > > +#include <linux/of_address.h>
> > >
> > > #include <asm/smp_plat.h>
> > > #include <asm/smp_scu.h>
> > > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> > > */
> > > flush_cache_all();
> > > }
> > > +
> > > +static const struct of_device_id scu_match[] = {
> > > + { .compatible = "arm,cortex-a9-scu", },
> > > + { .compatible = "arm,cortex-a5-scu", },
> > > + { }
> > > +};
> > > +
> > > +/*
> > > + * Helper API to get SCU base address
> > > + * In case platform DT do not have SCU node, or iomap fails
> > > + * this call will fallback and will try to map via call to
> > > + * scu_a9_get_base.
> > > + * This will return ownership of scu_base to the caller
> > > + */
> > > +void __iomem *of_scu_get_base(void)
> > > +{
> > > + unsigned long base = 0;
> > > + struct device_node *np;
> > > + void __iomem *scu_base;
> > > +
> > > + np = of_find_matching_node(NULL, scu_match);
> >
> > could we check np before calling of_iomap()?
> >
> > > + scu_base = of_iomap(np, 0);
> > > + of_node_put(np);
> > > + if (!scu_base) {
> > > + pr_err("%s failed to map scu_base via DT\n", __func__);
> >
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs
>
> oops, I just realized that the non-ca9 berlin arm SoC version isn't upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?
I just realized that (another realized :D) we uses PSCI enable method for
non-ca9 base SoCs, so "marvell,berlin-smp" enable method is only for the SoCs
which have CA9 compatible SCU. Sorry for the noise, your patch looks good to me.
Hi Pankaj,
On Mon, 14 Nov 2016 10:31:58 +0530 Pankaj Dubey wrote:
> Now as we have of_scu_enable which takes care of mapping
> scu base from DT, lets use it.
>
> CC: Jisheng Zhang <[email protected]>
> CC: Sebastian Hesselbarth <[email protected]>
> Signed-off-by: Pankaj Dubey <[email protected]>
> ---
> arch/arm/mach-berlin/platsmp.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-berlin/platsmp.c b/arch/arm/mach-berlin/platsmp.c
> index 93f9068..25a6ca5 100644
> --- a/arch/arm/mach-berlin/platsmp.c
> +++ b/arch/arm/mach-berlin/platsmp.c
> @@ -60,26 +60,21 @@ static int berlin_boot_secondary(unsigned int cpu, struct task_struct *idle)
> static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
> {
> struct device_node *np;
> - void __iomem *scu_base;
> void __iomem *vectors_base;
>
> - np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> - scu_base = of_iomap(np, 0);
> - of_node_put(np);
> - if (!scu_base)
> - return;
> -
> np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
> cpu_ctrl = of_iomap(np, 0);
> of_node_put(np);
> if (!cpu_ctrl)
> - goto unmap_scu;
> + return;
>
> vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
> if (!vectors_base)
> - goto unmap_scu;
> + return;
> +
> + if (of_scu_enable())
In err code path, we need to unmap vectors_base before return
> + return;
>
> - scu_enable(scu_base);
> flush_cache_all();
>
> /*
> @@ -95,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
> writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
>
> iounmap(vectors_base);
> -unmap_scu:
> - iounmap(scu_base);
> }
>
> #ifdef CONFIG_HOTPLUG_CPU
On Monday, November 14, 2016 10:31:59 AM CET Pankaj Dubey wrote:
> static const struct of_device_id realview_scu_match[] = {
> { .compatible = "arm,arm11mp-scu", },
> - { .compatible = "arm,cortex-a9-scu", },
> - { .compatible = "arm,cortex-a5-scu", },
> { }
> };
>
> @@ -41,27 +39,18 @@ static void __init realview_smp_prepare_cpus(unsigned int max_cpus)
> struct device_node *np;
> void __iomem *scu_base;
> struct regmap *map;
> - unsigned int ncores;
> int i;
>
> - np = of_find_matching_node(NULL, realview_scu_match);
> - if (!np) {
> - pr_err("PLATSMP: No SCU base address\n");
> - return;
> + if (of_scu_enable()) {
> + np = of_find_matching_node(NULL, realview_scu_match);
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("PLATSMP: No SCU remap\n");
> + return;
> + }
> + scu_enable(scu_base);
> }
>
The only difference here seems to be that realview also needs to handle
"arm,arm11mp-scu". Why not move that into the generic implementation?
Arnd
On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> >> + scu_base = of_iomap(np, 0);
> >> + of_node_put(np);
> >> + if (!scu_base) {
> >> + pr_err("%s failed to map scu_base via DT\n", __func__);
> >
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs
> >
>
> OK, let me see other reviewer's comment on this. Then we will decide if
> this error message is required or can be omitted.
We need to look at all callers here, to see if the function ever gets
called for a CPU that doesn't have an SCU. I'd say we should warn if
we know there is an SCU but we cannot map it, but never warn on
any of the CPU cores that don't support an SCU.
Arnd
On Monday 14 November 2016 05:26 PM, Arnd Bergmann wrote:
> On Monday, November 14, 2016 10:31:59 AM CET Pankaj Dubey wrote:
>> static const struct of_device_id realview_scu_match[] = {
>> { .compatible = "arm,arm11mp-scu", },
>> - { .compatible = "arm,cortex-a9-scu", },
>> - { .compatible = "arm,cortex-a5-scu", },
>> { }
>> };
>>
>> @@ -41,27 +39,18 @@ static void __init realview_smp_prepare_cpus(unsigned int max_cpus)
>> struct device_node *np;
>> void __iomem *scu_base;
>> struct regmap *map;
>> - unsigned int ncores;
>> int i;
>>
>> - np = of_find_matching_node(NULL, realview_scu_match);
>> - if (!np) {
>> - pr_err("PLATSMP: No SCU base address\n");
>> - return;
>> + if (of_scu_enable()) {
>> + np = of_find_matching_node(NULL, realview_scu_match);
>> + scu_base = of_iomap(np, 0);
>> + of_node_put(np);
>> + if (!scu_base) {
>> + pr_err("PLATSMP: No SCU remap\n");
>> + return;
>> + }
>> + scu_enable(scu_base);
>> }
>>
>
> The only difference here seems to be that realview also needs to handle
> "arm,arm11mp-scu". Why not move that into the generic implementation?
>
Do you mean "arm,arm11mp-scu" this is generic SCU specific to ARM?
If it's generic then it can be moved on the same line I moved a5-scu and
a9-scu.
I left it here in same file, as I understood it might be related with
very specific to realview platform.
Thanks,
Pankaj Dubey
> Arnd
>
>
>
Hi Arnd,
On 14 November 2016 at 17:26, Arnd Bergmann <[email protected]> wrote:
> On Monday, November 14, 2016 10:31:59 AM CET Pankaj Dubey wrote:
>> static const struct of_device_id realview_scu_match[] = {
>> { .compatible = "arm,arm11mp-scu", },
>> - { .compatible = "arm,cortex-a9-scu", },
>> - { .compatible = "arm,cortex-a5-scu", },
>> { }
>> };
>>
>> @@ -41,27 +39,18 @@ static void __init realview_smp_prepare_cpus(unsigned int max_cpus)
>> struct device_node *np;
>> void __iomem *scu_base;
>> struct regmap *map;
>> - unsigned int ncores;
>> int i;
>>
>> - np = of_find_matching_node(NULL, realview_scu_match);
>> - if (!np) {
>> - pr_err("PLATSMP: No SCU base address\n");
>> - return;
>> + if (of_scu_enable()) {
>> + np = of_find_matching_node(NULL, realview_scu_match);
>> + scu_base = of_iomap(np, 0);
>> + of_node_put(np);
>> + if (!scu_base) {
>> + pr_err("PLATSMP: No SCU remap\n");
>> + return;
>> + }
>> + scu_enable(scu_base);
>> }
>>
>
> The only difference here seems to be that realview also needs to handle
> "arm,arm11mp-scu". Why not move that into the generic implementation?
>
Just checked scu binding documentation for "arm,arm11mp-scu" and came
to know its for ARM11 MPCore based SoC's SCU. So as you said, it can
surely be moved to genenric imeplemenation. Will do this change and
resubmit again.
Thanks,
Pankaj Dubey
> Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This should be sent _to_ me because it's touching generic ARM code.
Thanks.
On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
>
> CC: Krzysztof Kozlowski <[email protected]>
> CC: Jisheng Zhang <[email protected]>
> CC: Russell King <[email protected]>
> CC: Dinh Nguyen <[email protected]>
> CC: Patrice Chotard <[email protected]>
> CC: Linus Walleij <[email protected]>
> CC: Liviu Dudau <[email protected]>
> CC: Ray Jui <[email protected]>
> CC: Stephen Warren <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Shawn Guo <[email protected]>
> CC: Michal Simek <[email protected]>
> CC: Wei Xu <[email protected]>
> CC: Andrew Lunn <[email protected]>
> CC: Jun Nie <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Pankaj Dubey <[email protected]>
> ---
> arch/arm/include/asm/smp_scu.h | 4 +++
> arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>
> #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
> #else
> static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
> #endif
>
> #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
> */
> #include <linux/init.h>
> #include <linux/io.h>
> +#include <linux/of_address.h>
>
> #include <asm/smp_plat.h>
> #include <asm/smp_scu.h>
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> */
> flush_cache_all();
> }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);
> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
> + return IOMEM_ERR_PTR(-ENOMEM);
I can't see the point of using error-pointers here - it's not like we
really know why we're failing, so just return NULL.
> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);
and just return your -ENOMEM here.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > >> + scu_base = of_iomap(np, 0);
> > >> + of_node_put(np);
> > >> + if (!scu_base) {
> > >> + pr_err("%s failed to map scu_base via DT\n", __func__);
> > >
> > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > what does it mean, but it may confuse normal users. In current version,
> > > berlin doesn't complain like this for non-ca9 SoCs
> > >
> >
> > OK, let me see other reviewer's comment on this. Then we will decide if
> > this error message is required or can be omitted.
>
> We need to look at all callers here, to see if the function ever gets
> called for a CPU that doesn't have an SCU. I'd say we should warn if
> we know there is an SCU but we cannot map it, but never warn on
> any of the CPU cores that don't support an SCU.
Maybe there should be two helpers:
of_scu_enable() which _only_ looks up the SCU address in DT and enables
it if it finds it, otherwise returning failure.
a9_scu_enable() which tries to use the A9 provided SCU address and
enables it if it finds it, otherwise returning failure.
Then callers can decide which of these to call, and what error messages
to print on their failures.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Mon, Nov 14, 2016 at 10:32:07AM +0530, Pankaj Dubey wrote:
> Now as we have of_scu_enable which takes care of mapping
> scu base from DT, lets use it.
>
> At the same time this patch cleans up mach-imx platform files by
> removing static mapping of SCU and dropping imx_scu_map_io function.
I remember that the static mapping of SCU is necessary because SCU is
being accessed at very early boot stage where dynamic mapping hasn't
been set up.
> CC: Shawn Guo <[email protected]>
> CC: Sascha Hauer <[email protected]>
> Signed-off-by: Pankaj Dubey <[email protected]>
> ---
> arch/arm/mach-imx/common.h | 5 -----
> arch/arm/mach-imx/mach-imx6q.c | 8 +-------
> arch/arm/mach-imx/platsmp.c | 32 +++++---------------------------
> arch/arm/mach-imx/pm-imx6.c | 3 ++-
> 4 files changed, 8 insertions(+), 40 deletions(-)
I tested it and saw that the booting of imx6q is broken like below.
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.9.0-rc5-00002-g3e5aac418b91 (r65073@dragon) (gcc version 4.9.3 20150413 (prerelease) (Linaro GCC 4.9-2015.04-1) ) #5 SMP Mon Nov 14 22:17:36 CST 2016
[ 0.000000] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
[ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[ 0.000000] OF: fdt:Machine model: Freescale i.MX6 Quad SABRE Smart Device Board
[ 0.000000] earlycon: ec_imx21 at MMIO 0x02020000 (options '')
[ 0.000000] bootconsole [ec_imx21] enabled
[ 0.000000] cma: Reserved 16 MiB at 0x4f000000
[ 0.000000] Memory policy: Data cache writealloc
[ 0.000000] Unable to handle kernel NULL pointer dereference at virtual address 00000004
[ 0.000000] pgd = c0004000
[ 0.000000] [00000004] *pgd=00000000
[ 0.000000] Internal error: Oops: 5 [#1] SMP ARM
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.9.0-rc5-00002-g3e5aac418b91 #5
[ 0.000000] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 0.000000] task: c0e086c0 task.stack: c0e00000
[ 0.000000] PC is at scu_get_core_count+0xc/0x1c
[ 0.000000] LR is at imx_smp_init_cpus+0x20/0x4c
[ 0.000000] pc : [<c0d05438>] lr : [<c0d0df2c>] psr: 000000d3
[ 0.000000] sp : c0e01f10 ip : c0e01f20 fp : c0e01f1c
[ 0.000000] r10: c0bed850 r9 : c0e051c0 r8 : c0e75140
[ 0.000000] r7 : c164c5f0 r6 : c0e09f08 r5 : c0d5d6fc r4 : c0e08340
[ 0.000000] r3 : c0e755f8 r2 : 00000000 r1 : c0120d3c r0 : 00000000
[ 0.000000] Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
[ 0.000000] Control: 10c5387d Table: 1000404a DAC: 00000051
[ 0.000000] Process swapper (pid: 0, stack limit = 0xc0e00210)
[ 0.000000] Stack: (0xc0e01f10 to 0xc0e02000)
[ 0.000000] 1f00: c0e01f34 c0e01f20 c0d0df2c c0d05438
[ 0.000000] 1f20: c0e08340 c0d5d6fc c0e01f44 c0e01f38 c0d052b8 c0d0df18 c0e01fac c0e01f48
[ 0.000000] 1f40: c0d0457c c0d052a4 ffffffff 10c5387d c0e050c0 1000406a 4fffffff efffeec0
[ 0.000000] 1f60: c0e01f84 c0e01f70 c017b614 c017af04 c0bebc44 c0e01fa4 c0e01f9c c0e01f88
[ 0.000000] 1f80: c01cf6dc c0e75294 c0e050d8 c0d5fa44 c0e050c0 1000406a 412fc09a 00000000
[ 0.000000] 1fa0: c0e01ff4 c0e01fb0 c0d009b0 c0d03d24 00000000 00000000 00000000 00000000
[ 0.000000] 1fc0: 00000000 c0d5fa48 00000000 c0e75294 c0e050d8 c0d5fa44 c0e0a070 1000406a
[ 0.000000] 1fe0: 412fc09a 00000000 00000000 c0e01ff8 1000807c c0d0096c 00000000 00000000
[ 0.000000] Backtrace:
[ 0.000000] [<c0d0542c>] (scu_get_core_count) from [<c0d0df2c>] (imx_smp_init_cpus+0x20/0x4c)
[ 0.000000] [<c0d0df0c>] (imx_smp_init_cpus) from [<c0d052b8>] (smp_init_cpus+0x20/0x28)
[ 0.000000] r5:c0d5d6fc[ 0.000000] r4:c0e08340
[ 0.000000]
[ 0.000000] [<c0d05298>] (smp_init_cpus) from [<c0d0457c>] (setup_arch+0x864/0xc50)
[ 0.000000] [<c0d03d18>] (setup_arch) from [<c0d009b0>] (start_kernel+0x50/0x398)
[ 0.000000] r10:00000000[ 0.000000] r9:412fc09a
r8:1000406a[ 0.000000] r7:c0e050c0
r6:c0d5fa44[ 0.000000] r5:c0e050d8
[ 0.000000] r4:c0e75294[ 0.000000]
[ 0.000000] [<c0d00960>] (start_kernel) from [<1000807c>] (0x1000807c)
[ 0.000000] r10:00000000[ 0.000000] r9:412fc09a
r8:1000406a[ 0.000000] r7:c0e0a070
r6:c0d5fa44[ 0.000000] r5:c0e050d8
[ 0.000000] r4:c0e75294[ 0.000000]
[ 0.000000] Code: c0e75338 e1a0c00d e92dd800 e24cb004 (e5900004)
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
On Monday, November 14, 2016 5:36:43 PM CET pankaj.dubey wrote:
> On Monday 14 November 2016 05:26 PM, Arnd Bergmann wrote:
> > On Monday, November 14, 2016 10:31:59 AM CET Pankaj Dubey wrote:
> >
> > The only difference here seems to be that realview also needs to handle
> > "arm,arm11mp-scu". Why not move that into the generic implementation?
> >
>
> Do you mean "arm,arm11mp-scu" this is generic SCU specific to ARM?
Yes.
> If it's generic then it can be moved on the same line I moved a5-scu and
> a9-scu.
>
> I left it here in same file, as I understood it might be related with
> very specific to realview platform.
The reason why only realview has it is that this is currently the
only platform we support that uses SMP on an ARM11MPcore.
cns3xxx in is also SMP hardware, but we only support the first core.
ARnd
On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > >> + scu_base = of_iomap(np, 0);
> > > >> + of_node_put(np);
> > > >> + if (!scu_base) {
> > > >> + pr_err("%s failed to map scu_base via DT\n", __func__);
> > > >
> > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > > what does it mean, but it may confuse normal users. In current version,
> > > > berlin doesn't complain like this for non-ca9 SoCs
> > > >
> > >
> > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > this error message is required or can be omitted.
> >
> > We need to look at all callers here, to see if the function ever gets
> > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > we know there is an SCU but we cannot map it, but never warn on
> > any of the CPU cores that don't support an SCU.
>
> Maybe there should be two helpers:
>
> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> it if it finds it, otherwise returning failure.
>
> a9_scu_enable() which tries to use the A9 provided SCU address and
> enables it if it finds it, otherwise returning failure.
>
> Then callers can decide which of these to call, and what error messages
> to print on their failures.
Splitting the function in two is probably simpler overall, but
we may still have to look at all the callers: Any platform that
currently tries to map it on any CPU and doesn't warn about the
absence of the device node (or about scu_a9_has_base() == false)
should really continue not to warn about that.
If all platforms only call these on SMP machines with an
ARM11MPcore, Cortex-A5 or Cortex-A9, everything should be
fine here, otherwise we can leave the warning in the caller
after checking the return code of the new APIs.
Arnd
On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> > On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > > >> + scu_base = of_iomap(np, 0);
> > > > >> + of_node_put(np);
> > > > >> + if (!scu_base) {
> > > > >> + pr_err("%s failed to map scu_base via DT\n", __func__);
> > > > >
> > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > > > what does it mean, but it may confuse normal users. In current version,
> > > > > berlin doesn't complain like this for non-ca9 SoCs
> > > > >
> > > >
> > > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > > this error message is required or can be omitted.
> > >
> > > We need to look at all callers here, to see if the function ever gets
> > > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > > we know there is an SCU but we cannot map it, but never warn on
> > > any of the CPU cores that don't support an SCU.
> >
> > Maybe there should be two helpers:
> >
> > of_scu_enable() which _only_ looks up the SCU address in DT and enables
> > it if it finds it, otherwise returning failure.
> >
> > a9_scu_enable() which tries to use the A9 provided SCU address and
> > enables it if it finds it, otherwise returning failure.
> >
> > Then callers can decide which of these to call, and what error messages
> > to print on their failures.
>
> Splitting the function in two is probably simpler overall, but
> we may still have to look at all the callers: Any platform that
> currently tries to map it on any CPU and doesn't warn about the
> absence of the device node (or about scu_a9_has_base() == false)
> should really continue not to warn about that.
Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
should produce any warnings or errors to be printed. It's up to the
caller to report the failure, otherwise doing this doesn't make sense:
if (of_scu_enable() < 0 && a9_scu_enable() < 0)
pr_err("Failed to map and enable the SCU\n");
because if of_scu_enable() prints a warning/error, then it's patently
misleading.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
Hi Jisheng,
On 14 November 2016 at 14:21, Jisheng Zhang <[email protected]> wrote:
> Hi Pankaj,
>
> On Mon, 14 Nov 2016 10:31:58 +0530 Pankaj Dubey wrote:
>
>> Now as we have of_scu_enable which takes care of mapping
>> scu base from DT, lets use it.
>>
>> CC: Jisheng Zhang <[email protected]>
>> CC: Sebastian Hesselbarth <[email protected]>
>> Signed-off-by: Pankaj Dubey <[email protected]>
>> ---
>> arch/arm/mach-berlin/platsmp.c | 17 +++++------------
>> 1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-berlin/platsmp.c b/arch/arm/mach-berlin/platsmp.c
>> index 93f9068..25a6ca5 100644
>> --- a/arch/arm/mach-berlin/platsmp.c
>> +++ b/arch/arm/mach-berlin/platsmp.c
>> @@ -60,26 +60,21 @@ static int berlin_boot_secondary(unsigned int cpu, struct task_struct *idle)
>> static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
>> {
>> struct device_node *np;
>> - void __iomem *scu_base;
>> void __iomem *vectors_base;
>>
>> - np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
>> - scu_base = of_iomap(np, 0);
>> - of_node_put(np);
>> - if (!scu_base)
>> - return;
>> -
>> np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
>> cpu_ctrl = of_iomap(np, 0);
>> of_node_put(np);
>> if (!cpu_ctrl)
>> - goto unmap_scu;
>> + return;
>>
>> vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
>> if (!vectors_base)
>> - goto unmap_scu;
>> + return;
>> +
>> + if (of_scu_enable())
>
> In err code path, we need to unmap vectors_base before return
>
You are correct. I missed this, Will update in v2.
Thanks for review.
Pankaj Dubey
>> + return;
>>
>> - scu_enable(scu_base);
>> flush_cache_all();
>>
>> /*
>> @@ -95,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int max_cpus)
>> writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
>>
>> iounmap(vectors_base);
>> -unmap_scu:
>> - iounmap(scu_base);
>> }
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Nov 14, 2016 at 10:31:57AM +0530, Pankaj Dubey wrote:
> Now as we have of_scu_enable which takes care of mapping
> scu base from DT, lets use it.
>
> This patch also fixes build failure in case !SMP caused
> by commit SHA ID: 94210b1abb2 which is already merged in
> krzk/for-next branch
>
> CC: Krzysztof Kozlowski <[email protected]>
> CC: [email protected]
> Signed-off-by: Pankaj Dubey <[email protected]>
> ---
> arch/arm/mach-exynos/common.h | 1 -
> arch/arm/mach-exynos/platsmp.c | 30 ++++--------------------------
> arch/arm/mach-exynos/pm.c | 4 ++--
> arch/arm/mach-exynos/suspend.c | 14 ++++----------
> 4 files changed, 10 insertions(+), 39 deletions(-)
>
Looks correct, for reference:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
However this depends on changes in my next/soc branch (and these changes
were the trigger for this patchset). I can either provide a tag with
Exynos commits or accept one with common SCU code.
Best regards,
Krzysztof
On 14/11/16 05:02, Pankaj Dubey wrote:
> Now as we have of_scu_enable which takes care of mapping
> scu base from DT, lets use it.
>
> CC: Liviu Dudau <[email protected]>
> CC: Sudeep Holla <[email protected]>
I assume you will take this series through a single tree. Also I assume
you may make changes around a9 SCU. I will try to test if I can get my
setup back working. But if this patch is not changed, then
Acked-by: Sudeep Holla <[email protected]>
--
Regards,
Sudeep
Hi Sudeep,
On Wednesday 16 November 2016 08:04 PM, Sudeep Holla wrote:
>
>
> On 14/11/16 05:02, Pankaj Dubey wrote:
>> Now as we have of_scu_enable which takes care of mapping
>> scu base from DT, lets use it.
>>
>> CC: Liviu Dudau <[email protected]>
>> CC: Sudeep Holla <[email protected]>
>
> I assume you will take this series through a single tree. Also I assume
> you may make changes around a9 SCU. I will try to test if I can get my
> setup back working. But if this patch is not changed, then
>
> Acked-by: Sudeep Holla <[email protected]>
Thanks for review and Ack.
Yes, plan is to take this series through a single tree.
As there are few comments for other platforms and common scu file, I
will address them and submit v2 of this series soon. But I can see there
won't be any changes for vexpress platform patch in v2, so I will
include your Ack.
Thanks,
Pankaj Dubey
Hi Krzysztof,
On Wednesday 16 November 2016 12:29 AM, Krzysztof Kozlowski wrote:
> On Mon, Nov 14, 2016 at 10:31:57AM +0530, Pankaj Dubey wrote:
>> Now as we have of_scu_enable which takes care of mapping
>> scu base from DT, lets use it.
>>
>> This patch also fixes build failure in case !SMP caused
>> by commit SHA ID: 94210b1abb2 which is already merged in
>> krzk/for-next branch
>>
>> CC: Krzysztof Kozlowski <[email protected]>
>> CC: [email protected]
>> Signed-off-by: Pankaj Dubey <[email protected]>
>> ---
>> arch/arm/mach-exynos/common.h | 1 -
>> arch/arm/mach-exynos/platsmp.c | 30 ++++--------------------------
>> arch/arm/mach-exynos/pm.c | 4 ++--
>> arch/arm/mach-exynos/suspend.c | 14 ++++----------
>> 4 files changed, 10 insertions(+), 39 deletions(-)
>>
>
> Looks correct, for reference:
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>
Thanks for review.
> However this depends on changes in my next/soc branch (and these changes
> were the trigger for this patchset). I can either provide a tag with
> Exynos commits or accept one with common SCU code.
>
I am not sure through which tree this series need to be taken. I will
leave this to you and ARM-SOC maintainers.
As there are few comments on few patches of this series, I will address
them and soon post v2. Though I don't see any changes will be in v2 for
exynos platform. So I will include your review tag.
Thanks,
Pankaj Dubey
> Best regards,
> Krzysztof
>
>
>
Hi Russell,
On Monday 14 November 2016 07:18 PM, Russell King - ARM Linux wrote:
> This should be sent _to_ me because it's touching generic ARM code.
> Thanks.
>
Sorry for this.
I had included your email in CC for this patch, but looks like my email
client had some issue and this patch could not reach to your mailbox. I
will take care in future.
> On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
>> Many platforms are duplicating code for enabling SCU, lets add
>> common code to enable SCU by parsing SCU device node so the duplication
>> in each platform can be avoided.
>>
>> CC: Krzysztof Kozlowski <[email protected]>
>> CC: Jisheng Zhang <[email protected]>
>> CC: Russell King <[email protected]>
>> CC: Dinh Nguyen <[email protected]>
>> CC: Patrice Chotard <[email protected]>
>> CC: Linus Walleij <[email protected]>
>> CC: Liviu Dudau <[email protected]>
>> CC: Ray Jui <[email protected]>
>> CC: Stephen Warren <[email protected]>
>> CC: Heiko Stuebner <[email protected]>
>> CC: Shawn Guo <[email protected]>
>> CC: Michal Simek <[email protected]>
>> CC: Wei Xu <[email protected]>
>> CC: Andrew Lunn <[email protected]>
>> CC: Jun Nie <[email protected]>
>> Suggested-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Pankaj Dubey <[email protected]>
>> ---
>> arch/arm/include/asm/smp_scu.h | 4 +++
>> arch/arm/kernel/smp_scu.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
>> index bfe163c..fdeec07 100644
>> --- a/arch/arm/include/asm/smp_scu.h
>> +++ b/arch/arm/include/asm/smp_scu.h
>> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>>
>> #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>> void scu_enable(void __iomem *scu_base);
>> +void __iomem *of_scu_get_base(void);
>> +int of_scu_enable(void);
>> #else
>> static inline void scu_enable(void __iomem *scu_base) {}
>> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
>> +static inline int of_scu_enable(void) {return 0; }
>> #endif
>>
>> #endif
>> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
>> index 72f9241..d0ac3ed 100644
>> --- a/arch/arm/kernel/smp_scu.c
>> +++ b/arch/arm/kernel/smp_scu.c
>> @@ -10,6 +10,7 @@
>> */
>> #include <linux/init.h>
>> #include <linux/io.h>
>> +#include <linux/of_address.h>
>>
>> #include <asm/smp_plat.h>
>> #include <asm/smp_scu.h>
>> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>> */
>> flush_cache_all();
>> }
>> +
>> +static const struct of_device_id scu_match[] = {
>> + { .compatible = "arm,cortex-a9-scu", },
>> + { .compatible = "arm,cortex-a5-scu", },
>> + { }
>> +};
>> +
>> +/*
>> + * Helper API to get SCU base address
>> + * In case platform DT do not have SCU node, or iomap fails
>> + * this call will fallback and will try to map via call to
>> + * scu_a9_get_base.
>> + * This will return ownership of scu_base to the caller
>> + */
>> +void __iomem *of_scu_get_base(void)
>> +{
>> + unsigned long base = 0;
>> + struct device_node *np;
>> + void __iomem *scu_base;
>> +
>> + np = of_find_matching_node(NULL, scu_match);
>> + scu_base = of_iomap(np, 0);
>> + of_node_put(np);
>> + if (!scu_base) {
>> + pr_err("%s failed to map scu_base via DT\n", __func__);
>> + if (scu_a9_has_base()) {
>> + base = scu_a9_get_base();
>> + scu_base = ioremap(base, SZ_4K);
>> + }
>> + if (!scu_base) {
>> + pr_err("%s failed to map scu_base\n", __func__);
>> + return IOMEM_ERR_PTR(-ENOMEM);
>
> I can't see the point of using error-pointers here - it's not like we
> really know why we're failing, so just return NULL.
>
>> + }
>> + }
>> + return scu_base;
>> +}
>> +
>> +/*
>> + * Enable SCU via mapping scu_base DT
>> + * If scu_base mapped successfully scu will be enabled and in case of
>> + * failure if will return non-zero error code
>> + */
>> +int of_scu_enable(void)
>> +{
>> + void __iomem *scu_base;
>> +
>> + scu_base = of_scu_get_base();
>> + if (!IS_ERR(scu_base)) {
>> + scu_enable(scu_base);
>> + iounmap(scu_base);
>> + return 0;
>> + }
>> + return PTR_ERR(scu_base);
>
> and just return your -ENOMEM here.
>
Hi Shawn,
On Monday 14 November 2016 07:56 PM, Shawn Guo wrote:
> On Mon, Nov 14, 2016 at 10:32:07AM +0530, Pankaj Dubey wrote:
>> Now as we have of_scu_enable which takes care of mapping
>> scu base from DT, lets use it.
>>
>> At the same time this patch cleans up mach-imx platform files by
>> removing static mapping of SCU and dropping imx_scu_map_io function.
>
> I remember that the static mapping of SCU is necessary because SCU is
> being accessed at very early boot stage where dynamic mapping hasn't
> been set up.
>
>> CC: Shawn Guo <[email protected]>
>> CC: Sascha Hauer <[email protected]>
>> Signed-off-by: Pankaj Dubey <[email protected]>
>> ---
>> arch/arm/mach-imx/common.h | 5 -----
>> arch/arm/mach-imx/mach-imx6q.c | 8 +-------
>> arch/arm/mach-imx/platsmp.c | 32 +++++---------------------------
>> arch/arm/mach-imx/pm-imx6.c | 3 ++-
>> 4 files changed, 8 insertions(+), 40 deletions(-)
>
> I tested it and saw that the booting of imx6q is broken like below.
Thanks for testing and letting me know about this.
Currently only two platforms (IMX and ZYNQ) are using SCU address at
very early stage of boot, for configuring possible cpus via
set_cpu_possible().. rest platforms are either using DT method or they
handle this in smp_prepare_cpus.
Since I am not sure if all boards based on IMX has been moved to DT
based boot, if it has moved to completely DT based then we do not need
this set_cpu_possible in smp_init_cpus, it will be taken care via
"arm_dt_init_cpu_maps" and this will avoid need of early mapping of SCU
base as well.
If not then, currently I can't see any other way to handle this and in
that case I will drop-out IMX platform's patch of using generic SCU APIs
in v2 version of series.
Thanks,
Pankaj Dubey
Hi Russell,
On Monday 14 November 2016 08:21 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
>> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
>>> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
>>>> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
>>>>>>> + scu_base = of_iomap(np, 0);
>>>>>>> + of_node_put(np);
>>>>>>> + if (!scu_base) {
>>>>>>> + pr_err("%s failed to map scu_base via DT\n", __func__);
>>>>>>
>>>>>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>>>>>> what does it mean, but it may confuse normal users. In current version,
>>>>>> berlin doesn't complain like this for non-ca9 SoCs
>>>>>>
>>>>>
>>>>> OK, let me see other reviewer's comment on this. Then we will decide if
>>>>> this error message is required or can be omitted.
>>>>
>>>> We need to look at all callers here, to see if the function ever gets
>>>> called for a CPU that doesn't have an SCU. I'd say we should warn if
>>>> we know there is an SCU but we cannot map it, but never warn on
>>>> any of the CPU cores that don't support an SCU.
>>>
>>> Maybe there should be two helpers:
>>>
>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
>>> it if it finds it, otherwise returning failure.
>>>
>>> a9_scu_enable() which tries to use the A9 provided SCU address and
>>> enables it if it finds it, otherwise returning failure.
>>>
OK, In that case I can see need for following four helpers as:
1: of_scu_enable() which will __only__ lookup the SCU address in DT and
enables it if it finds, otherwise return -ENOMEM failure.
This helper APIs is required and sufficient for most of platforms such
as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
mvebu
2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
enables it, if address mapped successfully, otherwise returning failure.
This helper APIs is required and sufficient for two ARM platforms as of
now tegra and hisi.
3: of_scu_get_base() which will lookup the SCU address in DT and if node
found maps address and returns ioremapped address to caller.
This helper APIs is required for three ARM plaforms rockchip, mvebu and
ux500, along with scu_enable() API to enable and find number_of_cores.
4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
do ioremap of scu address and returns ioremapped address to the caller
along with ownership (caller has responsibility to unmap it).
This helper APIs is required to simplify SCU enable and related code in
two ARM plaforms BCM ans ZX.
For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
are useful for the time-being, as they need SCU mapping very early of
boot, where we can't use iomap APIs. So I will drop patches related to
these platforms in v2 version.
Please let me know if any concern in this approach.
>>> Then callers can decide which of these to call, and what error messages
>>> to print on their failures.
>>
>> Splitting the function in two is probably simpler overall, but
>> we may still have to look at all the callers: Any platform that
>> currently tries to map it on any CPU and doesn't warn about the
>> absence of the device node (or about scu_a9_has_base() == false)
>> should really continue not to warn about that.
>
> Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> should produce any warnings or errors to be printed. It's up to the
> caller to report the failure, otherwise doing this doesn't make sense:
>
> if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> pr_err("Failed to map and enable the SCU\n");
>
> because if of_scu_enable() prints a warning/error, then it's patently
> misleading.
>
I will move out error message out of these helpers and let caller
(platform specific code) handle and print error if required.
Thanks,
Pankaj Dubey
On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
>
> >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> >>> it if it finds it, otherwise returning failure.
> >>>
> >>> a9_scu_enable() which tries to use the A9 provided SCU address and
> >>> enables it if it finds it, otherwise returning failure.
> >>>
>
> OK, In that case I can see need for following four helpers as:
>
> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
> enables it if it finds, otherwise return -ENOMEM failure.
> This helper APIs is required and sufficient for most of platforms such
> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
> mvebu
>
> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
> enables it, if address mapped successfully, otherwise returning failure.
> This helper APIs is required and sufficient for two ARM platforms as of
> now tegra and hisi.
>
> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
> found maps address and returns ioremapped address to caller.
> This helper APIs is required for three ARM plaforms rockchip, mvebu and
> ux500, along with scu_enable() API to enable and find number_of_cores.
>
> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
> do ioremap of scu address and returns ioremapped address to the caller
> along with ownership (caller has responsibility to unmap it).
> This helper APIs is required to simplify SCU enable and related code in
> two ARM plaforms BCM ans ZX.
>
> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
> are useful for the time-being, as they need SCU mapping very early of
> boot, where we can't use iomap APIs. So I will drop patches related to
> these platforms in v2 version.
>
> Please let me know if any concern in this approach.
I think ideally we wouldn't even need to know the virtual address
outside of smp_scu.c. If we can move all users of the address
into that file directly, it could become a local variable and
we change scu_power_mode() and scu_get_core_count() instead to
not require the address argument.
The only user I could find outside of that file is
static int shmobile_smp_scu_psr_core_disabled(int cpu)
{
unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);
if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
return 1;
return 0;
}
which can be done in the same file as well.
> >>> Then callers can decide which of these to call, and what error messages
> >>> to print on their failures.
> >>
> >> Splitting the function in two is probably simpler overall, but
> >> we may still have to look at all the callers: Any platform that
> >> currently tries to map it on any CPU and doesn't warn about the
> >> absence of the device node (or about scu_a9_has_base() == false)
> >> should really continue not to warn about that.
> >
> > Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> > should produce any warnings or errors to be printed. It's up to the
> > caller to report the failure, otherwise doing this doesn't make sense:
> >
> > if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> > pr_err("Failed to map and enable the SCU\n");
> >
> > because if of_scu_enable() prints a warning/error, then it's patently
> > misleading.
> >
That's why I said "otherwise we can leave the warning in the caller
after checking the return code of the new APIs." for the case where
we actually need it.
> I will move out error message out of these helpers and let caller
> (platform specific code) handle and print error if required.
Ok.
Arnd
On Thursday 17 November 2016 10:33 PM, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
>>
>>>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
>>>>> it if it finds it, otherwise returning failure.
>>>>>
>>>>> a9_scu_enable() which tries to use the A9 provided SCU address and
>>>>> enables it if it finds it, otherwise returning failure.
>>>>>
>>
>> OK, In that case I can see need for following four helpers as:
>>
>> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
>> enables it if it finds, otherwise return -ENOMEM failure.
>> This helper APIs is required and sufficient for most of platforms such
>> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
>> mvebu
>>
>> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
>> enables it, if address mapped successfully, otherwise returning failure.
>> This helper APIs is required and sufficient for two ARM platforms as of
>> now tegra and hisi.
>>
>> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
>> found maps address and returns ioremapped address to caller.
>> This helper APIs is required for three ARM plaforms rockchip, mvebu and
>> ux500, along with scu_enable() API to enable and find number_of_cores.
>>
>> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
>> do ioremap of scu address and returns ioremapped address to the caller
>> along with ownership (caller has responsibility to unmap it).
>> This helper APIs is required to simplify SCU enable and related code in
>> two ARM plaforms BCM ans ZX.
>>
>> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
>> are useful for the time-being, as they need SCU mapping very early of
>> boot, where we can't use iomap APIs. So I will drop patches related to
>> these platforms in v2 version.
>>
>> Please let me know if any concern in this approach.
>
> I think ideally we wouldn't even need to know the virtual address
> outside of smp_scu.c. If we can move all users of the address
> into that file directly, it could become a local variable and
> we change scu_power_mode() and scu_get_core_count() instead to
> not require the address argument.
>
If we change scu_get_core_count() without address argument, what about
those platforms which are calling this API very early boot (mostly in
smp_init_cpus), during this stage we can't use iomap. These platforms
are doing static mapping of SCU base, and passing virtual address.
Currently following are platforms which needs SCU virtual address at
very early stage mostly for calling scu_get_core_count(scu_address):
IMX, ZYNQ, SPEAr, and OMAP2.
I can think of handling of these platform's early need of SCU in the
following way:
Probably we need something like early_a9_scu_get_core_count() which can
be handled in this way in smp_scu.c file itself:
+static struct map_desc scu_map __initdata = {
+ .length = SZ_256,
+ .type = MT_DEVICE,
+};
+
+static void __iomem *early_scu_map_io(void)
+{
+ unsigned long base;
+ void __iomem *scu_base;
+
+ base = scu_a9_get_base();
+ scu_map.pfn = __phys_to_pfn(base);
+ scu_map.virtual = base;
+ iotable_init(&scu_map, 1);
+ scu_base = (void __iomem *)base;
+ return scu_base;
+}
+
+/*
+ * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
+ * Only platforms which needs number_of_cores during early boot should
call this
+ */
+unsigned int __init early_a9_scu_get_core_count(void)
+{
+ void __iomem *scu_base;
+
+ scu_base = early_scu_map_io();
+ return scu_get_core_count(scu_base);
+}
+
Please let me know how about using above approach to simplify platform
specific code of early users of SCU address.
Also for rest platforms, which are not using scu base at very early
stage, but are using virtual address which is mapped either from SCU
device node or using s9_scu_get_base() to map to SCU virtual address. To
separate these two we discussed to separate scu_enable in two helper
APIs as of_scu_enable and s9_scu_enable. So if we change
scu_get_core_count which do not requires address argument, this also
needs to be separated in two as of_scu_get_core_count and
s9_scu_get_core_count.
Thanks,
Pankaj Dubey
> The only user I could find outside of that file is
>
> static int shmobile_smp_scu_psr_core_disabled(int cpu)
> {
> unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);
>
> if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
> return 1;
>
> return 0;
> }
>
> which can be done in the same file as well.
>
>>>>> Then callers can decide which of these to call, and what error messages
>>>>> to print on their failures.
>>>>
>>>> Splitting the function in two is probably simpler overall, but
>>>> we may still have to look at all the callers: Any platform that
>>>> currently tries to map it on any CPU and doesn't warn about the
>>>> absence of the device node (or about scu_a9_has_base() == false)
>>>> should really continue not to warn about that.
>>>
>>> Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
>>> should produce any warnings or errors to be printed. It's up to the
>>> caller to report the failure, otherwise doing this doesn't make sense:
>>>
>>> if (of_scu_enable() < 0 && a9_scu_enable() < 0)
>>> pr_err("Failed to map and enable the SCU\n");
>>>
>>> because if of_scu_enable() prints a warning/error, then it's patently
>>> misleading.
>>>
>
> That's why I said "otherwise we can leave the warning in the caller
> after checking the return code of the new APIs." for the case where
> we actually need it.
>
>> I will move out error message out of these helpers and let caller
>> (platform specific code) handle and print error if required.
>
> Ok.
>
> Arnd
>
>
>
On Friday, November 18, 2016 8:54:30 AM CET pankaj.dubey wrote:
> >> Please let me know if any concern in this approach.
> >
> > I think ideally we wouldn't even need to know the virtual address
> > outside of smp_scu.c. If we can move all users of the address
> > into that file directly, it could become a local variable and
> > we change scu_power_mode() and scu_get_core_count() instead to
> > not require the address argument.
> >
>
> If we change scu_get_core_count() without address argument, what about
> those platforms which are calling this API very early boot (mostly in
> smp_init_cpus), during this stage we can't use iomap. These platforms
> are doing static mapping of SCU base, and passing virtual address.
>
> Currently following are platforms which needs SCU virtual address at
> very early stage mostly for calling scu_get_core_count(scu_address):
> IMX, ZYNQ, SPEAr, and OMAP2.
Ah, you are right, I missed how this is done earlier than the
scu_enable() call.
> I can think of handling of these platform's early need of SCU in the
> following way:
>
> Probably we need something like early_a9_scu_get_core_count() which can
> be handled in this way in smp_scu.c file itself:
>
> +static struct map_desc scu_map __initdata = {
> + .length = SZ_256,
> + .type = MT_DEVICE,
> +};
> +
> +static void __iomem *early_scu_map_io(void)
> +{
> + unsigned long base;
> + void __iomem *scu_base;
> +
> + base = scu_a9_get_base();
> + scu_map.pfn = __phys_to_pfn(base);
> + scu_map.virtual = base;
> + iotable_init(&scu_map, 1);
> + scu_base = (void __iomem *)base;
> + return scu_base;
> +}
Unfortunately, this doesn't work because scu_map.virtual must be
picked by the platform code in a way that doesn't conflict
with anything else. Setting up the iotable is probably not
something we should move into the smp_scu.c file but leave where
it currently is. You copied the trick from zynq that happens
to work there but probably not on the other three.
At some point we decided that at least new platforms should
always get the core count from DT rather than from the SCU,
but I don't know if we have to keep supporting old DTB files
without a full description of the CPUs on any of the
four platforms.
I see that there are four other users of scu_get_core_count()
that all call it from ->prepare_cpus, and we can probably
just use num_possible_cpus() at that point as the count
must have been initialized from DT already.
> +/*
> + * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
> + * Only platforms which needs number_of_cores during early boot should
> call this
> + */
> +unsigned int __init early_a9_scu_get_core_count(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = early_scu_map_io();
> + return scu_get_core_count(scu_base);
> +}
> +
>
> Please let me know how about using above approach to simplify platform
> specific code of early users of SCU address.
>
> Also for rest platforms, which are not using scu base at very early
> stage, but are using virtual address which is mapped either from SCU
> device node or using s9_scu_get_base() to map to SCU virtual address. To
> separate these two we discussed to separate scu_enable in two helper
> APIs as of_scu_enable and s9_scu_enable. So if we change
> scu_get_core_count which do not requires address argument, this also
> needs to be separated in two as of_scu_get_core_count and
> s9_scu_get_core_count.
We can probably leave get_core_count code alone for now, or
we change it so that we pass a virtual address into it
from the platform and have the SCU mapped there. One nice
property of the early mapping is that the later ioremap()
will just return the same virtual address, so we don't get
a double mapping when calling the scu_enable variant later.
Adding two functions of each doesn't sound too great though,
it would make the API more complicated when the intention was
to make it simpler by leaving out the address argument.
Maybe something like this?
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 72f9241ad5db..c248a16e980f 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -24,6 +24,8 @@
#define SCU_INVALIDATE 0x0c
#define SCU_FPGA_REVISION 0x10
+static void __iomem *scu_base_addr;
+
#ifdef CONFIG_SMP
/*
* Get the number of CPU cores from the SCU configuration
@@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
{
u32 scu_ctrl;
+ if (scu_base)
+ scu_base = scu_base_addr;
+
#ifdef CONFIG_ARM_ERRATA_764369
/* Cortex-A9 only */
if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
@@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
unsigned int val;
int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
+ if (scu_base)
+ scu_base = scu_base_addr;
+
if (mode > 3 || mode == 1 || cpu > 3)
return -EINVAL;
@@ -94,3 +102,31 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
return 0;
}
+
+int __init scu_probe_a9(void)
+{
+ phys_addr_t base;
+
+ if (!scu_a9_has_base)
+ return -ENODEV;
+
+ base = scu_a9_get_base()
+ if (!base)
+ return -ENODEV;
+
+ scu_base_addr = ioremap(base, PAGE_SIZE);
+ if (scu_base_addr)
+ return -ENOMEM;
+
+ return scu_get_core_count(scu_base_addr);
+}
+
+int __init scu_probe_dt(void)
+{
+ ...
+ scu_base_addr = of_iomap(np, 0);
+ if (scu_base_addr)
+ return -ENOMEM;
+
+ return scu_get_core_count(scu_base_addr);
+}
Arnd
On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
> {
> u32 scu_ctrl;
>
> + if (scu_base)
> + scu_base = scu_base_addr;
> +
This looks to me like nonsense.
> #ifdef CONFIG_ARM_ERRATA_764369
> /* Cortex-A9 only */
> if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
> @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> unsigned int val;
> int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
>
> + if (scu_base)
> + scu_base = scu_base_addr;
> +
Ditto.
Rather than doing this, I'd much prefer to always store the SCU base in
the SCU code, and remove the "void __iomem *scu_base" argment from all
these functions.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
> > {
> > u32 scu_ctrl;
> >
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
>
> This looks to me like nonsense.
>
> > #ifdef CONFIG_ARM_ERRATA_764369
> > /* Cortex-A9 only */
> > if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
> > unsigned int val;
> > int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
> >
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
>
> Ditto.
>
> Rather than doing this, I'd much prefer to always store the SCU base in
> the SCU code, and remove the "void __iomem *scu_base" argment from all
> these functions.
Ok, then we just need one scu_probe_*() variant for each of the
four methods of initializing it (iotable, of_iomap,
ioremap(scu_a9_get_base) and hardcoded.
The intention of doing the fallback for the NULL argument was
to avoid having to add lots of new API while also allowing
the change to be done one platform at a time.
If we remove the argument from the other functions, they either
need to get a new name, or we change them all to the new prototype
at once. Either way works fine, do you have a preference between
them?
Arnd
On 18 November 2016 at 19:02, Arnd Bergmann <[email protected]> wrote:
> On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
>> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
>> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
>> > {
>> > u32 scu_ctrl;
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> This looks to me like nonsense.
>>
>> > #ifdef CONFIG_ARM_ERRATA_764369
>> > /* Cortex-A9 only */
>> > if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
>> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>> > unsigned int val;
>> > int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> Ditto.
>>
>> Rather than doing this, I'd much prefer to always store the SCU base in
>> the SCU code, and remove the "void __iomem *scu_base" argment from all
>> these functions.
>
> Ok, then we just need one scu_probe_*() variant for each of the
> four methods of initializing it (iotable, of_iomap,
> ioremap(scu_a9_get_base) and hardcoded.
>
> The intention of doing the fallback for the NULL argument was
> to avoid having to add lots of new API while also allowing
> the change to be done one platform at a time.
>
> If we remove the argument from the other functions, they either
> need to get a new name, or we change them all to the new prototype
> at once. Either way works fine, do you have a preference between
> them?
>
Russell,
Any opinion on this. Are you OK, with the approach suggested by Arnd?
Thanks,
Pankaj Dubey
> Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel