| Message ID | 20171218090242.16527-4-linux-kernel-dev@beckhoff.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | arm: imx53: remove usage of mx53_dram_size | expand |
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
>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
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
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 > >
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: > 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.
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[] = {