| Message ID | 1479717007-20170-1-git-send-email-mike.looijmans@topic.nl |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Mike, On 21.11.2016 09:30, Mike Looijmans wrote: > Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring > separate bootloaders for each variant, just detect the RAM size at boot time > instead of relying on the devicetree information. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- > board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ > configs/topic_miami_defconfig | 1 + > configs/topic_miamiplus_defconfig | 1 + > 3 files changed, 41 insertions(+) > > diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c > index a95c9d1..8a5765e 100644 > --- a/board/topic/zynq/board.c > +++ b/board/topic/zynq/board.c > @@ -1 +1,40 @@ > +/* > + * (C) Copyright 2016 Topic Embedded Products > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +/* > + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing > + * separate bootloaders for each variant, just detect the RAM size at boot time > + * instead of relying on the devicetree information. > + */ > +#define CONFIG_SYS_SDRAM_BASE 0 > +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() > +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u > + > +static unsigned int topic_get_sdram_size(void); > + > #include "../../xilinx/zynq/board.c" > + > +#include <fdt_support.h> > + > +int ft_board_setup(void *blob, bd_t *bd) > +{ > + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); > + > + return 0; > +} > + > +void dram_init_banksize(void) > +{ > + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; > + gd->bd->bi_dram[0].size = gd->ram_size; > +} > + > +unsigned int topic_get_sdram_size(void) > +{ > + /* Detect and fix ram size */ > + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, > + CONFIG_SYS_SDRAM_SIZE_MAX); > +} > diff --git a/configs/topic_miami_defconfig b/configs/topic_miami_defconfig > index 3d6161e..8a02668 100644 > --- a/configs/topic_miami_defconfig > +++ b/configs/topic_miami_defconfig > @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y > CONFIG_CMD_FAT=y > CONFIG_CMD_FS_GENERIC=y > CONFIG_OF_EMBED=y > +CONFIG_OF_BOARD_SETUP=y > CONFIG_SPL_DM_SEQ_ALIAS=y > CONFIG_ZYNQ_SDHCI=y > CONFIG_SPI_FLASH=y > diff --git a/configs/topic_miamiplus_defconfig b/configs/topic_miamiplus_defconfig > index 3160f00..08bfab2 100644 > --- a/configs/topic_miamiplus_defconfig > +++ b/configs/topic_miamiplus_defconfig > @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y > CONFIG_CMD_FAT=y > CONFIG_CMD_FS_GENERIC=y > CONFIG_OF_EMBED=y > +CONFIG_OF_BOARD_SETUP=y > CONFIG_SPL_DM_SEQ_ALIAS=y > CONFIG_ZYNQ_SDHCI=y > CONFIG_SPI_FLASH=y > Sorry I am not getting this. I can't see that detection algorithm above and just looking at above description again that you have 256, 512 and 1G why not just run u-boot with 256MB - it should be enough for boot images and u-boot self, disable ARCH_FIXUP_FDT and update your bootscript to detect ram size and update dts by fdt commands? This will be much cleaner solution than code above. Thanks, Michal
On 21-11-16 10:04, Michal Simek wrote: > Hi Mike, > > On 21.11.2016 09:30, Mike Looijmans wrote: >> Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring >> separate bootloaders for each variant, just detect the RAM size at boot time >> instead of relying on the devicetree information. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ >> configs/topic_miami_defconfig | 1 + >> configs/topic_miamiplus_defconfig | 1 + >> 3 files changed, 41 insertions(+) >> >> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >> index a95c9d1..8a5765e 100644 >> --- a/board/topic/zynq/board.c >> +++ b/board/topic/zynq/board.c >> @@ -1 +1,40 @@ >> +/* >> + * (C) Copyright 2016 Topic Embedded Products >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +/* >> + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing >> + * separate bootloaders for each variant, just detect the RAM size at boot time >> + * instead of relying on the devicetree information. >> + */ >> +#define CONFIG_SYS_SDRAM_BASE 0 >> +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() >> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u >> + >> +static unsigned int topic_get_sdram_size(void); >> + >> #include "../../xilinx/zynq/board.c" >> + >> +#include <fdt_support.h> >> + >> +int ft_board_setup(void *blob, bd_t *bd) >> +{ >> + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); >> + >> + return 0; >> +} >> + >> +void dram_init_banksize(void) >> +{ >> + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; >> + gd->bd->bi_dram[0].size = gd->ram_size; >> +} >> + >> +unsigned int topic_get_sdram_size(void) >> +{ >> + /* Detect and fix ram size */ >> + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, >> + CONFIG_SYS_SDRAM_SIZE_MAX); >> +} >> diff --git a/configs/topic_miami_defconfig b/configs/topic_miami_defconfig >> index 3d6161e..8a02668 100644 >> --- a/configs/topic_miami_defconfig >> +++ b/configs/topic_miami_defconfig >> @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y >> CONFIG_CMD_FAT=y >> CONFIG_CMD_FS_GENERIC=y >> CONFIG_OF_EMBED=y >> +CONFIG_OF_BOARD_SETUP=y >> CONFIG_SPL_DM_SEQ_ALIAS=y >> CONFIG_ZYNQ_SDHCI=y >> CONFIG_SPI_FLASH=y >> diff --git a/configs/topic_miamiplus_defconfig b/configs/topic_miamiplus_defconfig >> index 3160f00..08bfab2 100644 >> --- a/configs/topic_miamiplus_defconfig >> +++ b/configs/topic_miamiplus_defconfig >> @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y >> CONFIG_CMD_FAT=y >> CONFIG_CMD_FS_GENERIC=y >> CONFIG_OF_EMBED=y >> +CONFIG_OF_BOARD_SETUP=y >> CONFIG_SPL_DM_SEQ_ALIAS=y >> CONFIG_ZYNQ_SDHCI=y >> CONFIG_SPI_FLASH=y >> > > > Sorry I am not getting this. I can't see that detection algorithm above > and just looking at above description again that you have 256, 512 and > 1G why not just run u-boot with 256MB - it should be enough for boot > images and u-boot self, disable ARCH_FIXUP_FDT and update your > bootscript to detect ram size and update dts by fdt commands? Lack of knowledge is one part. The only difference between the boards is the RAM chip installed on it. There's nothing else to detect which board you're running on. As far as I can see, it would be more difficult to perform this in a bootscript. I'd be happy to be proven wrong though. Plus, this way u-boot tells the truth at boot. Customers will yell at me if the bootloader reports only 256M at boot. > This will be much cleaner solution than code above. I only need 3 lines of code here to determine the memory configuration from the actual hardware. They look nice and clean to me. The original code in zynq/board.c needs a few dozen lines to fetch it from a devicetree. It could be made cleaner by replacing the 0x40000000 constant with reading the DDR config registers, but that'd be overkill and probably take more cycles than just running the test. And, from reading the documentation, calling get_ram_size is actually a good thing to do since it verifies that the memory actually works before using it. At this point I don't know how to proceed here. Mike. Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
On 21.11.2016 14:43, Mike Looijmans wrote: > On 21-11-16 10:04, Michal Simek wrote: >> Hi Mike, >> >> On 21.11.2016 09:30, Mike Looijmans wrote: >>> Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent >>> requiring >>> separate bootloaders for each variant, just detect the RAM size at >>> boot time >>> instead of relying on the devicetree information. >>> >>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >>> --- >>> board/topic/zynq/board.c | 39 >>> +++++++++++++++++++++++++++++++++++++++ >>> configs/topic_miami_defconfig | 1 + >>> configs/topic_miamiplus_defconfig | 1 + >>> 3 files changed, 41 insertions(+) >>> >>> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >>> index a95c9d1..8a5765e 100644 >>> --- a/board/topic/zynq/board.c >>> +++ b/board/topic/zynq/board.c >>> @@ -1 +1,40 @@ >>> +/* >>> + * (C) Copyright 2016 Topic Embedded Products >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +/* >>> + * Miami boards can have memory sizes of 256M, 512M or 1GB. To >>> prevent needing >>> + * separate bootloaders for each variant, just detect the RAM size >>> at boot time >>> + * instead of relying on the devicetree information. >>> + */ >>> +#define CONFIG_SYS_SDRAM_BASE 0 >>> +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() >>> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u >>> + >>> +static unsigned int topic_get_sdram_size(void); >>> + >>> #include "../../xilinx/zynq/board.c" >>> + >>> +#include <fdt_support.h> >>> + >>> +int ft_board_setup(void *blob, bd_t *bd) >>> +{ >>> + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, >>> (u64)gd->ram_size); >>> + >>> + return 0; >>> +} >>> + >>> +void dram_init_banksize(void) >>> +{ >>> + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; >>> + gd->bd->bi_dram[0].size = gd->ram_size; >>> +} >>> + >>> +unsigned int topic_get_sdram_size(void) >>> +{ >>> + /* Detect and fix ram size */ >>> + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, >>> + CONFIG_SYS_SDRAM_SIZE_MAX); >>> +} >>> diff --git a/configs/topic_miami_defconfig >>> b/configs/topic_miami_defconfig >>> index 3d6161e..8a02668 100644 >>> --- a/configs/topic_miami_defconfig >>> +++ b/configs/topic_miami_defconfig >>> @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y >>> CONFIG_CMD_FAT=y >>> CONFIG_CMD_FS_GENERIC=y >>> CONFIG_OF_EMBED=y >>> +CONFIG_OF_BOARD_SETUP=y >>> CONFIG_SPL_DM_SEQ_ALIAS=y >>> CONFIG_ZYNQ_SDHCI=y >>> CONFIG_SPI_FLASH=y >>> diff --git a/configs/topic_miamiplus_defconfig >>> b/configs/topic_miamiplus_defconfig >>> index 3160f00..08bfab2 100644 >>> --- a/configs/topic_miamiplus_defconfig >>> +++ b/configs/topic_miamiplus_defconfig >>> @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y >>> CONFIG_CMD_FAT=y >>> CONFIG_CMD_FS_GENERIC=y >>> CONFIG_OF_EMBED=y >>> +CONFIG_OF_BOARD_SETUP=y >>> CONFIG_SPL_DM_SEQ_ALIAS=y >>> CONFIG_ZYNQ_SDHCI=y >>> CONFIG_SPI_FLASH=y >>> >> >> >> Sorry I am not getting this. I can't see that detection algorithm above >> and just looking at above description again that you have 256, 512 and >> 1G why not just run u-boot with 256MB - it should be enough for boot >> images and u-boot self, disable ARCH_FIXUP_FDT and update your >> bootscript to detect ram size and update dts by fdt commands? > > Lack of knowledge is one part. > > The only difference between the boards is the RAM chip installed on it. > There's nothing else to detect which board you're running on. > > As far as I can see, it would be more difficult to perform this in a > bootscript. I'd be happy to be proven wrong though. > > Plus, this way u-boot tells the truth at boot. Customers will yell at me > if the bootloader reports only 256M at boot. > >> This will be much cleaner solution than code above. > > I only need 3 lines of code here to determine the memory configuration > from the actual hardware. They look nice and clean to me. The original > code in zynq/board.c needs a few dozen lines to fetch it from a devicetree. > > It could be made cleaner by replacing the 0x40000000 constant with > reading the DDR config registers, but that'd be overkill and probably > take more cycles than just running the test. And, from reading the > documentation, calling get_ram_size is actually a good thing to do since > it verifies that the memory actually works before using it. > > At this point I don't know how to proceed here. Let me go back to origin patch and clear some things there. Thanks, Michal
On 21.11.2016 09:30, Mike Looijmans wrote: > Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring > separate bootloaders for each variant, just detect the RAM size at boot time > instead of relying on the devicetree information. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- > board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ > configs/topic_miami_defconfig | 1 + > configs/topic_miamiplus_defconfig | 1 + > 3 files changed, 41 insertions(+) > > diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c > index a95c9d1..8a5765e 100644 > --- a/board/topic/zynq/board.c > +++ b/board/topic/zynq/board.c > @@ -1 +1,40 @@ > +/* > + * (C) Copyright 2016 Topic Embedded Products > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +/* > + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing > + * separate bootloaders for each variant, just detect the RAM size at boot time > + * instead of relying on the devicetree information. > + */ > +#define CONFIG_SYS_SDRAM_BASE 0 > +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() I am not happy with this but I see where you go. > +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u > + > +static unsigned int topic_get_sdram_size(void); > + > #include "../../xilinx/zynq/board.c" > + > +#include <fdt_support.h> > + > +int ft_board_setup(void *blob, bd_t *bd) > +{ > + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); > + > + return 0; > +} This action is taken at arch_fixup_fdt(). Can you please confirm that this is really needed? And it is not done there? That you don't duplicate stuff here. > + > +void dram_init_banksize(void) > +{ > + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; > + gd->bd->bi_dram[0].size = gd->ram_size; > +} This should be fine. > + > +unsigned int topic_get_sdram_size(void) > +{ > + /* Detect and fix ram size */ > + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, > + CONFIG_SYS_SDRAM_SIZE_MAX); In the first look I didn't know that get_ram_size is u-boot function for memsize detection. Thanks, Michal
On 22-11-16 12:00, Michal Simek wrote: > On 21.11.2016 09:30, Mike Looijmans wrote: >> Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring >> separate bootloaders for each variant, just detect the RAM size at boot time >> instead of relying on the devicetree information. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ >> configs/topic_miami_defconfig | 1 + >> configs/topic_miamiplus_defconfig | 1 + >> 3 files changed, 41 insertions(+) >> >> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >> index a95c9d1..8a5765e 100644 >> --- a/board/topic/zynq/board.c >> +++ b/board/topic/zynq/board.c >> @@ -1 +1,40 @@ >> +/* >> + * (C) Copyright 2016 Topic Embedded Products >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +/* >> + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing >> + * separate bootloaders for each variant, just detect the RAM size at boot time >> + * instead of relying on the devicetree information. >> + */ >> +#define CONFIG_SYS_SDRAM_BASE 0 >> +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() > > I am not happy with this but I see where you go. Of the several options I tried to "overload" the memory init functions, this was basically the lesser evil... Maybe it would be possible to make some changes to the generic Zynq board.c file to facilitate overlaying the code? Note that the memory detection would work on any board, not just the Topic boards. I agree that devicetree is considered the best thing since frozen pizza, but for the Zynq it looks as though detection is much simpler than static configuration. > >> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u >> + >> +static unsigned int topic_get_sdram_size(void); >> + >> #include "../../xilinx/zynq/board.c" >> + >> +#include <fdt_support.h> >> + >> +int ft_board_setup(void *blob, bd_t *bd) >> +{ >> + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); >> + >> + return 0; >> +} > > This action is taken at arch_fixup_fdt(). Can you please confirm that > this is really needed? And it is not done there? That you don't > duplicate stuff here. If I omitted this step, the Linux devicetree would not be adjusted (during bootm) and would still report its default. I needed both arch_fixup_fdt and this extra line to properly adjust the Linux devicetree for booting. > >> + >> +void dram_init_banksize(void) >> +{ >> + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; >> + gd->bd->bi_dram[0].size = gd->ram_size; >> +} > > This should be fine. > A note here, if you compile for Zynq with CONFIG_SYS_SDRAM_BASE and CONFIG_SYS_SDRAM_SIZE set, there will be no implementation of 'dram_init_banksize' and the system will fail to boot. It might be wise to put this snippet into the zynq/board.c in the "#else" clause. I'll send a separate patch for that if you agree. >> + >> +unsigned int topic_get_sdram_size(void) >> +{ >> + /* Detect and fix ram size */ >> + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, >> + CONFIG_SYS_SDRAM_SIZE_MAX); > > In the first look I didn't know that get_ram_size is u-boot function for > memsize detection. Indeed, the name "get_ram_size" doesn't really match what it actually does, but the function's documentation is quite good. I found "get_ram_size" and "ft_board_setup" while browsing the code of other boards to see how they handled dynamic configuration. > Thanks, > Michal > Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
Hi Mike, On 22.11.2016 12:00, Michal Simek wrote: > On 21.11.2016 09:30, Mike Looijmans wrote: >> Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring >> separate bootloaders for each variant, just detect the RAM size at boot time >> instead of relying on the devicetree information. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ >> configs/topic_miami_defconfig | 1 + >> configs/topic_miamiplus_defconfig | 1 + >> 3 files changed, 41 insertions(+) >> >> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >> index a95c9d1..8a5765e 100644 >> --- a/board/topic/zynq/board.c >> +++ b/board/topic/zynq/board.c >> @@ -1 +1,40 @@ >> +/* >> + * (C) Copyright 2016 Topic Embedded Products >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +/* >> + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing >> + * separate bootloaders for each variant, just detect the RAM size at boot time >> + * instead of relying on the devicetree information. >> + */ >> +#define CONFIG_SYS_SDRAM_BASE 0 >> +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() > > I am not happy with this but I see where you go. > >> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u >> + >> +static unsigned int topic_get_sdram_size(void); >> + >> #include "../../xilinx/zynq/board.c" >> + >> +#include <fdt_support.h> >> + >> +int ft_board_setup(void *blob, bd_t *bd) >> +{ >> + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); >> + >> + return 0; >> +} > > This action is taken at arch_fixup_fdt(). Can you please confirm that > this is really needed? And it is not done there? That you don't > duplicate stuff here. Did you check this? Thanks, Michal
On 11/22/16 16:54, Mike Looijmans wrote: > On 22-11-16 12:00, Michal Simek wrote: >> On 21.11.2016 09:30, Mike Looijmans wrote: >>> Miami boards can have memory sizes of 256M, 512M or 1GB. To >>> prevent requiring separate bootloaders for each variant, just >>> detect the RAM size at boot time instead of relying on the >>> devicetree information. >>> >>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- >>> board/topic/zynq/board.c | 39 >>> +++++++++++++++++++++++++++++++++++++++ >>> configs/topic_miami_defconfig | 1 + >>> configs/topic_miamiplus_defconfig | 1 + 3 files changed, 41 >>> insertions(+) >>> >>> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >>> index a95c9d1..8a5765e 100644 --- a/board/topic/zynq/board.c +++ >>> b/board/topic/zynq/board.c @@ -1 +1,40 @@ +/* + * (C) Copyright >>> 2016 Topic Embedded Products + * + * SPDX-License-Identifier: >>> GPL-2.0+ + */ + +/* + * Miami boards can have memory sizes of >>> 256M, 512M or 1GB. To prevent needing + * separate bootloaders >>> for each variant, just detect the RAM size at boot time + * >>> instead of relying on the devicetree information. + */ +#define >>> CONFIG_SYS_SDRAM_BASE 0 +#define CONFIG_SYS_SDRAM_SIZE >>> topic_get_sdram_size() >> >> I am not happy with this but I see where you go. > > Of the several options I tried to "overload" the memory init > functions, this was basically the lesser evil... > > Maybe it would be possible to make some changes to the generic Zynq > board.c file to facilitate overlaying the code? > > Note that the memory detection would work on any board, not just the > Topic boards. I agree that devicetree is considered the best thing > since frozen pizza, but for the Zynq it looks as though detection is > much simpler than static configuration. > >> >>> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u + +static unsigned >>> int topic_get_sdram_size(void); + #include >>> "../../xilinx/zynq/board.c" + +#include <fdt_support.h> + +int >>> ft_board_setup(void *blob, bd_t *bd) +{ + >>> fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, >>> (u64)gd->ram_size); + + return 0; +} >> >> This action is taken at arch_fixup_fdt(). Can you please confirm >> that this is really needed? And it is not done there? That you >> don't duplicate stuff here. > > If I omitted this step, the Linux devicetree would not be adjusted > (during bootm) and would still report its default. I needed both > arch_fixup_fdt and this extra line to properly adjust the Linux > devicetree for booting. > > >> >>> + +void dram_init_banksize(void) +{ + gd->bd->bi_dram[0].start >>> = CONFIG_SYS_SDRAM_BASE; + gd->bd->bi_dram[0].size = >>> gd->ram_size; +} >> >> This should be fine. >> > > A note here, if you compile for Zynq with CONFIG_SYS_SDRAM_BASE and > CONFIG_SYS_SDRAM_SIZE set, there will be no implementation of > 'dram_init_banksize' and the system will fail to boot. > > It might be wise to put this snippet into the zynq/board.c in the > "#else" clause. I'll send a separate patch for that if you agree. > > >>> + +unsigned int topic_get_sdram_size(void) +{ + /* Detect and >>> fix ram size */ + return get_ram_size((void >>> *)CONFIG_SYS_SDRAM_BASE, + >>> CONFIG_SYS_SDRAM_SIZE_MAX); >> >> In the first look I didn't know that get_ram_size is u-boot >> function for memsize detection. > > Indeed, the name "get_ram_size" doesn't really match what it actually > does, but the function's documentation is quite good. > > I found "get_ram_size" and "ft_board_setup" while browsing the code > of other boards to see how they handled dynamic configuration. +1 That's how things works in U-Boot for years...
On 12-12-16 08:46, Michal Simek wrote: > Hi Mike, > > On 22.11.2016 12:00, Michal Simek wrote: >> On 21.11.2016 09:30, Mike Looijmans wrote: >>> Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring >>> separate bootloaders for each variant, just detect the RAM size at boot time >>> instead of relying on the devicetree information. >>> >>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >>> --- >>> board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> configs/topic_miami_defconfig | 1 + >>> configs/topic_miamiplus_defconfig | 1 + >>> 3 files changed, 41 insertions(+) >>> >>> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c >>> index a95c9d1..8a5765e 100644 >>> --- a/board/topic/zynq/board.c >>> +++ b/board/topic/zynq/board.c >>> @@ -1 +1,40 @@ >>> +/* >>> + * (C) Copyright 2016 Topic Embedded Products >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +/* >>> + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing >>> + * separate bootloaders for each variant, just detect the RAM size at boot time >>> + * instead of relying on the devicetree information. >>> + */ >>> +#define CONFIG_SYS_SDRAM_BASE 0 >>> +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() >> >> I am not happy with this but I see where you go. >> >>> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u >>> + >>> +static unsigned int topic_get_sdram_size(void); >>> + >>> #include "../../xilinx/zynq/board.c" >>> + >>> +#include <fdt_support.h> >>> + >>> +int ft_board_setup(void *blob, bd_t *bd) >>> +{ >>> + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); >>> + >>> + return 0; >>> +} >> >> This action is taken at arch_fixup_fdt(). Can you please confirm that >> this is really needed? And it is not done there? That you don't >> duplicate stuff here. > > Did you check this? Well, if omitted, the linux devicetree doesn't get updated with the proper memory size, and the system malfunctions if the size differs. Apparently, the internal u-boot DT gets updated by the other routines, but this routine is required to patch the devicetree as it will be transferred to Linux. Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail
diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c index a95c9d1..8a5765e 100644 --- a/board/topic/zynq/board.c +++ b/board/topic/zynq/board.c @@ -1 +1,40 @@ +/* + * (C) Copyright 2016 Topic Embedded Products + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +/* + * Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent needing + * separate bootloaders for each variant, just detect the RAM size at boot time + * instead of relying on the devicetree information. + */ +#define CONFIG_SYS_SDRAM_BASE 0 +#define CONFIG_SYS_SDRAM_SIZE topic_get_sdram_size() +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u + +static unsigned int topic_get_sdram_size(void); + #include "../../xilinx/zynq/board.c" + +#include <fdt_support.h> + +int ft_board_setup(void *blob, bd_t *bd) +{ + fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE, (u64)gd->ram_size); + + return 0; +} + +void dram_init_banksize(void) +{ + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; + gd->bd->bi_dram[0].size = gd->ram_size; +} + +unsigned int topic_get_sdram_size(void) +{ + /* Detect and fix ram size */ + return get_ram_size((void *)CONFIG_SYS_SDRAM_BASE, + CONFIG_SYS_SDRAM_SIZE_MAX); +} diff --git a/configs/topic_miami_defconfig b/configs/topic_miami_defconfig index 3d6161e..8a02668 100644 --- a/configs/topic_miami_defconfig +++ b/configs/topic_miami_defconfig @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y CONFIG_CMD_FAT=y CONFIG_CMD_FS_GENERIC=y CONFIG_OF_EMBED=y +CONFIG_OF_BOARD_SETUP=y CONFIG_SPL_DM_SEQ_ALIAS=y CONFIG_ZYNQ_SDHCI=y CONFIG_SPI_FLASH=y diff --git a/configs/topic_miamiplus_defconfig b/configs/topic_miamiplus_defconfig index 3160f00..08bfab2 100644 --- a/configs/topic_miamiplus_defconfig +++ b/configs/topic_miamiplus_defconfig @@ -24,6 +24,7 @@ CONFIG_CMD_EXT4=y CONFIG_CMD_FAT=y CONFIG_CMD_FS_GENERIC=y CONFIG_OF_EMBED=y +CONFIG_OF_BOARD_SETUP=y CONFIG_SPL_DM_SEQ_ALIAS=y CONFIG_ZYNQ_SDHCI=y CONFIG_SPI_FLASH=y
Miami boards can have memory sizes of 256M, 512M or 1GB. To prevent requiring separate bootloaders for each variant, just detect the RAM size at boot time instead of relying on the devicetree information. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- board/topic/zynq/board.c | 39 +++++++++++++++++++++++++++++++++++++++ configs/topic_miami_defconfig | 1 + configs/topic_miamiplus_defconfig | 1 + 3 files changed, 41 insertions(+)