[U-Boot] [PATCH v2 0/4] arm: imx53: remove usage of mx53_dram_size
From: Patrick Bruenn p.bruenn@beckhoff.com
Global variables are not available during board_init_f(). The i.MX53 boards m53evk, mx53cx9020 and mx53loco are using the exact same dram initialization code, which uses 'static uint32_t mx53_dram_size[2];' in dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
This series replaces the static variable with multiple calls to get_ram_size() and moves the shared code into arch/arm/mach-imx/mx5/.
The first patch fixes cx9020. The next patch moves that code to a common place to be reused by m53evk and mx53loco with the next patches.
Changes in v2: - move dram initialization into common location - reuse fixed dram initialization for m53evk and mx53loco
Patrick Bruenn (4): arm: imx: cx9020: remove usage of mx53_dram_size arm: imx: cx9020: move dram init into common place arm: imx: m53evk: remove usage of mx53_dram_size arm: imx: mx53loco: remove usage of mx53_dram_size
arch/arm/mach-imx/mx5/Makefile | 5 ++++ arch/arm/mach-imx/mx5/mx53_dram.c | 45 ++++++++++++++++++++++++++++++++++ board/aries/m53evk/m53evk.c | 39 ----------------------------- board/beckhoff/mx53cx9020/mx53cx9020.c | 39 ----------------------------- board/freescale/mx53loco/mx53loco.c | 39 ----------------------------- 5 files changed, 50 insertions(+), 117 deletions(-) create mode 100644 arch/arm/mach-imx/mx5/mx53_dram.c
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f(). 'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
However multiple calls are better than undefined behavior... This fixes: https://lists.denx.de/pipermail/u-boot/2017-November/313214.html https://lists.denx.de/pipermail/u-boot/2017-December/314480.html
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
---
Changes in v2: - move dram initialization into common location - reuse fixed dram initialization for m53evk and mx53loco
board/beckhoff/mx53cx9020/mx53cx9020.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/board/beckhoff/mx53cx9020/mx53cx9020.c b/board/beckhoff/mx53cx9020/mx53cx9020.c index 021bd967c4..d8bdfc27bb 100644 --- a/board/beckhoff/mx53cx9020/mx53cx9020.c +++ b/board/beckhoff/mx53cx9020/mx53cx9020.c @@ -59,8 +59,6 @@ static const u32 CCAT_MODE_RUN = 0x0033DC8F;
DECLARE_GLOBAL_DATA_PTR;
-static uint32_t mx53_dram_size[2]; - phys_size_t get_effective_memsize(void) { /* @@ -74,15 +72,13 @@ phys_size_t get_effective_memsize(void) * U-Boot into invalid memory area close to the end of the first * DRAM bank. */ - return mx53_dram_size[0]; + return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); }
int dram_init(void) { - mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); - - gd->ram_size = mx53_dram_size[0] + mx53_dram_size[1]; + gd->ram_size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); + gd->ram_size += get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);
return 0; } @@ -90,10 +86,10 @@ int dram_init(void) int dram_init_banksize(void) { gd->bd->bi_dram[0].start = PHYS_SDRAM_1; - gd->bd->bi_dram[0].size = mx53_dram_size[0]; + gd->bd->bi_dram[0].size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);
gd->bd->bi_dram[1].start = PHYS_SDRAM_2; - gd->bd->bi_dram[1].size = mx53_dram_size[1]; + gd->bd->bi_dram[1].size = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);
return 0; }
From: Patrick Bruenn p.bruenn@beckhoff.com
Move dram_init(), dram_init_banksize() and get_effective_memsize() to arch/arm/mach-imx/mx5/mx53_dram.c, where it can be reused by m53evk and mx53loco.
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
---
Changes in v2: None
Patch-Cc: Fabio Estevam fabio.estevam@nxp.com Patch-Cc: Christian Gmeiner christian.gmeiner@gmail.com Patch-Cc: Jason Liu r64343@freescale.com Patch-Cc: Patrick Bruenn p.bruenn@beckhoff.com Patch-Cc: Stefano Babic sbabic@denx.de Patch-Cc: u-boot@lists.denx.de Patch-Cc: Marek Vasut marex@denx.de Patch-Cc: Albert Aribaud albert.u.boot@aribaud.net
--- arch/arm/mach-imx/mx5/Makefile | 3 +++ arch/arm/mach-imx/mx5/mx53_dram.c | 45 ++++++++++++++++++++++++++++++++++ board/beckhoff/mx53cx9020/mx53cx9020.c | 35 -------------------------- 3 files changed, 48 insertions(+), 35 deletions(-) create mode 100644 arch/arm/mach-imx/mx5/mx53_dram.c
diff --git a/arch/arm/mach-imx/mx5/Makefile b/arch/arm/mach-imx/mx5/Makefile index d021842f68..368cfde98b 100644 --- a/arch/arm/mach-imx/mx5/Makefile +++ b/arch/arm/mach-imx/mx5/Makefile @@ -9,3 +9,6 @@
obj-y := soc.o clock.o obj-y += lowlevel_init.o + +# common files for mx53 dram initialization +obj-$(CONFIG_TARGET_MX53CX9020) += mx53_dram.o diff --git a/arch/arm/mach-imx/mx5/mx53_dram.c b/arch/arm/mach-imx/mx5/mx53_dram.c new file mode 100644 index 0000000000..7e5fc42d1f --- /dev/null +++ b/arch/arm/mach-imx/mx5/mx53_dram.c @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2017 Beckhoff Automation GmbH & Co. KG + * Patrick Bruenn p.bruenn@beckhoff.com + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> + +DECLARE_GLOBAL_DATA_PTR; + +phys_size_t get_effective_memsize(void) +{ + /* + * WARNING: We must override get_effective_memsize() function here + * to report only the size of the first DRAM bank. This is to make + * U-Boot relocator place U-Boot into valid memory, that is, at the + * end of the first DRAM bank. If we did not override this function + * like so, U-Boot would be placed at the address of the first DRAM + * bank + total DRAM size - sizeof(uboot), which in the setup where + * each DRAM bank contains 512MiB of DRAM would result in placing + * U-Boot into invalid memory area close to the end of the first + * DRAM bank. + */ + return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); +} + +int dram_init(void) +{ + gd->ram_size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); + gd->ram_size += get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); + + return 0; +} + +int dram_init_banksize(void) +{ + gd->bd->bi_dram[0].start = PHYS_SDRAM_1; + gd->bd->bi_dram[0].size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); + + gd->bd->bi_dram[1].start = PHYS_SDRAM_2; + gd->bd->bi_dram[1].size = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); + + return 0; +} diff --git a/board/beckhoff/mx53cx9020/mx53cx9020.c b/board/beckhoff/mx53cx9020/mx53cx9020.c index d8bdfc27bb..f9df3604cd 100644 --- a/board/beckhoff/mx53cx9020/mx53cx9020.c +++ b/board/beckhoff/mx53cx9020/mx53cx9020.c @@ -59,41 +59,6 @@ static const u32 CCAT_MODE_RUN = 0x0033DC8F;
DECLARE_GLOBAL_DATA_PTR;
-phys_size_t get_effective_memsize(void) -{ - /* - * WARNING: We must override get_effective_memsize() function here - * to report only the size of the first DRAM bank. This is to make - * U-Boot relocator place U-Boot into valid memory, that is, at the - * end of the first DRAM bank. If we did not override this function - * like so, U-Boot would be placed at the address of the first DRAM - * bank + total DRAM size - sizeof(uboot), which in the setup where - * each DRAM bank contains 512MiB of DRAM would result in placing - * U-Boot into invalid memory area close to the end of the first - * DRAM bank. - */ - return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); -} - -int dram_init(void) -{ - gd->ram_size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - gd->ram_size += get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); - - return 0; -} - -int dram_init_banksize(void) -{ - gd->bd->bi_dram[0].start = PHYS_SDRAM_1; - gd->bd->bi_dram[0].size = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - - gd->bd->bi_dram[1].start = PHYS_SDRAM_2; - gd->bd->bi_dram[1].size = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); - - return 0; -} - u32 get_board_rev(void) { struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f(). 'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
---
Changes in v2: None
Only compile tested with make m53evk_defconfig
--- arch/arm/mach-imx/mx5/Makefile | 1 + board/aries/m53evk/m53evk.c | 39 --------------------------------------- 2 files changed, 1 insertion(+), 39 deletions(-)
diff --git a/arch/arm/mach-imx/mx5/Makefile b/arch/arm/mach-imx/mx5/Makefile index 368cfde98b..2cc2cbc07a 100644 --- a/arch/arm/mach-imx/mx5/Makefile +++ b/arch/arm/mach-imx/mx5/Makefile @@ -11,4 +11,5 @@ obj-y := soc.o clock.o obj-y += lowlevel_init.o
# common files for mx53 dram initialization +obj-$(CONFIG_TARGET_M53EVK) += mx53_dram.o obj-$(CONFIG_TARGET_MX53CX9020) += mx53_dram.o diff --git a/board/aries/m53evk/m53evk.c b/board/aries/m53evk/m53evk.c index ece8957aaf..a798d83376 100644 --- a/board/aries/m53evk/m53evk.c +++ b/board/aries/m53evk/m53evk.c @@ -31,45 +31,6 @@
DECLARE_GLOBAL_DATA_PTR;
-static uint32_t mx53_dram_size[2]; - -phys_size_t get_effective_memsize(void) -{ - /* - * WARNING: We must override get_effective_memsize() function here - * to report only the size of the first DRAM bank. This is to make - * U-Boot relocator place U-Boot into valid memory, that is, at the - * end of the first DRAM bank. If we did not override this function - * like so, U-Boot would be placed at the address of the first DRAM - * bank + total DRAM size - sizeof(uboot), which in the setup where - * each DRAM bank contains 512MiB of DRAM would result in placing - * U-Boot into invalid memory area close to the end of the first - * DRAM bank. - */ - return mx53_dram_size[0]; -} - -int dram_init(void) -{ - mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); - - gd->ram_size = mx53_dram_size[0] + mx53_dram_size[1]; - - return 0; -} - -int dram_init_banksize(void) -{ - gd->bd->bi_dram[0].start = PHYS_SDRAM_1; - gd->bd->bi_dram[0].size = mx53_dram_size[0]; - - gd->bd->bi_dram[1].start = PHYS_SDRAM_2; - gd->bd->bi_dram[1].size = mx53_dram_size[1]; - - return 0; -} - static void setup_iomux_uart(void) { static const iomux_v3_cfg_t uart_pads[] = {
On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
arch/arm/mach-imx/mx5/Makefile | 1 + board/aries/m53evk/m53evk.c | 39 --------------------------------------- 2 files changed, 1 insertion(+), 39 deletions(-)
diff --git a/arch/arm/mach-imx/mx5/Makefile b/arch/arm/mach-imx/mx5/Makefile index 368cfde98b..2cc2cbc07a 100644 --- a/arch/arm/mach-imx/mx5/Makefile +++ b/arch/arm/mach-imx/mx5/Makefile @@ -11,4 +11,5 @@ obj-y := soc.o clock.o obj-y += lowlevel_init.o
# common files for mx53 dram initialization +obj-$(CONFIG_TARGET_M53EVK) += mx53_dram.o obj-$(CONFIG_TARGET_MX53CX9020) += mx53_dram.o diff --git a/board/aries/m53evk/m53evk.c b/board/aries/m53evk/m53evk.c index ece8957aaf..a798d83376 100644 --- a/board/aries/m53evk/m53evk.c +++ b/board/aries/m53evk/m53evk.c @@ -31,45 +31,6 @@
DECLARE_GLOBAL_DATA_PTR;
-static uint32_t mx53_dram_size[2];
-phys_size_t get_effective_memsize(void) -{
- /*
* WARNING: We must override get_effective_memsize() function here* to report only the size of the first DRAM bank. This is to make* U-Boot relocator place U-Boot into valid memory, that is, at the* end of the first DRAM bank. If we did not override this function* like so, U-Boot would be placed at the address of the first DRAM* bank + total DRAM size - sizeof(uboot), which in the setup where* each DRAM bank contains 512MiB of DRAM would result in placing* U-Boot into invalid memory area close to the end of the first* DRAM bank.*/- return mx53_dram_size[0];
-}
-int dram_init(void) -{
- mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);
- mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);
- gd->ram_size = mx53_dram_size[0] + mx53_dram_size[1];
- return 0;
-}
-int dram_init_banksize(void) -{
- gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
- gd->bd->bi_dram[0].size = mx53_dram_size[0];
- gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
- gd->bd->bi_dram[1].size = mx53_dram_size[1];
- return 0;
-}
static void setup_iomux_uart(void) { static const iomux_v3_cfg_t uart_pads[] = {
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
From reading the README and common/board_f.c I got the impression, that dram_init() is called before it's save to access mx53_dram_size. And as that change seemed to cure the strange behavior I described in [1] and [2], I prepared a patch for my board, which ended up to be requested for m53evk and mx53loco, too.
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-
imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes: - return mx53_dram_size[0]; + return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);
So, yes I am convinced that get_effective_memsize() still returns only the size of the first dram bank. However, I would be fine with just keeping the changes to my board (cx9020). And if anyone has a better idea of what might be the root cause of [1] and [2], I would absolute appreciate any input to that.
Best regards and thanks in advance for any feedback, Patrick
[1] https://lists.denx.de/pipermail/u-boot/2017-November/313214.html [2] https://lists.denx.de/pipermail/u-boot/2017-December/314480.html
--- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 12/18/2017 11:47 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
From reading the README and common/board_f.c I got the impression, that dram_init() is called before it's save to access mx53_dram_size. And as that change seemed to cure the strange behavior I described in [1] and [2], I prepared a patch for my board, which ended up to be requested for m53evk and mx53loco, too.
Technically yes, board_init_f means it runs from FLASH and has no or minimal storage available. On the MX53 with SPL, it's running from RAM so it's safe to use static variables too.
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-
imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes:
return mx53_dram_size[0];
return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);So, yes I am convinced that get_effective_memsize() still returns only the size of the first dram bank.
I suspect that will fails on M53 due to it's split-bank configuration. The board has two DRAM banks with a hole between them.
However, I would be fine with just keeping the changes to my board (cx9020). And if anyone has a better idea of what might be the root cause of [1] and [2], I would absolute appreciate any input to that.
If your board also has two DRAM banks with a hole between them and you can test if this works fine, then I'm OK with this change.
Best regards and thanks in advance for any feedback, Patrick
[1] https://lists.denx.de/pipermail/u-boot/2017-November/313214.html [2] https://lists.denx.de/pipermail/u-boot/2017-December/314480.html
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 11:57 On 12/18/2017 11:47 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
From reading the README and common/board_f.c I got the impression, that dram_init() is called before it's save to access mx53_dram_size. And as that change seemed to cure the strange behavior I described in [1] and [2], I prepared a patch for my board, which ended up to be requested for m53evk and mx53loco, too.
Technically yes, board_init_f means it runs from FLASH and has no or minimal storage available. On the MX53 with SPL, it's running from RAM so it's safe to use static variables too.
Thank's for your explanation.
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-
imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes:
return mx53_dram_size[0];
return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);So, yes I am convinced that get_effective_memsize() still returns only the
size
of the first dram bank.
I suspect that will fails on M53 due to it's split-bank configuration. The board has two DRAM banks with a hole between them.
This is how mx53_dram_size was initialized on m53 before that patch: - mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);
So to me that's, absolutely the same. With the only difference, that get_ram_size(bank0) is called multiple times, now.
However, I would be fine with just keeping the changes to my board
(cx9020).
And if anyone has a better idea of what might be the root cause of [1] and
[2],
I would absolute appreciate any input to that.
If your board also has two DRAM banks with a hole between them and you can test if this works fine, then I'm OK with this change.
Yes, cx9020 uses two 512MB banks with 1GB between PHYS_SDRAM_1/2, too.
Best regards and thanks in advance for any feedback, Patrick
[1] https://lists.denx.de/pipermail/u-boot/2017-November/313214.html [2] https://lists.denx.de/pipermail/u-boot/2017-December/314480.html
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans
Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
-- Best regards, Marek Vasut
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 12/18/2017 12:40 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 11:57 On 12/18/2017 11:47 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
From reading the README and common/board_f.c I got the impression, that dram_init() is called before it's save to access mx53_dram_size. And as that change seemed to cure the strange behavior I described in [1] and [2], I prepared a patch for my board, which ended up to be requested for m53evk and mx53loco, too.
Technically yes, board_init_f means it runs from FLASH and has no or minimal storage available. On the MX53 with SPL, it's running from RAM so it's safe to use static variables too.
Thank's for your explanation.
No problem, it's a bit weird.
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-
imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes:
return mx53_dram_size[0];
return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);So, yes I am convinced that get_effective_memsize() still returns only the
size
of the first dram bank.
I suspect that will fails on M53 due to it's split-bank configuration. The board has two DRAM banks with a hole between them.
This is how mx53_dram_size was initialized on m53 before that patch:
mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);So to me that's, absolutely the same. With the only difference, that get_ram_size(bank0) is called multiple times, now.
The get_ram_size() above is called for two different bank (addresses), not for bank0 twice. Maybe I'm missing something.
btw where's the patch adding mx53_dram.c ? I don't see it in mainline yet.
However, I would be fine with just keeping the changes to my board
(cx9020).
And if anyone has a better idea of what might be the root cause of [1] and
[2],
I would absolute appreciate any input to that.
If your board also has two DRAM banks with a hole between them and you can test if this works fine, then I'm OK with this change.
Yes, cx9020 uses two 512MB banks with 1GB between PHYS_SDRAM_1/2, too.
Then that should be the same situation. Can you share "bdinfo" from that board of yours?
btw do you use SPL ? If not, you should ...
[...]
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 11:57 On 12/18/2017 11:47 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to
avoid
multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-
imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
Changes in v2: None
Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes:
return mx53_dram_size[0];
return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);So, yes I am convinced that get_effective_memsize() still returns only the
size
of the first dram bank.
I suspect that will fails on M53 due to it's split-bank configuration. The board has two DRAM banks with a hole between them.
This is how mx53_dram_size was initialized on m53 before that patch:
mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);So to me that's, absolutely the same. With the only difference, that
get_ram_size(bank0)
is called multiple times, now.
The get_ram_size() above is called for two different bank (addresses), not for bank0 twice. Maybe I'm missing something.
btw where's the patch adding mx53_dram.c ? I don't see it in mainline yet.
It's #2 of this series: https://lists.denx.de/pipermail/u-boot/2017-December/314742.html
However, I would be fine with just keeping the changes to my board
(cx9020).
And if anyone has a better idea of what might be the root cause of [1]
and
[2],
I would absolute appreciate any input to that.
If your board also has two DRAM banks with a hole between them and you can test if this works fine, then I'm OK with this change.
Yes, cx9020 uses two 512MB banks with 1GB between PHYS_SDRAM_1/2,
too.
Then that should be the same situation. Can you share "bdinfo" from that board of yours?
=> bdinfo arch_number = 0x00000000 boot_params = 0x70000100 DRAM bank = 0x00000000 -> start = 0x70000000 -> size = 0x20000000 DRAM bank = 0x00000001 -> start = 0xB0000000 -> size = 0x20000000 eth0name = FEC ethaddr = 00:01:05:23:87:85 current eth = FEC ip_addr = <NULL> baudrate = 115200 bps TLB addr = 0x8FFF0000 relocaddr = 0x8FF8B000 reloc off = 0x1878B000 irq_sp = 0x8F586960 sp start = 0x8F586950 FB base = 0x8F58C7C0 Early malloc usage: 134 / 400 fdt_blob = 8f586978
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
Regards, Patrick --- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 12/18/2017 01:16 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 11:57 On 12/18/2017 11:47 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 10:17 On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn p.bruenn@beckhoff.com > > 'static uint32_t mx53_dram_size[2];' was used in board specific > dram_init(), dram_init_banksize() and get_effective_memsize() to
avoid
> multiple calls to get_ram_size(). > > Reused dram initialization functions from arch/arm/mach- imx/mx5/mx53_dram.c > > Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com > > --- > > Changes in v2: None > > > Only compile tested with make m53evk_defconfig
Are you sure this patch retains the original functionality ? Note the warning ...
Effectively it changes:
return mx53_dram_size[0];
return get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);So, yes I am convinced that get_effective_memsize() still returns only the
size
of the first dram bank.
I suspect that will fails on M53 due to it's split-bank configuration. The board has two DRAM banks with a hole between them.
This is how mx53_dram_size was initialized on m53 before that patch:
mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30);mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30);So to me that's, absolutely the same. With the only difference, that
get_ram_size(bank0)
is called multiple times, now.
The get_ram_size() above is called for two different bank (addresses), not for bank0 twice. Maybe I'm missing something.
btw where's the patch adding mx53_dram.c ? I don't see it in mainline yet.
It's #2 of this series: https://lists.denx.de/pipermail/u-boot/2017-December/314742.html
Ah, sorry, I missed that.
However, I would be fine with just keeping the changes to my board
(cx9020).
And if anyone has a better idea of what might be the root cause of [1]
and
[2],
I would absolute appreciate any input to that.
If your board also has two DRAM banks with a hole between them and you can test if this works fine, then I'm OK with this change.
Yes, cx9020 uses two 512MB banks with 1GB between PHYS_SDRAM_1/2,
too.
Then that should be the same situation. Can you share "bdinfo" from that board of yours?
=> bdinfo arch_number = 0x00000000 boot_params = 0x70000100 DRAM bank = 0x00000000 -> start = 0x70000000 -> size = 0x20000000 DRAM bank = 0x00000001 -> start = 0xB0000000 -> size = 0x20000000 eth0name = FEC ethaddr = 00:01:05:23:87:85 current eth = FEC ip_addr = <NULL> baudrate = 115200 bps TLB addr = 0x8FFF0000 relocaddr = 0x8FF8B000 reloc off = 0x1878B000 irq_sp = 0x8F586960 sp start = 0x8F586950 FB base = 0x8F58C7C0 Early malloc usage: 134 / 400 fdt_blob = 8f586978
Looks fine then.
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
The other is that you can ie. skip the whole u-boot altogether and boot linux directly.
I wonder if it would be better to keep the static variable and avoid calling the get_ram_size() twice, it can save some CPU cycles. Besides that, thanks for the explanation/discussion !1
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 13:30 On 12/18/2017 01:16 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
[...]
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
The other is that you can ie. skip the whole u-boot altogether and boot linux directly.
Okay, I will try that.
I wonder if it would be better to keep the static variable and avoid calling the get_ram_size() twice, it can save some CPU cycles. Besides that, thanks for the explanation/discussion !1
The pleasure was all mine. I will test SPL on my board and in case that's working I think it still makes sense to move our common code into one file. Is arch/arm/mach-imx/mx53_dram.c the best location for it? At first I tried board/freescale/common/ but in that case I needed to use ugly relative pathes in the Makefiles "../../"
Regards, Patrick
--- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 12/19/2017 05:28 AM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 13:30 On 12/18/2017 01:16 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
[...]
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
The other is that you can ie. skip the whole u-boot altogether and boot linux directly.
Okay, I will try that.
I wonder if it would be better to keep the static variable and avoid calling the get_ram_size() twice, it can save some CPU cycles. Besides that, thanks for the explanation/discussion !1
The pleasure was all mine. I will test SPL on my board and in case that's working I think it still makes sense to move our common code into one file. Is arch/arm/mach-imx/mx53_dram.c the best location for it? At first I tried board/freescale/common/ but in that case I needed to use ugly relative pathes in the Makefiles "../../"
I think arch/arm/mach-imx/ is good , yes . We already have some imx6 ddr there too, maybe you can follow that scheme.
Regards, Patrick
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
From: Patrick Brünn Sent: Dienstag, 19. Dezember 2017 05:29
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 13:30 On 12/18/2017 01:16 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
[...]
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
The other is that you can ie. skip the whole u-boot altogether and boot linux directly.
Okay, I will try that.
I wonder if it would be better to keep the static variable and avoid calling the get_ram_size() twice, it can save some CPU cycles. Besides that, thanks for the explanation/discussion !1
The pleasure was all mine. I will test SPL on my board and in case that's working I think it still makes sense to move our common code into one file. Is arch/arm/mach-imx/mx53_dram.c the best location for it? At first I tried board/freescale/common/ but in that case I needed to use ugly relative pathes in the Makefiles "../../"
I spend a few hours this morning to experiment with SPL. From what I saw it would require some additional patches to enable SPL_MMC for imx53 and a few more changes to our sdcard layout (FAT vs. EXT4). Seeing all these additional #ifdefs to consider I just don't have a good feeling to port that adhoc for our board. Directly booting Linux still sounds tempting, but for the moment I cannot spend more effort into SPL for mx53cx9020. I will put that on my todo list. But for now I would like to concentrate to get mainline boot on mx53cx9020, again.
I would suggest: 1. I keep m53evk.c untouched for now. 2a. Either patch both mx53cx9020 and mx53loco to avoid static mx53_dram_size, and have the shared code in arch/arm/mach-imx/mx53_dram.c 2b. Or patch only board/beckhoff/mx53cx9020/mx53cx9020.c
Fabio, what do you prefer?
Regards, Patrick
--- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 12/19/2017 08:48 AM, Patrick Brünn wrote:
From: Patrick Brünn Sent: Dienstag, 19. Dezember 2017 05:29
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 13:30 On 12/18/2017 01:16 PM, Patrick Brünn wrote:
From: Marek Vasut [mailto:marex@denx.de] Sent: Montag, 18. Dezember 2017 12:52 On 12/18/2017 12:40 PM, Patrick Brünn wrote:
[...]
btw do you use SPL ? If not, you should ...
I will read more about SPL. Until now, I thought I will only benefit, if my initial boot media is limited in size. As we boot from sdcard, that wasn't a problem to store 360k u-boot.
The other is that you can ie. skip the whole u-boot altogether and boot linux directly.
Okay, I will try that.
I wonder if it would be better to keep the static variable and avoid calling the get_ram_size() twice, it can save some CPU cycles. Besides that, thanks for the explanation/discussion !1
The pleasure was all mine. I will test SPL on my board and in case that's working I think it still makes sense to move our common code into one file. Is arch/arm/mach-imx/mx53_dram.c the best location for it? At first I tried board/freescale/common/ but in that case I needed to use ugly relative pathes in the Makefiles "../../"
I spend a few hours this morning to experiment with SPL. From what I saw it would require some additional patches to enable SPL_MMC for imx53 and a few more changes to our sdcard layout (FAT vs. EXT4). Seeing all these additional #ifdefs to consider I just don't have a good feeling to port that adhoc for our board. Directly booting Linux still sounds tempting, but for the moment I cannot spend more effort into SPL for mx53cx9020. I will put that on my todo list. But for now I would like to concentrate to get mainline boot on mx53cx9020, again.
I would suggest:
- I keep m53evk.c untouched for now.
2a. Either patch both mx53cx9020 and mx53loco to avoid static mx53_dram_size, and have the shared code in arch/arm/mach-imx/mx53_dram.c 2b. Or patch only board/beckhoff/mx53cx9020/mx53cx9020.c
Fabio, what do you prefer?
Just patch the m53evk , use the static variable and all should be good IMO.
Hi,
On Mon, 18 Dec 2017 10:17:03 +0100 Marek Vasut wrote:
On 12/18/2017 10:02 AM, linux-kernel-dev@beckhoff.com wrote:
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f().
They are, since the board runs from RAM at that point already.
That's not the point. Zero-initialized variables (static or not) are located in the .bss section, which is overlayed by the .rel.dyn section. Thus writing to such a variable before relocation will trash the relocation data.
Lothar Waßmann
From: Patrick Bruenn p.bruenn@beckhoff.com
Static variables are not available during board_init_f(). 'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to avoid multiple calls to get_ram_size().
Reused dram initialization functions from arch/arm/mach-imx/mx5/mx53_dram.c
Signed-off-by: Patrick Bruenn p.bruenn@beckhoff.com
---
Changes in v2: None
Only compile tested with make mx53loco_defconfig
--- arch/arm/mach-imx/mx5/Makefile | 1 + board/freescale/mx53loco/mx53loco.c | 39 ------------------------------------- 2 files changed, 1 insertion(+), 39 deletions(-)
diff --git a/arch/arm/mach-imx/mx5/Makefile b/arch/arm/mach-imx/mx5/Makefile index 2cc2cbc07a..4e305e92cf 100644 --- a/arch/arm/mach-imx/mx5/Makefile +++ b/arch/arm/mach-imx/mx5/Makefile @@ -13,3 +13,4 @@ obj-y += lowlevel_init.o # common files for mx53 dram initialization obj-$(CONFIG_TARGET_M53EVK) += mx53_dram.o obj-$(CONFIG_TARGET_MX53CX9020) += mx53_dram.o +obj-$(CONFIG_TARGET_MX53LOCO) += mx53_dram.o diff --git a/board/freescale/mx53loco/mx53loco.c b/board/freescale/mx53loco/mx53loco.c index db0e2fbdd6..0beecf3459 100644 --- a/board/freescale/mx53loco/mx53loco.c +++ b/board/freescale/mx53loco/mx53loco.c @@ -31,45 +31,6 @@
DECLARE_GLOBAL_DATA_PTR;
-static uint32_t mx53_dram_size[2]; - -phys_size_t get_effective_memsize(void) -{ - /* - * WARNING: We must override get_effective_memsize() function here - * to report only the size of the first DRAM bank. This is to make - * U-Boot relocator place U-Boot into valid memory, that is, at the - * end of the first DRAM bank. If we did not override this function - * like so, U-Boot would be placed at the address of the first DRAM - * bank + total DRAM size - sizeof(uboot), which in the setup where - * each DRAM bank contains 512MiB of DRAM would result in placing - * U-Boot into invalid memory area close to the end of the first - * DRAM bank. - */ - return mx53_dram_size[0]; -} - -int dram_init(void) -{ - mx53_dram_size[0] = get_ram_size((void *)PHYS_SDRAM_1, 1 << 30); - mx53_dram_size[1] = get_ram_size((void *)PHYS_SDRAM_2, 1 << 30); - - gd->ram_size = mx53_dram_size[0] + mx53_dram_size[1]; - - return 0; -} - -int dram_init_banksize(void) -{ - gd->bd->bi_dram[0].start = PHYS_SDRAM_1; - gd->bd->bi_dram[0].size = mx53_dram_size[0]; - - gd->bd->bi_dram[1].start = PHYS_SDRAM_2; - gd->bd->bi_dram[1].size = mx53_dram_size[1]; - - return 0; -} - u32 get_board_rev(void) { struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
participants (4)
-
linux-kernel-dev＠beckhoff.com -
Lothar Waßmann -
Marek Vasut -
Patrick Brünn