On 22/03/2022 13:43, Guenter Roeck wrote:
> On 3/22/22 01:38, Sondhauß, Jan wrote: > When the watchdog is
> initialized and triggered before the kernel runs, > it must be ensured
> that the kernel uses a different trigger pattern. > Otherwise the
> watchdog cannot be kicked. > ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
>
> On 3/22/22 01:38, Sondhauß, Jan wrote:
>> When the watchdog is initialized and triggered before the kernel runs,
>> it must be ensured that the kernel uses a different trigger pattern.
>> Otherwise the watchdog cannot be kicked.
>>
>> Reading the current counter reload trigger pattern from the watchdog
>> hardware during probing makes sure that the trigger pattern is different
>> from the one previously used.
>>
>
> It is kind of annoying that u-boot uses the same trigger pattern. Question
> though: What happens if the register was not initialized by the ROM monitor
> and contains the power-up value. Is the chip ok with using that as base ?
In the current implementation the value is negated before writing to the
chip. After reset the register contains 0. In this case the driver would
alternate between ~0 and 0.
From the technical reference manual (SPRUH73P): Writing a different
value than the one already written in the Watchdog Trigger Register does
a watchdog counter reload.
As far as I can tell it should work. But I just noticed a different
problem with my patch: The access to the watchdog happens before the
hardware is enabled via pm_runtime enable. So in the current state that
could lead to an access violation..
>
> If not it might be easier to just use some other pattern like 0xfeedface
> or even some pseudo-random value.
The would be easier indeed. The idea behind reading out the current
value is that maybe the driver could get copied from the kernel again.
What do you suggest as a V2 patch? What kind of testing should I
provide? I only have a am335x soc at hand.
Thanks,
Jan
>
> Thanks,
> Guenter
>
>> Signed-off-by: Jan Sondhauss <jan.sondhauss@xxxxxxxx>
>> ---
>> drivers/watchdog/omap_wdt.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
>> index 74d785b2b478..680a34588425 100644
>> --- a/drivers/watchdog/omap_wdt.c
>> +++ b/drivers/watchdog/omap_wdt.c
>> @@ -85,6 +85,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
>> /* reloaded WCRR from WLDR */
>> }
>>
>> +static void omap_wdt_init_trgr_pattern(struct omap_wdt_dev *wdev)
>> +{
>> + void __iomem *base = wdev->base;
>> +
>> + wdev->wdt_trgr_pattern = readl_relaxed(base + OMAP_WATCHDOG_TGR);
>> +}
>> +
>> static void omap_wdt_enable(struct omap_wdt_dev *wdev)
>> {
>> void __iomem *base = wdev->base;
>> @@ -238,7 +245,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>>
>> wdev->omap_wdt_users = false;
>> wdev->dev = &pdev->dev;
>> - wdev->wdt_trgr_pattern = 0x1234;
>> mutex_init(&wdev->lock);
>>
>> /* reserve static register mappings */
>> @@ -253,6 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
>> wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
>> wdev->wdog.parent = &pdev->dev;
>>
>> + omap_wdt_init_trgr_pattern(wdev);
>> +
>> watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev);
>>
>> watchdog_set_nowayout(&wdev->wdog, nowayout);
>
