| Message ID | 20191127142029.30419-1-ch@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,RFC] ARM: reset: Move SYSRESET condition from Makefile into source file | expand |
On 11/27/19 3:20 PM, Claudius Heine wrote: > In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available > anywere, even if SYSRESET is disabled for SPL in the board specific header > file like this: > > #if defined(CONFIG_SPL_BUILD) > #undef CONFIG_WDT > #undef CONFIG_WATCHDOG > #undef CONFIG_SYSRESET > #define CONFIG_HW_WATCHDOG > #endif > > 'do_reset' is called from SPL for instance from the panic handler in case > SPL_USB_SDP is enabled and PANIC_HANG is not set. > > Setting PANIC_HANG would solve this issue, but it also changes the behavior > in case a panic occurs. > > Signed-off-by: Claudius Heine <ch@denx.de> > --- > arch/arm/lib/Makefile | 2 -- > arch/arm/lib/reset.c | 2 ++ > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile > index 9de9a9acee..763eb4498f 100644 > --- a/arch/arm/lib/Makefile > +++ b/arch/arm/lib/Makefile > @@ -56,9 +56,7 @@ obj-y += interrupts_64.o > else > obj-y += interrupts.o > endif > -ifndef CONFIG_SYSRESET > obj-y += reset.o > -endif > > obj-y += cache.o > obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o > diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c > index f3ea116e87..11e680be1d 100644 > --- a/arch/arm/lib/reset.c > +++ b/arch/arm/lib/reset.c > @@ -22,6 +22,7 @@ > > #include <common.h> > > +#if !defined(CONFIG_SYSRESET) > __weak void reset_misc(void) > { > } > @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > /*NOTREACHED*/ > return 0; > } > +#endif Does this mean there's now one huge ifdef around the entire source file? That's odd.
Hi Marek, On 27/11/2019 15.47, Marek Vasut wrote: > On 11/27/19 3:20 PM, Claudius Heine wrote: >> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >> anywere, even if SYSRESET is disabled for SPL in the board specific header >> file like this: >> >> #if defined(CONFIG_SPL_BUILD) >> #undef CONFIG_WDT >> #undef CONFIG_WATCHDOG >> #undef CONFIG_SYSRESET >> #define CONFIG_HW_WATCHDOG >> #endif >> >> 'do_reset' is called from SPL for instance from the panic handler in case >> SPL_USB_SDP is enabled and PANIC_HANG is not set. >> >> Setting PANIC_HANG would solve this issue, but it also changes the behavior >> in case a panic occurs. >> >> Signed-off-by: Claudius Heine <ch@denx.de> >> --- >> arch/arm/lib/Makefile | 2 -- >> arch/arm/lib/reset.c | 2 ++ >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >> index 9de9a9acee..763eb4498f 100644 >> --- a/arch/arm/lib/Makefile >> +++ b/arch/arm/lib/Makefile >> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >> else >> obj-y += interrupts.o >> endif >> -ifndef CONFIG_SYSRESET >> obj-y += reset.o >> -endif >> >> obj-y += cache.o >> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >> index f3ea116e87..11e680be1d 100644 >> --- a/arch/arm/lib/reset.c >> +++ b/arch/arm/lib/reset.c >> @@ -22,6 +22,7 @@ >> >> #include <common.h> >> >> +#if !defined(CONFIG_SYSRESET) >> __weak void reset_misc(void) >> { >> } >> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> /*NOTREACHED*/ >> return 0; >> } >> +#endif > > Does this mean there's now one huge ifdef around the entire source file? > That's odd. Right. Other suggestions? Maybe having 'do_reset' here as a weak instead, so that sysreset can overwrite it? But then the other definitions in arch/*/lib/reset.c should probably be the same for consistency sake? I tried with this patch not to change anything in case SYSRESET is enabled too much and since the file isn't too large I thought that would be ok for now. regards
On 11/27/19 4:09 PM, Claudius Heine wrote: > Hi Marek, > > On 27/11/2019 15.47, Marek Vasut wrote: >> On 11/27/19 3:20 PM, Claudius Heine wrote: >>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>> file like this: >>> >>> #if defined(CONFIG_SPL_BUILD) >>> #undef CONFIG_WDT >>> #undef CONFIG_WATCHDOG >>> #undef CONFIG_SYSRESET >>> #define CONFIG_HW_WATCHDOG >>> #endif >>> >>> 'do_reset' is called from SPL for instance from the panic handler in case >>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>> >>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>> in case a panic occurs. >>> >>> Signed-off-by: Claudius Heine <ch@denx.de> >>> --- >>> arch/arm/lib/Makefile | 2 -- >>> arch/arm/lib/reset.c | 2 ++ >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>> index 9de9a9acee..763eb4498f 100644 >>> --- a/arch/arm/lib/Makefile >>> +++ b/arch/arm/lib/Makefile >>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>> else >>> obj-y += interrupts.o >>> endif >>> -ifndef CONFIG_SYSRESET >>> obj-y += reset.o >>> -endif >>> >>> obj-y += cache.o >>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>> index f3ea116e87..11e680be1d 100644 >>> --- a/arch/arm/lib/reset.c >>> +++ b/arch/arm/lib/reset.c >>> @@ -22,6 +22,7 @@ >>> >>> #include <common.h> >>> >>> +#if !defined(CONFIG_SYSRESET) >>> __weak void reset_misc(void) >>> { >>> } >>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> /*NOTREACHED*/ >>> return 0; >>> } >>> +#endif >> >> Does this mean there's now one huge ifdef around the entire source file? >> That's odd. > > Right. Other suggestions? > > Maybe having 'do_reset' here as a weak instead, so that sysreset can > overwrite it? But then the other definitions in arch/*/lib/reset.c > should probably be the same for consistency sake? > > I tried with this patch not to change anything in case SYSRESET is > enabled too much and since the file isn't too large I thought that would > be ok for now. What if sysreset implemented do_reset ? Wouldn't that solve the issue ?
On 27/11/2019 16.12, Marek Vasut wrote: > On 11/27/19 4:09 PM, Claudius Heine wrote: >> Hi Marek, >> >> On 27/11/2019 15.47, Marek Vasut wrote: >>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>> file like this: >>>> >>>> #if defined(CONFIG_SPL_BUILD) >>>> #undef CONFIG_WDT >>>> #undef CONFIG_WATCHDOG >>>> #undef CONFIG_SYSRESET >>>> #define CONFIG_HW_WATCHDOG >>>> #endif >>>> >>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>> >>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>> in case a panic occurs. >>>> >>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>> --- >>>> arch/arm/lib/Makefile | 2 -- >>>> arch/arm/lib/reset.c | 2 ++ >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>> index 9de9a9acee..763eb4498f 100644 >>>> --- a/arch/arm/lib/Makefile >>>> +++ b/arch/arm/lib/Makefile >>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>> else >>>> obj-y += interrupts.o >>>> endif >>>> -ifndef CONFIG_SYSRESET >>>> obj-y += reset.o >>>> -endif >>>> >>>> obj-y += cache.o >>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>> index f3ea116e87..11e680be1d 100644 >>>> --- a/arch/arm/lib/reset.c >>>> +++ b/arch/arm/lib/reset.c >>>> @@ -22,6 +22,7 @@ >>>> >>>> #include <common.h> >>>> >>>> +#if !defined(CONFIG_SYSRESET) >>>> __weak void reset_misc(void) >>>> { >>>> } >>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>> /*NOTREACHED*/ >>>> return 0; >>>> } >>>> +#endif >>> >>> Does this mean there's now one huge ifdef around the entire source file? >>> That's odd. >> >> Right. Other suggestions? >> >> Maybe having 'do_reset' here as a weak instead, so that sysreset can >> overwrite it? But then the other definitions in arch/*/lib/reset.c >> should probably be the same for consistency sake? >> >> I tried with this patch not to change anything in case SYSRESET is >> enabled too much and since the file isn't too large I thought that would >> be ok for now. > > What if sysreset implemented do_reset ? Wouldn't that solve the issue ? Not sure what you mean... sysreset implements do_reset: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 But the SPL does not have sysreset in this case, so it needs something different.
On 11/27/19 4:17 PM, Claudius Heine wrote: > On 27/11/2019 16.12, Marek Vasut wrote: >> On 11/27/19 4:09 PM, Claudius Heine wrote: >>> Hi Marek, >>> >>> On 27/11/2019 15.47, Marek Vasut wrote: >>>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>>> file like this: >>>>> >>>>> #if defined(CONFIG_SPL_BUILD) >>>>> #undef CONFIG_WDT >>>>> #undef CONFIG_WATCHDOG >>>>> #undef CONFIG_SYSRESET >>>>> #define CONFIG_HW_WATCHDOG >>>>> #endif >>>>> >>>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>>> >>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>>> in case a panic occurs. >>>>> >>>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>>> --- >>>>> arch/arm/lib/Makefile | 2 -- >>>>> arch/arm/lib/reset.c | 2 ++ >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>>> index 9de9a9acee..763eb4498f 100644 >>>>> --- a/arch/arm/lib/Makefile >>>>> +++ b/arch/arm/lib/Makefile >>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>>> else >>>>> obj-y += interrupts.o >>>>> endif >>>>> -ifndef CONFIG_SYSRESET >>>>> obj-y += reset.o >>>>> -endif >>>>> >>>>> obj-y += cache.o >>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>>> index f3ea116e87..11e680be1d 100644 >>>>> --- a/arch/arm/lib/reset.c >>>>> +++ b/arch/arm/lib/reset.c >>>>> @@ -22,6 +22,7 @@ >>>>> >>>>> #include <common.h> >>>>> >>>>> +#if !defined(CONFIG_SYSRESET) >>>>> __weak void reset_misc(void) >>>>> { >>>>> } >>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>> /*NOTREACHED*/ >>>>> return 0; >>>>> } >>>>> +#endif >>>> >>>> Does this mean there's now one huge ifdef around the entire source file? >>>> That's odd. >>> >>> Right. Other suggestions? >>> >>> Maybe having 'do_reset' here as a weak instead, so that sysreset can >>> overwrite it? But then the other definitions in arch/*/lib/reset.c >>> should probably be the same for consistency sake? >>> >>> I tried with this patch not to change anything in case SYSRESET is >>> enabled too much and since the file isn't too large I thought that would >>> be ok for now. >> >> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? > > Not sure what you mean... sysreset implements do_reset: > > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 > > But the SPL does not have sysreset in this case, so it needs something > different. Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ?
On 27/11/2019 16.21, Marek Vasut wrote: > On 11/27/19 4:17 PM, Claudius Heine wrote: >> On 27/11/2019 16.12, Marek Vasut wrote: >>> On 11/27/19 4:09 PM, Claudius Heine wrote: >>>> Hi Marek, >>>> >>>> On 27/11/2019 15.47, Marek Vasut wrote: >>>>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>>>> file like this: >>>>>> >>>>>> #if defined(CONFIG_SPL_BUILD) >>>>>> #undef CONFIG_WDT >>>>>> #undef CONFIG_WATCHDOG >>>>>> #undef CONFIG_SYSRESET >>>>>> #define CONFIG_HW_WATCHDOG >>>>>> #endif >>>>>> >>>>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>>>> >>>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>>>> in case a panic occurs. >>>>>> >>>>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>>>> --- >>>>>> arch/arm/lib/Makefile | 2 -- >>>>>> arch/arm/lib/reset.c | 2 ++ >>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>>>> index 9de9a9acee..763eb4498f 100644 >>>>>> --- a/arch/arm/lib/Makefile >>>>>> +++ b/arch/arm/lib/Makefile >>>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>>>> else >>>>>> obj-y += interrupts.o >>>>>> endif >>>>>> -ifndef CONFIG_SYSRESET >>>>>> obj-y += reset.o >>>>>> -endif >>>>>> >>>>>> obj-y += cache.o >>>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>>>> index f3ea116e87..11e680be1d 100644 >>>>>> --- a/arch/arm/lib/reset.c >>>>>> +++ b/arch/arm/lib/reset.c >>>>>> @@ -22,6 +22,7 @@ >>>>>> >>>>>> #include <common.h> >>>>>> >>>>>> +#if !defined(CONFIG_SYSRESET) >>>>>> __weak void reset_misc(void) >>>>>> { >>>>>> } >>>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>>> /*NOTREACHED*/ >>>>>> return 0; >>>>>> } >>>>>> +#endif >>>>> >>>>> Does this mean there's now one huge ifdef around the entire source file? >>>>> That's odd. >>>> >>>> Right. Other suggestions? >>>> >>>> Maybe having 'do_reset' here as a weak instead, so that sysreset can >>>> overwrite it? But then the other definitions in arch/*/lib/reset.c >>>> should probably be the same for consistency sake? >>>> >>>> I tried with this patch not to change anything in case SYSRESET is >>>> enabled too much and since the file isn't too large I thought that would >>>> be ok for now. >>> >>> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? >> >> Not sure what you mean... sysreset implements do_reset: >> >> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 >> >> But the SPL does not have sysreset in this case, so it needs something >> different. > > Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ? Well that would probably not enough. I would also need settings for the watchdog, because the SPL does not have DM support, so while u-boot uses CONFIG_WATCHDOG the SPL uses CONFIG_HW_WATCHDOG. Easier that changing all this is something like this in the board header file (as I described in the commit description): #if defined(CONFIG_SPL_BUILD) #undef CONFIG_WDT #undef CONFIG_WATCHDOG #undef CONFIG_SYSRESET #define CONFIG_HW_WATCHDOG #endif In case of imx6, that way the SPL uses the hw_watchdog_reset from the imx watchdog driver instead of the 'watchdog_reset'. 'watchdog_reset' is not available since that is implemented in wdt-uclass.c and CONFIG_SPL_WDT depends on SPL_DM.
On 11/27/19 4:40 PM, Claudius Heine wrote: > On 27/11/2019 16.21, Marek Vasut wrote: >> On 11/27/19 4:17 PM, Claudius Heine wrote: >>> On 27/11/2019 16.12, Marek Vasut wrote: >>>> On 11/27/19 4:09 PM, Claudius Heine wrote: >>>>> Hi Marek, >>>>> >>>>> On 27/11/2019 15.47, Marek Vasut wrote: >>>>>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>>>>> file like this: >>>>>>> >>>>>>> #if defined(CONFIG_SPL_BUILD) >>>>>>> #undef CONFIG_WDT >>>>>>> #undef CONFIG_WATCHDOG >>>>>>> #undef CONFIG_SYSRESET >>>>>>> #define CONFIG_HW_WATCHDOG >>>>>>> #endif >>>>>>> >>>>>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>>>>> >>>>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>>>>> in case a panic occurs. >>>>>>> >>>>>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>>>>> --- >>>>>>> arch/arm/lib/Makefile | 2 -- >>>>>>> arch/arm/lib/reset.c | 2 ++ >>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>>>>> index 9de9a9acee..763eb4498f 100644 >>>>>>> --- a/arch/arm/lib/Makefile >>>>>>> +++ b/arch/arm/lib/Makefile >>>>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>>>>> else >>>>>>> obj-y += interrupts.o >>>>>>> endif >>>>>>> -ifndef CONFIG_SYSRESET >>>>>>> obj-y += reset.o >>>>>>> -endif >>>>>>> >>>>>>> obj-y += cache.o >>>>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>>>>> index f3ea116e87..11e680be1d 100644 >>>>>>> --- a/arch/arm/lib/reset.c >>>>>>> +++ b/arch/arm/lib/reset.c >>>>>>> @@ -22,6 +22,7 @@ >>>>>>> >>>>>>> #include <common.h> >>>>>>> >>>>>>> +#if !defined(CONFIG_SYSRESET) >>>>>>> __weak void reset_misc(void) >>>>>>> { >>>>>>> } >>>>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>>>> /*NOTREACHED*/ >>>>>>> return 0; >>>>>>> } >>>>>>> +#endif >>>>>> >>>>>> Does this mean there's now one huge ifdef around the entire source file? >>>>>> That's odd. >>>>> >>>>> Right. Other suggestions? >>>>> >>>>> Maybe having 'do_reset' here as a weak instead, so that sysreset can >>>>> overwrite it? But then the other definitions in arch/*/lib/reset.c >>>>> should probably be the same for consistency sake? >>>>> >>>>> I tried with this patch not to change anything in case SYSRESET is >>>>> enabled too much and since the file isn't too large I thought that would >>>>> be ok for now. >>>> >>>> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? >>> >>> Not sure what you mean... sysreset implements do_reset: >>> >>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 >>> >>> But the SPL does not have sysreset in this case, so it needs something >>> different. >> >> Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ? > > Well that would probably not enough. I would also need settings for the > watchdog, because the SPL does not have DM support, so while u-boot uses > CONFIG_WATCHDOG the SPL uses CONFIG_HW_WATCHDOG. > > Easier that changing all this is something like this in the board header > file (as I described in the commit description): > > #if defined(CONFIG_SPL_BUILD) > #undef CONFIG_WDT > #undef CONFIG_WATCHDOG > #undef CONFIG_SYSRESET > #define CONFIG_HW_WATCHDOG > #endif Can't we add DM watchdog to SPL instead ? > In case of imx6, that way the SPL uses the hw_watchdog_reset from the > imx watchdog driver instead of the 'watchdog_reset'. > > 'watchdog_reset' is not available since that is implemented in > wdt-uclass.c and CONFIG_SPL_WDT depends on SPL_DM.
On 27/11/2019 17.05, Marek Vasut wrote: > On 11/27/19 4:40 PM, Claudius Heine wrote: >> On 27/11/2019 16.21, Marek Vasut wrote: >>> On 11/27/19 4:17 PM, Claudius Heine wrote: >>>> On 27/11/2019 16.12, Marek Vasut wrote: >>>>> On 11/27/19 4:09 PM, Claudius Heine wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On 27/11/2019 15.47, Marek Vasut wrote: >>>>>>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>>>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>>>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>>>>>> file like this: >>>>>>>> >>>>>>>> #if defined(CONFIG_SPL_BUILD) >>>>>>>> #undef CONFIG_WDT >>>>>>>> #undef CONFIG_WATCHDOG >>>>>>>> #undef CONFIG_SYSRESET >>>>>>>> #define CONFIG_HW_WATCHDOG >>>>>>>> #endif >>>>>>>> >>>>>>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>>>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>>>>>> >>>>>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>>>>>> in case a panic occurs. >>>>>>>> >>>>>>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>>>>>> --- >>>>>>>> arch/arm/lib/Makefile | 2 -- >>>>>>>> arch/arm/lib/reset.c | 2 ++ >>>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>>>>>> index 9de9a9acee..763eb4498f 100644 >>>>>>>> --- a/arch/arm/lib/Makefile >>>>>>>> +++ b/arch/arm/lib/Makefile >>>>>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>>>>>> else >>>>>>>> obj-y += interrupts.o >>>>>>>> endif >>>>>>>> -ifndef CONFIG_SYSRESET >>>>>>>> obj-y += reset.o >>>>>>>> -endif >>>>>>>> >>>>>>>> obj-y += cache.o >>>>>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>>>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>>>>>> index f3ea116e87..11e680be1d 100644 >>>>>>>> --- a/arch/arm/lib/reset.c >>>>>>>> +++ b/arch/arm/lib/reset.c >>>>>>>> @@ -22,6 +22,7 @@ >>>>>>>> >>>>>>>> #include <common.h> >>>>>>>> >>>>>>>> +#if !defined(CONFIG_SYSRESET) >>>>>>>> __weak void reset_misc(void) >>>>>>>> { >>>>>>>> } >>>>>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>>>>> /*NOTREACHED*/ >>>>>>>> return 0; >>>>>>>> } >>>>>>>> +#endif >>>>>>> >>>>>>> Does this mean there's now one huge ifdef around the entire source file? >>>>>>> That's odd. >>>>>> >>>>>> Right. Other suggestions? >>>>>> >>>>>> Maybe having 'do_reset' here as a weak instead, so that sysreset can >>>>>> overwrite it? But then the other definitions in arch/*/lib/reset.c >>>>>> should probably be the same for consistency sake? >>>>>> >>>>>> I tried with this patch not to change anything in case SYSRESET is >>>>>> enabled too much and since the file isn't too large I thought that would >>>>>> be ok for now. >>>>> >>>>> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? >>>> >>>> Not sure what you mean... sysreset implements do_reset: >>>> >>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 >>>> >>>> But the SPL does not have sysreset in this case, so it needs something >>>> different. >>> >>> Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ? >> >> Well that would probably not enough. I would also need settings for the >> watchdog, because the SPL does not have DM support, so while u-boot uses >> CONFIG_WATCHDOG the SPL uses CONFIG_HW_WATCHDOG. >> >> Easier that changing all this is something like this in the board header >> file (as I described in the commit description): >> >> #if defined(CONFIG_SPL_BUILD) >> #undef CONFIG_WDT >> #undef CONFIG_WATCHDOG >> #undef CONFIG_SYSRESET >> #define CONFIG_HW_WATCHDOG >> #endif > > Can't we add DM watchdog to SPL instead ? Do you mean implementing SPL_DM support for this board so that we can use the DM watchdog and sysreset? Well you know more about this than me. But it probably comes down to technical limitations like size of the SPL. Lukasz has done something similar with the display5 board [1], but that uses PANIC_HANG to avoid this issue. [1] https://gitlab.denx.de/u-boot/u-boot/blob/4b19b89ca4a866b7baa642533e6dbd67cd832d27/include/configs/display5.h#L343 > >> In case of imx6, that way the SPL uses the hw_watchdog_reset from the >> imx watchdog driver instead of the 'watchdog_reset'. >> >> 'watchdog_reset' is not available since that is implemented in >> wdt-uclass.c and CONFIG_SPL_WDT depends on SPL_DM
On Wed, Nov 27, 2019 at 4:40 PM Claudius Heine <ch@denx.de> wrote: > > On 27/11/2019 16.21, Marek Vasut wrote: > > On 11/27/19 4:17 PM, Claudius Heine wrote: > >> On 27/11/2019 16.12, Marek Vasut wrote: > >>> On 11/27/19 4:09 PM, Claudius Heine wrote: > >>>> Hi Marek, > >>>> > >>>> On 27/11/2019 15.47, Marek Vasut wrote: > >>>>> On 11/27/19 3:20 PM, Claudius Heine wrote: > >>>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available > >>>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header > >>>>>> file like this: > >>>>>> > >>>>>> #if defined(CONFIG_SPL_BUILD) > >>>>>> #undef CONFIG_WDT > >>>>>> #undef CONFIG_WATCHDOG > >>>>>> #undef CONFIG_SYSRESET > >>>>>> #define CONFIG_HW_WATCHDOG > >>>>>> #endif > >>>>>> > >>>>>> 'do_reset' is called from SPL for instance from the panic handler in case > >>>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. > >>>>>> > >>>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior > >>>>>> in case a panic occurs. > >>>>>> > >>>>>> Signed-off-by: Claudius Heine <ch@denx.de> > >>>>>> --- > >>>>>> arch/arm/lib/Makefile | 2 -- > >>>>>> arch/arm/lib/reset.c | 2 ++ > >>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile > >>>>>> index 9de9a9acee..763eb4498f 100644 > >>>>>> --- a/arch/arm/lib/Makefile > >>>>>> +++ b/arch/arm/lib/Makefile > >>>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o > >>>>>> else > >>>>>> obj-y += interrupts.o > >>>>>> endif > >>>>>> -ifndef CONFIG_SYSRESET > >>>>>> obj-y += reset.o > >>>>>> -endif > >>>>>> > >>>>>> obj-y += cache.o > >>>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o > >>>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c > >>>>>> index f3ea116e87..11e680be1d 100644 > >>>>>> --- a/arch/arm/lib/reset.c > >>>>>> +++ b/arch/arm/lib/reset.c > >>>>>> @@ -22,6 +22,7 @@ > >>>>>> > >>>>>> #include <common.h> > >>>>>> > >>>>>> +#if !defined(CONFIG_SYSRESET) > >>>>>> __weak void reset_misc(void) > >>>>>> { > >>>>>> } > >>>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > >>>>>> /*NOTREACHED*/ > >>>>>> return 0; > >>>>>> } > >>>>>> +#endif > >>>>> > >>>>> Does this mean there's now one huge ifdef around the entire source file? > >>>>> That's odd. > >>>> > >>>> Right. Other suggestions? > >>>> > >>>> Maybe having 'do_reset' here as a weak instead, so that sysreset can > >>>> overwrite it? But then the other definitions in arch/*/lib/reset.c > >>>> should probably be the same for consistency sake? > >>>> > >>>> I tried with this patch not to change anything in case SYSRESET is > >>>> enabled too much and since the file isn't too large I thought that would > >>>> be ok for now. > >>> > >>> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? > >> > >> Not sure what you mean... sysreset implements do_reset: > >> > >> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 > >> > >> But the SPL does not have sysreset in this case, so it needs something > >> different. > > > > Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ? > > Well that would probably not enough. I would also need settings for the > watchdog, because the SPL does not have DM support, so while u-boot uses > CONFIG_WATCHDOG the SPL uses CONFIG_HW_WATCHDOG. > > Easier that changing all this is something like this in the board header > file (as I described in the commit description): > > #if defined(CONFIG_SPL_BUILD) > #undef CONFIG_WDT > #undef CONFIG_WATCHDOG > #undef CONFIG_SYSRESET > #define CONFIG_HW_WATCHDOG > #endif > > In case of imx6, that way the SPL uses the hw_watchdog_reset from the > imx watchdog driver instead of the 'watchdog_reset'. > > 'watchdog_reset' is not available since that is implemented in > wdt-uclass.c and CONFIG_SPL_WDT depends on SPL_DM. That seems totally unrelated to this patch. I think this patch should change the Makefile to use CONFIG_$(SPL_TPL_)SYSRESET and you need to solve your watchdog issue in a separate patch. Regards, Simon
On 11/28/19 9:10 AM, Claudius Heine wrote: > On 27/11/2019 17.05, Marek Vasut wrote: >> On 11/27/19 4:40 PM, Claudius Heine wrote: >>> On 27/11/2019 16.21, Marek Vasut wrote: >>>> On 11/27/19 4:17 PM, Claudius Heine wrote: >>>>> On 27/11/2019 16.12, Marek Vasut wrote: >>>>>> On 11/27/19 4:09 PM, Claudius Heine wrote: >>>>>>> Hi Marek, >>>>>>> >>>>>>> On 27/11/2019 15.47, Marek Vasut wrote: >>>>>>>> On 11/27/19 3:20 PM, Claudius Heine wrote: >>>>>>>>> In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available >>>>>>>>> anywere, even if SYSRESET is disabled for SPL in the board specific header >>>>>>>>> file like this: >>>>>>>>> >>>>>>>>> #if defined(CONFIG_SPL_BUILD) >>>>>>>>> #undef CONFIG_WDT >>>>>>>>> #undef CONFIG_WATCHDOG >>>>>>>>> #undef CONFIG_SYSRESET >>>>>>>>> #define CONFIG_HW_WATCHDOG >>>>>>>>> #endif >>>>>>>>> >>>>>>>>> 'do_reset' is called from SPL for instance from the panic handler in case >>>>>>>>> SPL_USB_SDP is enabled and PANIC_HANG is not set. >>>>>>>>> >>>>>>>>> Setting PANIC_HANG would solve this issue, but it also changes the behavior >>>>>>>>> in case a panic occurs. >>>>>>>>> >>>>>>>>> Signed-off-by: Claudius Heine <ch@denx.de> >>>>>>>>> --- >>>>>>>>> arch/arm/lib/Makefile | 2 -- >>>>>>>>> arch/arm/lib/reset.c | 2 ++ >>>>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile >>>>>>>>> index 9de9a9acee..763eb4498f 100644 >>>>>>>>> --- a/arch/arm/lib/Makefile >>>>>>>>> +++ b/arch/arm/lib/Makefile >>>>>>>>> @@ -56,9 +56,7 @@ obj-y += interrupts_64.o >>>>>>>>> else >>>>>>>>> obj-y += interrupts.o >>>>>>>>> endif >>>>>>>>> -ifndef CONFIG_SYSRESET >>>>>>>>> obj-y += reset.o >>>>>>>>> -endif >>>>>>>>> >>>>>>>>> obj-y += cache.o >>>>>>>>> obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o >>>>>>>>> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c >>>>>>>>> index f3ea116e87..11e680be1d 100644 >>>>>>>>> --- a/arch/arm/lib/reset.c >>>>>>>>> +++ b/arch/arm/lib/reset.c >>>>>>>>> @@ -22,6 +22,7 @@ >>>>>>>>> >>>>>>>>> #include <common.h> >>>>>>>>> >>>>>>>>> +#if !defined(CONFIG_SYSRESET) >>>>>>>>> __weak void reset_misc(void) >>>>>>>>> { >>>>>>>>> } >>>>>>>>> @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>>>>>> /*NOTREACHED*/ >>>>>>>>> return 0; >>>>>>>>> } >>>>>>>>> +#endif >>>>>>>> >>>>>>>> Does this mean there's now one huge ifdef around the entire source file? >>>>>>>> That's odd. >>>>>>> >>>>>>> Right. Other suggestions? >>>>>>> >>>>>>> Maybe having 'do_reset' here as a weak instead, so that sysreset can >>>>>>> overwrite it? But then the other definitions in arch/*/lib/reset.c >>>>>>> should probably be the same for consistency sake? >>>>>>> >>>>>>> I tried with this patch not to change anything in case SYSRESET is >>>>>>> enabled too much and since the file isn't too large I thought that would >>>>>>> be ok for now. >>>>>> >>>>>> What if sysreset implemented do_reset ? Wouldn't that solve the issue ? >>>>> >>>>> Not sure what you mean... sysreset implements do_reset: >>>>> >>>>> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/sysreset/sysreset-uclass.c#L112 >>>>> >>>>> But the SPL does not have sysreset in this case, so it needs something >>>>> different. >>>> >>>> Oh, so you need CONFIG_$(SPL_TPL_)SYSRESET then ? >>> >>> Well that would probably not enough. I would also need settings for the >>> watchdog, because the SPL does not have DM support, so while u-boot uses >>> CONFIG_WATCHDOG the SPL uses CONFIG_HW_WATCHDOG. >>> >>> Easier that changing all this is something like this in the board header >>> file (as I described in the commit description): >>> >>> #if defined(CONFIG_SPL_BUILD) >>> #undef CONFIG_WDT >>> #undef CONFIG_WATCHDOG >>> #undef CONFIG_SYSRESET >>> #define CONFIG_HW_WATCHDOG >>> #endif >> >> Can't we add DM watchdog to SPL instead ? > > Do you mean implementing SPL_DM support for this board so that we can > use the DM watchdog and sysreset? Isn't SPL DM already implemented ? [...]
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 9de9a9acee..763eb4498f 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -56,9 +56,7 @@ obj-y += interrupts_64.o else obj-y += interrupts.o endif -ifndef CONFIG_SYSRESET obj-y += reset.o -endif obj-y += cache.o obj-$(CONFIG_SYS_ARM_CACHE_CP15) += cache-cp15.o diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c index f3ea116e87..11e680be1d 100644 --- a/arch/arm/lib/reset.c +++ b/arch/arm/lib/reset.c @@ -22,6 +22,7 @@ #include <common.h> +#if !defined(CONFIG_SYSRESET) __weak void reset_misc(void) { } @@ -40,3 +41,4 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /*NOTREACHED*/ return 0; } +#endif
In case CONFIG_SYSRESET is set, do_reset from reset.c will not be available anywere, even if SYSRESET is disabled for SPL in the board specific header file like this: #if defined(CONFIG_SPL_BUILD) #undef CONFIG_WDT #undef CONFIG_WATCHDOG #undef CONFIG_SYSRESET #define CONFIG_HW_WATCHDOG #endif 'do_reset' is called from SPL for instance from the panic handler in case SPL_USB_SDP is enabled and PANIC_HANG is not set. Setting PANIC_HANG would solve this issue, but it also changes the behavior in case a panic occurs. Signed-off-by: Claudius Heine <ch@denx.de> --- arch/arm/lib/Makefile | 2 -- arch/arm/lib/reset.c | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)