Enabling socfpga on hardware

From: Dinh Nguyen <[email protected]>

Up to this point, support for socfpga has only been on a virtual platform. Now that actual hardware is available, we add the appropriate device tree source files.

Signed-off-by Dinh Nguyen <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

arch/arm/boot/dts/socfpga_cyclone5.dts +1 -1

index ab7e4a9..7ad3cc6 100644
@@ -20,7 +20,7 @@
 
 / {
 	model = "Altera SOCFPGA Cyclone V";
-	compatible = "altr,socfpga-cyclone5";
+	compatible = "altr,socfpga-cyclone5", "altr,socfpga";
 
 	chosen {
 		bootargs = "console=ttyS0,57600";

arch/arm/boot/dts/socfpga_vt.dts new file +0 -0

new file mode 100644
index 0000000..a0c6c65

arch/arm/boot/dts/socfpga_vt.dts new file +9 -0

+
+/ {
+	model = "Altera SOCFPGA VT";
+	compatible = "altr,socfpga-vt", "altr,socfpga";
+
+	chosen {
+		bootargs = "console=ttyS0,57600";
+	};
+

Quoted 6 lines · dinguyen at altera.com

From: Dinh Nguyen <[email protected]>

Up to this point, support for socfpga has only been on a virtual platform. Now that actual hardware is available, we add the appropriate device tree source files.

Quoted 6 lines

Signed-off-by Dinh Nguyen <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Are you planning on sending us a merge request, or do you want us to apply this to a branch in arm-soc for you?

(Note comment below though)

Quoted 13 lines

arch/arm/boot/dts/socfpga_cyclone5.dts +1 -1

index ab7e4a9..7ad3cc6 100644
@@ -20,7 +20,7 @@
 
 / {
 	model = "Altera SOCFPGA Cyclone V";
-	compatible = "altr,socfpga-cyclone5";
+	compatible = "altr,socfpga-cyclone5", "altr,socfpga";
 
 	chosen {
 		bootargs = "console=ttyS0,57600";

[...]

Quoted 5 lines

arch/arm/boot/dts/socfpga_vt.dts new file +0 -0

new file mode 100644
index 0000000..a0c6c65

[...]

Quoted 9 lines

arch/arm/boot/dts/socfpga_vt.dts new file +9 -0

+
+/ {
+	model = "Altera SOCFPGA VT";
+	compatible = "altr,socfpga-vt", "altr,socfpga";
+
+	chosen {
+		bootargs = "console=ttyS0,57600";
+	};
+

[...]

arch/arm/mach-socfpga/socfpga.c static void __init socfpga_cyclone5_init(void) · line 104

Quoted 11 lines

arch/arm/mach-socfpga/socfpga.c +1 -0

index 6732924..198f491 100644
@@ -99,6 +99,7 @@ static void __init socfpga_cyclone5_init(void)
 static const char *altera_dt_match[] = {
 	"altr,socfpga",
 	"altr,socfpga-cyclone5",
+	"altr,socfpga-vt",
 	NULL
 };

Since you have altr,socfpga in the compatible for the new board, you don't need to add an explicit check for the more specific one here. Same for cyclone5, you should even be able to remove that.

-Olof

From: Dinh Nguyen <[email protected]>

mach-socfpga is another platform that needs to use v7_invalidate_l1 to bringup additional cores. There was a comment that the ideal place for v7_invalidate_l1 should be in arm/mm/cache-v7.S

From: Dinh Nguyen <[email protected]>

mach-socfpga is another platform that needs to use v7_invalidate_l1 to bringup additional cores. There was a comment that the ideal place for v7_invalidate_l1 should be in arm/mm/cache-v7.S

Signed-off-by Dinh Nguyen <[email protected]>

Acked-by Simon Horman <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Russell King <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Stephen Warren <[email protected]>

From: Dinh Nguyen <[email protected]>

mach-socfpga is another platform that needs to use v7_invalidate_l1 to bringup additional cores. There was a comment that the ideal place for v7_invalidate_l1 should be in arm/mm/cache-v7.S

Signed-off-by Dinh Nguyen <[email protected]>

Acked-by Simon Horman <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Russell King <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Stephen Warren <[email protected]>

---
  arch/arm/mach-imx/headsmp.S      |   47 -------------------------------------
  arch/arm/mach-shmobile/headsmp.S |   48 --------------------------------------
  arch/arm/mach-tegra/headsmp.S    |   43 ----------------------------------
  arch/arm/mm/cache-v7.S           |   46 ++++++++++++++++++++++++++++++++++++
  4 files changed, 46 insertions(+), 138 deletions(-)

arch/arm/mm/cache-v7.S +3 -0

index 7539ec2..15451ee 100644
@@ -19,6 +19,52 @@
  #include "proc-macros.S"
 
  /*
+ * The secondary kernel init calls v7_flush_dcache_all before it enables
+ * the L1; however, the L1 comes out of reset in an undefined state, so
+ * the clean + invalidate performed by v7_flush_dcache_all causes a bunch

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Quoted 3 lines · Santosh Shilimkar

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Hmm. Do you really want a CPU being brought up to do that to the PoU, every time that it is brought up? I thought you wanted to get rid of that kind of stuff from the hotplug paths so that a CPU being brought up/taken down doesn't affect the caches for the other CPUs within the inner sharable domain.

Quoted 11 lines · Russell King - ARM Linux

Quoted 3 lines · Santosh Shilimkar

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Hmm. Do you really want a CPU being brought up to do that to the PoU, every time that it is brought up? I thought you wanted to get rid of that kind of stuff from the hotplug paths so that a CPU being brought up/taken down doesn't affect the caches for the other CPUs within the inner sharable domain.

You are right. We already git rid of the flush of all cache levels in hotplug and wakeup paths and now it is restricted till the PoU.

Assuming for the current v7 machines, PoU is L2, invalidating the cache *till* PoU means only CPU local cache. So the API will in a way invalidate only local cache.

Quoted 12 lines · Santosh Shilimkar

Quoted 11 lines · Russell King - ARM Linux

Quoted 3 lines · Santosh Shilimkar

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Hmm. Do you really want a CPU being brought up to do that to the PoU, every time that it is brought up? I thought you wanted to get rid of that kind of stuff from the hotplug paths so that a CPU being brought up/taken down doesn't affect the caches for the other CPUs within the inner sharable domain.

Quoted 6 lines

You are right. We already git rid of the flush of all cache levels in hotplug and wakeup paths and now it is restricted till the PoU.

Assuming for the current v7 machines, PoU is L2, invalidating the cache *till* PoU means only CPU local cache. So the API will in a way invalidate only local cache.

Err, you want to _invalidate_ the caches down to the point of I/D/TLB unification? Are you really sure you want to do that on a system here other CPUs are running?

Even going down to the LoUIS, that point is the point at which the _other_ CPUs may be sharing caches.

And invalidating those caches while the other CPUs are running on secondary CPU startup will be VERY VERY VERY bad.

Quoted 27 lines · Russell King - ARM Linux

Quoted 12 lines · Santosh Shilimkar

Quoted 11 lines · Russell King - ARM Linux

Quoted 3 lines · Santosh Shilimkar

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Hmm. Do you really want a CPU being brought up to do that to the PoU, every time that it is brought up? I thought you wanted to get rid of that kind of stuff from the hotplug paths so that a CPU being brought up/taken down doesn't affect the caches for the other CPUs within the inner sharable domain.

Quoted 6 lines

You are right. We already git rid of the flush of all cache levels in hotplug and wakeup paths and now it is restricted till the PoU.

Assuming for the current v7 machines, PoU is L2, invalidating the cache *till* PoU means only CPU local cache. So the API will in a way invalidate only local cache.

Err, you want to _invalidate_ the caches down to the point of I/D/TLB unification? Are you really sure you want to do that on a system here other CPUs are running?

Even going down to the LoUIS, that point is the point at which the _other_ CPUs may be sharing caches.

Quoted 3 lines

And invalidating those caches while the other CPUs are running on secondary CPU startup will be VERY VERY VERY bad.

Absolutly and my intention was never to invalidate all the cache levels. When I said lous, I mean till that point and not including that and next cache levels. May be my terminology isn't accurate.

Quoted 31 lines · Santosh Shilimkar

Quoted 30 lines · Russell King - ARM Linux

Quoted 12 lines · Santosh Shilimkar

Quoted 11 lines · Russell King - ARM Linux

Quoted 3 lines · Santosh Shilimkar

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Hmm. Do you really want a CPU being brought up to do that to the PoU, every time that it is brought up? I thought you wanted to get rid of that kind of stuff from the hotplug paths so that a CPU being brought up/taken down doesn't affect the caches for the other CPUs within the inner sharable domain.

Quoted 6 lines

You are right. We already git rid of the flush of all cache levels in hotplug and wakeup paths and now it is restricted till the PoU.

Assuming for the current v7 machines, PoU is L2, invalidating the cache *till* PoU means only CPU local cache. So the API will in a way invalidate only local cache.

Err, you want to _invalidate_ the caches down to the point of I/D/TLB unification? Are you really sure you want to do that on a system here other CPUs are running?

Even going down to the LoUIS, that point is the point at which the _other_ CPUs may be sharing caches.

And invalidating those caches while the other CPUs are running on secondary CPU startup will be VERY VERY VERY bad.

Quoted 3 lines

Absolutly and my intention was never to invalidate all the cache levels. When I said lous, I mean till that point and not including that and next cache levels. May be my terminology isn't accurate.

Confused. Can't find the term "lous" in your previous mails. I'll assume you mean LoUIS. I'm saying that's totally wrong because going down to that point _includes_ the other CPUs in the system.

What we should be doing on secondary CPU bringup for CPUs where the caches are in an unknown state is invalidating those caches which are local to _that_ _CPU_ _only_. That is not "all cache levels down to LoUIS".

Here's an example. CPU0-3 are in the inner sharable domain.

+----------+ +----------+ +----------+ +----------+
|   CPU0   | |   CPU1   | |   CPU2   | |   CPU3   |
+----------+ +----------+ +----------+ +----------+
    |    |       |    |       |    |       |    |
+--v-+--v-+  +--v-+--v-+  +--v-+--v-+  +--v-+--v-+
|CL0I|CL0D|  |CL0I|CL0D|  |CL0I|CL0D|  |CL0I|CL0D| <-- cache level 0
+----+----+  +----+----+  +----+----+  +----+----+
    |    |       |    |       |    |       |    |
    |    |   +---+    +--+    |    |       |    +------------------+
    |    |   |           |    |    +-------|--------------+        |
    |    |   |        +-------+            |              |        |
    |    |   |        |  +-----------------|-----+        |        |
    |    |   |        |        +-----------+     |        |        |
    |    +---|--------|--------|--------+        |        |        |
+--v--------v--------v--------v--+  +--v--------v--------v--------v--+
|            CL1I                |  |            CL1D                | level 1
+--------------------------------+  +--------------------------------+
                  |                                   |
+----------------v-----------------------------------v---------------+
|                                 CL2                                | level 2
+--------------------------------------------------------------------+

Therefore, because the point of unification for the inner sharable domain is defined as the point at which the I, D and TLB streams (TLB not shown) are combined, this happens at CL2, and does *not* include CL2. CL2 is where the two paths see the same data.

So, the LoUIS includes caches in level 0 and level 1.

However, CL1 _is_ shared between CPU0-3 - it is part of the inner sharable domain. If you invalidate CL1, then you're destroying data held there by the other CPUs. So, invalidating down to the LoUIS on secondary CPU bringup is _wrong_.

+ Lorenzo,

Quoted 25 lines · dinguyen at altera.com

From: Dinh Nguyen <[email protected]>

mach-socfpga is another platform that needs to use v7_invalidate_l1 to bringup additional cores. There was a comment that the ideal place for v7_invalidate_l1 should be in arm/mm/cache-v7.S

Signed-off-by Dinh Nguyen <[email protected]>

Acked-by Simon Horman <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Russell King <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Stephen Warren <[email protected]>

---
  arch/arm/mach-imx/headsmp.S      |   47 -------------------------------------
  arch/arm/mach-shmobile/headsmp.S |   48 --------------------------------------
  arch/arm/mach-tegra/headsmp.S    |   43 ----------------------------------
  arch/arm/mm/cache-v7.S           |   46 ++++++++++++++++++++++++++++++++++++
  4 files changed, 46 insertions(+), 138 deletions(-)

[..]

Quoted 11 lines

arch/arm/mm/cache-v7.S +3 -0

index 7539ec2..15451ee 100644
@@ -19,6 +19,52 @@
  #include "proc-macros.S"
 
  /*
+ * The secondary kernel init calls v7_flush_dcache_all before it enables
+ * the L1; however, the L1 comes out of reset in an undefined state, so
+ * the clean + invalidate performed by v7_flush_dcache_all causes a bunch

arch/arm/mm/cache-v7.S line 34

Quoted 10 lines

arch/arm/mm/cache-v7.S +10 -0

+ * of cache lines with uninitialized data and uninitialized tags to get
+ * written out to memory, which does really unpleasant things to the main
+ * processor.  We fix this by performing an invalidate, rather than a
+ * clean + invalidate, before jumping into the kernel.
+ *
+ * This function is cloned from arch/arm/mach-tegra/headsmp.S, and needs
+ * to be called for both secondary cores startup and primary core resume
+ * procedures.
+ */
+ENTRY(v7_invalidate_l1)

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Lorenzo,
Does that make sense ?

Quoted 55 lines · Santosh Shilimkar

+ Lorenzo,

Quoted 25 lines · dinguyen at altera.com

From: Dinh Nguyen <[email protected]>

mach-socfpga is another platform that needs to use v7_invalidate_l1 to bringup additional cores. There was a comment that the ideal place for v7_invalidate_l1 should be in arm/mm/cache-v7.S

Signed-off-by Dinh Nguyen <[email protected]>

Acked-by Simon Horman <[email protected]>

Tested-by Pavel Machek <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Russell King <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Magnus Damm <[email protected]>

Cc Stephen Warren <[email protected]>

---
   arch/arm/mach-imx/headsmp.S      |   47 -------------------------------------
   arch/arm/mach-shmobile/headsmp.S |   48 --------------------------------------
   arch/arm/mach-tegra/headsmp.S    |   43 ----------------------------------
   arch/arm/mm/cache-v7.S           |   46 ++++++++++++++++++++++++++++++++++++
   4 files changed, 46 insertions(+), 138 deletions(-)

[..]

Quoted 11 lines

arch/arm/mm/cache-v7.S +3 -0

index 7539ec2..15451ee 100644
@@ -19,6 +19,52 @@
   #include "proc-macros.S"
 
   /*
+ * The secondary kernel init calls v7_flush_dcache_all before it enables
+ * the L1; however, the L1 comes out of reset in an undefined state, so
+ * the clean + invalidate performed by v7_flush_dcache_all causes a bunch

arch/arm/mm/cache-v7.S line 34

Quoted 10 lines

arch/arm/mm/cache-v7.S +10 -0

+ * of cache lines with uninitialized data and uninitialized tags to get
+ * written out to memory, which does really unpleasant things to the main
+ * processor.  We fix this by performing an invalidate, rather than a
+ * clean + invalidate, before jumping into the kernel.
+ *
+ * This function is cloned from arch/arm/mach-tegra/headsmp.S, and needs
+ * to be called for both secondary cores startup and primary core resume
+ * procedures.
+ */
+ENTRY(v7_invalidate_l1)

Now since we are moving the code under common place, probably we should update this a function a bit so that it invalidates the CPU cache till line of unification. Just to be consistent with other flush API.

Quoted 2 lines

Lorenzo,
Does that make sense ?

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

Is it worth adding a v7 cache function where the only difference wrt the clean and invalidate operation is a coprocessor opcode ? Probably not IMHO, why add the set/way loop again ?

It is never called from C code, so I do not think there is a point in adding a C API either.

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

Quoted 7 lines · Lorenzo Pieralisi

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

No. Think about it. c7, c6, 2 _invalidates_ the cache. That means any data contained within the cache is discarded. Data is not written back.

If you do this down to the LoUIS, that includes all cache levels in the inner sharable domain. The inner sharable domain includes the other CPUs in the system which may already be running (certainly the boot CPU will be running).

Are you _really_ sure you want to be invalidating _valid_ data held in caches in the inner sharable domain by other CPUs, rather than just the cache associated with the CPU which is being brought online?

Quoted 12 lines · Russell King - ARM Linux

Quoted 7 lines · Lorenzo Pieralisi

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

No. Think about it. c7, c6, 2 _invalidates_ the cache. That means any data contained within the cache is discarded. Data is not written back.

Quoted 4 lines

If you do this down to the LoUIS, that includes all cache levels in the inner sharable domain. The inner sharable domain includes the other CPUs in the system which may already be running (certainly the boot CPU will be running).

On all v7 ARM systems I know of LoUIS correspond to L1 and using LoUIS is a plaster that works for current systems (where by LoUIS I mean all cache levels down to but not inclusive of, LoUIS).

Quoted 13 lines · Lorenzo Pieralisi

Quoted 12 lines · Russell King - ARM Linux

Quoted 7 lines · Lorenzo Pieralisi

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

No. Think about it. c7, c6, 2 _invalidates_ the cache. That means any data contained within the cache is discarded. Data is not written back.

Quoted 8 lines

Quoted 4 lines

If you do this down to the LoUIS, that includes all cache levels in the inner sharable domain. The inner sharable domain includes the other CPUs in the system which may already be running (certainly the boot CPU will be running).

On all v7 ARM systems I know of LoUIS correspond to L1 and using LoUIS is a plaster that works for current systems (where by LoUIS I mean all cache levels down to but not inclusive of, LoUIS).

All that I'm saying is that suggesting that v7_invalidate_l1 should go down to LoUIS is wrong, in much the same way as _invalidating_ only the first level of cache.

However, invalidating the first level of cache only is safer than invalidating down to LoUIS.

The only path which needs this is the secondary CPU bringup path; that's the only path we have where some platforms give us a CPU which has only just come out of reset, and so the caches for that CPU may be in an unknown state. And it only happens to a small subset of platforms.

Currently, that small subset of platforms only need the first level of cache invalidating. (Most platforms don't need this; most platforms this would be a waste of time - and people seem to care about hotplug times.)

So, that's what we should do. And v7_invalidate_l1 is a perfectly reasonable name for a function to do that for the V7 architecture.

As has been pointed out, there's several duplications of that. That's fine, let's move them into the v7 cache code. But... let's not change how they work and go through a pointless design exercise for invalidating more levels of cache when we know that no platform needs it.

If/when we do end up with a platform which requires it, we can deal with it then. But let's not lead people down the route of thinking that LoUIS is suitable here when it isn't.

Just to further provide some insight into the reasoning:

Invalidating data out of a working cache risks data corruption; maybe the data being invalidated is filesystem metadata which was about to be cleaned and written back to storage. That risks filesystem corruption.

Invalidating fewer levels than are actually required is different: we may leave dirty cache lines behind which may be evicted, but there's also the chance that the CPU will end up _reading_ from its uninitialized caches and may crash before that happens.

So, the risks are:
1. invalidate more levels than are necessary and risk discarding data which other CPUs are using, which may be important data.
2. invalidate less levels than are necessary and risk writing out data from the CPU cache, which may or may not happen _before_ the CPU crashes due to reading invalid data.

Out of those two, (2) sounds to me to be the safer approach.

Plus, I can't think of a reason why you'd want to put on a SMP system more than one layer of CPU local caches... to do so would seem to me to be an exercise in coherency complexity... So, I suspect that in the real world, we will _never_ see any system which has more than one layer of caches local to the CPU. But we may see a system with a cache architecture similar to the one I drew in my email to Santosh.

Quoted 19 lines · Russell King - ARM Linux

Just to further provide some insight into the reasoning:

Invalidating data out of a working cache risks data corruption; maybe the data being invalidated is filesystem metadata which was about to be cleaned and written back to storage. That risks filesystem corruption.

Invalidating fewer levels than are actually required is different: we may leave dirty cache lines behind which may be evicted, but there's also the chance that the CPU will end up _reading_ from its uninitialized caches and may crash before that happens.

So, the risks are:
1. invalidate more levels than are necessary and risk discarding data which other CPUs are using, which may be important data.
2. invalidate less levels than are necessary and risk writing out data from the CPU cache, which may or may not happen _before_ the CPU crashes due to reading invalid data.

Quoted 9 lines

Out of those two, (2) sounds to me to be the safer approach.

Plus, I can't think of a reason why you'd want to put on a SMP system more than one layer of CPU local caches... to do so would seem to me to be an exercise in coherency complexity... So, I suspect that in the real world, we will _never_ see any system which has more than one layer of caches local to the CPU. But we may see a system with a cache architecture similar to the one I drew in my email to Santosh.

I still scratching my head on why you would even have a CPU design with two L2 shared caches for a 4 CPU system.

If you ever design such a system, you need to ensure that

1. Both L2 are used in exclusive mode
2. Both L2 cache has coherency hardware connected to keep them in sync for shared data.

For 1, one would just increase the size of L2 and have only 1 memory.

2 Doesn't bring much advantage unless and until your L3 is too far away for access in terms of CPU access cycles.

Quoted 40 lines · Santosh Shilimkar

Quoted 19 lines · Russell King - ARM Linux

Just to further provide some insight into the reasoning:

Invalidating data out of a working cache risks data corruption; maybe the data being invalidated is filesystem metadata which was about to be cleaned and written back to storage. That risks filesystem corruption.

Invalidating fewer levels than are actually required is different: we may leave dirty cache lines behind which may be evicted, but there's also the chance that the CPU will end up _reading_ from its uninitialized caches and may crash before that happens.

So, the risks are:
1. invalidate more levels than are necessary and risk discarding data which other CPUs are using, which may be important data.
2. invalidate less levels than are necessary and risk writing out data from the CPU cache, which may or may not happen _before_ the CPU crashes due to reading invalid data.

Quoted 9 lines

Out of those two, (2) sounds to me to be the safer approach.

Plus, I can't think of a reason why you'd want to put on a SMP system more than one layer of CPU local caches... to do so would seem to me to be an exercise in coherency complexity... So, I suspect that in the real world, we will _never_ see any system which has more than one layer of caches local to the CPU. But we may see a system with a cache architecture similar to the one I drew in my email to Santosh.

I still scratching my head on why you would even have a CPU design with two L2 shared caches for a 4 CPU system.

If you ever design such a system, you need to ensure that

1. Both L2 are used in exclusive mode
2. Both L2 cache has coherency hardware connected to keep them in sync for shared data.

For 1, one would just increase the size of L2 and have only 1 memory.

Quoted 2 lines

2 Doesn't bring much advantage unless and until your L3 is too far away for access in terms of CPU access cycles.

I don't think you quite understood my diagram. There aren't two separate L2 data caches (CL1I and CL1D). I'm showing the L2 cache as having a harvard structure (separate instruction and data) with no coherency between them - and because they're harvard structured, that means the unification level must be _below_ that point.

Quoted 13 lines · Lorenzo Pieralisi

Quoted 12 lines · Russell King - ARM Linux

Quoted 7 lines · Lorenzo Pieralisi

Well, on latest processors (A15, A7) caches are invalidated on reset unless the chip is programmed to skip that on reset (ie L2 retained).

But it makes sense, for sure it should not be called v7_invalidate_l1, but invalidate_louis, and instead of forcing level 0 we should be reading LoUIS and invalidate up to that level as we do in the clean and invalidate function.

No. Think about it. c7, c6, 2 _invalidates_ the cache. That means any data contained within the cache is discarded. Data is not written back.

Quoted 8 lines

Quoted 4 lines

If you do this down to the LoUIS, that includes all cache levels in the inner sharable domain. The inner sharable domain includes the other CPUs in the system which may already be running (certainly the boot CPU will be running).

On all v7 ARM systems I know of LoUIS correspond to L1 and using LoUIS is a plaster that works for current systems (where by LoUIS I mean all cache levels down to but not inclusive of, LoUIS).

All that I'm saying is that suggesting that v7_invalidate_l1 should go down to LoUIS is wrong, in much the same way as _invalidating_ only the first level of cache.

However, invalidating the first level of cache only is safer than invalidating down to LoUIS.

The only path which needs this is the secondary CPU bringup path; that's the only path we have where some platforms give us a CPU which has only just come out of reset, and so the caches for that CPU may be in an unknown state. And it only happens to a small subset of platforms.

Currently, that small subset of platforms only need the first level of cache invalidating. (Most platforms don't need this; most platforms this would be a waste of time - and people seem to care about hotplug times.)

So, that's what we should do. And v7_invalidate_l1 is a perfectly reasonable name for a function to do that for the V7 architecture.

As has been pointed out, there's several duplications of that. That's fine, let's move them into the v7 cache code. But... let's not change how they work and go through a pointless design exercise for invalidating more levels of cache when we know that no platform needs it.

If/when we do end up with a platform which requires it, we can deal with it then. But let's not lead people down the route of thinking that LoUIS is suitable here when it isn't.

From: Dinh Nguyen <[email protected]>

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

Signed-off-by Dinh Nguyen <[email protected]>

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Pavel Machek <[email protected]>

---
 .../bindings/arm/altera/socfpga-system.txt         |    2 ++
 arch/arm/boot/dts/socfpga_cyclone5.dts             |    4 ++++
 arch/arm/boot/dts/socfpga_vt.dts                   |    4 ++++
 arch/arm/mach-socfpga/core.h                       |    4 +++-
 arch/arm/mach-socfpga/headsmp.S                    |   16 ++++++++++++----
 arch/arm/mach-socfpga/platsmp.c                    |    3 ++-
 arch/arm/mach-socfpga/socfpga.c                    |    8 ++++++++
 7 files changed, 35 insertions(+), 6 deletions(-)

arch/arm/mach-socfpga/socfpga.c +0 -0

@@ -72,6 +73,13 @@ void __init socfpga_sysmgr_init(void)
 	struct device_node *np;

Quoted 6 lines

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

arch/arm/mach-socfpga/socfpga.c +0 -0

@@ -72,6 +73,13 @@ void __init socfpga_sysmgr_init(void)
 	struct device_node *np;
 

arch/arm/mach-socfpga/socfpga.c void __init socfpga_sysmgr_init(void) · line 83

Quoted 9 lines

arch/arm/mach-socfpga/socfpga.c +7 -0

 	np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
+
+	if (of_property_read_u32(np, "cpu1-start-addr",
+			(u32 *) &cpu1start_addr)) {
+		early_printk("Need cpu1-start-addr in device tree.\n");
+		panic("Need cpu1-start-addr in device tree.\n");
+	}
+
 	sys_manager_base_addr = of_iomap(np, 0);

Wouldn't it be easier to diagnose this failure if you just printed the error and continued booting without the second CPU? An early panic is usually really hard to debug since you might not get early console without extra work.

Hi!

Quoted 6 lines

Quoted 6 lines

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

arch/arm/mach-socfpga/socfpga.c +0 -0

@@ -72,6 +73,13 @@ void __init socfpga_sysmgr_init(void)
  struct device_node *np;
 

Quoted 13 lines

arch/arm/mach-socfpga/socfpga.c void __init socfpga_sysmgr_init(void) · line 83

Quoted 9 lines

arch/arm/mach-socfpga/socfpga.c +7 -0

  np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
+
+ if (of_property_read_u32(np, "cpu1-start-addr",
+                 (u32 *) &cpu1start_addr)) {
+         early_printk("Need cpu1-start-addr in device tree.\n");
+         panic("Need cpu1-start-addr in device tree.\n");
+ }
+
  sys_manager_base_addr = of_iomap(np, 0);

Wouldn't it be easier to diagnose this failure if you just printed the error and continued booting without the second CPU? An early panic is usually really hard to debug since you might not get early console without extra work.

I actually thought about that... but could not think of non-ugly way of doing that. I hope dts will normally be "right" for any production system...

Pavel

Hi Olof,

Quoted 22 lines · ZY - pavel

Hi!

Quoted 6 lines

Quoted 6 lines

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

arch/arm/mach-socfpga/socfpga.c +0 -0

@@ -72,6 +73,13 @@ void __init socfpga_sysmgr_init(void)
  struct device_node *np;
 

Quoted 13 lines

arch/arm/mach-socfpga/socfpga.c void __init socfpga_sysmgr_init(void) · line 83

Quoted 9 lines

arch/arm/mach-socfpga/socfpga.c +7 -0

  np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
+
+ if (of_property_read_u32(np, "cpu1-start-addr",
+                 (u32 *) &cpu1start_addr)) {
+         early_printk("Need cpu1-start-addr in device tree.\n");
+         panic("Need cpu1-start-addr in device tree.\n");
+ }
+
  sys_manager_base_addr = of_iomap(np, 0);

Wouldn't it be easier to diagnose this failure if you just printed the error and continued booting without the second CPU? An early panic is usually really hard to debug since you might not get early console without extra work.

Quoted 3 lines

I actually thought about that... but could not think of non-ugly way of doing that. I hope dts will normally be "right" for any production system...

I think a panic is better just for the reason that if someone is expecting SMP, but missed the warning message, and later finds out that the secondary core never came up, it would save some debugging time.

Since I have to send out a v3 from the 1st patch anyways, let me verify that I can get the early warning.

Quoted 28 lines · Dinh Nguyen

Hi Olof,

Quoted 22 lines · ZY - pavel

Hi!

Quoted 6 lines

Quoted 6 lines

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

arch/arm/mach-socfpga/socfpga.c +0 -0

@@ -72,6 +73,13 @@ void __init socfpga_sysmgr_init(void)
  struct device_node *np;
 

Quoted 13 lines

arch/arm/mach-socfpga/socfpga.c void __init socfpga_sysmgr_init(void) · line 83

Quoted 9 lines

arch/arm/mach-socfpga/socfpga.c +7 -0

  np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
+
+ if (of_property_read_u32(np, "cpu1-start-addr",
+                 (u32 *) &cpu1start_addr)) {
+         early_printk("Need cpu1-start-addr in device tree.\n");
+         panic("Need cpu1-start-addr in device tree.\n");
+ }
+
  sys_manager_base_addr = of_iomap(np, 0);

Wouldn't it be easier to diagnose this failure if you just printed the error and continued booting without the second CPU? An early panic is usually really hard to debug since you might not get early console without extra work.

Quoted 3 lines

I actually thought about that... but could not think of non-ugly way of doing that. I hope dts will normally be "right" for any production system...

Quoted 6 lines

I think a panic is better just for the reason that if someone is expecting SMP, but missed the warning message, and later finds out that the secondary core never came up, it would save some debugging time.

Since I have to send out a v3 from the 1st patch anyways, let me verify that I can get the early warning.

The choice is between a panic() at a point where the only way to find out is to throw in printascii() or a working printk, and ending up with an unbootable kernel, vs continuing the boot and having an almost working system which can be logged into and the messages viewed.

If you have an application which relies on the second CPU coming up, why not have it verify that the second CPU came up (it's quite easy to do - there's POSIX standard libc calls to get the number of online CPUs).

Quoted 16 lines

Quoted 10 lines

Quoted 3 lines

I actually thought about that... but could not think of non-ugly way of doing that. I hope dts will normally be "right" for any production system...

I think a panic is better just for the reason that if someone is expecting SMP, but missed the warning message, and later finds out that the secondary core never came up, it would save some debugging time.

Since I have to send out a v3 from the 1st patch anyways, let me verify that I can get the early warning.

The choice is between a panic() at a point where the only way to find out is to throw in printascii() or a working printk, and ending up with an unbootable kernel, vs continuing the boot and having an almost working system which can be logged into and the messages viewed.

Quoted 4 lines

If you have an application which relies on the second CPU coming up, why not have it verify that the second CPU came up (it's quite easy to do - there's POSIX standard libc calls to get the number of online CPUs).

Point taken...thanks Russell.

-----Original Message-----
From: ZY - pavel
Sent: Saturday, February 02, 2013 1:24 PM
To: Dinh Nguyen
Cc: Russell King - ARM Linux; olof at lixom.net; linux-arm- kernel at lists.infradead.org; arnd at arndb.de
Subject: Re: [PATCHv2 for soc 4/4] arm: socfpga: Add SMP support for actual socfpga harware

Hi!

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

I actually thought about that... but could not think of non-

ugly way

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

of doing that. I hope dts will normally be "right" for any

production

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

system...

I think a panic is better just for the reason that if someone is expecting SMP, but missed the warning message, and later finds

out that

Quoted 1 line

Quoted 1 line

Quoted 1 line

the secondary core never came up, it would save some debugging

time.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Since I have to send out a v3 from the 1st patch anyways, let me

verify

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that I can get the early warning.

The choice is between a panic() at a point where the only way to

find

Quoted 1 line

Quoted 1 line

out is to throw in printascii() or a working printk, and ending up

with

Quoted 4 lines

Quoted 4 lines

an unbootable kernel, vs continuing the boot and having an almost working system which can be logged into and the messages viewed.

If you have an application which relies on the second CPU coming

up,

Quoted 2 lines

Quoted 2 lines

why not have it verify that the second CPU came up (it's quite easy to do - there's POSIX standard libc calls to get the number of

online

Quoted 3 lines

Quoted 1 line

CPUs).

Point taken...thanks Russell.

Well, I don't think we normally check dtbs for validity with user-helpful error messages, but it is relatively easy in this case.

---cut here---

Continue booting with second core disabled if cpu1-start-addr is not present in .dtb.

Signed-off-by Pavel Machek <[email protected]>

arch/arm/mach- renamed from arch/arm/mach-socfpga/platsmp.c +0 -0

socfpga/platsmp.c
index 81e0da0..90facdd 100644

arch/arm/mach-socfpga/platsmp.c +0 -0

      for (i = 0; i < ncores; i++)
              set_cpu_possible(i, true);
 

arch/arm/mach- renamed from arch/arm/mach-socfpga/socfpga.c +0 -0

socfpga/socfpga.c
index 334c330..c3cd88b 100644

arch/arm/mach-socfpga/socfpga.c +3 -4

@@ -74,10 +74,9 @@ static void __init socfpga_sysmgr_init(void)
 
      np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
 
-     if (of_property_read_u32(np, "cpu1-start-addr", (u32 *)
 &cpu1start_addr)) {
-             early_printk("Need cpu1-start-addr in device tree.\n");
-             panic("Need cpu1-start-addr in device tree.\n");
-     }
+     if (of_property_read_u32(np, "cpu1-start-addr", (u32 *)
 &cpu1start_addr))
+             printk(KERN_ALERT "Need cpu1-start-addr in device tree for
 SMP operation.\n");
+

      sys_manager_base_addr = of_iomap(np, 0);

      np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Quoted 12 lines

Quoted 1 line

Point taken...thanks Russell.

Well, I don't think we normally check dtbs for validity with user-helpful error messages, but it is relatively easy in this case.

---cut here---

Continue booting with second core disabled if cpu1-start-addr is not present in .dtb.

Signed-off-by Pavel Machek <[email protected]>

arch/arm/mach-socfpga/platsmp.c static void __init socfpga_smp_init_cpus(void) · line 87

Quoted 12 lines

arch/arm/mach- renamed from arch/arm/mach-socfpga/platsmp.c +0 -0

socfpga/platsmp.c
index 81e0da0..90facdd 100644

arch/arm/mach-socfpga/platsmp.c +3 -0

@@ -82,6 +82,9 @@ static void __init socfpga_smp_init_cpus(void)
 		ncores = 1;
 	}
 #endif
+	if (!cpu1start_addr)
+		ncores = 1;
+

This will not work because of commit 5587164eea4aad88fcb79d9b21dc8f14fea598cd I sent out a V3 series of this patch, CPU1 will simply fail to come online if cpu1-start-addr is not defined.

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

Point taken...thanks Russell.

Well, I don't think we normally check dtbs for validity with user-helpful error messages, but it is relatively easy in this case.

---cut here---

Continue booting with second core disabled if cpu1-start-addr is not present in .dtb.

Signed-off-by Pavel Machek <[email protected]>

Quoted 16 lines

arch/arm/mach-socfpga/platsmp.c static void __init socfpga_smp_init_cpus(void) · line 87

Quoted 12 lines

arch/arm/mach- renamed from arch/arm/mach-socfpga/platsmp.c +0 -0

socfpga/platsmp.c
index 81e0da0..90facdd 100644

arch/arm/mach-socfpga/platsmp.c +3 -0

@@ -82,6 +82,9 @@ static void __init socfpga_smp_init_cpus(void)
 		ncores = 1;
 	}
 #endif
+	if (!cpu1start_addr)
+		ncores = 1;
+

This will not work because of commit 5587164eea4aad88fcb79d9b21dc8f14fea598cd I sent out a V3 series of this patch, CPU1 will simply fail to come online if cpu1-start-addr is not defined.

Are you sure? As far as I can see you still need such a line in v3 of the patch:

arch/arm/mach-socfpga/socfpga.c +5 -0

@@ -72,6 +73,11 @@ void __init socfpga_sysmgr_init(void)
        struct device_node *np;
 
        np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
+       
+       if (of_property_read_u32(np, "cpu1-start-addr",
+                       (u32 *) &cpu1start_addr))
+               pr_err("SMP: Need cpu1-start-addr in device tree.\n");
+
        sys_manager_base_addr = of_iomap(np, 0);
 
        np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");

...so cpu1-start-addr is not there, cpu1start_addr is NULL but you continue booting.

ENTRY(secondary_trampoline)

(quoted diff) +4 -2

-       movw    r0, #:lower16:CPU1_START_ADDR
-       movt  r0, #:upper16:CPU1_START_ADDR
+       movw    r2, #:lower16:cpu1start_addr
+       movt  r2, #:upper16:cpu1start_addr
+                       
+       /* The socfpga VT cannot handle a 0xC0000000 page offset when

loading
+ the cpu1start_addr, we bit clear it. Tested on HW and
VT. */

(quoted diff) +2 -0

+       bic     r2, r2, #0x40000000
 
+       ldr     r0, [r2]
        ldr     r1, [r0]
        bx      r1

...and you'll dereference the NULL pointer here, no?

Sorry for the noise, this really is not all that important...
Pavel

From: Dinh Nguyen <[email protected]>

Because the CPU1 start address is different for socfpga-vt and socfpga-cyclone5, we add code to use the correct CPU1 start addr.

Signed-off-by Dinh Nguyen <[email protected]>

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Pavel Machek <[email protected]>