[U-Boot] [RFC PATCH] ARM: reset: Move SYSRESET condition from Makefile into source file

[Simon Goldschmidt]

On Wed, Nov 27, 2019 at 4:40 PM Claudius Heine <ch at 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 at 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