[PATCH u-boot-mvebu 0/3] arm: mvebu: a37xx: Fix and extend building memory map
Fix calling build_mem_map() function and extend it to map also CCI-400 and AP BootROM address space.
With this change it is possible to access A53 AP BootROM on Armada 3720 from U-Boot and e.g. dump it via U-Boot md command:
=> md fff00000 1000
Pali Rohár (3): arm: mvebu: a37xx: Fix calling build_mem_map() arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space arm: mvebu: a37xx: Fix comment with name of the function
arch/arm/mach-mvebu/armada3700/cpu.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
Function build_mem_map() modifies global variable mem_map. This variable is used by the get_page_table_size() function which is called by function arm_reserve_mmu() (as aliased macro PGTABLE_SIZE). Function arm_reserve_mmu() is called earlier than enable_caches() which calls build_mem_map(). So arm_reserve_mmu() does not calculate reserved memory correctly.
Fix this issue by calling build_mem_map() from a3700_dram_init() which is called before arm_reserve_mmu().
Signed-off-by: Pali Rohár pali@kernel.org --- arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index bdf8dc377528..9da0d08f947c 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -136,8 +136,6 @@ static void build_mem_map(void)
void enable_caches(void) { - build_mem_map(); - icache_enable(); dcache_enable(); } @@ -146,6 +144,8 @@ int a3700_dram_init(void) { int win;
+ build_mem_map(); + gd->ram_size = 0; for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size;
On 2/15/22 00:28, Pali Rohár wrote:
Function build_mem_map() modifies global variable mem_map. This variable is used by the get_page_table_size() function which is called by function arm_reserve_mmu() (as aliased macro PGTABLE_SIZE). Function arm_reserve_mmu() is called earlier than enable_caches() which calls build_mem_map(). So arm_reserve_mmu() does not calculate reserved memory correctly.
Fix this issue by calling build_mem_map() from a3700_dram_init() which is called before arm_reserve_mmu().
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index bdf8dc377528..9da0d08f947c 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -136,8 +136,6 @@ static void build_mem_map(void)
void enable_caches(void) {
- build_mem_map();
- icache_enable(); dcache_enable(); }
@@ -146,6 +144,8 @@ int a3700_dram_init(void) { int win;
- build_mem_map();
- gd->ram_size = 0; for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size;
Viele Grüße, Stefan Roese
On Tue, 15 Feb 2022 00:28:33 +0100 Pali Rohár pali@kernel.org wrote:
Function build_mem_map() modifies global variable mem_map. This variable is used by the get_page_table_size() function which is called by function arm_reserve_mmu() (as aliased macro PGTABLE_SIZE). Function arm_reserve_mmu() is called earlier than enable_caches() which calls build_mem_map(). So arm_reserve_mmu() does not calculate reserved memory correctly.
Fix this issue by calling build_mem_map() from a3700_dram_init() which is called before arm_reserve_mmu().
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Marek Behún marek.behun@nic.cz
In function build_mem_map() prepares also mapping for CCI-400 and AP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long. CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
Signed-off-by: Pali Rohár pali@kernel.org --- arch/arm/mach-mvebu/armada3700/cpu.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 9da0d08f947c..e01fea130022 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -40,8 +40,10 @@ #define MVEBU_CPU_DEC_WIN_REMAP(w) (MVEBU_CPU_DEC_WIN_CTRL(w) + 0xc) #define MVEBU_CPU_DEC_WIN_GRANULARITY 16 #define MVEBU_CPU_DEC_WINS 5 +#define MVEBU_CPU_DEC_CCI_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xe0) +#define MVEBU_CPU_DEC_ROM_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xf4)
-#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 2) +#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 4)
#define A3700_PTE_BLOCK_NORMAL \ (PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE) @@ -104,8 +106,26 @@ static int get_cpu_dec_win(int win, u32 *tgt, u32 *base, u32 *size) static void build_mem_map(void) { int win, region; + u32 reg;
region = 1; + + /* CCI-400 */ + reg = readl(MVEBU_CPU_DEC_CCI_BASE); + mvebu_mem_map[region].phys = reg << 20; + mvebu_mem_map[region].virt = reg << 20; + mvebu_mem_map[region].size = 0x10000; + mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_DEVICE; + ++region; + + /* AP BootROM */ + reg = readl(MVEBU_CPU_DEC_ROM_BASE); + mvebu_mem_map[region].phys = reg << 20; + mvebu_mem_map[region].virt = reg << 20; + mvebu_mem_map[region].size = 0x4000; + mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_NORMAL; + ++region; + for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size; u64 attrs;
On Tue, 15 Feb 2022 00:28:34 +0100 Pali Rohár pali@kernel.org wrote:
In function build_mem_map() prepares also mapping for CCI-400 and
* prepare
AP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
As we talked about in private conversation, I still don't think we should do this unless it is needed.
CCI may be needed to be mapped if ever there is some driver that needs to interact with it.
BootROM is never needed by the U-Boot code.
I really don't think that we should map these in production U-Boot binaries for everyone, when the intention is "for debugging purposes only". In the last 4 years there were 2 people (me, and you :)) who were interested in BootROM. In the next 10 years there will be maybe 2 more. So I really don't think the windows should be mapped for everyone.
Maybe you can map them if some debug option is enabled in menuconfig?
Marek
On Tuesday 15 February 2022 13:11:25 Marek Behún wrote:
On Tue, 15 Feb 2022 00:28:34 +0100 Pali Rohár pali@kernel.org wrote:
In function build_mem_map() prepares also mapping for CCI-400 and
* prepareAP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
As we talked about in private conversation, I still don't think we should do this unless it is needed.
CCI may be needed to be mapped if ever there is some driver that needs to interact with it.
BootROM is never needed by the U-Boot code.
I really don't think that we should map these in production U-Boot binaries for everyone, when the intention is "for debugging purposes only". In the last 4 years there were 2 people (me, and you :)) who were interested in BootROM. In the next 10 years there will be maybe 2 more. So I really don't think the windows should be mapped for everyone.
I looked at this stuff because other people asked me about possibility to dump BootROM. So it is not "only me".
Anyway, the main issue with all this stuff is that there is no public documentation for it and things which are missing in U-Boot would be missing forever (because there are only few people with access to documentation; which is even more incomplete, inconsistent and incorrect). So adding this stuff may help other people from community who would like to study this platform or code.
Note that, by default U-Boot has already enabled 'md', 'mw', 'pci' and so other commands which are intended for debug purposes only, so I do not see reason why _in this version_ cannot be mapped also BootROM code.
In _production version_ where is no debug capability and no access to any memory (just ability to boot) is is probably not needed, but none of A3720 board is building this kind of version (by default). And in case BootROM is mapped also in these versions, is there any issue with it? BootROM is read-only, same in all A3720 SoCs, so by this definition it is public and everybody can inspect it...
Stefan, what do you think about it?
Maybe you can map them if some debug option is enabled in menuconfig?
Marek
On Tue, 15 Feb 2022 14:04:47 +0100 Pali Rohár pali@kernel.org wrote:
On Tuesday 15 February 2022 13:11:25 Marek Behún wrote:
On Tue, 15 Feb 2022 00:28:34 +0100 Pali Rohár pali@kernel.org wrote:
In function build_mem_map() prepares also mapping for CCI-400 and
* prepareAP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
As we talked about in private conversation, I still don't think we should do this unless it is needed.
CCI may be needed to be mapped if ever there is some driver that needs to interact with it.
BootROM is never needed by the U-Boot code.
I really don't think that we should map these in production U-Boot binaries for everyone, when the intention is "for debugging purposes only". In the last 4 years there were 2 people (me, and you :)) who were interested in BootROM. In the next 10 years there will be maybe 2 more. So I really don't think the windows should be mapped for everyone.
I looked at this stuff because other people asked me about possibility to dump BootROM. So it is not "only me".
Even then, they can just enable the mapping once, dump the code and publish it somewhere. No need to keep it enabled for everyone.
Anyway, the main issue with all this stuff is that there is no public documentation for it and things which are missing in U-Boot would be missing forever (because there are only few people with access to documentation; which is even more incomplete, inconsistent and incorrect). So adding this stuff may help other people from community who would like to study this platform or code.
As I am saying above, the BootROM code needs to be dumped only once, if someone wants it. No need to always keep the mapping. The code does not change.
Note that, by default U-Boot has already enabled 'md', 'mw', 'pci' and so other commands which are intended for debug purposes only, so I do not see reason why _in this version_ cannot be mapped also BootROM code.
mw/cmp is not for debugging only. md may be, but mw can reasonably be used from boot scripts.
In _production version_ where is no debug capability and no access to any memory (just ability to boot) is is probably not needed, but none of A3720 board is building this kind of version (by default). And in case BootROM is mapped also in these versions, is there any issue with it?
My issue is that it isn't needed. You can just dump it once, publish it somewhere, and you are done. No need to keep that window mapped for everyone else.
BootROM is read-only, same in all A3720 SoCs, so by this definition it is public and everybody can inspect it...
Yes, I also consider the BootROM blob public, since everyone can access it if they know how to, and the necessary information for that is available in TF-A.
Stefan, what do you think about it?
Maybe you can map them if some debug option is enabled in menuconfig?
Marek
On Tue, 15 Feb 2022 14:15:17 +0100 Marek Behún marek.behun@nic.cz wrote:
In _production version_ where is no debug capability and no access to any memory (just ability to boot) is is probably not needed, but none of A3720 board is building this kind of version (by default). And in case BootROM is mapped also in these versions, is there any issue with it?
My issue is that it isn't needed. You can just dump it once, publish it somewhere, and you are done. No need to keep that window mapped for everyone else.
BTW, I can imagine situation where mapping BootROM code can be useful. For example if the BootROM code contains some cryptographic functions (which are necessary for secure boot) and you know where they are and their type, so you can use them if you want to save space in your own code.
But mapping BootROM so that everyone, if they want, can dump it, is unnecessary IMO, because you can simply do it once and then you have the code.
Keep in mind that this is my opinion. If Stefan agrees with you, I have no issue with merging this.
Marek
On 2/15/22 14:04, Pali Rohár wrote:
On Tuesday 15 February 2022 13:11:25 Marek Behún wrote:
On Tue, 15 Feb 2022 00:28:34 +0100 Pali Rohár pali@kernel.org wrote:
In function build_mem_map() prepares also mapping for CCI-400 and
* prepareAP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
As we talked about in private conversation, I still don't think we should do this unless it is needed.
CCI may be needed to be mapped if ever there is some driver that needs to interact with it.
BootROM is never needed by the U-Boot code.
I really don't think that we should map these in production U-Boot binaries for everyone, when the intention is "for debugging purposes only". In the last 4 years there were 2 people (me, and you :)) who were interested in BootROM. In the next 10 years there will be maybe 2 more. So I really don't think the windows should be mapped for everyone.
I looked at this stuff because other people asked me about possibility to dump BootROM. So it is not "only me".
Anyway, the main issue with all this stuff is that there is no public documentation for it and things which are missing in U-Boot would be missing forever (because there are only few people with access to documentation; which is even more incomplete, inconsistent and incorrect). So adding this stuff may help other people from community who would like to study this platform or code.
Note that, by default U-Boot has already enabled 'md', 'mw', 'pci' and so other commands which are intended for debug purposes only, so I do not see reason why _in this version_ cannot be mapped also BootROM code.
In _production version_ where is no debug capability and no access to any memory (just ability to boot) is is probably not needed, but none of A3720 board is building this kind of version (by default). And in case BootROM is mapped also in these versions, is there any issue with it? BootROM is read-only, same in all A3720 SoCs, so by this definition it is public and everybody can inspect it...
Stefan, what do you think about it?
Frankly, my first thought was similar to what Marek expressed. Do we really need this? But I also see your point and from the "security" point of view, I don't see that the system get's more insecure by enabling access to these areas. And at least I myself have been happy to have all kind of debugging possibilities enabled in U-Boot per default.
So to sum it up, I'm currently in favor to accepting these changes right now.
Thanks, Stefan
On Wednesday 16 February 2022 09:55:15 Stefan Roese wrote:
On 2/15/22 14:04, Pali Rohár wrote:
On Tuesday 15 February 2022 13:11:25 Marek Behún wrote:
On Tue, 15 Feb 2022 00:28:34 +0100 Pali Rohár pali@kernel.org wrote:
In function build_mem_map() prepares also mapping for CCI-400 and
* prepareAP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long.
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
As we talked about in private conversation, I still don't think we should do this unless it is needed.
CCI may be needed to be mapped if ever there is some driver that needs to interact with it.
BootROM is never needed by the U-Boot code.
I really don't think that we should map these in production U-Boot binaries for everyone, when the intention is "for debugging purposes only". In the last 4 years there were 2 people (me, and you :)) who were interested in BootROM. In the next 10 years there will be maybe 2 more. So I really don't think the windows should be mapped for everyone.
I looked at this stuff because other people asked me about possibility to dump BootROM. So it is not "only me".
Anyway, the main issue with all this stuff is that there is no public documentation for it and things which are missing in U-Boot would be missing forever (because there are only few people with access to documentation; which is even more incomplete, inconsistent and incorrect). So adding this stuff may help other people from community who would like to study this platform or code.
Note that, by default U-Boot has already enabled 'md', 'mw', 'pci' and so other commands which are intended for debug purposes only, so I do not see reason why _in this version_ cannot be mapped also BootROM code.
In _production version_ where is no debug capability and no access to any memory (just ability to boot) is is probably not needed, but none of A3720 board is building this kind of version (by default). And in case BootROM is mapped also in these versions, is there any issue with it? BootROM is read-only, same in all A3720 SoCs, so by this definition it is public and everybody can inspect it...
Stefan, what do you think about it?
Frankly, my first thought was similar to what Marek expressed. Do we really need this? But I also see your point and from the "security" point of view, I don't see that the system get's more insecure by enabling access to these areas. And at least I myself have been happy to have all kind of debugging possibilities enabled in U-Boot per default.
So to sum it up, I'm currently in favor to accepting these changes right now.
Thanks, Stefan
Ok! So I will fix issue:
RVBAR_EL3 register has value 0xffff0000. The BootROM is 16 KiB long but the window is 1 MiB long, so the content repeats every 4 KiB.
And will send a new patch version.
On 2/15/22 00:28, Pali Rohár wrote:
In function build_mem_map() prepares also mapping for CCI-400 and AP BootROM address space.
A53 AP BootROM by default starts at address 0xfff00000 and is 16 kB long. CCI-400 in new TF-A version starts at address 0xfe000000 and is 64 kB long.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future then it would not cause any issue.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
arch/arm/mach-mvebu/armada3700/cpu.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 9da0d08f947c..e01fea130022 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -40,8 +40,10 @@ #define MVEBU_CPU_DEC_WIN_REMAP(w) (MVEBU_CPU_DEC_WIN_CTRL(w) + 0xc) #define MVEBU_CPU_DEC_WIN_GRANULARITY 16 #define MVEBU_CPU_DEC_WINS 5 +#define MVEBU_CPU_DEC_CCI_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xe0) +#define MVEBU_CPU_DEC_ROM_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xf4)
-#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 2) +#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 4)
#define A3700_PTE_BLOCK_NORMAL \ (PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE) @@ -104,8 +106,26 @@ static int get_cpu_dec_win(int win, u32 *tgt, u32 *base, u32 *size) static void build_mem_map(void) { int win, region;
u32 reg;
region = 1;
/* CCI-400 */
reg = readl(MVEBU_CPU_DEC_CCI_BASE);
mvebu_mem_map[region].phys = reg << 20;
mvebu_mem_map[region].virt = reg << 20;
mvebu_mem_map[region].size = 0x10000;
mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_DEVICE;
++region;
/* AP BootROM */
reg = readl(MVEBU_CPU_DEC_ROM_BASE);
mvebu_mem_map[region].phys = reg << 20;
mvebu_mem_map[region].virt = reg << 20;
mvebu_mem_map[region].size = 0x4000;
mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_NORMAL;
++region;
for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size; u64 attrs;
Viele Grüße, Stefan Roese
Function is named build_mem_map, not a3700_build_mem_map.
Signed-off-by: Pali Rohár pali@kernel.org --- arch/arm/mach-mvebu/armada3700/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index e01fea130022..fd85a2bbef8f 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -56,7 +56,7 @@ static struct mm_region mvebu_mem_map[MAX_MEM_MAP_REGIONS] = { { /* * SRAM, MMIO regions - * Don't remove this, a3700_build_mem_map needs it. + * Don't remove this, build_mem_map needs it. */ .phys = SOC_REGS_PHY_BASE, .virt = SOC_REGS_PHY_BASE,
On Tue, 15 Feb 2022 00:28:35 +0100 Pali Rohár pali@kernel.org wrote:
Function is named build_mem_map, not a3700_build_mem_map.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Marek Behún marek.behun@nic.cz
On 2/15/22 00:28, Pali Rohár wrote:
Function is named build_mem_map, not a3700_build_mem_map.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
arch/arm/mach-mvebu/armada3700/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index e01fea130022..fd85a2bbef8f 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -56,7 +56,7 @@ static struct mm_region mvebu_mem_map[MAX_MEM_MAP_REGIONS] = { { /* * SRAM, MMIO regions
* Don't remove this, a3700_build_mem_map needs it.
* Don't remove this, build_mem_map needs it.
Viele Grüße, Stefan Roese
Fix calling build_mem_map() function and extend it to map also CCI-400 and AP BootROM address space.
With this change it is possible to access A53 AP BootROM on Armada 3720 from U-Boot and e.g. dump it via U-Boot md command:
=> md ffff0000 4000
(Changes in v2: Fixed above dump command)
Pali Rohár (3): arm: mvebu: a37xx: Fix calling build_mem_map() arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space arm: mvebu: a37xx: Fix comment with name of the function
arch/arm/mach-mvebu/armada3700/cpu.c | 29 ++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
Function build_mem_map() modifies global variable mem_map. This variable is used by the get_page_table_size() function which is called by function arm_reserve_mmu() (as aliased macro PGTABLE_SIZE). Function arm_reserve_mmu() is called earlier than enable_caches() which calls build_mem_map(). So arm_reserve_mmu() does not calculate reserved memory correctly.
Fix this issue by calling build_mem_map() from a3700_dram_init() which is called before arm_reserve_mmu().
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Marek Behún marek.behun@nic.cz --- arch/arm/mach-mvebu/armada3700/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 7702028ba19b..57a811b36ac6 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -142,8 +142,6 @@ static void build_mem_map(void)
void enable_caches(void) { - build_mem_map(); - icache_enable(); dcache_enable(); } @@ -152,6 +150,8 @@ int a3700_dram_init(void) { int win;
+ build_mem_map(); + gd->ram_size = 0; for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size;
In function build_mem_map() prepare also mapping for CCI-400 and BootROM windows.
BootROM window is 1 MB long and by default starts at address 0xfff00000. A53 AP BootROM is 16 kB long and repeats in this BootROM window 64 times. RVBAR_EL3 register is set to value 0xffff0000, so by default A53 AP BootROM is accessed via range 0xffff0000-0xffff3fff.
CCI-400 window when new TF-A version is used, starts at address 0xfe000000 and when old TF-A version is used, starts at address 0xd8000000.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future (again) then it would not cause any issue for U-Boot.
Signed-off-by: Pali Rohár pali@kernel.org
--- Changes in v2: * Use SZ_* macros for sizes * Fix size of BootROM window * Fix commit message about 1 MB BootROM window vs 16 kB BootROM code --- arch/arm/mach-mvebu/armada3700/cpu.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 57a811b36ac6..e9bdc181ef02 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -13,6 +13,7 @@ #include <asm/global_data.h> #include <linux/bitops.h> #include <linux/libfdt.h> +#include <linux/sizes.h> #include <asm/io.h> #include <asm/system.h> #include <asm/arch/cpu.h> @@ -46,8 +47,10 @@ #define MVEBU_CPU_DEC_WIN_REMAP(w) (MVEBU_CPU_DEC_WIN_CTRL(w) + 0xc) #define MVEBU_CPU_DEC_WIN_GRANULARITY 16 #define MVEBU_CPU_DEC_WINS 5 +#define MVEBU_CPU_DEC_CCI_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xe0) +#define MVEBU_CPU_DEC_ROM_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xf4)
-#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 2) +#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 4)
#define A3700_PTE_BLOCK_NORMAL \ (PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE) @@ -110,8 +113,26 @@ static int get_cpu_dec_win(int win, u32 *tgt, u32 *base, u32 *size) static void build_mem_map(void) { int win, region; + u32 reg;
region = 1; + + /* CCI-400 */ + reg = readl(MVEBU_CPU_DEC_CCI_BASE); + mvebu_mem_map[region].phys = reg << 20; + mvebu_mem_map[region].virt = reg << 20; + mvebu_mem_map[region].size = SZ_64K; + mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_DEVICE; + ++region; + + /* AP BootROM */ + reg = readl(MVEBU_CPU_DEC_ROM_BASE); + mvebu_mem_map[region].phys = reg << 20; + mvebu_mem_map[region].virt = reg << 20; + mvebu_mem_map[region].size = SZ_1M; + mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_NORMAL; + ++region; + for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size; u64 attrs;
On 2/16/22 11:18, Pali Rohár wrote:
In function build_mem_map() prepare also mapping for CCI-400 and BootROM windows.
BootROM window is 1 MB long and by default starts at address 0xfff00000. A53 AP BootROM is 16 kB long and repeats in this BootROM window 64 times. RVBAR_EL3 register is set to value 0xffff0000, so by default A53 AP BootROM is accessed via range 0xffff0000-0xffff3fff.
CCI-400 window when new TF-A version is used, starts at address 0xfe000000 and when old TF-A version is used, starts at address 0xd8000000.
Physical addresses are read directly from mvebu registers, so if TF-A remaps it in future (again) then it would not cause any issue for U-Boot.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
Changes in v2:
- Use SZ_* macros for sizes
- Fix size of BootROM window
- Fix commit message about 1 MB BootROM window vs 16 kB BootROM code
arch/arm/mach-mvebu/armada3700/cpu.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index 57a811b36ac6..e9bdc181ef02 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -13,6 +13,7 @@ #include <asm/global_data.h> #include <linux/bitops.h> #include <linux/libfdt.h> +#include <linux/sizes.h> #include <asm/io.h> #include <asm/system.h> #include <asm/arch/cpu.h> @@ -46,8 +47,10 @@ #define MVEBU_CPU_DEC_WIN_REMAP(w) (MVEBU_CPU_DEC_WIN_CTRL(w) + 0xc) #define MVEBU_CPU_DEC_WIN_GRANULARITY 16 #define MVEBU_CPU_DEC_WINS 5 +#define MVEBU_CPU_DEC_CCI_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xe0) +#define MVEBU_CPU_DEC_ROM_BASE (MVEBU_CPU_DEC_WIN_REG_BASE + 0xf4)
-#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 2) +#define MAX_MEM_MAP_REGIONS (MVEBU_CPU_DEC_WINS + 4)
#define A3700_PTE_BLOCK_NORMAL \ (PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE) @@ -110,8 +113,26 @@ static int get_cpu_dec_win(int win, u32 *tgt, u32 *base, u32 *size) static void build_mem_map(void) { int win, region;
u32 reg;
region = 1;
/* CCI-400 */
reg = readl(MVEBU_CPU_DEC_CCI_BASE);
mvebu_mem_map[region].phys = reg << 20;
mvebu_mem_map[region].virt = reg << 20;
mvebu_mem_map[region].size = SZ_64K;
mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_DEVICE;
++region;
/* AP BootROM */
reg = readl(MVEBU_CPU_DEC_ROM_BASE);
mvebu_mem_map[region].phys = reg << 20;
mvebu_mem_map[region].virt = reg << 20;
mvebu_mem_map[region].size = SZ_1M;
mvebu_mem_map[region].attrs = A3700_PTE_BLOCK_NORMAL;
++region;
for (win = 0; win < MVEBU_CPU_DEC_WINS; ++win) { u32 base, tgt, size; u64 attrs;
Viele Grüße, Stefan Roese
Function is named build_mem_map, not a3700_build_mem_map.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Marek Behún marek.behun@nic.cz --- arch/arm/mach-mvebu/armada3700/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c b/arch/arm/mach-mvebu/armada3700/cpu.c index e9bdc181ef02..23492f49dae3 100644 --- a/arch/arm/mach-mvebu/armada3700/cpu.c +++ b/arch/arm/mach-mvebu/armada3700/cpu.c @@ -63,7 +63,7 @@ static struct mm_region mvebu_mem_map[MAX_MEM_MAP_REGIONS] = { { /* * SRAM, MMIO regions - * Don't remove this, a3700_build_mem_map needs it. + * Don't remove this, build_mem_map needs it. */ .phys = SOC_REGS_PHY_BASE, .virt = SOC_REGS_PHY_BASE,
On 2/16/22 11:18, Pali Rohár wrote:
Fix calling build_mem_map() function and extend it to map also CCI-400 and AP BootROM address space.
With this change it is possible to access A53 AP BootROM on Armada 3720 from U-Boot and e.g. dump it via U-Boot md command:
=> md ffff0000 4000
(Changes in v2: Fixed above dump command)
Pali Rohár (3): arm: mvebu: a37xx: Fix calling build_mem_map() arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space arm: mvebu: a37xx: Fix comment with name of the function
arch/arm/mach-mvebu/armada3700/cpu.c | 29 ++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
Applied to u-boot-marvell/master
Thanks, Stefan
participants (3)
-
Marek Behún -
Pali Rohár -
Stefan Roese