[PATCH v2 0/3] ARM: OMAP2+: AM437x: L2 cache support

[PATCH v2 0/3] ARM: OMAP2+: AM437x: L2 cache support

[Sekhar Nori]

This patch series adds L2 cache support for AM437x
and does some clean-ups for existing OMAP4 support
along the way.

Tested on OMAP4 Panda and AM437x EPOS EVMs.

It is based on RMK's 75 patch series titled "l2c series".

Sekhar Nori (3):
  ARM: OMAP2+: L2 cache: allow different aux ctrl settings
  ARM: OMAP2+: L2 cache: get rid of init call
  ARM: OMAP2+: AM43x: L2 cache support

 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/common.h       |    2 ++
 arch/arm/mach-omap2/io.c           |    2 ++
 arch/arm/mach-omap2/omap4-common.c |   45 +++++++++++++++++++-----------------
 4 files changed, 29 insertions(+), 21 deletions(-)

-- 
1.7.10.1

[PATCH v2 1/3] ARM: OMAP2+: L2 cache: allow different aux ctrl settings

[Sekhar Nori]

Different SoCs likely need different L2 cache aux ctrl
settings based on use cases and optimizations required.

For example, if CMA is always used for coherent memory allocations,
there is no need for sharable attribute override bit to be set.

Pass aux control settings as argument to the L2 cache initialization
function to allow for this.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 arch/arm/mach-omap2/omap4-common.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index 06c6a18..0a2e4f0 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -196,10 +196,8 @@ static void omap4_l2c310_write_sec(unsigned long val, unsigned reg)
 	omap_smc1(smc_op, val);
 }
 
-static int __init omap_l2_cache_init(void)
+static int __init omap_l2_cache_init(u32 aux_ctrl, u32 aux_flags)
 {
-	u32 aux_ctrl;
-
 	/*
 	 * To avoid code running on other OMAPs in
 	 * multi-omap builds
@@ -212,23 +210,28 @@ static int __init omap_l2_cache_init(void)
 	if (WARN_ON(!l2cache_base))
 		return -ENOMEM;
 
-	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
-	aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
-		   L310_AUX_CTRL_NS_LOCKDOWN |
-		   L310_AUX_CTRL_NS_INT_CTRL |
-		   L2C_AUX_CTRL_SHARED_OVERRIDE |
-		   L310_AUX_CTRL_DATA_PREFETCH |
-		   L310_AUX_CTRL_INSTR_PREFETCH;
-
 	outer_cache.write_sec = omap4_l2c310_write_sec;
 	if (of_have_populated_dt())
-		l2x0_of_init(aux_ctrl, 0xc19fffff);
+		l2x0_of_init(aux_ctrl, aux_flags);
 	else
-		l2x0_init(l2cache_base, aux_ctrl, 0xc19fffff);
+		l2x0_init(l2cache_base, aux_ctrl, aux_flags);
 
 	return 0;
 }
-omap_early_initcall(omap_l2_cache_init);
+
+static int __init omap4_l2_cache_init(void)
+{
+	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
+	u32 aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
+		       L310_AUX_CTRL_NS_LOCKDOWN |
+		       L310_AUX_CTRL_NS_INT_CTRL |
+		       L2C_AUX_CTRL_SHARED_OVERRIDE |
+		       L310_AUX_CTRL_DATA_PREFETCH |
+		       L310_AUX_CTRL_INSTR_PREFETCH;
+
+	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
+}
+omap_early_initcall(omap4_l2_cache_init);
 #endif
 
 void __iomem *omap4_get_sar_ram_base(void)
-- 
1.7.10.1

[PATCH v2 2/3] ARM: OMAP2+: L2 cache: get rid of init call

[Sekhar Nori]

Get rid of init call to initialize L2 cache.
Instead use the init_early machine hook. This
helps in using the initialization routine across
SoCs without the need of ugly cpu_is_*() checks.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 arch/arm/mach-omap2/common.h       |    1 +
 arch/arm/mach-omap2/io.c           |    1 +
 arch/arm/mach-omap2/omap4-common.c |   10 +---------
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index d88aff7..c64d5f5 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -91,6 +91,7 @@ extern void omap3_sync32k_timer_init(void);
 extern void omap3_secure_sync32k_timer_init(void);
 extern void omap3_gptimer_timer_init(void);
 extern void omap4_local_timer_init(void);
+int omap4_l2_cache_init(void);
 extern void omap5_realtime_timer_init(void);
 
 void omap2420_init_early(void);
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index f14f9ac..81bc89c 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -640,6 +640,7 @@ void __init omap4430_init_early(void)
 	omap44xx_clockdomains_init();
 	omap44xx_hwmod_init();
 	omap_hwmod_init_postsetup();
+	omap4_l2_cache_init();
 	omap_clk_soc_init = omap4xxx_dt_clk_init;
 }
 
diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index 0a2e4f0..f8b8dac 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -198,13 +198,6 @@ static void omap4_l2c310_write_sec(unsigned long val, unsigned reg)
 
 static int __init omap_l2_cache_init(u32 aux_ctrl, u32 aux_flags)
 {
-	/*
-	 * To avoid code running on other OMAPs in
-	 * multi-omap builds
-	 */
-	if (!cpu_is_omap44xx())
-		return -ENODEV;
-
 	/* Static mapping, never released */
 	l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K);
 	if (WARN_ON(!l2cache_base))
@@ -219,7 +212,7 @@ static int __init omap_l2_cache_init(u32 aux_ctrl, u32 aux_flags)
 	return 0;
 }
 
-static int __init omap4_l2_cache_init(void)
+int __init omap4_l2_cache_init(void)
 {
 	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
 	u32 aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
@@ -231,7 +224,6 @@ static int __init omap4_l2_cache_init(void)
 
 	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
 }
-omap_early_initcall(omap4_l2_cache_init);
 #endif
 
 void __iomem *omap4_get_sar_ram_base(void)
-- 
1.7.10.1

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

Add support for L2 cache controller (PL310) on
AM437x SoC.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/common.h       |    1 +
 arch/arm/mach-omap2/io.c           |    1 +
 arch/arm/mach-omap2/omap4-common.c |    8 ++++++++
 4 files changed, 11 insertions(+)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 1124155..1fd34d2 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -65,6 +65,7 @@ config SOC_AM43XX
 	select ARCH_HAS_OPP
 	select ARM_GIC
 	select MACH_OMAP_GENERIC
+	select MIGHT_HAVE_CACHE_L2X0
 
 config SOC_DRA7XX
 	bool "TI DRA7XX"
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index c64d5f5..fc59b49 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -93,6 +93,7 @@ extern void omap3_gptimer_timer_init(void);
 extern void omap4_local_timer_init(void);
 int omap4_l2_cache_init(void);
 extern void omap5_realtime_timer_init(void);
+int am43xx_l2_cache_init(void);
 
 void omap2420_init_early(void);
 void omap2430_init_early(void);
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 81bc89c..131c207 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -609,6 +609,7 @@ void __init am43xx_init_early(void)
 	am43xx_clockdomains_init();
 	am43xx_hwmod_init();
 	omap_hwmod_init_postsetup();
+	am43xx_l2_cache_init();
 	omap_clk_soc_init = am43xx_dt_clk_init;
 }
 
diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index f8b8dac..6b2a056 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
 
 	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
 }
+
+int __init am43xx_l2_cache_init(void)
+{
+	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
+		       L310_AUX_CTRL_INSTR_PREFETCH;
+
+	return omap_l2_cache_init(aux_ctrl, 0xcfffffff);
+}
 #endif
 
 void __iomem *omap4_get_sar_ram_base(void)
-- 
1.7.10.1

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> index f8b8dac..6b2a056 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>  
>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>  }
> +
> +int __init am43xx_l2_cache_init(void)
> +{
> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
> +		       L310_AUX_CTRL_INSTR_PREFETCH;

It would be good to documenting the difference between this and OMAP4,
and why you have chosen different values.

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>> index f8b8dac..6b2a056 100644
>> --- a/arch/arm/mach-omap2/omap4-common.c
>> +++ b/arch/arm/mach-omap2/omap4-common.c
>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>  
>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>  }
>> +
>> +int __init am43xx_l2_cache_init(void)
>> +{
>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
> 
> It would be good to documenting the difference between this and OMAP4,
> and why you have chosen different values.

There are two main differences:

1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
not needed even in OMAP4 with latest kernel, but I am not sure if I can
do this safely without breaking any usecase currently working with OMAP4.

2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
searched through the commit history of L2 cache support on OMAP4 but
there is no mention of why this was needed on OMAP4. I am checking
internally on the history behind this.

3) OMAP4 sets cache replacement policy to RR which is not a big deal
since thats the default anyway. We can probably drop this setting even
from OMAP4.

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Santosh Shilimkar]

On Tuesday 08 April 2014 10:53 AM, Sekhar Nori wrote:
> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
>> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>>> index f8b8dac..6b2a056 100644
>>> --- a/arch/arm/mach-omap2/omap4-common.c
>>> +++ b/arch/arm/mach-omap2/omap4-common.c
>>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>>  
>>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>>  }
>>> +
>>> +int __init am43xx_l2_cache_init(void)
>>> +{
>>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
>>
>> It would be good to documenting the difference between this and OMAP4,
>> and why you have chosen different values.
> 
> There are two main differences:
> 
> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
> not needed even in OMAP4 with latest kernel, but I am not sure if I can
> do this safely without breaking any usecase currently working with OMAP4.
> 
Wrong. Shared bit is mandatory for the OMAP4. Its a SMP system
which needs that.

> 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
> searched through the commit history of L2 cache support on OMAP4 but
> there is no mention of why this was needed on OMAP4. I am checking
> internally on the history behind this.
>
These have also come from the aligned settings with hardware folks.
 
> 3) OMAP4 sets cache replacement policy to RR which is not a big deal
> since thats the default anyway. We can probably drop this setting even
> from OMAP4.
> 
Don't change anything on OMAP4 since these settings have come up from
multiple alignments.

In my view, Aegis can use exact same setting as OMAP4. Things like
shared bit etc would not make much difference because of UP config,
keeping that doesn't hurt either.

Why don't you just re-use that as is ? Sorry if I have missed any
other discussion on the thread.

Regards,
Santosh

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Tuesday 08 April 2014 08:47 PM, Santosh Shilimkar wrote:
> On Tuesday 08 April 2014 10:53 AM, Sekhar Nori wrote:
>> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
>>> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>>>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>>>> index f8b8dac..6b2a056 100644
>>>> --- a/arch/arm/mach-omap2/omap4-common.c
>>>> +++ b/arch/arm/mach-omap2/omap4-common.c
>>>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>>>  
>>>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>>>  }
>>>> +
>>>> +int __init am43xx_l2_cache_init(void)
>>>> +{
>>>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>>>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
>>>
>>> It would be good to documenting the difference between this and OMAP4,
>>> and why you have chosen different values.
>>
>> There are two main differences:
>>
>> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
>> not needed even in OMAP4 with latest kernel, but I am not sure if I can
>> do this safely without breaking any usecase currently working with OMAP4.
>>
> Wrong. Shared bit is mandatory for the OMAP4. Its a SMP system
> which needs that.

Can you please explain a little bit more since I am obviously lacking
the background on OMAP4?

Commit b0f20ff9 ("omap4: l2x0: Set share override bit") talks about
possibility of data corruption due to speculative prefetch and coherent
DMA buffers having a cachable alias. But based on recent mailing list
discussions, with introduction of CMA, we should not have such a
cachable alias since the mapping is modified in place. If
arm_memblock_steal() or memblock_remove() is used, thats not a problem
as well since that memory is not mapped in kernel page tables.

As I indicated earlier, I am too not in favor of changing anything on
OMAP4 but it will be instructive to know exactly which scenarios shared
bit becomes mandatory on OMAP4.

>> 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
>> searched through the commit history of L2 cache support on OMAP4 but
>> there is no mention of why this was needed on OMAP4. I am checking
>> internally on the history behind this.
>>
> These have also come from the aligned settings with hardware folks.

Okay. AFAIK, There has not been such a recommendation from hardware team
of AM437x AFAIK. But, the AM437x ROM does leave these two bits set after
booting so even though Linux does not touch these, these are already set.

Given this, I see no reason for not setting the same bits again from
Linux just to get close to OMAP4 code.

>  
>> 3) OMAP4 sets cache replacement policy to RR which is not a big deal
>> since thats the default anyway. We can probably drop this setting even
>> from OMAP4.
>>
> Don't change anything on OMAP4 since these settings have come up from
> multiple alignments.

I agree. Thats what the $subject series is doing too.

> In my view, Aegis can use exact same setting as OMAP4. Things like
> shared bit etc would not make much difference because of UP config,
> keeping that doesn't hurt either.
> 
> Why don't you just re-use that as is ? Sorry if I have missed any
> other discussion on the thread.

We could reuse as is. I don't see any functional issue. This is what I
will probably do for the next version of the series. The only setting
thats actually being done differently is the Shared attribute override
enable bit.

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Tue, Apr 08, 2014 at 11:17:17AM -0400, Santosh Shilimkar wrote:
> On Tuesday 08 April 2014 10:53 AM, Sekhar Nori wrote:
> > On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
> >> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
> >>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> >>> index f8b8dac..6b2a056 100644
> >>> --- a/arch/arm/mach-omap2/omap4-common.c
> >>> +++ b/arch/arm/mach-omap2/omap4-common.c
> >>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
> >>>  
> >>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
> >>>  }
> >>> +
> >>> +int __init am43xx_l2_cache_init(void)
> >>> +{
> >>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
> >>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
> >>
> >> It would be good to documenting the difference between this and OMAP4,
> >> and why you have chosen different values.
> > 
> > There are two main differences:
> > 
> > 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
> > not needed even in OMAP4 with latest kernel, but I am not sure if I can
> > do this safely without breaking any usecase currently working with OMAP4.
> > 
> Wrong. Shared bit is mandatory for the OMAP4. Its a SMP system
> which needs that.

Errr.  This bit affects the L2 cache behaviour for Normal memory, outer
non-cacheable accesses - in other words, those performed to memory mapped
via dma_alloc_coherent() or dma_alloc_writecombine().  It does not affect
other types of mappings (other access types ignore the sharable attribute).

When this bit is clear, accesses to such memory are:

- read: cacheable, no allocate
- write: write through, no write allocate

what this means is that if there are no cache lines in the L2 cache
corresponding with the physical address, then none will be allocated.
However, if there are cache lines present, then they will be hit,
read or updated as appropriate.

This may matter before CMA where we had the memory returned by
dma_alloc_coherent() and friends mapped as normal, cacheable mappings
which could be speculatively prefetched, and therefore cache lines
dragged into the L2 cache for these physical addresses.

However, now that we're using CMA, this does not apply as we no longer
have this aliasing mapping.

So, with CMA enabled, it should be safe not to set this bit.

However, the shared bit in the page tables must be set for SMP systems.
Are you sure you're not confusing the shared bit in the page tables
with the shared override bit in the L2 cache controller?

> > 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
> > searched through the commit history of L2 cache support on OMAP4 but
> > there is no mention of why this was needed on OMAP4. I am checking
> > internally on the history behind this.
> >
> These have also come from the aligned settings with hardware folks.

Again, this doesn't have much to do with hardware, it's secure/non-secure
access rights configuration to the L2 cache controller.

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Santosh Shilimkar]

On Wednesday 09 April 2014 12:33 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 08, 2014 at 11:17:17AM -0400, Santosh Shilimkar wrote:
>> On Tuesday 08 April 2014 10:53 AM, Sekhar Nori wrote:
>>> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
>>>> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>>>>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>>>>> index f8b8dac..6b2a056 100644
>>>>> --- a/arch/arm/mach-omap2/omap4-common.c
>>>>> +++ b/arch/arm/mach-omap2/omap4-common.c
>>>>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>>>>  
>>>>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>>>>  }
>>>>> +
>>>>> +int __init am43xx_l2_cache_init(void)
>>>>> +{
>>>>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>>>>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
>>>>
>>>> It would be good to documenting the difference between this and OMAP4,
>>>> and why you have chosen different values.
>>>
>>> There are two main differences:
>>>
>>> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
>>> not needed even in OMAP4 with latest kernel, but I am not sure if I can
>>> do this safely without breaking any usecase currently working with OMAP4.
>>>
>> Wrong. Shared bit is mandatory for the OMAP4. Its a SMP system
>> which needs that.
> 
> Errr.  This bit affects the L2 cache behaviour for Normal memory, outer
> non-cacheable accesses - in other words, those performed to memory mapped
> via dma_alloc_coherent() or dma_alloc_writecombine().  It does not affect
> other types of mappings (other access types ignore the sharable attribute).
> 
> When this bit is clear, accesses to such memory are:
> 
> - read: cacheable, no allocate
> - write: write through, no write allocate
> 
> what this means is that if there are no cache lines in the L2 cache
> corresponding with the physical address, then none will be allocated.
> However, if there are cache lines present, then they will be hit,
> read or updated as appropriate.
> 
> This may matter before CMA where we had the memory returned by
> dma_alloc_coherent() and friends mapped as normal, cacheable mappings
> which could be speculatively prefetched, and therefore cache lines
> dragged into the L2 cache for these physical addresses.
> 
> However, now that we're using CMA, this does not apply as we no longer
> have this aliasing mapping.
> 
> So, with CMA enabled, it should be safe not to set this bit.
> 
Agree. That should be safe now.

> However, the shared bit in the page tables must be set for SMP systems.
> Are you sure you're not confusing the shared bit in the page tables
> with the shared override bit in the L2 cache controller?
>
No i didn't confuse with page table bits. But the SMP remark wasn't
relevant which might have indicated that.
 
>>> 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
>>> searched through the commit history of L2 cache support on OMAP4 but
>>> there is no mention of why this was needed on OMAP4. I am checking
>>> internally on the history behind this.
>>>
>> These have also come from the aligned settings with hardware folks.
> 
> Again, this doesn't have much to do with hardware, it's secure/non-secure
> access rights configuration to the L2 cache controller.
> 
The settings were aligned by hardware team after consulting security
team and those couple of bit settings came from them. The folks
are no longer working for TI so I can't go back and check the reasons.
We just just leave them as is.

Regards,
Santosh

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Wednesday 09 April 2014 10:22 PM, Santosh Shilimkar wrote:
> On Wednesday 09 April 2014 12:33 PM, Russell King - ARM Linux wrote:
>> On Tue, Apr 08, 2014 at 11:17:17AM -0400, Santosh Shilimkar wrote:
>>> On Tuesday 08 April 2014 10:53 AM, Sekhar Nori wrote:
>>>> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
>>>>> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>>>>>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>>>>>> index f8b8dac..6b2a056 100644
>>>>>> --- a/arch/arm/mach-omap2/omap4-common.c
>>>>>> +++ b/arch/arm/mach-omap2/omap4-common.c
>>>>>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>>>>>  
>>>>>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>>>>>  }
>>>>>> +
>>>>>> +int __init am43xx_l2_cache_init(void)
>>>>>> +{
>>>>>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>>>>>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
>>>>>
>>>>> It would be good to documenting the difference between this and OMAP4,
>>>>> and why you have chosen different values.
>>>>
>>>> There are two main differences:
>>>>
>>>> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
>>>> not needed even in OMAP4 with latest kernel, but I am not sure if I can
>>>> do this safely without breaking any usecase currently working with OMAP4.
>>>>
>>> Wrong. Shared bit is mandatory for the OMAP4. Its a SMP system
>>> which needs that.
>>
>> Errr.  This bit affects the L2 cache behaviour for Normal memory, outer
>> non-cacheable accesses - in other words, those performed to memory mapped
>> via dma_alloc_coherent() or dma_alloc_writecombine().  It does not affect
>> other types of mappings (other access types ignore the sharable attribute).
>>
>> When this bit is clear, accesses to such memory are:
>>
>> - read: cacheable, no allocate
>> - write: write through, no write allocate
>>
>> what this means is that if there are no cache lines in the L2 cache
>> corresponding with the physical address, then none will be allocated.
>> However, if there are cache lines present, then they will be hit,
>> read or updated as appropriate.
>>
>> This may matter before CMA where we had the memory returned by
>> dma_alloc_coherent() and friends mapped as normal, cacheable mappings
>> which could be speculatively prefetched, and therefore cache lines
>> dragged into the L2 cache for these physical addresses.
>>
>> However, now that we're using CMA, this does not apply as we no longer
>> have this aliasing mapping.
>>
>> So, with CMA enabled, it should be safe not to set this bit.
>>
> Agree. That should be safe now.

Since we cannot guarantee that CONFIG_DMA_CMA will always be enabled in
kernel config, shall we take the safer route and keep the Shared
attribute override bit enabled in L2C configuration?

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Tue, Apr 08, 2014 at 08:23:39PM +0530, Sekhar Nori wrote:
> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
> > On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
> >> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> >> index f8b8dac..6b2a056 100644
> >> --- a/arch/arm/mach-omap2/omap4-common.c
> >> +++ b/arch/arm/mach-omap2/omap4-common.c
> >> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
> >>  
> >>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
> >>  }
> >> +
> >> +int __init am43xx_l2_cache_init(void)
> >> +{
> >> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
> >> +		       L310_AUX_CTRL_INSTR_PREFETCH;
> > 
> > It would be good to documenting the difference between this and OMAP4,
> > and why you have chosen different values.
> 
> There are two main differences:
> 
> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
> not needed even in OMAP4 with latest kernel, but I am not sure if I can
> do this safely without breaking any usecase currently working with OMAP4.
> 
> 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
> searched through the commit history of L2 cache support on OMAP4 but
> there is no mention of why this was needed on OMAP4. I am checking
> internally on the history behind this.

That is required because as part of the enable sequence, we write to the
lockdown registers to clear out anything that may be there before we
enable the L2 cache.  If we didn't set the NS lockdown bit, then we
would need the secure monitor to do it for us.

The NS interrupt access bit is also a good idea to be set, since this
allows us to eventually support EDAC with PL310.  As we don't support
EDAC at the moment, or touch the interrupt registers, we can probably
ignore this difference and just preserve whatever value is there for
the time being.

Both of these bits should be managed within the L2C code rather than by
platforms.

> 3) OMAP4 sets cache replacement policy to RR which is not a big deal
> since thats the default anyway. We can probably drop this setting even
> from OMAP4.

Yes, since that would just be a case of preserving that bit.

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Wednesday 09 April 2014 09:53 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 08, 2014 at 08:23:39PM +0530, Sekhar Nori wrote:
>> On Friday 04 April 2014 03:48 PM, Russell King - ARM Linux wrote:
>>> On Fri, Apr 04, 2014 at 03:40:29PM +0530, Sekhar Nori wrote:
>>>> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
>>>> index f8b8dac..6b2a056 100644
>>>> --- a/arch/arm/mach-omap2/omap4-common.c
>>>> +++ b/arch/arm/mach-omap2/omap4-common.c
>>>> @@ -224,6 +224,14 @@ int __init omap4_l2_cache_init(void)
>>>>  
>>>>  	return omap_l2_cache_init(aux_ctrl, 0xc19fffff);
>>>>  }
>>>> +
>>>> +int __init am43xx_l2_cache_init(void)
>>>> +{
>>>> +	u32 aux_ctrl = L310_AUX_CTRL_DATA_PREFETCH |
>>>> +		       L310_AUX_CTRL_INSTR_PREFETCH;
>>>
>>> It would be good to documenting the difference between this and OMAP4,
>>> and why you have chosen different values.
>>
>> There are two main differences:
>>
>> 1) OMAP4 sets Shared attribute override enable bit. TBH, I think this is
>> not needed even in OMAP4 with latest kernel, but I am not sure if I can
>> do this safely without breaking any usecase currently working with OMAP4.
>>
>> 2) OMAP4 sets NS lockdown and NS interrupt access control bits. I
>> searched through the commit history of L2 cache support on OMAP4 but
>> there is no mention of why this was needed on OMAP4. I am checking
>> internally on the history behind this.
> 
> That is required because as part of the enable sequence, we write to the
> lockdown registers to clear out anything that may be there before we
> enable the L2 cache.  If we didn't set the NS lockdown bit, then we
> would need the secure monitor to do it for us.

And I realized yesterday that the only reason L2C is working on AM437x
is because AM437x ROM is setting these bits up for us.

> 
> The NS interrupt access bit is also a good idea to be set, since this
> allows us to eventually support EDAC with PL310.  As we don't support
> EDAC at the moment, or touch the interrupt registers, we can probably
> ignore this difference and just preserve whatever value is there for
> the time being.
> 
> Both of these bits should be managed within the L2C code rather than by
> platforms.

The current L2C code is not managing the NS_LOCKDOWN bit. I can take a
shot at adding this support unless you are already looking at it.

> 
>> 3) OMAP4 sets cache replacement policy to RR which is not a big deal
>> since thats the default anyway. We can probably drop this setting even
>> from OMAP4.
> 
> Yes, since that would just be a case of preserving that bit.

Okay will drop this explicit setting.

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Thu, Apr 10, 2014 at 05:26:15PM +0530, Sekhar Nori wrote:
> On Wednesday 09 April 2014 09:53 PM, Russell King - ARM Linux wrote:
> > That is required because as part of the enable sequence, we write to the
> > lockdown registers to clear out anything that may be there before we
> > enable the L2 cache.  If we didn't set the NS lockdown bit, then we
> > would need the secure monitor to do it for us.
> 
> And I realized yesterday that the only reason L2C is working on AM437x
> is because AM437x ROM is setting these bits up for us.
> 
> > Both of these bits should be managed within the L2C code rather than by
> > platforms.
> 
> The current L2C code is not managing the NS_LOCKDOWN bit. I can take a
> shot at adding this support unless you are already looking at it.

True, and I'm aware that it's missing.  So... how about this on top
of my series so far.  We can deal with L310_AUX_CTRL_NS_INT_CTRL when
the need to access those registers arises (if/when the edac driver
is submitted.)

diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index c0f9a81a2d32..4a494cde8367 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -213,8 +213,6 @@ static int __init omap_l2_cache_init(void)
 
 	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
 	aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
-		   L310_AUX_CTRL_NS_LOCKDOWN |
-		   L310_AUX_CTRL_NS_INT_CTRL |
 		   L2C_AUX_CTRL_SHARED_OVERRIDE |
 		   L310_AUX_CTRL_DATA_PREFETCH |
 		   L310_AUX_CTRL_INSTR_PREFETCH;
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 98796b789eb9..837f384c1d51 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -776,6 +776,13 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
 	}
 
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
+
 	l2c_enable(base, aux, num_lock);
 
 	if (aux & L310_AUX_CTRL_FULL_LINE_ZERO) {

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Thursday 10 April 2014 05:33 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 10, 2014 at 05:26:15PM +0530, Sekhar Nori wrote:
>> On Wednesday 09 April 2014 09:53 PM, Russell King - ARM Linux wrote:
>>> That is required because as part of the enable sequence, we write to the
>>> lockdown registers to clear out anything that may be there before we
>>> enable the L2 cache.  If we didn't set the NS lockdown bit, then we
>>> would need the secure monitor to do it for us.
>>
>> And I realized yesterday that the only reason L2C is working on AM437x
>> is because AM437x ROM is setting these bits up for us.
>>
>>> Both of these bits should be managed within the L2C code rather than by
>>> platforms.
>>
>> The current L2C code is not managing the NS_LOCKDOWN bit. I can take a
>> shot at adding this support unless you are already looking at it.
> 
> True, and I'm aware that it's missing.  So... how about this on top
> of my series so far.  We can deal with L310_AUX_CTRL_NS_INT_CTRL when
> the need to access those registers arises (if/when the edac driver
> is submitted.)
> 
> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> index c0f9a81a2d32..4a494cde8367 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -213,8 +213,6 @@ static int __init omap_l2_cache_init(void)
>  
>  	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
>  	aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
> -		   L310_AUX_CTRL_NS_LOCKDOWN |
> -		   L310_AUX_CTRL_NS_INT_CTRL |
>  		   L2C_AUX_CTRL_SHARED_OVERRIDE |
>  		   L310_AUX_CTRL_DATA_PREFETCH |
>  		   L310_AUX_CTRL_INSTR_PREFETCH;
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 98796b789eb9..837f384c1d51 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -776,6 +776,13 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
>  			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
>  	}
>  
> +	/*
> +	 * Always enable non-secure access to the lockdown registers -
> +	 * we write to them as part of the L2C enable sequence so they
> +	 * need to be accessible.
> +	 */
> +	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
> +

This will work. NS_LOCKDOWN is required for L2C-220 as well and so I was
thinking about adding a new l2c220_enable() which will set the
NS_LOCKDOWN and then call l2c_enable()

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Thursday 10 April 2014 05:46 PM, Sekhar Nori wrote:
> This will work. NS_LOCKDOWN is required for L2C-220 as well and so I was
> thinking about adding a new l2c220_enable() which will set the
> NS_LOCKDOWN and then call l2c_enable()

Here is a patch for what I was saying above.

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index c47ac8f..dc9e03b 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -105,6 +105,8 @@
 #define L2X0_AUX_CTRL_DIRTY_LATENCY_MASK	(7 << 9)
 #define L2X0_AUX_CTRL_ASSOC_SHIFT		13
 #define L2X0_AUX_CTRL_ASSOC_MASK		(15 << 13)
+/* L2C-220/310 common bits */
+#define L2C_AUX_CTRL_NS_LOCKDOWN		BIT(26)
 /* L2C-210 specific bits */
 #define L210_AUX_CTRL_WRAP_DISABLE		BIT(12)
 #define L210_AUX_CTRL_WA_OVERRIDE		BIT(23)
@@ -113,7 +115,6 @@
 #define L220_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
 #define L220_AUX_CTRL_FWA_SHIFT			23
 #define L220_AUX_CTRL_FWA_MASK			(3 << 23)
-#define L220_AUX_CTRL_NS_LOCKDOWN		BIT(26)
 #define L220_AUX_CTRL_NS_INT_CTRL		BIT(27)
 /* L2C-310 specific bits */
 #define L310_AUX_CTRL_FULL_LINE_ZERO		BIT(0)	/* R2P0+ */
@@ -122,7 +123,6 @@
 #define L310_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
 #define L310_AUX_CTRL_ASSOCIATIVITY_16		BIT(16)
 #define L310_AUX_CTRL_CACHE_REPLACE_RR		BIT(25)	/* R2P0+ */
-#define L310_AUX_CTRL_NS_LOCKDOWN		BIT(26)
 #define L310_AUX_CTRL_NS_INT_CTRL		BIT(27)
 #define L310_AUX_CTRL_DATA_PREFETCH		BIT(28)
 #define L310_AUX_CTRL_INSTR_PREFETCH		BIT(29)
diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index 6b2a056..34cafe0 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -216,8 +216,6 @@ int __init omap4_l2_cache_init(void)
 {
 	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
 	u32 aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
-		       L310_AUX_CTRL_NS_LOCKDOWN |
-		       L310_AUX_CTRL_NS_INT_CTRL |
 		       L2C_AUX_CTRL_SHARED_OVERRIDE |
 		       L310_AUX_CTRL_DATA_PREFETCH |
 		       L310_AUX_CTRL_INSTR_PREFETCH;
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index b1f103d..b6af13f 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -498,11 +498,23 @@ static void l2c220_sync(void)
 	raw_spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
+static void __init l2c220_enable(void __iomem *base, u32 aux, unsigned num_lock)
+{
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L2C_AUX_CTRL_NS_LOCKDOWN;
+
+	l2c_enable(base, aux, num_lock);
+}
+
 static const struct l2c_init_data l2c220_data = {
 	.type = "L2C-220",
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.outer_cache = {
 		.inv_range = l2c220_inv_range,
 		.clean_range = l2c220_clean_range,
@@ -764,7 +776,7 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
 	}
 
-	l2c_enable(base, aux, num_lock);
+	l2c220_enable(base, aux, num_lock);
 
 	if (aux & L310_AUX_CTRL_FULL_LINE_ZERO) {
 		set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
@@ -1027,7 +1039,7 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
 	.of_parse = l2x0_of_parse,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.outer_cache = {
 		.inv_range   = l2c220_inv_range,
 		.clean_range = l2c220_clean_range,

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Thu, Apr 10, 2014 at 06:57:05PM +0530, Sekhar Nori wrote:
> On Thursday 10 April 2014 05:46 PM, Sekhar Nori wrote:
> > This will work. NS_LOCKDOWN is required for L2C-220 as well and so I was
> > thinking about adding a new l2c220_enable() which will set the
> > NS_LOCKDOWN and then call l2c_enable()
> 
> Here is a patch for what I was saying above.
> 
> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> index c47ac8f..dc9e03b 100644
> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> @@ -105,6 +105,8 @@
>  #define L2X0_AUX_CTRL_DIRTY_LATENCY_MASK	(7 << 9)
>  #define L2X0_AUX_CTRL_ASSOC_SHIFT		13
>  #define L2X0_AUX_CTRL_ASSOC_MASK		(15 << 13)
> +/* L2C-220/310 common bits */
> +#define L2C_AUX_CTRL_NS_LOCKDOWN		BIT(26)
>  /* L2C-210 specific bits */
>  #define L210_AUX_CTRL_WRAP_DISABLE		BIT(12)
>  #define L210_AUX_CTRL_WA_OVERRIDE		BIT(23)
> @@ -113,7 +115,6 @@
>  #define L220_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
>  #define L220_AUX_CTRL_FWA_SHIFT			23
>  #define L220_AUX_CTRL_FWA_MASK			(3 << 23)
> -#define L220_AUX_CTRL_NS_LOCKDOWN		BIT(26)
>  #define L220_AUX_CTRL_NS_INT_CTRL		BIT(27)
>  /* L2C-310 specific bits */
>  #define L310_AUX_CTRL_FULL_LINE_ZERO		BIT(0)	/* R2P0+ */
> @@ -122,7 +123,6 @@
>  #define L310_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
>  #define L310_AUX_CTRL_ASSOCIATIVITY_16		BIT(16)
>  #define L310_AUX_CTRL_CACHE_REPLACE_RR		BIT(25)	/* R2P0+ */
> -#define L310_AUX_CTRL_NS_LOCKDOWN		BIT(26)

NAK.  The reason for this split is because the NS lockdown bit is *not*
on L2C-210, and so it does not deserve to be a "common" bit - because it
isn't common to all variants.

> @@ -764,7 +776,7 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
>  			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
>  	}
>  
> -	l2c_enable(base, aux, num_lock);
> +	l2c220_enable(base, aux, num_lock);

My first reaction to this is to say NAK again - I don't want to create
a multi-layered maze of X calls Y calls Z.  Who's to say that The 220
won't need to do something different from 310 in the future?

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Thursday 10 April 2014 07:10 PM, Russell King - ARM Linux wrote:
> On Thu, Apr 10, 2014 at 06:57:05PM +0530, Sekhar Nori wrote:
>> On Thursday 10 April 2014 05:46 PM, Sekhar Nori wrote:
>>> This will work. NS_LOCKDOWN is required for L2C-220 as well and so I was
>>> thinking about adding a new l2c220_enable() which will set the
>>> NS_LOCKDOWN and then call l2c_enable()
>>
>> Here is a patch for what I was saying above.
>>
>> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
>> index c47ac8f..dc9e03b 100644
>> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
>> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
>> @@ -105,6 +105,8 @@
>>  #define L2X0_AUX_CTRL_DIRTY_LATENCY_MASK	(7 << 9)
>>  #define L2X0_AUX_CTRL_ASSOC_SHIFT		13
>>  #define L2X0_AUX_CTRL_ASSOC_MASK		(15 << 13)
>> +/* L2C-220/310 common bits */
>> +#define L2C_AUX_CTRL_NS_LOCKDOWN		BIT(26)
>>  /* L2C-210 specific bits */
>>  #define L210_AUX_CTRL_WRAP_DISABLE		BIT(12)
>>  #define L210_AUX_CTRL_WA_OVERRIDE		BIT(23)
>> @@ -113,7 +115,6 @@
>>  #define L220_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
>>  #define L220_AUX_CTRL_FWA_SHIFT			23
>>  #define L220_AUX_CTRL_FWA_MASK			(3 << 23)
>> -#define L220_AUX_CTRL_NS_LOCKDOWN		BIT(26)
>>  #define L220_AUX_CTRL_NS_INT_CTRL		BIT(27)
>>  /* L2C-310 specific bits */
>>  #define L310_AUX_CTRL_FULL_LINE_ZERO		BIT(0)	/* R2P0+ */
>> @@ -122,7 +123,6 @@
>>  #define L310_AUX_CTRL_EXCLUSIVE_CACHE		BIT(12)
>>  #define L310_AUX_CTRL_ASSOCIATIVITY_16		BIT(16)
>>  #define L310_AUX_CTRL_CACHE_REPLACE_RR		BIT(25)	/* R2P0+ */
>> -#define L310_AUX_CTRL_NS_LOCKDOWN		BIT(26)
> 
> NAK.  The reason for this split is because the NS lockdown bit is *not*
> on L2C-210, and so it does not deserve to be a "common" bit - because it
> isn't common to all variants.

Okay.

> 
>> @@ -764,7 +776,7 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
>>  			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
>>  	}
>>  
>> -	l2c_enable(base, aux, num_lock);
>> +	l2c220_enable(base, aux, num_lock);
> 
> My first reaction to this is to say NAK again - I don't want to create
> a multi-layered maze of X calls Y calls Z.  Who's to say that The 220
> won't need to do something different from 310 in the future?

Here is a revised patch which is just an extension of your patch 
with L2C-220 case handled. I dont really have L2C-220 hardware so even 
if you want to handle that at a later time, it would be perfectly okay 
with me.

Thanks,
Sekhar

diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index 6b2a056..34cafe0 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -216,8 +216,6 @@ int __init omap4_l2_cache_init(void)
 {
 	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
 	u32 aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
-		       L310_AUX_CTRL_NS_LOCKDOWN |
-		       L310_AUX_CTRL_NS_INT_CTRL |
 		       L2C_AUX_CTRL_SHARED_OVERRIDE |
 		       L310_AUX_CTRL_DATA_PREFETCH |
 		       L310_AUX_CTRL_INSTR_PREFETCH;
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index b1f103d..72f41fa 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -498,11 +498,23 @@ static void l2c220_sync(void)
 	raw_spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
+static void __init l2c220_enable(void __iomem *base, u32 aux, unsigned num_lock)
+{
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L220_AUX_CTRL_NS_LOCKDOWN;
+
+	l2c_enable(base, aux, num_lock);
+}
+
 static const struct l2c_init_data l2c220_data = {
 	.type = "L2C-220",
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.outer_cache = {
 		.inv_range = l2c220_inv_range,
 		.clean_range = l2c220_clean_range,
@@ -764,6 +776,13 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
 	}
 
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
+
 	l2c_enable(base, aux, num_lock);
 
 	if (aux & L310_AUX_CTRL_FULL_LINE_ZERO) {
@@ -1027,7 +1046,7 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
 	.of_parse = l2x0_of_parse,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.outer_cache = {
 		.inv_range   = l2c220_inv_range,
 		.clean_range = l2c220_clean_range,

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Russell King - ARM Linux]

On Fri, Apr 11, 2014 at 11:03:57AM +0530, Sekhar Nori wrote:
> Here is a revised patch which is just an extension of your patch 
> with L2C-220 case handled. I dont really have L2C-220 hardware so even 
> if you want to handle that at a later time, it would be perfectly okay 
> with me.

This is what I came up with, which of course is very similar to yours.
I think the only difference is that I'm allowing the state of the NS
access bits to be preserved by the OMAP code, getting OMAP closer to
the target of a ~0 mask.  The only bits which are clear in the mask
passed into the L2 code by OMAP now are:

- L310_AUX_CTRL_INSTR_PREFETCH
- L310_AUX_CTRL_DATA_PREFETCH
- L310_AUX_CTRL_CACHE_REPLACE_RR
- L2C_AUX_CTRL_SHARED_OVERRIDE
- L2C_AUX_CTRL_PARITY_ENABLE

It sounds like we can kill L310_AUX_CTRL_CACHE_REPLACE_RR as well since
that's already set for us (and fwir is the power-on-reset default too.)

 arch/arm/mach-omap2/omap4-common.c |  4 +---
 arch/arm/mm/cache-l2x0.c           | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
index c0f9a81a2d32..3b01c5223b11 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -213,8 +213,6 @@ static int __init omap_l2_cache_init(void)
 
 	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
 	aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
-		   L310_AUX_CTRL_NS_LOCKDOWN |
-		   L310_AUX_CTRL_NS_INT_CTRL |
 		   L2C_AUX_CTRL_SHARED_OVERRIDE |
 		   L310_AUX_CTRL_DATA_PREFETCH |
 		   L310_AUX_CTRL_INSTR_PREFETCH;
@@ -223,7 +221,7 @@ static int __init omap_l2_cache_init(void)
 	if (of_have_populated_dt())
 		l2x0_of_init(aux_ctrl, 0xc19fffff);
 	else
-		l2x0_init(l2cache_base, aux_ctrl, 0xc19fffff);
+		l2x0_init(l2cache_base, aux_ctrl, 0xcd9fffff);
 
 	return 0;
 }
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 98796b789eb9..5ec454d51a9f 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -502,11 +502,23 @@ static void l2c220_sync(void)
 	raw_spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
+static void l2c220_enable(void __iomem *base, u32 aux, unsigned num_lock)
+{
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L220_AUX_CTRL_NS_LOCKDOWN;
+
+	l2c_enable(base, aux, num_lock);
+}
+
 static const struct l2c_init_data l2c220_data = {
 	.type = "L2C-220",
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.save = l2c_save,
 	.outer_cache = {
 		.inv_range = l2c220_inv_range,
@@ -776,6 +788,13 @@ static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 			power_ctrl & L310_STNDBY_MODE_EN ? "en" : "dis");
 	}
 
+	/*
+	 * Always enable non-secure access to the lockdown registers -
+	 * we write to them as part of the L2C enable sequence so they
+	 * need to be accessible.
+	 */
+	aux |= L310_AUX_CTRL_NS_LOCKDOWN;
+
 	l2c_enable(base, aux, num_lock);
 
 	if (aux & L310_AUX_CTRL_FULL_LINE_ZERO) {
@@ -1052,7 +1071,7 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
 	.way_size_0 = SZ_8K,
 	.num_lock = 1,
 	.of_parse = l2x0_of_parse,
-	.enable = l2c_enable,
+	.enable = l2c220_enable,
 	.save = l2c_save,
 	.outer_cache = {
 		.inv_range   = l2c220_inv_range,

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

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Friday 11 April 2014 04:55 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 11, 2014 at 11:03:57AM +0530, Sekhar Nori wrote:
>> Here is a revised patch which is just an extension of your patch 
>> with L2C-220 case handled. I dont really have L2C-220 hardware so even 
>> if you want to handle that at a later time, it would be perfectly okay 
>> with me.
> 
> This is what I came up with, which of course is very similar to yours.
> I think the only difference is that I'm allowing the state of the NS
> access bits to be preserved by the OMAP code, getting OMAP closer to
> the target of a ~0 mask.  The only bits which are clear in the mask
> passed into the L2 code by OMAP now are:

Thanks for sending the patch. I will use it to rebase the $subject
series against.

> 
> - L310_AUX_CTRL_INSTR_PREFETCH
> - L310_AUX_CTRL_DATA_PREFETCH
> - L310_AUX_CTRL_CACHE_REPLACE_RR
> - L2C_AUX_CTRL_SHARED_OVERRIDE
> - L2C_AUX_CTRL_PARITY_ENABLE
> 
> It sounds like we can kill L310_AUX_CTRL_CACHE_REPLACE_RR as well since
> that's already set for us (and fwir is the power-on-reset default too.)

Thanks right.

Thanks,
Sekhar

[PATCH v2 3/3] ARM: OMAP2+: AM43x: L2 cache support

[Sekhar Nori]

On Friday 11 April 2014 04:55 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 11, 2014 at 11:03:57AM +0530, Sekhar Nori wrote:
>> Here is a revised patch which is just an extension of your patch 
>> with L2C-220 case handled. I dont really have L2C-220 hardware so even 
>> if you want to handle that at a later time, it would be perfectly okay 
>> with me.
> 
> This is what I came up with, which of course is very similar to yours.
> I think the only difference is that I'm allowing the state of the NS
> access bits to be preserved by the OMAP code, getting OMAP closer to
> the target of a ~0 mask.  The only bits which are clear in the mask
> passed into the L2 code by OMAP now are:
> 
> - L310_AUX_CTRL_INSTR_PREFETCH
> - L310_AUX_CTRL_DATA_PREFETCH
> - L310_AUX_CTRL_CACHE_REPLACE_RR
> - L2C_AUX_CTRL_SHARED_OVERRIDE
> - L2C_AUX_CTRL_PARITY_ENABLE
> 
> It sounds like we can kill L310_AUX_CTRL_CACHE_REPLACE_RR as well since
> that's already set for us (and fwir is the power-on-reset default too.)
> 
>  arch/arm/mach-omap2/omap4-common.c |  4 +---
>  arch/arm/mm/cache-l2x0.c           | 23 +++++++++++++++++++++--
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c
> index c0f9a81a2d32..3b01c5223b11 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -213,8 +213,6 @@ static int __init omap_l2_cache_init(void)
>  
>  	/* 16-way associativity, parity disabled, way size - 64KB (es2.0 +) */
>  	aux_ctrl = L310_AUX_CTRL_CACHE_REPLACE_RR |
> -		   L310_AUX_CTRL_NS_LOCKDOWN |
> -		   L310_AUX_CTRL_NS_INT_CTRL |
>  		   L2C_AUX_CTRL_SHARED_OVERRIDE |
>  		   L310_AUX_CTRL_DATA_PREFETCH |
>  		   L310_AUX_CTRL_INSTR_PREFETCH;
> @@ -223,7 +221,7 @@ static int __init omap_l2_cache_init(void)
>  	if (of_have_populated_dt())
>  		l2x0_of_init(aux_ctrl, 0xc19fffff);

Just noticed that the mask here too should have been changed to
0xcd9fffff. I will be making this change to the local version I am
committing to base my AM43x support series on.

>  	else
> -		l2x0_init(l2cache_base, aux_ctrl, 0xc19fffff);
> +		l2x0_init(l2cache_base, aux_ctrl, 0xcd9fffff);

Thanks,
Sekhar