| Message ID | 20190326121642.23302-6-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | [U-Boot,01/12,v2] arm: at91: Makefile: Compile lowlevel_init only when really necessary | expand |
Hello Stefan, Am 26.03.2019 um 13:16 schrieb Stefan Roese: > This patch enables and starts the watchdog on the AT91 platform if > configured. Currently the WD timeout is configured to 16 seconds, > which is the longest value for this timer. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Heiko Schocher <hs@denx.de> > Cc: Andreas Bießmann <andreas@biessmann.org> > Cc: Eugen Hristev <eugen.hristev@microchip.com> > --- > v2: > - Remove #ifdef to enable compilation also in SPL version > > arch/arm/mach-at91/clock.c | 39 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c > index 64cbc3d1ed..e3513f3473 100644 > --- a/arch/arm/mach-at91/clock.c > +++ b/arch/arm/mach-at91/clock.c > @@ -5,12 +5,16 @@ > */ > > #include <common.h> > +#include <dm.h> > +#include <wdt.h> > #include <asm/io.h> > #include <asm/arch/hardware.h> > #include <asm/arch/at91_pmc.h> > > #define EN_UPLL_TIMEOUT 500 > > +static struct udevice *watchdog_dev; This does not work for me on the taurus Board! This variable sits in the BSS, which is not set to 0 before U-Boot is relocated. On the taurus board I see: System.map: 21040930 b next_reset.9546 21040934 b watchdog_dev 21040938 b data.8182 hexudmp u-boot.bin: (TEXT_BASE 0x21000000) 00040920 00 00 00 00 00 00 00 00 01 00 00 00 20 00 00 21 |............ ..!| 00040930 17 00 00 00 24 00 00 21 17 00 00 00 28 00 00 21 |....$..!....(..!| 00040940 17 00 00 00 2c 00 00 21 17 00 00 00 30 00 00 21 |....,..!....0..!| And I see before relocation 0x21000024 for *watchdog_dev ... which leads in failure of the check "if (!watchdog_dev)" and cpu accesses wrong addresses in the end ... As discussed offline moving watchdog_dev to data section helps: diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c index e3513f3473..45e5f2fb57 100644 --- a/arch/arm/mach-at91/clock.c +++ b/arch/arm/mach-at91/clock.c @@ -13,7 +13,7 @@ #define EN_UPLL_TIMEOUT 500 -static struct udevice *watchdog_dev; +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; void at91_periph_clk_enable(int id) { The big question is, how many places do we have in code, where we access BSS before relocation ? May we better clear BSS very early (at last may possible on arm)? > + > void at91_periph_clk_enable(int id) > { > struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC; > @@ -118,3 +122,38 @@ void at91_pllicpr_init(u32 icpr) > > writel(icpr, &pmc->pllicpr); > } > + > +/* Called by macro WATCHDOG_RESET */ > +void watchdog_reset(void) > +{ > + static ulong next_reset; > + ulong now; > + > + if (!watchdog_dev) > + return; > + > + now = get_timer(0); > + > + /* Do not reset the watchdog too often */ > + if (now > next_reset) { > + next_reset = now + 1000; /* reset every 1000ms */ > + wdt_reset(watchdog_dev); > + } > +} > + > +int arch_misc_init(void) > +{ > + /* Init watchdog */ > + if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { > + debug("Watchdog: Not found by seq!\n"); > + if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { > + puts("Watchdog: Not found!\n"); > + return 0; > + } > + } > + > + wdt_start(watchdog_dev, 16000, 0); /* 16 seconds is max */ Here we have now a fix wdt timeout for all at91 based boards ... We should use the value from DTS. Beside of this, wdt is now running fine again on the taurus board! Thanks for this work. > + printf("Watchdog: Started\n"); > + > + return 0; > +} > bye, Heiko
Dear Heiko, In message <81e69dec-21e7-7b34-a261-e22ae9bef795@denx.de> you wrote: > > The big question is, how many places do we have in code, where we access > BSS before relocation ? Hopefully none. > May we better clear BSS very early (at last may possible on arm)? You cannot. There is no storage allocated for BSS - this happens only during relocation, and then it also gets zeroed. Best regards, Wolfgang Denk
Hi Wolfgang, On 29.03.19 13:06, Wolfgang Denk wrote: > In message <81e69dec-21e7-7b34-a261-e22ae9bef795@denx.de> you wrote: >> >> The big question is, how many places do we have in code, where we access >> BSS before relocation ? > > Hopefully none. > >> May we better clear BSS very early (at last may possible on arm)? > > You cannot. There is no storage allocated for BSS - this happens > only during relocation, and then it also gets zeroed. This is not 100% correct for all platforms. On some targets, e.g. where the DDR is initialized by SPL, the BSS location before relocation is already a valid address space in DDR in the main U-Boot proper. AFAICT, the BSS *could* already be used before relocation in these cases. Here the BBS could be cleared very early and relocated later with the code etc. No clearing in the relocation stage necessary. But since this is a special case, it makes no real sense to implement such an "early BSS" strategy into U-Boot, as other platforms will always exist, where the BSS is really only available after relocation (as you mentioned above). Thanks, Stefan
Hello Wolfgang, added Simon Goldschmidt to cc, as he just posted a patchset, which exactly wants to introduce "clear BSS before relocation" ... Am 29.03.2019 um 13:06 schrieb Wolfgang Denk: > Dear Heiko, > > In message <81e69dec-21e7-7b34-a261-e22ae9bef795@denx.de> you wrote: >> >> The big question is, how many places do we have in code, where we access >> BSS before relocation ? > > Hopefully none. Hmm... Hopefully, but I think, not easy to detect when reviewing patches ... I just stumbeld over this issue in this patch from Stefan, because I could try it on a hardware, and my hardware doesn;t boot with this patch... Theoretically you must check all vars, which are in BSS segment, if they are used before relocation ... and drop an error, no idea yet, how to detect this at compile time ... >> May we better clear BSS very early (at last may possible on arm)? > > You cannot. There is no storage allocated for BSS - this happens > only during relocation, and then it also gets zeroed. Yes, valid for the boards which have no SPL ... but if U-Boot is loaded with SPL into RAM, BSS is writeable. But as this is not valid for all boards, we cannot do this! Just see the patches from Simon: https://lists.denx.de/pipermail/u-boot/2019-March/361452.html http://patchwork.ozlabs.org/patch/1067363/ Same problem ... bye, Heiko
On Mon, Apr 1, 2019 at 7:41 AM Heiko Schocher <hs@denx.de> wrote: > > Hello Wolfgang, > > added Simon Goldschmidt to cc, as he just posted a patchset, which > exactly wants to introduce "clear BSS before relocation" ... > > Am 29.03.2019 um 13:06 schrieb Wolfgang Denk: > > Dear Heiko, > > > > In message <81e69dec-21e7-7b34-a261-e22ae9bef795@denx.de> you wrote: > >> > >> The big question is, how many places do we have in code, where we access > >> BSS before relocation ? > > > > Hopefully none. > > Hmm... Hopefully, but I think, not easy to detect when reviewing > patches ... I just stumbeld over this issue in this patch from > Stefan, because I could try it on a hardware, and my hardware doesn;t > boot with this patch... > > Theoretically you must check all vars, which are in BSS segment, if > they are used before relocation ... and drop an error, no idea yet, > how to detect this at compile time ... > > >> May we better clear BSS very early (at last may possible on arm)? > > > > You cannot. There is no storage allocated for BSS - this happens > > only during relocation, and then it also gets zeroed. > > Yes, valid for the boards which have no SPL ... but if U-Boot is > loaded with SPL into RAM, BSS is writeable. But as this is not > valid for all boards, we cannot do this! > > Just see the patches from Simon: > > https://lists.denx.de/pipermail/u-boot/2019-March/361452.html > > http://patchwork.ozlabs.org/patch/1067363/ > > Same problem ... OK, so the word stands "BSS is not used before relocation". Like already mentioned, we don't have a check that tells us when this rule is violated. Being like that, I would be surprised if such a check (if added) would yield zero failures... However, I'm not sure what "relocation" means for SPL boards where the SPL is loaded into SRAM (as is the case on many ARM boards at least). The correct wording here might be "RAM available" or something like that, instead of "relocation". With this definition, a new question arises: how can I be forced to provide a malloc implementation in "pre-reloc" phase (for pre-reloc driver model, e.g. drivers for serial and SDRAM) when people keep telling me bss might not even be available? That sounds a bit confusing to me. Regards, Simon
Hello Simon, Am 01.04.2019 um 11:13 schrieb Simon Goldschmidt: > On Mon, Apr 1, 2019 at 7:41 AM Heiko Schocher <hs@denx.de> wrote: >> >> Hello Wolfgang, >> >> added Simon Goldschmidt to cc, as he just posted a patchset, which >> exactly wants to introduce "clear BSS before relocation" ... >> >> Am 29.03.2019 um 13:06 schrieb Wolfgang Denk: >>> Dear Heiko, >>> >>> In message <81e69dec-21e7-7b34-a261-e22ae9bef795@denx.de> you wrote: >>>> >>>> The big question is, how many places do we have in code, where we access >>>> BSS before relocation ? >>> >>> Hopefully none. >> >> Hmm... Hopefully, but I think, not easy to detect when reviewing >> patches ... I just stumbeld over this issue in this patch from >> Stefan, because I could try it on a hardware, and my hardware doesn;t >> boot with this patch... >> >> Theoretically you must check all vars, which are in BSS segment, if >> they are used before relocation ... and drop an error, no idea yet, >> how to detect this at compile time ... >> >>>> May we better clear BSS very early (at last may possible on arm)? >>> >>> You cannot. There is no storage allocated for BSS - this happens >>> only during relocation, and then it also gets zeroed. >> >> Yes, valid for the boards which have no SPL ... but if U-Boot is >> loaded with SPL into RAM, BSS is writeable. But as this is not >> valid for all boards, we cannot do this! >> >> Just see the patches from Simon: >> >> https://lists.denx.de/pipermail/u-boot/2019-March/361452.html >> >> http://patchwork.ozlabs.org/patch/1067363/ >> >> Same problem ... > > OK, so the word stands "BSS is not used before relocation". Like already > mentioned, we don't have a check that tells us when this rule is > violated. Being like > that, I would be surprised if such a check (if added) would yield zero > failures... Hmm... I hope not. > However, I'm not sure what "relocation" means for SPL boards where the SPL is > loaded into SRAM (as is the case on many ARM boards at least). The correct > wording here might be "RAM available" or something like that, instead > of "relocation". In case of SPL ... there is no code relocation and "after relocation" is equivalent to "RAM available". > With this definition, a new question arises: how can I be forced to > provide a malloc > implementation in "pre-reloc" phase (for pre-reloc driver model, e.g. > drivers for serial > and SDRAM) when people keep telling me bss might not even be available? > That sounds a bit confusing to me. But Simon Glass suggestion to put all variables dlmalloc needs into a struct and put a pointer into GD should solve this problem, or? bye, Heiko
diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c index 64cbc3d1ed..e3513f3473 100644 --- a/arch/arm/mach-at91/clock.c +++ b/arch/arm/mach-at91/clock.c @@ -5,12 +5,16 @@ */ #include <common.h> +#include <dm.h> +#include <wdt.h> #include <asm/io.h> #include <asm/arch/hardware.h> #include <asm/arch/at91_pmc.h> #define EN_UPLL_TIMEOUT 500 +static struct udevice *watchdog_dev; + void at91_periph_clk_enable(int id) { struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC; @@ -118,3 +122,38 @@ void at91_pllicpr_init(u32 icpr) writel(icpr, &pmc->pllicpr); } + +/* Called by macro WATCHDOG_RESET */ +void watchdog_reset(void) +{ + static ulong next_reset; + ulong now; + + if (!watchdog_dev) + return; + + now = get_timer(0); + + /* Do not reset the watchdog too often */ + if (now > next_reset) { + next_reset = now + 1000; /* reset every 1000ms */ + wdt_reset(watchdog_dev); + } +} + +int arch_misc_init(void) +{ + /* Init watchdog */ + if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { + debug("Watchdog: Not found by seq!\n"); + if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { + puts("Watchdog: Not found!\n"); + return 0; + } + } + + wdt_start(watchdog_dev, 16000, 0); /* 16 seconds is max */ + printf("Watchdog: Started\n"); + + return 0; +}
This patch enables and starts the watchdog on the AT91 platform if configured. Currently the WD timeout is configured to 16 seconds, which is the longest value for this timer. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Heiko Schocher <hs@denx.de> Cc: Andreas Bießmann <andreas@biessmann.org> Cc: Eugen Hristev <eugen.hristev@microchip.com> --- v2: - Remove #ifdef to enable compilation also in SPL version arch/arm/mach-at91/clock.c | 39 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)