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

Wed Apr 10 09:05:31 UTC 2019

On 10. 04. 19 10:52, Stefan Roese wrote:
> On 10.04.19 10:46, Michal Simek wrote:
>> On 10. 04. 19 10:40, Stefan Roese wrote:
>>> 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>
>>
>> On this board disabling watchdog servicing was done by purpose.
> 
> And it will stay disabled even with the imply, as the defconfig
> currently disables CONFIG_WATCHDOG. So no change here.

yup

> 
> BTW: bitmain_antminer_s9_defconfig seems to be the only board that
> disables CONFIG_WATCHDOG.

Possible and that's the board I used for tuning this feature.

M