[PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

This series intends to add support for L2 cache on Exynos4 SoCs on boards
running under secure firmware, which requires certain initialization steps
to be done with help of firmware, as selected registers are writable only
from secure mode.

First four patches extend existing support for secure write in L2C driver
to account for design of secure firmware running on Exynos. Namely:
 1) direct read access to certain registers is needed on Exynos, because
    secure firmware calls set several registers at once,
 2) not all boards are running secure firmware, so .write_sec callback
    needs to be installed in Exynos firmware ops initialization code,
 3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
    is not allowed and so must use l2c_write_sec as well,
 4) on certain boards, default value of prefetch register is incorrect
    and must be overridden at L2C initialization.
Patches 1-3 might affect other platforms using .write_sec callback, so
I'd like to kindly ask any interested people for testing.

Further two patches add impelmentation of .write_sec for Exynos secure
firmware and necessary DT nodes to enable L2 cache.

Tested on Exynos4210-based Universal C210 and Trats (both without secure
firmware) and Exynos4412-based TRATS2 and ODROID-U3 boards (both with secure
firmware).

Changes since v1:
(https://www.mail-archive.com/linux-omap@vger.kernel.org/msg106323.html)
 - rebased onto for-next branch of linux-samsung tree,
 - changed argument order of outer_cache.write_sec() callback to match
   l2c_write_sec() function in cache-l2x0.c,
 - added support of overriding of prefetch settings to work around incorrect
   default settings on certain Exynos4x12-based boards,
 - added call to firmware to invalidate whole L2 cache before setting enable
   bit in L2C control register (required by Exynos secure firmware).

Tomasz Figa (6):
  ARM: mm: cache-l2x0: Add base address argument to write_sec callback
  ARM: Get outer cache .write_sec callback from mach_desc only if not
    NULL
  ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers
  ARM: mm: l2x0: Add support for overriding prefetch settings
  ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
  ARM: dts: exynos4: Add nodes for L2 cache controller

 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++
 arch/arm/boot/dts/exynos4210.dtsi              |  9 ++++
 arch/arm/boot/dts/exynos4x12.dtsi              | 14 ++++++
 arch/arm/include/asm/mach/arch.h               |  3 +-
 arch/arm/include/asm/outercache.h              |  2 +-
 arch/arm/kernel/irq.c                          |  3 +-
 arch/arm/mach-exynos/firmware.c                | 63 +++++++++++++++++++++++++
 arch/arm/mach-highbank/highbank.c              |  3 +-
 arch/arm/mach-omap2/omap4-common.c             |  3 +-
 arch/arm/mach-ux500/cache-l2x0.c               |  3 +-
 arch/arm/mm/cache-l2x0.c                       | 64 ++++++++++++++++++++++----
 11 files changed, 162 insertions(+), 15 deletions(-)

-- 
1.9.3

[PATCH v2 1/6] ARM: mm: cache-l2x0: Add base address argument to write_sec callback

[Tomasz Figa]

For certain platforms (e.g. Exynos) it is necessary to read back some
values from registers before they can be written (i.e. SMC calls that
set multiple registers per call), so base address of L2C controller is
needed for .write_sec operation. This patch adds base argument to
.write_sec callback so that its implementation can also access registers
directly.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/include/asm/mach/arch.h   | 3 ++-
 arch/arm/include/asm/outercache.h  | 2 +-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-omap2/omap4-common.c | 3 ++-
 arch/arm/mach-ux500/cache-l2x0.c   | 3 ++-
 arch/arm/mm/cache-l2x0.c           | 2 +-
 6 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 060a75e..fefff4d 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -46,7 +46,8 @@ struct machine_desc {
 	enum reboot_mode	reboot_mode;	/* default restart mode	*/
 	unsigned		l2c_aux_val;	/* L2 cache aux value	*/
 	unsigned		l2c_aux_mask;	/* L2 cache aux mask	*/
-	void			(*l2c_write_sec)(unsigned long, unsigned);
+	void			(*l2c_write_sec)(unsigned long,
+						 void __iomem *, unsigned);
 	struct smp_operations	*smp;		/* SMP operations	*/
 	bool			(*smp_init)(void);
 	void			(*fixup)(struct tag *, char **);
diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/outercache.h
index 891a56b..25b70b5 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -35,7 +35,7 @@ struct outer_cache_fns {
 	void (*resume)(void);
 
 	/* This is an ARM L2C thing */
-	void (*write_sec)(unsigned long, unsigned);
+	void (*write_sec)(unsigned long, void __iomem *, unsigned);
 };
 
 extern struct outer_cache_fns outer_cache;
diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c
index 8c35ae4..20cf160 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -51,7 +51,8 @@ static void __init highbank_scu_map_io(void)
 }
 
 
-static void highbank_l2c310_write_sec(unsigned long val, unsigned reg)
+static void highbank_l2c310_write_sec(unsigned long val, void __iomem *base,
+				      unsigned reg)
 {
 	if (reg == L2X0_CTRL)
 		highbank_smc1(0x102, val);
diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index 326cd98..ce04afa 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -167,7 +167,8 @@ void __iomem *omap4_get_l2cache_base(void)
 	return l2cache_base;
 }
 
-static void omap4_l2c310_write_sec(unsigned long val, unsigned reg)
+static void omap4_l2c310_write_sec(unsigned long val, void __iomem *base,
+				   unsigned reg)
 {
 	unsigned smc_op;
 
diff --git a/arch/arm/mach-ux500/cache-l2x0.c b/arch/arm/mach-ux500/cache-l2x0.c
index 842ebed..bd87108 100644
--- a/arch/arm/mach-ux500/cache-l2x0.c
+++ b/arch/arm/mach-ux500/cache-l2x0.c
@@ -35,7 +35,8 @@ static int __init ux500_l2x0_unlock(void)
 	return 0;
 }
 
-static void ux500_l2c310_write_sec(unsigned long val, unsigned reg)
+static void ux500_l2c310_write_sec(unsigned long val, void __iomem *base,
+				   unsigned reg)
 {
 	/*
 	 * We can't write to secure registers as we are in non-secure
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index efc5cab..478566b 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -72,7 +72,7 @@ static void l2c_write_sec(unsigned long val, void __iomem *base, unsigned reg)
 	if (val == readl_relaxed(base + reg))
 		return;
 	if (outer_cache.write_sec)
-		outer_cache.write_sec(val, reg);
+		outer_cache.write_sec(val, base, reg);
 	else
 		writel_relaxed(val, base + reg);
 }
-- 
1.9.3

Re: [PATCH v2 1/6] ARM: mm: cache-l2x0: Add base address argument to write_sec callback

[Linus Walleij]

On Wed, Jun 25, 2014 at 3:37 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> For certain platforms (e.g. Exynos) it is necessary to read back some
> values from registers before they can be written (i.e. SMC calls that
> set multiple registers per call), so base address of L2C controller is
> needed for .write_sec operation. This patch adds base argument to
> .write_sec callback so that its implementation can also access registers
> directly.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>

>  arch/arm/mach-ux500/cache-l2x0.c   | 3 ++-

In our case just changing the signature of the function I see so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/6] ARM: Get outer cache .write_sec callback from mach_desc only if not NULL

[Tomasz Figa]

Certain platforms (i.e. Exynos) might need to set .write_sec callback
from firmware initialization which is happenning in .init_early callback
of machine descriptor. However current code will overwrite the pointer
with whatever is present in machine descriptor, even though it can be
already set earlier. This patch fixes this by making the assignment
conditional, depending on whether current .write_sec callback is NULL.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 2c42576..e7383b9 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -125,7 +125,8 @@ void __init init_IRQ(void)
 
 	if (IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_CACHE_L2X0) &&
 	    (machine_desc->l2c_aux_mask || machine_desc->l2c_aux_val)) {
-		outer_cache.write_sec = machine_desc->l2c_write_sec;
+		if (!outer_cache.write_sec)
+			outer_cache.write_sec = machine_desc->l2c_write_sec;
 		ret = l2x0_of_init(machine_desc->l2c_aux_val,
 				   machine_desc->l2c_aux_mask);
 		if (ret)
-- 
1.9.3

[PATCH v2 3/6] ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers

[Tomasz Figa]

According to the documentation, TAG_LATENCY_CTRL and DATA_LATENCY_CTRL
registers of L2C-310 can be written only in secure mode, so
l2c_write_sec() should be used to change them, instead of plain
writel_relaxed().

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/mm/cache-l2x0.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 478566b..6600fd9 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -622,10 +622,10 @@ static void l2c310_resume(void)
 		unsigned revision;
 
 		/* restore pl310 setup */
-		writel_relaxed(l2x0_saved_regs.tag_latency,
-			       base + L310_TAG_LATENCY_CTRL);
-		writel_relaxed(l2x0_saved_regs.data_latency,
-			       base + L310_DATA_LATENCY_CTRL);
+		l2c_write_sec(l2x0_saved_regs.tag_latency,
+			       base, L310_TAG_LATENCY_CTRL);
+		l2c_write_sec(l2x0_saved_regs.data_latency,
+			       base, L310_DATA_LATENCY_CTRL);
 		writel_relaxed(l2x0_saved_regs.filter_end,
 			       base + L310_ADDR_FILTER_END);
 		writel_relaxed(l2x0_saved_regs.filter_start,
@@ -1024,20 +1024,20 @@ static void __init l2c310_of_parse(const struct device_node *np,
 
 	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
 	if (tag[0] && tag[1] && tag[2])
-		writel_relaxed(
+		l2c_write_sec(
 			L310_LATENCY_CTRL_RD(tag[0] - 1) |
 			L310_LATENCY_CTRL_WR(tag[1] - 1) |
 			L310_LATENCY_CTRL_SETUP(tag[2] - 1),
-			l2x0_base + L310_TAG_LATENCY_CTRL);
+			l2x0_base, L310_TAG_LATENCY_CTRL);
 
 	of_property_read_u32_array(np, "arm,data-latency",
 				   data, ARRAY_SIZE(data));
 	if (data[0] && data[1] && data[2])
-		writel_relaxed(
+		l2c_write_sec(
 			L310_LATENCY_CTRL_RD(data[0] - 1) |
 			L310_LATENCY_CTRL_WR(data[1] - 1) |
 			L310_LATENCY_CTRL_SETUP(data[2] - 1),
-			l2x0_base + L310_DATA_LATENCY_CTRL);
+			l2x0_base, L310_DATA_LATENCY_CTRL);
 
 	of_property_read_u32_array(np, "arm,filter-ranges",
 				   filter, ARRAY_SIZE(filter));
-- 
1.9.3

[PATCH v2 4/6] ARM: mm: l2x0: Add support for overriding prefetch settings

[Tomasz Figa]

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++++
 arch/arm/mm/cache-l2x0.c                       | 46 ++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
index b513cb8..8096fcd 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -44,6 +44,16 @@ Optional properties:
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
+- arm,double-linefill : Override double linefill enable setting. Enable if
+  non-zero, disable if zero.
+- arm,double-linefill-incr : Override double linefill on INCR read. Enable
+  if non-zero, disable if zero.
+- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
+  if non-zero, disable if zero.
+- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero,
+  disable if zero.
+- arm,prefetch-offset : Override prefetch offset value. Valid values are
+  0-7, 15, 23, and 31.
 
 Example:
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 6600fd9..fd23cce 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1018,9 +1018,12 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
 static void __init l2c310_of_parse(const struct device_node *np,
 	u32 *aux_val, u32 *aux_mask)
 {
+	bool set_prefetch = false;
 	u32 data[3] = { 0, 0, 0 };
 	u32 tag[3] = { 0, 0, 0 };
 	u32 filter[2] = { 0, 0 };
+	u32 prefetch;
+	u32 val;
 
 	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
 	if (tag[0] && tag[1] && tag[2])
@@ -1047,6 +1050,49 @@ static void __init l2c310_of_parse(const struct device_node *np,
 		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN,
 			       l2x0_base + L310_ADDR_FILTER_START);
 	}
+
+	prefetch = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
+
+	if (!of_property_read_u32(np, "arm,double-linefill", &val)) {
+		if (val)
+			prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+		else
+			prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL;
+		set_prefetch = true;
+	}
+
+	if (!of_property_read_u32(np, "arm,double-linefill-incr", &val)) {
+		if (val)
+			prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+		else
+			prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+		set_prefetch = true;
+	}
+
+	if (!of_property_read_u32(np, "arm,double-linefill-wrap", &val)) {
+		if (!val)
+			prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+		else
+			prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+		set_prefetch = true;
+	}
+
+	if (!of_property_read_u32(np, "arm,prefetch-drop", &val)) {
+		if (val)
+			prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+		else
+			prefetch &= ~L310_PREFETCH_CTRL_PREFETCH_DROP;
+		set_prefetch = true;
+	}
+
+	if (!of_property_read_u32(np, "arm,prefetch-offset", &val)) {
+		prefetch &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
+		prefetch |= val & L310_PREFETCH_CTRL_OFFSET_MASK;
+		set_prefetch = true;
+	}
+
+	if (set_prefetch)
+		l2c_write_sec(prefetch, l2x0_base, L310_PREFETCH_CTRL);
 }
 
 static const struct l2c_init_data of_l2c310_data __initconst = {
-- 
1.9.3

[PATCH v2 5/6] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

Exynos4 SoCs equipped with an L2C-310 cache controller and running under
secure firmware require certain registers of aforementioned IP to be
accessed only from secure mode. This means that SMC calls are required
for certain register writes. To handle this, an implementation of
.write_sec callback is provided by this patch.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/mach-exynos/firmware.c | 63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index eb91d23..def7bb4 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -14,7 +14,9 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 
+#include <asm/cputype.h>
 #include <asm/firmware.h>
+#include <asm/hardware/cache-l2x0.h>
 
 #include <mach/map.h>
 
@@ -70,6 +72,57 @@ static const struct firmware_ops exynos_firmware_ops = {
 	.cpu_boot		= exynos_cpu_boot,
 };
 
+static void exynos_l2_write_sec(unsigned long val, void __iomem *base,
+				unsigned reg)
+{
+	switch (reg) {
+	case L2X0_CTRL:
+		if (val & L2X0_CTRL_EN)
+			exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
+		exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
+		break;
+
+	case L2X0_AUX_CTRL:
+		exynos_smc(SMC_CMD_L2X0SETUP2,
+				readl_relaxed(base + L310_POWER_CTRL),
+				val, 0);
+		break;
+
+	case L2X0_DEBUG_CTRL:
+		exynos_smc(SMC_CMD_L2X0DEBUG, val, 0, 0);
+		break;
+
+	case L310_TAG_LATENCY_CTRL:
+		exynos_smc(SMC_CMD_L2X0SETUP1,
+				val,
+				readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+				readl_relaxed(base + L310_PREFETCH_CTRL));
+		break;
+
+	case L310_DATA_LATENCY_CTRL:
+		exynos_smc(SMC_CMD_L2X0SETUP1,
+				readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+				val,
+				readl_relaxed(base + L310_PREFETCH_CTRL));
+		break;
+
+	case L310_PREFETCH_CTRL:
+		exynos_smc(SMC_CMD_L2X0SETUP1,
+				readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+				readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+				val);
+		break;
+
+	case L310_POWER_CTRL:
+		exynos_smc(SMC_CMD_L2X0SETUP2, val,
+				readl_relaxed(base + L2X0_AUX_CTRL), 0);
+		break;
+
+	default:
+		WARN_ONCE(1, "%s: ignoring write to reg 0x%x\n", __func__, reg);
+	}
+}
+
 void __init exynos_firmware_init(void)
 {
 	struct device_node *nd;
@@ -89,4 +142,14 @@ void __init exynos_firmware_init(void)
 	pr_info("Running under secure firmware.\n");
 
 	register_firmware_ops(&exynos_firmware_ops);
+
+	/*
+	 * Exynos 4 SoCs (based on Cortex A9 and equipped with L2C-310),
+	 * running under secure firmware, require certain registers of L2
+	 * cache controller to be written in secure mode. Here .write_sec
+	 * callback is provided to perform necessary SMC calls.
+	 */
+	if (IS_ENABLED(CONFIG_CACHE_L2X0)
+	    && read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
+		outer_cache.write_sec = exynos_l2_write_sec;
 }
-- 
1.9.3

[PATCH v2 6/6] ARM: dts: exynos4: Add nodes for L2 cache controller

[Tomasz Figa]

This patch adds device tree nodes for L2 cache controller present on
Exynos4 SoCs.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi |  9 +++++++++
 arch/arm/boot/dts/exynos4x12.dtsi | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index ee3001f..99970ab 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -54,6 +54,15 @@
 		reg = <0x10023CA0 0x20>;
 	};
 
+	l2c: l2-cache-controller@10502000 {
+		compatible = "arm,pl310-cache";
+		reg = <0x10502000 0x1000>;
+		cache-unified;
+		cache-level = <2>;
+		arm,tag-latency = <2 2 1>;
+		arm,data-latency = <2 2 1>;
+	};
+
 	gic: interrupt-controller@10490000 {
 		cpu-offset = <0x8000>;
 	};
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index c5a943d..ddffefe 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -60,6 +60,20 @@
 		reg = <0x10023CA0 0x20>;
 	};
 
+	l2c: l2-cache-controller@10502000 {
+		compatible = "arm,pl310-cache";
+		reg = <0x10502000 0x1000>;
+		cache-unified;
+		cache-level = <2>;
+		arm,tag-latency = <2 2 1>;
+		arm,data-latency = <3 2 1>;
+		arm,double-linefill = <1>;
+		arm,double-linefill-incr = <0>;
+		arm,double-linefill-wrap = <1>;
+		arm,prefetch-drop = <1>;
+		arm,prefetch-offset = <7>;
+	};
+
 	clock: clock-controller@10030000 {
 		compatible = "samsung,exynos4412-clock";
 		reg = <0x10030000 0x20000>;
-- 
1.9.3

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
> This series intends to add support for L2 cache on Exynos4 SoCs on boards
> running under secure firmware, which requires certain initialization steps
> to be done with help of firmware, as selected registers are writable only
> from secure mode.

What I said in my message on June 12th applies to this series.  I'm
not having the virtual address exposed via the write_sec call.

Yes, you need to read other registers in order to use your secure
firmware implementation.  Let's fix that by providing a better write_sec
interface so you don't have to read back these registers, rather than
working around this short-coming.

That's exactly what I meant when I talked on June 12th about turning
cache-l2x0.c back into a pile of crap.  You're working around problems
rather than fixing the underlying issue, as seems to be standard
platform maintainer behaviour when things like core ARM code is
concerned.  This is why things devolve over time into piles of crap,
because platforms just hack around problems rather than fixing the
root cause of the problem.

So... I'm NAKing the entire series.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

On 25.06.2014 15:50, Russell King - ARM Linux wrote:
> On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
>> This series intends to add support for L2 cache on Exynos4 SoCs on boards
>> running under secure firmware, which requires certain initialization steps
>> to be done with help of firmware, as selected registers are writable only
>> from secure mode.
> 
> What I said in my message on June 12th applies to this series.  I'm
> not having the virtual address exposed via the write_sec call.
> 
> Yes, you need to read other registers in order to use your secure
> firmware implementation.  Let's fix that by providing a better write_sec
> interface so you don't have to read back these registers, rather than
> working around this short-coming.

Do you have anything in particular in mind? I would be glad to implement
it and send patches.

> 
> That's exactly what I meant when I talked on June 12th about turning
> cache-l2x0.c back into a pile of crap.  You're working around problems
> rather than fixing the underlying issue, as seems to be standard
> platform maintainer behaviour when things like core ARM code is
> concerned.  This is why things devolve over time into piles of crap,
> because platforms just hack around problems rather than fixing the
> root cause of the problem.

I'm not sure what part of my patches exactly is turning cache-l2x0.c
into a pile of crap. On the contrary, I believe that working around the
firmware brokenness on platform level, while keeping the core code
simple does the opposite.

However, I'll be happy to rework my series if you have some more
specific suggestions.

> So... I'm NAKing the entire series.

Your opinion is always appreciated, thanks.

Best regards,
Tomasz

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 04:13:16PM +0200, Tomasz Figa wrote:
> On 25.06.2014 15:50, Russell King - ARM Linux wrote:
> > On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
> >> This series intends to add support for L2 cache on Exynos4 SoCs on boards
> >> running under secure firmware, which requires certain initialization steps
> >> to be done with help of firmware, as selected registers are writable only
> >> from secure mode.
> > 
> > What I said in my message on June 12th applies to this series.  I'm
> > not having the virtual address exposed via the write_sec call.
> > 
> > Yes, you need to read other registers in order to use your secure
> > firmware implementation.  Let's fix that by providing a better write_sec
> > interface so you don't have to read back these registers, rather than
> > working around this short-coming.
> 
> Do you have anything in particular in mind? I would be glad to implement
> it and send patches.

As I've already said, you are not the only ones who need fuller information
to make your secure monitor calls.  So, what that implies is that rather
than the interface being "please write register X with value V", and then
having platforms work-around that by reading various registers, we need
a more flexible interface which passes the desired state.

> > That's exactly what I meant when I talked on June 12th about turning
> > cache-l2x0.c back into a pile of crap.  You're working around problems
> > rather than fixing the underlying issue, as seems to be standard
> > platform maintainer behaviour when things like core ARM code is
> > concerned.  This is why things devolve over time into piles of crap,
> > because platforms just hack around problems rather than fixing the
> > root cause of the problem.
> 
> I'm not sure what part of my patches exactly is turning cache-l2x0.c
> into a pile of crap. On the contrary, I believe that working around the
> firmware brokenness on platform level, while keeping the core code
> simple does the opposite.

What you're doing is making the future maintanence of cache-l2x0.c
harder by breaking the modularity of it.  By exposing the virtual
address of the registers, reading the registers and getting your
secure monitor to write them back, you're making assumptions about
the behaviours inside cache-l2x0.c - while this may seem to be a no-op
think about what happens a few years down the line when someone needs
to change how this stuff operates, and you've long since moved on to
new pastures and aren't around to answer questions on the exynos
implementation.

Compare that to modifying the implementation to give the secure
monitors what they want - the desired state of the secure registers
when enabling and resuming the L2 cache.  The API becomes much
clearer, and maintainable, because - from the core persective,
there isn't a load of platforms doing weird crap with an API which
doesn't really fit what they're trying to do.

So, if we accept your patches as is and let that approach continue,
then years down the line, cleaning up the crap that you've deposited
becomes much harder, and we end up either having to break Exynos
declaring it as a SoC we just don't give a damn about it, or keep the
old interface.  That is bad news which ever way you look at it.

Quite simply, if a job is worth doing, then it's worth doing well and
properly.

The reason we have l2c_write_sec() right now is that when I sorted out
the existing shitpile that platform maintainers had turned that code
into over the years, it was the interface which suited the current
implementations.  As the secure APIs are typically confidential, there
is no way to consider what other alternatives are out there, so this
is something that is going to have to remain flexible - in other words,
it needs to remain long term maintainable, so that it can change.  So,
working around it's short-comings in platform code is totally the wrong
thing to do.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

On 25.06.2014 16:37, Russell King - ARM Linux wrote:
> On Wed, Jun 25, 2014 at 04:13:16PM +0200, Tomasz Figa wrote:
>> On 25.06.2014 15:50, Russell King - ARM Linux wrote:
>>> On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
>>>> This series intends to add support for L2 cache on Exynos4 SoCs on boards
>>>> running under secure firmware, which requires certain initialization steps
>>>> to be done with help of firmware, as selected registers are writable only
>>>> from secure mode.
>>>
>>> What I said in my message on June 12th applies to this series.  I'm
>>> not having the virtual address exposed via the write_sec call.
>>>
>>> Yes, you need to read other registers in order to use your secure
>>> firmware implementation.  Let's fix that by providing a better write_sec
>>> interface so you don't have to read back these registers, rather than
>>> working around this short-coming.
>>
>> Do you have anything in particular in mind? I would be glad to implement
>> it and send patches.
> 
> As I've already said, you are not the only ones who need fuller information
> to make your secure monitor calls.  So, what that implies is that rather
> than the interface being "please write register X with value V", and then
> having platforms work-around that by reading various registers, we need
> a more flexible interface which passes the desired state.
> 

So it's still not clear to me how this should be done correctly.

One thing that comes to my mind is precomputing register values to some
kind of structure, then calling some kind of magical platform-specific
.enable() or .configure() callback, which takes the structure and, in
one shot, configures the L2C according to firmware requirements. Then
the generic code would read back those values to verify the final
configuration (as it does right now) and rest of the operation would be
identical.

Is this something you would deem acceptable?

Best regards,
Tomasz