Skip to contentSkip to conversation

Fixup Broadcom CPU enable method

PATCH In review Chris Brand 3 participants 6 messages Apr 28, 2016 – May 3, 2016

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

cover Fixup Broadcom CPU enable method

This is preparation for supporting the quad-core BCM23550 chip.

Documentation/devicetree/bindings/arm/cpus.txt states that "enable-method" should be a property of the "cpu" node rather than the "cpus" node.

Commit 84320e1a635fcf90cff4185f029ce9e31bf1d4a7
("ARM: BCM: Clean up SMP support for Broadcom Kona") moved the "secondary-boot-reg" property from the "cpus" node to the individual "cpu" nodes but negelected to actually support multiple "secondary-boot-reg" properties.

This patchset moves the enable-method property to the correct place, adds the missing enable-method to the binding documentation, and actually supports setting the "enable-method" property on multiple CPU nodes.

Without this change, "secondary-boot-reg" on even-numbered CPUs is ignored, and the value specified on the last odd-numbered CPU to be processed overrides any earlier values.

Behaviour is slightly changed by this patchset, in that the "secondary-boot-reg" property is only examined when the CPU is being enabled. This means that the omission of that property will be reported slightly later, or never if the CPU in question is never brought online. It also means that the omission in one CPU has no effect on other CPUs, whereas previously omitting it from one CPU would force the system into single-core mode.

Chris Brand (3):
Documentation: Binding docs for bcm11351 enable method
arm: dts: fix use of bcm11351 enable method
arm: modify Broadcom CPU enable method

 .../bindings/arm/bcm/brcm,bcm11351-cpu-method.txt  |   6 +-
 Documentation/devicetree/bindings/arm/cpus.txt     |   1 +
 arch/arm/boot/dts/bcm11351.dtsi                    |   2 +-
 arch/arm/boot/dts/bcm21664.dtsi                    |   2 +-
 arch/arm/mach-bcm/platsmp.c                        | 116 +++++++--------------
 5 files changed, 43 insertions(+), 84 deletions(-)
Signature · 1 line
1.9.1
#1 lore raw

PATCH 1/3 Documentation: Binding docs for bcm11351 enable method

Commit 84320e1a635fcf90cff4185f029ce9e31bf1d4a7
("ARM: BCM: Clean up SMP support for Broadcom Kona") moved the "secondary-boot-reg" property from the "cpus" node to the individual "cpu" nodes but negelected to update brcm,bcm11351-cpu-method.txt to match.

bcm11351-cpu-method was apparently never added to the list of methods in bindings/arm/cpus.txt.

bindings/arm/cpus.txt states that "enable-method" should be a property of the "cpu" node rather than the "cpus" node.

This patch rectifies these two omissions and one inconsistency.

---
 .../devicetree/bindings/arm/bcm/brcm,bcm11351-cpu-method.txt        | 6 +++---
 Documentation/devicetree/bindings/arm/cpus.txt                      | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/arm/bcm/brcm,bcm11351-cpu-method.txt +3 -3
index 8240c023e202..e3f996920403 100644
@@ -5,7 +5,7 @@ CPUs in the following Broadcom SoCs:
   BCM11130, BCM11140, BCM11351, BCM28145, BCM28155, BCM21664
 
 The enable method is specified by defining the following required
-properties in the "cpus" device tree node:
+properties in the "cpu" device tree node:
   - enable-method = "brcm,bcm11351-cpu-method";
   - secondary-boot-reg = <...>;
 
@@ -19,8 +19,6 @@ Example:
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-		enable-method = "brcm,bcm11351-cpu-method";
-		secondary-boot-reg = <0x3500417c>;
 
 		cpu0: cpu at 0 {
 			device_type = "cpu";
@@ -32,5 +30,7 @@ Example:
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
 			reg = <1>;
+			enable-method = "brcm,bcm11351-cpu-method";
+			secondary-boot-reg = <0x3500417c>;
 		};
 	};
Documentation/devicetree/bindings/arm/cpus.txt +1 -0
index ccc62f145306..953350f085c9 100644
@@ -194,6 +194,7 @@ nodes to be present and contain the properties described below.
 			    "allwinner,sun8i-a23"
 			    "arm,psci"
 			    "arm,realview-smp"
+			    "brcm,bcm11351-cpu-method"
 			    "brcm,bcm-nsp-smp"
 			    "brcm,brahma-b15"
 			    "marvell,armada-375-smp"
Signature · 1 line
1.9.1
#2 lore raw

PATCH 2/3 arm: dts: fix use of bcm11351 enable method

In reply to Chris Brand

bindings/arm/cpus.txt states that "enable-method" should be a property of the "cpu" node rather than the "cpus" node.

---
 arch/arm/boot/dts/bcm11351.dtsi | 2 +-
 arch/arm/boot/dts/bcm21664.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/boot/dts/bcm11351.dtsi +1 -1
index 3dc7a8cc5812..18045c38bcf1 100644
@@ -30,7 +30,6 @@
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-		enable-method = "brcm,bcm11351-cpu-method";
 
 		cpu0: cpu at 0 {
 			device_type = "cpu";
@@ -41,6 +40,7 @@
 		cpu1: cpu at 1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
+			enable-method = "brcm,bcm11351-cpu-method";
 			secondary-boot-reg = <0x3500417c>;
 			reg = <1>;
 		};
arch/arm/boot/dts/bcm21664.dtsi +1 -1
index 3f525be28fd0..6dde95f21cef 100644
@@ -30,7 +30,6 @@
        cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-		enable-method = "brcm,bcm11351-cpu-method";
 
 		cpu0: cpu at 0 {
 			device_type = "cpu";
@@ -41,6 +40,7 @@
 		cpu1: cpu at 1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
+			enable-method = "brcm,bcm11351-cpu-method";
 			secondary-boot-reg = <0x35004178>;
 			reg = <1>;
 		};
Signature · 1 line
1.9.1
#3 lore raw

PATCH 3/3 arm: modify Broadcom CPU enable method

In reply to Chris Brand

Commit 84320e1a635fcf90cff4185f029ce9e31bf1d4a7
("ARM: BCM: Clean up SMP support for Broadcom Kona") moved the "secondary-boot-reg" property from the "cpus" node to the individual "cpu" nodes but negelected to actually support multiple "secondary-boot-reg" properties.

This patchset rectifies that omission. Note that the behaviour is changed slightly in that the "secondary-boot-reg" property is now read in smp_boot_secondary() rather than smp_prepare_cpus(). This means that any omissions will now only be reported when and if the cpu in question is being brought up. It also means that an omission for one cpu will not force uniprocessor mode.

---
 arch/arm/mach-bcm/platsmp.c | 116 ++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 79 deletions(-)
arch/arm/mach-bcm/platsmp.c +37 -79
index cfae9c71fb74..5e53cf2fc052 100644
@@ -37,9 +37,6 @@
 #define OF_SECONDARY_BOOT	"secondary-boot-reg"
 #define MPIDR_CPUID_BITMASK	0x3
 
-/* I/O address of register used to coordinate secondary core startup */
-static u32	secondary_boot_addr;
-
 /*
  * Enable the Cortex A9 Snoop Control Unit
  *
@@ -81,20 +78,40 @@ static int __init scu_a9_enable(void)
 	return 0;
 }
 
-static int nsp_write_lut(void)
+static u32 secondary_boot_addr_for(unsigned int cpu)
+{
+	u32 secondary_boot_addr = 0;
+	struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+        if (!cpu_node) {
+		pr_err("Failed to find device tree node for CPU%u\n", cpu);
+		return 0;
+	}
+
+	if (of_property_read_u32(cpu_node,
+				 OF_SECONDARY_BOOT,
+				 &secondary_boot_addr))
+		pr_err("required secondary boot register not specified for CPU%u\n",
+			cpu);
+
+	of_node_put(cpu_node);
+
+	return secondary_boot_addr;
+}
+
+static int nsp_write_lut(unsigned int cpu)
 {
 	void __iomem *sku_rom_lut;
 	phys_addr_t secondary_startup_phy;
+	const u32 secondary_boot_addr = secondary_boot_addr_for(cpu);
 
-	if (!secondary_boot_addr) {
-		pr_warn("required secondary boot register not specified\n");
+	if (!secondary_boot_addr)
 		return -EINVAL;
-	}
 
 	sku_rom_lut = ioremap_nocache((phys_addr_t)secondary_boot_addr,
-						sizeof(secondary_boot_addr));
+				      sizeof(phys_addr_t));
 	if (!sku_rom_lut) {
-		pr_warn("unable to ioremap SKU-ROM LUT register\n");
+		pr_warn("unable to ioremap SKU-ROM LUT register for cpu %u\n", cpu);
 		return -ENOMEM;
 	}
 
@@ -113,70 +130,12 @@ static int nsp_write_lut(void)
 
 static void __init bcm_smp_prepare_cpus(unsigned int max_cpus)
 {
-	static cpumask_t only_cpu_0 = { CPU_BITS_CPU0 };
-	struct device_node *cpus_node = NULL;
-	struct device_node *cpu_node = NULL;
-	int ret;
-
-	/*
-	 * This function is only called via smp_ops->smp_prepare_cpu().
-	 * That only happens if a "/cpus" device tree node exists
-	 * and has an "enable-method" property that selects the SMP
-	 * operations defined herein.
-	 */
-	cpus_node = of_find_node_by_path("/cpus");
-	if (!cpus_node)
-		return;
-
-	for_each_child_of_node(cpus_node, cpu_node) {
-		u32 cpuid;
-
-		if (of_node_cmp(cpu_node->type, "cpu"))
-			continue;
-
-		if (of_property_read_u32(cpu_node, "reg", &cpuid)) {
-			pr_debug("%s: missing reg property\n",
-				     cpu_node->full_name);
-			ret = -ENOENT;
-			goto out;
-		}
-
-		/*
-		 * "secondary-boot-reg" property should be defined only
-		 * for secondary cpu
-		 */
-		if ((cpuid & MPIDR_CPUID_BITMASK) == 1) {
-			/*
-			 * Our secondary enable method requires a
-			 * "secondary-boot-reg" property to specify a register
-			 * address used to request the ROM code boot a secondary
-			 * core. If we have any trouble getting this we fall
-			 * back to uniprocessor mode.
-			 */
-			if (of_property_read_u32(cpu_node,
-						OF_SECONDARY_BOOT,
-						&secondary_boot_addr)) {
-				pr_warn("%s: no" OF_SECONDARY_BOOT "property\n",
-					cpu_node->name);
-				ret = -ENOENT;
-				goto out;
-			}
-		}
-	}
-
-	/*
-	 * Enable the SCU on Cortex A9 based SoCs. If -ENOENT is
-	 * returned, the SoC reported a uniprocessor configuration.
-	 * We bail on any other error.
-	 */
-	ret = scu_a9_enable();
-out:
-	of_node_put(cpu_node);
-	of_node_put(cpus_node);
+	const cpumask_t only_cpu_0 = { CPU_BITS_CPU0 };
 
-	if (ret) {
+	/* Enable the SCU on Cortex A9 based SoCs */
+	if (scu_a9_enable()) {
 		/* Update the CPU present map to reflect uniprocessor mode */
-		pr_warn("disabling SMP\n");
+		pr_warn("failed to enable A9 SCU - disabling SMP\n");
 		init_cpu_present(&only_cpu_0);
 	}
 }
@@ -207,6 +166,7 @@ static int kona_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	u32 cpu_id;
 	u32 boot_val;
 	bool timeout = false;
+	const u32 secondary_boot_addr = secondary_boot_addr_for(cpu);
 
 	cpu_id = cpu_logical_map(cpu);
 	if (cpu_id & ~BOOT_ADDR_CPUID_MASK) {
@@ -214,13 +174,11 @@ static int kona_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		return -EINVAL;
 	}
 
-	if (!secondary_boot_addr) {
-		pr_err("required secondary boot register not specified\n");
+	if (!secondary_boot_addr)
 		return -EINVAL;
-	}
 
-	boot_reg = ioremap_nocache(
-			(phys_addr_t)secondary_boot_addr, sizeof(u32));
+	boot_reg = ioremap_nocache((phys_addr_t)secondary_boot_addr,
+				   sizeof(phys_addr_t));
 	if (!boot_reg) {
 		pr_err("unable to map boot register for cpu %u\n", cpu_id);
 		return -ENOMEM;
@@ -263,7 +221,7 @@ static int nsp_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * After wake up, secondary core branches to the startup
 	 * address programmed at SKU ROM LUT location.
 	 */
-	ret = nsp_write_lut();
+	ret = nsp_write_lut(cpu);
 	if (ret) {
 		pr_err("unable to write startup addr to SKU ROM LUT\n");
 		goto out;
@@ -276,12 +234,12 @@ out:
 	return ret;
 }
 
-static const struct smp_operations bcm_smp_ops __initconst = {
+static const struct smp_operations kona_smp_ops __initconst = {
 	.smp_prepare_cpus	= bcm_smp_prepare_cpus,
 	.smp_boot_secondary	= kona_boot_secondary,
 };
 CPU_METHOD_OF_DECLARE(bcm_smp_bcm281xx, "brcm,bcm11351-cpu-method",
-			&bcm_smp_ops);
+			&kona_smp_ops);
 
 static const struct smp_operations nsp_smp_ops __initconst = {
 	.smp_prepare_cpus	= bcm_smp_prepare_cpus,
Signature · 1 line
1.9.1
#4 lore raw

Re: [PATCH 3/3] arm: modify Broadcom CPU enable method

Quoted 14 lines · Chris Brand

Commit 84320e1a635fcf90cff4185f029ce9e31bf1d4a7
("ARM: BCM: Clean up SMP support for Broadcom Kona") moved the "secondary-boot-reg" property from the "cpus" node to the individual "cpu" nodes but negelected to actually support multiple "secondary-boot-reg" properties.

This patchset rectifies that omission. Note that the behaviour is changed slightly in that the "secondary-boot-reg" property is now read in smp_boot_secondary() rather than smp_prepare_cpus(). This means that any omissions will now only be reported when and if the cpu in question is being
brought up. It also means that an omission for one cpu will not force uniprocessor mode.

Quoted 2 lines

Looks good to me. Also, I verified that it works on NSP.

Reviewed-and-Tested-by Jon Mason <[email protected]>
Quoted 206 lines
---
 arch/arm/mach-bcm/platsmp.c | 116
++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 79 deletions(-)
arch/arm/mach-bcm/platsmp.c +26 -9
index cfae9c71fb74..5e53cf2fc052 100644
@@ -37,9 +37,6 @@
 #define OF_SECONDARY_BOOT      "secondary-boot-reg"
 #define MPIDR_CPUID_BITMASK    0x3
 
-/* I/O address of register used to coordinate secondary core startup */
-static u32     secondary_boot_addr;
-
 /*
  * Enable the Cortex A9 Snoop Control Unit
  *
@@ -81,20 +78,40 @@ static int __init scu_a9_enable(void)
        return 0;
 }
 
-static int nsp_write_lut(void)
+static u32 secondary_boot_addr_for(unsigned int cpu)
+{
+       u32 secondary_boot_addr = 0;
+       struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+        if (!cpu_node) {
+               pr_err("Failed to find device tree node for CPU%u\n", cpu);
+               return 0;
+       }
+
+       if (of_property_read_u32(cpu_node,
+                                OF_SECONDARY_BOOT,
+                                &secondary_boot_addr))
+               pr_err("required secondary boot register not specified for
 CPU%u\n",
+                       cpu);
+
+       of_node_put(cpu_node);
+
+       return secondary_boot_addr;
+}
+
+static int nsp_write_lut(unsigned int cpu)
 {
        void __iomem *sku_rom_lut;
        phys_addr_t secondary_startup_phy;
+       const u32 secondary_boot_addr = secondary_boot_addr_for(cpu);
 
-       if (!secondary_boot_addr) {
-               pr_warn("required secondary boot register not
 specified\n");
+       if (!secondary_boot_addr)
                return -EINVAL;
-       }
 
        sku_rom_lut = ioremap_nocache((phys_addr_t)secondary_boot_addr,
-
 sizeof(secondary_boot_addr));
+                                     sizeof(phys_addr_t));
        if (!sku_rom_lut) {
-               pr_warn("unable to ioremap SKU-ROM LUT register\n");
+               pr_warn("unable to ioremap SKU-ROM LUT register for cpu
%u\n", cpu);
                return -ENOMEM;
        }
arch/arm/mach-bcm/platsmp.c +0 -36
@@ -113,70 +130,12 @@ static int nsp_write_lut(void)
 
 static void __init bcm_smp_prepare_cpus(unsigned int max_cpus)
 {
-       static cpumask_t only_cpu_0 = { CPU_BITS_CPU0 };
-       struct device_node *cpus_node = NULL;
-       struct device_node *cpu_node = NULL;
-       int ret;
-
-       /*
-        * This function is only called via smp_ops->smp_prepare_cpu().
-        * That only happens if a "/cpus" device tree node exists
-        * and has an "enable-method" property that selects the SMP
-        * operations defined herein.
-        */
-       cpus_node = of_find_node_by_path("/cpus");
-       if (!cpus_node)
-               return;
-
-       for_each_child_of_node(cpus_node, cpu_node) {
-               u32 cpuid;
-
-               if (of_node_cmp(cpu_node->type, "cpu"))
-                       continue;
-
-               if (of_property_read_u32(cpu_node, "reg", &cpuid)) {
-                       pr_debug("%s: missing reg property\n",
-                                    cpu_node->full_name);
-                       ret = -ENOENT;
-                       goto out;
-               }
-
-               /*
-                * "secondary-boot-reg" property should be defined only
-                * for secondary cpu
-                */
-               if ((cpuid & MPIDR_CPUID_BITMASK) == 1) {
-                       /*
-                        * Our secondary enable method requires a
-                        * "secondary-boot-reg" property to specify a

register
- * address used to request the ROM code boot a
secondary
- * core. If we have any trouble getting this we
fall

arch/arm/mach-bcm/platsmp.c +0 -6
-                        * back to uniprocessor mode.
-                        */
-                       if (of_property_read_u32(cpu_node,
-                                               OF_SECONDARY_BOOT,
-                                               &secondary_boot_addr)) {
-                               pr_warn("%s: no" OF_SECONDARY_BOOT

"property\n",

arch/arm/mach-bcm/platsmp.c +3 -17
-                                       cpu_node->name);
-                               ret = -ENOENT;
-                               goto out;
-                       }
-               }
-       }
-
-       /*
-        * Enable the SCU on Cortex A9 based SoCs. If -ENOENT is
-        * returned, the SoC reported a uniprocessor configuration.
-        * We bail on any other error.
-        */
-       ret = scu_a9_enable();
-out:
-       of_node_put(cpu_node);
-       of_node_put(cpus_node);
+       const cpumask_t only_cpu_0 = { CPU_BITS_CPU0 };
 
-       if (ret) {
+       /* Enable the SCU on Cortex A9 based SoCs */
+       if (scu_a9_enable()) {
                /* Update the CPU present map to reflect uniprocessor mode

*/

arch/arm/mach-bcm/platsmp.c +2 -1
-               pr_warn("disabling SMP\n");
+               pr_warn("failed to enable A9 SCU - disabling SMP\n");
                init_cpu_present(&only_cpu_0);
        }
 }
@@ -207,6 +166,7 @@ static int kona_boot_secondary(unsigned int cpu,
 struct task_struct *idle)
        u32 cpu_id;
        u32 boot_val;
        bool timeout = false;
+       const u32 secondary_boot_addr = secondary_boot_addr_for(cpu);
 
        cpu_id = cpu_logical_map(cpu);
        if (cpu_id & ~BOOT_ADDR_CPUID_MASK) {
arch/arm/mach-bcm/platsmp.c +3 -5
@@ -214,13 +174,11 @@ static int kona_boot_secondary(unsigned int cpu,
 struct task_struct *idle)
                return -EINVAL;
        }
 
-       if (!secondary_boot_addr) {
-               pr_err("required secondary boot register not specified\n");
+       if (!secondary_boot_addr)
                return -EINVAL;
-       }
 
-       boot_reg = ioremap_nocache(
-                       (phys_addr_t)secondary_boot_addr, sizeof(u32));
+       boot_reg = ioremap_nocache((phys_addr_t)secondary_boot_addr,
+                                  sizeof(phys_addr_t));
        if (!boot_reg) {
                pr_err("unable to map boot register for cpu %u\n", cpu_id);
                return -ENOMEM;
arch/arm/mach-bcm/platsmp.c +1 -1
@@ -263,7 +221,7 @@ static int nsp_boot_secondary(unsigned int cpu, struct
 task_struct *idle)
         * After wake up, secondary core branches to the startup
         * address programmed at SKU ROM LUT location.
         */
-       ret = nsp_write_lut();
+       ret = nsp_write_lut(cpu);
        if (ret) {
                pr_err("unable to write startup addr to SKU ROM LUT\n");
                goto out;
arch/arm/mach-bcm/platsmp.c +2 -3
@@ -276,12 +234,12 @@ out:
        return ret;
 }
 
-static const struct smp_operations bcm_smp_ops __initconst = {
+static const struct smp_operations kona_smp_ops __initconst = {
        .smp_prepare_cpus       = bcm_smp_prepare_cpus,
        .smp_boot_secondary     = kona_boot_secondary,
 };
 CPU_METHOD_OF_DECLARE(bcm_smp_bcm281xx, "brcm,bcm11351-cpu-method",
-                       &bcm_smp_ops);
+                       &kona_smp_ops);
 
 static const struct smp_operations nsp_smp_ops __initconst = {
        .smp_prepare_cpus       = bcm_smp_prepare_cpus,
--

1.9.1

#5 lore raw

cover Fixup Broadcom CPU enable method

In reply to Chris Brand
Quoted 19 lines · Chris Brand

This is preparation for supporting the quad-core BCM23550 chip.

Documentation/devicetree/bindings/arm/cpus.txt states that "enable-method" should be a property of the "cpu" node rather than the "cpus" node.

Commit 84320e1a635fcf90cff4185f029ce9e31bf1d4a7
("ARM: BCM: Clean up SMP support for Broadcom Kona") moved the "secondary-boot-reg" property from the "cpus" node to the individual "cpu" nodes but negelected to actually support multiple "secondary-boot-reg" properties.

This patchset moves the enable-method property to the correct place, adds the missing enable-method to the binding documentation, and actually supports setting the "enable-method" property on multiple CPU nodes.

Without this change, "secondary-boot-reg" on even-numbered CPUs is ignored, and the value specified on the last odd-numbered CPU to be processed overrides any earlier values.

Quoted 6 lines

Behaviour is slightly changed by this patchset, in that the "secondary-boot-reg" property is only examined when the CPU is being enabled. This means that the omission of that property will be reported slightly later, or never if the CPU in question is never brought online. It also means that the omission in one CPU has no effect on other CPUs, whereas previously omitting it from one CPU would force the system into single-core mode.

Series applied to devicetree/next, thanks Chris!

Signature · 1 line
Florian