[U-Boot] [PATCH] fdt: fix get_next_memory_node()

[U-Boot] [PATCH] fdt: fix get_next_memory_node()

[Marek Vasut]

The get_next_memory_node() always sets mem to -1 , which is incorrect,
because then every iteration of memory bank parsing will start from the
first memory bank instead of the previous one.

On systems with 1 memory bank defined in DT and CONFIG_NR_DRAM_BANKS=4 ,
like ie. r8a77965-salvator-x , this will result in U-Boot incorrectly
reporting four identical memory banks with the same memory configuration.

Fix this by setting mem to startoffset value, which restores the behavior
before the fixed patch was applied.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
Fixes: 452bc121027d ("fdt: fix fdtdec_setup_memory_banksize()")
---
 lib/fdtdec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index a208589c48..b2ec12e9b5 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1182,10 +1182,8 @@ int fdtdec_setup_mem_size_base(void)
 
 #if defined(CONFIG_NR_DRAM_BANKS)
 
-static int get_next_memory_node(const void *blob, int startoffset)
+static int get_next_memory_node(const void *blob, int mem)
 {
-	int mem = -1;
-
 	do {
 		mem = fdt_node_offset_by_prop_value(gd->fdt_blob, mem,
 						    "device_type", "memory", 7);
-- 
2.18.0

[U-Boot] [PATCH] fdt: fix get_next_memory_node()

[Michal Simek]

On 9.9.2018 16:30, Marek Vasut wrote:
> The get_next_memory_node() always sets mem to -1 , which is incorrect,
> because then every iteration of memory bank parsing will start from the
> first memory bank instead of the previous one.
> 
> On systems with 1 memory bank defined in DT and CONFIG_NR_DRAM_BANKS=4 ,
> like ie. r8a77965-salvator-x , this will result in U-Boot incorrectly
> reporting four identical memory banks with the same memory configuration.
> 
> Fix this by setting mem to startoffset value, which restores the behavior
> before the fixed patch was applied.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Fixes: 452bc121027d ("fdt: fix fdtdec_setup_memory_banksize()")


Tested-by: Michal Simek <michal.simek@xilinx.com> [on ZynqMP}

Before
arch_number = 0x00000000
boot_params = 0x00000000
DRAM bank   = 0x00000000
-> start    = 0x00000000
-> size     = 0x10000000
DRAM bank   = 0x00000001
-> start    = 0x10000000
-> size     = 0x40000000
DRAM bank   = 0x00000002
-> start    = 0x50000000
-> size     = 0x30000000
DRAM bank   = 0x00000003
-> start    = 0x00000000
-> size     = 0x10000000


after
arch_number = 0x00000000
boot_params = 0x00000000
DRAM bank   = 0x00000000
-> start    = 0x00000000
-> size     = 0x10000000
DRAM bank   = 0x00000001
-> start    = 0x10000000
-> size     = 0x40000000
DRAM bank   = 0x00000002
-> start    = 0x50000000
-> size     = 0x30000000

Thanks,
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180910/08b8dd21/attachment.sig>

[U-Boot] [PATCH] fdt: fix get_next_memory_node()

[Marek Vasut]

On 09/10/2018 01:23 PM, Michal Simek wrote:
> On 9.9.2018 16:30, Marek Vasut wrote:
>> The get_next_memory_node() always sets mem to -1 , which is incorrect,
>> because then every iteration of memory bank parsing will start from the
>> first memory bank instead of the previous one.
>>
>> On systems with 1 memory bank defined in DT and CONFIG_NR_DRAM_BANKS=4 ,
>> like ie. r8a77965-salvator-x , this will result in U-Boot incorrectly
>> reporting four identical memory banks with the same memory configuration.
>>
>> Fix this by setting mem to startoffset value, which restores the behavior
>> before the fixed patch was applied.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Jens Wiklander <jens.wiklander@linaro.org>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> Fixes: 452bc121027d ("fdt: fix fdtdec_setup_memory_banksize()")
> 
> 
> Tested-by: Michal Simek <michal.simek@xilinx.com> [on ZynqMP}
> 
> Before
> arch_number = 0x00000000
> boot_params = 0x00000000
> DRAM bank   = 0x00000000
> -> start    = 0x00000000
> -> size     = 0x10000000
> DRAM bank   = 0x00000001
> -> start    = 0x10000000
> -> size     = 0x40000000
> DRAM bank   = 0x00000002
> -> start    = 0x50000000
> -> size     = 0x30000000
> DRAM bank   = 0x00000003
> -> start    = 0x00000000
> -> size     = 0x10000000
> 
> 
> after
> arch_number = 0x00000000
> boot_params = 0x00000000
> DRAM bank   = 0x00000000
> -> start    = 0x00000000
> -> size     = 0x10000000
> DRAM bank   = 0x00000001
> -> start    = 0x10000000
> -> size     = 0x40000000
> DRAM bank   = 0x00000002
> -> start    = 0x50000000
> -> size     = 0x30000000

That's on a system with three banks, so it was broken before this patch?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] fdt: fix get_next_memory_node()

[Michal Simek]

On 10.9.2018 13:27, Marek Vasut wrote:
> On 09/10/2018 01:23 PM, Michal Simek wrote:
>> On 9.9.2018 16:30, Marek Vasut wrote:
>>> The get_next_memory_node() always sets mem to -1 , which is incorrect,
>>> because then every iteration of memory bank parsing will start from the
>>> first memory bank instead of the previous one.
>>>
>>> On systems with 1 memory bank defined in DT and CONFIG_NR_DRAM_BANKS=4 ,
>>> like ie. r8a77965-salvator-x , this will result in U-Boot incorrectly
>>> reporting four identical memory banks with the same memory configuration.
>>>
>>> Fix this by setting mem to startoffset value, which restores the behavior
>>> before the fixed patch was applied.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Jens Wiklander <jens.wiklander@linaro.org>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> Fixes: 452bc121027d ("fdt: fix fdtdec_setup_memory_banksize()")
>>
>>
>> Tested-by: Michal Simek <michal.simek@xilinx.com> [on ZynqMP}
>>
>> Before
>> arch_number = 0x00000000
>> boot_params = 0x00000000
>> DRAM bank   = 0x00000000
>> -> start    = 0x00000000
>> -> size     = 0x10000000
>> DRAM bank   = 0x00000001
>> -> start    = 0x10000000
>> -> size     = 0x40000000
>> DRAM bank   = 0x00000002
>> -> start    = 0x50000000
>> -> size     = 0x30000000
>> DRAM bank   = 0x00000003
>> -> start    = 0x00000000
>> -> size     = 0x10000000
>>
>>
>> after
>> arch_number = 0x00000000
>> boot_params = 0x00000000
>> DRAM bank   = 0x00000000
>> -> start    = 0x00000000
>> -> size     = 0x10000000
>> DRAM bank   = 0x00000001
>> -> start    = 0x10000000
>> -> size     = 0x40000000
>> DRAM bank   = 0x00000002
>> -> start    = 0x50000000
>> -> size     = 0x30000000
> 
> That's on a system with three banks, so it was broken before this patch?

yes that's on 3 bank system and as is visible above there are 4 banks
listed and also all of them were calculated together.
That's why system with 2GB of memory had surprisingly 2.3GiB.

Before with my 3 bank setup
DRAM:  2.3 GiB
After
DRAM:  2 GiB

With one bank this is even more tricky because
DRAM:  8 GiB
is reported but relocation works properly.

Definitely this is regression and it should go to this release.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180910/f337ae88/attachment.sig>

[U-Boot] fdt: fix get_next_memory_node()

[Tom Rini]

On Sun, Sep 09, 2018 at 04:30:11PM +0200, Marek Vasut wrote:

> The get_next_memory_node() always sets mem to -1 , which is incorrect,
> because then every iteration of memory bank parsing will start from the
> first memory bank instead of the previous one.
> 
> On systems with 1 memory bank defined in DT and CONFIG_NR_DRAM_BANKS=4 ,
> like ie. r8a77965-salvator-x , this will result in U-Boot incorrectly
> reporting four identical memory banks with the same memory configuration.
> 
> Fix this by setting mem to startoffset value, which restores the behavior
> before the fixed patch was applied.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Fixes: 452bc121027d ("fdt: fix fdtdec_setup_memory_banksize()")
> Tested-by: Michal Simek <michal.simek@xilinx.com> [on ZynqMP}

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180910/aba7698e/attachment.sig>