[PATCH u-boot-mvebu 0/3] arm: mvebu: a37xx: Fix and extend building memory map

[PATCH u-boot-mvebu 0/3] arm: mvebu: a37xx: Fix and extend building memory map

[Pali Rohár]

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(-)

-- 
2.20.1

[PATCH u-boot-mvebu 1/3] arm: mvebu: a37xx: Fix calling build_mem_map()

[Pali Rohár]

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;
-- 
2.20.1

[PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Pali Rohár]

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;
-- 
2.20.1

[PATCH u-boot-mvebu 3/3] arm: mvebu: a37xx: Fix comment with name of the function

[Pali Rohár]

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,
-- 
2.20.1

Re: [PATCH u-boot-mvebu 1/3] arm: mvebu: a37xx: Fix calling build_mem_map()

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Marek Behún]

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

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Pali Rohár]

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
>                             * 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.

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

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Marek Behún]

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  
> >                             * 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.  
> 
> 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  

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: a37xx: Fix comment with name of the function

[Marek Behún]

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>

Re: [PATCH u-boot-mvebu 1/3] arm: mvebu: a37xx: Fix calling build_mem_map()

[Marek Behún]

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>

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Marek Behún]

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

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Stefan Roese]

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
>>                              * 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.
> 
> 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

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu 3/3] arm: mvebu: a37xx: Fix comment with name of the function

[Stefan Roese]

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.
>   		 */
>   		.phys = SOC_REGS_PHY_BASE,
>   		.virt = SOC_REGS_PHY_BASE,

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Pali Rohár]

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
> > >                              * 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.
> > 
> > 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.

[PATCH u-boot-mvebu v2 0/3] arm: mvebu: a37xx: Fix and extend building memory map

[Pali Rohár]

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(-)

-- 
2.20.1

[PATCH u-boot-mvebu v2 1/3] arm: mvebu: a37xx: Fix calling build_mem_map()

[Pali Rohár]

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;
-- 
2.20.1

[PATCH u-boot-mvebu v2 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Pali Rohár]

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;
-- 
2.20.1

[PATCH u-boot-mvebu v2 3/3] arm: mvebu: a37xx: Fix comment with name of the function

[Pali Rohár]

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,
-- 
2.20.1

Re: [PATCH u-boot-mvebu v2 2/3] arm: mvebu: a37xx: Map CCI-400 and AP BootROM address space

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-mvebu v2 0/3] arm: mvebu: a37xx: Fix and extend building memory map

[Stefan Roese]

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