[U-Boot] [PATCH 1/2 v2] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 09. 04. 19 16:34, Stefan Roese wrote:
> On 09.04.19 16:27, Michal Simek wrote:
>> On 09. 04. 19 16:22, Stefan Roese wrote:
>>> On 09.04.19 12:45, Michal Simek wrote:
>>>> On 08. 04. 19 11:28, Stefan Roese wrote:
>>>>> This patch tries to implement a generic watchdog_reset() function that
>>>>> can be used by all boards that want to service the watchdog device in
>>>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG.
>>>>>
>>>>> Without this approach, new boards or platforms needed to implement a
>>>>> board specific version of this functionality, mostly copy'ing the same
>>>>> code over and over again into their board or platforms code base.
>>>>>
>>>>> With this new generic function, the scattered other functions are now
>>>>> removed to be replaced by the generic one. The new version also
>>>>> enables
>>>>> the configuration of the watchdog timeout via the DT "timeout-sec"
>>>>> property (if enabled via CONFIG_OF_CONTROL).
>>>>>
>>>>> This patch also adds a new flag to the GD flags, to flag that the
>>>>> watchdog is ready to use and adds the pointer to the watchdog device
>>>>> to the GD. This enables us to remove the global "watchdog_dev"
>>>>> variable, which was prone to cause problems because of its potentially
>>>>> very early use in watchdog_reset(), even before the BSS is cleared.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>> Cc: Heiko Schocher <hs at denx.de>
>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>>>> Cc: "Marek Behún" <marek.behun at nic.cz>
>>>>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>>>> ---
>>>>> This patch depends on the following patches:
>>>>>
>>>>> [1] watchdog: Move watchdog_dev to data section (BSS may not be
>>>>> cleared)
>>>>> https://patchwork.ozlabs.org/patch/1075500/
>>>>>
>>>>> [2] watchdog: Handle SPL build with watchdog disabled
>>>>> https://patchwork.ozlabs.org/patch/1074098/
>>>>>
>>>>> I would like to see [1] applied in this release, since its a real
>>>>> fix on
>>>>> some of the platforms with minimal chances of breakage.
>>>>>
>>>>> This patch now is a bigger rework and is intended for the next merge
>>>>> window.
>>>>>
>>>>> Please note that I didn't remove the "timeout-sec" handling from the
>>>>> driver already reading it from the DT (cdns & at91) in this patch.
>>>>> The reason for this is, that in the cdns case, the removal also brings
>>>>> a functional change, which I wanted to do in a separate patch. And
>>>>> in the at91 case its because there are updates to this driver already
>>>>> queued in the at91 next git branch which would most likely cause merge
>>>>> conflict. I'll send a cleanup patch for this driver later after these
>>>>> patches have been applied.
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>> v2:
>>>>> - Rename watchdog_start() to initr_watchdog() and move it to
>>>>> board_r.c.
>>>>>     This way its only called once, after the DM subsystem has bound
>>>>> all
>>>>>     watchdog drivers. This enables the use of the currently
>>>>> implemented
>>>>>     logic of using the correct watchdog in U-Boot (via alias etc).
>>>>>
>>>>>    arch/mips/mach-mt7620/cpu.c                   | 36
>>>>> -----------------
>>>>>    board/CZ.NIC/turris_mox/turris_mox.c          | 30 --------------
>>>>>    board/CZ.NIC/turris_omnia/turris_omnia.c      | 35 ----------------
>>>>>    .../microblaze-generic/microblaze-generic.c   | 40
>>>>> -------------------
>>>>>    board/xilinx/zynq/board.c                     | 39
>>>>> ------------------
>>>>>    board/xilinx/zynqmp/zynqmp.c                  | 39
>>>>> ------------------
>>>>>    common/board_r.c                              | 40
>>>>> +++++++++++++++++++
>>>>>    drivers/watchdog/wdt-uclass.c                 | 26 ++++++++++++
>>>>>    include/asm-generic/global_data.h             |  4 ++
>>>>>    9 files changed, 70 insertions(+), 219 deletions(-)
>>>
>>> <snip>
>>>
>>>>>    int board_early_init_r(void)
>>>>>    {
>>>>>        u32 val;
>>>>> diff --git a/common/board_r.c b/common/board_r.c
>>>>> index 472987d5d5..d80f16c3ed 100644
>>>>> --- a/common/board_r.c
>>>>> +++ b/common/board_r.c
>>>>> @@ -48,6 +48,7 @@
>>>>>    #include <linux/compiler.h>
>>>>>    #include <linux/err.h>
>>>>>    #include <efi_loader.h>
>>>>> +#include <wdt.h>
>>>>>      DECLARE_GLOBAL_DATA_PTR;
>>>>>    @@ -621,6 +622,42 @@ static int initr_bedbug(void)
>>>>>    }
>>>>>    #endif
>>>>>    +#if defined(CONFIG_WATCHDOG) && defined(CONFIG_WDT)
>>>>
>>>> This is not correct.
>>>> Here should be just CONFIG_WDT.
>>>>
>>>> Because there are cases where you want just start watchdog in u-boot
>>>> but
>>>> never service it by u-boot.
>>>
>>> AFAIK, this would change the current behavior. Currently, only when
>>> CONFIG_WATCHDOG is enabled the watchdog driver is started automatically
>>> in U-Boot (and serviced). If I make the change you suggest above, all
>>> boards defining CONFIG_WDT (DM watchdog support) will automatically
>>> enable the watchdog. This might make sense, but AFAICT this changes
>>> the current behavior.
>>
>> The question is what default is. Because when I was adding support for
>> Zynq/ZynqMP/Microblaze this logic is used there.
>> I think that WDT should be there and if you think there are boards which
>> want to have both we can cover that by Kconfig.
> 
> As mentioned above, I agree that it makes sense to start the watchdog
> automatically, if its enabled / selected via CONFIG_WDT. I suggest
> that I move forward with your suggested change, but it would be fair
> to Cc the maintainers of boards that have CONFIG_WDT set but
> CONFIG_WATCHDOG unset. Do you (or anyone else) know of a tool / script
> to detect such board configurations in U-Boot (one option set and
> another unset)?

good.
Tom?

This should be also fine.

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 10fd3039aa20..3c0f90559b86 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -51,6 +51,7 @@ config ULP_WATCHDOG
 config WDT
        bool "Enable driver model for watchdog timer drivers"
        depends on DM
+       imply WATCHDOG
        help
          Enable driver model for watchdog timer. At the moment the API
          is very simple and only supports four operations:



M