* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM @ 2018-05-12 20:27 Marek Vasut 2018-05-14 8:03 ` Simon Goldschmidt 0 siblings, 1 reply; 11+ messages in thread From: Marek Vasut @ 2018-05-12 20:27 UTC (permalink / raw) To: u-boot The global data are in the .data section, so there's no point in reserving any space for it above stack. Put stack at the end of SRAM. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Chin Liang See <chin.liang.see@intel.com> Cc: Dinh Nguyen <dinguyen@kernel.org> --- include/configs/socfpga_common.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 4de2aa7929..cb67d539b1 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -35,10 +35,8 @@ #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ #endif -#define CONFIG_SYS_INIT_SP_OFFSET \ - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) #define CONFIG_SYS_INIT_SP_ADDR \ - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) #define CONFIG_SYS_SDRAM_BASE PHYS_SDRAM_1 -- 2.16.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-12 20:27 [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM Marek Vasut @ 2018-05-14 8:03 ` Simon Goldschmidt 2018-05-14 8:17 ` Marek Vasut 0 siblings, 1 reply; 11+ messages in thread From: Simon Goldschmidt @ 2018-05-14 8:03 UTC (permalink / raw) To: u-boot On 12.05.2018 22:27, Marek Vasut wrote: > The global data are in the .data section, so there's no point in > reserving any space for it above stack. Put stack at the end of > SRAM. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Chin Liang See <chin.liang.see@intel.com> > Cc: Dinh Nguyen <dinguyen@kernel.org> > --- > include/configs/socfpga_common.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > index 4de2aa7929..cb67d539b1 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -35,10 +35,8 @@ > #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 > #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ > #endif > -#define CONFIG_SYS_INIT_SP_OFFSET \ > - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) > #define CONFIG_SYS_INIT_SP_ADDR \ > - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) > + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) I think this interferes with the bootcount being stored at the end of SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local defconfig here :-) I'd really like to keep some room here for bootcounter and some related things I store here. Simon > > #define CONFIG_SYS_SDRAM_BASE PHYS_SDRAM_1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 8:03 ` Simon Goldschmidt @ 2018-05-14 8:17 ` Marek Vasut 2018-05-14 9:01 ` Simon Goldschmidt 0 siblings, 1 reply; 11+ messages in thread From: Marek Vasut @ 2018-05-14 8:17 UTC (permalink / raw) To: u-boot On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: > On 12.05.2018 22:27, Marek Vasut wrote: >> The global data are in the .data section, so there's no point in >> reserving any space for it above stack. Put stack at the end of >> SRAM. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Chin Liang See <chin.liang.see@intel.com> >> Cc: Dinh Nguyen <dinguyen@kernel.org> >> --- >> include/configs/socfpga_common.h | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/include/configs/socfpga_common.h >> b/include/configs/socfpga_common.h >> index 4de2aa7929..cb67d539b1 100644 >> --- a/include/configs/socfpga_common.h >> +++ b/include/configs/socfpga_common.h >> @@ -35,10 +35,8 @@ >> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >> #endif >> -#define CONFIG_SYS_INIT_SP_OFFSET \ >> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >> #define CONFIG_SYS_INIT_SP_ADDR \ >> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) > > I think this interferes with the bootcount being stored at the end of > SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local > defconfig here :-) > > I'd really like to keep some room here for bootcounter and some related > things I store here. So if I understand it correctly, before this patch, the boards overwrote the global data ? Anyway, if you need some extra space, look at board_init_f_alloc_reserve() . -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 8:17 ` Marek Vasut @ 2018-05-14 9:01 ` Simon Goldschmidt 2018-05-14 9:06 ` Marek Vasut 0 siblings, 1 reply; 11+ messages in thread From: Simon Goldschmidt @ 2018-05-14 9:01 UTC (permalink / raw) To: u-boot On 14.05.2018 10:17, Marek Vasut wrote: > On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >> On 12.05.2018 22:27, Marek Vasut wrote: >>> The global data are in the .data section, so there's no point in >>> reserving any space for it above stack. Put stack at the end of >>> SRAM. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Chin Liang See <chin.liang.see@intel.com> >>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>> --- >>> include/configs/socfpga_common.h | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/include/configs/socfpga_common.h >>> b/include/configs/socfpga_common.h >>> index 4de2aa7929..cb67d539b1 100644 >>> --- a/include/configs/socfpga_common.h >>> +++ b/include/configs/socfpga_common.h >>> @@ -35,10 +35,8 @@ >>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>> #endif >>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>> #define CONFIG_SYS_INIT_SP_ADDR \ >>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >> >> I think this interferes with the bootcount being stored at the end of >> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >> defconfig here :-) >> >> I'd really like to keep some room here for bootcounter and some related >> things I store here. > > So if I understand it correctly, before this patch, the boards overwrote > the global data ? Ehrm, reading up on the init code path in the S and C files, it seems like that, yes. I just used the two boards as an example. Are they actively maintained? get_maintainer.pl shows the boards are maintained by denx, so maybe you could clarify this? Chances are high that these bytes just fit into the 16-byte alignment allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like that). > > Anyway, if you need some extra space, look at board_init_f_alloc_reserve() . > OK, but that function does not seem to be overridable? How can I reserve my own memory here? And reading this function, I guess your commit message is a bit misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like it was was to reserve memory for the initial gd (before relocation), which is now done in board_init_f_alloc_reserve(). Simon ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 9:01 ` Simon Goldschmidt @ 2018-05-14 9:06 ` Marek Vasut 2018-05-14 15:51 ` Stefan Roese 0 siblings, 1 reply; 11+ messages in thread From: Marek Vasut @ 2018-05-14 9:06 UTC (permalink / raw) To: u-boot On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: > > > On 14.05.2018 10:17, Marek Vasut wrote: >> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>> On 12.05.2018 22:27, Marek Vasut wrote: >>>> The global data are in the .data section, so there's no point in >>>> reserving any space for it above stack. Put stack at the end of >>>> SRAM. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>> --- >>>> include/configs/socfpga_common.h | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/include/configs/socfpga_common.h >>>> b/include/configs/socfpga_common.h >>>> index 4de2aa7929..cb67d539b1 100644 >>>> --- a/include/configs/socfpga_common.h >>>> +++ b/include/configs/socfpga_common.h >>>> @@ -35,10 +35,8 @@ >>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>> #endif >>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>> >>> I think this interferes with the bootcount being stored at the end of >>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>> defconfig here :-) >>> >>> I'd really like to keep some room here for bootcounter and some related >>> things I store here. >> >> So if I understand it correctly, before this patch, the boards overwrote >> the global data ? > > Ehrm, reading up on the init code path in the S and C files, it seems > like that, yes. I just used the two boards as an example. Are they > actively maintained? get_maintainer.pl shows the boards are maintained > by denx, so maybe you could clarify this? +CC Stefan. > Chances are high that these bytes just fit into the 16-byte alignment > allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like that). > >> >> Anyway, if you need some extra space, look at >> board_init_f_alloc_reserve() . >> > > OK, but that function does not seem to be overridable? How can I reserve > my own memory here? Add a __weak there I guess ? > And reading this function, I guess your commit message is a bit > misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like > it was was to reserve memory for the initial gd (before relocation), > which is now done in board_init_f_alloc_reserve(). The code is confusing as hell, I give you that. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 9:06 ` Marek Vasut @ 2018-05-14 15:51 ` Stefan Roese 2018-05-14 19:43 ` Simon Goldschmidt 0 siblings, 1 reply; 11+ messages in thread From: Stefan Roese @ 2018-05-14 15:51 UTC (permalink / raw) To: u-boot On 14.05.2018 11:06, Marek Vasut wrote: > On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >> >> >> On 14.05.2018 10:17, Marek Vasut wrote: >>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>> The global data are in the .data section, so there's no point in >>>>> reserving any space for it above stack. Put stack at the end of >>>>> SRAM. >>>>> >>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>> --- >>>>> include/configs/socfpga_common.h | 4 +--- >>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/include/configs/socfpga_common.h >>>>> b/include/configs/socfpga_common.h >>>>> index 4de2aa7929..cb67d539b1 100644 >>>>> --- a/include/configs/socfpga_common.h >>>>> +++ b/include/configs/socfpga_common.h >>>>> @@ -35,10 +35,8 @@ >>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>> #endif >>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>>> >>>> I think this interferes with the bootcount being stored at the end of >>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>> defconfig here :-) >>>> >>>> I'd really like to keep some room here for bootcounter and some related >>>> things I store here. >>> >>> So if I understand it correctly, before this patch, the boards overwrote >>> the global data ? >> >> Ehrm, reading up on the init code path in the S and C files, it seems >> like that, yes. I just used the two boards as an example. Are they >> actively maintained? get_maintainer.pl shows the boards are maintained >> by denx, so maybe you could clarify this? > > +CC Stefan. Thanks. At the time I added the bootcounter support on this platform, it used to work. I might have missed something though. Right now, I don't have access to such a platform, so its hard for me to test something. If you have some patches to fix any potential overlapping memory usages, I'll gladly review them. >> Chances are high that these bytes just fit into the 16-byte alignment >> allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like that). >> >>> >>> Anyway, if you need some extra space, look at >>> board_init_f_alloc_reserve() . >>> >> >> OK, but that function does not seem to be overridable? How can I reserve >> my own memory here? > > Add a __weak there I guess ? > >> And reading this function, I guess your commit message is a bit >> misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like >> it was was to reserve memory for the initial gd (before relocation), >> which is now done in board_init_f_alloc_reserve(). > > The code is confusing as hell, I give you that. Thanks, Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 15:51 ` Stefan Roese @ 2018-05-14 19:43 ` Simon Goldschmidt 2018-05-14 20:06 ` Simon Goldschmidt 2018-05-14 20:43 ` Marek Vasut 0 siblings, 2 replies; 11+ messages in thread From: Simon Goldschmidt @ 2018-05-14 19:43 UTC (permalink / raw) To: u-boot On 14.05.2018 17:51, Stefan Roese wrote: > On 14.05.2018 11:06, Marek Vasut wrote: >> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >>> >>> >>> On 14.05.2018 10:17, Marek Vasut wrote: >>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>>> The global data are in the .data section, so there's no point in >>>>>> reserving any space for it above stack. Put stack at the end of >>>>>> SRAM. >>>>>> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>>> --- >>>>>> include/configs/socfpga_common.h | 4 +--- >>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/include/configs/socfpga_common.h >>>>>> b/include/configs/socfpga_common.h >>>>>> index 4de2aa7929..cb67d539b1 100644 >>>>>> --- a/include/configs/socfpga_common.h >>>>>> +++ b/include/configs/socfpga_common.h >>>>>> @@ -35,10 +35,8 @@ >>>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>>> #endif >>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>>>> >>>>> I think this interferes with the bootcount being stored at the end of >>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>>> defconfig here :-) >>>>> >>>>> I'd really like to keep some room here for bootcounter and some >>>>> related >>>>> things I store here. >>>> >>>> So if I understand it correctly, before this patch, the boards >>>> overwrote >>>> the global data ? >>> >>> Ehrm, reading up on the init code path in the S and C files, it seems >>> like that, yes. I just used the two boards as an example. Are they >>> actively maintained? get_maintainer.pl shows the boards are maintained >>> by denx, so maybe you could clarify this? >> >> +CC Stefan. > > Thanks. At the time I added the bootcounter support on this platform, > it used to work. I might have missed something though. Right now, > I don't have access to such a platform, so its hard for me to test > something. If you have some patches to fix any potential overlapping > memory usages, I'll gladly review them. I just checked out ae996c80 and from what I can tell, the RAM layout was the same at that time: include/generated/generated-asm-offsets.h contains: - #define GENERATED_GBL_DATA_SIZE 208 - #define GD_SIZE 200 That leaves us with an 8 byte alignment gap at the end of the SRAM and that's where the boot counter is located. So the global data is not overwritten, but this is more or less by chance, any change to the struct definition might change this. Looking at just another example: the x600 board defines its SRAM_SIZE 8 bytes smaller than what it really is. That seems like a good idea for socfpga, too. I'll see if I can provide a patch. Slightly off-topic: I have a constellation where the boot-counter is not reset although the power has been pulled. This device has big capacitors and the SRAM content seems to survive although the processor is reset. To me it seems as if looking at the boot reason might be safer than checking for a magic word... Any ideas? Thanks, Simon > >>> Chances are high that these bytes just fit into the 16-byte alignment >>> allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like >>> that). >>> >>>> >>>> Anyway, if you need some extra space, look at >>>> board_init_f_alloc_reserve() . >>>> >>> >>> OK, but that function does not seem to be overridable? How can I reserve >>> my own memory here? >> >> Add a __weak there I guess ? >> >>> And reading this function, I guess your commit message is a bit >>> misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like >>> it was was to reserve memory for the initial gd (before relocation), >>> which is now done in board_init_f_alloc_reserve(). >> >> The code is confusing as hell, I give you that. > > Thanks, > Stefan > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 19:43 ` Simon Goldschmidt @ 2018-05-14 20:06 ` Simon Goldschmidt 2018-05-14 20:43 ` Marek Vasut 1 sibling, 0 replies; 11+ messages in thread From: Simon Goldschmidt @ 2018-05-14 20:06 UTC (permalink / raw) To: u-boot On 14.05.2018 21:43, Simon Goldschmidt wrote: > > > On 14.05.2018 17:51, Stefan Roese wrote: >> On 14.05.2018 11:06, Marek Vasut wrote: >>> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >>>> >>>> >>>> On 14.05.2018 10:17, Marek Vasut wrote: >>>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>>>> The global data are in the .data section, so there's no point in >>>>>>> reserving any space for it above stack. Put stack at the end of >>>>>>> SRAM. >>>>>>> >>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>>>> --- >>>>>>> include/configs/socfpga_common.h | 4 +--- >>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>> b/include/configs/socfpga_common.h >>>>>>> index 4de2aa7929..cb67d539b1 100644 >>>>>>> --- a/include/configs/socfpga_common.h >>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>> @@ -35,10 +35,8 @@ >>>>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>>>> #endif >>>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) Anyway, after reading up on this in the code and the resulting binaries, I do think this patch is correct, so: Reviewed-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> Sorry for the confusion, but this really helped to clarify my understanding of the initial memory allocation ;-) Thanks, Simon >>>>>> >>>>>> I think this interferes with the bootcount being stored at the end of >>>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>>>> defconfig here :-) >>>>>> >>>>>> I'd really like to keep some room here for bootcounter and some >>>>>> related >>>>>> things I store here. >>>>> >>>>> So if I understand it correctly, before this patch, the boards >>>>> overwrote >>>>> the global data ? >>>> >>>> Ehrm, reading up on the init code path in the S and C files, it seems >>>> like that, yes. I just used the two boards as an example. Are they >>>> actively maintained? get_maintainer.pl shows the boards are maintained >>>> by denx, so maybe you could clarify this? >>> >>> +CC Stefan. >> >> Thanks. At the time I added the bootcounter support on this platform, >> it used to work. I might have missed something though. Right now, >> I don't have access to such a platform, so its hard for me to test >> something. If you have some patches to fix any potential overlapping >> memory usages, I'll gladly review them. > > I just checked out ae996c80 and from what I can tell, the RAM layout was > the same at that time: include/generated/generated-asm-offsets.h contains: > - #define GENERATED_GBL_DATA_SIZE 208 > - #define GD_SIZE 200 > > That leaves us with an 8 byte alignment gap at the end of the SRAM and > that's where the boot counter is located. So the global data is not > overwritten, but this is more or less by chance, any change to the > struct definition might change this. > > Looking at just another example: the x600 board defines its SRAM_SIZE 8 > bytes smaller than what it really is. That seems like a good idea for > socfpga, too. I'll see if I can provide a patch. > > Slightly off-topic: I have a constellation where the boot-counter is not > reset although the power has been pulled. This device has big capacitors > and the SRAM content seems to survive although the processor is reset. > To me it seems as if looking at the boot reason might be safer than > checking for a magic word... Any ideas? > > Thanks, > Simon > >> >>>> Chances are high that these bytes just fit into the 16-byte alignment >>>> allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like >>>> that). >>>> >>>>> >>>>> Anyway, if you need some extra space, look at >>>>> board_init_f_alloc_reserve() . >>>>> >>>> >>>> OK, but that function does not seem to be overridable? How can I >>>> reserve >>>> my own memory here? >>> >>> Add a __weak there I guess ? >>> >>>> And reading this function, I guess your commit message is a bit >>>> misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like >>>> it was was to reserve memory for the initial gd (before relocation), >>>> which is now done in board_init_f_alloc_reserve(). >>> >>> The code is confusing as hell, I give you that. >> >> Thanks, >> Stefan >> > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > https://lists.denx.de/listinfo/u-boot ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 19:43 ` Simon Goldschmidt 2018-05-14 20:06 ` Simon Goldschmidt @ 2018-05-14 20:43 ` Marek Vasut 2018-05-15 7:26 ` Simon Goldschmidt 1 sibling, 1 reply; 11+ messages in thread From: Marek Vasut @ 2018-05-14 20:43 UTC (permalink / raw) To: u-boot On 05/14/2018 09:43 PM, Simon Goldschmidt wrote: > > > On 14.05.2018 17:51, Stefan Roese wrote: >> On 14.05.2018 11:06, Marek Vasut wrote: >>> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >>>> >>>> >>>> On 14.05.2018 10:17, Marek Vasut wrote: >>>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>>>> The global data are in the .data section, so there's no point in >>>>>>> reserving any space for it above stack. Put stack at the end of >>>>>>> SRAM. >>>>>>> >>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>>>> --- >>>>>>> include/configs/socfpga_common.h | 4 +--- >>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>> b/include/configs/socfpga_common.h >>>>>>> index 4de2aa7929..cb67d539b1 100644 >>>>>>> --- a/include/configs/socfpga_common.h >>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>> @@ -35,10 +35,8 @@ >>>>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>>>> #endif >>>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>>>>> >>>>>> I think this interferes with the bootcount being stored at the end of >>>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>>>> defconfig here :-) >>>>>> >>>>>> I'd really like to keep some room here for bootcounter and some >>>>>> related >>>>>> things I store here. >>>>> >>>>> So if I understand it correctly, before this patch, the boards >>>>> overwrote >>>>> the global data ? >>>> >>>> Ehrm, reading up on the init code path in the S and C files, it seems >>>> like that, yes. I just used the two boards as an example. Are they >>>> actively maintained? get_maintainer.pl shows the boards are maintained >>>> by denx, so maybe you could clarify this? >>> >>> +CC Stefan. >> >> Thanks. At the time I added the bootcounter support on this platform, >> it used to work. I might have missed something though. Right now, >> I don't have access to such a platform, so its hard for me to test >> something. If you have some patches to fix any potential overlapping >> memory usages, I'll gladly review them. > > I just checked out ae996c80 and from what I can tell, the RAM layout was > the same at that time: include/generated/generated-asm-offsets.h contains: > - #define GENERATED_GBL_DATA_SIZE 208 > - #define GD_SIZE 200 > > That leaves us with an 8 byte alignment gap at the end of the SRAM and > that's where the boot counter is located. So the global data is not > overwritten, but this is more or less by chance, any change to the > struct definition might change this. > > Looking at just another example: the x600 board defines its SRAM_SIZE 8 > bytes smaller than what it really is. That seems like a good idea for > socfpga, too. I'll see if I can provide a patch. This looks like a hack and a bogus one. Lying about RAM size is never a good idea. > Slightly off-topic: I have a constellation where the boot-counter is not > reset although the power has been pulled. This device has big capacitors > and the SRAM content seems to survive although the processor is reset. > To me it seems as if looking at the boot reason might be safer than > checking for a magic word... Any ideas? What boot reason options do you get ? [...] -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-14 20:43 ` Marek Vasut @ 2018-05-15 7:26 ` Simon Goldschmidt 2018-05-15 9:09 ` Marek Vasut 0 siblings, 1 reply; 11+ messages in thread From: Simon Goldschmidt @ 2018-05-15 7:26 UTC (permalink / raw) To: u-boot On 14.05.2018 22:43, Marek Vasut wrote: > On 05/14/2018 09:43 PM, Simon Goldschmidt wrote: >> >> >> On 14.05.2018 17:51, Stefan Roese wrote: >>> On 14.05.2018 11:06, Marek Vasut wrote: >>>> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >>>>> >>>>> >>>>> On 14.05.2018 10:17, Marek Vasut wrote: >>>>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>>>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>>>>> The global data are in the .data section, so there's no point in >>>>>>>> reserving any space for it above stack. Put stack at the end of >>>>>>>> SRAM. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>>>>> --- >>>>>>>> include/configs/socfpga_common.h | 4 +--- >>>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>>> b/include/configs/socfpga_common.h >>>>>>>> index 4de2aa7929..cb67d539b1 100644 >>>>>>>> --- a/include/configs/socfpga_common.h >>>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>>> @@ -35,10 +35,8 @@ >>>>>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>>>>> #endif >>>>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>>>>>> >>>>>>> I think this interferes with the bootcount being stored at the end of >>>>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>>>>> defconfig here :-) >>>>>>> >>>>>>> I'd really like to keep some room here for bootcounter and some >>>>>>> related >>>>>>> things I store here. >>>>>> >>>>>> So if I understand it correctly, before this patch, the boards >>>>>> overwrote >>>>>> the global data ? >>>>> >>>>> Ehrm, reading up on the init code path in the S and C files, it seems >>>>> like that, yes. I just used the two boards as an example. Are they >>>>> actively maintained? get_maintainer.pl shows the boards are maintained >>>>> by denx, so maybe you could clarify this? >>>> >>>> +CC Stefan. >>> >>> Thanks. At the time I added the bootcounter support on this platform, >>> it used to work. I might have missed something though. Right now, >>> I don't have access to such a platform, so its hard for me to test >>> something. If you have some patches to fix any potential overlapping >>> memory usages, I'll gladly review them. >> >> I just checked out ae996c80 and from what I can tell, the RAM layout was >> the same at that time: include/generated/generated-asm-offsets.h contains: >> - #define GENERATED_GBL_DATA_SIZE 208 >> - #define GD_SIZE 200 >> >> That leaves us with an 8 byte alignment gap at the end of the SRAM and >> that's where the boot counter is located. So the global data is not >> overwritten, but this is more or less by chance, any change to the >> struct definition might change this. >> >> Looking at just another example: the x600 board defines its SRAM_SIZE 8 >> bytes smaller than what it really is. That seems like a good idea for >> socfpga, too. I'll see if I can provide a patch. > > This looks like a hack and a bogus one. Lying about RAM size is never a > good idea. > >> Slightly off-topic: I have a constellation where the boot-counter is not >> reset although the power has been pulled. This device has big capacitors >> and the SRAM content seems to survive although the processor is reset. >> To me it seems as if looking at the boot reason might be safer than >> checking for a magic word... Any ideas? > > What boot reason options do you get ? The reset manager has a status register [1] that has various bits for different reasons of warm or cold reset. I'm just not sure if it's worth going through the hassle or if I'm alone with that problem of the bootcounter surviving a pull-and-replug. [1] https://www.altera.com/hps/cyclone-v/topic/sfo1410067763568.html Simon ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM 2018-05-15 7:26 ` Simon Goldschmidt @ 2018-05-15 9:09 ` Marek Vasut 0 siblings, 0 replies; 11+ messages in thread From: Marek Vasut @ 2018-05-15 9:09 UTC (permalink / raw) To: u-boot On 05/15/2018 09:26 AM, Simon Goldschmidt wrote: > > > On 14.05.2018 22:43, Marek Vasut wrote: >> On 05/14/2018 09:43 PM, Simon Goldschmidt wrote: >>> >>> >>> On 14.05.2018 17:51, Stefan Roese wrote: >>>> On 14.05.2018 11:06, Marek Vasut wrote: >>>>> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote: >>>>>> >>>>>> >>>>>> On 14.05.2018 10:17, Marek Vasut wrote: >>>>>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote: >>>>>>>> On 12.05.2018 22:27, Marek Vasut wrote: >>>>>>>>> The global data are in the .data section, so there's no point in >>>>>>>>> reserving any space for it above stack. Put stack at the end of >>>>>>>>> SRAM. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>>>> Cc: Chin Liang See <chin.liang.see@intel.com> >>>>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org> >>>>>>>>> --- >>>>>>>>> include/configs/socfpga_common.h | 4 +--- >>>>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>>>> b/include/configs/socfpga_common.h >>>>>>>>> index 4de2aa7929..cb67d539b1 100644 >>>>>>>>> --- a/include/configs/socfpga_common.h >>>>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>>>> @@ -35,10 +35,8 @@ >>>>>>>>> #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE00000 >>>>>>>>> #define CONFIG_SYS_INIT_RAM_SIZE 0x40000 /* 256KB */ >>>>>>>>> #endif >>>>>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET \ >>>>>>>>> - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) >>>>>>>>> #define CONFIG_SYS_INIT_SP_ADDR \ >>>>>>>>> - (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) >>>>>>>>> + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE) >>>>>>>> >>>>>>>> I think this interferes with the bootcount being stored at the >>>>>>>> end of >>>>>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local >>>>>>>> defconfig here :-) >>>>>>>> >>>>>>>> I'd really like to keep some room here for bootcounter and some >>>>>>>> related >>>>>>>> things I store here. >>>>>>> >>>>>>> So if I understand it correctly, before this patch, the boards >>>>>>> overwrote >>>>>>> the global data ? >>>>>> >>>>>> Ehrm, reading up on the init code path in the S and C files, it seems >>>>>> like that, yes. I just used the two boards as an example. Are they >>>>>> actively maintained? get_maintainer.pl shows the boards are >>>>>> maintained >>>>>> by denx, so maybe you could clarify this? >>>>> >>>>> +CC Stefan. >>>> >>>> Thanks. At the time I added the bootcounter support on this platform, >>>> it used to work. I might have missed something though. Right now, >>>> I don't have access to such a platform, so its hard for me to test >>>> something. If you have some patches to fix any potential overlapping >>>> memory usages, I'll gladly review them. >>> >>> I just checked out ae996c80 and from what I can tell, the RAM layout was >>> the same at that time: include/generated/generated-asm-offsets.h >>> contains: >>> - #define GENERATED_GBL_DATA_SIZE 208 >>> - #define GD_SIZE 200 >>> >>> That leaves us with an 8 byte alignment gap at the end of the SRAM and >>> that's where the boot counter is located. So the global data is not >>> overwritten, but this is more or less by chance, any change to the >>> struct definition might change this. >>> >>> Looking at just another example: the x600 board defines its SRAM_SIZE 8 >>> bytes smaller than what it really is. That seems like a good idea for >>> socfpga, too. I'll see if I can provide a patch. >> >> This looks like a hack and a bogus one. Lying about RAM size is never a >> good idea. >> >>> Slightly off-topic: I have a constellation where the boot-counter is not >>> reset although the power has been pulled. This device has big capacitors >>> and the SRAM content seems to survive although the processor is reset. >>> To me it seems as if looking at the boot reason might be safer than >>> checking for a magic word... Any ideas? >> >> What boot reason options do you get ? > > The reset manager has a status register [1] that has various bits for > different reasons of warm or cold reset. I'm just not sure if it's worth > going through the hassle or if I'm alone with that problem of the > bootcounter surviving a pull-and-replug. > > [1] https://www.altera.com/hps/cyclone-v/topic/sfo1410067763568.html Do you get warm or cold reset indication during plug-and-replug ? -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-15 9:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-12 20:27 [U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM Marek Vasut 2018-05-14 8:03 ` Simon Goldschmidt 2018-05-14 8:17 ` Marek Vasut 2018-05-14 9:01 ` Simon Goldschmidt 2018-05-14 9:06 ` Marek Vasut 2018-05-14 15:51 ` Stefan Roese 2018-05-14 19:43 ` Simon Goldschmidt 2018-05-14 20:06 ` Simon Goldschmidt 2018-05-14 20:43 ` Marek Vasut 2018-05-15 7:26 ` Simon Goldschmidt 2018-05-15 9:09 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.