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

[Claudius Heine]

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?

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