[PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Guillaume Tucker]

The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
sequence.  There is no need to set it in the default register value,
this was done before support for it was implemented in the code.  It
is not set in the hardware initial value either.

Clean this up by removing this flag from the default l2c_aux_val, and
add it to the l2c_aux_mask to print an alert message if it was already
set before the kernel initialisation.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/mach-exynos/exynos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index 36c37444485a..a96f3353a0c1 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -193,8 +193,8 @@ static void __init exynos_dt_fixup(void)
 }
 
 DT_MACHINE_START(EXYNOS_DT, "Samsung Exynos (Flattened Device Tree)")
-	.l2c_aux_val	= 0x3c400000,
-	.l2c_aux_mask	= 0xc20fffff,
+	.l2c_aux_val	= 0x38400000,
+	.l2c_aux_mask	= 0xc60fffff,
 	.smp		= smp_ops(exynos_smp_ops),
 	.map_io		= exynos_init_io,
 	.init_early	= exynos_firmware_init,
-- 
2.20.1

[PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Guillaume Tucker]

The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
sequence.  There is no need to set it in the default register value,
this was done before support for it was implemented in the code.  It
is not set in the hardware initial value either.

Clean this up by removing this flag from the default l2c_aux_val, and
add it to the l2c_aux_mask to print an alert message if it was already
set before the kernel initialisation.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/mach-exynos/exynos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index 36c37444485a..a96f3353a0c1 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -193,8 +193,8 @@ static void __init exynos_dt_fixup(void)
 }
 
 DT_MACHINE_START(EXYNOS_DT, "Samsung Exynos (Flattened Device Tree)")
-	.l2c_aux_val	= 0x3c400000,
-	.l2c_aux_mask	= 0xc20fffff,
+	.l2c_aux_val	= 0x38400000,
+	.l2c_aux_mask	= 0xc60fffff,
 	.smp		= smp_ops(exynos_smp_ops),
 	.map_io		= exynos_init_io,
 	.init_early	= exynos_firmware_init,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
instruction prefetch respectively can also be accessed via the
L2X0_AUX_CTRL register.  They appear to be actually wired together in
hardware between the registers.  Changing them in the prefetch
register only will get undone when restoring the aux control register
later on.  For this reason, set these bits in both registers during
initialisation according to the DT attributes.

Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 12c26eb88afb..43d91bfd2360 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
 
 	ret = of_property_read_u32(np, "prefetch-data", &val);
 	if (ret == 0) {
-		if (val)
+		if (val) {
 			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
-		else
+			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
+		} else {
 			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
+			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
+		}
+		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
 	} else if (ret != -EINVAL) {
 		pr_err("L2C-310 OF prefetch-data property value is missing\n");
 	}
 
 	ret = of_property_read_u32(np, "prefetch-instr", &val);
 	if (ret == 0) {
-		if (val)
+		if (val) {
 			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
-		else
+			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
+		} else {
 			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
+			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
+		}
+		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
 	} else if (ret != -EINVAL) {
 		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
 	}
-- 
2.20.1

[PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
instruction prefetch respectively can also be accessed via the
L2X0_AUX_CTRL register.  They appear to be actually wired together in
hardware between the registers.  Changing them in the prefetch
register only will get undone when restoring the aux control register
later on.  For this reason, set these bits in both registers during
initialisation according to the DT attributes.

Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 12c26eb88afb..43d91bfd2360 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
 
 	ret = of_property_read_u32(np, "prefetch-data", &val);
 	if (ret == 0) {
-		if (val)
+		if (val) {
 			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
-		else
+			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
+		} else {
 			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
+			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
+		}
+		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
 	} else if (ret != -EINVAL) {
 		pr_err("L2C-310 OF prefetch-data property value is missing\n");
 	}
 
 	ret = of_property_read_u32(np, "prefetch-instr", &val);
 	if (ret == 0) {
-		if (val)
+		if (val) {
 			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
-		else
+			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
+		} else {
 			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
+			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
+		}
+		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
 	} else if (ret != -EINVAL) {
 		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
 	}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Guillaume Tucker]

Use the standard l2c2x0 device tree bindings to enable data and
instruction prefetch on exynos4210 and exynos4412 and clear the
respective bits in the default l2c_aux_val.  No other Exynos platform
relying on this default register value appears to be using the l2x0
cache.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 2 ++
 arch/arm/boot/dts/exynos4412.dtsi | 2 ++
 arch/arm/mach-exynos/exynos.c     | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index b4466232f0c1..7e0d253b26ef 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -102,6 +102,8 @@
 			reg = <0x10502000 0x1000>;
 			cache-unified;
 			cache-level = <2>;
+			prefetch-data = <1>;
+			prefetch-instr = <1>;
 			arm,tag-latency = <2 2 1>;
 			arm,data-latency = <2 2 1>;
 		};
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index 48868947373e..37efa247bf4d 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -218,6 +218,8 @@
 			reg = <0x10502000 0x1000>;
 			cache-unified;
 			cache-level = <2>;
+			prefetch-data = <1>;
+			prefetch-instr = <1>;
 			arm,tag-latency = <2 2 1>;
 			arm,data-latency = <3 2 1>;
 			arm,double-linefill = <1>;
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index a96f3353a0c1..0e906cc3a48e 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -193,8 +193,8 @@ static void __init exynos_dt_fixup(void)
 }
 
 DT_MACHINE_START(EXYNOS_DT, "Samsung Exynos (Flattened Device Tree)")
-	.l2c_aux_val	= 0x38400000,
-	.l2c_aux_mask	= 0xc60fffff,
+	.l2c_aux_val	= 0x08400000,
+	.l2c_aux_mask	= 0xf60fffff,
 	.smp		= smp_ops(exynos_smp_ops),
 	.map_io		= exynos_init_io,
 	.init_early	= exynos_firmware_init,
-- 
2.20.1

[PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Guillaume Tucker]

Use the standard l2c2x0 device tree bindings to enable data and
instruction prefetch on exynos4210 and exynos4412 and clear the
respective bits in the default l2c_aux_val.  No other Exynos platform
relying on this default register value appears to be using the l2x0
cache.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 2 ++
 arch/arm/boot/dts/exynos4412.dtsi | 2 ++
 arch/arm/mach-exynos/exynos.c     | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index b4466232f0c1..7e0d253b26ef 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -102,6 +102,8 @@
 			reg = <0x10502000 0x1000>;
 			cache-unified;
 			cache-level = <2>;
+			prefetch-data = <1>;
+			prefetch-instr = <1>;
 			arm,tag-latency = <2 2 1>;
 			arm,data-latency = <2 2 1>;
 		};
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index 48868947373e..37efa247bf4d 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -218,6 +218,8 @@
 			reg = <0x10502000 0x1000>;
 			cache-unified;
 			cache-level = <2>;
+			prefetch-data = <1>;
+			prefetch-instr = <1>;
 			arm,tag-latency = <2 2 1>;
 			arm,data-latency = <3 2 1>;
 			arm,double-linefill = <1>;
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index a96f3353a0c1..0e906cc3a48e 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -193,8 +193,8 @@ static void __init exynos_dt_fixup(void)
 }
 
 DT_MACHINE_START(EXYNOS_DT, "Samsung Exynos (Flattened Device Tree)")
-	.l2c_aux_val	= 0x38400000,
-	.l2c_aux_mask	= 0xc60fffff,
+	.l2c_aux_val	= 0x08400000,
+	.l2c_aux_mask	= 0xf60fffff,
 	.smp		= smp_ops(exynos_smp_ops),
 	.map_io		= exynos_init_io,
 	.init_early	= exynos_firmware_init,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Russell King - ARM Linux admin]

On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
> instruction prefetch respectively can also be accessed via the
> L2X0_AUX_CTRL register.  They appear to be actually wired together in
> hardware between the registers.  Changing them in the prefetch
> register only will get undone when restoring the aux control register
> later on.  For this reason, set these bits in both registers during
> initialisation according to the DT attributes.

How will that happen?

We write the auxiliary control register before the prefetch control
register, so the prefetch control register will take precedence.  See
l2c310_configure() - l2c_configure() writes the auxiliary control
register, and the function writes the prefetch control register later.

I think the real issue is that Exynos has been modifying the prefetch
settings via its machine .aux_mask / .aux_val configuration, and the
opposite is actually true: the prefetch control register values will
overwrite the attempt to modify the auxiliary control values set through
the machine .aux_mask/.aux_val.

> 
> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 12c26eb88afb..43d91bfd2360 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>  
>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>  	if (ret == 0) {
> -		if (val)
> +		if (val) {
>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> -		else
> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +		} else {
>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
> +		}
> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>  	} else if (ret != -EINVAL) {
>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>  	}
>  
>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>  	if (ret == 0) {
> -		if (val)
> +		if (val) {
>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> -		else
> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +		} else {
>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +		}
> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>  	} else if (ret != -EINVAL) {
>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>  	}
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Russell King - ARM Linux admin]

On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
> instruction prefetch respectively can also be accessed via the
> L2X0_AUX_CTRL register.  They appear to be actually wired together in
> hardware between the registers.  Changing them in the prefetch
> register only will get undone when restoring the aux control register
> later on.  For this reason, set these bits in both registers during
> initialisation according to the DT attributes.

How will that happen?

We write the auxiliary control register before the prefetch control
register, so the prefetch control register will take precedence.  See
l2c310_configure() - l2c_configure() writes the auxiliary control
register, and the function writes the prefetch control register later.

I think the real issue is that Exynos has been modifying the prefetch
settings via its machine .aux_mask / .aux_val configuration, and the
opposite is actually true: the prefetch control register values will
overwrite the attempt to modify the auxiliary control values set through
the machine .aux_mask/.aux_val.

> 
> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 12c26eb88afb..43d91bfd2360 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>  
>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>  	if (ret == 0) {
> -		if (val)
> +		if (val) {
>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> -		else
> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +		} else {
>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
> +		}
> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>  	} else if (ret != -EINVAL) {
>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>  	}
>  
>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>  	if (ret == 0) {
> -		if (val)
> +		if (val) {
>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> -		else
> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +		} else {
>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +		}
> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>  	} else if (ret != -EINVAL) {
>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>  	}
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

On 29/07/2020 15:18, Russell King - ARM Linux admin wrote:
> On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
>> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
>> instruction prefetch respectively can also be accessed via the
>> L2X0_AUX_CTRL register.  They appear to be actually wired together in
>> hardware between the registers.  Changing them in the prefetch
>> register only will get undone when restoring the aux control register
>> later on.  For this reason, set these bits in both registers during
>> initialisation according to the DT attributes.
> 
> How will that happen?
> 
> We write the auxiliary control register before the prefetch control
> register, so the prefetch control register will take precedence.  See
> l2c310_configure() - l2c_configure() writes the auxiliary control
> register, and the function writes the prefetch control register later.

What I'm seeing is that outer_cache.configure() gets called, at
least on exynos4412-odroidx2.  See l2c_enable():

	if (outer_cache.configure)
		outer_cache.configure(&l2x0_saved_regs);
	else
		l2x0_data->configure(base);

Then instead of l2c310_configure(), exynos_l2_configure() gets
called and writes prefetch_ctrl right before aux_ctrl.  Should
exynos_l2_configure() be changed to swap the register writes?


> I think the real issue is that Exynos has been modifying the prefetch
> settings via its machine .aux_mask / .aux_val configuration, and the
> opposite is actually true: the prefetch control register values will
> overwrite the attempt to modify the auxiliary control values set through
> the machine .aux_mask/.aux_val.

Yes with l2c310_configure() but not with exynos_l2_configure().

To be clear, this is what I've found to be happening, if you
switch to using the device tree prefetch attributes and clear
the bits in the default l2c_aux_val (see PATCH 3/3):

1. l2x0_of_init() first gets called with the default aux_val

2. l2c310_of_parse() sets the bits in l2x0_saved_regs.prefetch_ctrl
   but not in aux_val (unless you apply this patch 2/3)

3. l2c_enable() calls exynos_l2_configure() which writes
   prefetch_ctrl and then aux_ctrl - thus setting the prefetch bits
   and then clearing them just after

4. l2c310_enable() reads back aux_ctrl and prefetch, both of which
   now have the bits cleared (the pr_info() message about prefetch
   enabled gets skipped)


That's why I thought it would be safer to set the prefetch bits
in both registers so it should work regardless if the
initialisation sequence.  Also, if we want these bits to be
changed, we should clear them in the aux_mask value to not get
another error message about register corruption - so I'm doing
that too.

Thanks,
Guillaume


>> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>> index 12c26eb88afb..43d91bfd2360 100644
>> --- a/arch/arm/mm/cache-l2x0.c
>> +++ b/arch/arm/mm/cache-l2x0.c
>> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>>  
>>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>>  	if (ret == 0) {
>> -		if (val)
>> +		if (val) {
>>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>> -		else
>> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +		} else {
>>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +		}
>> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>  	} else if (ret != -EINVAL) {
>>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>>  	}
>>  
>>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>>  	if (ret == 0) {
>> -		if (val)
>> +		if (val) {
>>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> -		else
>> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +		} else {
>>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +		}
>> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>  	} else if (ret != -EINVAL) {
>>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>>  	}
>> -- 
>> 2.20.1
>>
>>
> 

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

On 29/07/2020 15:18, Russell King - ARM Linux admin wrote:
> On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
>> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
>> instruction prefetch respectively can also be accessed via the
>> L2X0_AUX_CTRL register.  They appear to be actually wired together in
>> hardware between the registers.  Changing them in the prefetch
>> register only will get undone when restoring the aux control register
>> later on.  For this reason, set these bits in both registers during
>> initialisation according to the DT attributes.
> 
> How will that happen?
> 
> We write the auxiliary control register before the prefetch control
> register, so the prefetch control register will take precedence.  See
> l2c310_configure() - l2c_configure() writes the auxiliary control
> register, and the function writes the prefetch control register later.

What I'm seeing is that outer_cache.configure() gets called, at
least on exynos4412-odroidx2.  See l2c_enable():

	if (outer_cache.configure)
		outer_cache.configure(&l2x0_saved_regs);
	else
		l2x0_data->configure(base);

Then instead of l2c310_configure(), exynos_l2_configure() gets
called and writes prefetch_ctrl right before aux_ctrl.  Should
exynos_l2_configure() be changed to swap the register writes?


> I think the real issue is that Exynos has been modifying the prefetch
> settings via its machine .aux_mask / .aux_val configuration, and the
> opposite is actually true: the prefetch control register values will
> overwrite the attempt to modify the auxiliary control values set through
> the machine .aux_mask/.aux_val.

Yes with l2c310_configure() but not with exynos_l2_configure().

To be clear, this is what I've found to be happening, if you
switch to using the device tree prefetch attributes and clear
the bits in the default l2c_aux_val (see PATCH 3/3):

1. l2x0_of_init() first gets called with the default aux_val

2. l2c310_of_parse() sets the bits in l2x0_saved_regs.prefetch_ctrl
   but not in aux_val (unless you apply this patch 2/3)

3. l2c_enable() calls exynos_l2_configure() which writes
   prefetch_ctrl and then aux_ctrl - thus setting the prefetch bits
   and then clearing them just after

4. l2c310_enable() reads back aux_ctrl and prefetch, both of which
   now have the bits cleared (the pr_info() message about prefetch
   enabled gets skipped)


That's why I thought it would be safer to set the prefetch bits
in both registers so it should work regardless if the
initialisation sequence.  Also, if we want these bits to be
changed, we should clear them in the aux_mask value to not get
another error message about register corruption - so I'm doing
that too.

Thanks,
Guillaume


>> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>> index 12c26eb88afb..43d91bfd2360 100644
>> --- a/arch/arm/mm/cache-l2x0.c
>> +++ b/arch/arm/mm/cache-l2x0.c
>> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>>  
>>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>>  	if (ret == 0) {
>> -		if (val)
>> +		if (val) {
>>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>> -		else
>> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +		} else {
>>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>> +		}
>> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>  	} else if (ret != -EINVAL) {
>>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>>  	}
>>  
>>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>>  	if (ret == 0) {
>> -		if (val)
>> +		if (val) {
>>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> -		else
>> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +		} else {
>>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>> +		}
>> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>  	} else if (ret != -EINVAL) {
>>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>>  	}
>> -- 
>> 2.20.1
>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Krzysztof Kozlowski]

On Wed, Jul 29, 2020 at 02:47:33PM +0100, Guillaume Tucker wrote:
> Use the standard l2c2x0 device tree bindings to enable data and
> instruction prefetch on exynos4210 and exynos4412 and clear the
> respective bits in the default l2c_aux_val.  No other Exynos platform
> relying on this default register value appears to be using the l2x0
> cache.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>  arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>  arch/arm/mach-exynos/exynos.c     | 4 ++--

I will need these split between DTS and mach changes.

Best regards,
Krzysztof

Re: [PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Krzysztof Kozlowski]

On Wed, Jul 29, 2020 at 02:47:33PM +0100, Guillaume Tucker wrote:
> Use the standard l2c2x0 device tree bindings to enable data and
> instruction prefetch on exynos4210 and exynos4412 and clear the
> respective bits in the default l2c_aux_val.  No other Exynos platform
> relying on this default register value appears to be using the l2x0
> cache.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>  arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>  arch/arm/mach-exynos/exynos.c     | 4 ++--

I will need these split between DTS and mach changes.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Krzysztof Kozlowski]

On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
> The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
> sequence.  There is no need to set it in the default register value,
> this was done before support for it was implemented in the code.  It
> is not set in the hardware initial value either.
> 
> Clean this up by removing this flag from the default l2c_aux_val, and
> add it to the l2c_aux_mask to print an alert message if it was already
> set before the kernel initialisation.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/mach-exynos/exynos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Makes sense. I'll take it after the merge window.

Best regards,
Krzysztof

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Krzysztof Kozlowski]

On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
> The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
> sequence.  There is no need to set it in the default register value,
> this was done before support for it was implemented in the code.  It
> is not set in the hardware initial value either.
> 
> Clean this up by removing this flag from the default l2c_aux_val, and
> add it to the l2c_aux_mask to print an alert message if it was already
> set before the kernel initialisation.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/mach-exynos/exynos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Makes sense. I'll take it after the merge window.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 03:34:39PM +0200, Krzysztof Kozlowski wrote:
> On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
> > The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
> > sequence.  There is no need to set it in the default register value,
> > this was done before support for it was implemented in the code.  It
> > is not set in the hardware initial value either.
> > 
> > Clean this up by removing this flag from the default l2c_aux_val, and
> > add it to the l2c_aux_mask to print an alert message if it was already
> > set before the kernel initialisation.
> > 
> > Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> > ---
> >  arch/arm/mach-exynos/exynos.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Makes sense. I'll take it after the merge window.

Yes, because platforms actually have no control over this bit through
these values.

Please fix the description to use the right define, it's
L310_AUX_CTRL_NS_LOCKDOWN not L220_AUX_CTRL_NS_LOCKDOWN.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Russell King - ARM Linux admin]

On Mon, Aug 03, 2020 at 03:34:39PM +0200, Krzysztof Kozlowski wrote:
> On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
> > The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
> > sequence.  There is no need to set it in the default register value,
> > this was done before support for it was implemented in the code.  It
> > is not set in the hardware initial value either.
> > 
> > Clean this up by removing this flag from the default l2c_aux_val, and
> > add it to the l2c_aux_mask to print an alert message if it was already
> > set before the kernel initialisation.
> > 
> > Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> > ---
> >  arch/arm/mach-exynos/exynos.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Makes sense. I'll take it after the merge window.

Yes, because platforms actually have no control over this bit through
these values.

Please fix the description to use the right define, it's
L310_AUX_CTRL_NS_LOCKDOWN not L220_AUX_CTRL_NS_LOCKDOWN.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Guillaume Tucker]

On 03/08/2020 15:22, Russell King - ARM Linux admin wrote:
> On Mon, Aug 03, 2020 at 03:34:39PM +0200, Krzysztof Kozlowski wrote:
>> On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
>>> The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
>>> sequence.  There is no need to set it in the default register value,
>>> this was done before support for it was implemented in the code.  It
>>> is not set in the hardware initial value either.
>>>
>>> Clean this up by removing this flag from the default l2c_aux_val, and
>>> add it to the l2c_aux_mask to print an alert message if it was already
>>> set before the kernel initialisation.
>>>
>>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>>> ---
>>>  arch/arm/mach-exynos/exynos.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Makes sense. I'll take it after the merge window.
> 
> Yes, because platforms actually have no control over this bit through
> these values.
> 
> Please fix the description to use the right define, it's
> L310_AUX_CTRL_NS_LOCKDOWN not L220_AUX_CTRL_NS_LOCKDOWN.

Thanks, fixed in v2.

Guilaume

Re: [PATCH 1/3] ARM: exynos: clear L220_AUX_CTRL_NS_LOCKDOWN in default l2c_aux_val

[Guillaume Tucker]

On 03/08/2020 15:22, Russell King - ARM Linux admin wrote:
> On Mon, Aug 03, 2020 at 03:34:39PM +0200, Krzysztof Kozlowski wrote:
>> On Wed, Jul 29, 2020 at 02:47:31PM +0100, Guillaume Tucker wrote:
>>> The L220_AUX_CTRL_NS_LOCKDOWN flag is set during the L2C enable
>>> sequence.  There is no need to set it in the default register value,
>>> this was done before support for it was implemented in the code.  It
>>> is not set in the hardware initial value either.
>>>
>>> Clean this up by removing this flag from the default l2c_aux_val, and
>>> add it to the l2c_aux_mask to print an alert message if it was already
>>> set before the kernel initialisation.
>>>
>>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>>> ---
>>>  arch/arm/mach-exynos/exynos.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Makes sense. I'll take it after the merge window.
> 
> Yes, because platforms actually have no control over this bit through
> these values.
> 
> Please fix the description to use the right define, it's
> L310_AUX_CTRL_NS_LOCKDOWN not L220_AUX_CTRL_NS_LOCKDOWN.

Thanks, fixed in v2.

Guilaume


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Guillaume Tucker]

On 03/08/2020 14:13, Krzysztof Kozlowski wrote:
> On Wed, Jul 29, 2020 at 02:47:33PM +0100, Guillaume Tucker wrote:
>> Use the standard l2c2x0 device tree bindings to enable data and
>> instruction prefetch on exynos4210 and exynos4412 and clear the
>> respective bits in the default l2c_aux_val.  No other Exynos platform
>> relying on this default register value appears to be using the l2x0
>> cache.
>>
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>>  arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>>  arch/arm/mach-exynos/exynos.c     | 4 ++--
> 
> I will need these split between DTS and mach changes.

Of course, sorry.  Fixed in v2.

Thanks,
Guillaume

Re: [PATCH 3/3] ARM: exynos: use DT prefetch attributes rather than l2c_aux_val

[Guillaume Tucker]

On 03/08/2020 14:13, Krzysztof Kozlowski wrote:
> On Wed, Jul 29, 2020 at 02:47:33PM +0100, Guillaume Tucker wrote:
>> Use the standard l2c2x0 device tree bindings to enable data and
>> instruction prefetch on exynos4210 and exynos4412 and clear the
>> respective bits in the default l2c_aux_val.  No other Exynos platform
>> relying on this default register value appears to be using the l2x0
>> cache.
>>
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>>  arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>>  arch/arm/mach-exynos/exynos.c     | 4 ++--
> 
> I will need these split between DTS and mach changes.

Of course, sorry.  Fixed in v2.

Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

On 29/07/2020 17:22, Guillaume Tucker wrote:
> On 29/07/2020 15:18, Russell King - ARM Linux admin wrote:
>> On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
>>> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
>>> instruction prefetch respectively can also be accessed via the
>>> L2X0_AUX_CTRL register.  They appear to be actually wired together in
>>> hardware between the registers.  Changing them in the prefetch
>>> register only will get undone when restoring the aux control register
>>> later on.  For this reason, set these bits in both registers during
>>> initialisation according to the DT attributes.
>>
>> How will that happen?
>>
>> We write the auxiliary control register before the prefetch control
>> register, so the prefetch control register will take precedence.  See
>> l2c310_configure() - l2c_configure() writes the auxiliary control
>> register, and the function writes the prefetch control register later.
> 
> What I'm seeing is that outer_cache.configure() gets called, at
> least on exynos4412-odroidx2.  See l2c_enable():
> 
> 	if (outer_cache.configure)
> 		outer_cache.configure(&l2x0_saved_regs);
> 	else
> 		l2x0_data->configure(base);
> 
> Then instead of l2c310_configure(), exynos_l2_configure() gets
> called and writes prefetch_ctrl right before aux_ctrl.  Should
> exynos_l2_configure() be changed to swap the register writes?
> 
> 
>> I think the real issue is that Exynos has been modifying the prefetch
>> settings via its machine .aux_mask / .aux_val configuration, and the
>> opposite is actually true: the prefetch control register values will
>> overwrite the attempt to modify the auxiliary control values set through
>> the machine .aux_mask/.aux_val.
> 
> Yes with l2c310_configure() but not with exynos_l2_configure().
> 
> To be clear, this is what I've found to be happening, if you
> switch to using the device tree prefetch attributes and clear
> the bits in the default l2c_aux_val (see PATCH 3/3):
> 
> 1. l2x0_of_init() first gets called with the default aux_val
> 
> 2. l2c310_of_parse() sets the bits in l2x0_saved_regs.prefetch_ctrl
>    but not in aux_val (unless you apply this patch 2/3)
> 
> 3. l2c_enable() calls exynos_l2_configure() which writes
>    prefetch_ctrl and then aux_ctrl - thus setting the prefetch bits
>    and then clearing them just after
> 
> 4. l2c310_enable() reads back aux_ctrl and prefetch, both of which
>    now have the bits cleared (the pr_info() message about prefetch
>    enabled gets skipped)
> 
> 
> That's why I thought it would be safer to set the prefetch bits
> in both registers so it should work regardless if the
> initialisation sequence.  Also, if we want these bits to be
> changed, we should clear them in the aux_mask value to not get
> another error message about register corruption - so I'm doing
> that too.

I've kept this patch as-is in the v2 because I wasn't sure
whether you wanted the issue to be addressed differently in the
end.  I just made it a bit clearer in the commit message that
it's fixing an issue when using the DT prefetch properties.
Please let me know if you want me to rework this in any way.

Thanks,
Guillaume

>>> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
>>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>>> ---
>>>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>>> index 12c26eb88afb..43d91bfd2360 100644
>>> --- a/arch/arm/mm/cache-l2x0.c
>>> +++ b/arch/arm/mm/cache-l2x0.c
>>> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>>>  
>>>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>>>  	if (ret == 0) {
>>> -		if (val)
>>> +		if (val) {
>>>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> -		else
>>> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +		} else {
>>>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +		}
>>> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>>  	} else if (ret != -EINVAL) {
>>>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>>>  	}
>>>  
>>>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>>>  	if (ret == 0) {
>>> -		if (val)
>>> +		if (val) {
>>>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> -		else
>>> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +		} else {
>>>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +		}
>>> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>>  	} else if (ret != -EINVAL) {
>>>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>>>  	}
>>> -- 
>>> 2.20.1
>>>
>>>
>>
> 

Re: [PATCH 2/3] ARM: l2c: update prefetch bits in L2X0_AUX_CTRL using DT value

[Guillaume Tucker]

On 29/07/2020 17:22, Guillaume Tucker wrote:
> On 29/07/2020 15:18, Russell King - ARM Linux admin wrote:
>> On Wed, Jul 29, 2020 at 02:47:32PM +0100, Guillaume Tucker wrote:
>>> The L310_PREFETCH_CTRL register bits 28 and 29 to enable data and
>>> instruction prefetch respectively can also be accessed via the
>>> L2X0_AUX_CTRL register.  They appear to be actually wired together in
>>> hardware between the registers.  Changing them in the prefetch
>>> register only will get undone when restoring the aux control register
>>> later on.  For this reason, set these bits in both registers during
>>> initialisation according to the DT attributes.
>>
>> How will that happen?
>>
>> We write the auxiliary control register before the prefetch control
>> register, so the prefetch control register will take precedence.  See
>> l2c310_configure() - l2c_configure() writes the auxiliary control
>> register, and the function writes the prefetch control register later.
> 
> What I'm seeing is that outer_cache.configure() gets called, at
> least on exynos4412-odroidx2.  See l2c_enable():
> 
> 	if (outer_cache.configure)
> 		outer_cache.configure(&l2x0_saved_regs);
> 	else
> 		l2x0_data->configure(base);
> 
> Then instead of l2c310_configure(), exynos_l2_configure() gets
> called and writes prefetch_ctrl right before aux_ctrl.  Should
> exynos_l2_configure() be changed to swap the register writes?
> 
> 
>> I think the real issue is that Exynos has been modifying the prefetch
>> settings via its machine .aux_mask / .aux_val configuration, and the
>> opposite is actually true: the prefetch control register values will
>> overwrite the attempt to modify the auxiliary control values set through
>> the machine .aux_mask/.aux_val.
> 
> Yes with l2c310_configure() but not with exynos_l2_configure().
> 
> To be clear, this is what I've found to be happening, if you
> switch to using the device tree prefetch attributes and clear
> the bits in the default l2c_aux_val (see PATCH 3/3):
> 
> 1. l2x0_of_init() first gets called with the default aux_val
> 
> 2. l2c310_of_parse() sets the bits in l2x0_saved_regs.prefetch_ctrl
>    but not in aux_val (unless you apply this patch 2/3)
> 
> 3. l2c_enable() calls exynos_l2_configure() which writes
>    prefetch_ctrl and then aux_ctrl - thus setting the prefetch bits
>    and then clearing them just after
> 
> 4. l2c310_enable() reads back aux_ctrl and prefetch, both of which
>    now have the bits cleared (the pr_info() message about prefetch
>    enabled gets skipped)
> 
> 
> That's why I thought it would be safer to set the prefetch bits
> in both registers so it should work regardless if the
> initialisation sequence.  Also, if we want these bits to be
> changed, we should clear them in the aux_mask value to not get
> another error message about register corruption - so I'm doing
> that too.

I've kept this patch as-is in the v2 because I wasn't sure
whether you wanted the issue to be addressed differently in the
end.  I just made it a bit clearer in the commit message that
it's fixing an issue when using the DT prefetch properties.
Please let me know if you want me to rework this in any way.

Thanks,
Guillaume

>>> Fixes: ec3bd0e68a67 ("ARM: 8391/1: l2c: add options to overwrite prefetching behavior")
>>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>>> ---
>>>  arch/arm/mm/cache-l2x0.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>>> index 12c26eb88afb..43d91bfd2360 100644
>>> --- a/arch/arm/mm/cache-l2x0.c
>>> +++ b/arch/arm/mm/cache-l2x0.c
>>> @@ -1249,20 +1249,28 @@ static void __init l2c310_of_parse(const struct device_node *np,
>>>  
>>>  	ret = of_property_read_u32(np, "prefetch-data", &val);
>>>  	if (ret == 0) {
>>> -		if (val)
>>> +		if (val) {
>>>  			prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> -		else
>>> +			*aux_val |= L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +		} else {
>>>  			prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +			*aux_val &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>> +		}
>>> +		*aux_mask &= ~L310_PREFETCH_CTRL_DATA_PREFETCH;
>>>  	} else if (ret != -EINVAL) {
>>>  		pr_err("L2C-310 OF prefetch-data property value is missing\n");
>>>  	}
>>>  
>>>  	ret = of_property_read_u32(np, "prefetch-instr", &val);
>>>  	if (ret == 0) {
>>> -		if (val)
>>> +		if (val) {
>>>  			prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> -		else
>>> +			*aux_val |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +		} else {
>>>  			prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +			*aux_val &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>> +		}
>>> +		*aux_mask &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH;
>>>  	} else if (ret != -EINVAL) {
>>>  		pr_err("L2C-310 OF prefetch-instr property value is missing\n");
>>>  	}
>>> -- 
>>> 2.20.1
>>>
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel