[PATCH v4 00/10] Sunxi: Add SMP support on A83T

[PATCH v4 00/10] Sunxi: Add SMP support on A83T

[Mylène Josserand]

Hello everyone,

This is a V4 of my series that adds SMP support for Allwinner sun8i-a83t.
Based on sunxi's tree, sunxi/for-next branch.

Changes since v3:
    - Take into account Maxime's reviews:
	- split the first patch into 4 new patches: add sun9i device tree
	parsing, rename some variables, add a83t support and finally,
	add hotplug support.
	- Move the code of previous patch 07 (to disable CPU0 disabling)
	into hotplug support patch (see patch 04)
	- Remove the patch that added PRCM register because it is already
	available. Because of that, update the device tree parsing to use
	"sun8i-a83t-r-ccu".
	- Use a variable to know which SoC we currently have
    - Take into account Chen-Yu's reviews: create two iounmap functions
    to release the resources of the device tree parsing.
    - Take into account Marc's review: Update the code to initialize CNTVOFF
    register. As there is already assembly code in the driver, I decided
    to create an assembly file not to mix assembly and C code.
    For that, I create 3 new patches: move the current assembly code that
    handles the cluster cache enabling into a file, move the cpu_resume entry
    in this file and finally, add a new assembly entry to initialize the timer
    offset for boot CPU and secondary CPUs.

Changes since v2:
    - Rebased my modifications according to new Chen Yu's patch series
    that adds SMP support for sun9i-a80 (without MCPM).
    - Split the device-tree patches into 3 patches for CPUCFG, R_CPUCFG
    and PRCM registers for more visibility.
    - The hotplug of CPU0 is currently not working (even after trying what
    Allwinner's code is doing) so remove the possibility of disabling
    this CPU. Created a new patch for it.

Changes since v1:
    - Add Chen Yu's patch in my series (see path 01)
    - Add new compatibles for prcm and cpucfg registers for sun8i-a83t.
    Create two functions to separate the DT parsing of sun9i-a80 and
    sun8i-a83t.
    - Thanks to Maxime's review: order device tree's nodes according
    to physical addresses, remove unused label and fix registers' sizes.
    Update the commit log and commit title of my last patch (see
    patch 05).

Patch 01: Create a function that handles all the current code doing a
device tree parsing.
Patch 02: Rename a register to prepare the a83t support with different
registers.
Patch 03: Convert the sunxi SMP driver to add support for A83T.
This SoC has a bit flip that needs to be handled.
Patch 04: Add hotplug support for a83t. Remove the possibilite to
disable the CPU0 because CPU0 hotplug is currently not working for
this SoC.
Patch 05-06: Add registers nodes (cpucfg and r_cpucfg) needed
for SMP bringup.
Patch 07: Add CCI-400 node for a83t.
Patch 08: Move the current assembly code into an assembly file
Patch 09: Move another assemble code (for resuming) into this file
Patch 10: Add a new entry to initialize the timer offset for all CPUs.

If you have any remarks/questions, let me know.
Thank you in advance,
Mylène

Mylène Josserand (10):
  ARM: sun9i: smp: Add sun9i dt parsing function
  ARM: sun9i: smp: Rename clusters's power-off register
  ARM: sun8i: smp: Add support for A83T
  ARM: sun8i: smp: Add hotplug support for sun8i-a83t
  ARM: dts: sun8i: Add CPUCFG device node for A83T dtsi
  ARM: dts: sun8i: Add R_CPUCFG device node for the A83T dtsi
  ARM: dts: sun8i: a83t: Add CCI-400 node
  ARM: sunxi: smp: Move assembly code into a file
  ARM: sunxi: smp: Move cpu_resume assembly entry into file
  ARM: sunxi: smp: Add initialization of CNTVOFF

 arch/arm/boot/dts/sun8i-a83t.dtsi |  51 ++++++
 arch/arm/mach-sunxi/Kconfig       |   2 +-
 arch/arm/mach-sunxi/Makefile      |   1 +
 arch/arm/mach-sunxi/headsmp.S     |  99 +++++++++++
 arch/arm/mach-sunxi/mc_smp.c      | 350 ++++++++++++++++++++++++--------------
 arch/arm/mach-sunxi/sunxi.c       |   4 +
 6 files changed, 383 insertions(+), 124 deletions(-)
 create mode 100644 arch/arm/mach-sunxi/headsmp.S

-- 
2.11.0

[PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Mylène Josserand]

To prepare the support for sun8i-a83t, create a new function
that handles all the resources needed on sun9i-a80 (because
it will be different from sun8i-a83t).

All the parsing of device tree is moved into this new function:
sun9i_dt_parsing. Create also a function to release the resources
retrieved during the dt parsing in case there is an error in init
function.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/mc_smp.c | 99 ++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 41 deletions(-)

diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index 11e46c6efb90..650a2ad4398f 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
 	return ret;
 }
 
-static int __init sunxi_mc_smp_init(void)
+static int sun9i_dt_parsing(struct resource res)
 {
-	struct device_node *cpucfg_node, *sram_node, *node;
-	struct resource res;
+	struct device_node *prcm_node, *cpucfg_node, *sram_node;
 	int ret;
 
-	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
-		return -ENODEV;
-
-	if (!sunxi_mc_smp_cpu_table_init())
-		return -EINVAL;
-
-	if (!cci_probed()) {
-		pr_err("%s: CCI-400 not available\n", __func__);
-		return -ENODEV;
-	}
-
-	node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
-	if (!node) {
-		pr_err("%s: PRCM not available\n", __func__);
+	prcm_node = of_find_compatible_node(NULL, NULL,
+				       "allwinner,sun9i-a80-prcm");
+	if (!prcm_node)
 		return -ENODEV;
-	}
 
 	/*
 	 * Unfortunately we can not request the I/O region for the PRCM.
 	 * It is shared with the PRCM clock.
 	 */
-	prcm_base = of_iomap(node, 0);
-	of_node_put(node);
+	prcm_base = of_iomap(prcm_node, 0);
+	of_node_put(prcm_node);
 	if (!prcm_base) {
 		pr_err("%s: failed to map PRCM registers\n", __func__);
 		return -ENOMEM;
@@ -748,33 +735,12 @@ static int __init sunxi_mc_smp_init(void)
 		goto err_put_sram_node;
 	}
 
-	/* Configure CCI-400 for boot cluster */
-	ret = sunxi_mc_smp_lookback();
-	if (ret) {
-		pr_err("%s: failed to configure boot cluster: %d\n",
-		       __func__, ret);
-		goto err_unmap_release_secure_sram;
-	}
-
 	/* We don't need the CPUCFG and SRAM device nodes anymore */
 	of_node_put(cpucfg_node);
 	of_node_put(sram_node);
 
-	/* Set the hardware entry point address */
-	writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
-	       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
-
-	/* Actually enable multi cluster SMP */
-	smp_set_ops(&sunxi_mc_smp_smp_ops);
-
-	pr_info("sunxi multi cluster SMP support installed\n");
-
 	return 0;
 
-err_unmap_release_secure_sram:
-	iounmap(sram_b_smp_base);
-	of_address_to_resource(sram_node, 0, &res);
-	release_mem_region(res.start, resource_size(&res));
 err_put_sram_node:
 	of_node_put(sram_node);
 err_unmap_release_cpucfg:
@@ -785,6 +751,57 @@ static int __init sunxi_mc_smp_init(void)
 	of_node_put(cpucfg_node);
 err_unmap_prcm:
 	iounmap(prcm_base);
+
+	return ret;
+}
+
+static void sun9i_iounmap(void)
+{
+	iounmap(sram_b_smp_base);
+	iounmap(cpucfg_base);
+	iounmap(prcm_base);
+}
+
+static int __init sunxi_mc_smp_init(void)
+{
+	struct resource res;
+	int ret;
+
+	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
+		return -ENODEV;
+
+	if (!sunxi_mc_smp_cpu_table_init())
+		return -EINVAL;
+
+	if (!cci_probed()) {
+		pr_err("%s: CCI-400 not available\n", __func__);
+		return -ENODEV;
+	}
+
+	ret = sun9i_dt_parsing(res);
+	if (ret) {
+		pr_err("%s: failed to parse DT: %d\n", __func__, ret);
+		return ret;
+	}
+
+	/* Configure CCI-400 for boot cluster */
+	ret = sunxi_mc_smp_lookback();
+	if (ret) {
+		pr_err("%s: failed to configure boot cluster: %d\n",
+		       __func__, ret);
+		sun9i_iounmap();
+		return ret;
+	}
+
+	/* Set the hardware entry point address */
+	writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
+	       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
+
+	/* Actually enable multi cluster SMP */
+	smp_set_ops(&sunxi_mc_smp_smp_ops);
+
+	pr_info("sunxi multi cluster SMP support installed\n");
+
 	return ret;
 }
 
-- 
2.11.0

[PATCH v4 02/10] ARM: sun9i: smp: Rename clusters's power-off register

[Mylène Josserand]

To prepare the support for sun8i-a83t, rename the variable name
that handles the power-off of clusters because it is different from
sun9i-a80 to sun8i-a83t.

The power off register for clusters are different from SUN9I and SUN8I

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/mc_smp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index 650a2ad4398f..de02e5662557 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -60,7 +60,7 @@
 #define PRCM_CPU_PO_RST_CTRL_CORE(n)	BIT(n)
 #define PRCM_CPU_PO_RST_CTRL_CORE_ALL	0xf
 #define PRCM_PWROFF_GATING_REG(c)	(0x100 + 0x4 * (c))
-#define PRCM_PWROFF_GATING_REG_CLUSTER	BIT(4)
+#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I	BIT(4)
 #define PRCM_PWROFF_GATING_REG_CORE(n)	BIT(n)
 #define PRCM_PWR_SWITCH_REG(c, cpu)	(0x140 + 0x10 * (c) + 0x4 * (cpu))
 #define PRCM_CPU_SOFT_ENTRY_REG		0x164
@@ -252,7 +252,7 @@ static int sunxi_cluster_powerup(unsigned int cluster)
 
 	/* clear cluster power gate */
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
-	reg &= ~PRCM_PWROFF_GATING_REG_CLUSTER;
+	reg &= ~PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
@@ -516,7 +516,7 @@ static int sunxi_cluster_powerdown(unsigned int cluster)
 	/* gate cluster power */
 	pr_debug("%s: gate cluster power\n", __func__);
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
-	reg |= PRCM_PWROFF_GATING_REG_CLUSTER;
+	reg |= PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
-- 
2.11.0

[PATCH v4 03/10] ARM: sun8i: smp: Add support for A83T

[Mylène Josserand]

Add the support for A83T.

A83T SoC has an additional register than A80 to handle CPU configurations:
R_CPUS_CFG. Information about the register comes from Allwinner's BSP
driver.
An important difference is the Power Off Gating register for clusters
which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/Kconfig  |   2 +-
 arch/arm/mach-sunxi/mc_smp.c | 168 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 162 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index ce53ceaf4cc5..a0ad35c41c02 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -51,7 +51,7 @@ config MACH_SUN9I
 config ARCH_SUNXI_MC_SMP
 	bool
 	depends on SMP
-	default MACH_SUN9I
+	default y if MACH_SUN9I || MACH_SUN8I
 	select ARM_CCI400_PORT_CTRL
 	select ARM_CPU_SUSPEND
 
diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index de02e5662557..3bd9066a1422 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -55,22 +55,32 @@
 #define CPUCFG_CX_RST_CTRL_L2_RST	BIT(8)
 #define CPUCFG_CX_RST_CTRL_CX_RST(n)	BIT(4 + (n))
 #define CPUCFG_CX_RST_CTRL_CORE_RST(n)	BIT(n)
+#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL	(0xf << 0)
 
 #define PRCM_CPU_PO_RST_CTRL(c)		(0x4 + 0x4 * (c))
 #define PRCM_CPU_PO_RST_CTRL_CORE(n)	BIT(n)
 #define PRCM_CPU_PO_RST_CTRL_CORE_ALL	0xf
 #define PRCM_PWROFF_GATING_REG(c)	(0x100 + 0x4 * (c))
+/* The power off register for clusters are different from SUN9I and SUN8I */
+#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I	BIT(0)
 #define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I	BIT(4)
 #define PRCM_PWROFF_GATING_REG_CORE(n)	BIT(n)
 #define PRCM_PWR_SWITCH_REG(c, cpu)	(0x140 + 0x10 * (c) + 0x4 * (cpu))
 #define PRCM_CPU_SOFT_ENTRY_REG		0x164
 
+/* R_CPUCFG registers, specific to SUN8I */
+#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)	(0x30 + (c) * 0x4)
+#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n)	BIT(n)
+#define R_CPUCFG_CPU_SOFT_ENTRY_REG	0x01a4
+
 #define CPU0_SUPPORT_HOTPLUG_MAGIC0	0xFA50392F
 #define CPU0_SUPPORT_HOTPLUG_MAGIC1	0x790DCA3A
 
 static void __iomem *cpucfg_base;
+static void __iomem *r_cpucfg_base;
 static void __iomem *prcm_base;
 static void __iomem *sram_b_smp_base;
+static bool is_sun9i;
 
 static bool sunxi_core_is_cortex_a15(unsigned int core, unsigned int cluster)
 {
@@ -157,6 +167,16 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned int cluster)
 	reg &= ~PRCM_CPU_PO_RST_CTRL_CORE(cpu);
 	writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster));
 
+	if (r_cpucfg_base) {
+		/* assert cpu power-on reset */
+		reg  = readl(r_cpucfg_base +
+			     R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		reg &= ~(R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu));
+		writel(reg, r_cpucfg_base +
+		       R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		udelay(10);
+	}
+
 	/* Cortex-A7: hold L1 reset disable signal low */
 	if (!sunxi_core_is_cortex_a15(cpu, cluster)) {
 		reg = readl(cpucfg_base + CPUCFG_CX_CTRL_REG0(cluster));
@@ -180,17 +200,37 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned int cluster)
 	/* open power switch */
 	sunxi_cpu_power_switch_set(cpu, cluster, true);
 
+	/* Handle A83T bit swap */
+	if (!is_sun9i) {
+		if (cpu == 0)
+			cpu = 4;
+	}
+
 	/* clear processor power gate */
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	reg &= ~PRCM_PWROFF_GATING_REG_CORE(cpu);
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
+	if (!is_sun9i) {
+		if (cpu == 4)
+			cpu = 0;
+	}
+
 	/* de-assert processor power-on reset */
 	reg = readl(prcm_base + PRCM_CPU_PO_RST_CTRL(cluster));
 	reg |= PRCM_CPU_PO_RST_CTRL_CORE(cpu);
 	writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster));
 
+	if (r_cpucfg_base) {
+		reg  = readl(r_cpucfg_base +
+			     R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		reg |= R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu);
+		writel(reg, r_cpucfg_base +
+		       R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		udelay(10);
+	}
+
 	/* de-assert all processor resets */
 	reg = readl(cpucfg_base + CPUCFG_CX_RST_CTRL(cluster));
 	reg |= CPUCFG_CX_RST_CTRL_DBG_RST(cpu);
@@ -212,6 +252,14 @@ static int sunxi_cluster_powerup(unsigned int cluster)
 	if (cluster >= SUNXI_NR_CLUSTERS)
 		return -EINVAL;
 
+	/* For A83T, assert cluster cores resets */
+	if (!is_sun9i) {
+		reg = readl(cpucfg_base + CPUCFG_CX_RST_CTRL(cluster));
+		reg &= ~CPUCFG_CX_RST_CTRL_CORE_RST_ALL;   /* Core Reset    */
+		writel(reg, cpucfg_base + CPUCFG_CX_RST_CTRL(cluster));
+		udelay(10);
+	}
+
 	/* assert ACINACTM */
 	reg = readl(cpucfg_base + CPUCFG_CX_CTRL_REG1(cluster));
 	reg |= CPUCFG_CX_CTRL_REG1_ACINACTM;
@@ -222,6 +270,16 @@ static int sunxi_cluster_powerup(unsigned int cluster)
 	reg &= ~PRCM_CPU_PO_RST_CTRL_CORE_ALL;
 	writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster));
 
+	/* assert cluster cores resets */
+	if (r_cpucfg_base) {
+		reg  = readl(r_cpucfg_base +
+			     R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		reg &= ~CPUCFG_CX_RST_CTRL_CORE_RST_ALL;
+		writel(reg, r_cpucfg_base +
+		       R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster));
+		udelay(10);
+	}
+
 	/* assert cluster resets */
 	reg = readl(cpucfg_base + CPUCFG_CX_RST_CTRL(cluster));
 	reg &= ~CPUCFG_CX_RST_CTRL_DBG_SOC_RST;
@@ -252,7 +310,10 @@ static int sunxi_cluster_powerup(unsigned int cluster)
 
 	/* clear cluster power gate */
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
-	reg &= ~PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
+	if (is_sun9i)
+		reg &= ~PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
+	else
+		reg &= ~PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I;
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
@@ -516,7 +577,8 @@ static int sunxi_cluster_powerdown(unsigned int cluster)
 	/* gate cluster power */
 	pr_debug("%s: gate cluster power\n", __func__);
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
-	reg |= PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
+	if (is_sun9i)
+		reg |= PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
@@ -762,12 +824,92 @@ static void sun9i_iounmap(void)
 	iounmap(prcm_base);
 }
 
+static int sun8i_dt_parsing(struct resource res)
+{
+	struct device_node *node, *cpucfg_node;
+	int ret;
+
+	node = of_find_compatible_node(NULL, NULL,
+				       "allwinner,sun8i-a83t-r-ccu");
+	if (!node)
+		return -ENODEV;
+
+	/*
+	 * Unfortunately we can not request the I/O region for the PRCM.
+	 * It is shared with the PRCM clock.
+	 */
+	prcm_base = of_iomap(node, 0);
+	of_node_put(node);
+	if (!prcm_base) {
+		pr_err("%s: failed to map PRCM registers\n", __func__);
+		iounmap(prcm_base);
+		return -ENOMEM;
+	}
+
+	cpucfg_node = of_find_compatible_node(NULL, NULL,
+					      "allwinner,sun8i-a83t-cpucfg");
+	if (!cpucfg_node) {
+		ret = -ENODEV;
+		pr_err("%s: CPUCFG not available\n", __func__);
+		goto err_unmap_prcm;
+	}
+
+	cpucfg_base = of_io_request_and_map(cpucfg_node, 0, "sunxi-mc-smp");
+	if (IS_ERR(cpucfg_base)) {
+		ret = PTR_ERR(cpucfg_base);
+		pr_err("%s: failed to map CPUCFG registers: %d\n",
+		       __func__, ret);
+		goto err_put_cpucfg_node;
+	}
+
+	node = of_find_compatible_node(NULL, NULL,
+				       "allwinner,sun8i-a83t-r-cpucfg");
+	if (!node) {
+		ret = -ENODEV;
+		goto err_unmap_release_cpucfg;
+	}
+
+	r_cpucfg_base = of_iomap(node, 0);
+	if (!r_cpucfg_base) {
+		pr_err("%s: failed to map R-CPUCFG registers\n",
+		       __func__);
+		ret = -ENOMEM;
+		goto err_put_node;
+	}
+
+	/* We don't need the CPUCFG device node anymore */
+	of_node_put(cpucfg_node);
+	of_node_put(node);
+
+	return 0;
+err_put_node:
+	of_node_put(node);
+err_unmap_release_cpucfg:
+	iounmap(cpucfg_base);
+	of_address_to_resource(cpucfg_node, 0, &res);
+	release_mem_region(res.start, resource_size(&res));
+err_put_cpucfg_node:
+	of_node_put(cpucfg_node);
+err_unmap_prcm:
+	iounmap(prcm_base);
+
+	return ret;
+}
+
+static void sun8i_iounmap(void)
+{
+	iounmap(r_cpucfg_base);
+	iounmap(cpucfg_base);
+	iounmap(prcm_base);
+}
+
 static int __init sunxi_mc_smp_init(void)
 {
 	struct resource res;
 	int ret;
 
-	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
+	if (!of_machine_is_compatible("allwinner,sun9i-a80") &&
+	    !of_machine_is_compatible("allwinner,sun8i-a83t"))
 		return -ENODEV;
 
 	if (!sunxi_mc_smp_cpu_table_init())
@@ -778,7 +920,12 @@ static int __init sunxi_mc_smp_init(void)
 		return -ENODEV;
 	}
 
-	ret = sun9i_dt_parsing(res);
+	is_sun9i = of_machine_is_compatible("allwinner,sun9i-a80");
+
+	if (is_sun9i)
+		ret = sun9i_dt_parsing(res);
+	else
+		ret = sun8i_dt_parsing(res);
 	if (ret) {
 		pr_err("%s: failed to parse DT: %d\n", __func__, ret);
 		return ret;
@@ -789,13 +936,20 @@ static int __init sunxi_mc_smp_init(void)
 	if (ret) {
 		pr_err("%s: failed to configure boot cluster: %d\n",
 		       __func__, ret);
-		sun9i_iounmap();
+		if (is_sun9i)
+			sun9i_iounmap();
+		else
+			sun8i_iounmap();
 		return ret;
 	}
 
 	/* Set the hardware entry point address */
-	writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
-	       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
+	if (is_sun9i)
+		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
+		       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
+	else
+		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
+		       r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG);
 
 	/* Actually enable multi cluster SMP */
 	smp_set_ops(&sunxi_mc_smp_smp_ops);
-- 
2.11.0

[PATCH v4 04/10] ARM: sun8i: smp: Add hotplug support for sun8i-a83t

[Mylène Josserand]

Add hotplug support for sun8i-a83t.

Hotplug CPU for CPU0 is currently not working.
Remove the possibility to disable CPU0 only for sun8i-a83t.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/mc_smp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index 3bd9066a1422..f2c2cfca28cd 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -579,6 +579,8 @@ static int sunxi_cluster_powerdown(unsigned int cluster)
 	reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	if (is_sun9i)
 		reg |= PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I;
+	else
+		reg |= PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I;
 	writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster));
 	udelay(20);
 
@@ -660,8 +662,12 @@ static int sunxi_mc_smp_cpu_kill(unsigned int l_cpu)
 	return !ret;
 }
 
-static bool sunxi_mc_smp_cpu_can_disable(unsigned int __unused)
+static bool sunxi_mc_smp_cpu_can_disable(unsigned int cpu)
 {
+	/* CPU0 hotplug handled only for sun9i */
+	if (!is_sun9i)
+		if (cpu == 0)
+			return false;
 	return true;
 }
 #endif
-- 
2.11.0

[PATCH v4 05/10] ARM: dts: sun8i: Add CPUCFG device node for A83T dtsi

[Mylène Josserand]

As we found in sun9i-a80, CPUCFG is a collection of registers that are
mapped to the SoC's signals from each individual processor core and
associated peripherals.

These registers are used for SMP bringup and CPU hotplugging.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 46ae4faa5894..01993284c0f4 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -231,6 +231,11 @@
 			};
 		};
 
+		cpucfg@1700000 {
+			compatible = "allwinner,sun8i-a83t-cpucfg";
+			reg = <0x01700000 0x400>;
+		};
+
 		syscon: syscon@1c00000 {
 			compatible = "allwinner,sun8i-a83t-system-controller",
 				"syscon";
-- 
2.11.0

[PATCH v4 06/10] ARM: dts: sun8i: Add R_CPUCFG device node for the A83T dtsi

[Mylène Josserand]

The R_CPUCFG is a collection of registers needed for SMP bringup
on clusters and cluster's reset.
For the moment, documentation about this register is found in
Allwinner's code only.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 01993284c0f4..3eae3c27e04d 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -815,6 +815,11 @@
 			#reset-cells = <1>;
 		};
 
+		r_cpucfg@1f01c00 {
+			compatible = "allwinner,sun8i-a83t-r-cpucfg";
+			reg = <0x1f01c00 0x100>;
+		};
+
 		r_pio: pinctrl@1f02c00 {
 			compatible = "allwinner,sun8i-a83t-r-pinctrl";
 			reg = <0x01f02c00 0x400>;
-- 
2.11.0

[PATCH v4 07/10] ARM: dts: sun8i: a83t: Add CCI-400 node

[Mylène Josserand]

Add CCI-400 node and control-port on CPUs needed by SMP bringup.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 41 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 3eae3c27e04d..84d6c7738125 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -64,48 +64,56 @@
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0>;
+			cci-control-port = <&cci_control0>;
 		};
 
 		cpu@1 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <1>;
+			cci-control-port = <&cci_control0>;
 		};
 
 		cpu@2 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <2>;
+			cci-control-port = <&cci_control0>;
 		};
 
 		cpu@3 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <3>;
+			cci-control-port = <&cci_control0>;
 		};
 
 		cpu@100 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0x100>;
+			cci-control-port = <&cci_control1>;
 		};
 
 		cpu@101 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0x101>;
+			cci-control-port = <&cci_control1>;
 		};
 
 		cpu@102 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0x102>;
+			cci-control-port = <&cci_control1>;
 		};
 
 		cpu@103 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0x103>;
+			cci-control-port = <&cci_control1>;
 		};
 	};
 
@@ -236,6 +244,39 @@
 			reg = <0x01700000 0x400>;
 		};
 
+		cci@1790000 {
+			compatible = "arm,cci-400";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x01790000 0x10000>;
+			ranges = <0x0 0x01790000 0x10000>;
+
+			cci_control0: slave-if@4000 {
+				compatible = "arm,cci-400-ctrl-if";
+				interface-type = "ace";
+				reg = <0x4000 0x1000>;
+			};
+
+			cci_control1: slave-if@5000 {
+				compatible = "arm,cci-400-ctrl-if";
+				interface-type = "ace";
+				reg = <0x5000 0x1000>;
+			};
+
+			pmu@9000 {
+				compatible = "arm,cci-400-pmu,r1";
+				reg = <0x9000 0x5000>;
+				interrupts = <GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
+			};
+		};
+
 		syscon: syscon@1c00000 {
 			compatible = "allwinner,sun8i-a83t-system-controller",
 				"syscon";
-- 
2.11.0

[PATCH v4 08/10] ARM: sunxi: smp: Move assembly code into a file

[Mylène Josserand]

Move the assembly code for cluster cache enabling
into an assembly file instead of having it directly in C code.

Create a sunxi_boot entry that will perform a cluster cached
enabling and called secondary_startup.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/Makefile  |  1 +
 arch/arm/mach-sunxi/headsmp.S | 73 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-sunxi/mc_smp.c  | 76 ++++---------------------------------------
 3 files changed, 80 insertions(+), 70 deletions(-)
 create mode 100644 arch/arm/mach-sunxi/headsmp.S

diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
index 7de9cc286d53..d1a072b879ed 100644
--- a/arch/arm/mach-sunxi/Makefile
+++ b/arch/arm/mach-sunxi/Makefile
@@ -1,5 +1,6 @@
 CFLAGS_mc_smp.o	+= -march=armv7-a
 
 obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
+obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
 obj-$(CONFIG_ARCH_SUNXI_MC_SMP) += mc_smp.o
 obj-$(CONFIG_SMP) += platsmp.o
diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
new file mode 100644
index 000000000000..4f5957a6e188
--- /dev/null
+++ b/arch/arm/mach-sunxi/headsmp.S
@@ -0,0 +1,73 @@
+/*
+ * SMP support for sunxi based systems with Cortex A7/A15
+ *
+ * Copyright (C) 2018 Bootlin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+
+ENTRY(sunxi_mc_smp_cluster_cache_enable)
+	/*
+	* Enable cluster-level coherency, in preparation for turning on the MMU.
+	*
+	* Also enable regional clock gating and L2 data latency settings for
+	* Cortex-A15. These settings are from the vendor kernel.
+	*/
+	mrc	p15, 0, r1, c0, c0, 0
+	movw	r2, #(0xff00fff0&0xffff)
+	movt	r2, #(0xff00fff0>>16)
+	and	r1, r1, r2
+	movw	r2, #(0x4100c0f0&0xffff)
+	movt	r2, #(0x4100c0f0>>16)
+	cmp	r1, r2
+	bne	not_a15
+
+	/* The following is Cortex-A15 specific */
+
+	/* ACTLR2: Enable CPU regional clock gates */
+	mrc p15, 1, r1, c15, c0, 4
+	orr r1, r1, #(0x1<<31)
+	mcr p15, 1, r1, c15, c0, 4
+
+	/* L2ACTLR */
+	mrc p15, 1, r1, c15, c0, 0
+	/* Enable L2, GIC, and Timer regional clock gates */
+	orr r1, r1, #(0x1<<26)
+	/* Disable clean/evict from being pushed to external */
+	orr r1, r1, #(0x1<<3)
+	mcr p15, 1, r1, c15, c0, 0
+
+	/* L2CTRL: L2 data RAM latency */
+	mrc p15, 1, r1, c9, c0, 2
+	bic r1, r1, #(0x7<<0)
+	orr r1, r1, #(0x3<<0)
+	mcr p15, 1, r1, c9, c0, 2
+
+	/* End of Cortex-A15 specific setup */
+	not_a15:
+
+	/* Get value of sunxi_mc_smp_first_comer */
+	adr	r1, first
+	ldr	r0, [r1]
+	ldr	r0, [r1, r0]
+
+	/* Skip cci_enable_port_for_self if not first comer */
+	cmp	r0, #0
+	bxeq	lr
+	b	cci_enable_port_for_self
+
+	.align 2
+	first: .word sunxi_mc_smp_first_comer - .
+ENDPROC(sunxi_mc_smp_cluster_cache_enable)
+
+#ifdef CONFIG_SMP
+ENTRY(sunxi_boot)
+	bl	sunxi_mc_smp_cluster_cache_enable
+	b	secondary_startup
+ENDPROC(sunxi_boot)
+#endif
diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index f2c2cfca28cd..4e807cc11a0f 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -82,6 +82,9 @@ static void __iomem *prcm_base;
 static void __iomem *sram_b_smp_base;
 static bool is_sun9i;
 
+extern void sunxi_boot(void);
+extern void sunxi_cluster_cache_enable(void);
+
 static bool sunxi_core_is_cortex_a15(unsigned int core, unsigned int cluster)
 {
 	struct device_node *node;
@@ -361,74 +364,7 @@ static void sunxi_cluster_cache_disable_without_axi(void)
 }
 
 static int sunxi_mc_smp_cpu_table[SUNXI_NR_CLUSTERS][SUNXI_CPUS_PER_CLUSTER];
-static int sunxi_mc_smp_first_comer;
-
-/*
- * Enable cluster-level coherency, in preparation for turning on the MMU.
- *
- * Also enable regional clock gating and L2 data latency settings for
- * Cortex-A15. These settings are from the vendor kernel.
- */
-static void __naked sunxi_mc_smp_cluster_cache_enable(void)
-{
-	asm volatile (
-		"mrc	p15, 0, r1, c0, c0, 0\n"
-		"movw	r2, #" __stringify(ARM_CPU_PART_MASK & 0xffff) "\n"
-		"movt	r2, #" __stringify(ARM_CPU_PART_MASK >> 16) "\n"
-		"and	r1, r1, r2\n"
-		"movw	r2, #" __stringify(ARM_CPU_PART_CORTEX_A15 & 0xffff) "\n"
-		"movt	r2, #" __stringify(ARM_CPU_PART_CORTEX_A15 >> 16) "\n"
-		"cmp	r1, r2\n"
-		"bne	not_a15\n"
-
-		/* The following is Cortex-A15 specific */
-
-		/* ACTLR2: Enable CPU regional clock gates */
-		"mrc p15, 1, r1, c15, c0, 4\n"
-		"orr r1, r1, #(0x1<<31)\n"
-		"mcr p15, 1, r1, c15, c0, 4\n"
-
-		/* L2ACTLR */
-		"mrc p15, 1, r1, c15, c0, 0\n"
-		/* Enable L2, GIC, and Timer regional clock gates */
-		"orr r1, r1, #(0x1<<26)\n"
-		/* Disable clean/evict from being pushed to external */
-		"orr r1, r1, #(0x1<<3)\n"
-		"mcr p15, 1, r1, c15, c0, 0\n"
-
-		/* L2CTRL: L2 data RAM latency */
-		"mrc p15, 1, r1, c9, c0, 2\n"
-		"bic r1, r1, #(0x7<<0)\n"
-		"orr r1, r1, #(0x3<<0)\n"
-		"mcr p15, 1, r1, c9, c0, 2\n"
-
-		/* End of Cortex-A15 specific setup */
-		"not_a15:\n"
-
-		/* Get value of sunxi_mc_smp_first_comer */
-		"adr	r1, first\n"
-		"ldr	r0, [r1]\n"
-		"ldr	r0, [r1, r0]\n"
-
-		/* Skip cci_enable_port_for_self if not first comer */
-		"cmp	r0, #0\n"
-		"bxeq	lr\n"
-		"b	cci_enable_port_for_self\n"
-
-		".align 2\n"
-		"first: .word sunxi_mc_smp_first_comer - .\n"
-	);
-}
-
-static void __naked sunxi_mc_smp_secondary_startup(void)
-{
-	asm volatile(
-		"bl	sunxi_mc_smp_cluster_cache_enable\n"
-		"b	secondary_startup"
-		/* Let compiler know about sunxi_mc_smp_cluster_cache_enable */
-		:: "i" (sunxi_mc_smp_cluster_cache_enable)
-	);
-}
+int sunxi_mc_smp_first_comer;
 
 static DEFINE_SPINLOCK(boot_lock);
 
@@ -951,10 +887,10 @@ static int __init sunxi_mc_smp_init(void)
 
 	/* Set the hardware entry point address */
 	if (is_sun9i)
-		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
+		writel(__pa_symbol(sunxi_boot),
 		       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
 	else
-		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
+		writel(__pa_symbol(sunxi_boot),
 		       r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG);
 
 	/* Actually enable multi cluster SMP */
-- 
2.11.0

[PATCH v4 09/10] ARM: sunxi: smp: Move cpu_resume assembly entry into file

[Mylène Josserand]

Move the CPU resume assembly function into the assembly file
to remove all assembly code from C code.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/headsmp.S |  5 +++++
 arch/arm/mach-sunxi/mc_smp.c  | 11 +----------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
index 4f5957a6e188..d5c97e945e69 100644
--- a/arch/arm/mach-sunxi/headsmp.S
+++ b/arch/arm/mach-sunxi/headsmp.S
@@ -70,4 +70,9 @@ ENTRY(sunxi_boot)
 	bl	sunxi_mc_smp_cluster_cache_enable
 	b	secondary_startup
 ENDPROC(sunxi_boot)
+
+ENTRY(sunxi_mc_smp_resume)
+	bl	sunxi_mc_smp_cluster_cache_enable
+	b	cpu_resume
+ENDPROC(sunxi_mc_smp_resume)
 #endif
diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
index 4e807cc11a0f..f1ad425a469c 100644
--- a/arch/arm/mach-sunxi/mc_smp.c
+++ b/arch/arm/mach-sunxi/mc_smp.c
@@ -84,6 +84,7 @@ static bool is_sun9i;
 
 extern void sunxi_boot(void);
 extern void sunxi_cluster_cache_enable(void);
+extern void sunxi_mc_smp_resume(void);
 
 static bool sunxi_core_is_cortex_a15(unsigned int core, unsigned int cluster)
 {
@@ -641,16 +642,6 @@ static bool __init sunxi_mc_smp_cpu_table_init(void)
  */
 typedef typeof(cpu_reset) phys_reset_t;
 
-static void __init __naked sunxi_mc_smp_resume(void)
-{
-	asm volatile(
-		"bl	sunxi_mc_smp_cluster_cache_enable\n"
-		"b	cpu_resume"
-		/* Let compiler know about sunxi_mc_smp_cluster_cache_enable */
-		:: "i" (sunxi_mc_smp_cluster_cache_enable)
-	);
-}
-
 static int __init nocache_trampoline(unsigned long __unused)
 {
 	phys_reset_t phys_reset;
-- 
2.11.0

[PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Mylène Josserand]

On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
It should be done by the bootloader but it is currently not the case,
even for boot CPU because this SoC is booting in secure mode.
It leads to an random offset value meaning that each CPU will have a
different time, which isn't working very well.

Add assembly code used for boot CPU and secondary CPU cores to make
sure that the CNTVOFF register is initialized.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
 arch/arm/mach-sunxi/sunxi.c   |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
index d5c97e945e69..605896251927 100644
--- a/arch/arm/mach-sunxi/headsmp.S
+++ b/arch/arm/mach-sunxi/headsmp.S
@@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
 	first: .word sunxi_mc_smp_first_comer - .
 ENDPROC(sunxi_mc_smp_cluster_cache_enable)
 
+ENTRY(sunxi_init_cntvoff)
+	/*
+	 * CNTVOFF has to be initialized either from non-secure Hypervisor
+	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
+	 * then it should be handled by the secure code
+	 */
+	cps	#MON_MODE
+	mrc	p15, 0, r1, c1, c1, 0		/* Get Secure Config */
+	orr	r0, r1, #1
+	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
+	instr_sync
+	mov	r0, #0
+	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
+	instr_sync
+	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
+	instr_sync
+	cps	#SVC_MODE
+	ret	lr
+ENDPROC(sunxi_init_cntvoff)
+
 #ifdef CONFIG_SMP
 ENTRY(sunxi_boot)
 	bl	sunxi_mc_smp_cluster_cache_enable
+	bl	sunxi_init_cntvoff
 	b	secondary_startup
 ENDPROC(sunxi_boot)
 
diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
index 5e9602ce1573..4bb041492b54 100644
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
 };
 
 extern void __init sun6i_reset_init(void);
+extern void sunxi_init_cntvoff(void);
+
 static void __init sun6i_timer_init(void)
 {
+	sunxi_init_cntvoff();
+
 	of_clk_init(NULL);
 	if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
 		sun6i_reset_init();
-- 
2.11.0

Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

Hi,

On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> To prepare the support for sun8i-a83t, create a new function
> that handles all the resources needed on sun9i-a80 (because
> it will be different from sun8i-a83t).
> 
> All the parsing of device tree is moved into this new function:
> sun9i_dt_parsing. Create also a function to release the resources
> retrieved during the dt parsing in case there is an error in init
> function.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/mc_smp.c | 99 ++++++++++++++++++++++++++------------------
>  1 file changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index 11e46c6efb90..650a2ad4398f 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
>  	return ret;
>  }
>  
> -static int __init sunxi_mc_smp_init(void)
> +static int sun9i_dt_parsing(struct resource res)

Like I told you in the previous version, this should be _parse_dt, and
not _dt_parsing.

Also, I'm not sure why you are passing by copy the resource structure?

>  {
> -	struct device_node *cpucfg_node, *sram_node, *node;
> -	struct resource res;
> +	struct device_node *prcm_node, *cpucfg_node, *sram_node;
>  	int ret;
>  
> -	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> -		return -ENODEV;
> -
> -	if (!sunxi_mc_smp_cpu_table_init())
> -		return -EINVAL;
> -
> -	if (!cci_probed()) {
> -		pr_err("%s: CCI-400 not available\n", __func__);
> -		return -ENODEV;
> -	}
> -
> -	node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> -	if (!node) {
> -		pr_err("%s: PRCM not available\n", __func__);
> +	prcm_node = of_find_compatible_node(NULL, NULL,
> +				       "allwinner,sun9i-a80-prcm");
> +	if (!prcm_node)
>  		return -ENODEV;
> -	}
>  
>  	/*
>  	 * Unfortunately we can not request the I/O region for the PRCM.
>  	 * It is shared with the PRCM clock.
>  	 */
> -	prcm_base = of_iomap(node, 0);
> -	of_node_put(node);
> +	prcm_base = of_iomap(prcm_node, 0);

So it does more a bit more than just parsing the DT?

Can you maybe just fill the DT nodes and have the common part map the
memory regions if the pointer is not NULL?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 02/10] ARM: sun9i: smp: Rename clusters's power-off register

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Fri, Feb 23, 2018 at 02:37:34PM +0100, Mylène Josserand wrote:
> To prepare the support for sun8i-a83t, rename the variable name
> that handles the power-off of clusters because it is different from
> sun9i-a80 to sun8i-a83t.
>
> The power off register for clusters are different from SUN9I and SUN8I

This is just a nitpick, and I see you use them everywhere, but sun8i
and sun9i are the family of SoCs, and A80 and A83t the name of SoCs
within their respective families.

So in your commit log, you want to support the A83t because it's
different than the A80, and the sun9i and sun8i mentionned in your
last sentence is not really meaningful.

> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 03/10] ARM: sun8i: smp: Add support for A83T

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]

On Fri, Feb 23, 2018 at 02:37:35PM +0100, Mylène Josserand wrote:
> Add the support for A83T.
> 
> A83T SoC has an additional register than A80 to handle CPU configurations:
> R_CPUS_CFG. Information about the register comes from Allwinner's BSP
> driver.
> An important difference is the Power Off Gating register for clusters
> which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/Kconfig  |   2 +-
>  arch/arm/mach-sunxi/mc_smp.c | 168 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 162 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index ce53ceaf4cc5..a0ad35c41c02 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -51,7 +51,7 @@ config MACH_SUN9I
>  config ARCH_SUNXI_MC_SMP
>  	bool
>  	depends on SMP
> -	default MACH_SUN9I
> +	default y if MACH_SUN9I || MACH_SUN8I
>  	select ARM_CCI400_PORT_CTRL
>  	select ARM_CPU_SUSPEND
>  
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index de02e5662557..3bd9066a1422 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -55,22 +55,32 @@
>  #define CPUCFG_CX_RST_CTRL_L2_RST	BIT(8)
>  #define CPUCFG_CX_RST_CTRL_CX_RST(n)	BIT(4 + (n))
>  #define CPUCFG_CX_RST_CTRL_CORE_RST(n)	BIT(n)
> +#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL	(0xf << 0)
>  
>  #define PRCM_CPU_PO_RST_CTRL(c)		(0x4 + 0x4 * (c))
>  #define PRCM_CPU_PO_RST_CTRL_CORE(n)	BIT(n)
>  #define PRCM_CPU_PO_RST_CTRL_CORE_ALL	0xf
>  #define PRCM_PWROFF_GATING_REG(c)	(0x100 + 0x4 * (c))
> +/* The power off register for clusters are different from SUN9I and SUN8I */
> +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I	BIT(0)
>  #define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I	BIT(4)
>  #define PRCM_PWROFF_GATING_REG_CORE(n)	BIT(n)
>  #define PRCM_PWR_SWITCH_REG(c, cpu)	(0x140 + 0x10 * (c) + 0x4 * (cpu))
>  #define PRCM_CPU_SOFT_ENTRY_REG		0x164
>  
> +/* R_CPUCFG registers, specific to SUN8I */
> +#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)	(0x30 + (c) * 0x4)
> +#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n)	BIT(n)
> +#define R_CPUCFG_CPU_SOFT_ENTRY_REG	0x01a4
> +
>  #define CPU0_SUPPORT_HOTPLUG_MAGIC0	0xFA50392F
>  #define CPU0_SUPPORT_HOTPLUG_MAGIC1	0x790DCA3A
>  
>  static void __iomem *cpucfg_base;
> +static void __iomem *r_cpucfg_base;
>  static void __iomem *prcm_base;
>  static void __iomem *sram_b_smp_base;
> +static bool is_sun9i;

Since you always check for that condition to always be false, can't
you do the opposite, ie have it called is_a83t, and verify it to be
true?

>  	/* Set the hardware entry point address */
> -	writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> -	       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> +	if (is_sun9i)
> +		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> +		       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> +	else
> +		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> +		       r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG);

if (is_a83t)
	reg = prcm_base + PRCM_CPU_SOFT_ENTRY_REG;
else
	reg = r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG;
writel(__pa_symbol(sunxi_mc_smp_secondary_startup), reg);

?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 08/10] ARM: sunxi: smp: Move assembly code into a file

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4547 bytes --]

On Fri, Feb 23, 2018 at 02:37:40PM +0100, Mylène Josserand wrote:
> Move the assembly code for cluster cache enabling
> into an assembly file instead of having it directly in C code.
> 
> Create a sunxi_boot entry that will perform a cluster cached
> enabling and called secondary_startup.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/Makefile  |  1 +
>  arch/arm/mach-sunxi/headsmp.S | 73 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-sunxi/mc_smp.c  | 76 ++++---------------------------------------
>  3 files changed, 80 insertions(+), 70 deletions(-)
>  create mode 100644 arch/arm/mach-sunxi/headsmp.S
> 
> diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> index 7de9cc286d53..d1a072b879ed 100644
> --- a/arch/arm/mach-sunxi/Makefile
> +++ b/arch/arm/mach-sunxi/Makefile
> @@ -1,5 +1,6 @@
>  CFLAGS_mc_smp.o	+= -march=armv7-a
>  
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
> +obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
>  obj-$(CONFIG_ARCH_SUNXI_MC_SMP) += mc_smp.o
>  obj-$(CONFIG_SMP) += platsmp.o
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> new file mode 100644
> index 000000000000..4f5957a6e188
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -0,0 +1,73 @@
> +/*
> + * SMP support for sunxi based systems with Cortex A7/A15
> + *
> + * Copyright (C) 2018 Bootlin

This is just a copy, and while you can claim that you are one of the
copyrights holder, you are not the sole one and the original author
should be there.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

You want to use SPDX there instead.

> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> +
> +ENTRY(sunxi_mc_smp_cluster_cache_enable)
> +	/*
> +	* Enable cluster-level coherency, in preparation for turning on the MMU.
> +	*
> +	* Also enable regional clock gating and L2 data latency settings for
> +	* Cortex-A15. These settings are from the vendor kernel.
> +	*/

The indentation is not correct there, the * should be aligned

> +	mrc	p15, 0, r1, c0, c0, 0
> +	movw	r2, #(0xff00fff0&0xffff)
> +	movt	r2, #(0xff00fff0>>16)

This used to be defines, we should keep them, and we should have
spaces around the operators as well.

> +	and	r1, r1, r2
> +	movw	r2, #(0x4100c0f0&0xffff)
> +	movt	r2, #(0x4100c0f0>>16)
> +	cmp	r1, r2
> +	bne	not_a15
> +
> +	/* The following is Cortex-A15 specific */
> +
> +	/* ACTLR2: Enable CPU regional clock gates */
> +	mrc p15, 1, r1, c15, c0, 4
> +	orr r1, r1, #(0x1<<31)
> +	mcr p15, 1, r1, c15, c0, 4
> +
> +	/* L2ACTLR */
> +	mrc p15, 1, r1, c15, c0, 0
> +	/* Enable L2, GIC, and Timer regional clock gates */
> +	orr r1, r1, #(0x1<<26)
> +	/* Disable clean/evict from being pushed to external */
> +	orr r1, r1, #(0x1<<3)
> +	mcr p15, 1, r1, c15, c0, 0
> +
> +	/* L2CTRL: L2 data RAM latency */
> +	mrc p15, 1, r1, c9, c0, 2
> +	bic r1, r1, #(0x7<<0)
> +	orr r1, r1, #(0x3<<0)
> +	mcr p15, 1, r1, c9, c0, 2
> +
> +	/* End of Cortex-A15 specific setup */
> +	not_a15:
> +
> +	/* Get value of sunxi_mc_smp_first_comer */
> +	adr	r1, first
> +	ldr	r0, [r1]
> +	ldr	r0, [r1, r0]
> +
> +	/* Skip cci_enable_port_for_self if not first comer */
> +	cmp	r0, #0
> +	bxeq	lr
> +	b	cci_enable_port_for_self
> +
> +	.align 2
> +	first: .word sunxi_mc_smp_first_comer - .
> +ENDPROC(sunxi_mc_smp_cluster_cache_enable)
> +
> +#ifdef CONFIG_SMP

I guess that whole file should be compiled only if we have SMP
enabled.

> +ENTRY(sunxi_boot)
> +	bl	sunxi_mc_smp_cluster_cache_enable
> +	b	secondary_startup
> +ENDPROC(sunxi_boot)
> +#endif
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index f2c2cfca28cd..4e807cc11a0f 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -82,6 +82,9 @@ static void __iomem *prcm_base;
>  static void __iomem *sram_b_smp_base;
>  static bool is_sun9i;
>  
> +extern void sunxi_boot(void);

Why did you change the name of that function? The older one made it
more obvious to tell what is going on.

> +extern void sunxi_cluster_cache_enable(void);

Is that used somewhere?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Fri, Feb 23, 2018 at 02:37:42PM +0100, Mylène Josserand wrote:
> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> It should be done by the bootloader but it is currently not the case,
> even for boot CPU because this SoC is booting in secure mode.
> It leads to an random offset value meaning that each CPU will have a
> different time, which isn't working very well.
> 
> Add assembly code used for boot CPU and secondary CPU cores to make
> sure that the CNTVOFF register is initialized.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index d5c97e945e69..605896251927 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>  	first: .word sunxi_mc_smp_first_comer - .
>  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>  
> +ENTRY(sunxi_init_cntvoff)
> +	/*
> +	 * CNTVOFF has to be initialized either from non-secure Hypervisor
> +	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> +	 * then it should be handled by the secure code
> +	 */
> +	cps	#MON_MODE
> +	mrc	p15, 0, r1, c1, c1, 0		/* Get Secure Config */
> +	orr	r0, r1, #1
> +	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
> +	instr_sync
> +	mov	r0, #0
> +	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
> +	instr_sync
> +	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
> +	instr_sync
> +	cps	#SVC_MODE
> +	ret	lr
> +ENDPROC(sunxi_init_cntvoff)
> +
>  #ifdef CONFIG_SMP
>  ENTRY(sunxi_boot)
>  	bl	sunxi_mc_smp_cluster_cache_enable
> +	bl	sunxi_init_cntvoff
>  	b	secondary_startup
>  ENDPROC(sunxi_boot)
>  
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..4bb041492b54 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>  };
>  
>  extern void __init sun6i_reset_init(void);
> +extern void sunxi_init_cntvoff(void);
> +
>  static void __init sun6i_timer_init(void)
>  {
> +	sunxi_init_cntvoff();
> +

This would deserve a comment explaining why this is needed in the
first place, and this is not correct. All the other SoCs listed in
that machine have their CNTVOFF setup correctly on CPU0, and since
Linux is booted in HYP, it's probably not even valid to do that.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Chen-Yu Tsai]

On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
<mylene.josserand@bootlin.com> wrote:
> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> It should be done by the bootloader but it is currently not the case,
> even for boot CPU because this SoC is booting in secure mode.
> It leads to an random offset value meaning that each CPU will have a
> different time, which isn't working very well.
>
> Add assembly code used for boot CPU and secondary CPU cores to make
> sure that the CNTVOFF register is initialized.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index d5c97e945e69..605896251927 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>         first: .word sunxi_mc_smp_first_comer - .
>  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>
> +ENTRY(sunxi_init_cntvoff)
> +       /*
> +        * CNTVOFF has to be initialized either from non-secure Hypervisor
> +        * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> +        * then it should be handled by the secure code
> +        */
> +       cps     #MON_MODE
> +       mrc     p15, 0, r1, c1, c1, 0           /* Get Secure Config */
> +       orr     r0, r1, #1
> +       mcr     p15, 0, r0, c1, c1, 0           /* Set Non Secure bit */
> +       instr_sync
> +       mov     r0, #0
> +       mcrr    p15, 4, r0, r0, c14             /* CNTVOFF = 0 */
> +       instr_sync
> +       mcr     p15, 0, r1, c1, c1, 0           /* Set Secure bit */
> +       instr_sync
> +       cps     #SVC_MODE
> +       ret     lr
> +ENDPROC(sunxi_init_cntvoff)

There is no need to move all the assembly into a separate file, just
to add this function. Everything can be inlined as a naked function.
The "instr_sync" macro can be replaced with "isb", which is what it
expands to anyway.

I really want to keep everything self-contained without global symbols,
and in C files if possible.

> +
>  #ifdef CONFIG_SMP
>  ENTRY(sunxi_boot)
>         bl      sunxi_mc_smp_cluster_cache_enable
> +       bl      sunxi_init_cntvoff
>         b       secondary_startup
>  ENDPROC(sunxi_boot)
>
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..4bb041492b54 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>  };
>
>  extern void __init sun6i_reset_init(void);
> +extern void sunxi_init_cntvoff(void);
> +
>  static void __init sun6i_timer_init(void)
>  {
> +       sunxi_init_cntvoff();

You should check the enable-method to see if PSCI is set or not,
as an indicator whether the kernel is booted secure or non-secure.
AFAIK trying to set CNTVOFF under non-secure would be very bad.


Regards
ChenYu

> +
>         of_clk_init(NULL);
>         if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
>                 sun6i_reset_init();
> --
> 2.11.0
>

Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Mylène Josserand]

Hi,

On Fri, 23 Feb 2018 15:54:23 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> Hi,
> 
> On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, create a new function
> > that handles all the resources needed on sun9i-a80 (because
> > it will be different from sun8i-a83t).
> > 
> > All the parsing of device tree is moved into this new function:
> > sun9i_dt_parsing. Create also a function to release the resources
> > retrieved during the dt parsing in case there is an error in init
> > function.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/mc_smp.c | 99 ++++++++++++++++++++++++++------------------
> >  1 file changed, 58 insertions(+), 41 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index 11e46c6efb90..650a2ad4398f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
> >  	return ret;
> >  }
> >  
> > -static int __init sunxi_mc_smp_init(void)
> > +static int sun9i_dt_parsing(struct resource res)  
> 
> Like I told you in the previous version, this should be _parse_dt, and
> not _dt_parsing.

Sorry, I forgot this review.

> 
> Also, I'm not sure why you are passing by copy the resource structure?
> 
> >  {
> > -	struct device_node *cpucfg_node, *sram_node, *node;
> > -	struct resource res;
> > +	struct device_node *prcm_node, *cpucfg_node, *sram_node;
> >  	int ret;
> >  
> > -	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> > -		return -ENODEV;
> > -
> > -	if (!sunxi_mc_smp_cpu_table_init())
> > -		return -EINVAL;
> > -
> > -	if (!cci_probed()) {
> > -		pr_err("%s: CCI-400 not available\n", __func__);
> > -		return -ENODEV;
> > -	}
> > -
> > -	node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> > -	if (!node) {
> > -		pr_err("%s: PRCM not available\n", __func__);
> > +	prcm_node = of_find_compatible_node(NULL, NULL,
> > +				       "allwinner,sun9i-a80-prcm");
> > +	if (!prcm_node)
> >  		return -ENODEV;
> > -	}
> >  
> >  	/*
> >  	 * Unfortunately we can not request the I/O region for the PRCM.
> >  	 * It is shared with the PRCM clock.
> >  	 */
> > -	prcm_base = of_iomap(node, 0);
> > -	of_node_put(node);
> > +	prcm_base = of_iomap(prcm_node, 0);  
> 
> So it does more a bit more than just parsing the DT?
> 
> Can you maybe just fill the DT nodes and have the common part map the
> memory regions if the pointer is not NULL?

Yep, I will rework this function to only parse device tree nodes.

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 02/10] ARM: sun9i: smp: Rename clusters's power-off register

[Mylène Josserand]

Hi,

On Fri, 23 Feb 2018 15:56:50 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> On Fri, Feb 23, 2018 at 02:37:34PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, rename the variable name
> > that handles the power-off of clusters because it is different from
> > sun9i-a80 to sun8i-a83t.
> >
> > The power off register for clusters are different from SUN9I and SUN8I  
> 
> This is just a nitpick, and I see you use them everywhere, but sun8i
> and sun9i are the family of SoCs, and A80 and A83t the name of SoCs
> within their respective families.
> 
> So in your commit log, you want to support the A83t because it's
> different than the A80, and the sun9i and sun8i mentionned in your
> last sentence is not really meaningful.

True, I will pay attention next time.

> 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>  
> 
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Maxime
> 

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 03/10] ARM: sun8i: smp: Add support for A83T

[Mylène Josserand]

Hi,

On Fri, 23 Feb 2018 16:03:05 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> On Fri, Feb 23, 2018 at 02:37:35PM +0100, Mylène Josserand wrote:
> > Add the support for A83T.
> > 
> > A83T SoC has an additional register than A80 to handle CPU configurations:
> > R_CPUS_CFG. Information about the register comes from Allwinner's BSP
> > driver.
> > An important difference is the Power Off Gating register for clusters
> > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/Kconfig  |   2 +-
> >  arch/arm/mach-sunxi/mc_smp.c | 168 +++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 162 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index ce53ceaf4cc5..a0ad35c41c02 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -51,7 +51,7 @@ config MACH_SUN9I
> >  config ARCH_SUNXI_MC_SMP
> >  	bool
> >  	depends on SMP
> > -	default MACH_SUN9I
> > +	default y if MACH_SUN9I || MACH_SUN8I
> >  	select ARM_CCI400_PORT_CTRL
> >  	select ARM_CPU_SUSPEND
> >  
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index de02e5662557..3bd9066a1422 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -55,22 +55,32 @@
> >  #define CPUCFG_CX_RST_CTRL_L2_RST	BIT(8)
> >  #define CPUCFG_CX_RST_CTRL_CX_RST(n)	BIT(4 + (n))
> >  #define CPUCFG_CX_RST_CTRL_CORE_RST(n)	BIT(n)
> > +#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL	(0xf << 0)
> >  
> >  #define PRCM_CPU_PO_RST_CTRL(c)		(0x4 + 0x4 * (c))
> >  #define PRCM_CPU_PO_RST_CTRL_CORE(n)	BIT(n)
> >  #define PRCM_CPU_PO_RST_CTRL_CORE_ALL	0xf
> >  #define PRCM_PWROFF_GATING_REG(c)	(0x100 + 0x4 * (c))
> > +/* The power off register for clusters are different from SUN9I and SUN8I */
> > +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I	BIT(0)
> >  #define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I	BIT(4)
> >  #define PRCM_PWROFF_GATING_REG_CORE(n)	BIT(n)
> >  #define PRCM_PWR_SWITCH_REG(c, cpu)	(0x140 + 0x10 * (c) + 0x4 * (cpu))
> >  #define PRCM_CPU_SOFT_ENTRY_REG		0x164
> >  
> > +/* R_CPUCFG registers, specific to SUN8I */
> > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)	(0x30 + (c) * 0x4)
> > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n)	BIT(n)
> > +#define R_CPUCFG_CPU_SOFT_ENTRY_REG	0x01a4
> > +
> >  #define CPU0_SUPPORT_HOTPLUG_MAGIC0	0xFA50392F
> >  #define CPU0_SUPPORT_HOTPLUG_MAGIC1	0x790DCA3A
> >  
> >  static void __iomem *cpucfg_base;
> > +static void __iomem *r_cpucfg_base;
> >  static void __iomem *prcm_base;
> >  static void __iomem *sram_b_smp_base;
> > +static bool is_sun9i;  
> 
> Since you always check for that condition to always be false, can't
> you do the opposite, ie have it called is_a83t, and verify it to be
> true?

Yes, I will switch the test.

> 
> >  	/* Set the hardware entry point address */
> > -	writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > -	       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> > +	if (is_sun9i)
> > +		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > +		       prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> > +	else
> > +		writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > +		       r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG);  
> 
> if (is_a83t)
> 	reg = prcm_base + PRCM_CPU_SOFT_ENTRY_REG;
> else
> 	reg = r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG;
> writel(__pa_symbol(sunxi_mc_smp_secondary_startup), reg);
> 

Make sense, thanks for the review.

Mylene

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 08/10] ARM: sunxi: smp: Move assembly code into a file

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

Hi Mylène,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20180223]
[cannot apply to arm-soc/for-next robh/for-next linux-rpi/for-rpi-next v4.16-rc2 v4.16-rc1 v4.15 v4.16-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/Sunxi-Add-SMP-support-on-A83T/20180226-035312
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   arch/arm/mach-sunxi/headsmp.S: Assembler messages:
>> arch/arm/mach-sunxi/headsmp.S:22: Error: selected processor does not support `movw r2,#(0xff00fff0&0xffff)' in ARM mode
>> arch/arm/mach-sunxi/headsmp.S:23: Error: selected processor does not support `movt r2,#(0xff00fff0>>16)' in ARM mode
   arch/arm/mach-sunxi/headsmp.S:25: Error: selected processor does not support `movw r2,#(0x4100c0f0&0xffff)' in ARM mode
   arch/arm/mach-sunxi/headsmp.S:26: Error: selected processor does not support `movt r2,#(0x4100c0f0>>16)' in ARM mode

vim +22 arch/arm/mach-sunxi/headsmp.S

    13	
    14	ENTRY(sunxi_mc_smp_cluster_cache_enable)
    15		/*
    16		* Enable cluster-level coherency, in preparation for turning on the MMU.
    17		*
    18		* Also enable regional clock gating and L2 data latency settings for
    19		* Cortex-A15. These settings are from the vendor kernel.
    20		*/
    21		mrc	p15, 0, r1, c0, c0, 0
  > 22		movw	r2, #(0xff00fff0&0xffff)
  > 23		movt	r2, #(0xff00fff0>>16)
    24		and	r1, r1, r2
    25		movw	r2, #(0x4100c0f0&0xffff)
    26		movt	r2, #(0x4100c0f0>>16)
    27		cmp	r1, r2
    28		bne	not_a15
    29	
    30		/* The following is Cortex-A15 specific */
    31	
    32		/* ACTLR2: Enable CPU regional clock gates */
    33		mrc p15, 1, r1, c15, c0, 4
    34		orr r1, r1, #(0x1<<31)
    35		mcr p15, 1, r1, c15, c0, 4
    36	
    37		/* L2ACTLR */
    38		mrc p15, 1, r1, c15, c0, 0
    39		/* Enable L2, GIC, and Timer regional clock gates */
    40		orr r1, r1, #(0x1<<26)
    41		/* Disable clean/evict from being pushed to external */
    42		orr r1, r1, #(0x1<<3)
    43		mcr p15, 1, r1, c15, c0, 0
    44	
    45		/* L2CTRL: L2 data RAM latency */
    46		mrc p15, 1, r1, c9, c0, 2
    47		bic r1, r1, #(0x7<<0)
    48		orr r1, r1, #(0x3<<0)
    49		mcr p15, 1, r1, c9, c0, 2
    50	
    51		/* End of Cortex-A15 specific setup */
    52		not_a15:
    53	
    54		/* Get value of sunxi_mc_smp_first_comer */
    55		adr	r1, first
    56		ldr	r0, [r1]
    57		ldr	r0, [r1, r0]
    58	
    59		/* Skip cci_enable_port_for_self if not first comer */
    60		cmp	r0, #0
    61		bxeq	lr
    62		b	cci_enable_port_for_self
    63	
    64		.align 2
    65		first: .word sunxi_mc_smp_first_comer - .
    66	ENDPROC(sunxi_mc_smp_cluster_cache_enable)
    67	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65297 bytes --]

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4112 bytes --]

On Sat, Feb 24, 2018 at 12:17:13AM +0800, Chen-Yu Tsai wrote:
> On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
> <mylene.josserand@bootlin.com> wrote:
> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> > It should be done by the bootloader but it is currently not the case,
> > even for boot CPU because this SoC is booting in secure mode.
> > It leads to an random offset value meaning that each CPU will have a
> > different time, which isn't working very well.
> >
> > Add assembly code used for boot CPU and secondary CPU cores to make
> > sure that the CNTVOFF register is initialized.
> >
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
> >  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> > index d5c97e945e69..605896251927 100644
> > --- a/arch/arm/mach-sunxi/headsmp.S
> > +++ b/arch/arm/mach-sunxi/headsmp.S
> > @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
> >         first: .word sunxi_mc_smp_first_comer - .
> >  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
> >
> > +ENTRY(sunxi_init_cntvoff)
> > +       /*
> > +        * CNTVOFF has to be initialized either from non-secure Hypervisor
> > +        * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> > +        * then it should be handled by the secure code
> > +        */
> > +       cps     #MON_MODE
> > +       mrc     p15, 0, r1, c1, c1, 0           /* Get Secure Config */
> > +       orr     r0, r1, #1
> > +       mcr     p15, 0, r0, c1, c1, 0           /* Set Non Secure bit */
> > +       instr_sync
> > +       mov     r0, #0
> > +       mcrr    p15, 4, r0, r0, c14             /* CNTVOFF = 0 */
> > +       instr_sync
> > +       mcr     p15, 0, r1, c1, c1, 0           /* Set Secure bit */
> > +       instr_sync
> > +       cps     #SVC_MODE
> > +       ret     lr
> > +ENDPROC(sunxi_init_cntvoff)
> 
> There is no need to move all the assembly into a separate file, just
> to add this function. Everything can be inlined as a naked function.
> The "instr_sync" macro can be replaced with "isb", which is what it
> expands to anyway.
> 
> I really want to keep everything self-contained without global symbols,
> and in C files if possible.

What is the rationale for keeping it in C files (beside the global
symbols)? Because the syntax is quite ugly, and it's much easier to
read, review and amend using a separate file.

> >  #ifdef CONFIG_SMP
> >  ENTRY(sunxi_boot)
> >         bl      sunxi_mc_smp_cluster_cache_enable
> > +       bl      sunxi_init_cntvoff
> >         b       secondary_startup
> >  ENDPROC(sunxi_boot)
> >
> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> > index 5e9602ce1573..4bb041492b54 100644
> > --- a/arch/arm/mach-sunxi/sunxi.c
> > +++ b/arch/arm/mach-sunxi/sunxi.c
> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> >  };
> >
> >  extern void __init sun6i_reset_init(void);
> > +extern void sunxi_init_cntvoff(void);
> > +
> >  static void __init sun6i_timer_init(void)
> >  {
> > +       sunxi_init_cntvoff();
> 
> You should check the enable-method to see if PSCI is set or not,
> as an indicator whether the kernel is booted secure or non-secure.

It's an indicator, but it's not really a perfect one. You could very
well have your kernel booted in non-secure, without PSCI. Or even with
PSCI, but without the SMP ops.

We have a quite big number of these cases already, where, depending on
the configuration, we might not have access to the device we write to,
the number of hacks to just enable that device for non-secure is a
good example of that.

> AFAIK trying to set CNTVOFF under non-secure would be very bad.

Just like any other access we do :/

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Chen-Yu Tsai]

On Mon, Feb 26, 2018 at 6:12 PM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Sat, Feb 24, 2018 at 12:17:13AM +0800, Chen-Yu Tsai wrote:
>> On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
>> <mylene.josserand@bootlin.com> wrote:
>> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
>> > It should be done by the bootloader but it is currently not the case,
>> > even for boot CPU because this SoC is booting in secure mode.
>> > It leads to an random offset value meaning that each CPU will have a
>> > different time, which isn't working very well.
>> >
>> > Add assembly code used for boot CPU and secondary CPU cores to make
>> > sure that the CNTVOFF register is initialized.
>> >
>> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
>> > ---
>> >  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>> >  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>> >  2 files changed, 25 insertions(+)
>> >
>> > diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
>> > index d5c97e945e69..605896251927 100644
>> > --- a/arch/arm/mach-sunxi/headsmp.S
>> > +++ b/arch/arm/mach-sunxi/headsmp.S
>> > @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>> >         first: .word sunxi_mc_smp_first_comer - .
>> >  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>> >
>> > +ENTRY(sunxi_init_cntvoff)
>> > +       /*
>> > +        * CNTVOFF has to be initialized either from non-secure Hypervisor
>> > +        * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
>> > +        * then it should be handled by the secure code
>> > +        */
>> > +       cps     #MON_MODE
>> > +       mrc     p15, 0, r1, c1, c1, 0           /* Get Secure Config */
>> > +       orr     r0, r1, #1
>> > +       mcr     p15, 0, r0, c1, c1, 0           /* Set Non Secure bit */
>> > +       instr_sync
>> > +       mov     r0, #0
>> > +       mcrr    p15, 4, r0, r0, c14             /* CNTVOFF = 0 */
>> > +       instr_sync
>> > +       mcr     p15, 0, r1, c1, c1, 0           /* Set Secure bit */
>> > +       instr_sync
>> > +       cps     #SVC_MODE
>> > +       ret     lr
>> > +ENDPROC(sunxi_init_cntvoff)
>>
>> There is no need to move all the assembly into a separate file, just
>> to add this function. Everything can be inlined as a naked function.
>> The "instr_sync" macro can be replaced with "isb", which is what it
>> expands to anyway.
>>
>> I really want to keep everything self-contained without global symbols,
>> and in C files if possible.
>
> What is the rationale for keeping it in C files (beside the global
> symbols)? Because the syntax is quite ugly, and it's much easier to
> read, review and amend using a separate file.

Global symbols and keeping everything in one place I guess.
This symbol would be used in a few places, so I suppose having it
in a separate assembly file would be OK.

>> >  #ifdef CONFIG_SMP
>> >  ENTRY(sunxi_boot)
>> >         bl      sunxi_mc_smp_cluster_cache_enable
>> > +       bl      sunxi_init_cntvoff
>> >         b       secondary_startup
>> >  ENDPROC(sunxi_boot)
>> >
>> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
>> > index 5e9602ce1573..4bb041492b54 100644
>> > --- a/arch/arm/mach-sunxi/sunxi.c
>> > +++ b/arch/arm/mach-sunxi/sunxi.c
>> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>> >  };
>> >
>> >  extern void __init sun6i_reset_init(void);
>> > +extern void sunxi_init_cntvoff(void);
>> > +
>> >  static void __init sun6i_timer_init(void)
>> >  {
>> > +       sunxi_init_cntvoff();
>>
>> You should check the enable-method to see if PSCI is set or not,
>> as an indicator whether the kernel is booted secure or non-secure.
>
> It's an indicator, but it's not really a perfect one. You could very
> well have your kernel booted in non-secure, without PSCI. Or even with
> PSCI, but without the SMP ops.
>
> We have a quite big number of these cases already, where, depending on
> the configuration, we might not have access to the device we write to,
> the number of hacks to just enable that device for non-secure is a
> good example of that.

I wouldn't consider them hacks though. The hardware gives the option
to have control of many devices delegated solely to secure-only, or
secure/non-secure. Our present model is to support everything we can
in Linux directly, instead of through some firmware interface to a
non-existent firmware.

ChenYu

>> AFAIK trying to set CNTVOFF under non-secure would be very bad.
>
> Just like any other access we do :/
>
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v4 08/10] ARM: sunxi: smp: Move assembly code into a file

[Mylène Josserand]

Hello,

On Fri, 23 Feb 2018 16:09:49 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> On Fri, Feb 23, 2018 at 02:37:40PM +0100, Mylène Josserand wrote:
> > Move the assembly code for cluster cache enabling
> > into an assembly file instead of having it directly in C code.
> > 
> > Create a sunxi_boot entry that will perform a cluster cached
> > enabling and called secondary_startup.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/Makefile  |  1 +
> >  arch/arm/mach-sunxi/headsmp.S | 73 +++++++++++++++++++++++++++++++++++++++++
> >  arch/arm/mach-sunxi/mc_smp.c  | 76 ++++---------------------------------------
> >  3 files changed, 80 insertions(+), 70 deletions(-)
> >  create mode 100644 arch/arm/mach-sunxi/headsmp.S
> > 
> > diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> > index 7de9cc286d53..d1a072b879ed 100644
> > --- a/arch/arm/mach-sunxi/Makefile
> > +++ b/arch/arm/mach-sunxi/Makefile
> > @@ -1,5 +1,6 @@
> >  CFLAGS_mc_smp.o	+= -march=armv7-a
> >  
> >  obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
> > +obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
> >  obj-$(CONFIG_ARCH_SUNXI_MC_SMP) += mc_smp.o
> >  obj-$(CONFIG_SMP) += platsmp.o
> > diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> > new file mode 100644
> > index 000000000000..4f5957a6e188
> > --- /dev/null
> > +++ b/arch/arm/mach-sunxi/headsmp.S
> > @@ -0,0 +1,73 @@
> > +/*
> > + * SMP support for sunxi based systems with Cortex A7/A15
> > + *
> > + * Copyright (C) 2018 Bootlin  
> 
> This is just a copy, and while you can claim that you are one of the
> copyrights holder, you are not the sole one and the original author
> should be there.

yep, sorry about that.

> 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.  
> 
> You want to use SPDX there instead.

Sure, I will update it.

> 
> > + */
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/assembler.h>
> > +
> > +ENTRY(sunxi_mc_smp_cluster_cache_enable)
> > +	/*
> > +	* Enable cluster-level coherency, in preparation for turning on the MMU.
> > +	*
> > +	* Also enable regional clock gating and L2 data latency settings for
> > +	* Cortex-A15. These settings are from the vendor kernel.
> > +	*/  
> 
> The indentation is not correct there, the * should be aligned

Okay

> 
> > +	mrc	p15, 0, r1, c0, c0, 0
> > +	movw	r2, #(0xff00fff0&0xffff)
> > +	movt	r2, #(0xff00fff0>>16)  
> 
> This used to be defines, we should keep them, and we should have
> spaces around the operators as well.

Okay 

> 
> > +	and	r1, r1, r2
> > +	movw	r2, #(0x4100c0f0&0xffff)
> > +	movt	r2, #(0x4100c0f0>>16)
> > +	cmp	r1, r2
> > +	bne	not_a15
> > +
> > +	/* The following is Cortex-A15 specific */
> > +
> > +	/* ACTLR2: Enable CPU regional clock gates */
> > +	mrc p15, 1, r1, c15, c0, 4
> > +	orr r1, r1, #(0x1<<31)
> > +	mcr p15, 1, r1, c15, c0, 4
> > +
> > +	/* L2ACTLR */
> > +	mrc p15, 1, r1, c15, c0, 0
> > +	/* Enable L2, GIC, and Timer regional clock gates */
> > +	orr r1, r1, #(0x1<<26)
> > +	/* Disable clean/evict from being pushed to external */
> > +	orr r1, r1, #(0x1<<3)
> > +	mcr p15, 1, r1, c15, c0, 0
> > +
> > +	/* L2CTRL: L2 data RAM latency */
> > +	mrc p15, 1, r1, c9, c0, 2
> > +	bic r1, r1, #(0x7<<0)
> > +	orr r1, r1, #(0x3<<0)
> > +	mcr p15, 1, r1, c9, c0, 2
> > +
> > +	/* End of Cortex-A15 specific setup */
> > +	not_a15:
> > +
> > +	/* Get value of sunxi_mc_smp_first_comer */
> > +	adr	r1, first
> > +	ldr	r0, [r1]
> > +	ldr	r0, [r1, r0]
> > +
> > +	/* Skip cci_enable_port_for_self if not first comer */
> > +	cmp	r0, #0
> > +	bxeq	lr
> > +	b	cci_enable_port_for_self
> > +
> > +	.align 2
> > +	first: .word sunxi_mc_smp_first_comer - .
> > +ENDPROC(sunxi_mc_smp_cluster_cache_enable)
> > +
> > +#ifdef CONFIG_SMP  
> 
> I guess that whole file should be compiled only if we have SMP
> enabled.

True, thanks

> 
> > +ENTRY(sunxi_boot)
> > +	bl	sunxi_mc_smp_cluster_cache_enable
> > +	b	secondary_startup
> > +ENDPROC(sunxi_boot)
> > +#endif
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index f2c2cfca28cd..4e807cc11a0f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -82,6 +82,9 @@ static void __iomem *prcm_base;
> >  static void __iomem *sram_b_smp_base;
> >  static bool is_sun9i;
> >  
> > +extern void sunxi_boot(void);  
> 
> Why did you change the name of that function? The older one made it
> more obvious to tell what is going on.

Okay, I will use the old name.

> 
> > +extern void sunxi_cluster_cache_enable(void);  
> 
> Is that used somewhere?

No, I will remove it.

> 
> Thanks!
> Maxime
> 

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Mylène Josserand]

Hello,

On Mon, 26 Feb 2018 18:25:10 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Mon, Feb 26, 2018 at 6:12 PM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > On Sat, Feb 24, 2018 at 12:17:13AM +0800, Chen-Yu Tsai wrote:  
> >> On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
> >> <mylene.josserand@bootlin.com> wrote:  
> >> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> >> > It should be done by the bootloader but it is currently not the case,
> >> > even for boot CPU because this SoC is booting in secure mode.
> >> > It leads to an random offset value meaning that each CPU will have a
> >> > different time, which isn't working very well.
> >> >
> >> > Add assembly code used for boot CPU and secondary CPU cores to make
> >> > sure that the CNTVOFF register is initialized.
> >> >
> >> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> >> > ---
> >> >  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
> >> >  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
> >> >  2 files changed, 25 insertions(+)
> >> >
> >> > diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> >> > index d5c97e945e69..605896251927 100644
> >> > --- a/arch/arm/mach-sunxi/headsmp.S
> >> > +++ b/arch/arm/mach-sunxi/headsmp.S
> >> > @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
> >> >         first: .word sunxi_mc_smp_first_comer - .
> >> >  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
> >> >
> >> > +ENTRY(sunxi_init_cntvoff)
> >> > +       /*
> >> > +        * CNTVOFF has to be initialized either from non-secure Hypervisor
> >> > +        * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> >> > +        * then it should be handled by the secure code
> >> > +        */
> >> > +       cps     #MON_MODE
> >> > +       mrc     p15, 0, r1, c1, c1, 0           /* Get Secure Config */
> >> > +       orr     r0, r1, #1
> >> > +       mcr     p15, 0, r0, c1, c1, 0           /* Set Non Secure bit */
> >> > +       instr_sync
> >> > +       mov     r0, #0
> >> > +       mcrr    p15, 4, r0, r0, c14             /* CNTVOFF = 0 */
> >> > +       instr_sync
> >> > +       mcr     p15, 0, r1, c1, c1, 0           /* Set Secure bit */
> >> > +       instr_sync
> >> > +       cps     #SVC_MODE
> >> > +       ret     lr
> >> > +ENDPROC(sunxi_init_cntvoff)  
> >>
> >> There is no need to move all the assembly into a separate file, just
> >> to add this function. Everything can be inlined as a naked function.
> >> The "instr_sync" macro can be replaced with "isb", which is what it
> >> expands to anyway.
> >>
> >> I really want to keep everything self-contained without global symbols,
> >> and in C files if possible.  
> >
> > What is the rationale for keeping it in C files (beside the global
> > symbols)? Because the syntax is quite ugly, and it's much easier to
> > read, review and amend using a separate file.  
> 
> Global symbols and keeping everything in one place I guess.
> This symbol would be used in a few places, so I suppose having it
> in a separate assembly file would be OK.

Okay so I will keep it in a separate file.

> 
> >> >  #ifdef CONFIG_SMP
> >> >  ENTRY(sunxi_boot)
> >> >         bl      sunxi_mc_smp_cluster_cache_enable
> >> > +       bl      sunxi_init_cntvoff
> >> >         b       secondary_startup
> >> >  ENDPROC(sunxi_boot)
> >> >
> >> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> >> > index 5e9602ce1573..4bb041492b54 100644
> >> > --- a/arch/arm/mach-sunxi/sunxi.c
> >> > +++ b/arch/arm/mach-sunxi/sunxi.c
> >> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> >> >  };
> >> >
> >> >  extern void __init sun6i_reset_init(void);
> >> > +extern void sunxi_init_cntvoff(void);
> >> > +
> >> >  static void __init sun6i_timer_init(void)
> >> >  {
> >> > +       sunxi_init_cntvoff();  
> >>
> >> You should check the enable-method to see if PSCI is set or not,
> >> as an indicator whether the kernel is booted secure or non-secure.  
> >
> > It's an indicator, but it's not really a perfect one. You could very
> > well have your kernel booted in non-secure, without PSCI. Or even with
> > PSCI, but without the SMP ops.
> >
> > We have a quite big number of these cases already, where, depending on
> > the configuration, we might not have access to the device we write to,
> > the number of hacks to just enable that device for non-secure is a
> > good example of that.  
> 
> I wouldn't consider them hacks though. The hardware gives the option
> to have control of many devices delegated solely to secure-only, or
> secure/non-secure. Our present model is to support everything we can
> in Linux directly, instead of through some firmware interface to a
> non-existent firmware.

I am not sure to understand what is the conclusion about it.
Should I use "psci"/enable-method or should I use another mechanism to
detect we are in secure/non-secure (if it exists)?

Otherwise, for the moment, I can use machine-compatible on sun8i-a83t
and we will see later how we can handle it in a better way.

Thank you in advance,

Best regards,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com


> 
> ChenYu
> 
> >> AFAIK trying to set CNTVOFF under non-secure would be very bad.  
> >
> > Just like any other access we do :/
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com  

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

On Mon, Mar 05, 2018 at 08:51:48AM +0100, Mylène Josserand wrote:
> > >> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> > >> > index 5e9602ce1573..4bb041492b54 100644
> > >> > --- a/arch/arm/mach-sunxi/sunxi.c
> > >> > +++ b/arch/arm/mach-sunxi/sunxi.c
> > >> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> > >> >  };
> > >> >
> > >> >  extern void __init sun6i_reset_init(void);
> > >> > +extern void sunxi_init_cntvoff(void);
> > >> > +
> > >> >  static void __init sun6i_timer_init(void)
> > >> >  {
> > >> > +       sunxi_init_cntvoff();  
> > >>
> > >> You should check the enable-method to see if PSCI is set or not,
> > >> as an indicator whether the kernel is booted secure or non-secure.  
> > >
> > > It's an indicator, but it's not really a perfect one. You could very
> > > well have your kernel booted in non-secure, without PSCI. Or even with
> > > PSCI, but without the SMP ops.
> > >
> > > We have a quite big number of these cases already, where, depending on
> > > the configuration, we might not have access to the device we write to,
> > > the number of hacks to just enable that device for non-secure is a
> > > good example of that.  
> > 
> > I wouldn't consider them hacks though. The hardware gives the option
> > to have control of many devices delegated solely to secure-only, or
> > secure/non-secure. Our present model is to support everything we can
> > in Linux directly, instead of through some firmware interface to a
> > non-existent firmware.
> 
> I am not sure to understand what is the conclusion about it.
> Should I use "psci"/enable-method or should I use another mechanism to
> detect we are in secure/non-secure (if it exists)?
> 
> Otherwise, for the moment, I can use machine-compatible on sun8i-a83t
> and we will see later how we can handle it in a better way.

Can't we have another approach here?

If we use an enable-method (and we should), instead of having it tied
to the machine compatible, the SMP setup code will run only if our
enable-method is the one we set up. If PSCI is in use, the
enable-method is not going to be the one defined here, and the code
will not run.

So why not just move that call to the SMP ops setup function, just
like renesas does?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Mylène Josserand]

Hello,

On Mon, 5 Mar 2018 09:31:14 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> On Mon, Mar 05, 2018 at 08:51:48AM +0100, Mylène Josserand wrote:
> > > >> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> > > >> > index 5e9602ce1573..4bb041492b54 100644
> > > >> > --- a/arch/arm/mach-sunxi/sunxi.c
> > > >> > +++ b/arch/arm/mach-sunxi/sunxi.c
> > > >> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> > > >> >  };
> > > >> >
> > > >> >  extern void __init sun6i_reset_init(void);
> > > >> > +extern void sunxi_init_cntvoff(void);
> > > >> > +
> > > >> >  static void __init sun6i_timer_init(void)
> > > >> >  {
> > > >> > +       sunxi_init_cntvoff();    
> > > >>
> > > >> You should check the enable-method to see if PSCI is set or not,
> > > >> as an indicator whether the kernel is booted secure or non-secure.    
> > > >
> > > > It's an indicator, but it's not really a perfect one. You could very
> > > > well have your kernel booted in non-secure, without PSCI. Or even with
> > > > PSCI, but without the SMP ops.
> > > >
> > > > We have a quite big number of these cases already, where, depending on
> > > > the configuration, we might not have access to the device we write to,
> > > > the number of hacks to just enable that device for non-secure is a
> > > > good example of that.    
> > > 
> > > I wouldn't consider them hacks though. The hardware gives the option
> > > to have control of many devices delegated solely to secure-only, or
> > > secure/non-secure. Our present model is to support everything we can
> > > in Linux directly, instead of through some firmware interface to a
> > > non-existent firmware.  
> > 
> > I am not sure to understand what is the conclusion about it.
> > Should I use "psci"/enable-method or should I use another mechanism to
> > detect we are in secure/non-secure (if it exists)?
> > 
> > Otherwise, for the moment, I can use machine-compatible on sun8i-a83t
> > and we will see later how we can handle it in a better way.  
> 
> Can't we have another approach here?
> 
> If we use an enable-method (and we should), instead of having it tied
> to the machine compatible, the SMP setup code will run only if our
> enable-method is the one we set up. If PSCI is in use, the
> enable-method is not going to be the one defined here, and the code
> will not run.
> 
> So why not just move that call to the SMP ops setup function, just
> like renesas does?
> 
> Maxime
> 

Okay, I will update my series and handle the differences using
enable-method instead of machine-compatible.

Best regards,

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Marc Zyngier]

On 23/02/18 16:17, Chen-Yu Tsai wrote:
> On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
> <mylene.josserand@bootlin.com> wrote:
>> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
>> It should be done by the bootloader but it is currently not the case,
>> even for boot CPU because this SoC is booting in secure mode.
>> It leads to an random offset value meaning that each CPU will have a
>> different time, which isn't working very well.
>>
>> Add assembly code used for boot CPU and secondary CPU cores to make
>> sure that the CNTVOFF register is initialized.
>>
>> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
>> ---
>>  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>>  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
>> index d5c97e945e69..605896251927 100644
>> --- a/arch/arm/mach-sunxi/headsmp.S
>> +++ b/arch/arm/mach-sunxi/headsmp.S
>> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>>         first: .word sunxi_mc_smp_first_comer - .
>>  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>>
>> +ENTRY(sunxi_init_cntvoff)
>> +       /*
>> +        * CNTVOFF has to be initialized either from non-secure Hypervisor
>> +        * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
>> +        * then it should be handled by the secure code
>> +        */
>> +       cps     #MON_MODE
>> +       mrc     p15, 0, r1, c1, c1, 0           /* Get Secure Config */
>> +       orr     r0, r1, #1
>> +       mcr     p15, 0, r0, c1, c1, 0           /* Set Non Secure bit */
>> +       instr_sync
>> +       mov     r0, #0
>> +       mcrr    p15, 4, r0, r0, c14             /* CNTVOFF = 0 */
>> +       instr_sync
>> +       mcr     p15, 0, r1, c1, c1, 0           /* Set Secure bit */
>> +       instr_sync
>> +       cps     #SVC_MODE
>> +       ret     lr
>> +ENDPROC(sunxi_init_cntvoff)
> 
> There is no need to move all the assembly into a separate file, just
> to add this function. Everything can be inlined as a naked function.
> The "instr_sync" macro can be replaced with "isb", which is what it
> expands to anyway.
> 
> I really want to keep everything self-contained without global symbols,
> and in C files if possible.
> 
>> +
>>  #ifdef CONFIG_SMP
>>  ENTRY(sunxi_boot)
>>         bl      sunxi_mc_smp_cluster_cache_enable
>> +       bl      sunxi_init_cntvoff
>>         b       secondary_startup
>>  ENDPROC(sunxi_boot)
>>
>> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
>> index 5e9602ce1573..4bb041492b54 100644
>> --- a/arch/arm/mach-sunxi/sunxi.c
>> +++ b/arch/arm/mach-sunxi/sunxi.c
>> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>>  };
>>
>>  extern void __init sun6i_reset_init(void);
>> +extern void sunxi_init_cntvoff(void);
>> +
>>  static void __init sun6i_timer_init(void)
>>  {
>> +       sunxi_init_cntvoff();
> 
> You should check the enable-method to see if PSCI is set or not,
> as an indicator whether the kernel is booted secure or non-secure.
> AFAIK trying to set CNTVOFF under non-secure would be very bad.

Absolutely not. CNTVOFF *is* a non-secure register. The fact that it is
not accessible from NS-PL1 is another problem. Please don't conflate the
two things together.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Marc Zyngier]

On 23/02/18 13:37, Mylène Josserand wrote:
> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.

Only on A7? Is that specific to your platform?

> It should be done by the bootloader but it is currently not the case,
> even for boot CPU because this SoC is booting in secure mode.
> It leads to an random offset value meaning that each CPU will have a
> different time, which isn't working very well.
> 
> Add assembly code used for boot CPU and secondary CPU cores to make
> sure that the CNTVOFF register is initialized.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index d5c97e945e69..605896251927 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>  	first: .word sunxi_mc_smp_first_comer - .
>  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>  
> +ENTRY(sunxi_init_cntvoff)
> +	/*
> +	 * CNTVOFF has to be initialized either from non-secure Hypervisor
> +	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> +	 * then it should be handled by the secure code
> +	 */
> +	cps	#MON_MODE
> +	mrc	p15, 0, r1, c1, c1, 0		/* Get Secure Config */
> +	orr	r0, r1, #1
> +	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
> +	instr_sync

Since these CPUs are all ARMv7, you can use isb directly. Nobody wants
to see more of the CP15 barriers.

> +	mov	r0, #0
> +	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
> +	instr_sync
> +	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
> +	instr_sync
> +	cps	#SVC_MODE
> +	ret	lr

Given that this code is identical to the shmobile hack, it'd be good to
make it common, one way or another.

> +ENDPROC(sunxi_init_cntvoff)
> +
>  #ifdef CONFIG_SMP
>  ENTRY(sunxi_boot)
>  	bl	sunxi_mc_smp_cluster_cache_enable
> +	bl	sunxi_init_cntvoff
>  	b	secondary_startup
>  ENDPROC(sunxi_boot)
>  
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..4bb041492b54 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>  };
>  
>  extern void __init sun6i_reset_init(void);
> +extern void sunxi_init_cntvoff(void);
> +
>  static void __init sun6i_timer_init(void)
>  {
> +	sunxi_init_cntvoff();
> +

It is slightly odd that some CPUs get initialized from the early asm
code, and some others do a detour via some C code. I'm sure this could
be made to work

>  	of_clk_init(NULL);
>  	if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
>  		sun6i_reset_init();
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Mylène Josserand]

Hello Mark,

Please, excuse me for this late answer and thank you for the review!

On Wed, 7 Mar 2018 12:18:33 +0000
Marc Zyngier <marc.zyngier@arm.com> wrote:

> On 23/02/18 13:37, Mylène Josserand wrote:
> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.  
> 
> Only on A7? Is that specific to your platform?

I do not really know other Allwinner's platforms about this subject. At
least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
Maxime could help us on it.

> 
> > It should be done by the bootloader but it is currently not the case,
> > even for boot CPU because this SoC is booting in secure mode.
> > It leads to an random offset value meaning that each CPU will have a
> > different time, which isn't working very well.
> > 
> > Add assembly code used for boot CPU and secondary CPU cores to make
> > sure that the CNTVOFF register is initialized.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
> >  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> > index d5c97e945e69..605896251927 100644
> > --- a/arch/arm/mach-sunxi/headsmp.S
> > +++ b/arch/arm/mach-sunxi/headsmp.S
> > @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
> >  	first: .word sunxi_mc_smp_first_comer - .
> >  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
> >  
> > +ENTRY(sunxi_init_cntvoff)
> > +	/*
> > +	 * CNTVOFF has to be initialized either from non-secure Hypervisor
> > +	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> > +	 * then it should be handled by the secure code
> > +	 */
> > +	cps	#MON_MODE
> > +	mrc	p15, 0, r1, c1, c1, 0		/* Get Secure Config */
> > +	orr	r0, r1, #1
> > +	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
> > +	instr_sync  
> 
> Since these CPUs are all ARMv7, you can use isb directly. Nobody wants
> to see more of the CP15 barriers.

Okay, thanks, I will update that.

> 
> > +	mov	r0, #0
> > +	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
> > +	instr_sync
> > +	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
> > +	instr_sync
> > +	cps	#SVC_MODE
> > +	ret	lr  
> 
> Given that this code is identical to the shmobile hack, it'd be good to
> make it common, one way or another.

Sure, I will try that.
May you have some hints to give me on how to implement it?

> 
> > +ENDPROC(sunxi_init_cntvoff)
> > +
> >  #ifdef CONFIG_SMP
> >  ENTRY(sunxi_boot)
> >  	bl	sunxi_mc_smp_cluster_cache_enable
> > +	bl	sunxi_init_cntvoff
> >  	b	secondary_startup
> >  ENDPROC(sunxi_boot)
> >  
> > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> > index 5e9602ce1573..4bb041492b54 100644
> > --- a/arch/arm/mach-sunxi/sunxi.c
> > +++ b/arch/arm/mach-sunxi/sunxi.c
> > @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> >  };
> >  
> >  extern void __init sun6i_reset_init(void);
> > +extern void sunxi_init_cntvoff(void);
> > +
> >  static void __init sun6i_timer_init(void)
> >  {
> > +	sunxi_init_cntvoff();
> > +  
> 
> It is slightly odd that some CPUs get initialized from the early asm
> code, and some others do a detour via some C code. I'm sure this could
> be made to work

Renesa's code was also doing that so I thought it could be ok to do
it.

Without this code in this timer_init function, it fails to initialize
cntvoff correctly:
http://code.bulix.org/i9fhc9-302612?raw

How should I implement that?

> 
> >  	of_clk_init(NULL);
> >  	if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
> >  		sun6i_reset_init();
> >   
> 
> Thanks,
> 
> 	M.

Thanks,

Best regards,

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Chen-Yu Tsai]

On Mon, Mar 19, 2018 at 3:07 AM, Mylène Josserand
<mylene.josserand@bootlin.com> wrote:
> Hello Mark,
>
> Please, excuse me for this late answer and thank you for the review!
>
> On Wed, 7 Mar 2018 12:18:33 +0000
> Marc Zyngier <marc.zyngier@arm.com> wrote:
>
>> On 23/02/18 13:37, Mylène Josserand wrote:
>> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
>>
>> Only on A7? Is that specific to your platform?
>
> I do not really know other Allwinner's platforms about this subject. At
> least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
> is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
> Maxime could help us on it.

AFAIK all Allwinner CPUs need it if there isn't a firmware (PSCI) layer
beneath the kernel that will do the setup. We just have
"arm,cpu-registers-not-fw-configured" set for all the other SoCs that
have in-kernel SMP support, which includes the A31, A23, A33 and A80.

ChenYu

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Marc Zyngier]

Hi Mylène,

On 18/03/18 19:07, Mylène Josserand wrote:
> Hello Mark,
> 
> Please, excuse me for this late answer and thank you for the review!

No worries.

> 
> On Wed, 7 Mar 2018 12:18:33 +0000
> Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
>> On 23/02/18 13:37, Mylène Josserand wrote:
>>> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.  
>>
>> Only on A7? Is that specific to your platform?
> 
> I do not really know other Allwinner's platforms about this subject. At
> least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
> is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
> Maxime could help us on it.

My point was that having an uninitialized CNTVOFF is hardly a property
of Cortex-A7. The same behaviour exists on all CPUs that implement the
arch timers. What you have here is more a property of the platform, and
its lack of firmware support.

>>
>>> It should be done by the bootloader but it is currently not the case,
>>> even for boot CPU because this SoC is booting in secure mode.
>>> It leads to an random offset value meaning that each CPU will have a
>>> different time, which isn't working very well.
>>>
>>> Add assembly code used for boot CPU and secondary CPU cores to make
>>> sure that the CNTVOFF register is initialized.
>>>
>>> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
>>> ---
>>>  arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
>>>  arch/arm/mach-sunxi/sunxi.c   |  4 ++++
>>>  2 files changed, 25 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
>>> index d5c97e945e69..605896251927 100644
>>> --- a/arch/arm/mach-sunxi/headsmp.S
>>> +++ b/arch/arm/mach-sunxi/headsmp.S
>>> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
>>>  	first: .word sunxi_mc_smp_first_comer - .
>>>  ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>>>  
>>> +ENTRY(sunxi_init_cntvoff)
>>> +	/*
>>> +	 * CNTVOFF has to be initialized either from non-secure Hypervisor
>>> +	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
>>> +	 * then it should be handled by the secure code
>>> +	 */
>>> +	cps	#MON_MODE
>>> +	mrc	p15, 0, r1, c1, c1, 0		/* Get Secure Config */
>>> +	orr	r0, r1, #1
>>> +	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
>>> +	instr_sync  
>>
>> Since these CPUs are all ARMv7, you can use isb directly. Nobody wants
>> to see more of the CP15 barriers.
> 
> Okay, thanks, I will update that.
> 
>>
>>> +	mov	r0, #0
>>> +	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
>>> +	instr_sync
>>> +	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
>>> +	instr_sync
>>> +	cps	#SVC_MODE
>>> +	ret	lr  
>>
>> Given that this code is identical to the shmobile hack, it'd be good to
>> make it common, one way or another.
> 
> Sure, I will try that.
> May you have some hints to give me on how to implement it?

You could move it to arch/arm/common, for example.

> 
>>
>>> +ENDPROC(sunxi_init_cntvoff)
>>> +
>>>  #ifdef CONFIG_SMP
>>>  ENTRY(sunxi_boot)
>>>  	bl	sunxi_mc_smp_cluster_cache_enable
>>> +	bl	sunxi_init_cntvoff
>>>  	b	secondary_startup
>>>  ENDPROC(sunxi_boot)
>>>  
>>> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
>>> index 5e9602ce1573..4bb041492b54 100644
>>> --- a/arch/arm/mach-sunxi/sunxi.c
>>> +++ b/arch/arm/mach-sunxi/sunxi.c
>>> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
>>>  };
>>>  
>>>  extern void __init sun6i_reset_init(void);
>>> +extern void sunxi_init_cntvoff(void);
>>> +
>>>  static void __init sun6i_timer_init(void)
>>>  {
>>> +	sunxi_init_cntvoff();
>>> +  
>>
>> It is slightly odd that some CPUs get initialized from the early asm
>> code, and some others do a detour via some C code. I'm sure this could
>> be made to work
> 
> Renesa's code was also doing that so I thought it could be ok to do
> it.

It is not that it isn't OK, it is just a slightly bizarre construct.

> Without this code in this timer_init function, it fails to initialize
> cntvoff correctly:
> http://code.bulix.org/i9fhc9-302612?raw
> 
> How should I implement that?

I would make it part of a path that all CPUs have to hit at bring-up
time. You already have such a path in your SMP init code, so why not
take advantage of that, making it completely uniform across the board?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Maxime Ripard]

On Sun, Mar 18, 2018 at 08:07:15PM +0100, Mylène Josserand wrote:
> Hello Mark,
> 
> Please, excuse me for this late answer and thank you for the review!
> 
> On Wed, 7 Mar 2018 12:18:33 +0000
> Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
> > On 23/02/18 13:37, Mylène Josserand wrote:
> > > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.  
> > 
> > Only on A7? Is that specific to your platform?
> 
> I do not really know other Allwinner's platforms about this subject. At
> least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
> is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
> Maxime could help us on it.

This is not related to the CPU.

On all the other SoCs but the A80 and the A83t, U-Boot will boot the
system in HYP, and while switching to non-secure will setup CNTVOFF on
the boot CPU.

On the A80 and A83t, U-Boot will execute the kernel in secure, it will
not switch to non-secure, and CNTVOFF will not be set on the boot CPU.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

[Maxime Ripard]

On Mon, Mar 19, 2018 at 10:14:19AM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 19, 2018 at 3:07 AM, Mylène Josserand
> <mylene.josserand@bootlin.com> wrote:
> > Hello Mark,
> >
> > Please, excuse me for this late answer and thank you for the review!
> >
> > On Wed, 7 Mar 2018 12:18:33 +0000
> > Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> >> On 23/02/18 13:37, Mylène Josserand wrote:
> >> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> >>
> >> Only on A7? Is that specific to your platform?
> >
> > I do not really know other Allwinner's platforms about this subject. At
> > least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
> > is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
> > Maxime could help us on it.
> 
> AFAIK all Allwinner CPUs need it if there isn't a firmware (PSCI) layer
> beneath the kernel that will do the setup. We just have
> "arm,cpu-registers-not-fw-configured" set for all the other SoCs that
> have in-kernel SMP support, which includes the A31, A23, A33 and A80.

Most of these ones are here for historical reasons though. Now that we
have U-Boot properly setting it up, we could probably remove it.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com