[PATCH 0/5] ARM: Centralize the access to the SCU register

[PATCH 0/5] ARM: Centralize the access to the SCU register

[Gregory CLEMENT]

Hello,

Following the feedback I go on the patch "ARM: mvebu: Enable SCU
Speculative linefills to L2 for Armada 375/38x" :
http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993

I take the opportunity of adding new functions in smp_scu.c to
centralize the other access done on the SCU register from C file in
this file.

The first patch is a preliminary clean-up in smp_scu.c.

The second and the third patches add functions to manipulate the SCU
control register.

The forth patch use the new scu_spec_linefills_enable()
function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
was the reason of this series.

The last patch removed a direct access to the SCU register by an
access through the new scu_standby_enable() function. For this one I
have just checked that the kernel can be built using the
imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.

Thanks,

Gregory CLEMENT (4):
  ARM: smp_scu: Used defined value instead of literal constant
  ARM: smp_scu: Add the enable speculative linefills operation
  ARM: smp_scu: Add the enable standby operation
  ARM: imx6q: Use the new function scu_standby_enable()

Nadav Haklai (1):
  ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

 arch/arm/include/asm/smp_scu.h |  5 ++++
 arch/arm/kernel/smp_scu.c      | 52 ++++++++++++++++++++++++++++++++++++++----
 arch/arm/mach-imx/platsmp.c    |  7 +-----
 arch/arm/mach-mvebu/board-v7.c |  1 +
 4 files changed, 55 insertions(+), 10 deletions(-)

-- 
1.8.1.2

[PATCH 0/5] ARM: Centralize the access to the SCU register

[Gregory CLEMENT]

Hello,

Following the feedback I go on the patch "ARM: mvebu: Enable SCU
Speculative linefills to L2 for Armada 375/38x" :
http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993

I take the opportunity of adding new functions in smp_scu.c to
centralize the other access done on the SCU register from C file in
this file.

The first patch is a preliminary clean-up in smp_scu.c.

The second and the third patches add functions to manipulate the SCU
control register.

The forth patch use the new scu_spec_linefills_enable()
function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
was the reason of this series.

The last patch removed a direct access to the SCU register by an
access through the new scu_standby_enable() function. For this one I
have just checked that the kernel can be built using the
imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.

Thanks,

Gregory CLEMENT (4):
  ARM: smp_scu: Used defined value instead of literal constant
  ARM: smp_scu: Add the enable speculative linefills operation
  ARM: smp_scu: Add the enable standby operation
  ARM: imx6q: Use the new function scu_standby_enable()

Nadav Haklai (1):
  ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

 arch/arm/include/asm/smp_scu.h |  5 ++++
 arch/arm/kernel/smp_scu.c      | 52 ++++++++++++++++++++++++++++++++++++++----
 arch/arm/mach-imx/platsmp.c    |  7 +-----
 arch/arm/mach-mvebu/board-v7.c |  1 +
 4 files changed, 55 insertions(+), 10 deletions(-)

-- 
1.8.1.2

[PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Gregory CLEMENT]

The first bit of the SCU control register is actually the enable
it. So let's name it instead of using literal constant.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/kernel/smp_scu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 1aafa0d785eb..cfea41b41ad0 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -17,6 +17,7 @@
 #include <asm/cputype.h>
 
 #define SCU_CTRL		0x00
+#define SCU_CTRL_ENABLE		    BIT(1)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
 	/* Cortex-A9 only */
 	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
 		scu_ctrl = readl_relaxed(scu_base + 0x30);
-		if (!(scu_ctrl & 1))
-			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
+		if (!(scu_ctrl & SCU_CTRL_ENABLE))
+			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
+				scu_base + 0x30);
 	}
 #endif
 
 	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
 	/* already enabled? */
-	if (scu_ctrl & 1)
+	if (scu_ctrl & SCU_CTRL_ENABLE)
 		return;
 
-	scu_ctrl |= 1;
+	scu_ctrl |= SCU_CTRL_ENABLE;
 	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
 
 	/*
-- 
1.8.1.2

[PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Gregory CLEMENT]

The first bit of the SCU control register is actually the enable
it. So let's name it instead of using literal constant.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/kernel/smp_scu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 1aafa0d785eb..cfea41b41ad0 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -17,6 +17,7 @@
 #include <asm/cputype.h>
 
 #define SCU_CTRL		0x00
+#define SCU_CTRL_ENABLE		    BIT(1)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
 	/* Cortex-A9 only */
 	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
 		scu_ctrl = readl_relaxed(scu_base + 0x30);
-		if (!(scu_ctrl & 1))
-			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
+		if (!(scu_ctrl & SCU_CTRL_ENABLE))
+			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
+				scu_base + 0x30);
 	}
 #endif
 
 	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
 	/* already enabled? */
-	if (scu_ctrl & 1)
+	if (scu_ctrl & SCU_CTRL_ENABLE)
 		return;
 
-	scu_ctrl |= 1;
+	scu_ctrl |= SCU_CTRL_ENABLE;
 	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
 
 	/*
-- 
1.8.1.2

[PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Gregory CLEMENT]

Quotin the ARM datasheet "When set, coherent linefill requests are
sent speculatively to the L2C-310 in parallel with the tag look-up. If
the tag look-up misses, the confirmed linefill is sent to the L2C-310
and gets RDATA earlier because the data request was already initiated
by the speculative request. "

Some SoC (such as the Armada 375/38x) can benefit of this feature. As
this is something related to the Cortex A9 and not specific to a SoC,
we can expose it in a common place.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/include/asm/smp_scu.h |  3 +++
 arch/arm/kernel/smp_scu.c      | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 0393fbab8dd5..d9650dce48c7 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -26,6 +26,7 @@ static inline unsigned long scu_a9_get_base(void)
 #ifdef CONFIG_HAVE_ARM_SCU
 unsigned int scu_get_core_count(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);
+void scu_spec_linefills_enable(void __iomem *scu_base, bool enable);
 #else
 static inline unsigned int scu_get_core_count(void __iomem *scu_base)
 {
@@ -35,6 +36,8 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 {
 	return -EINVAL;
 }
+static inline void scu_spec_linefills_enable(void __iomem *scu_base,
+					bool enable) {}
 #endif
 
 #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index cfea41b41ad0..3fd21a495028 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -18,6 +18,7 @@
 
 #define SCU_CTRL		0x00
 #define SCU_CTRL_ENABLE		    BIT(1)
+#define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -88,3 +89,24 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 
 	return 0;
 }
+
+/*
+ * When enabled, coherent linefill requests are sent speculatively to
+ * the L2C-310 in parallel with the tag look-up
+ *
+ */
+void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
+{
+	u32 scu_ctrl;
+
+	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
+	/* already enabled? */
+	if (scu_ctrl & SCU_CTRL_ENABLE)
+		return;
+	if (enable)
+		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
+	else
+		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
+
+	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
+}
-- 
1.8.1.2

[PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Gregory CLEMENT]

Quotin the ARM datasheet "When set, coherent linefill requests are
sent speculatively to the L2C-310 in parallel with the tag look-up. If
the tag look-up misses, the confirmed linefill is sent to the L2C-310
and gets RDATA earlier because the data request was already initiated
by the speculative request. "

Some SoC (such as the Armada 375/38x) can benefit of this feature. As
this is something related to the Cortex A9 and not specific to a SoC,
we can expose it in a common place.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/include/asm/smp_scu.h |  3 +++
 arch/arm/kernel/smp_scu.c      | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index 0393fbab8dd5..d9650dce48c7 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -26,6 +26,7 @@ static inline unsigned long scu_a9_get_base(void)
 #ifdef CONFIG_HAVE_ARM_SCU
 unsigned int scu_get_core_count(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);
+void scu_spec_linefills_enable(void __iomem *scu_base, bool enable);
 #else
 static inline unsigned int scu_get_core_count(void __iomem *scu_base)
 {
@@ -35,6 +36,8 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 {
 	return -EINVAL;
 }
+static inline void scu_spec_linefills_enable(void __iomem *scu_base,
+					bool enable) {}
 #endif
 
 #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index cfea41b41ad0..3fd21a495028 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -18,6 +18,7 @@
 
 #define SCU_CTRL		0x00
 #define SCU_CTRL_ENABLE		    BIT(1)
+#define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -88,3 +89,24 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 
 	return 0;
 }
+
+/*
+ * When enabled, coherent linefill requests are sent speculatively to
+ * the L2C-310 in parallel with the tag look-up
+ *
+ */
+void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
+{
+	u32 scu_ctrl;
+
+	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
+	/* already enabled? */
+	if (scu_ctrl & SCU_CTRL_ENABLE)
+		return;
+	if (enable)
+		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
+	else
+		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
+
+	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
+}
-- 
1.8.1.2

[PATCH 3/5] ARM: smp_scu: Add the enable standby operation

[Gregory CLEMENT]

Quoting the ARM datasheet: "When set, SCU CLK is turned off when all
processors are in WFI mode, there is no pending request on the ACP, if
implemented, and there is no remaining activity in the SCU.

When SCU CLK is off, ARREADYS, AWREADYS and WREADYS on the ACP are
forced LOW. The clock is turned on when any processor leaves WFI mode,
or if there is a new request on the ACP."

This feature is currently used by imx6 SoC. This patch add this
operation inside smp_scu in order to centralized all the access to SCU
in the same place.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/include/asm/smp_scu.h |  2 ++
 arch/arm/kernel/smp_scu.c      | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index d9650dce48c7..d9694aa7aaf7 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -27,6 +27,7 @@ static inline unsigned long scu_a9_get_base(void)
 unsigned int scu_get_core_count(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);
 void scu_spec_linefills_enable(void __iomem *scu_base, bool enable);
+void scu_standby_enable(void __iomem *scu_base, bool enable);
 #else
 static inline unsigned int scu_get_core_count(void __iomem *scu_base)
 {
@@ -38,6 +39,7 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 }
 static inline void scu_spec_linefills_enable(void __iomem *scu_base,
 					bool enable) {}
+static inline void scu_standby_enable(void __iomem *scu_base, bool enable) {}
 #endif
 
 #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 3fd21a495028..de68dbf8fc66 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -19,6 +19,7 @@
 #define SCU_CTRL		0x00
 #define SCU_CTRL_ENABLE		    BIT(1)
 #define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
+#define SCU_CTRL_STANDBY_ENABLE	    BIT(5)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -110,3 +111,22 @@ void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
 
 	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
 }
+
+/*
+ * When enabled, SCU CLK is turned off when all processors are in WFI
+ * mode. The clock is turned on when any processor leaves WFI mode.
+ *
+ */
+void scu_standby_enable(void __iomem *scu_base, bool enable)
+{
+	u32 scu_ctrl;
+
+	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
+
+	if (enable)
+		scu_ctrl |= SCU_CTRL_STANDBY_ENABLE;
+	else
+		scu_ctrl &= ~SCU_CTRL_STANDBY_ENABLE;
+
+	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
+}
-- 
1.8.1.2

[PATCH 3/5] ARM: smp_scu: Add the enable standby operation

[Gregory CLEMENT]

Quoting the ARM datasheet: "When set, SCU CLK is turned off when all
processors are in WFI mode, there is no pending request on the ACP, if
implemented, and there is no remaining activity in the SCU.

When SCU CLK is off, ARREADYS, AWREADYS and WREADYS on the ACP are
forced LOW. The clock is turned on when any processor leaves WFI mode,
or if there is a new request on the ACP."

This feature is currently used by imx6 SoC. This patch add this
operation inside smp_scu in order to centralized all the access to SCU
in the same place.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/include/asm/smp_scu.h |  2 ++
 arch/arm/kernel/smp_scu.c      | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
index d9650dce48c7..d9694aa7aaf7 100644
--- a/arch/arm/include/asm/smp_scu.h
+++ b/arch/arm/include/asm/smp_scu.h
@@ -27,6 +27,7 @@ static inline unsigned long scu_a9_get_base(void)
 unsigned int scu_get_core_count(void __iomem *);
 int scu_power_mode(void __iomem *, unsigned int);
 void scu_spec_linefills_enable(void __iomem *scu_base, bool enable);
+void scu_standby_enable(void __iomem *scu_base, bool enable);
 #else
 static inline unsigned int scu_get_core_count(void __iomem *scu_base)
 {
@@ -38,6 +39,7 @@ static inline int scu_power_mode(void __iomem *scu_base, unsigned int mode)
 }
 static inline void scu_spec_linefills_enable(void __iomem *scu_base,
 					bool enable) {}
+static inline void scu_standby_enable(void __iomem *scu_base, bool enable) {}
 #endif
 
 #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 3fd21a495028..de68dbf8fc66 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -19,6 +19,7 @@
 #define SCU_CTRL		0x00
 #define SCU_CTRL_ENABLE		    BIT(1)
 #define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
+#define SCU_CTRL_STANDBY_ENABLE	    BIT(5)
 #define SCU_CONFIG		0x04
 #define SCU_CPU_STATUS		0x08
 #define SCU_INVALIDATE		0x0c
@@ -110,3 +111,22 @@ void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
 
 	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
 }
+
+/*
+ * When enabled, SCU CLK is turned off when all processors are in WFI
+ * mode. The clock is turned on when any processor leaves WFI mode.
+ *
+ */
+void scu_standby_enable(void __iomem *scu_base, bool enable)
+{
+	u32 scu_ctrl;
+
+	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
+
+	if (enable)
+		scu_ctrl |= SCU_CTRL_STANDBY_ENABLE;
+	else
+		scu_ctrl &= ~SCU_CTRL_STANDBY_ENABLE;
+
+	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
+}
-- 
1.8.1.2

[PATCH 4/5] ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

[Gregory CLEMENT]

From: Nadav Haklai <nadavh@marvell.com>

For Armada 375 and Armada 38x SoCs, enable the SCU Speculative
linefills.

This feature may improve overall system performance.

[Gregory: modify the title of the patch and no more copy the defintion
of the bit here]

[Gregory: use the new function scu_spec_linefills_enable instead of
implementing SCU specific feature at board level]

Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/board-v7.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 8bb742fdf5ca..a24ea105e709 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -45,6 +45,7 @@ static void __init mvebu_scu_enable(void)
 		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
 	if (np) {
 		scu_base = of_iomap(np, 0);
+		scu_spec_linefills_enable(scu_base, true);
 		scu_enable(scu_base);
 		of_node_put(np);
 	}
-- 
1.8.1.2

[PATCH 4/5] ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

[Gregory CLEMENT]

From: Nadav Haklai <nadavh@marvell.com>

For Armada 375 and Armada 38x SoCs, enable the SCU Speculative
linefills.

This feature may improve overall system performance.

[Gregory: modify the title of the patch and no more copy the defintion
of the bit here]

[Gregory: use the new function scu_spec_linefills_enable instead of
implementing SCU specific feature at board level]

Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/board-v7.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 8bb742fdf5ca..a24ea105e709 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -45,6 +45,7 @@ static void __init mvebu_scu_enable(void)
 		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
 	if (np) {
 		scu_base = of_iomap(np, 0);
+		scu_spec_linefills_enable(scu_base, true);
 		scu_enable(scu_base);
 		of_node_put(np);
 	}
-- 
1.8.1.2

[PATCH 5/5] ARM: imx6q: Use the new function scu_standby_enable()

[Gregory CLEMENT]

Enabling the SCU standby is now done in smp_scu.c. We don't need
anymore to manipulate the SCU register outside of this file.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-imx/platsmp.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 5b57c17c06bd..d57ff2c24c2e 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -20,8 +20,6 @@
 #include "common.h"
 #include "hardware.h"
 
-#define SCU_STANDBY_ENABLE	(1 << 5)
-
 u32 g_diag_reg;
 static void __iomem *scu_base;
 
@@ -47,10 +45,7 @@ void __init imx_scu_map_io(void)
 
 void imx_scu_standby_enable(void)
 {
-	u32 val = readl_relaxed(scu_base);
-
-	val |= SCU_STANDBY_ENABLE;
-	writel_relaxed(val, scu_base);
+	scu_standby_enable(scu_base, true);
 }
 
 static int imx_boot_secondary(unsigned int cpu, struct task_struct *idle)
-- 
1.8.1.2

[PATCH 5/5] ARM: imx6q: Use the new function scu_standby_enable()

[Gregory CLEMENT]

Enabling the SCU standby is now done in smp_scu.c. We don't need
anymore to manipulate the SCU register outside of this file.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-imx/platsmp.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 5b57c17c06bd..d57ff2c24c2e 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -20,8 +20,6 @@
 #include "common.h"
 #include "hardware.h"
 
-#define SCU_STANDBY_ENABLE	(1 << 5)
-
 u32 g_diag_reg;
 static void __iomem *scu_base;
 
@@ -47,10 +45,7 @@ void __init imx_scu_map_io(void)
 
 void imx_scu_standby_enable(void)
 {
-	u32 val = readl_relaxed(scu_base);
-
-	val |= SCU_STANDBY_ENABLE;
-	writel_relaxed(val, scu_base);
+	scu_standby_enable(scu_base, true);
 }
 
 static int imx_boot_secondary(unsigned int cpu, struct task_struct *idle)
-- 
1.8.1.2

Re: [PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Gregory CLEMENT]

On 27/06/2014 00:43, Gregory CLEMENT wrote:
> The first bit of the SCU control register is actually the enable
> it. So let's name it instead of using literal constant.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/kernel/smp_scu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 1aafa0d785eb..cfea41b41ad0 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -17,6 +17,7 @@
>  #include <asm/cputype.h>
>  
>  #define SCU_CTRL		0x00
> +#define SCU_CTRL_ENABLE		    BIT(1)

As Ezequiel rightly pointed it to me, this line is obviously wrong!
it should be
+#define SCU_CTRL_ENABLE		    BIT(0)

>  #define SCU_CONFIG		0x04
>  #define SCU_CPU_STATUS		0x08
>  #define SCU_INVALIDATE		0x0c
> @@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
>  	/* Cortex-A9 only */
>  	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
>  		scu_ctrl = readl_relaxed(scu_base + 0x30);
> -		if (!(scu_ctrl & 1))
> -			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
> +		if (!(scu_ctrl & SCU_CTRL_ENABLE))
> +			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
> +				scu_base + 0x30);
>  	}
>  #endif
>  
>  	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
>  	/* already enabled? */
> -	if (scu_ctrl & 1)
> +	if (scu_ctrl & SCU_CTRL_ENABLE)
>  		return;
>  
> -	scu_ctrl |= 1;
> +	scu_ctrl |= SCU_CTRL_ENABLE;
>  	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
>  
>  	/*
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Gregory CLEMENT]

On 27/06/2014 00:43, Gregory CLEMENT wrote:
> The first bit of the SCU control register is actually the enable
> it. So let's name it instead of using literal constant.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/kernel/smp_scu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 1aafa0d785eb..cfea41b41ad0 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -17,6 +17,7 @@
>  #include <asm/cputype.h>
>  
>  #define SCU_CTRL		0x00
> +#define SCU_CTRL_ENABLE		    BIT(1)

As Ezequiel rightly pointed it to me, this line is obviously wrong!
it should be
+#define SCU_CTRL_ENABLE		    BIT(0)

>  #define SCU_CONFIG		0x04
>  #define SCU_CPU_STATUS		0x08
>  #define SCU_INVALIDATE		0x0c
> @@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
>  	/* Cortex-A9 only */
>  	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
>  		scu_ctrl = readl_relaxed(scu_base + 0x30);
> -		if (!(scu_ctrl & 1))
> -			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
> +		if (!(scu_ctrl & SCU_CTRL_ENABLE))
> +			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
> +				scu_base + 0x30);
>  	}
>  #endif
>  
>  	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
>  	/* already enabled? */
> -	if (scu_ctrl & 1)
> +	if (scu_ctrl & SCU_CTRL_ENABLE)
>  		return;
>  
> -	scu_ctrl |= 1;
> +	scu_ctrl |= SCU_CTRL_ENABLE;
>  	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
>  
>  	/*
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/5] ARM: Centralize the access to the SCU register

[Rob Herring]

On Thu, Jun 26, 2014 at 5:43 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hello,
>
> Following the feedback I go on the patch "ARM: mvebu: Enable SCU
> Speculative linefills to L2 for Armada 375/38x" :
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993
>
> I take the opportunity of adding new functions in smp_scu.c to
> centralize the other access done on the SCU register from C file in
> this file.
>
> The first patch is a preliminary clean-up in smp_scu.c.
>
> The second and the third patches add functions to manipulate the SCU
> control register.
>
> The forth patch use the new scu_spec_linefills_enable()
> function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
> was the reason of this series.
>
> The last patch removed a direct access to the SCU register by an
> access through the new scu_standby_enable() function. For this one I
> have just checked that the kernel can be built using the
> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.

Why would we not just turn on these 2 features unconditionally? If we
don't know of any platform where they are broken, then we should just
enable them. We can add these functions only if necessary later.

Rob

[PATCH 0/5] ARM: Centralize the access to the SCU register

[Rob Herring]

On Thu, Jun 26, 2014 at 5:43 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hello,
>
> Following the feedback I go on the patch "ARM: mvebu: Enable SCU
> Speculative linefills to L2 for Armada 375/38x" :
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993
>
> I take the opportunity of adding new functions in smp_scu.c to
> centralize the other access done on the SCU register from C file in
> this file.
>
> The first patch is a preliminary clean-up in smp_scu.c.
>
> The second and the third patches add functions to manipulate the SCU
> control register.
>
> The forth patch use the new scu_spec_linefills_enable()
> function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
> was the reason of this series.
>
> The last patch removed a direct access to the SCU register by an
> access through the new scu_standby_enable() function. For this one I
> have just checked that the kernel can be built using the
> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.

Why would we not just turn on these 2 features unconditionally? If we
don't know of any platform where they are broken, then we should just
enable them. We can add these functions only if necessary later.

Rob

Re: [PATCH 0/5] ARM: Centralize the access to the SCU register

[Gregory CLEMENT]

On 27/06/2014 00:56, Rob Herring wrote:
> On Thu, Jun 26, 2014 at 5:43 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hello,
>>
>> Following the feedback I go on the patch "ARM: mvebu: Enable SCU
>> Speculative linefills to L2 for Armada 375/38x" :
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993
>>
>> I take the opportunity of adding new functions in smp_scu.c to
>> centralize the other access done on the SCU register from C file in
>> this file.
>>
>> The first patch is a preliminary clean-up in smp_scu.c.
>>
>> The second and the third patches add functions to manipulate the SCU
>> control register.
>>
>> The forth patch use the new scu_spec_linefills_enable()
>> function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
>> was the reason of this series.
>>
>> The last patch removed a direct access to the SCU register by an
>> access through the new scu_standby_enable() function. For this one I
>> have just checked that the kernel can be built using the
>> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.
> 
> Why would we not just turn on these 2 features unconditionally? If we

You mean in scu_enbale() ?

> don't know of any platform where they are broken, then we should just

At least on some imx6 SCU standby is broken according to the code and
the comments.

> enable them. We can add these functions only if necessary later.
> 
> Rob
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/5] ARM: Centralize the access to the SCU register

[Gregory CLEMENT]

On 27/06/2014 00:56, Rob Herring wrote:
> On Thu, Jun 26, 2014 at 5:43 PM, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hello,
>>
>> Following the feedback I go on the patch "ARM: mvebu: Enable SCU
>> Speculative linefills to L2 for Armada 375/38x" :
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/335961/focus=335993
>>
>> I take the opportunity of adding new functions in smp_scu.c to
>> centralize the other access done on the SCU register from C file in
>> this file.
>>
>> The first patch is a preliminary clean-up in smp_scu.c.
>>
>> The second and the third patches add functions to manipulate the SCU
>> control register.
>>
>> The forth patch use the new scu_spec_linefills_enable()
>> function. Enabling SCU Speculative linefills to L2 for Armada 375/38x
>> was the reason of this series.
>>
>> The last patch removed a direct access to the SCU register by an
>> access through the new scu_standby_enable() function. For this one I
>> have just checked that the kernel can be built using the
>> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.
> 
> Why would we not just turn on these 2 features unconditionally? If we

You mean in scu_enbale() ?

> don't know of any platform where they are broken, then we should just

At least on some imx6 SCU standby is broken according to the code and
the comments.

> enable them. We can add these functions only if necessary later.
> 
> Rob
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Jason Cooper]

Gregory,

Since you'll be resending this:

On Fri, Jun 27, 2014 at 12:55:47AM +0200, Gregory CLEMENT wrote:
> On 27/06/2014 00:43, Gregory CLEMENT wrote:
> > The first bit of the SCU control register is actually the enable
> > it. So let's name it instead of using literal constant.

nit: s/^it/bit/

thx,

Jason.

> > 
> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >  arch/arm/kernel/smp_scu.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 1aafa0d785eb..cfea41b41ad0 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -17,6 +17,7 @@
> >  #include <asm/cputype.h>
> >  
> >  #define SCU_CTRL		0x00
> > +#define SCU_CTRL_ENABLE		    BIT(1)
> 
> As Ezequiel rightly pointed it to me, this line is obviously wrong!
> it should be
> +#define SCU_CTRL_ENABLE		    BIT(0)
> 
> >  #define SCU_CONFIG		0x04
> >  #define SCU_CPU_STATUS		0x08
> >  #define SCU_INVALIDATE		0x0c
> > @@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
> >  	/* Cortex-A9 only */
> >  	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
> >  		scu_ctrl = readl_relaxed(scu_base + 0x30);
> > -		if (!(scu_ctrl & 1))
> > -			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
> > +		if (!(scu_ctrl & SCU_CTRL_ENABLE))
> > +			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
> > +				scu_base + 0x30);
> >  	}
> >  #endif
> >  
> >  	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
> >  	/* already enabled? */
> > -	if (scu_ctrl & 1)
> > +	if (scu_ctrl & SCU_CTRL_ENABLE)
> >  		return;
> >  
> > -	scu_ctrl |= 1;
> > +	scu_ctrl |= SCU_CTRL_ENABLE;
> >  	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
> >  
> >  	/*
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

[PATCH 1/5] ARM: smp_scu: Used defined value instead of literal constant

[Jason Cooper]

Gregory,

Since you'll be resending this:

On Fri, Jun 27, 2014 at 12:55:47AM +0200, Gregory CLEMENT wrote:
> On 27/06/2014 00:43, Gregory CLEMENT wrote:
> > The first bit of the SCU control register is actually the enable
> > it. So let's name it instead of using literal constant.

nit: s/^it/bit/

thx,

Jason.

> > 
> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >  arch/arm/kernel/smp_scu.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 1aafa0d785eb..cfea41b41ad0 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -17,6 +17,7 @@
> >  #include <asm/cputype.h>
> >  
> >  #define SCU_CTRL		0x00
> > +#define SCU_CTRL_ENABLE		    BIT(1)
> 
> As Ezequiel rightly pointed it to me, this line is obviously wrong!
> it should be
> +#define SCU_CTRL_ENABLE		    BIT(0)
> 
> >  #define SCU_CONFIG		0x04
> >  #define SCU_CPU_STATUS		0x08
> >  #define SCU_INVALIDATE		0x0c
> > @@ -43,17 +44,18 @@ void scu_enable(void __iomem *scu_base)
> >  	/* Cortex-A9 only */
> >  	if ((read_cpuid_id() & 0xff0ffff0) == 0x410fc090) {
> >  		scu_ctrl = readl_relaxed(scu_base + 0x30);
> > -		if (!(scu_ctrl & 1))
> > -			writel_relaxed(scu_ctrl | 0x1, scu_base + 0x30);
> > +		if (!(scu_ctrl & SCU_CTRL_ENABLE))
> > +			writel_relaxed(scu_ctrl | SCU_CTRL_ENABLE,
> > +				scu_base + 0x30);
> >  	}
> >  #endif
> >  
> >  	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
> >  	/* already enabled? */
> > -	if (scu_ctrl & 1)
> > +	if (scu_ctrl & SCU_CTRL_ENABLE)
> >  		return;
> >  
> > -	scu_ctrl |= 1;
> > +	scu_ctrl |= SCU_CTRL_ENABLE;
> >  	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
> >  
> >  	/*
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

Re: [PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 00:43:25 +0200, Gregory CLEMENT wrote:
> Quotin the ARM datasheet "When set, coherent linefill requests are

Quotin -> Quoting

> sent speculatively to the L2C-310 in parallel with the tag look-up. If
> the tag look-up misses, the confirmed linefill is sent to the L2C-310
> and gets RDATA earlier because the data request was already initiated
> by the speculative request. "
> 
> Some SoC (such as the Armada 375/38x) can benefit of this feature. As

benefit of -> benefit from.

>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index cfea41b41ad0..3fd21a495028 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -18,6 +18,7 @@
>  
>  #define SCU_CTRL		0x00
>  #define SCU_CTRL_ENABLE		    BIT(1)
> +#define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
>  #define SCU_CONFIG		0x04
>  #define SCU_CPU_STATUS		0x08
>  #define SCU_INVALIDATE		0x0c
> @@ -88,3 +89,24 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>  
>  	return 0;
>  }
> +
> +/*
> + * When enabled, coherent linefill requests are sent speculatively to
> + * the L2C-310 in parallel with the tag look-up
> + *
> + */
> +void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
> +{
> +	u32 scu_ctrl;
> +
> +	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
> +	/* already enabled? */

Comment not needed, since SCU_CTRL_ENABLE already documents what's
happening. Or a more useful comment would be: "We cannot change the SCU
configuration while it is enabled".

> +	if (scu_ctrl & SCU_CTRL_ENABLE)
> +		return;

Return an error in this case maybe?

> +	if (enable)
> +		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
> +	else
> +		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
> +
> +	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
> +}

Instead of having a separate function to do this (and the standby
operation), what about doing that directly in scu_enable() ? Either
unconditionally if that is fine for all SCU users, or through a flags
argument?

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 00:43:25 +0200, Gregory CLEMENT wrote:
> Quotin the ARM datasheet "When set, coherent linefill requests are

Quotin -> Quoting

> sent speculatively to the L2C-310 in parallel with the tag look-up. If
> the tag look-up misses, the confirmed linefill is sent to the L2C-310
> and gets RDATA earlier because the data request was already initiated
> by the speculative request. "
> 
> Some SoC (such as the Armada 375/38x) can benefit of this feature. As

benefit of -> benefit from.

>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index cfea41b41ad0..3fd21a495028 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -18,6 +18,7 @@
>  
>  #define SCU_CTRL		0x00
>  #define SCU_CTRL_ENABLE		    BIT(1)
> +#define SCU_CTRL_SPEC_LINEFILLS	    BIT(3)
>  #define SCU_CONFIG		0x04
>  #define SCU_CPU_STATUS		0x08
>  #define SCU_INVALIDATE		0x0c
> @@ -88,3 +89,24 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
>  
>  	return 0;
>  }
> +
> +/*
> + * When enabled, coherent linefill requests are sent speculatively to
> + * the L2C-310 in parallel with the tag look-up
> + *
> + */
> +void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
> +{
> +	u32 scu_ctrl;
> +
> +	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
> +	/* already enabled? */

Comment not needed, since SCU_CTRL_ENABLE already documents what's
happening. Or a more useful comment would be: "We cannot change the SCU
configuration while it is enabled".

> +	if (scu_ctrl & SCU_CTRL_ENABLE)
> +		return;

Return an error in this case maybe?

> +	if (enable)
> +		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
> +	else
> +		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
> +
> +	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
> +}

Instead of having a separate function to do this (and the standby
operation), what about doing that directly in scu_enable() ? Either
unconditionally if that is fine for all SCU users, or through a flags
argument?

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4/5] ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 00:43:27 +0200, Gregory CLEMENT wrote:

> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> index 8bb742fdf5ca..a24ea105e709 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -45,6 +45,7 @@ static void __init mvebu_scu_enable(void)
>  		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
>  	if (np) {
>  		scu_base = of_iomap(np, 0);
> +		scu_spec_linefills_enable(scu_base, true);

I find it rather weird to have a function call <foo>_enable() take a
boolean parameter to indicate whether <foo> should be enabled or
disabled. You should choose between:

	scu_spec_linefills_ctrl(scu_base, true/false)

(or some better suffix than _ctrl), or:

	scu_spec_linefills_enable(scu_base)
	scu_spec_linefills_disable(scu_base)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4/5] ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 00:43:27 +0200, Gregory CLEMENT wrote:

> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> index 8bb742fdf5ca..a24ea105e709 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -45,6 +45,7 @@ static void __init mvebu_scu_enable(void)
>  		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
>  	if (np) {
>  		scu_base = of_iomap(np, 0);
> +		scu_spec_linefills_enable(scu_base, true);

I find it rather weird to have a function call <foo>_enable() take a
boolean parameter to indicate whether <foo> should be enabled or
disabled. You should choose between:

	scu_spec_linefills_ctrl(scu_base, true/false)

(or some better suffix than _ctrl), or:

	scu_spec_linefills_enable(scu_base)
	scu_spec_linefills_disable(scu_base)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Gregory CLEMENT]

Hi Thomas,>> + */
>> +void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
>> +{
>> +	u32 scu_ctrl;
>> +
>> +	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
>> +	/* already enabled? */
> 
> Comment not needed, since SCU_CTRL_ENABLE already documents what's
> happening. Or a more useful comment would be: "We cannot change the SCU
> configuration while it is enabled".

Right, however I need to figure out if it is really the case. Because in
ARM documentation about the SCU control register, I didn't find any mention
of this restriction.

> 
>> +	if (scu_ctrl & SCU_CTRL_ENABLE)
>> +		return;
> 
> Return an error in this case maybe?

If it failed we just don't benefit of an optimization, it won't prevent the
system working. And also, we can't do anything more if it failed. However
it could be nice to let the calling function know that it failed.

> 
>> +	if (enable)
>> +		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
>> +	else
>> +		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
>> +
>> +	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
>> +}
> 
> Instead of having a separate function to do this (and the standby
> operation), what about doing that directly in scu_enable() ? Either
> unconditionally if that is fine for all SCU users, or through a flags
> argument?

OK using a flag argument makes sens indeed. About setting it unconditionally,
I would prefer not taking the risk to break the other platforms.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/5] ARM: smp_scu: Add the enable speculative linefills operation

[Gregory CLEMENT]

Hi Thomas,>> + */
>> +void scu_spec_linefills_enable(void __iomem *scu_base, bool enable)
>> +{
>> +	u32 scu_ctrl;
>> +
>> +	scu_ctrl = readl_relaxed(scu_base + SCU_CTRL);
>> +	/* already enabled? */
> 
> Comment not needed, since SCU_CTRL_ENABLE already documents what's
> happening. Or a more useful comment would be: "We cannot change the SCU
> configuration while it is enabled".

Right, however I need to figure out if it is really the case. Because in
ARM documentation about the SCU control register, I didn't find any mention
of this restriction.

> 
>> +	if (scu_ctrl & SCU_CTRL_ENABLE)
>> +		return;
> 
> Return an error in this case maybe?

If it failed we just don't benefit of an optimization, it won't prevent the
system working. And also, we can't do anything more if it failed. However
it could be nice to let the calling function know that it failed.

> 
>> +	if (enable)
>> +		scu_ctrl |= SCU_CTRL_SPEC_LINEFILLS;
>> +	else
>> +		scu_ctrl &= ~SCU_CTRL_SPEC_LINEFILLS;
>> +
>> +	writel_relaxed(scu_ctrl, scu_base + SCU_CTRL);
>> +}
> 
> Instead of having a separate function to do this (and the standby
> operation), what about doing that directly in scu_enable() ? Either
> unconditionally if that is fine for all SCU users, or through a flags
> argument?

OK using a flag argument makes sens indeed. About setting it unconditionally,
I would prefer not taking the risk to break the other platforms.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/5] ARM: Centralize the access to the SCU register

[Shawn Guo]

On Fri, Jun 27, 2014 at 01:01:03AM +0200, Gregory CLEMENT wrote:
> >> The last patch removed a direct access to the SCU register by an
> >> access through the new scu_standby_enable() function. For this one I
> >> have just checked that the kernel can be built using the
> >> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.
> > 
> > Why would we not just turn on these 2 features unconditionally? If we
> 
> You mean in scu_enbale() ?
> 
> > don't know of any platform where they are broken, then we should just
> 
> At least on some imx6 SCU standby is broken according to the code and
> the comments.

Hi Gregory,

What's broken on particular revisions of some i.MX6 SoC is WAIT mode
support, not SCU standby.  I think the SCU standby can just be
unconditionally enabled in scu_enbale().

Shawn

[PATCH 0/5] ARM: Centralize the access to the SCU register

[Shawn Guo]

On Fri, Jun 27, 2014 at 01:01:03AM +0200, Gregory CLEMENT wrote:
> >> The last patch removed a direct access to the SCU register by an
> >> access through the new scu_standby_enable() function. For this one I
> >> have just checked that the kernel can be built using the
> >> imx_v6_v7_defconfig config, but I didn't test it on an imx6 hardware.
> > 
> > Why would we not just turn on these 2 features unconditionally? If we
> 
> You mean in scu_enbale() ?
> 
> > don't know of any platform where they are broken, then we should just
> 
> At least on some imx6 SCU standby is broken according to the code and
> the comments.

Hi Gregory,

What's broken on particular revisions of some i.MX6 SoC is WAIT mode
support, not SCU standby.  I think the SCU standby can just be
unconditionally enabled in scu_enbale().

Shawn