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

[Claudius Heine]

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.