Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-19 Thread Patrick Brünn 
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Montag, 18. Dezember 2017 13:30
>On 12/18/2017 01:16 PM, Patrick Brünn wrote:
>>> From: Marek Vasut [mailto:ma...@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


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-19 Thread Patrick Brünn 
>From: Patrick Brünn
>Sent: Dienstag, 19. Dezember 2017 05:29
>>From: Marek Vasut [mailto:ma...@denx.de]
>>Sent: Montag, 18. Dezember 2017 13:30
>>On 12/18/2017 01:16 PM, Patrick Brünn wrote:
 From: Marek Vasut [mailto:ma...@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


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-19 Thread Marek Vasut 
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:ma...@denx.de]
>>> Sent: Montag, 18. Dezember 2017 13:30
>>> On 12/18/2017 01:16 PM, Patrick Brünn wrote:
> From: Marek Vasut [mailto:ma...@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?
Just patch the m53evk , use the static variable and all should be good IMO.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-19 Thread Marek Vasut 
On 12/19/2017 05:28 AM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Montag, 18. Dezember 2017 13:30
>> On 12/18/2017 01:16 PM, Patrick Brünn wrote:
 From: Marek Vasut [mailto:ma...@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
> 
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-19 Thread Lothar Waßmann 
Hi,

On Mon, 18 Dec 2017 10:17:03 +0100 Marek Vasut wrote:
> On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
> > From: Patrick Bruenn 
> > 
> > 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Patrick Brünn 
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Montag, 18. Dezember 2017 11:57
>On 12/18/2017 11:47 AM, Patrick Brünn wrote:
>>> From: Marek Vasut [mailto:ma...@denx.de]
>>> Sent: Montag, 18. Dezember 2017 10:17
>>> On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
 From: Patrick Bruenn 

 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 

 ---

 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


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Patrick Brünn 
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Montag, 18. Dezember 2017 12:52
>On 12/18/2017 12:40 PM, Patrick Brünn wrote:
>>> From: Marek Vasut [mailto:ma...@denx.de]
>>> Sent: Montag, 18. Dezember 2017 11:57
>>> On 12/18/2017 11:47 AM, Patrick Brünn wrote:
> From: Marek Vasut [mailto:ma...@denx.de]
> Sent: Montag, 18. Dezember 2017 10:17
> On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
>> From: Patrick Bruenn 
>>
>> '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 
>>
>> ---
>>
>> 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 = 0x
boot_params = 0x7100
DRAM bank   = 0x
-> start= 0x7000
-> size = 0x2000
DRAM bank   = 0x0001
-> start= 0xB000
-> size = 0x2000
eth0name= FEC
ethaddr = 00:01:05:23:87:85
current eth = FEC
ip_addr = 
baudrate= 115200 bps
TLB addr= 0x8FFF
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


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Patrick Brünn 
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Montag, 18. Dezember 2017 10:17
>On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
>> From: Patrick Bruenn 
>>
>> 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 
>>
>> ---
>>
>> 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


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Marek Vasut 
On 12/18/2017 01:16 PM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Montag, 18. Dezember 2017 12:52
>> On 12/18/2017 12:40 PM, Patrick Brünn wrote:
 From: Marek Vasut [mailto:ma...@denx.de]
 Sent: Montag, 18. Dezember 2017 11:57
 On 12/18/2017 11:47 AM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Montag, 18. Dezember 2017 10:17
>> On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
>>> From: Patrick Bruenn 
>>>
>>> '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 
>>>
>>> ---
>>>
>>> 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 = 0x
> boot_params = 0x7100
> DRAM bank   = 0x
> -> start= 0x7000
> -> size = 0x2000
> DRAM bank   = 0x0001
> -> start= 0xB000
> -> size = 0x2000
> eth0name= FEC
> ethaddr = 00:01:05:23:87:85
> current eth = FEC
> ip_addr = 
> baudrate= 115200 bps
> TLB addr= 0x8FFF
> 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

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Marek Vasut 
On 12/18/2017 12:40 PM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Montag, 18. Dezember 2017 11:57
>> On 12/18/2017 11:47 AM, Patrick Brünn wrote:
 From: Marek Vasut [mailto:ma...@denx.de]
 Sent: Montag, 18. Dezember 2017 10:17
 On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
> From: Patrick Bruenn 
>
> 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 
>
> ---
>
> 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 ...

[...]

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Marek Vasut 
On 12/18/2017 11:47 AM, Patrick Brünn wrote:
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Montag, 18. Dezember 2017 10:17
>> On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
>>> From: Patrick Bruenn 
>>>
>>> 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 
>>>
>>> ---
>>>
>>> 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
> 
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread Marek Vasut 
On 12/18/2017 10:02 AM, linux-kernel-...@beckhoff.com wrote:
> From: Patrick Bruenn 
> 
> 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 
> 
> ---
> 
> 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[] = {
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 3/4] arm: imx: m53evk: remove usage of mx53_dram_size

2017-12-18 Thread linux-kernel-dev 
From: Patrick Bruenn 

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 

---

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[] = {
-- 
2.11.0


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot