[0/5] Handle non-secure L2C initialization on Exynos4
diff mbox

Message ID CAD8Lp46MbKZrCAMp+9T9LbWTB4uHRDdivhtjoQbkTyBbK48EgQ@mail.gmail.com
State New, archived
Headers show

Commit Message

Daniel Drake June 12, 2014, 1:38 p.m. UTC
Hi Tomasz,

Thanks for working on this!

I have just tried this, against Linus master
64b2d1fbbfda07765dae3f601862796a61b2c451.
Added patch "ARM: dts: Initial ODROID U2 support" and booted on
ODROID-U2. I believe this board has the security enabled.

Unfortunately, it hangs during early boot. With earlyprintk the last
messages seen are:

L2C: platform modifies aux control register: 0x02070000 -> 0x3e470001
L2C: platform provided aux values permit register corruption.
L2C: DT/platform modifies aux control register: 0x02070000 -> 0x3e470001
L2C-310 enabling early BRESP for Cortex-A9
L2C-310: enabling full line of zeros but not enabled in Cortex-A9
L2C-310 ID prefetch enabled, offset 1 lines
L2C-310 dynamic clock gating enabled, standby mode enabled
L2C-310 cache controller enabled, 16 ways, 1024 kB
L2C-310: CACHE_ID 0x4100c4c8, AUX_CTRL 0x7e470001

I then tried to go back to the earlier patch "ARM: EXYNOS: Add secure
firmware support for l2x0 init" (attached, needed a rebase) but that
one also now hangs at:

L2C: platform modifies aux control register: 0x02070000 -> 0x3e470001

It did work on 3.14 though. Looking at the changelogs, many changes
have been made to l2x0 recently. Can you confirm that you have tested
your patches against a kernel with all of Russell King's recent
changes?

Thanks
Daniel
From 2e67231f10ed0b05c2bacfdd05774fe21315d6da Mon Sep 17 00:00:00 2001
From: Gu1 <gu1@aeroxteam.fr>
Date: Mon, 21 Jan 2013 04:13:56 +0100
Subject: [PATCH] ARM: EXYNOS: Add secure firmware support for l2x0 init

Conflicts:
	arch/arm/mm/cache-l2x0.c
---
 arch/arm/mach-exynos/firmware.c | 19 +++++++++++++++++++
 arch/arm/mm/cache-l2x0.c        |  3 +++
 2 files changed, 22 insertions(+)

Comments

Tomasz Figa June 12, 2014, 2:13 p.m. UTC | #1
Hi Daniel,

On 12.06.2014 15:38, Daniel Drake wrote:
> Hi Tomasz,
> 
> Thanks for working on this!
> 
> I have just tried this, against Linus master
> 64b2d1fbbfda07765dae3f601862796a61b2c451.
> Added patch "ARM: dts: Initial ODROID U2 support" and booted on
> ODROID-U2. I believe this board has the security enabled.
> 
> Unfortunately, it hangs during early boot. With earlyprintk the last
> messages seen are:
> 
> L2C: platform modifies aux control register: 0x02070000 -> 0x3e470001
> L2C: platform provided aux values permit register corruption.
> L2C: DT/platform modifies aux control register: 0x02070000 -> 0x3e470001
> L2C-310 enabling early BRESP for Cortex-A9
> L2C-310: enabling full line of zeros but not enabled in Cortex-A9
> L2C-310 ID prefetch enabled, offset 1 lines
> L2C-310 dynamic clock gating enabled, standby mode enabled
> L2C-310 cache controller enabled, 16 ways, 1024 kB
> L2C-310: CACHE_ID 0x4100c4c8, AUX_CTRL 0x7e470001
> 

Thanks for testing. We have some ODROIDs U2 and U3 here too so I'll try
to reproduce and investigate the issue.

> I then tried to go back to the earlier patch "ARM: EXYNOS: Add secure
> firmware support for l2x0 init" (attached, needed a rebase) but that
> one also now hangs at:
> 
> L2C: platform modifies aux control register: 0x02070000 -> 0x3e470001
> 
> It did work on 3.14 though. Looking at the changelogs, many changes
> have been made to l2x0 recently. Can you confirm that you have tested
> your patches against a kernel with all of Russell King's recent
> changes?

I'm usually working on latest linux-next, so I'm pretty sure all the
mentioned patches were already in my tree. The base for this series was
next-20140610 tag of linux-next tree.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux June 12, 2014, 4:20 p.m. UTC | #2
On Thu, Jun 12, 2014 at 02:38:49PM +0100, Daniel Drake wrote:
> From 2e67231f10ed0b05c2bacfdd05774fe21315d6da Mon Sep 17 00:00:00 2001
> From: Gu1 <gu1@aeroxteam.fr>
> Date: Mon, 21 Jan 2013 04:13:56 +0100
> Subject: [PATCH] ARM: EXYNOS: Add secure firmware support for l2x0 init
> 
> Conflicts:
> 	arch/arm/mm/cache-l2x0.c

For this patch... it's a big NAK because it's taking us right back down
the route of a totally fucked up cache-l2x0.c driver.

Why can't you people write stuff properly?  There's already another set
of patches on this mailing list which want to pass the virtual base
address of the l2x0 controller to l2c_write_sec() so that various
registers can be read back, because the platform's secure API can
only update several registers at the same time.

This is the same pattern that is revealed in this patch.  So, what
this means is that the l2c_write_sec API is wrong.  We need to come
up with a *replacement* API which allows the platforms to do this
kind of setup in a *clean* way, and stop creating rotten hacks like
this which just makes long term maintanence a nightmare.

So... please start doing stuff properly.  If you don't, you're going
to be getting more flames from me, especially if you start doing this
kind of hackery on code that I've been cleaning up to get rid of such
crap.
Tomasz Figa June 12, 2014, 4:47 p.m. UTC | #3
Hi Russell,

On 12.06.2014 18:20, Russell King - ARM Linux wrote:
> On Thu, Jun 12, 2014 at 02:38:49PM +0100, Daniel Drake wrote:
>> From 2e67231f10ed0b05c2bacfdd05774fe21315d6da Mon Sep 17 00:00:00 2001
>> From: Gu1 <gu1@aeroxteam.fr>
>> Date: Mon, 21 Jan 2013 04:13:56 +0100
>> Subject: [PATCH] ARM: EXYNOS: Add secure firmware support for l2x0 init
>>
>> Conflicts:
>> 	arch/arm/mm/cache-l2x0.c
> 
> For this patch... it's a big NAK because it's taking us right back down
> the route of a totally fucked up cache-l2x0.c driver.

This is just an old, internal patch that was not going to be upstreamed.
I just posted another series yesterday[1], hopefully doing it the right
way. Looking forward for review comments.

[1] http://thread.gmane.org/gmane.linux.kernel/1722989

> 
> Why can't you people write stuff properly?  There's already another set
> of patches on this mailing list which want to pass the virtual base
> address of the l2x0 controller to l2c_write_sec() so that various
> registers can be read back, because the platform's secure API can
> only update several registers at the same time.

Probably the series you mention is [1]? :)

> 
> This is the same pattern that is revealed in this patch.  So, what
> this means is that the l2c_write_sec API is wrong.  We need to come
> up with a *replacement* API which allows the platforms to do this
> kind of setup in a *clean* way, and stop creating rotten hacks like
> this which just makes long term maintanence a nightmare.
> 
> So... please start doing stuff properly.  If you don't, you're going
> to be getting more flames from me, especially if you start doing this
> kind of hackery on code that I've been cleaning up to get rid of such
> crap.
> 

Yes, I fully agree. Fortunately that was not supposed to hit upstream.

You've done a lot of great work to refactor this driver (thanks!), which
made it possible to support Exynos secure firmware in a sane way and
this is how it should be done.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index eb91d23..c200f77 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -16,6 +16,7 @@ 
 
 #include <asm/firmware.h>
 
+#include <asm/hardware/cache-l2x0.h>
 #include <mach/map.h>
 
 #include "common.h"
@@ -64,10 +65,28 @@  static int exynos_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
 	return 0;
 }
 
+extern struct l2x0_regs l2x0_saved_regs;
+#define L2_AUX_VAL 0x7C470001
+#define L2_AUX_MASK 0xC200ffff
+
+static int exynos_l2x0_init(void)
+{
+	exynos_smc(SMC_CMD_L2X0SETUP1, l2x0_saved_regs.tag_latency,
+			l2x0_saved_regs.data_latency,
+			l2x0_saved_regs.prefetch_ctrl);
+	exynos_smc(SMC_CMD_L2X0SETUP2, l2x0_saved_regs.pwr_ctrl,
+			L2_AUX_VAL, L2_AUX_MASK);
+	exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
+	exynos_smc(SMC_CMD_L2X0CTRL, 1, 0, 0);
+	return 0;
+}
+
+
 static const struct firmware_ops exynos_firmware_ops = {
 	.do_idle		= exynos_do_idle,
 	.set_cpu_boot_addr	= exynos_set_cpu_boot_addr,
 	.cpu_boot		= exynos_cpu_boot,
+	.l2x0_init		= exynos_l2x0_init,
 };
 
 void __init exynos_firmware_init(void)
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index efc5cab..7367bef 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -29,6 +29,7 @@ 
 #include <asm/cp15.h>
 #include <asm/cputype.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/firmware.h>
 #include "cache-tauros3.h"
 #include "cache-aurora-l2.h"
 
@@ -1504,6 +1505,8 @@  int __init l2x0_of_init(u32 aux_val, u32 aux_mask)
 		if (data->of_parse)
 			data->of_parse(np, &aux_val, &aux_mask);
 
+	call_firmware_op(l2x0_init);
+
 	if (cache_id_part_number_from_dt)
 		cache_id = cache_id_part_number_from_dt;
 	else