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

[Marek Vasut]

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 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
>>
>> 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 ?

[...]