arm: imx53: remove usage of mx53_dram_size

From: Patrick Bruenn <[email protected]>

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 <[email protected]>

---

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

arch/arm/mach-imx/mx5/Makefile +1 -0

index 368cfde98b..2cc2cbc07a 100644
@@ -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

board/aries/m53evk/m53evk.c +0 -39

index ece8957aaf..a798d83376 100644
@@ -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 at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 3 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

Static variables are not available during board_init_f().

They are, since the board runs from RAM at that point already.

Quoted 5 lines

'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

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 4 lines

Changes in v2: None

Only compile tested with make m53evk_defconfig

Are you sure this patch retains the original functionality ? Note the warning ...

Quoted 7 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 3 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

Static variables are not available during board_init_f().

Quoted 2 lines

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.

Quoted 11 lines

Quoted 5 lines

'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

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

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

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 11:57

Quoted 7 lines · Patrick Brünn

Quoted 7 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 3 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

Static variables are not available during board_init_f().

Quoted 7 lines

Quoted 2 lines

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.

Quoted 23 lines

Quoted 11 lines

Quoted 5 lines

'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

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

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

Quoted 1 line

So, yes I am convinced that get_effective_memsize() still returns only the

size

Quoted 1 line

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.

Quoted 1 line

However, I would be fine with just keeping the changes to my board

(cx9020).

Quoted 1 line

And if anyone has a better idea of what might be the root cause of [1] and

[2],

Quoted 1 line

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.

Quoted 8 lines

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

Quoted 3 lines

Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

--
Best regards,
Marek Vasut

Quoted 18 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 11:57

Quoted 14 lines · Patrick Brünn

Quoted 9 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 3 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

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.

Quoted 4 lines

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.

Quoted 23 lines

Quoted 23 lines

Quoted 11 lines

Quoted 5 lines

'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

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 1 line

So, yes I am convinced that get_effective_memsize() still returns only the

size

Quoted 1 line

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:

board/aries/m53evk/m53evk.c +0 -2

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

Quoted 9 lines

Quoted 1 line

However, I would be fine with just keeping the changes to my board

(cx9020).

Quoted 1 line

And if anyone has a better idea of what might be the root cause of [1] and

[2],

Quoted 1 line

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.

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 12:52

Quoted 10 lines · Patrick Brünn

Quoted 10 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 11:57

Quoted 7 lines · Patrick Brünn

Quoted 7 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 4 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to

avoid

Quoted 32 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

Quoted 3 lines

multiple calls to get_ram_size().

Reused dram initialization functions from arch/arm/mach-

imx/mx5/mx53_dram.c

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 1 line

So, yes I am convinced that get_effective_memsize() still returns only the

size

Quoted 1 line

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:

board/aries/m53evk/m53evk.c +0 -2

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

Quoted 1 line

So to me that's, absolutely the same. With the only difference, that

get_ram_size(bank0)

Quoted 1 line

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.

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

However, I would be fine with just keeping the changes to my board

(cx9020).

Quoted 1 line

And if anyone has a better idea of what might be the root cause of [1]

and

Quoted 3 lines

Quoted 3 lines

[2],

Quoted 1 line

I would absolute appreciate any input to that.

Quoted 4 lines

Quoted 3 lines

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

Quoted 50 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 12:52

Quoted 10 lines · Patrick Brünn

Quoted 10 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 11:57

Quoted 7 lines · Patrick Brünn

Quoted 7 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 10:17

Quoted 4 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

'static uint32_t mx53_dram_size[2];' was used in board specific dram_init(), dram_init_banksize() and get_effective_memsize() to

avoid

Quoted 32 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

Quoted 3 lines

multiple calls to get_ram_size().

Reused dram initialization functions from arch/arm/mach-

imx/mx5/mx53_dram.c

Quoted 5 lines

Signed-off-by Patrick Bruenn <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 1 line

So, yes I am convinced that get_effective_memsize() still returns only the

size

Quoted 1 line

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:

(quoted diff) +0 -2

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

Quoted 1 line

So to me that's, absolutely the same. With the only difference, that

get_ram_size(bank0)

Quoted 1 line

is called multiple times, now.

Quoted 5 lines

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

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

However, I would be fine with just keeping the changes to my board

(cx9020).

Quoted 1 line

And if anyone has a better idea of what might be the root cause of [1]

and

Quoted 3 lines

Quoted 3 lines

[2],

Quoted 1 line

I would absolute appreciate any input to that.

Quoted 9 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 1 line

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.

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 13:30

Quoted 3 lines · Patrick Brünn

Quoted 3 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 12:52
On 12/18/2017 12:40 PM, Patrick Brünn wrote:

Quoted 4 lines

Quoted 1 line

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: Patrick Brünn
Sent: Dienstag, 19. Dezember 2017 05:29

Quoted 6 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 13:30

Quoted 3 lines · Patrick Brünn

Quoted 3 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 12:52
On 12/18/2017 12:40 PM, Patrick Brünn wrote:

[...]

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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 "../../"

Quoted 3 lines · linux-kernel-dev at beckhoff.com

From: Patrick Bruenn <[email protected]>

Static variables are not available during board_init_f().

They are, since the board runs from RAM at that point already.

Quoted 6 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 13:30

Quoted 3 lines · Patrick Brünn

Quoted 3 lines

From: Marek Vasut [mailto:marex at denx.de]
Sent: Montag, 18. Dezember 2017 12:52
On 12/18/2017 12:40 PM, Patrick Brünn wrote:

[...]

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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