On 31/07/2021 12.06, Stefan Roese wrote: > Hi Rasmus, > > On 15.07.21 10:15, Stefan Roese wrote: >> Hi Rasmus, >> >> On 05.07.21 17:30, Simon Glass wrote: >>> Hi Rasmus, >>> >>> On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes >>> <rasmus.villem...@prevas.dk> wrote: >>>> >>>> A board can have and make use of more than one watchdog device, say >>>> one built into the SOC and an external gpio-petted one. Having >>>> wdt-uclass only handle the first is both a little arbitrary and >>>> unexpected. >>>> >>>> So change initr_watchdog() so we visit (probe) all DM watchdog >>>> devices. This essentially boils down to making the init_watchdog_dev >>>> function into a .post_probe method. >>>> >>>> Similarly let watchdog_reset() loop over the whole uclass - each >>>> having their own ratelimiting metadata, and a separate "is this device >>>> running" flag. >>>> >>>> This gets rid of the watchdog_dev member of struct global_data. We >>>> do, however, still need the GD_FLG_WDT_READY set in >>>> initr_watchdog(). This is because watchdog_reset() can get called >>>> before DM is ready, and I don't think we can call uclass_get() that >>>> early. >>>> >>>> The current code just returns 0 if "getting" the first device fails - >>>> that can of course happen because there are no devices, but it could >>>> also happen if its ->probe call failed. In keeping with that, continue >>>> with the handling of the remaining devices even if one fails to >>>> probe. This is also why we cannot use uclass_probe_all(). >>>> >>>> If desired, it's possible to later add a per-device "u-boot,autostart" >>>> boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART >>>> per-device. >>>> >>>> Reviewed-by: Stefan Roese <s...@denx.de> >>>> Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> >>>> --- >>>> drivers/watchdog/wdt-uclass.c | 96 >>>> ++++++++++++++++++------------- >>>> include/asm-generic/global_data.h | 6 -- >>>> 2 files changed, 56 insertions(+), 46 deletions(-) >>>> >>> >>> Please see my belated reply on the previous version of this patch. >> >> Rasmus, do you plan to send an updated patchset version, addressing >> Simons's comments? > > Any updates on this?
Just got back from vacation and trying to catch up. I'm a little confused. On June 29, you said I'm fine with this post_probe() implementation but have no strong feelings about this. So if Simon (or someone else) does not object, then please continue this way. Then Simon does reply on July 5 Imagine you probe the device but then go into SDRAM init that hangs the CPU for a few seconds. We need a way to signal that we want the watchdog to start, so the board owner has a choice as to when this happens. [The board owner does have a choice, it's called CONFIG_WATCHDOG_AUTOSTART.] I also understand this is not quite how things work at present and I'm fine with copying the existing behaviour. which I didn't read as an objection, as I am precisely not changing any existing behaviour. As I've said previously, keeping what is done in my current post_probe function in a separate helper, called from inside the loop in initr_watchdog which probes all watchdog devices, won't change anything at all. But precisely because it won't change anything, in the interest of getting the gpio driver in, I'll send a v4 with that change alone, then I'll let Stefan pick whichever version he and Simon can agree to. But let me one last time repeat why I think the post_probe approach is the cleanest and a natural fit for DM: post_probe is (AIUI) a place where a uclass can do some action it wants done for every device belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set, wdt_uclass does have such an action. When it's not set, the post_probe method is a no-op. Rasmus
