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

Wed Apr 10 08:40:49 UTC 2019

On 10.04.19 09:44, Michal Simek wrote:
> 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?

I've run a Travis job on all boards with an error added for this
configuration (CONFIG_WDT set but CONFIG_WATCHDOG unset). Here the
list of these boards with their maintainers:

taurus: Heiko Schocher <hs at denx.de>
smartweb: Heiko Schocher <hs at denx.de>
ast2500: Maxim Sloyko <maxims at google.com>
picosam9g45: Erik van Luijk <evanluijk at interact.nl>
mt7623n_bpir2: Ryder Lee <ryder.lee at mediatek.com> Weijie Gao <weijie.gao at mediatek.com>
mt7629_rfb: Ryder Lee <ryder.lee at mediatek.com> Weijie Gao <weijie.gao at mediatek.com>
bitmain_antminer_s9: Michal Simek <monstr at monstr.eu>
sandbox64: Simon Glass <sjg at chromium.org>
sandbox: Simon Glass <sjg at chromium.org>
comtrend_ct5361_ram: "Álvaro Fernández Rojas" <noltari at gmail.com>
netgear_cg3100d_ram: "Álvaro Fernández Rojas" <noltari at gmail.com>
bcm968380gerg_ram: Philippe Reynes <philippe.reynes at softathome.com>
bcm963158_ram: Philippe Reynes <philippe.reynes at softathome.com>
bcm968580xref_ram: Philippe Reynes <philippe.reynes at softathome.com>
MCR3000: Christophe Leroy <christophe.leroy at c-s.fr>

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

I like this idea to keep the current configuration but enabling
board maintainers to not use the U-Boot watchdog servicing feature
by disabling CONFIG_WATCHDOG if they want to.

I'll send v3 of this patch with the "imply" added and will add the
board maintainer to Cc, so that they can decide themselves.

Thanks,
Stefan