| Message ID | 20181030212311.28037-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot] arm: socfpga: check total size of SPL | expand |
On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: > Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm > linker script for SPL check the total SRAM size available for SPL > (code, data, bss, heap, global data). > > The previously existing define CONFIG_SPL_MAX_SIZE seems to only > check the binary size (which is without bss, heap and gd). > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > > include/configs/socfpga_common.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > index 2330143cf1..9103d0a966 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR > #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE > > +/* Check total size of SPL including BSS, malloc area and gd */ > +#include <generated/generic-asm-offsets.h> > +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ > + CONFIG_SYS_INIT_RAM_ADDR - \ > + CONFIG_SYS_MALLOC_F_LEN - \ > + GENERATED_GBL_DATA_SIZE) Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
On 30.10.2018 22:26, Marek Vasut wrote: > On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >> linker script for SPL check the total SRAM size available for SPL >> (code, data, bss, heap, global data). >> >> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >> check the binary size (which is without bss, heap and gd). >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> --- >> >> include/configs/socfpga_common.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h >> index 2330143cf1..9103d0a966 100644 >> --- a/include/configs/socfpga_common.h >> +++ b/include/configs/socfpga_common.h >> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >> >> +/* Check total size of SPL including BSS, malloc area and gd */ >> +#include <generated/generic-asm-offsets.h> >> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >> + CONFIG_SYS_INIT_RAM_ADDR - \ >> + CONFIG_SYS_MALLOC_F_LEN - \ >> + GENERATED_GBL_DATA_SIZE) > Are you sure this calculation is correct ? INIT_SP_ADDR is I think the > SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or > something ? Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE. So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR - MALLOC_F_LEN - GBL_DATA_SIZE". But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied. Simon
On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: > On 30.10.2018 22:26, Marek Vasut wrote: >> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>> linker script for SPL check the total SRAM size available for SPL >>> (code, data, bss, heap, global data). >>> >>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>> check the binary size (which is without bss, heap and gd). >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> --- >>> >>> include/configs/socfpga_common.h | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/include/configs/socfpga_common.h >>> b/include/configs/socfpga_common.h >>> index 2330143cf1..9103d0a966 100644 >>> --- a/include/configs/socfpga_common.h >>> +++ b/include/configs/socfpga_common.h >>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>> +/* Check total size of SPL including BSS, malloc area and gd */ >>> +#include <generated/generic-asm-offsets.h> >>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>> + CONFIG_SYS_MALLOC_F_LEN - \ >>> + GENERATED_GBL_DATA_SIZE) >> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the >> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or >> something ? > > Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + > INIT_RAM_SIZE. > So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR > - MALLOC_F_LEN - GBL_DATA_SIZE". > > But I did it this way to keep it working after Stefan's fix for > reserving the boot counter location is applied. Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: > > On 30.10.2018 22:26, Marek Vasut wrote: > >> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: > >>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm > >>> linker script for SPL check the total SRAM size available for SPL > >>> (code, data, bss, heap, global data). > >>> > >>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only > >>> check the binary size (which is without bss, heap and gd). > >>> > >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>> --- > >>> > >>> include/configs/socfpga_common.h | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/include/configs/socfpga_common.h > >>> b/include/configs/socfpga_common.h > >>> index 2330143cf1..9103d0a966 100644 > >>> --- a/include/configs/socfpga_common.h > >>> +++ b/include/configs/socfpga_common.h > >>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > >>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR > >>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE > >>> +/* Check total size of SPL including BSS, malloc area and gd */ > >>> +#include <generated/generic-asm-offsets.h> > >>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ > >>> + CONFIG_SYS_INIT_RAM_ADDR - \ > >>> + CONFIG_SYS_MALLOC_F_LEN - \ > >>> + GENERATED_GBL_DATA_SIZE) > >> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the > >> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or > >> something ? > > > > Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + > > INIT_RAM_SIZE. > > So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR > > - MALLOC_F_LEN - GBL_DATA_SIZE". > > > > But I did it this way to keep it working after Stefan's fix for > > reserving the boot counter location is applied. > > Now that's confusing :-) Add a comment explaining this into a V2 please, > otherwise the confusion will continue ... You're probably right. I'll send a V2 making this more clear. Simon
On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: > On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>> On 30.10.2018 22:26, Marek Vasut wrote: >>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>> linker script for SPL check the total SRAM size available for SPL >>>>> (code, data, bss, heap, global data). >>>>> >>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>> check the binary size (which is without bss, heap and gd). >>>>> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>> --- >>>>> >>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/include/configs/socfpga_common.h >>>>> b/include/configs/socfpga_common.h >>>>> index 2330143cf1..9103d0a966 100644 >>>>> --- a/include/configs/socfpga_common.h >>>>> +++ b/include/configs/socfpga_common.h >>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>> +/* Check total size of SPL including BSS, malloc area and gd */ >>>>> +#include <generated/generic-asm-offsets.h> >>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>> + GENERATED_GBL_DATA_SIZE) >>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the >>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or >>>> something ? >>> >>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>> INIT_RAM_SIZE. >>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR >>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>> >>> But I did it this way to keep it working after Stefan's fix for >>> reserving the boot counter location is applied. >> >> Now that's confusing :-) Add a comment explaining this into a V2 please, >> otherwise the confusion will continue ... > > You're probably right. I'll send a V2 making this more clear. Thanks
On 31.10.2018 11:00, Marek Vasut wrote: > On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: >> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>>> On 30.10.2018 22:26, Marek Vasut wrote: >>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>>> linker script for SPL check the total SRAM size available for SPL >>>>>> (code, data, bss, heap, global data). >>>>>> >>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>>> check the binary size (which is without bss, heap and gd). >>>>>> >>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>> --- >>>>>> >>>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/include/configs/socfpga_common.h >>>>>> b/include/configs/socfpga_common.h >>>>>> index 2330143cf1..9103d0a966 100644 >>>>>> --- a/include/configs/socfpga_common.h >>>>>> +++ b/include/configs/socfpga_common.h >>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ >>>>>> +#include <generated/generic-asm-offsets.h> >>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>>> + GENERATED_GBL_DATA_SIZE) >>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the >>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or >>>>> something ? >>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>>> INIT_RAM_SIZE. >>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR >>>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>>> >>>> But I did it this way to keep it working after Stefan's fix for >>>> reserving the boot counter location is applied. >>> Now that's confusing :-) Add a comment explaining this into a V2 please, >>> otherwise the confusion will continue ... >> You're probably right. I'll send a V2 making this more clear. > Thanks Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere? Simon
On 11/01/2018 08:26 PM, Simon Goldschmidt wrote: > On 31.10.2018 11:00, Marek Vasut wrote: >> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: >>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> >>> wrote: >>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>>>> On 30.10.2018 22:26, Marek Vasut wrote: >>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>>>> linker script for SPL check the total SRAM size available for SPL >>>>>>> (code, data, bss, heap, global data). >>>>>>> >>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>>>> check the binary size (which is without bss, heap and gd). >>>>>>> >>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>> --- >>>>>>> >>>>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>>>> 1 file changed, 7 insertions(+) >>>>>>> >>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>> b/include/configs/socfpga_common.h >>>>>>> index 2330143cf1..9103d0a966 100644 >>>>>>> --- a/include/configs/socfpga_common.h >>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>> @@ -242,6 +242,13 @@ unsigned int >>>>>>> cm_get_qspi_controller_clk_hz(void); >>>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ >>>>>>> +#include <generated/generic-asm-offsets.h> >>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>>>> + GENERATED_GBL_DATA_SIZE) >>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think >>>>>> the >>>>>> SRAM offset in the address space. Shouldn't that contain >>>>>> INIT_SP_SIZE or >>>>>> something ? >>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>>>> INIT_RAM_SIZE. >>>>> So by subtracting INIT_RAM_ADDR again, I effectively get >>>>> "INIT_RAM_ADDR >>>>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>>>> >>>>> But I did it this way to keep it working after Stefan's fix for >>>>> reserving the boot counter location is applied. >>>> Now that's confusing :-) Add a comment explaining this into a V2 >>>> please, >>>> otherwise the confusion will continue ... >>> You're probably right. I'll send a V2 making this more clear. >> Thanks > > Re-checking this, I found the check is still not correct as it would > leave 4 bytes for the stack only. Is there an option that controls how > much stack should be preserved for SPL somewhere? Stack just grows down, that's all, there's no limit.
On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > > On 31.10.2018 11:00, Marek Vasut wrote: > > On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: > >> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: > >>>> On 30.10.2018 22:26, Marek Vasut wrote: > >>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: > >>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm > >>>>>> linker script for SPL check the total SRAM size available for SPL > >>>>>> (code, data, bss, heap, global data). > >>>>>> > >>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only > >>>>>> check the binary size (which is without bss, heap and gd). > >>>>>> > >>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>>> --- > >>>>>> > >>>>>> include/configs/socfpga_common.h | 7 +++++++ > >>>>>> 1 file changed, 7 insertions(+) > >>>>>> > >>>>>> diff --git a/include/configs/socfpga_common.h > >>>>>> b/include/configs/socfpga_common.h > >>>>>> index 2330143cf1..9103d0a966 100644 > >>>>>> --- a/include/configs/socfpga_common.h > >>>>>> +++ b/include/configs/socfpga_common.h > >>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > >>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR > >>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE > >>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ > >>>>>> +#include <generated/generic-asm-offsets.h> > >>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ > >>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ > >>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ > >>>>>> + GENERATED_GBL_DATA_SIZE) > >>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the > >>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or > >>>>> something ? > >>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + > >>>> INIT_RAM_SIZE. > >>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR > >>>> - MALLOC_F_LEN - GBL_DATA_SIZE". > >>>> > >>>> But I did it this way to keep it working after Stefan's fix for > >>>> reserving the boot counter location is applied. > >>> Now that's confusing :-) Add a comment explaining this into a V2 please, > >>> otherwise the confusion will continue ... > >> You're probably right. I'll send a V2 making this more clear. > > Thanks > > Re-checking this, I found the check is still not correct as it would > leave 4 bytes for the stack only. Is there an option that controls how > much stack should be preserved for SPL somewhere? And to make matters worse, the devicetree is also not included in the size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select CONFIG_OF_EMBED. While this would be OK for SPL, I think it wouldn't be for U-Boot. How do other platforms handle this size check? Simon
On 11/01/2018 09:31 PM, Simon Goldschmidt wrote: > On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: >> >> On 31.10.2018 11:00, Marek Vasut wrote: >>> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: >>>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>>>>> On 30.10.2018 22:26, Marek Vasut wrote: >>>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>>>>> linker script for SPL check the total SRAM size available for SPL >>>>>>>> (code, data, bss, heap, global data). >>>>>>>> >>>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>>>>> check the binary size (which is without bss, heap and gd). >>>>>>>> >>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>> --- >>>>>>>> >>>>>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>>>>> 1 file changed, 7 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>>> b/include/configs/socfpga_common.h >>>>>>>> index 2330143cf1..9103d0a966 100644 >>>>>>>> --- a/include/configs/socfpga_common.h >>>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>>>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ >>>>>>>> +#include <generated/generic-asm-offsets.h> >>>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>>>>> + GENERATED_GBL_DATA_SIZE) >>>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the >>>>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or >>>>>>> something ? >>>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>>>>> INIT_RAM_SIZE. >>>>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR >>>>>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>>>>> >>>>>> But I did it this way to keep it working after Stefan's fix for >>>>>> reserving the boot counter location is applied. >>>>> Now that's confusing :-) Add a comment explaining this into a V2 please, >>>>> otherwise the confusion will continue ... >>>> You're probably right. I'll send a V2 making this more clear. >>> Thanks >> >> Re-checking this, I found the check is still not correct as it would >> leave 4 bytes for the stack only. Is there an option that controls how >> much stack should be preserved for SPL somewhere? > > And to make matters worse, the devicetree is also not included in the > size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select > CONFIG_OF_EMBED. > > While this would be OK for SPL, I think it wouldn't be for U-Boot. How > do other platforms handle this size check? Can git grep help here ?
On 01.11.2018 21:59, Marek Vasut wrote: > On 11/01/2018 09:31 PM, Simon Goldschmidt wrote: >> On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt >> <simon.k.r.goldschmidt@gmail.com> wrote: >>> On 31.10.2018 11:00, Marek Vasut wrote: >>>> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: >>>>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>>>>>> On 30.10.2018 22:26, Marek Vasut wrote: >>>>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>>>>>> linker script for SPL check the total SRAM size available for SPL >>>>>>>>> (code, data, bss, heap, global data). >>>>>>>>> >>>>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>>>>>> check the binary size (which is without bss, heap and gd). >>>>>>>>> >>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>>>>>> 1 file changed, 7 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>>>> b/include/configs/socfpga_common.h >>>>>>>>> index 2330143cf1..9103d0a966 100644 >>>>>>>>> --- a/include/configs/socfpga_common.h >>>>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>>>>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ >>>>>>>>> +#include <generated/generic-asm-offsets.h> >>>>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>>>>>> + GENERATED_GBL_DATA_SIZE) >>>>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the >>>>>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or >>>>>>>> something ? >>>>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>>>>>> INIT_RAM_SIZE. >>>>>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR >>>>>>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>>>>>> >>>>>>> But I did it this way to keep it working after Stefan's fix for >>>>>>> reserving the boot counter location is applied. >>>>>> Now that's confusing :-) Add a comment explaining this into a V2 please, >>>>>> otherwise the confusion will continue ... >>>>> You're probably right. I'll send a V2 making this more clear. >>>> Thanks >>> Re-checking this, I found the check is still not correct as it would >>> leave 4 bytes for the stack only. Is there an option that controls how >>> much stack should be preserved for SPL somewhere? >> And to make matters worse, the devicetree is also not included in the >> size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select >> CONFIG_OF_EMBED. >> >> While this would be OK for SPL, I think it wouldn't be for U-Boot. How >> do other platforms handle this size check? > Can git grep help here ? Not really. If so, I wouldn't have asked ;-) I can only git grep for known strings and I only see constants for CONFIG_SPL_MAX_SIZE. CONFIG_SPL_MAX_FOOTPRINT seems to be rarely used and no platform seems to be taking the devicetree into account. Stack usage seems to be covered in one or two boards, but I expect this to be quite platform specific (given he different drivers at least), so we might need an additional runtime check, which I haven't seen so far? In my (smaller) embedded projects, I often enable runtime stack checks to be on the safe side... Maybe printing free memory at build-time and printing stack usage at runtime would be a solution? But I only know U-Boot from the socfpga gen5 perspective, that's why I ask. Simon
On 11/01/2018 10:13 PM, Simon Goldschmidt wrote: > On 01.11.2018 21:59, Marek Vasut wrote: >> On 11/01/2018 09:31 PM, Simon Goldschmidt wrote: >>> On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt >>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>> On 31.10.2018 11:00, Marek Vasut wrote: >>>>> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: >>>>>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut >>>>>> <marek.vasut@gmail.com> wrote: >>>>>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: >>>>>>>> On 30.10.2018 22:26, Marek Vasut wrote: >>>>>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: >>>>>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm >>>>>>>>>> linker script for SPL check the total SRAM size available for SPL >>>>>>>>>> (code, data, bss, heap, global data). >>>>>>>>>> >>>>>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only >>>>>>>>>> check the binary size (which is without bss, heap and gd). >>>>>>>>>> >>>>>>>>>> Signed-off-by: Simon Goldschmidt >>>>>>>>>> <simon.k.r.goldschmidt@gmail.com> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> include/configs/socfpga_common.h | 7 +++++++ >>>>>>>>>> 1 file changed, 7 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/include/configs/socfpga_common.h >>>>>>>>>> b/include/configs/socfpga_common.h >>>>>>>>>> index 2330143cf1..9103d0a966 100644 >>>>>>>>>> --- a/include/configs/socfpga_common.h >>>>>>>>>> +++ b/include/configs/socfpga_common.h >>>>>>>>>> @@ -242,6 +242,13 @@ unsigned int >>>>>>>>>> cm_get_qspi_controller_clk_hz(void); >>>>>>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR >>>>>>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE >>>>>>>>>> +/* Check total size of SPL including BSS, malloc area and >>>>>>>>>> gd */ >>>>>>>>>> +#include <generated/generic-asm-offsets.h> >>>>>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ >>>>>>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ >>>>>>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ >>>>>>>>>> + GENERATED_GBL_DATA_SIZE) >>>>>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I >>>>>>>>> think the >>>>>>>>> SRAM offset in the address space. Shouldn't that contain >>>>>>>>> INIT_SP_SIZE or >>>>>>>>> something ? >>>>>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + >>>>>>>> INIT_RAM_SIZE. >>>>>>>> So by subtracting INIT_RAM_ADDR again, I effectively get >>>>>>>> "INIT_RAM_ADDR >>>>>>>> - MALLOC_F_LEN - GBL_DATA_SIZE". >>>>>>>> >>>>>>>> But I did it this way to keep it working after Stefan's fix for >>>>>>>> reserving the boot counter location is applied. >>>>>>> Now that's confusing :-) Add a comment explaining this into a V2 >>>>>>> please, >>>>>>> otherwise the confusion will continue ... >>>>>> You're probably right. I'll send a V2 making this more clear. >>>>> Thanks >>>> Re-checking this, I found the check is still not correct as it would >>>> leave 4 bytes for the stack only. Is there an option that controls how >>>> much stack should be preserved for SPL somewhere? >>> And to make matters worse, the devicetree is also not included in the >>> size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select >>> CONFIG_OF_EMBED. >>> >>> While this would be OK for SPL, I think it wouldn't be for U-Boot. How >>> do other platforms handle this size check? >> Can git grep help here ? > > Not really. If so, I wouldn't have asked ;-) > > I can only git grep for known strings and I only see constants for > CONFIG_SPL_MAX_SIZE. > CONFIG_SPL_MAX_FOOTPRINT seems to be rarely used and no platform seems > to be taking the devicetree into account. Ouch :( > Stack usage seems to be covered in one or two boards, but I expect this > to be quite platform specific (given he different drivers at least), so > we might need an additional runtime check, which I haven't seen so far? > In my (smaller) embedded projects, I often enable runtime stack checks > to be on the safe side... Makes sense. > Maybe printing free memory at build-time and printing stack usage at > runtime would be a solution? But I only know U-Boot from the socfpga > gen5 perspective, that's why I ask. Wouldn't the stack usage change with usage too ? I think it might, albeit slightly. I'm not sure this would be really useful then.
Am Fr., 2. Nov. 2018, 01:08 hat Marek Vasut <marex@denx.de> geschrieben: > On 11/01/2018 08:26 PM, Simon Goldschmidt wrote: > > On 31.10.2018 11:00, Marek Vasut wrote: > >> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote: > >>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut@gmail.com> > >>> wrote: > >>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote: > >>>>> On 30.10.2018 22:26, Marek Vasut wrote: > >>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote: > >>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm > >>>>>>> linker script for SPL check the total SRAM size available for SPL > >>>>>>> (code, data, bss, heap, global data). > >>>>>>> > >>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only > >>>>>>> check the binary size (which is without bss, heap and gd). > >>>>>>> > >>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>>>> --- > >>>>>>> > >>>>>>> include/configs/socfpga_common.h | 7 +++++++ > >>>>>>> 1 file changed, 7 insertions(+) > >>>>>>> > >>>>>>> diff --git a/include/configs/socfpga_common.h > >>>>>>> b/include/configs/socfpga_common.h > >>>>>>> index 2330143cf1..9103d0a966 100644 > >>>>>>> --- a/include/configs/socfpga_common.h > >>>>>>> +++ b/include/configs/socfpga_common.h > >>>>>>> @@ -242,6 +242,13 @@ unsigned int > >>>>>>> cm_get_qspi_controller_clk_hz(void); > >>>>>>> #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR > >>>>>>> #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE > >>>>>>> +/* Check total size of SPL including BSS, malloc area and gd */ > >>>>>>> +#include <generated/generic-asm-offsets.h> > >>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ > >>>>>>> + CONFIG_SYS_INIT_RAM_ADDR - \ > >>>>>>> + CONFIG_SYS_MALLOC_F_LEN - \ > >>>>>>> + GENERATED_GBL_DATA_SIZE) > >>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think > >>>>>> the > >>>>>> SRAM offset in the address space. Shouldn't that contain > >>>>>> INIT_SP_SIZE or > >>>>>> something ? > >>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + > >>>>> INIT_RAM_SIZE. > >>>>> So by subtracting INIT_RAM_ADDR again, I effectively get > >>>>> "INIT_RAM_ADDR > >>>>> - MALLOC_F_LEN - GBL_DATA_SIZE". > >>>>> > >>>>> But I did it this way to keep it working after Stefan's fix for > >>>>> reserving the boot counter location is applied. > >>>> Now that's confusing :-) Add a comment explaining this into a V2 > >>>> please, > >>>> otherwise the confusion will continue ... > >>> You're probably right. I'll send a V2 making this more clear. > >> Thanks > > > > Re-checking this, I found the check is still not correct as it would > > leave 4 bytes for the stack only. Is there an option that controls how > > much stack should be preserved for SPL somewhere? > > Stack just grows down, that's all, there's no limit. > Right. But that sounds kind of dangerous... And it would make sense to let heap and stack grow against each other instead of letting heap grow up against end of ram and stack down against bss_end or the devicetree... To achieve that, 'gd' could be placed as low as possible, but the common startup code doesn't seem to make that easy, especially as there is no linker symbol for the devicetree start/end of it is not embedded. Simon >
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 2330143cf1..9103d0a966 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void); #define CONFIG_SPL_TEXT_BASE CONFIG_SYS_INIT_RAM_ADDR #define CONFIG_SPL_MAX_SIZE CONFIG_SYS_INIT_RAM_SIZE +/* Check total size of SPL including BSS, malloc area and gd */ +#include <generated/generic-asm-offsets.h> +#define CONFIG_SPL_MAX_FOOTPRINT (CONFIG_SYS_INIT_SP_ADDR - \ + CONFIG_SYS_INIT_RAM_ADDR - \ + CONFIG_SYS_MALLOC_F_LEN - \ + GENERATED_GBL_DATA_SIZE) + #if defined(CONFIG_TARGET_SOCFPGA_ARRIA10) /* SPL memory allocation configuration, this is for FAT implementation */ #ifndef CONFIG_SYS_SPL_MALLOC_START
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data). The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd). Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- include/configs/socfpga_common.h | 7 +++++++ 1 file changed, 7 insertions(+)