On 12/03/2020 12.58, Stefan Roese wrote: > Hi Rasmus, > > (added Christophe to Cc) > > On 12.03.20 12:40, Rasmus Villemoes wrote: >> On the MPC8309-derived board I'm currently working on, the current >> rate-limiting logic in the generic watchdog_reset function poses two >> problems: >> >> First, the hard-coded limit of 1000ms is too large for the >> external GPIO-triggered watchdog we have. > > I remember a discussion a few weeks ago with Christophe, where you > pointed out, that there is already the official "hw_margin_ms" DT > property for this delay here. Why don't you introduce it here so that > all boards can use it as well?
Well, I considered that. Reading the value is probably just adding a dev_read_u32_default(..., 1000) next to the one reading the timeout-sec property in initr_watchdog(). But what is a good place to stash the result? It really belongs with the device, but as long as only one is supported I suppose one could move initr_watchdog to wdt-uclass.c and use a static variable in that file. I can certainly do that. That leaves the other problem. >> Second, in the SPL, the decrementer interrupt is not enabled, so the >> get_timer() call calls always returns 0, so wdt_reset() is never >> actually called. Enabling that interrupt (i.e. calling >> interrupt_init() somewhere in my early board code) is a bit >> problematic, since U-Boot proper gets loaded to address 0, hence >> overwriting exception vectors - and the reason I even need to care >> about the watchdog in the SPL is that the signature verification takes >> a rather long time, so just disabling interrupts before loading U-Boot >> proper doesn't work. >> >> Both problems can be solved by allowing the board to override the >> rate-limiting logic. For example, in my case I will implement the >> function in terms of get_ticks() (i.e. the time base registers on >> PPC) which do not depend on interrupts, and use a threshold of >> gd->bus_clk / (4*16) ticks (~62ms). > > I would assume that you might run into multiple issues, when your timer > infrastructure is not working correctly in SPL. None so far, except for the ratelimiting in terms of get_timer() not working. > Can't you instead "fix" > this by using this get_ticks() option for the get_timer() functions in > SPL so that you can use the common functions here and in all other > places in SPL as well? I don't understand what you mean. On PPC, get_timer(0) just returns a static variable which is incremented from the timer_interrupt(). When that interrupt is not enabled, that variable is always 0. I can enable the timer interrupt and get the time (in terms of get_timer) going, but as I wrote, I would have to disable that interrupt again before actually loading U-Boot [you can probably imagine the fun of debugging what happens when one overwrites the exception vectors], so time would stop passing as far as get_timer() is concerned, which means that again the watchdog would no longer be serviced. So if there is to be any ratelimiting at all, it really has to be based on a time that does tick without relying on interrupts. Rasmus
