diff mbox

ARM: mvebu: Enable SCU Speculative linefills to L2 for Armada 375/38x

Message ID 1403790178-23902-1-git-send-email-gregory.clement@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gregory CLEMENT June 26, 2014, 1:42 p.m. UTC 
From: Nadav Haklai <nadavh@marvell.com>

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.

This feature may improve overall system performance.

[Gregory: use a define for the SCU Speculative linefills enable bit,
modify the title of the patch]

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Thomas Petazzoni June 26, 2014, 2:52 p.m. UTC | #1 
Dear Gregory CLEMENT,

On Thu, 26 Jun 2014 15:42:58 +0200, Gregory CLEMENT wrote:

> +#define SCU_CTRL		0x00
> +#define SCU_SPEC_LINEFILL_EN	    BIT(3)
> +
> +
>  /*
>   * Enables the SCU when available. Obviously, this is only useful on
>   * Cortex-A based SOCs, not on PJ4B based ones.
> @@ -44,7 +48,15 @@ static void __init mvebu_scu_enable(void)
>  	struct device_node *np =
>  		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
>  	if (np) {
> +		u32 scu_ctrl;
>  		scu_base = of_iomap(np, 0);
> +		/* already enabled? */
> +		scu_ctrl = __raw_readl(scu_base + SCU_CTRL);

Unless the SCU uses the native endianess of the system, and therefore
switches to big endian when the system is running big endian, using
__raw_{readl,writel} is wrong here, and will break big endian
configurations.

> +		if (!(scu_ctrl & 1)) {

What is this bit 0 you're checking here? How does it relate to the bit
3 you're setting when bit 0 is not set?

A broader question is: if this feature is generic to all Cortex-A9, why
not offer a function in smp_scu.c to enable it? It seems weird to have
the offset and bit definitions for something as generic as the SCU
present deep into a Marvell-specific file.

Best regards,

Thomas
Gregory CLEMENT June 26, 2014, 3:09 p.m. UTC | #2 
Hi Thomas,

On 26/06/2014 16:52, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 26 Jun 2014 15:42:58 +0200, Gregory CLEMENT wrote:
> 
>> +#define SCU_CTRL		0x00
>> +#define SCU_SPEC_LINEFILL_EN	    BIT(3)
>> +
>> +
>>  /*
>>   * Enables the SCU when available. Obviously, this is only useful on
>>   * Cortex-A based SOCs, not on PJ4B based ones.
>> @@ -44,7 +48,15 @@ static void __init mvebu_scu_enable(void)
>>  	struct device_node *np =
>>  		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
>>  	if (np) {
>> +		u32 scu_ctrl;
>>  		scu_base = of_iomap(np, 0);
>> +		/* already enabled? */
>> +		scu_ctrl = __raw_readl(scu_base + SCU_CTRL);
> 
> Unless the SCU uses the native endianess of the system, and therefore
> switches to big endian when the system is running big endian, using
> __raw_{readl,writel} is wrong here, and will break big endian
> configurations.

Yes you are right here.

> 
>> +		if (!(scu_ctrl & 1)) {
> 
> What is this bit 0 you're checking here? How does it relate to the bit
> 3 you're setting when bit 0 is not set?

It is related to the comment just before: we checked if the SCU
is already enabled. So I guess we can't set the SCU Speculative linefills
bit if the SCU is enabled.

> 
> A broader question is: if this feature is generic to all Cortex-A9, why
> not offer a function in smp_scu.c to enable it? It seems weird to have
> the offset and bit definitions for something as generic as the SCU
> present deep into a Marvell-specific file.

I have no strong opinion on it, and I also thoough about adding a function
in smp_scu.c but when you are looking at the usage of SCU is the kernel
the other SOC manipulate it directly.


Thanks,

Gregory
Andrew Lunn June 26, 2014, 4:12 p.m. UTC | #3 
> > A broader question is: if this feature is generic to all Cortex-A9, why
> > not offer a function in smp_scu.c to enable it? It seems weird to have
> > the offset and bit definitions for something as generic as the SCU
> > present deep into a Marvell-specific file.
> 
> I have no strong opinion on it, and I also thoough about adding a function
> in smp_scu.c but when you are looking at the usage of SCU is the kernel
> the other SOC manipulate it directly.

Hi Gregory

Is that just cult cargo coding? Maybe now is the time to refactor this
into one central location?

     Andrew
diff mbox

Patch

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 8bb742fdf5ca..c9b02651e895 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -33,6 +33,10 @@ 
 #include "coherency.h"
 #include "mvebu-soc-id.h"
 
+#define SCU_CTRL		0x00
+#define SCU_SPEC_LINEFILL_EN	    BIT(3)
+
+
 /*
  * Enables the SCU when available. Obviously, this is only useful on
  * Cortex-A based SOCs, not on PJ4B based ones.
@@ -44,7 +48,15 @@  static void __init mvebu_scu_enable(void)
 	struct device_node *np =
 		of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
 	if (np) {
+		u32 scu_ctrl;
 		scu_base = of_iomap(np, 0);
+		/* already enabled? */
+		scu_ctrl = __raw_readl(scu_base + SCU_CTRL);
+		if (!(scu_ctrl & 1)) {
+			/* Enable SCU Speculative linefills to L2 */
+			scu_ctrl |= SCU_SPEC_LINEFILL_EN;
+			__raw_writel(scu_ctrl, scu_base + SCU_CTRL);
+		}
 		scu_enable(scu_base);
 		of_node_put(np);
 	}