| Message ID | 20190319155632.5680-4-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | [U-Boot,01/11] arm: at91: Makefile: Compile lowlevel_init only when really necessary | expand |
On 19.03.2019 17:56, Stefan Roese wrote: > External E-Mail > > > This patch adds some checks, so that the watchdog can be enabled in main > U-Boot proper but can be disabled in SPL. Hi Stefan, Actually your code looks at CONFIG_SPL_WATCHDOG_SUPPORT , so , if this is disabled in the config, you say that the watchdog was still enabled? (thus broken CONFIG_SPL_WATCHDOG_SUPPORT ?) Eugen > > This will be used by some AT91SAM based boards, which might enable the > watchdog in the main U-Boot proper and not in SPL. It will be enabled in > SPL by default there, so no need to configure it there. This approach > saves some space in SPL. > > 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> > --- > include/watchdog.h | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/watchdog.h b/include/watchdog.h > index 14073cfdd2..3a357de903 100644 > --- a/include/watchdog.h > +++ b/include/watchdog.h > @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); > #if defined(__ASSEMBLY__) > #define WATCHDOG_RESET bl watchdog_reset > #else > - extern void watchdog_reset(void); > + /* Don't require the watchdog to be enabled in SPL */ > + #if defined(CONFIG_SPL_BUILD) && \ > + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) > + #define WATCHDOG_RESET() {} > + #else > + extern void watchdog_reset(void); > > - #define WATCHDOG_RESET watchdog_reset > + #define WATCHDOG_RESET watchdog_reset > + #endif > #endif > #else > /* >
On 20.03.19 08:30, Eugen.Hristev@microchip.com wrote: > > > On 19.03.2019 17:56, Stefan Roese wrote: >> External E-Mail >> >> >> This patch adds some checks, so that the watchdog can be enabled in main >> U-Boot proper but can be disabled in SPL. > > Hi Stefan, > > Actually your code looks at CONFIG_SPL_WATCHDOG_SUPPORT , so , if this > is disabled in the config, you say that the watchdog was still enabled? > (thus broken CONFIG_SPL_WATCHDOG_SUPPORT ?) Yes, in my case here, the watchdog is disabled in SPL and enabled in main U-Boot proper. This use case is what this patch fixes. Is this still unclear? Sorry, I didn't fully understand your question. Thanks, Stefan > Eugen > >> >> This will be used by some AT91SAM based boards, which might enable the >> watchdog in the main U-Boot proper and not in SPL. It will be enabled in >> SPL by default there, so no need to configure it there. This approach >> saves some space in SPL. >> >> 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> >> --- >> include/watchdog.h | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/include/watchdog.h b/include/watchdog.h >> index 14073cfdd2..3a357de903 100644 >> --- a/include/watchdog.h >> +++ b/include/watchdog.h >> @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); >> #if defined(__ASSEMBLY__) >> #define WATCHDOG_RESET bl watchdog_reset >> #else >> - extern void watchdog_reset(void); >> + /* Don't require the watchdog to be enabled in SPL */ >> + #if defined(CONFIG_SPL_BUILD) && \ >> + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) >> + #define WATCHDOG_RESET() {} >> + #else >> + extern void watchdog_reset(void); >> >> - #define WATCHDOG_RESET watchdog_reset >> + #define WATCHDOG_RESET watchdog_reset >> + #endif >> #endif >> #else >> /* >> Viele Grüße, Stefan
On 20.03.2019 09:33, Stefan Roese wrote: > External E-Mail > > > On 20.03.19 08:30, Eugen.Hristev@microchip.com wrote: >> >> >> On 19.03.2019 17:56, Stefan Roese wrote: >>> External E-Mail >>> >>> >>> This patch adds some checks, so that the watchdog can be enabled in main >>> U-Boot proper but can be disabled in SPL. >> >> Hi Stefan, >> >> Actually your code looks at CONFIG_SPL_WATCHDOG_SUPPORT , so , if this >> is disabled in the config, you say that the watchdog was still enabled? >> (thus broken CONFIG_SPL_WATCHDOG_SUPPORT ?) > > Yes, in my case here, the watchdog is disabled in SPL and enabled in > main U-Boot proper. This use case is what this patch fixes. > > Is this still unclear? Sorry, I didn't fully understand your question. There is a Kconfig named CONFIG_SPL_WATCHDOG_SUPPORT If this is y, then the watchdog support should be included in SPL If this is n, then the watchdog support should not be included in SPL. Considering your use case, you want CONFIG_SPL_WATCHDOG_SUPPORT=n Configuring this, the watchdog is still enabled in SPL? So my question: is the behavior of CONFIG_SPL_WATCHDOG_SUPPORT=n not aligned with your use case ? So you are actually fixing the behavior of CONFIG_SPL_WATCHDOG_SUPPORT=n ? > Thanks, > Stefan > >> Eugen >> >>> >>> This will be used by some AT91SAM based boards, which might enable the >>> watchdog in the main U-Boot proper and not in SPL. It will be enabled in >>> SPL by default there, so no need to configure it there. This approach >>> saves some space in SPL. >>> >>> 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> >>> --- >>> include/watchdog.h | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/watchdog.h b/include/watchdog.h >>> index 14073cfdd2..3a357de903 100644 >>> --- a/include/watchdog.h >>> +++ b/include/watchdog.h >>> @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); >>> #if defined(__ASSEMBLY__) >>> #define WATCHDOG_RESET bl watchdog_reset >>> #else >>> - extern void watchdog_reset(void); >>> + /* Don't require the watchdog to be enabled in SPL */ >>> + #if defined(CONFIG_SPL_BUILD) && \ >>> + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) >>> + #define WATCHDOG_RESET() {} >>> + #else >>> + extern void watchdog_reset(void); >>> - #define WATCHDOG_RESET watchdog_reset >>> + #define WATCHDOG_RESET watchdog_reset >>> + #endif >>> #endif >>> #else >>> /* >>> > > Viele Grüße, > Stefan >
On 20.03.19 08:41, Eugen.Hristev@microchip.com wrote: > > > On 20.03.2019 09:33, Stefan Roese wrote: >> External E-Mail >> >> >> On 20.03.19 08:30, Eugen.Hristev@microchip.com wrote: >>> >>> >>> On 19.03.2019 17:56, Stefan Roese wrote: >>>> External E-Mail >>>> >>>> >>>> This patch adds some checks, so that the watchdog can be enabled in main >>>> U-Boot proper but can be disabled in SPL. >>> >>> Hi Stefan, >>> >>> Actually your code looks at CONFIG_SPL_WATCHDOG_SUPPORT , so , if this >>> is disabled in the config, you say that the watchdog was still enabled? >>> (thus broken CONFIG_SPL_WATCHDOG_SUPPORT ?) >> >> Yes, in my case here, the watchdog is disabled in SPL and enabled in >> main U-Boot proper. This use case is what this patch fixes. >> >> Is this still unclear? Sorry, I didn't fully understand your question. > > There is a Kconfig named CONFIG_SPL_WATCHDOG_SUPPORT > If this is y, then the watchdog support should be included in SPL > If this is n, then the watchdog support should not be included in SPL. > > Considering your use case, you want CONFIG_SPL_WATCHDOG_SUPPORT=n Correct. > Configuring this, the watchdog is still enabled in SPL? I don't want the U-Boot SPL support enabled (mainly because of code size). AFAIU, the AT91SAM watchdog is enabled by default. So its enabled in the SoC when SPL is running, as I don't want to disable it (as done e.g. in AT91Bootrap if configured this way). > So my question: is the behavior of CONFIG_SPL_WATCHDOG_SUPPORT=n not > aligned with your use case ? So you are actually fixing the behavior of > CONFIG_SPL_WATCHDOG_SUPPORT=n ? Yes. Without this patch I do get this build error: ... LD spl/u-boot-spl lib/built-in.o: In function `udelay': /home/stefan/git/u-boot/u-boot/lib/time.c:167: undefined reference to `watchdog_reset' drivers/built-in.o: In function `atmel_nand_pmecc_write_page': /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:592: undefined reference to `watchdog_reset' drivers/built-in.o: In function `atmel_nand_pmecc_read_page': /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:552: undefined reference to `watchdog_reset' drivers/built-in.o: In function `pmecc_err_location': /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:416: undefined reference to `watchdog_reset' scripts/Makefile.spl:384: recipe for target 'spl/u-boot-spl' failed make[1]: *** [spl/u-boot-spl] Error 1 Makefile:1651: recipe for target 'spl/u-boot-spl' failed make: *** [spl/u-boot-spl] Error 2 Thanks, Stefan > >> Thanks, >> Stefan >> >>> Eugen >>> >>>> >>>> This will be used by some AT91SAM based boards, which might enable the >>>> watchdog in the main U-Boot proper and not in SPL. It will be enabled in >>>> SPL by default there, so no need to configure it there. This approach >>>> saves some space in SPL. >>>> >>>> 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> >>>> --- >>>> include/watchdog.h | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/watchdog.h b/include/watchdog.h >>>> index 14073cfdd2..3a357de903 100644 >>>> --- a/include/watchdog.h >>>> +++ b/include/watchdog.h >>>> @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); >>>> #if defined(__ASSEMBLY__) >>>> #define WATCHDOG_RESET bl watchdog_reset >>>> #else >>>> - extern void watchdog_reset(void); >>>> + /* Don't require the watchdog to be enabled in SPL */ >>>> + #if defined(CONFIG_SPL_BUILD) && \ >>>> + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) >>>> + #define WATCHDOG_RESET() {} >>>> + #else >>>> + extern void watchdog_reset(void); >>>> - #define WATCHDOG_RESET watchdog_reset >>>> + #define WATCHDOG_RESET watchdog_reset >>>> + #endif >>>> #endif >>>> #else >>>> /* >>>> >> >> Viele Grüße, >> Stefan >> Viele Grüße, Stefan
On 20.03.2019 09:48, Stefan Roese wrote: > External E-Mail > > > On 20.03.19 08:41, Eugen.Hristev@microchip.com wrote: >> >> >> On 20.03.2019 09:33, Stefan Roese wrote: >>> External E-Mail >>> >>> >>> On 20.03.19 08:30, Eugen.Hristev@microchip.com wrote: >>>> >>>> >>>> On 19.03.2019 17:56, Stefan Roese wrote: >>>>> External E-Mail >>>>> >>>>> >>>>> This patch adds some checks, so that the watchdog can be enabled in >>>>> main >>>>> U-Boot proper but can be disabled in SPL. >>>> >>>> Hi Stefan, >>>> >>>> Actually your code looks at CONFIG_SPL_WATCHDOG_SUPPORT , so , if this >>>> is disabled in the config, you say that the watchdog was still enabled? >>>> (thus broken CONFIG_SPL_WATCHDOG_SUPPORT ?) >>> >>> Yes, in my case here, the watchdog is disabled in SPL and enabled in >>> main U-Boot proper. This use case is what this patch fixes. >>> >>> Is this still unclear? Sorry, I didn't fully understand your question. >> >> There is a Kconfig named CONFIG_SPL_WATCHDOG_SUPPORT >> If this is y, then the watchdog support should be included in SPL >> If this is n, then the watchdog support should not be included in SPL. >> >> Considering your use case, you want CONFIG_SPL_WATCHDOG_SUPPORT=n > > Correct. > >> Configuring this, the watchdog is still enabled in SPL? > > I don't want the U-Boot SPL support enabled (mainly because of code > size). AFAIU, the AT91SAM watchdog is enabled by default. So its > enabled in the SoC when SPL is running, as I don't want to disable > it (as done e.g. in AT91Bootrap if configured this way). > >> So my question: is the behavior of CONFIG_SPL_WATCHDOG_SUPPORT=n not >> aligned with your use case ? So you are actually fixing the behavior of >> CONFIG_SPL_WATCHDOG_SUPPORT=n ? > > Yes. Without this patch I do get this build error: > > ... > LD spl/u-boot-spl > lib/built-in.o: In function `udelay': > /home/stefan/git/u-boot/u-boot/lib/time.c:167: undefined reference to > `watchdog_reset' > drivers/built-in.o: In function `atmel_nand_pmecc_write_page': > /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:592: > undefined reference to `watchdog_reset' > drivers/built-in.o: In function `atmel_nand_pmecc_read_page': > /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:552: > undefined reference to `watchdog_reset' > drivers/built-in.o: In function `pmecc_err_location': > /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:416: > undefined reference to `watchdog_reset' > scripts/Makefile.spl:384: recipe for target 'spl/u-boot-spl' failed > make[1]: *** [spl/u-boot-spl] Error 1 > Makefile:1651: recipe for target 'spl/u-boot-spl' failed > make: *** [spl/u-boot-spl] Error 2 > OK so that's what I want to settle: you are actually fixing the issue of CONFIG_SPL_WATCHDOG_SUPPORT=n not working properly Also, we may have to look inside the atmel_nand.c as it may be affected? Thanks, Eugen > Thanks, > Stefan > >> >>> Thanks, >>> Stefan >>> >>>> Eugen >>>> >>>>> >>>>> This will be used by some AT91SAM based boards, which might enable the >>>>> watchdog in the main U-Boot proper and not in SPL. It will be >>>>> enabled in >>>>> SPL by default there, so no need to configure it there. This approach >>>>> saves some space in SPL. >>>>> >>>>> 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> >>>>> --- >>>>> include/watchdog.h | 10 ++++++++-- >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/include/watchdog.h b/include/watchdog.h >>>>> index 14073cfdd2..3a357de903 100644 >>>>> --- a/include/watchdog.h >>>>> +++ b/include/watchdog.h >>>>> @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); >>>>> #if defined(__ASSEMBLY__) >>>>> #define WATCHDOG_RESET bl watchdog_reset >>>>> #else >>>>> - extern void watchdog_reset(void); >>>>> + /* Don't require the watchdog to be enabled in SPL */ >>>>> + #if defined(CONFIG_SPL_BUILD) && \ >>>>> + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) >>>>> + #define WATCHDOG_RESET() {} >>>>> + #else >>>>> + extern void watchdog_reset(void); >>>>> - #define WATCHDOG_RESET watchdog_reset >>>>> + #define WATCHDOG_RESET watchdog_reset >>>>> + #endif >>>>> #endif >>>>> #else >>>>> /* >>>>> >>> >>> Viele Grüße, >>> Stefan >>> > > Viele Grüße, > Stefan >
On 20.03.19 09:06, Eugen.Hristev@microchip.com wrote: <snip> >>> So my question: is the behavior of CONFIG_SPL_WATCHDOG_SUPPORT=n not >>> aligned with your use case ? So you are actually fixing the behavior of >>> CONFIG_SPL_WATCHDOG_SUPPORT=n ? >> >> Yes. Without this patch I do get this build error: >> >> ... >> LD spl/u-boot-spl >> lib/built-in.o: In function `udelay': >> /home/stefan/git/u-boot/u-boot/lib/time.c:167: undefined reference to >> `watchdog_reset' >> drivers/built-in.o: In function `atmel_nand_pmecc_write_page': >> /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:592: >> undefined reference to `watchdog_reset' >> drivers/built-in.o: In function `atmel_nand_pmecc_read_page': >> /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:552: >> undefined reference to `watchdog_reset' >> drivers/built-in.o: In function `pmecc_err_location': >> /home/stefan/git/u-boot/u-boot/drivers/mtd/nand/raw/atmel_nand.c:416: >> undefined reference to `watchdog_reset' >> scripts/Makefile.spl:384: recipe for target 'spl/u-boot-spl' failed >> make[1]: *** [spl/u-boot-spl] Error 1 >> Makefile:1651: recipe for target 'spl/u-boot-spl' failed >> make: *** [spl/u-boot-spl] Error 2 >> > > OK so that's what I want to settle: you are actually fixing the issue of > CONFIG_SPL_WATCHDOG_SUPPORT=n not working properly Yes, its *not* a AT91 specific issue. Its a general issue of the code. > Also, we may have to look inside the atmel_nand.c as it may be affected? Affected in which way? You mean that the watchdog might time out while loading the main U-Boot proper from NAND? Or what are you referring to? Thanks, Stefan
diff --git a/include/watchdog.h b/include/watchdog.h index 14073cfdd2..3a357de903 100644 --- a/include/watchdog.h +++ b/include/watchdog.h @@ -51,9 +51,15 @@ int init_func_watchdog_reset(void); #if defined(__ASSEMBLY__) #define WATCHDOG_RESET bl watchdog_reset #else - extern void watchdog_reset(void); + /* Don't require the watchdog to be enabled in SPL */ + #if defined(CONFIG_SPL_BUILD) && \ + !defined(CONFIG_SPL_WATCHDOG_SUPPORT) + #define WATCHDOG_RESET() {} + #else + extern void watchdog_reset(void); - #define WATCHDOG_RESET watchdog_reset + #define WATCHDOG_RESET watchdog_reset + #endif #endif #else /*
This patch adds some checks, so that the watchdog can be enabled in main U-Boot proper but can be disabled in SPL. This will be used by some AT91SAM based boards, which might enable the watchdog in the main U-Boot proper and not in SPL. It will be enabled in SPL by default there, so no need to configure it there. This approach saves some space in SPL. 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> --- include/watchdog.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)