| Message ID | 20210527220017.1266765-8-rasmus.villemoes@prevas.dk |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | handling all DM watchdogs in watchdog_reset() | expand |
Hi Rasmus, On Thu, 27 May 2021 at 16:00, Rasmus Villemoes <rasmus.villemoes@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. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/watchdog/wdt-uclass.c | 87 ++++++++++++++++--------------- > include/asm-generic/global_data.h | 6 --- > 2 files changed, 46 insertions(+), 47 deletions(-) > > diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c > index 026b6d1eb4..e062a75574 100644 > --- a/drivers/watchdog/wdt-uclass.c > +++ b/drivers/watchdog/wdt-uclass.c > @@ -25,44 +25,20 @@ struct wdt_priv { > bool running; > }; > > -static void init_watchdog_dev(struct udevice *dev) > +int initr_watchdog(void) > { > - struct wdt_priv *priv; > + struct udevice *dev; > + struct uclass *uc; > int ret; > > - priv = dev_get_uclass_priv(dev); > - > - if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { > - printf("WDT: Not starting %s\n", dev->name); > - return; > - } > - > - ret = wdt_start(dev, priv->timeout * 1000, 0); > - if (ret != 0) { > - printf("WDT: Failed to start %s\n", dev->name); > - return; > + ret = uclass_get(UCLASS_WDT, &uc); > + if (ret) { > + printf("Error getting UCLASS_WDT: %d\n", ret); log_debug()as this should not happen > + return 0; Should return error here > } > > - printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, > - IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); > -} > - > -int initr_watchdog(void) > -{ > - /* > - * Init watchdog: This will call the probe function of the > - * watchdog driver, enabling the use of the device > - */ > - if (uclass_get_device_by_seq(UCLASS_WDT, 0, > - (struct udevice **)&gd->watchdog_dev)) { > - debug("WDT: Not found by seq!\n"); > - if (uclass_get_device(UCLASS_WDT, 0, > - (struct udevice **)&gd->watchdog_dev)) { > - printf("WDT: Not found!\n"); > - return 0; > - } > - } > - init_watchdog_dev(gd->watchdog_dev); > + uclass_foreach_dev(dev, uc) > + device_probe(dev); If watchdog fails, should we not stop? Even if we don't, I think some sort of message should be shown so people know to fix it. > > gd->flags |= GD_FLG_WDT_READY; > return 0; > @@ -145,22 +121,26 @@ void watchdog_reset(void) > { > struct wdt_priv *priv; > struct udevice *dev; > + struct uclass *uc; > ulong now; > > /* Exit if GD is not ready or watchdog is not initialized yet */ > if (!gd || !(gd->flags & GD_FLG_WDT_READY)) > return; > > - dev = gd->watchdog_dev; > - priv = dev_get_uclass_priv(dev); > - if (!priv->running) > + if (uclass_get(UCLASS_WDT, &uc)) > return; > > - /* Do not reset the watchdog too often */ > - now = get_timer(0); > - if (time_after_eq(now, priv->next_reset)) { > - priv->next_reset = now + priv->reset_period; > - wdt_reset(dev); > + uclass_foreach_dev(dev, uc) { Don't you need to use foreach_dev_probe() ? > + priv = dev_get_uclass_priv(dev); > + if (!priv->running) > + continue; > + /* Do not reset the watchdog too often */ > + now = get_timer(0); > + if (time_after_eq(now, priv->next_reset)) { > + priv->next_reset = now + priv->reset_period; > + wdt_reset(dev); > + } > } > } > #endif > @@ -214,11 +194,36 @@ static int wdt_pre_probe(struct udevice *dev) > return 0; > } > > +static int wdt_post_probe(struct udevice *dev) > +{ > + struct wdt_priv *priv; > + int ret; > + > + priv = dev_get_uclass_priv(dev); > + > + if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { > + printf("WDT: Not starting %s\n", dev->name); log_debug() > + return 0; > + } > + > + ret = wdt_start(dev, priv->timeout * 1000, 0); > + if (ret != 0) { > + printf("WDT: Failed to start %s\n", dev->name); log_debug() > + return 0; > + } I don't think it is good to start it in the post-probe. Can you do it separately, afterwards? Probing is supposed to just probe it and it should be safe to do that without side effects. > + > + printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, > + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); log_debug() I think > + > + return 0; > +} > + > UCLASS_DRIVER(wdt) = { > .id = UCLASS_WDT, > .name = "watchdog", > .flags = DM_UC_FLAG_SEQ_ALIAS, > .post_bind = wdt_post_bind, > .pre_probe = wdt_pre_probe, > + .post_probe = wdt_post_probe, > .per_device_auto = sizeof(struct wdt_priv), > }; > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h > index 47921d27b1..b554016628 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -445,12 +445,6 @@ struct global_data { > */ > fdt_addr_t translation_offset; > #endif > -#if CONFIG_IS_ENABLED(WDT) > - /** > - * @watchdog_dev: watchdog device > - */ > - struct udevice *watchdog_dev; > -#endif > #ifdef CONFIG_GENERATE_ACPI_TABLE > /** > * @acpi_ctx: ACPI context pointer > -- > 2.31.1 > Regards, Simon
Hi Simon, Thanks for review. >> - if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { >> - printf("WDT: Not starting %s\n", dev->name); >> - return; >> - } >> - >> - ret = wdt_start(dev, priv->timeout * 1000, 0); >> - if (ret != 0) { >> - printf("WDT: Failed to start %s\n", dev->name); >> - return; >> + ret = uclass_get(UCLASS_WDT, &uc); >> + if (ret) { >> + printf("Error getting UCLASS_WDT: %d\n", ret); > > log_debug()as this should not happen OK. [I assume the rationale is: don't add .text which will most likely never be used, but allow the debug statements to be easily turned on per-TU if one should ever need it.] >> + return 0; > > Should return error here And have the initr sequence stop completely instead of trying to limp along? Why? Note that I touched upon this in the commit message: The existing code doesn't pass on an error from uclass_get_device*() [which would most likely come from an underlying probe call], and returns 0 regardless from initr_watchdog(). I see no point changing that. >> - } >> - init_watchdog_dev(gd->watchdog_dev); >> + uclass_foreach_dev(dev, uc) >> + device_probe(dev); > > If watchdog fails, should we not stop? Even if we don't, I think some > sort of message should be shown so people know to fix it. I'd assume device_probe() would print an error message. If not, sure, I can add some [log_debug?] message. >> >> gd->flags |= GD_FLG_WDT_READY; >> return 0; >> @@ -145,22 +121,26 @@ void watchdog_reset(void) >> { >> struct wdt_priv *priv; >> struct udevice *dev; >> + struct uclass *uc; >> ulong now; >> >> /* Exit if GD is not ready or watchdog is not initialized yet */ >> if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >> return; >> >> - dev = gd->watchdog_dev; >> - priv = dev_get_uclass_priv(dev); >> - if (!priv->running) >> + if (uclass_get(UCLASS_WDT, &uc)) >> return; >> >> - /* Do not reset the watchdog too often */ >> - now = get_timer(0); >> - if (time_after_eq(now, priv->next_reset)) { >> - priv->next_reset = now + priv->reset_period; >> - wdt_reset(dev); >> + uclass_foreach_dev(dev, uc) { > > Don't you need to use foreach_dev_probe() ? Why? They've all been probed in initr_watchdog(). And the guts of WATHCDOG_RESET(), which can be and is called from everywhere, is certainly not the place to do anything other than actually pinging the watchdog devices. >> +static int wdt_post_probe(struct udevice *dev) >> +{ >> + struct wdt_priv *priv; >> + int ret; >> + >> + priv = dev_get_uclass_priv(dev); >> + >> + if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { >> + printf("WDT: Not starting %s\n", dev->name); > > log_debug() Perhaps, but this is just existing code I've moved around. >> + return 0; >> + } >> + >> + ret = wdt_start(dev, priv->timeout * 1000, 0); >> + if (ret != 0) { >> + printf("WDT: Failed to start %s\n", dev->name); > > log_debug() Ditto. >> + return 0; >> + } > > I don't think it is good to start it in the post-probe. Can you do it > separately, afterwards? Eh, yes, of course I could do this in the loop in initr_watchdog() where I probe all watchdog devices, but the end result is exactly the same, and it seemed that this was a perfect fit for DM since it provided this post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, and if it is, that precisely means the developer wanted to start handling the device(s) ASAP. > Probing is supposed to just probe it and it > should be safe to do that without side effects. I agree in general, but watchdog devices are a bit special compared to some random USB controller or LCD display or spi master - those devices generally don't do anything unless asked to by the CPU, while a watchdog device is the other way around, it does its thing _unless_ the CPU asks it not to (often enough). Which in turn, I suppose, is the whole reason wdt-uclass has its own hook into the initr sequence - one needs to probe and possibly start handling the watchdog(s) ASAP. BTW, next on my agenda is hooking in even earlier so the few boards that use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely random places in init_sequence_f[] is a bit silly. >> + >> + printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, >> + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); > > log_debug() I think Ditto, existing code moved around. Thanks, Rasmus
On 22.06.21 22:28, Rasmus Villemoes wrote: > Hi Simon, > > Thanks for review. > >>> - if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { >>> - printf("WDT: Not starting %s\n", dev->name); >>> - return; >>> - } >>> - >>> - ret = wdt_start(dev, priv->timeout * 1000, 0); >>> - if (ret != 0) { >>> - printf("WDT: Failed to start %s\n", dev->name); >>> - return; >>> + ret = uclass_get(UCLASS_WDT, &uc); >>> + if (ret) { >>> + printf("Error getting UCLASS_WDT: %d\n", ret); >> >> log_debug()as this should not happen > > OK. [I assume the rationale is: don't add .text which will most likely > never be used, but allow the debug statements to be easily turned on > per-TU if one should ever need it.] > >>> + return 0; >> >> Should return error here > > And have the initr sequence stop completely instead of trying to limp > along? Why? Note that I touched upon this in the commit message: The > existing code doesn't pass on an error from uclass_get_device*() [which > would most likely come from an underlying probe call], and returns 0 > regardless from initr_watchdog(). I see no point changing that. > >>> - } >>> - init_watchdog_dev(gd->watchdog_dev); >>> + uclass_foreach_dev(dev, uc) >>> + device_probe(dev); >> >> If watchdog fails, should we not stop? Even if we don't, I think some >> sort of message should be shown so people know to fix it. > > I'd assume device_probe() would print an error message. If not, sure, I > can add some [log_debug?] message. > >>> >>> gd->flags |= GD_FLG_WDT_READY; >>> return 0; >>> @@ -145,22 +121,26 @@ void watchdog_reset(void) >>> { >>> struct wdt_priv *priv; >>> struct udevice *dev; >>> + struct uclass *uc; >>> ulong now; >>> >>> /* Exit if GD is not ready or watchdog is not initialized yet */ >>> if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>> return; >>> >>> - dev = gd->watchdog_dev; >>> - priv = dev_get_uclass_priv(dev); >>> - if (!priv->running) >>> + if (uclass_get(UCLASS_WDT, &uc)) >>> return; >>> >>> - /* Do not reset the watchdog too often */ >>> - now = get_timer(0); >>> - if (time_after_eq(now, priv->next_reset)) { >>> - priv->next_reset = now + priv->reset_period; >>> - wdt_reset(dev); >>> + uclass_foreach_dev(dev, uc) { >> >> Don't you need to use foreach_dev_probe() ? > > Why? They've all been probed in initr_watchdog(). And the guts of > WATHCDOG_RESET(), which can be and is called from everywhere, is > certainly not the place to do anything other than actually pinging the > watchdog devices. > >>> +static int wdt_post_probe(struct udevice *dev) >>> +{ >>> + struct wdt_priv *priv; >>> + int ret; >>> + >>> + priv = dev_get_uclass_priv(dev); >>> + >>> + if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { >>> + printf("WDT: Not starting %s\n", dev->name); >> >> log_debug() > > Perhaps, but this is just existing code I've moved around. I would prefer to not change (remove) the currently existing output of the status of the watchdog driver. If necessary, it could be changed to log_info(). >>> + return 0; >>> + } >>> + >>> + ret = wdt_start(dev, priv->timeout * 1000, 0); >>> + if (ret != 0) { >>> + printf("WDT: Failed to start %s\n", dev->name); >> >> log_debug() > > Ditto. > >>> + return 0; >>> + } >> >> I don't think it is good to start it in the post-probe. Can you do it >> separately, afterwards? > > Eh, yes, of course I could do this in the loop in initr_watchdog() where > I probe all watchdog devices, but the end result is exactly the same, > and it seemed that this was a perfect fit for DM since it provided this > post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, > and if it is, that precisely means the developer wanted to start > handling the device(s) ASAP. > >> Probing is supposed to just probe it and it >> should be safe to do that without side effects. > > I agree in general, but watchdog devices are a bit special compared to > some random USB controller or LCD display or spi master - those devices > generally don't do anything unless asked to by the CPU, while a watchdog > device is the other way around, it does its thing _unless_ the CPU asks > it not to (often enough). Which in turn, I suppose, is the whole reason > wdt-uclass has its own hook into the initr sequence - one needs to probe > and possibly start handling the watchdog(s) ASAP. > > BTW, next on my agenda is hooking in even earlier so the few boards that > use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and > be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely > random places in init_sequence_f[] is a bit silly. Nice. >>> + >>> + printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, >>> + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); >> >> log_debug() I think > > Ditto, existing code moved around. Again, please don't change this to debug level. Thanks, Stefan
Hi Rasmus, On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > Hi Simon, > > Thanks for review. > > >> - if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { > >> - printf("WDT: Not starting %s\n", dev->name); > >> - return; > >> - } > >> - > >> - ret = wdt_start(dev, priv->timeout * 1000, 0); > >> - if (ret != 0) { > >> - printf("WDT: Failed to start %s\n", dev->name); > >> - return; > >> + ret = uclass_get(UCLASS_WDT, &uc); > >> + if (ret) { > >> + printf("Error getting UCLASS_WDT: %d\n", ret); > > > > log_debug()as this should not happen > > OK. [I assume the rationale is: don't add .text which will most likely > never be used, but allow the debug statements to be easily turned on > per-TU if one should ever need it.] Yes, also the error return value should give you a clue. Please, use log_msg_ret() on your return values. > > >> + return 0; > > > > Should return error here > > And have the initr sequence stop completely instead of trying to limp > along? Why? Note that I touched upon this in the commit message: The > existing code doesn't pass on an error from uclass_get_device*() [which > would most likely come from an underlying probe call], and returns 0 > regardless from initr_watchdog(). I see no point changing that. OK, I'll leave it to you. My feeling is that failure is a good way to get a bug fixed. > > >> - } > >> - init_watchdog_dev(gd->watchdog_dev); > >> + uclass_foreach_dev(dev, uc) > >> + device_probe(dev); > > > > If watchdog fails, should we not stop? Even if we don't, I think some > > sort of message should be shown so people know to fix it. > > I'd assume device_probe() would print an error message. If not, sure, I > can add some [log_debug?] message. No driver model never prints messages. Yes you can use log_debuig(). > > >> > >> gd->flags |= GD_FLG_WDT_READY; > >> return 0; > >> @@ -145,22 +121,26 @@ void watchdog_reset(void) > >> { > >> struct wdt_priv *priv; > >> struct udevice *dev; > >> + struct uclass *uc; > >> ulong now; > >> > >> /* Exit if GD is not ready or watchdog is not initialized yet */ > >> if (!gd || !(gd->flags & GD_FLG_WDT_READY)) > >> return; > >> > >> - dev = gd->watchdog_dev; > >> - priv = dev_get_uclass_priv(dev); > >> - if (!priv->running) > >> + if (uclass_get(UCLASS_WDT, &uc)) > >> return; > >> > >> - /* Do not reset the watchdog too often */ > >> - now = get_timer(0); > >> - if (time_after_eq(now, priv->next_reset)) { > >> - priv->next_reset = now + priv->reset_period; > >> - wdt_reset(dev); > >> + uclass_foreach_dev(dev, uc) { > > > > Don't you need to use foreach_dev_probe() ? > > Why? They've all been probed in initr_watchdog(). And the guts of > WATHCDOG_RESET(), which can be and is called from everywhere, is > certainly not the place to do anything other than actually pinging the > watchdog devices. Then you should add a comment. You are using a low-level iterator, then assuming the devices are probed. > > >> +static int wdt_post_probe(struct udevice *dev) > >> +{ > >> + struct wdt_priv *priv; > >> + int ret; > >> + > >> + priv = dev_get_uclass_priv(dev); > >> + > >> + if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { > >> + printf("WDT: Not starting %s\n", dev->name); > > > > log_debug() > > Perhaps, but this is just existing code I've moved around. OK, well then you can leave it alone. > > >> + return 0; > >> + } > >> + > >> + ret = wdt_start(dev, priv->timeout * 1000, 0); > >> + if (ret != 0) { > >> + printf("WDT: Failed to start %s\n", dev->name); > > > > log_debug() > > Ditto. > > >> + return 0; > >> + } > > > > I don't think it is good to start it in the post-probe. Can you do it > > separately, afterwards? > > Eh, yes, of course I could do this in the loop in initr_watchdog() where > I probe all watchdog devices, but the end result is exactly the same, > and it seemed that this was a perfect fit for DM since it provided this > post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, > and if it is, that precisely means the developer wanted to start > handling the device(s) ASAP. > > > Probing is supposed to just probe it and it > > should be safe to do that without side effects. > > I agree in general, but watchdog devices are a bit special compared to > some random USB controller or LCD display or spi master - those devices > generally don't do anything unless asked to by the CPU, while a watchdog > device is the other way around, it does its thing _unless_ the CPU asks > it not to (often enough). Which in turn, I suppose, is the whole reason > wdt-uclass has its own hook into the initr sequence - one needs to probe > and possibly start handling the watchdog(s) ASAP. It still needs a 'start' method to make it start. Having it start on probe means the board will reset at the command line if the device is probed. Yuck. > > BTW, next on my agenda is hooking in even earlier so the few boards that > use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and > be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely > random places in init_sequence_f[] is a bit silly. OK. > > >> + > >> + printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, > >> + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); > > > > log_debug() I think > > Ditto, existing code moved around. OK. Regards, Simon
On 26/06/2021 20.32, Simon Glass wrote: > Hi Rasmus, > > On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: >> >>> I don't think it is good to start it in the post-probe. Can you do it >>> separately, afterwards? >> >> Eh, yes, of course I could do this in the loop in initr_watchdog() where >> I probe all watchdog devices, but the end result is exactly the same, >> and it seemed that this was a perfect fit for DM since it provided this >> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, >> and if it is, that precisely means the developer wanted to start >> handling the device(s) ASAP. >> >>> Probing is supposed to just probe it and it >>> should be safe to do that without side effects. >> >> I agree in general, but watchdog devices are a bit special compared to >> some random USB controller or LCD display or spi master - those devices >> generally don't do anything unless asked to by the CPU, while a watchdog >> device is the other way around, it does its thing _unless_ the CPU asks >> it not to (often enough). Which in turn, I suppose, is the whole reason >> wdt-uclass has its own hook into the initr sequence - one needs to probe >> and possibly start handling the watchdog(s) ASAP. > > It still needs a 'start' method to make it start. Having it start on > probe means the board will reset at the command line if the device is > probed. Yuck. No, because while sitting in the command line waiting for user input, WATCHDOG_RESET() is called something like a million times per second (or at least extremely often). For the most common case of there only being one (or zero) DM watchdogs, I'm not changing anything at all about how things behave. I'm just expanding the handling done in the wdt-uclass provided functions initr_watchdog() and watchdog_reset() to all DM watchdogs, making things more consistent. And there's CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe function into a no-op. As I said, yes, I can move the call of the post_probe function into the loop in initr_watchdog (and then it wouldn't be called post_probe, but probably named something including auto_start). In practice, that won't change anything. Stefan, what do you think? I think this is the only contentious point at this time, so I'll do whatever you think is right, then resend the patches with Simon's other feedback incorporated. Rasmus
On 28.06.21 09:44, Rasmus Villemoes wrote: > On 26/06/2021 20.32, Simon Glass wrote: >> Hi Rasmus, >> >> On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes >> <rasmus.villemoes@prevas.dk> wrote: >>> > >>>> I don't think it is good to start it in the post-probe. Can you do it >>>> separately, afterwards? >>> >>> Eh, yes, of course I could do this in the loop in initr_watchdog() where >>> I probe all watchdog devices, but the end result is exactly the same, >>> and it seemed that this was a perfect fit for DM since it provided this >>> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, >>> and if it is, that precisely means the developer wanted to start >>> handling the device(s) ASAP. >>> >>>> Probing is supposed to just probe it and it >>>> should be safe to do that without side effects. >>> >>> I agree in general, but watchdog devices are a bit special compared to >>> some random USB controller or LCD display or spi master - those devices >>> generally don't do anything unless asked to by the CPU, while a watchdog >>> device is the other way around, it does its thing _unless_ the CPU asks >>> it not to (often enough). Which in turn, I suppose, is the whole reason >>> wdt-uclass has its own hook into the initr sequence - one needs to probe >>> and possibly start handling the watchdog(s) ASAP. >> >> It still needs a 'start' method to make it start. Having it start on >> probe means the board will reset at the command line if the device is >> probed. Yuck. > > No, because while sitting in the command line waiting for user input, > WATCHDOG_RESET() is called something like a million times per second (or > at least extremely often). We have some rate-limiting in place since a few years (reset_period). > For the most common case of there only being > one (or zero) DM watchdogs, I'm not changing anything at all about how > things behave. I'm just expanding the handling done in the wdt-uclass > provided functions initr_watchdog() and watchdog_reset() to all DM > watchdogs, making things more consistent. And there's > CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe > function into a no-op. > > As I said, yes, I can move the call of the post_probe function into the > loop in initr_watchdog (and then it wouldn't be called post_probe, but > probably named something including auto_start). In practice, that won't > change anything. > > Stefan, what do you think? I think this is the only contentious point at > this time, so I'll do whatever you think is right, then resend the > patches with Simon's other feedback incorporated. 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. Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan
Hi Rasmus, On Mon, 28 Jun 2021 at 01:44, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 26/06/2021 20.32, Simon Glass wrote: > > Hi Rasmus, > > > > On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > >> > > >>> I don't think it is good to start it in the post-probe. Can you do it > >>> separately, afterwards? > >> > >> Eh, yes, of course I could do this in the loop in initr_watchdog() where > >> I probe all watchdog devices, but the end result is exactly the same, > >> and it seemed that this was a perfect fit for DM since it provided this > >> post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, > >> and if it is, that precisely means the developer wanted to start > >> handling the device(s) ASAP. > >> > >>> Probing is supposed to just probe it and it > >>> should be safe to do that without side effects. > >> > >> I agree in general, but watchdog devices are a bit special compared to > >> some random USB controller or LCD display or spi master - those devices > >> generally don't do anything unless asked to by the CPU, while a watchdog > >> device is the other way around, it does its thing _unless_ the CPU asks > >> it not to (often enough). Which in turn, I suppose, is the whole reason > >> wdt-uclass has its own hook into the initr sequence - one needs to probe > >> and possibly start handling the watchdog(s) ASAP. > > > > It still needs a 'start' method to make it start. Having it start on > > probe means the board will reset at the command line if the device is > > probed. Yuck. > > No, because while sitting in the command line waiting for user input, > WATCHDOG_RESET() is called something like a million times per second (or > at least extremely often). For the most common case of there only being > one (or zero) DM watchdogs, I'm not changing anything at all about how > things behave. I'm just expanding the handling done in the wdt-uclass > provided functions initr_watchdog() and watchdog_reset() to all DM > watchdogs, making things more consistent. And there's > CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe > function into a no-op. > > As I said, yes, I can move the call of the post_probe function into the > loop in initr_watchdog (and then it wouldn't be called post_probe, but > probably named something including auto_start). In practice, that won't > change anything. > > Stefan, what do you think? I think this is the only contentious point at > this time, so I'll do whatever you think is right, then resend the > patches with Simon's other feedback incorporated. From a driver model perspective, it is good practice to have a 'start' method separate from probing. I understand that you are saying that the watchdog should always be enabled, but we can handle that with a separate method to start it. I don't think it needs to be started in the post-probe method. We can just start it later. 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. I also understand this is not quite how things work at present and I'm fine with copying the existing behaviour. It's just that you are introducing a driver model interface and I have pretty strong views about the separation between probe at start, with that. Regards, Simon
diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 026b6d1eb4..e062a75574 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -25,44 +25,20 @@ struct wdt_priv { bool running; }; -static void init_watchdog_dev(struct udevice *dev) +int initr_watchdog(void) { - struct wdt_priv *priv; + struct udevice *dev; + struct uclass *uc; int ret; - priv = dev_get_uclass_priv(dev); - - if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { - printf("WDT: Not starting %s\n", dev->name); - return; - } - - ret = wdt_start(dev, priv->timeout * 1000, 0); - if (ret != 0) { - printf("WDT: Failed to start %s\n", dev->name); - return; + ret = uclass_get(UCLASS_WDT, &uc); + if (ret) { + printf("Error getting UCLASS_WDT: %d\n", ret); + return 0; } - printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, - IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); -} - -int initr_watchdog(void) -{ - /* - * Init watchdog: This will call the probe function of the - * watchdog driver, enabling the use of the device - */ - if (uclass_get_device_by_seq(UCLASS_WDT, 0, - (struct udevice **)&gd->watchdog_dev)) { - debug("WDT: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, - (struct udevice **)&gd->watchdog_dev)) { - printf("WDT: Not found!\n"); - return 0; - } - } - init_watchdog_dev(gd->watchdog_dev); + uclass_foreach_dev(dev, uc) + device_probe(dev); gd->flags |= GD_FLG_WDT_READY; return 0; @@ -145,22 +121,26 @@ void watchdog_reset(void) { struct wdt_priv *priv; struct udevice *dev; + struct uclass *uc; ulong now; /* Exit if GD is not ready or watchdog is not initialized yet */ if (!gd || !(gd->flags & GD_FLG_WDT_READY)) return; - dev = gd->watchdog_dev; - priv = dev_get_uclass_priv(dev); - if (!priv->running) + if (uclass_get(UCLASS_WDT, &uc)) return; - /* Do not reset the watchdog too often */ - now = get_timer(0); - if (time_after_eq(now, priv->next_reset)) { - priv->next_reset = now + priv->reset_period; - wdt_reset(dev); + uclass_foreach_dev(dev, uc) { + priv = dev_get_uclass_priv(dev); + if (!priv->running) + continue; + /* Do not reset the watchdog too often */ + now = get_timer(0); + if (time_after_eq(now, priv->next_reset)) { + priv->next_reset = now + priv->reset_period; + wdt_reset(dev); + } } } #endif @@ -214,11 +194,36 @@ static int wdt_pre_probe(struct udevice *dev) return 0; } +static int wdt_post_probe(struct udevice *dev) +{ + struct wdt_priv *priv; + int ret; + + priv = dev_get_uclass_priv(dev); + + if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) { + printf("WDT: Not starting %s\n", dev->name); + return 0; + } + + ret = wdt_start(dev, priv->timeout * 1000, 0); + if (ret != 0) { + printf("WDT: Failed to start %s\n", dev->name); + return 0; + } + + printf("WDT: Started %s with%s servicing (%ds timeout)\n", dev->name, + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout); + + return 0; +} + UCLASS_DRIVER(wdt) = { .id = UCLASS_WDT, .name = "watchdog", .flags = DM_UC_FLAG_SEQ_ALIAS, .post_bind = wdt_post_bind, .pre_probe = wdt_pre_probe, + .post_probe = wdt_post_probe, .per_device_auto = sizeof(struct wdt_priv), }; diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 47921d27b1..b554016628 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -445,12 +445,6 @@ struct global_data { */ fdt_addr_t translation_offset; #endif -#if CONFIG_IS_ENABLED(WDT) - /** - * @watchdog_dev: watchdog device - */ - struct udevice *watchdog_dev; -#endif #ifdef CONFIG_GENERATE_ACPI_TABLE /** * @acpi_ctx: ACPI context pointer
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. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/watchdog/wdt-uclass.c | 87 ++++++++++++++++--------------- include/asm-generic/global_data.h | 6 --- 2 files changed, 46 insertions(+), 47 deletions(-)