[PATCH v2 0/6] ARM: rockchip: add smp functionality

[PATCH v2 0/6] ARM: rockchip: add smp functionality

[Heiko Stübner]

This series enables the use of the additional cores on Rockchip
Cortex-A9 SoCs.

To achieve this, add the scu, the needed sram and power-management-unit.

changes since v1:
- add reserved block feature for mmio-sram, to not use two logical
  sram nodes
- the sram content is kept intact while the device is running, so
  copying the trampoline is only needed once

Heiko Stuebner (6):
  misc: sram: fix error path in sram_probe
  misc: sram: add ability to mark sram sections as reserved
  arm: rockchip: add snoop-control-unit
  arm: rockchip: add sram dt nodes and documentation for the smp part
  arm: rockchip: add power-management-unit dt node
  arm: rockchip: add smp bringup code

 .../devicetree/bindings/arm/rockchip/pmu.txt       |   16 ++
 .../devicetree/bindings/arm/rockchip/smp-sram.txt  |   23 +++
 Documentation/devicetree/bindings/misc/sram.txt    |    8 +
 arch/arm/boot/dts/rk3066a.dtsi                     |   16 ++
 arch/arm/mach-rockchip/Kconfig                     |    1 +
 arch/arm/mach-rockchip/Makefile                    |    1 +
 arch/arm/mach-rockchip/core.h                      |   22 +++
 arch/arm/mach-rockchip/headsmp.S                   |   32 +++
 arch/arm/mach-rockchip/platsmp.c                   |  203 ++++++++++++++++++++
 arch/arm/mach-rockchip/rockchip.c                  |    2 +
 drivers/misc/sram.c                                |   85 +++++++-
 11 files changed, 404 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/pmu.txt
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/smp-sram.txt
 create mode 100644 arch/arm/mach-rockchip/core.h
 create mode 100644 arch/arm/mach-rockchip/headsmp.S
 create mode 100644 arch/arm/mach-rockchip/platsmp.c

-- 
1.7.10.4

[PATCH v2 1/6] misc: sram: fix error path in sram_probe

[Heiko Stübner]

The pool is created thru devm_gen_pool_create, so the call to
gen_pool_destroy is not necessary.
Instead the sram-clock must be turned off again if it exists.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/misc/sram.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index d87cc91..afe66571 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)
 	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
 				res->start, size, -1);
 	if (ret < 0) {
-		gen_pool_destroy(sram->pool);
+		if (sram->clk)
+			clk_disable_unprepare(sram->clk);
 		return ret;
 	}
 
-- 
1.7.10.4

[PATCH v2 2/6] misc: sram: add ability to mark sram sections as reserved

[Heiko Stübner]

Some SoCs need parts of their sram for special purposes. So while being part
of the periphal, it should not be part of the genpool controlling the sram.

Threfore add an option mmio-sram-reserved to keep arbitary portions of the
sram from being part of the pool.

Suggested-by: Rob Herring <robherring2@gmail.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 Documentation/devicetree/bindings/misc/sram.txt |    8 +++
 drivers/misc/sram.c                             |   86 +++++++++++++++++++++--
 2 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
index 4d0a00e..eae080e 100644
--- a/Documentation/devicetree/bindings/misc/sram.txt
+++ b/Documentation/devicetree/bindings/misc/sram.txt
@@ -8,9 +8,17 @@ Required properties:
 
 - reg : SRAM iomem address range
 
+Optional properties:
+
+- mmio-sram-reserved: ordered list of reserved chunks inside the sram that
+  should not become part of the genalloc pool.
+  Format is <base size>, <base size>, ...; with base being relative to the
+  reg property base.
+
 Example:
 
 sram: sram@5c000000 {
 	compatible = "mmio-sram";
 	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
+	mmio-sram-reserved = <0x0 0x100>; /* reserve 0x5c000000-0x5c000100 */
 };
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index afe66571..5fccbe3 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -42,6 +42,8 @@ static int sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	struct resource *res;
 	unsigned long size;
+	const __be32 *reserved_list = NULL;
+	int reserved_size = 0;
 	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -65,12 +67,89 @@ static int sram_probe(struct platform_device *pdev)
 	if (!sram->pool)
 		return -ENOMEM;
 
-	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
-				res->start, size, -1);
-	if (ret < 0) {
-		if (sram->clk)
-			clk_disable_unprepare(sram->clk);
-		return ret;
+	if (pdev->dev.of_node) {
+		reserved_list = of_get_property(pdev->dev.of_node,
+						"mmio-sram-reserved",
+						&reserved_size);
+		if (reserved_list) {
+			reserved_size /= sizeof(*reserved_list);
+			if (!reserved_size || reserved_size % 2) {
+				dev_warn(&pdev->dev, "wrong number of arguments in mmio-sram-reserved\n");
+				reserved_list = NULL;
+			}
+		}
+	}
+
+	if (!reserved_list) {
+		ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
+					res->start, size, -1);
+		if (ret < 0) {
+			if (sram->clk)
+				clk_disable_unprepare(sram->clk);
+			return ret;
+		}
+	} else {
+		unsigned int cur_start = 0;
+		unsigned int cur_size;
+		unsigned int rstart;
+		unsigned int rsize;
+		int i;
+
+		for (i = 0; i < reserved_size; i += 2) {
+			/* get the next reserved block */
+			rstart = be32_to_cpu(*reserved_list++);
+			rsize = be32_to_cpu(*reserved_list++);
+
+			/* catch unsorted list entries */
+			if (rstart < cur_start) {
+				dev_err(&pdev->dev, "unsorted reserved list (0x%x before current 0x%x)\n",
+					rstart, cur_start);
+				if (sram->clk)
+					clk_disable_unprepare(sram->clk);
+				return -EINVAL;
+			}
+
+			dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
+				 rstart, rstart + rsize);
+
+			/* current start is in a reserved block */
+			if (rstart <= cur_start) {
+				cur_start = rstart + rsize;
+				continue;
+			}
+
+			/*
+			 * allocate the space between the current starting
+			 * address and the following reserved block
+			 */
+			cur_size = rstart - cur_start;
+
+			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
+				 cur_start, cur_start + cur_size);
+			ret = gen_pool_add_virt(sram->pool,
+					(unsigned long)virt_base + cur_start,
+					res->start + cur_start, cur_size, -1);
+			if (ret < 0) {
+				if (sram->clk)
+					clk_disable_unprepare(sram->clk);
+				return ret;
+			}
+
+			/* next allocation after this reserved block */
+			cur_start = rstart + rsize;
+		}
+
+		/* allocate the space after the last reserved block */
+		if (cur_start < size) {
+			cur_size = size - cur_start;
+
+			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
+				 cur_start, cur_start + cur_size);
+			ret = gen_pool_add_virt(sram->pool,
+					(unsigned long)virt_base + cur_start,
+					res->start + cur_start, cur_size, -1);
+		}
+
 	}
 
 	platform_set_drvdata(pdev, sram);
-- 
1.7.10.4

[PATCH v2 3/6] ARM: rockchip: add snoop-control-unit

[Heiko Stübner]

This adds the device-node and config select to enable the
scu in all Rockchip Cortex-A9 SoCs.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3066a.dtsi |    5 +++++
 arch/arm/mach-rockchip/Kconfig |    1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
index 56bfac9..26c4311 100644
--- a/arch/arm/boot/dts/rk3066a.dtsi
+++ b/arch/arm/boot/dts/rk3066a.dtsi
@@ -48,6 +48,11 @@
 		compatible = "simple-bus";
 		ranges;
 
+		scu@1013c000 {
+			compatible = "arm,cortex-a9-scu";
+			reg = <0x1013c000 0x100>;
+		};
+
 		gic: interrupt-controller@1013d000 {
 			compatible = "arm,cortex-a9-gic";
 			interrupt-controller;
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 25ee12b..0f5484c 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -5,6 +5,7 @@ config ARCH_ROCKCHIP
 	select ARCH_REQUIRE_GPIOLIB
 	select ARM_GIC
 	select CACHE_L2X0
+	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD if LOCAL_TIMERS
 	select HAVE_SMP
 	select LOCAL_TIMERS if SMP
-- 
1.7.10.4

[PATCH v2 4/6] ARM: rockchip: add sram dt nodes and documentation

[Heiko Stübner]

The Rockchip SoCs need a special part of their sram for bringup
of additional cores. Therefore also add a reserved section when adding the
mmio-sram node to keep the sram driver from using this space.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../devicetree/bindings/arm/rockchip/smp-sram.txt  |   23 ++++++++++++++++++++
 arch/arm/boot/dts/rk3066a.dtsi                     |    6 +++++
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/smp-sram.txt

diff --git a/Documentation/devicetree/bindings/arm/rockchip/smp-sram.txt b/Documentation/devicetree/bindings/arm/rockchip/smp-sram.txt
new file mode 100644
index 0000000..80c878e
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/rockchip/smp-sram.txt
@@ -0,0 +1,23 @@
+Rockchip SRAM for smp bringup:
+------------------------------
+
+Rockchip's smp-capable SoCs use the first part of the sram for the bringup
+of the cores. Once the core gets powered up it executes the code that is
+residing at the very beginning of the sram.
+
+Therefore a reserved section has to be added to the mmio-sram declaration.
+
+Required node properties:
+- compatible : should contain both "rockchip,rk3066-sram", "mmio-sram"
+  so that the smp code can select the correct sram node.
+
+The rest of the properties should follow the generic mmio-sram discription
+found in ../../misc/sram.txt
+
+Example:
+
+	sram: sram@10080000 {
+		compatible = "rockchip,rk3066-sram", "mmio-sram";
+		reg = <0x10080000 0x10000>;
+		mmio-sram-reserved = <0x0 0x50>;
+	};
diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
index 26c4311..24d1941 100644
--- a/arch/arm/boot/dts/rk3066a.dtsi
+++ b/arch/arm/boot/dts/rk3066a.dtsi
@@ -53,6 +53,12 @@
 			reg = <0x1013c000 0x100>;
 		};
 
+		sram: sram@10080000 {
+			compatible = "rockchip,rk3066-sram", "mmio-sram";
+			reg = <0x10080000 0x10000>;
+			mmio-sram-reserved = <0x0 0x50>;
+		};
+
 		gic: interrupt-controller@1013d000 {
 			compatible = "arm,cortex-a9-gic";
 			interrupt-controller;
-- 
1.7.10.4

[PATCH v2 5/6] ARM: rockchip: add power-management-unit dt node

[Heiko Stübner]

The pmu is needed to bring up the cores during smp operations.
Therefore add a node and documentation for it.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 Documentation/devicetree/bindings/arm/rockchip/pmu.txt |   16 ++++++++++++++++
 arch/arm/boot/dts/rk3066a.dtsi                         |    5 +++++
 2 files changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/pmu.txt

diff --git a/Documentation/devicetree/bindings/arm/rockchip/pmu.txt b/Documentation/devicetree/bindings/arm/rockchip/pmu.txt
new file mode 100644
index 0000000..3ee9b42
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/rockchip/pmu.txt
@@ -0,0 +1,16 @@
+Rockchip power-management-unit:
+-------------------------------
+
+The pmu is used to turn off and on different power domains of the SoCs
+This includes the power to the CPU cores.
+
+Required node properties:
+- compatible value : = "rockchip,rk3066-pmu";
+- reg : physical base address and the size of the registers window
+
+Example:
+
+	pmu@20004000 {
+		compatible = "rockchip,rk3066-pmu";
+		reg = <0x20004000 0x100>;
+	};
diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
index 24d1941..43ac7c8 100644
--- a/arch/arm/boot/dts/rk3066a.dtsi
+++ b/arch/arm/boot/dts/rk3066a.dtsi
@@ -59,6 +59,11 @@
 			mmio-sram-reserved = <0x0 0x50>;
 		};
 
+		pmu@20004000 {
+			compatible = "rockchip,rk3066-pmu";
+			reg = <0x20004000 0x100>;
+		};
+
 		gic: interrupt-controller@1013d000 {
 			compatible = "arm,cortex-a9-gic";
 			interrupt-controller;
-- 
1.7.10.4

[PATCH v2 6/6] ARM: rockchip: add smp bringup code

[Heiko Stübner]

This adds the necessary smp-operations and startup code to use
additional cores on Rockchip SoCs.

We currently hog the power management unit in the smp code, as it is
necessary to control the power to the cpu core and nothing else is
currently using it, so a generic implementation can be done later.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/mach-rockchip/Makefile   |    1 +
 arch/arm/mach-rockchip/core.h     |   22 ++++
 arch/arm/mach-rockchip/headsmp.S  |   32 ++++++
 arch/arm/mach-rockchip/platsmp.c  |  203 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rockchip/rockchip.c |    2 +
 5 files changed, 260 insertions(+)
 create mode 100644 arch/arm/mach-rockchip/core.h
 create mode 100644 arch/arm/mach-rockchip/headsmp.S
 create mode 100644 arch/arm/mach-rockchip/platsmp.c

diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
index 1547d4f..4377a14 100644
--- a/arch/arm/mach-rockchip/Makefile
+++ b/arch/arm/mach-rockchip/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
+obj-$(CONFIG_SMP) += headsmp.o platsmp.o
diff --git a/arch/arm/mach-rockchip/core.h b/arch/arm/mach-rockchip/core.h
new file mode 100644
index 0000000..e2e7c9d
--- /dev/null
+++ b/arch/arm/mach-rockchip/core.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2013 MundoReader S.L.
+ * Author: Heiko Stuebner <heiko@sntech.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+extern char rockchip_secondary_trampoline;
+extern char rockchip_secondary_trampoline_end;
+
+extern unsigned long rockchip_boot_fn;
+extern void rockchip_secondary_startup(void);
+
+extern struct smp_operations rockchip_smp_ops;
diff --git a/arch/arm/mach-rockchip/headsmp.S b/arch/arm/mach-rockchip/headsmp.S
new file mode 100644
index 0000000..3dd72f7
--- /dev/null
+++ b/arch/arm/mach-rockchip/headsmp.S
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2013 MundoReader S.L.
+ * Author: Heiko Stuebner <heiko@sntech.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/linkage.h>
+#include <linux/init.h>
+
+	__CPUINIT
+
+ENTRY(rockchip_secondary_startup)
+	bl	v7_invalidate_l1
+	b	secondary_startup
+ENDPROC(rockchip_secondary_startup)
+
+ENTRY(rockchip_secondary_trampoline)
+	ldr	pc, 1f
+ENDPROC(rockchip_secondary_trampoline)
+	.globl	rockchip_boot_fn
+rockchip_boot_fn:
+1:	.space	4
+
+ENTRY(rockchip_secondary_trampoline_end)
diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
new file mode 100644
index 0000000..4b8fadc
--- /dev/null
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -0,0 +1,203 @@
+/*
+ * Copyright (c) 2013 MundoReader S.L.
+ * Author: Heiko Stuebner <heiko@sntech.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/smp.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <asm/cacheflush.h>
+#include <asm/smp_scu.h>
+#include <asm/smp_plat.h>
+#include <asm/mach/map.h>
+
+#include "core.h"
+
+static void __iomem *scu_base_addr;
+static void __iomem *sram_base_addr;
+static int ncores;
+
+/*
+ * temporary PMU handling
+ */
+
+#define PMU_PWRDN_CON		0x08
+#define PMU_PWRDN_ST		0x0c
+
+static void __iomem *pmu_base_addr;
+
+static inline bool pmu_power_domain_is_on(int pd)
+{
+	return !(readl_relaxed(pmu_base_addr + PMU_PWRDN_ST) & BIT(pd));
+}
+
+static void pmu_set_power_domain(int pd, bool on)
+{
+	u32 val = readl_relaxed(pmu_base_addr + PMU_PWRDN_CON);
+	if (on)
+		val &= ~BIT(pd);
+	else
+		val |=  BIT(pd);
+	writel(val, pmu_base_addr + PMU_PWRDN_CON);
+
+	while (pmu_power_domain_is_on(pd) != on) { }
+}
+
+/*
+ * Handling of CPU cores
+ */
+
+static int __cpuinit rockchip_boot_secondary(unsigned int cpu,
+					     struct task_struct *idle)
+{
+	if (!sram_base_addr || !pmu_base_addr) {
+		pr_err("%s: sram or pmu missing for cpu boot\n", __func__);
+		return -ENXIO;
+	}
+
+	if (cpu >= ncores) {
+		pr_err("%s: cpu %d outside maximum number of cpus %d\n",
+							__func__, cpu, ncores);
+		return -ENXIO;
+	}
+
+	/* start the core */
+	pmu_set_power_domain(0 + cpu, true);
+
+	return 0;
+}
+
+/**
+ * rockchip_smp_prepare_sram - populate necessary sram block
+ * Starting cores execute the code residing at the start of the on-chip sram
+ * after powe-on. Therefore make sure, this sram region is reserved and
+ * big enough. After this check, copy the trampoline code that directs the
+ * core to the real startup code in ram into the sram-region.
+ * @node: mmio-sram device node
+ */
+static int __init rockchip_smp_prepare_sram(struct device_node *node)
+{
+	unsigned int trampoline_sz = &rockchip_secondary_trampoline_end -
+					    &rockchip_secondary_trampoline;
+	const __be32 *reserved_list = NULL;
+	int reserved_size;
+	int rstart = -1;
+	unsigned int rsize;
+	unsigned int i;
+
+	reserved_list = of_get_property(node, "mmio-sram-reserved",
+					&reserved_size);
+	if (!reserved_list) {
+		pr_err("%s: wrong number of arguments in mmio-sram-reserved\n",
+		       __func__);
+		return -ENOENT;
+	}
+
+	reserved_size /= sizeof(*reserved_list);
+	if (!reserved_size || reserved_size % 2) {
+		pr_err("%s: wrong number of arguments in mmio-sram-reserved\n",
+		       __func__);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < reserved_size; i += 2) {
+		/* get the next reserved block */
+		rstart = be32_to_cpu(*reserved_list++);
+		rsize = be32_to_cpu(*reserved_list++);
+
+		if (!rstart)
+			break;
+	}
+
+	if (rstart) {
+		pr_err("%s: start of sram is not reserved from mmio-sram\n",
+		       __func__);
+		return -EINVAL;
+	}
+
+	if (rsize < trampoline_sz) {
+		pr_err("%s: reserved block with size 0x%x is to small for trampoline size 0x%x\n",
+		       __func__, rsize, trampoline_sz);
+		return -EINVAL;
+	}
+
+	sram_base_addr = of_iomap(node, 0);
+
+	/* set the boot function for the sram code */
+	rockchip_boot_fn = virt_to_phys(rockchip_secondary_startup);
+
+	/* copy the trampoline to sram, that runs during startup of the core */
+	memcpy(sram_base_addr, &rockchip_secondary_trampoline, trampoline_sz);
+	flush_cache_all();
+	outer_clean_range(0, trampoline_sz);
+
+	dsb_sev();
+
+	return 0;
+}
+
+static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
+{
+	struct device_node *node;
+	unsigned int i;
+
+	node = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
+	if (!node) {
+		pr_err("%s: missing scu\n", __func__);
+		return;
+	}
+
+	scu_base_addr = of_iomap(node, 0);
+	if (!scu_base_addr) {
+		pr_err("%s: could not map scu registers\n", __func__);
+		return;
+	}
+
+	node = of_find_compatible_node(NULL, NULL, "rockchip,rk3066-sram");
+	if (!node) {
+		pr_err("%s: could not find sram dt node\n", __func__);
+		return;
+	}
+
+	if (rockchip_smp_prepare_sram(node))
+		return;
+
+	node = of_find_compatible_node(NULL, NULL, "rockchip,rk3066-pmu");
+	if (!node) {
+		pr_err("%s: could not find sram dt node\n", __func__);
+		return;
+	}
+
+	pmu_base_addr = of_iomap(node, 0);
+
+	/*
+	 * While the number of cpus is gathered from dt, also get the number
+	 * of cores from the scu to verify this value when booting the cores.
+	 */
+	ncores = scu_get_core_count(scu_base_addr);
+
+	scu_enable(scu_base_addr);
+
+	/* Make sure that all cores except the first are really off */
+	for (i = 1; i < ncores; i++)
+		pmu_set_power_domain(0 + i, false);
+}
+
+struct smp_operations rockchip_smp_ops __initdata = {
+	.smp_prepare_cpus	= rockchip_smp_prepare_cpus,
+	.smp_boot_secondary	= rockchip_boot_secondary,
+};
diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
index 724d2d8..a0c1f19 100644
--- a/arch/arm/mach-rockchip/rockchip.c
+++ b/arch/arm/mach-rockchip/rockchip.c
@@ -24,6 +24,7 @@
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <asm/hardware/cache-l2x0.h>
+#include "core.h"
 
 static void __init rockchip_timer_init(void)
 {
@@ -46,6 +47,7 @@ static const char * const rockchip_board_dt_compat[] = {
 };
 
 DT_MACHINE_START(ROCKCHIP_DT, "Rockchip Cortex-A9 (Device Tree)")
+	.smp		= smp_ops(rockchip_smp_ops),
 	.init_machine	= rockchip_dt_init,
 	.init_time	= rockchip_timer_init,
 	.dt_compat	= rockchip_board_dt_compat,
-- 
1.7.10.4

Re: [PATCH v2 1/6] misc: sram: fix error path in sram_probe

[Philipp Zabel]

Hi Heiko,

Am Dienstag, den 25.06.2013, 10:46 +0200 schrieb Heiko Stübner:
> The pool is created thru devm_gen_pool_create, so the call to
> gen_pool_destroy is not necessary.
> Instead the sram-clock must be turned off again if it exists.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/misc/sram.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index d87cc91..afe66571 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)
>  	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
>  				res->start, size, -1);
>  	if (ret < 0) {
> -		gen_pool_destroy(sram->pool);

Right, thanks.

> +		if (sram->clk)
> +			clk_disable_unprepare(sram->clk);
>  		return ret;
>  	}
>  

In light of the following patch, I'd rather move the
clk_prepare_enable() call after gen_pool_add_virt() and its error path.

regards
Philipp

Re: [PATCH v2 2/6] misc: sram: add ability to mark sram sections as reserved

[Philipp Zabel]

Hi Heiko,

Am Dienstag, den 25.06.2013, 10:47 +0200 schrieb Heiko Stübner:
> Some SoCs need parts of their sram for special purposes. So while being part
> of the periphal, it should not be part of the genpool controlling the sram.
> 
> Threfore add an option mmio-sram-reserved to keep arbitary portions of the
> sram from being part of the pool.
> 
> Suggested-by: Rob Herring <robherring2@gmail.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  Documentation/devicetree/bindings/misc/sram.txt |    8 +++
>  drivers/misc/sram.c                             |   86 +++++++++++++++++++++--
>  2 files changed, 88 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/misc/sram.txt b/Documentation/devicetree/bindings/misc/sram.txt
> index 4d0a00e..eae080e 100644
> --- a/Documentation/devicetree/bindings/misc/sram.txt
> +++ b/Documentation/devicetree/bindings/misc/sram.txt
> @@ -8,9 +8,17 @@ Required properties:
>  
>  - reg : SRAM iomem address range
>  
> +Optional properties:
> +
> +- mmio-sram-reserved: ordered list of reserved chunks inside the sram that
> +  should not become part of the genalloc pool.
> +  Format is <base size>, <base size>, ...; with base being relative to the
> +  reg property base.
> +

the keyword to reserve blocks of ram is /memreserve/ - should this
property name be aligned with that?

>  Example:
>  
>  sram: sram@5c000000 {
>  	compatible = "mmio-sram";
>  	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
> +	mmio-sram-reserved = <0x0 0x100>; /* reserve 0x5c000000-0x5c000100 */
>  };
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index afe66571..5fccbe3 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -42,6 +42,8 @@ static int sram_probe(struct platform_device *pdev)
>  	struct sram_dev *sram;
>  	struct resource *res;
>  	unsigned long size;
> +	const __be32 *reserved_list = NULL;
> +	int reserved_size = 0;
>  	int ret;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -65,12 +67,89 @@ static int sram_probe(struct platform_device *pdev)
>  	if (!sram->pool)
>  		return -ENOMEM;
>  
> -	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> -				res->start, size, -1);
> -	if (ret < 0) {
> -		if (sram->clk)
> -			clk_disable_unprepare(sram->clk);
> -		return ret;
> +	if (pdev->dev.of_node) {
> +		reserved_list = of_get_property(pdev->dev.of_node,
> +						"mmio-sram-reserved",
> +						&reserved_size);
> +		if (reserved_list) {
> +			reserved_size /= sizeof(*reserved_list);
> +			if (!reserved_size || reserved_size % 2) {
> +				dev_warn(&pdev->dev, "wrong number of arguments in mmio-sram-reserved\n");
> +				reserved_list = NULL;
> +			}
> +		}
> +	}
> +
> +	if (!reserved_list) {
> +		ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> +					res->start, size, -1);
> +		if (ret < 0) {
> +			if (sram->clk)
> +				clk_disable_unprepare(sram->clk);
> +			return ret;
> +		}

Moving the clk_prepare_enable() further down would allow to avoid the
clk_disable_unprepare() in every error path,

> +	} else {
> +		unsigned int cur_start = 0;
> +		unsigned int cur_size;
> +		unsigned int rstart;
> +		unsigned int rsize;
> +		int i;
> +
> +		for (i = 0; i < reserved_size; i += 2) {
> +			/* get the next reserved block */
> +			rstart = be32_to_cpu(*reserved_list++);
> +			rsize = be32_to_cpu(*reserved_list++);
> +
> +			/* catch unsorted list entries */
> +			if (rstart < cur_start) {
> +				dev_err(&pdev->dev, "unsorted reserved list (0x%x before current 0x%x)\n",
> +					rstart, cur_start);
> +				if (sram->clk)
> +					clk_disable_unprepare(sram->clk);

like here

> +				return -EINVAL;
> +			}
> +
> +			dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
> +				 rstart, rstart + rsize);
> +
> +			/* current start is in a reserved block */
> +			if (rstart <= cur_start) {
> +				cur_start = rstart + rsize;
> +				continue;
> +			}
> +
> +			/*
> +			 * allocate the space between the current starting
> +			 * address and the following reserved block
> +			 */
> +			cur_size = rstart - cur_start;
> +
> +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> +				 cur_start, cur_start + cur_size);
> +			ret = gen_pool_add_virt(sram->pool,
> +					(unsigned long)virt_base + cur_start,
> +					res->start + cur_start, cur_size, -1);
> +			if (ret < 0) {
> +				if (sram->clk)
> +					clk_disable_unprepare(sram->clk);

and here.

> +				return ret;
> +			}
> +
> +			/* next allocation after this reserved block */
> +			cur_start = rstart + rsize;
> +		}
> +
> +		/* allocate the space after the last reserved block */
> +		if (cur_start < size) {
> +			cur_size = size - cur_start;
> +
> +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> +				 cur_start, cur_start + cur_size);
> +			ret = gen_pool_add_virt(sram->pool,
> +					(unsigned long)virt_base + cur_start,
> +					res->start + cur_start, cur_size, -1);
> +		}
> +
>  	}
>  
>  	platform_set_drvdata(pdev, sram);

Also, I think you could reduce the duplication of gen_pool_add_virt()
function calls, somehow like this:

	unsigned int cur_start = 0;
	unsigned int cur_size;
	unsigned int rstart;
	unsigned int rsize;
	int i = 0;

	if (!reserved_list)
		reserved_size = 0;

	for (i = 0; i < (reserved_size + 2); i += 2) {
		if (i < reserved_size) {
			/* get the next reserved block */
			rstart = be32_to_cpu(*reserved_list++);
			rsize = be32_to_cpu(*reserved_list++);

			/* catch unsorted list entries */
			if (rstart < cur_start) {
				dev_err(&pdev->dev,
					"unsorted reserved list (0x%x before current 0x%x)\n",
					rstart, cur_start);
				return -EINVAL;
			}

			dev_dbg(&pdev->dev,
				"found reserved block 0x%x-0x%x\n",
				rstart, rstart + rsize);
		} else {
			/* the last chunk extends to the end of the region */
			rstart = size;
		}

		/* current start is in a reserved block */
		if (rstart <= cur_start) {
			cur_start = rstart + rsize;
			continue;
		}

		/*
		 * allocate the space between the current starting
		 * address and the following reserved block, or the
		 * end of the region.
		 */
		cur_size = rstart - cur_start;

		dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
			cur_start, cur_start + cur_size);
		ret = gen_pool_add_virt(sram->pool,
				(unsigned long)virt_base + cur_start,
				res->start + cur_start, cur_size, -1);
		if (ret < 0)
			return ret;
	}

regards
Philipp

Re: [PATCH v2 2/6] misc: sram: add ability to mark sram sections as reserved

[Heiko Stübner]

Hi Philipp,

Am Dienstag, 25. Juni 2013, 12:17:05 schrieb Philipp Zabel:
> Hi Heiko,
> 
> Am Dienstag, den 25.06.2013, 10:47 +0200 schrieb Heiko Stübner:
> > Some SoCs need parts of their sram for special purposes. So while being
> > part of the periphal, it should not be part of the genpool controlling
> > the sram.
> > 
> > Threfore add an option mmio-sram-reserved to keep arbitary portions of
> > the sram from being part of the pool.
> > 
> > Suggested-by: Rob Herring <robherring2@gmail.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  Documentation/devicetree/bindings/misc/sram.txt |    8 +++
> >  drivers/misc/sram.c                             |   86
> >  +++++++++++++++++++++-- 2 files changed, 88 insertions(+), 6
> >  deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/sram.txt
> > b/Documentation/devicetree/bindings/misc/sram.txt index 4d0a00e..eae080e
> > 100644
> > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > 
> > @@ -8,9 +8,17 @@ Required properties:
> >  - reg : SRAM iomem address range
> > 
> > +Optional properties:
> > +
> > +- mmio-sram-reserved: ordered list of reserved chunks inside the sram
> > that +  should not become part of the genalloc pool.
> > +  Format is <base size>, <base size>, ...; with base being relative to
> > the +  reg property base.
> > +
> 
> the keyword to reserve blocks of ram is /memreserve/ - should this
> property name be aligned with that?

The mmio-sram-reserved name was suggested by Rob Herring, who I suppose has 
some slight experience with devicetree :-) .

I wasn't able to find real documentation on /memreserve/ but it looks more 
like it's used to reserve generic memregions, not being node-specific.
So reusing this might also cause confusion when the reserve-data now is 
relative to it's node reg.


> >  Example:
> >  
> >  sram: sram@5c000000 {
> >  
> >  	compatible = "mmio-sram";
> >  	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
> > 
> > +	mmio-sram-reserved = <0x0 0x100>; /* reserve 0x5c000000-0x5c000100 */
> > 
> >  };
> > 
> > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > index afe66571..5fccbe3 100644
> > --- a/drivers/misc/sram.c
> > +++ b/drivers/misc/sram.c
> > @@ -42,6 +42,8 @@ static int sram_probe(struct platform_device *pdev)
> > 
> >  	struct sram_dev *sram;
> >  	struct resource *res;
> >  	unsigned long size;
> > 
> > +	const __be32 *reserved_list = NULL;
> > +	int reserved_size = 0;
> > 
> >  	int ret;
> >  	
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > 
> > @@ -65,12 +67,89 @@ static int sram_probe(struct platform_device *pdev)
> > 
> >  	if (!sram->pool)
> >  	
> >  		return -ENOMEM;
> > 
> > -	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > -				res->start, size, -1);
> > -	if (ret < 0) {
> > -		if (sram->clk)
> > -			clk_disable_unprepare(sram->clk);
> > -		return ret;
> > +	if (pdev->dev.of_node) {
> > +		reserved_list = of_get_property(pdev->dev.of_node,
> > +						"mmio-sram-reserved",
> > +						&reserved_size);
> > +		if (reserved_list) {
> > +			reserved_size /= sizeof(*reserved_list);
> > +			if (!reserved_size || reserved_size % 2) {
> > +				dev_warn(&pdev->dev, "wrong number of arguments in
> > mmio-sram-reserved\n"); +				reserved_list = NULL;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (!reserved_list) {
> > +		ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > +					res->start, size, -1);
> > +		if (ret < 0) {
> > +			if (sram->clk)
> > +				clk_disable_unprepare(sram->clk);
> > +			return ret;
> > +		}
> 
> Moving the clk_prepare_enable() further down would allow to avoid the
> clk_disable_unprepare() in every error path,
> 
> > +	} else {
> > +		unsigned int cur_start = 0;
> > +		unsigned int cur_size;
> > +		unsigned int rstart;
> > +		unsigned int rsize;
> > +		int i;
> > +
> > +		for (i = 0; i < reserved_size; i += 2) {
> > +			/* get the next reserved block */
> > +			rstart = be32_to_cpu(*reserved_list++);
> > +			rsize = be32_to_cpu(*reserved_list++);
> > +
> > +			/* catch unsorted list entries */
> > +			if (rstart < cur_start) {
> > +				dev_err(&pdev->dev, "unsorted reserved list (0x%x before 
current
> > 0x%x)\n", +					rstart, cur_start);
> > +				if (sram->clk)
> > +					clk_disable_unprepare(sram->clk);
> 
> like here
> 
> > +				return -EINVAL;
> > +			}
> > +
> > +			dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
> > +				 rstart, rstart + rsize);
> > +
> > +			/* current start is in a reserved block */
> > +			if (rstart <= cur_start) {
> > +				cur_start = rstart + rsize;
> > +				continue;
> > +			}
> > +
> > +			/*
> > +			 * allocate the space between the current starting
> > +			 * address and the following reserved block
> > +			 */
> > +			cur_size = rstart - cur_start;
> > +
> > +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> > +				 cur_start, cur_start + cur_size);
> > +			ret = gen_pool_add_virt(sram->pool,
> > +					(unsigned long)virt_base + cur_start,
> > +					res->start + cur_start, cur_size, -1);
> > +			if (ret < 0) {
> > +				if (sram->clk)
> > +					clk_disable_unprepare(sram->clk);
> 
> and here.
> 
> > +				return ret;
> > +			}
> > +
> > +			/* next allocation after this reserved block */
> > +			cur_start = rstart + rsize;
> > +		}
> > +
> > +		/* allocate the space after the last reserved block */
> > +		if (cur_start < size) {
> > +			cur_size = size - cur_start;
> > +
> > +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> > +				 cur_start, cur_start + cur_size);
> > +			ret = gen_pool_add_virt(sram->pool,
> > +					(unsigned long)virt_base + cur_start,
> > +					res->start + cur_start, cur_size, -1);
> > +		}
> > +
> > 
> >  	}
> >  	
> >  	platform_set_drvdata(pdev, sram);
> 
> Also, I think you could reduce the duplication of gen_pool_add_virt()
> function calls, somehow like this:
> 
> 	unsigned int cur_start = 0;
> 	unsigned int cur_size;
> 	unsigned int rstart;
> 	unsigned int rsize;
> 	int i = 0;
> 
> 	if (!reserved_list)
> 		reserved_size = 0;
> 
> 	for (i = 0; i < (reserved_size + 2); i += 2) {
> 		if (i < reserved_size) {
> 			/* get the next reserved block */
> 			rstart = be32_to_cpu(*reserved_list++);
> 			rsize = be32_to_cpu(*reserved_list++);
> 
> 			/* catch unsorted list entries */
> 			if (rstart < cur_start) {
> 				dev_err(&pdev->dev,
> 					"unsorted reserved list (0x%x before current 0x%x)\n",
> 					rstart, cur_start);
> 				return -EINVAL;
> 			}
> 
> 			dev_dbg(&pdev->dev,
> 				"found reserved block 0x%x-0x%x\n",
> 				rstart, rstart + rsize);
> 		} else {
> 			/* the last chunk extends to the end of the region */
> 			rstart = size;
> 		}
> 
> 		/* current start is in a reserved block */
> 		if (rstart <= cur_start) {
> 			cur_start = rstart + rsize;
> 			continue;
> 		}
> 
> 		/*
> 		 * allocate the space between the current starting
> 		 * address and the following reserved block, or the
> 		 * end of the region.
> 		 */
> 		cur_size = rstart - cur_start;
> 
> 		dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> 			cur_start, cur_start + cur_size);
> 		ret = gen_pool_add_virt(sram->pool,
> 				(unsigned long)virt_base + cur_start,
> 				res->start + cur_start, cur_size, -1);
> 		if (ret < 0)
> 			return ret;
> 	}

yep, this looks nicer - same for moving the clk_prepare_enable to below this 
loop to unclutter the error-path.

So I will incorporate this in v3.


Thanks
Heiko

Re: [PATCH v2 2/6] misc: sram: add ability to mark sram sections as reserved

[Philipp Zabel]

Am Mittwoch, den 26.06.2013, 11:18 +0200 schrieb Heiko Stübner:
> Hi Philipp,
> 
> Am Dienstag, 25. Juni 2013, 12:17:05 schrieb Philipp Zabel:
> > Hi Heiko,
> > 
> > Am Dienstag, den 25.06.2013, 10:47 +0200 schrieb Heiko Stübner:
> > > Some SoCs need parts of their sram for special purposes. So while being
> > > part of the periphal, it should not be part of the genpool controlling
> > > the sram.
> > > 
> > > Threfore add an option mmio-sram-reserved to keep arbitary portions of
> > > the sram from being part of the pool.
> > > 
> > > Suggested-by: Rob Herring <robherring2@gmail.com>
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/misc/sram.txt |    8 +++
> > >  drivers/misc/sram.c                             |   86
> > >  +++++++++++++++++++++-- 2 files changed, 88 insertions(+), 6
> > >  deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/misc/sram.txt
> > > b/Documentation/devicetree/bindings/misc/sram.txt index 4d0a00e..eae080e
> > > 100644
> > > --- a/Documentation/devicetree/bindings/misc/sram.txt
> > > +++ b/Documentation/devicetree/bindings/misc/sram.txt
> > > 
> > > @@ -8,9 +8,17 @@ Required properties:
> > >  - reg : SRAM iomem address range
> > > 
> > > +Optional properties:
> > > +
> > > +- mmio-sram-reserved: ordered list of reserved chunks inside the sram
> > > that +  should not become part of the genalloc pool.
> > > +  Format is <base size>, <base size>, ...; with base being relative to
> > > the +  reg property base.
> > > +
> > 
> > the keyword to reserve blocks of ram is /memreserve/ - should this
> > property name be aligned with that?
> 
> The mmio-sram-reserved name was suggested by Rob Herring, who I suppose has 
> some slight experience with devicetree :-) .

That did sound like a suggestion from the top of his head, though.
I just wanted to point out the similarity in function, in case it is
relevant.

> I wasn't able to find real documentation on /memreserve/ but it looks more 
> like it's used to reserve generic memregions, not being node-specific.
> So reusing this might also cause confusion when the reserve-data now is 
> relative to it's node reg.

True.

> > >  Example:
> > >  
> > >  sram: sram@5c000000 {
> > >  
> > >  	compatible = "mmio-sram";
> > >  	reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
> > > 
> > > +	mmio-sram-reserved = <0x0 0x100>; /* reserve 0x5c000000-0x5c000100 */
> > > 
> > >  };
> > > 
> > > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > > index afe66571..5fccbe3 100644
> > > --- a/drivers/misc/sram.c
> > > +++ b/drivers/misc/sram.c
> > > @@ -42,6 +42,8 @@ static int sram_probe(struct platform_device *pdev)
> > > 
> > >  	struct sram_dev *sram;
> > >  	struct resource *res;
> > >  	unsigned long size;
> > > 
> > > +	const __be32 *reserved_list = NULL;
> > > +	int reserved_size = 0;
> > > 
> > >  	int ret;
> > >  	
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > 
> > > @@ -65,12 +67,89 @@ static int sram_probe(struct platform_device *pdev)
> > > 
> > >  	if (!sram->pool)
> > >  	
> > >  		return -ENOMEM;
> > > 
> > > -	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > > -				res->start, size, -1);
> > > -	if (ret < 0) {
> > > -		if (sram->clk)
> > > -			clk_disable_unprepare(sram->clk);
> > > -		return ret;
> > > +	if (pdev->dev.of_node) {
> > > +		reserved_list = of_get_property(pdev->dev.of_node,
> > > +						"mmio-sram-reserved",
> > > +						&reserved_size);
> > > +		if (reserved_list) {
> > > +			reserved_size /= sizeof(*reserved_list);
> > > +			if (!reserved_size || reserved_size % 2) {
> > > +				dev_warn(&pdev->dev, "wrong number of arguments in
> > > mmio-sram-reserved\n"); +				reserved_list = NULL;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	if (!reserved_list) {
> > > +		ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > > +					res->start, size, -1);
> > > +		if (ret < 0) {
> > > +			if (sram->clk)
> > > +				clk_disable_unprepare(sram->clk);
> > > +			return ret;
> > > +		}
> > 
> > Moving the clk_prepare_enable() further down would allow to avoid the
> > clk_disable_unprepare() in every error path,
> > 
> > > +	} else {
> > > +		unsigned int cur_start = 0;
> > > +		unsigned int cur_size;
> > > +		unsigned int rstart;
> > > +		unsigned int rsize;
> > > +		int i;
> > > +
> > > +		for (i = 0; i < reserved_size; i += 2) {
> > > +			/* get the next reserved block */
> > > +			rstart = be32_to_cpu(*reserved_list++);
> > > +			rsize = be32_to_cpu(*reserved_list++);
> > > +
> > > +			/* catch unsorted list entries */
> > > +			if (rstart < cur_start) {
> > > +				dev_err(&pdev->dev, "unsorted reserved list (0x%x before 
> current
> > > 0x%x)\n", +					rstart, cur_start);
> > > +				if (sram->clk)
> > > +					clk_disable_unprepare(sram->clk);
> > 
> > like here
> > 
> > > +				return -EINVAL;
> > > +			}
> > > +
> > > +			dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
> > > +				 rstart, rstart + rsize);
> > > +
> > > +			/* current start is in a reserved block */
> > > +			if (rstart <= cur_start) {
> > > +				cur_start = rstart + rsize;
> > > +				continue;
> > > +			}
> > > +
> > > +			/*
> > > +			 * allocate the space between the current starting
> > > +			 * address and the following reserved block
> > > +			 */
> > > +			cur_size = rstart - cur_start;
> > > +
> > > +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> > > +				 cur_start, cur_start + cur_size);
> > > +			ret = gen_pool_add_virt(sram->pool,
> > > +					(unsigned long)virt_base + cur_start,
> > > +					res->start + cur_start, cur_size, -1);
> > > +			if (ret < 0) {
> > > +				if (sram->clk)
> > > +					clk_disable_unprepare(sram->clk);
> > 
> > and here.
> > 
> > > +				return ret;
> > > +			}
> > > +
> > > +			/* next allocation after this reserved block */
> > > +			cur_start = rstart + rsize;
> > > +		}
> > > +
> > > +		/* allocate the space after the last reserved block */
> > > +		if (cur_start < size) {
> > > +			cur_size = size - cur_start;
> > > +
> > > +			dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> > > +				 cur_start, cur_start + cur_size);
> > > +			ret = gen_pool_add_virt(sram->pool,
> > > +					(unsigned long)virt_base + cur_start,
> > > +					res->start + cur_start, cur_size, -1);
> > > +		}
> > > +
> > > 
> > >  	}
> > >  	
> > >  	platform_set_drvdata(pdev, sram);
> > 
> > Also, I think you could reduce the duplication of gen_pool_add_virt()
> > function calls, somehow like this:
> > 
> > 	unsigned int cur_start = 0;
> > 	unsigned int cur_size;
> > 	unsigned int rstart;
> > 	unsigned int rsize;
> > 	int i = 0;
> > 
> > 	if (!reserved_list)
> > 		reserved_size = 0;
> > 
> > 	for (i = 0; i < (reserved_size + 2); i += 2) {
> > 		if (i < reserved_size) {
> > 			/* get the next reserved block */
> > 			rstart = be32_to_cpu(*reserved_list++);
> > 			rsize = be32_to_cpu(*reserved_list++);
> > 
> > 			/* catch unsorted list entries */
> > 			if (rstart < cur_start) {
> > 				dev_err(&pdev->dev,
> > 					"unsorted reserved list (0x%x before current 0x%x)\n",
> > 					rstart, cur_start);
> > 				return -EINVAL;
> > 			}
> > 
> > 			dev_dbg(&pdev->dev,
> > 				"found reserved block 0x%x-0x%x\n",
> > 				rstart, rstart + rsize);
> > 		} else {
> > 			/* the last chunk extends to the end of the region */
> > 			rstart = size;
> > 		}
> > 
> > 		/* current start is in a reserved block */
> > 		if (rstart <= cur_start) {
> > 			cur_start = rstart + rsize;
> > 			continue;
> > 		}
> > 
> > 		/*
> > 		 * allocate the space between the current starting
> > 		 * address and the following reserved block, or the
> > 		 * end of the region.
> > 		 */
> > 		cur_size = rstart - cur_start;
> > 
> > 		dev_dbg(&pdev->dev, "adding chunk 0x%x-0x%x\n",
> > 			cur_start, cur_start + cur_size);
> > 		ret = gen_pool_add_virt(sram->pool,
> > 				(unsigned long)virt_base + cur_start,
> > 				res->start + cur_start, cur_size, -1);
> > 		if (ret < 0)
> > 			return ret;
> > 	}
> 
> yep, this looks nicer - same for moving the clk_prepare_enable to below this 
> loop to unclutter the error-path.
> 
> So I will incorporate this in v3.

Excellent.

thanks
Philipp

Re: [PATCH v2 1/6] misc: sram: fix error path in sram_probe

[Heiko Stübner]

Hi Philipp,

Am Dienstag, 25. Juni 2013, 11:04:34 schrieb Philipp Zabel:
> Hi Heiko,
> 
> Am Dienstag, den 25.06.2013, 10:46 +0200 schrieb Heiko Stübner:
> > The pool is created thru devm_gen_pool_create, so the call to
> > gen_pool_destroy is not necessary.
> > Instead the sram-clock must be turned off again if it exists.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/misc/sram.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > index d87cc91..afe66571 100644
> > --- a/drivers/misc/sram.c
> > +++ b/drivers/misc/sram.c
> > @@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)
> > 
> >  	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> >  	
> >  				res->start, size, -1);
> >  	
> >  	if (ret < 0) {
> > 
> > -		gen_pool_destroy(sram->pool);
> 
> Right, thanks.
> 
> > +		if (sram->clk)
> > +			clk_disable_unprepare(sram->clk);
> > 
> >  		return ret;
> >  	
> >  	}
> 
> In light of the following patch, I'd rather move the
> clk_prepare_enable() call after gen_pool_add_virt() and its error path.

I'm not sure, but isn't moving the clock enablement below the pool allocation 
producing a race condition?

I.e. can the case happen that some other part wants to allocate part of the 
newly generated pool already, while the subsequent gen_pool_add_virt calls 
from the following patch are still running? ... And what will happen in this 
case, when the sram clock is still disabled?


Thanks
Heiko

Re: [PATCH v2 1/6] misc: sram: fix error path in sram_probe

[Philipp Zabel]

Hi Heiko,

Am Donnerstag, den 04.07.2013, 16:34 +0200 schrieb Heiko Stübner:
> Hi Philipp,
> 
> Am Dienstag, 25. Juni 2013, 11:04:34 schrieb Philipp Zabel:
> > Hi Heiko,
> > 
> > Am Dienstag, den 25.06.2013, 10:46 +0200 schrieb Heiko Stübner:
> > > The pool is created thru devm_gen_pool_create, so the call to
> > > gen_pool_destroy is not necessary.
> > > Instead the sram-clock must be turned off again if it exists.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > > 
> > >  drivers/misc/sram.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > > index d87cc91..afe66571 100644
> > > --- a/drivers/misc/sram.c
> > > +++ b/drivers/misc/sram.c
> > > @@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)
> > > 
> > >  	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
> > >  	
> > >  				res->start, size, -1);
> > >  	
> > >  	if (ret < 0) {
> > > 
> > > -		gen_pool_destroy(sram->pool);
> > 
> > Right, thanks.
> > 
> > > +		if (sram->clk)
> > > +			clk_disable_unprepare(sram->clk);
> > > 
> > >  		return ret;
> > >  	
> > >  	}
> > 
> > In light of the following patch, I'd rather move the
> > clk_prepare_enable() call after gen_pool_add_virt() and its error path.
> 
> I'm not sure, but isn't moving the clock enablement below the pool allocation 
> producing a race condition?

since this is a platform device, and the driver is not allowed to be
compiled as a module, it is probed at the time platform_driver_register
is called from postcore_initcall.
You'd have to go to quite some lengths to make this race by probing your
driver before mmio-sram and then asynchronously trying to allocate
memory from the pool some time later, without having made sure that the
pool exists during probe.

It would be nice to have callbacks from gen_pool_alloc/free to
enable/disable the clock.
For the time being, maybe it's the safest bet to just enable the clock
before, especially if you reorder the loop so there is only one
gen_pool_add_virt and corresponding error path.

> I.e. can the case happen that some other part wants to allocate part of the 
> newly generated pool already, while the subsequent gen_pool_add_virt calls 
> from the following patch are still running? ... And what will happen in this 
> case, when the sram clock is still disabled?

On i.MX6, the system will hang.

regards
Philipp