Re: [U-Boot] [PATCH u-boot-marvell v3 09/10] board: turris_mox: Support 1 GB version of Turris Mox
Hi, I have found the bug causing this issue.
If I understand the algorithm in get_ram_size correctly, it does approximately this. Suppose A, B, C, D, E, F are different constatnts. X(i) is a value at address 1<<i (couting in longs).
save[5] <- X(5) X(5) <- F save[4] <- X(4) X(4) <- E save[3] <- X(3) X(3) <- D save[2] <- X(2) X(2) <- C save[1] <- X(1) X(1) <- B save[0] <- X(0) X(0) <- A
So the previous values are stored in array save[]. The algorithm then checks if the values written (the constants A, B, C, D, E, F) are present at those addresses. The problem is that the previous value from save[] is written during checking of address i:
Now suppose the RAM is wrapped similarily as in MOX, so that X(i+3) is the same as X(i).
After the first part, the values are as follows
X([0,1,2,3,4,5]) = [A,B,C,A,B,C] save = [D,E,F,_3,_4,_5]
Here _3, _4, _5 are the values at addresses X(3), X(4), X(5) before the algorithm.
The code that checks the values written does this:
if X(0) != A return 0 X(0) <- save[0] !!! this also writes D to X(3)
if X(1) != B return 1 X(1) <- save[1] !!! this also writes E to X(4)
if X(2) != C return 2 X(2) <- save[2] !!! this also writes F to X(F)
if X(3) != D return 3 !!! this should return, but won't X(3) <- save[3]
...
One solution would be to write the previous values from the array save[] only immediately before return from the function.
I have to confess that I do not like how this function is written at all. It does not, for example, solve correctly the case when a device has 768 MiB of RAM from two chips (512 + 256). Given 1024 MiB as argument, it would return 1024 MiB, but the system only has 768 MiB. This maybe is never an issue with devices that run u-boot, but still.
Marek
On Tue, 11 Dec 2018 16:06:42 +0100 Stefan Roese sr@denx.de wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 11.12.18 14:59, Marek Behún wrote:
get_ram_size does not work correctly on Mox. On a 512 MiB board it detects 1024 MiB of RAM, because on the 512 MiB RAM chip the topmost address bit is simply ignored and the RAM wraps - on 0x20000000-0x40000000 CPU sees the same data as on 0x0-0x20000000.
That's what get_ram_size() does: It does detect such aliases when the same memory is mapped at multiple areas (power of 2). Did you give it a try with a max value of 1024 MiB? It should return 512 on such boards.
I checked it and it returned 1024 MiB. I did printf("%08x %08x\n", get_ram_size(0, 512<<20), get_ram_size(0, 1024<<20)); on a 512 MiB board and 0x20000000 0x40000000 was printed.
Very strange. Could you please debug this issue? get_ram_size() should be able to work in such situations.
Thanks, Stefan
ATF does not run RAM size determining code either, it just gets RAM size from a register, this register is written before ATF by BootROM and we have done it so that there is always 1 GB so that we could use same secure firmware image for all Moxes. I tried to change this register in secure firmware, but this lead to Synchornous Abort events in U-Boot.
Maybe we could move the dram_init funcitons from arm64-common.c to specific board files, or maybe we could declare them __weak in arm64-common.c and turris_mox can then redefine them.
Would that be OK with you?
Please fist check if get_ram_size() can't be used.
Thanks, Stefan
Marek
On Thu, 29 Nov 2018 14:07:59 +0100 Stefan Roese sr@denx.de wrote:
On 20.11.18 13:04, Marek Behún wrote:
Depending on the data in the OTP memory, differentiate between the 512 MiB and 1 GiB versions of Turris Mox and report these RAM sizes in dram_init and dram_init_banksize.
Signed-off-by: Marek Behún marek.behun@nic.cz
arch/arm/mach-mvebu/arm64-common.c | 7 ++++++- board/CZ.NIC/turris_mox/turris_mox.c | 27+++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c index f47273fde9..5e6ac9fc4a 100644 --- a/arch/arm/mach-mvebu/arm64-common.c +++ b/arch/arm/mach-mvebu/arm64-common.c @@ -43,8 +43,12 @@ const struct mbus_dram_target_info *mvebu_mbus_dram_info(void) return NULL; }
-/* DRAM init code ... */ +/*
- DRAM init code ...
- Turris Mox defines this itself, depending on data in burned
eFuses
- */
+#ifndef CONFIG_TARGET_TURRIS_MOX int dram_init_banksize(void) { fdtdec_setup_memory_banksize(); @@ -59,6 +63,7 @@ int dram_init(void)
return 0; }+#endif /* !CONFIG_TARGET_TURRIS_MOX */
2 Problems with this:
a) This does not apply any more with the latest changes in mainline.
b) I really don't like #ifdef's here in this common code. Can you not get rid of this somehow? Isn't the turris_mox also using ATF and will read the RAM size from there?
U-Boot still has the good old get_ram_size() function, which can easily auto-detect 512MiB vs 1GiB when run with 1GiB as parameter.
Thanks, Stefan
int arch_cpu_init(void) {diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c index 89b3cd2ce0..9aa2fc004d 100644 --- a/board/CZ.NIC/turris_mox/turris_mox.c +++ b/board/CZ.NIC/turris_mox/turris_mox.c @@ -14,6 +14,7 @@ #include <linux/string.h> #include <linux/libfdt.h> #include <fdt_support.h> +#include <environment.h>
#ifdef CONFIG_WDT_ARMADA_37XX #include <wdt.h>@@ -40,6 +41,32 @@
DECLARE_GLOBAL_DATA_PTR;+int dram_init(void) +{
- int ret, ram_size;
- gd->ram_base = 0;
- gd->ram_size = (phys_size_t)0x20000000;
- ret = mbox_sp_get_board_info(NULL, NULL, NULL, NULL,
&ram_size);
- if (ret < 0) {
puts("Cannot read RAM size from OTP, defaultingto 512 MiB");
- } else {
if (ram_size == 1024)gd->ram_size = (phys_size_t)0x40000000;- }
- return 0;
+}
+int dram_init_banksize(void) +{
- gd->bd->bi_dram[0].start = (phys_addr_t)0;
- gd->bd->bi_dram[0].size = gd->ram_size;
- return 0;
+}
- #if defined(CONFIG_OF_BOARD_FIXUP) int board_fix_fdt(void *blob) {
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
Hi Marek,
On 12.12.18 03:23, Marek Behun wrote:
Hi, I have found the bug causing this issue.
Good.
If I understand the algorithm in get_ram_size correctly, it does approximately this. Suppose A, B, C, D, E, F are different constatnts. X(i) is a value at address 1<<i (couting in longs).
save[5] <- X(5) X(5) <- F save[4] <- X(4) X(4) <- E save[3] <- X(3) X(3) <- D save[2] <- X(2) X(2) <- C save[1] <- X(1) X(1) <- B save[0] <- X(0) X(0) <- A
So the previous values are stored in array save[]. The algorithm then checks if the values written (the constants A, B, C, D, E, F) are present at those addresses. The problem is that the previous value from save[] is written during checking of address i:
Now suppose the RAM is wrapped similarily as in MOX, so that X(i+3) is the same as X(i).
After the first part, the values are as follows
X([0,1,2,3,4,5]) = [A,B,C,A,B,C] save = [D,E,F,_3,_4,_5]
Here _3, _4, _5 are the values at addresses X(3), X(4), X(5) before the algorithm.
The code that checks the values written does this:
if X(0) != A return 0 X(0) <- save[0] !!! this also writes D to X(3)
if X(1) != B return 1 X(1) <- save[1] !!! this also writes E to X(4)
if X(2) != C return 2 X(2) <- save[2] !!! this also writes F to X(F)
if X(3) != D return 3 !!! this should return, but won't X(3) <- save[3]
...
One solution would be to write the previous values from the array save[] only immediately before return from the function.
I have to admit that I didn't fully try to understand this issue you describe above (sorry, lack of time). If you have found a bug and do have a fix for it, then please submit a patch. Please add all developers (e.g. Patrick Delaunay etc) who did some work on this code to Cc, as changes here might be critical.
I have to confess that I do not like how this function is written at all. It does not, for example, solve correctly the case when a device has 768 MiB of RAM from two chips (512 + 256). Given 1024 MiB as argument, it would return 1024 MiB, but the system only has 768 MiB. This maybe is never an issue with devices that run u-boot, but still.
If you have a nice and easy implementation to also support such memory configurations, that would be perfect of course. But I really think that such non-power-of-2 memory configurations are rather uncommon for U-Boot and most likely don't need to be supported by this function. Such configuration usually are a result of using multiple DIMM's (or SODIMM's) which can be equipped with various sized memories. And here the memory size can be read from the DIMM itself. So no need to support this in get_ram_size().
Thanks, Stefan
Marek
On Tue, 11 Dec 2018 16:06:42 +0100 Stefan Roese sr@denx.de wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 11.12.18 14:59, Marek Behún wrote:
get_ram_size does not work correctly on Mox. On a 512 MiB board it detects 1024 MiB of RAM, because on the 512 MiB RAM chip the topmost address bit is simply ignored and the RAM wraps - on 0x20000000-0x40000000 CPU sees the same data as on 0x0-0x20000000.
That's what get_ram_size() does: It does detect such aliases when the same memory is mapped at multiple areas (power of 2). Did you give it a try with a max value of 1024 MiB? It should return 512 on such boards.
I checked it and it returned 1024 MiB. I did printf("%08x %08x\n", get_ram_size(0, 512<<20), get_ram_size(0, 1024<<20)); on a 512 MiB board and 0x20000000 0x40000000 was printed.
Very strange. Could you please debug this issue? get_ram_size() should be able to work in such situations.
Thanks, Stefan
ATF does not run RAM size determining code either, it just gets RAM size from a register, this register is written before ATF by BootROM and we have done it so that there is always 1 GB so that we could use same secure firmware image for all Moxes. I tried to change this register in secure firmware, but this lead to Synchornous Abort events in U-Boot.
Maybe we could move the dram_init funcitons from arm64-common.c to specific board files, or maybe we could declare them __weak in arm64-common.c and turris_mox can then redefine them.
Would that be OK with you?
Please fist check if get_ram_size() can't be used.
Thanks, Stefan
Marek
On Thu, 29 Nov 2018 14:07:59 +0100 Stefan Roese sr@denx.de wrote:
On 20.11.18 13:04, Marek Behún wrote: > Depending on the data in the OTP memory, differentiate between > the 512 MiB and 1 GiB versions of Turris Mox and report these > RAM sizes in dram_init and dram_init_banksize. > > Signed-off-by: Marek Behún marek.behun@nic.cz > --- > arch/arm/mach-mvebu/arm64-common.c | 7 ++++++- > board/CZ.NIC/turris_mox/turris_mox.c | 27 > +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 > deletion(-) > > diff --git a/arch/arm/mach-mvebu/arm64-common.c > b/arch/arm/mach-mvebu/arm64-common.c index > f47273fde9..5e6ac9fc4a 100644 --- > a/arch/arm/mach-mvebu/arm64-common.c +++ > b/arch/arm/mach-mvebu/arm64-common.c @@ -43,8 +43,12 @@ const > struct mbus_dram_target_info *mvebu_mbus_dram_info(void) return > NULL; } > > -/* DRAM init code ... */ > +/* > + * DRAM init code ... > + * Turris Mox defines this itself, depending on data in burned > eFuses > + */ > > +#ifndef CONFIG_TARGET_TURRIS_MOX > int dram_init_banksize(void) > { > fdtdec_setup_memory_banksize(); > @@ -59,6 +63,7 @@ int dram_init(void) > > return 0; > } > +#endif /* !CONFIG_TARGET_TURRIS_MOX */
2 Problems with this:
a) This does not apply any more with the latest changes in mainline.
b) I really don't like #ifdef's here in this common code. Can you not get rid of this somehow? Isn't the turris_mox also using ATF and will read the RAM size from there?
U-Boot still has the good old get_ram_size() function, which can easily auto-detect 512MiB vs 1GiB when run with 1GiB as parameter.
Thanks, Stefan
> > int arch_cpu_init(void) > { > diff --git a/board/CZ.NIC/turris_mox/turris_mox.c > b/board/CZ.NIC/turris_mox/turris_mox.c index > 89b3cd2ce0..9aa2fc004d 100644 --- > a/board/CZ.NIC/turris_mox/turris_mox.c +++ > b/board/CZ.NIC/turris_mox/turris_mox.c @@ -14,6 +14,7 @@ > #include <linux/string.h> > #include <linux/libfdt.h> > #include <fdt_support.h> > +#include <environment.h> > > #ifdef CONFIG_WDT_ARMADA_37XX > #include <wdt.h> > @@ -40,6 +41,32 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +int dram_init(void) > +{ > + int ret, ram_size; > + > + gd->ram_base = 0; > + gd->ram_size = (phys_size_t)0x20000000; > + > + ret = mbox_sp_get_board_info(NULL, NULL, NULL, NULL, > &ram_size); > + if (ret < 0) { > + puts("Cannot read RAM size from OTP, defaulting > to 512 MiB"); > + } else { > + if (ram_size == 1024) > + gd->ram_size = (phys_size_t)0x40000000; > + } > + > + return 0; > +} > + > +int dram_init_banksize(void) > +{ > + gd->bd->bi_dram[0].start = (phys_addr_t)0; > + gd->bd->bi_dram[0].size = gd->ram_size; > + > + return 0; > +} > + > #if defined(CONFIG_OF_BOARD_FIXUP) > int board_fix_fdt(void *blob) > { >
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
Hi Stefan,
it turned out that what I found out was not causing the bug. get_ram_size reported 1 GiB of ram because I tried it when dcache was already enabled. If I call get_ram_size in dram_init, it returns the correct size on both 512 MiB and 1 GiB board.
In the next patch I shall define dram_init and dram_init_banksize in arm64-common.c as __weak, and the definition in turris_mox.c shall call get_ram_size. Is this acceptable?
Marek
On Wed, 12 Dec 2018 10:44:15 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 12.12.18 03:23, Marek Behun wrote:
Hi, I have found the bug causing this issue.
Good.
If I understand the algorithm in get_ram_size correctly, it does approximately this. Suppose A, B, C, D, E, F are different constatnts. X(i) is a value at address 1<<i (couting in longs).
save[5] <- X(5) X(5) <- F save[4] <- X(4) X(4) <- E save[3] <- X(3) X(3) <- D save[2] <- X(2) X(2) <- C save[1] <- X(1) X(1) <- B save[0] <- X(0) X(0) <- A
So the previous values are stored in array save[]. The algorithm then checks if the values written (the constants A, B, C, D, E, F) are present at those addresses. The problem is that the previous value from save[] is written during checking of address i:
Now suppose the RAM is wrapped similarily as in MOX, so that X(i+3) is the same as X(i).
After the first part, the values are as follows
X([0,1,2,3,4,5]) = [A,B,C,A,B,C] save = [D,E,F,_3,_4,_5]
Here _3, _4, _5 are the values at addresses X(3), X(4), X(5) before the algorithm.
The code that checks the values written does this:
if X(0) != A return 0 X(0) <- save[0] !!! this also writes D to X(3)
if X(1) != B return 1 X(1) <- save[1] !!! this also writes E to X(4)
if X(2) != C return 2 X(2) <- save[2] !!! this also writes F to X(F)
if X(3) != D return 3 !!! this should return, but won't X(3) <- save[3]
...
One solution would be to write the previous values from the array save[] only immediately before return from the function.
I have to admit that I didn't fully try to understand this issue you describe above (sorry, lack of time). If you have found a bug and do have a fix for it, then please submit a patch. Please add all developers (e.g. Patrick Delaunay etc) who did some work on this code to Cc, as changes here might be critical.
I have to confess that I do not like how this function is written at all. It does not, for example, solve correctly the case when a device has 768 MiB of RAM from two chips (512 + 256). Given 1024 MiB as argument, it would return 1024 MiB, but the system only has 768 MiB. This maybe is never an issue with devices that run u-boot, but still.
If you have a nice and easy implementation to also support such memory configurations, that would be perfect of course. But I really think that such non-power-of-2 memory configurations are rather uncommon for U-Boot and most likely don't need to be supported by this function. Such configuration usually are a result of using multiple DIMM's (or SODIMM's) which can be equipped with various sized memories. And here the memory size can be read from the DIMM itself. So no need to support this in get_ram_size().
Thanks, Stefan
Marek
On Tue, 11 Dec 2018 16:06:42 +0100 Stefan Roese sr@denx.de wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 11.12.18 14:59, Marek Behún wrote:
get_ram_size does not work correctly on Mox. On a 512 MiB board it detects 1024 MiB of RAM, because on the 512 MiB RAM chip the topmost address bit is simply ignored and the RAM wraps - on 0x20000000-0x40000000 CPU sees the same data as on 0x0-0x20000000.
That's what get_ram_size() does: It does detect such aliases when the same memory is mapped at multiple areas (power of 2). Did you give it a try with a max value of 1024 MiB? It should return 512 on such boards.
I checked it and it returned 1024 MiB. I did printf("%08x %08x\n", get_ram_size(0, 512<<20), get_ram_size(0, 1024<<20)); on a 512 MiB board and 0x20000000 0x40000000 was printed.
Very strange. Could you please debug this issue? get_ram_size() should be able to work in such situations.
Thanks, Stefan
ATF does not run RAM size determining code either, it just gets RAM size from a register, this register is written before ATF by BootROM and we have done it so that there is always 1 GB so that we could use same secure firmware image for all Moxes. I tried to change this register in secure firmware, but this lead to Synchornous Abort events in U-Boot.
Maybe we could move the dram_init funcitons from arm64-common.c to specific board files, or maybe we could declare them __weak in arm64-common.c and turris_mox can then redefine them.
Would that be OK with you?
Please fist check if get_ram_size() can't be used.
Thanks, Stefan
Marek
On Thu, 29 Nov 2018 14:07:59 +0100 Stefan Roese sr@denx.de wrote:
> On 20.11.18 13:04, Marek Behún wrote: >> Depending on the data in the OTP memory, differentiate between >> the 512 MiB and 1 GiB versions of Turris Mox and report these >> RAM sizes in dram_init and dram_init_banksize. >> >> Signed-off-by: Marek Behún marek.behun@nic.cz >> --- >> arch/arm/mach-mvebu/arm64-common.c | 7 ++++++- >> board/CZ.NIC/turris_mox/turris_mox.c | 27 >> +++++++++++++++++++++++++++ 2 files changed, 33 >> insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-mvebu/arm64-common.c >> b/arch/arm/mach-mvebu/arm64-common.c index >> f47273fde9..5e6ac9fc4a 100644 --- >> a/arch/arm/mach-mvebu/arm64-common.c +++ >> b/arch/arm/mach-mvebu/arm64-common.c @@ -43,8 +43,12 @@ const >> struct mbus_dram_target_info *mvebu_mbus_dram_info(void) >> return NULL; } >> >> -/* DRAM init code ... */ >> +/* >> + * DRAM init code ... >> + * Turris Mox defines this itself, depending on data in >> burned eFuses >> + */ >> >> +#ifndef CONFIG_TARGET_TURRIS_MOX >> int dram_init_banksize(void) >> { >> fdtdec_setup_memory_banksize(); >> @@ -59,6 +63,7 @@ int dram_init(void) >> >> return 0; >> } >> +#endif /* !CONFIG_TARGET_TURRIS_MOX */ > > 2 Problems with this: > > a) > This does not apply any more with the latest changes in > mainline. > > b) > I really don't like #ifdef's here in this common code. Can you > not get rid of this somehow? Isn't the turris_mox also using > ATF and will read the RAM size from there? > > U-Boot still has the good old get_ram_size() function, which > can easily auto-detect 512MiB vs 1GiB when run with 1GiB as > parameter. > > Thanks, > Stefan > >> >> int arch_cpu_init(void) >> { >> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c >> b/board/CZ.NIC/turris_mox/turris_mox.c index >> 89b3cd2ce0..9aa2fc004d 100644 --- >> a/board/CZ.NIC/turris_mox/turris_mox.c +++ >> b/board/CZ.NIC/turris_mox/turris_mox.c @@ -14,6 +14,7 @@ >> #include <linux/string.h> >> #include <linux/libfdt.h> >> #include <fdt_support.h> >> +#include <environment.h> >> >> #ifdef CONFIG_WDT_ARMADA_37XX >> #include <wdt.h> >> @@ -40,6 +41,32 @@ >> >> DECLARE_GLOBAL_DATA_PTR; >> >> +int dram_init(void) >> +{ >> + int ret, ram_size; >> + >> + gd->ram_base = 0; >> + gd->ram_size = (phys_size_t)0x20000000; >> + >> + ret = mbox_sp_get_board_info(NULL, NULL, NULL, NULL, >> &ram_size); >> + if (ret < 0) { >> + puts("Cannot read RAM size from OTP, >> defaulting to 512 MiB"); >> + } else { >> + if (ram_size == 1024) >> + gd->ram_size = >> (phys_size_t)0x40000000; >> + } >> + >> + return 0; >> +} >> + >> +int dram_init_banksize(void) >> +{ >> + gd->bd->bi_dram[0].start = (phys_addr_t)0; >> + gd->bd->bi_dram[0].size = gd->ram_size; >> + >> + return 0; >> +} >> + >> #if defined(CONFIG_OF_BOARD_FIXUP) >> int board_fix_fdt(void *blob) >> { >> > > Viele Grüße, > Stefan >
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
Hi Marek,
On 13.12.18 04:53, Marek Behun wrote:
it turned out that what I found out was not causing the bug. get_ram_size reported 1 GiB of ram because I tried it when dcache was already enabled. If I call get_ram_size in dram_init, it returns the correct size on both 512 MiB and 1 GiB board.
In the next patch I shall define dram_init and dram_init_banksize in arm64-common.c as __weak, and the definition in turris_mox.c shall call get_ram_size. Is this acceptable?
Okay, please prepare the patch and I'll review it then.
Thanks, Stefan
Marek
On Wed, 12 Dec 2018 10:44:15 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 12.12.18 03:23, Marek Behun wrote:
Hi, I have found the bug causing this issue.
Good.
If I understand the algorithm in get_ram_size correctly, it does approximately this. Suppose A, B, C, D, E, F are different constatnts. X(i) is a value at address 1<<i (couting in longs).
save[5] <- X(5) X(5) <- F save[4] <- X(4) X(4) <- E save[3] <- X(3) X(3) <- D save[2] <- X(2) X(2) <- C save[1] <- X(1) X(1) <- B save[0] <- X(0) X(0) <- A
So the previous values are stored in array save[]. The algorithm then checks if the values written (the constants A, B, C, D, E, F) are present at those addresses. The problem is that the previous value from save[] is written during checking of address i:
Now suppose the RAM is wrapped similarily as in MOX, so that X(i+3) is the same as X(i).
After the first part, the values are as follows
X([0,1,2,3,4,5]) = [A,B,C,A,B,C] save = [D,E,F,_3,_4,_5]
Here _3, _4, _5 are the values at addresses X(3), X(4), X(5) before the algorithm.
The code that checks the values written does this:
if X(0) != A return 0 X(0) <- save[0] !!! this also writes D to X(3)
if X(1) != B return 1 X(1) <- save[1] !!! this also writes E to X(4)
if X(2) != C return 2 X(2) <- save[2] !!! this also writes F to X(F)
if X(3) != D return 3 !!! this should return, but won't X(3) <- save[3]
...
One solution would be to write the previous values from the array save[] only immediately before return from the function.
I have to admit that I didn't fully try to understand this issue you describe above (sorry, lack of time). If you have found a bug and do have a fix for it, then please submit a patch. Please add all developers (e.g. Patrick Delaunay etc) who did some work on this code to Cc, as changes here might be critical.
I have to confess that I do not like how this function is written at all. It does not, for example, solve correctly the case when a device has 768 MiB of RAM from two chips (512 + 256). Given 1024 MiB as argument, it would return 1024 MiB, but the system only has 768 MiB. This maybe is never an issue with devices that run u-boot, but still.
If you have a nice and easy implementation to also support such memory configurations, that would be perfect of course. But I really think that such non-power-of-2 memory configurations are rather uncommon for U-Boot and most likely don't need to be supported by this function. Such configuration usually are a result of using multiple DIMM's (or SODIMM's) which can be equipped with various sized memories. And here the memory size can be read from the DIMM itself. So no need to support this in get_ram_size().
Thanks, Stefan
Marek
On Tue, 11 Dec 2018 16:06:42 +0100 Stefan Roese sr@denx.de wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 11.12.18 14:59, Marek Behún wrote: > get_ram_size does not work correctly on Mox. On a 512 MiB board > it detects 1024 MiB of RAM, because on the 512 MiB RAM chip the > topmost address bit is simply ignored and the RAM wraps - on > 0x20000000-0x40000000 CPU sees the same data as on > 0x0-0x20000000.
That's what get_ram_size() does: It does detect such aliases when the same memory is mapped at multiple areas (power of 2). Did you give it a try with a max value of 1024 MiB? It should return 512 on such boards.
I checked it and it returned 1024 MiB. I did printf("%08x %08x\n", get_ram_size(0, 512<<20), get_ram_size(0, 1024<<20)); on a 512 MiB board and 0x20000000 0x40000000 was printed.
Very strange. Could you please debug this issue? get_ram_size() should be able to work in such situations.
Thanks, Stefan
> ATF does not run RAM size determining code either, it just gets > RAM size from a register, this register is written before ATF by > BootROM and we have done it so that there is always 1 GB so that > we could use same secure firmware image for all Moxes. I tried > to change this register in secure firmware, but this lead to > Synchornous Abort events in U-Boot. > > Maybe we could move the dram_init funcitons from arm64-common.c > to specific board files, or maybe we could declare them __weak > in arm64-common.c and turris_mox can then redefine them. > > Would that be OK with you?
Please fist check if get_ram_size() can't be used.
Thanks, Stefan
> Marek > > On Thu, 29 Nov 2018 14:07:59 +0100 > Stefan Roese sr@denx.de wrote: > >> On 20.11.18 13:04, Marek Behún wrote: >>> Depending on the data in the OTP memory, differentiate between >>> the 512 MiB and 1 GiB versions of Turris Mox and report these >>> RAM sizes in dram_init and dram_init_banksize. >>> >>> Signed-off-by: Marek Behún marek.behun@nic.cz >>> --- >>> arch/arm/mach-mvebu/arm64-common.c | 7 ++++++- >>> board/CZ.NIC/turris_mox/turris_mox.c | 27 >>> +++++++++++++++++++++++++++ 2 files changed, 33 >>> insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-mvebu/arm64-common.c >>> b/arch/arm/mach-mvebu/arm64-common.c index >>> f47273fde9..5e6ac9fc4a 100644 --- >>> a/arch/arm/mach-mvebu/arm64-common.c +++ >>> b/arch/arm/mach-mvebu/arm64-common.c @@ -43,8 +43,12 @@ const >>> struct mbus_dram_target_info *mvebu_mbus_dram_info(void) >>> return NULL; } >>> >>> -/* DRAM init code ... */ >>> +/* >>> + * DRAM init code ... >>> + * Turris Mox defines this itself, depending on data in >>> burned eFuses >>> + */ >>> >>> +#ifndef CONFIG_TARGET_TURRIS_MOX >>> int dram_init_banksize(void) >>> { >>> fdtdec_setup_memory_banksize(); >>> @@ -59,6 +63,7 @@ int dram_init(void) >>> >>> return 0; >>> } >>> +#endif /* !CONFIG_TARGET_TURRIS_MOX */ >> >> 2 Problems with this: >> >> a) >> This does not apply any more with the latest changes in >> mainline. >> >> b) >> I really don't like #ifdef's here in this common code. Can you >> not get rid of this somehow? Isn't the turris_mox also using >> ATF and will read the RAM size from there? >> >> U-Boot still has the good old get_ram_size() function, which >> can easily auto-detect 512MiB vs 1GiB when run with 1GiB as >> parameter. >> >> Thanks, >> Stefan >> >>> >>> int arch_cpu_init(void) >>> { >>> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c >>> b/board/CZ.NIC/turris_mox/turris_mox.c index >>> 89b3cd2ce0..9aa2fc004d 100644 --- >>> a/board/CZ.NIC/turris_mox/turris_mox.c +++ >>> b/board/CZ.NIC/turris_mox/turris_mox.c @@ -14,6 +14,7 @@ >>> #include <linux/string.h> >>> #include <linux/libfdt.h> >>> #include <fdt_support.h> >>> +#include <environment.h> >>> >>> #ifdef CONFIG_WDT_ARMADA_37XX >>> #include <wdt.h> >>> @@ -40,6 +41,32 @@ >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> +int dram_init(void) >>> +{ >>> + int ret, ram_size; >>> + >>> + gd->ram_base = 0; >>> + gd->ram_size = (phys_size_t)0x20000000; >>> + >>> + ret = mbox_sp_get_board_info(NULL, NULL, NULL, NULL, >>> &ram_size); >>> + if (ret < 0) { >>> + puts("Cannot read RAM size from OTP, >>> defaulting to 512 MiB"); >>> + } else { >>> + if (ram_size == 1024) >>> + gd->ram_size = >>> (phys_size_t)0x40000000; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int dram_init_banksize(void) >>> +{ >>> + gd->bd->bi_dram[0].start = (phys_addr_t)0; >>> + gd->bd->bi_dram[0].size = gd->ram_size; >>> + >>> + return 0; >>> +} >>> + >>> #if defined(CONFIG_OF_BOARD_FIXUP) >>> int board_fix_fdt(void *blob) >>> { >>> >> >> Viele Grüße, >> Stefan >> >
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan
participants (2)
-
Marek Behun -
Stefan Roese