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@denx.de >> Cc: Heiko Schocher hs@denx.de >> Cc: Tom Rini trini@konsulko.com >> Cc: Michal Simek michal.simek@xilinx.com >> Cc: "Marek Behún" marek.behun@nic.cz >> Cc: Daniel Schwierzeck daniel.schwierzeck@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@denx.de smartweb: Heiko Schocher hs@denx.de ast2500: Maxim Sloyko maxims@google.com picosam9g45: Erik van Luijk evanluijk@interact.nl mt7623n_bpir2: Ryder Lee ryder.lee@mediatek.com Weijie Gao weijie.gao@mediatek.com mt7629_rfb: Ryder Lee ryder.lee@mediatek.com Weijie Gao weijie.gao@mediatek.com bitmain_antminer_s9: Michal Simek monstr@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.
BTW: bitmain_antminer_s9_defconfig seems to be the only board that disables CONFIG_WATCHDOG.
Thanks, Stefan