Hi Guenter, Thanks for your review. On Wed, 11 Oct 2023 at 22:20, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > On Wed, Oct 11, 2023 at 07:48:18PM +0100, Peter Griffin wrote: > > This patch adds the compatibles and drvdata for the Google > > gs101 & gs201 SoCs found in Pixel 6 and Pixel 7 phones. Similar > > to Exynos850 it has two watchdog instances, one for each cluster > > and has some control bits in PMU registers. > > > > The watchdog IP found in gs101 SoCs also supports a few > > additional bits/features in the WTCON register which we add > > support for and an additional register detailed below. > > > > dbgack-mask - Enables masking WDT interrupt and reset request > > according to asserted DBGACK input > > > > windowed-mode - Enabled Windowed watchdog mode > > > > Windowed watchdog mode also has an additional register WTMINCNT. > > If windowed watchdog is enabled and you reload WTCNT when the > > value is greater than WTMINCNT, it prompts interrupt or reset > > request as if the watchdog time has expired. > > Sorry, I don't understand what the code is doing here. No need to be sorry, I appreciate the review feedback and questions :) > > It looks like it enables window mode unconditionally (?). If so, > what is the impact ? Does it mean that any code requesting multiple > keepalives in a row on the affected hardware will now cause an > immediate reset ? If so, what is the rationale ? Essentially yes, it stops continual keepalives being issued as a keepalive is only considered valid at certain times in the windowed cycle. By way of example, if the watchdog interval is 30s and wtmincnt is set to *half* of wtcnt, then we would have a "closed window" for the first 15s whereby issuing a keepalive will reset the system, and a "open window" for the second half of the interval where issuing a keepalive would reset wtcnt (and be considered a valid service of the watchdog). The rationale is to stop the watchdog being re-armed "too much" and it should enable detection of abnormally early as well as abnormally late servicing of the watchdog. > Alternatively, if it enables window mode and configures it such > that WTMINCNT is always equal or larger than WTCNT, what is the > point of enabling window mode in the first place ? I looked again, and you are indeed correct that the code currently is essentially turning windowed mode into a no-op. This appears to be a bug in the downstream driver as well. Setting WTMINCNT to half of WTCNT results in a 50% closed, 50% open window, and works on the hardware as I would expect. I guess this is a good example of why upstreaming your code, and public code review results in better quality drivers. Thanks, Peter
