[PATCH 0/3] watchdog: honour hw_margin_ms property

[Rasmus Villemoes]

On 02/06/2020 16.53, Stefan Roese wrote:
> On 02.06.20 15:29, Rasmus Villemoes wrote:
>> On 16/03/2020 16.52, Rasmus Villemoes wrote:
>>> On 14/03/2020 13.04, Stefan Roese wrote:
>>>> On 13.03.20 17:04, Rasmus Villemoes wrote:
>>>
>>>>> That at least solves half my problems and
>>>>> might be useful to others as well. Then I'll have to figure out the
>>>>> time-stands-still problem in some other way.
>>>>
>>>> If its too hard to enable interrupts in SPL for you or to provide some
>>>> other means of a working get_timer() API, then we needto find another
>>>> solution. You started with this weak function, which of course works.
>>>> What other options are there? Adding a callback mechanism to register
>>>> platform specific callback functions? Even though this might get a
>>>> little bit too complicated.
>>>
>>> Now that I dig a bit more into this, I seem to remember that we actually
>>> also had problems in U-Boot proper when loading a compressed kernel, so
>>> for now we're using an uncompressed kernel in our FIT images. I will
>>> have to re-investigate, but it now occurs to me that it might be due to
>>> the fact that interrupts get disabled during bootm (which makes sense,
>>> the same reason I stated previously of interrupt vectors about to be
>>> overwritten), so even in U-Boot proper, time as measured by get_timer()
>>> ceases to pass after that point, so all the WATCHDOG_RESET() calls from
>>> the inflate code effectively get ignored.
>>>
>>> So it may be necessary to have some wdt_ratelimit_disable() hook that
>>> can be called from bootm_disable_interrupts() and e.g. some
>>> board-specific SPL code. I'll do some experiments and figure out if I do
>>> indeed need such a hook.
>>
>> OK, I have now had time to do some more experiments. I have enabled the
>> timer tick in SPL, so get_timer() now "normally" works. Together with
>> the .dts based read of the hardware margin, that makes the watchdog
>> handling mostly work.
>>
>> But, as I suspected, I do have a problem when loading a compressed
>> kernel image - what I write above "so even in U-Boot proper, time as
>> measured by get_timer() ceases to pass after that point, so all the
>> WATCHDOG_RESET() calls from the inflate code effectively get ignored."
>> is indeed the case.
>>
>> So, what's the best way to proceed? Should there be a hook disabling the
>>   rate-limiting logic that bootm_disable_interrupts() can call? Or must
>> get_timer() always return a sensible result even with interrupts
>> disabled?
> 
> Wouldn't it make sense to move the bootm_disable_interrupts() call to
> after loading and uncompressing the OS image? To right before jumping
> to the OS?

No, because the point of disabling interrupts is that we may start
writing to physical address 0 (e.g. if that's the load= address in the
FIT image), which is also where the interrupt vectors reside - i.e.,
we're about to overwrite 0x900 (the decrementer interrupt vector), so if
we don't disable interrupts, we'll crash on the very next decrementer
interrupt (i.e., within one millisecond).

Rasmus