Re: [PATCH v3 8/8] riscv: dts: eswin: add watchdog support

Hi Sashiko,

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] Incorrect timer reference clock mapping for watchdog nodes
> --
> 
> --- Patch [8]: [PATCH v3 8/8] riscv: dts: eswin: add watchdog support ---
> commit bfe09d553ee566b62eb6947526013fd32a57d405
> Author: Pritesh Patel <[email protected]>
> 
> riscv: dts: eswin: add watchdog support
> 
> This commit adds watchdog nodes for the ESWIN EIC7700 SoC and enables
> them for the HiFive Premier P550 board.
> 
> [ ... ]
> 
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > index c13432d145042..f873e9eeb9290 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > @@ -265,6 +265,46 @@ syscon: syscon@50440000 {
> >                       };
> >               };
> >
> > +             wdt0: watchdog@50800000 {
> > +                     compatible = "snps,dw-wdt";
> > +                     reg = <0x0 0x50800000 0x0 0x4000>;
> > +                     interrupts = <87>;
> > +                     clocks = <&clk EIC7700_CLK_GATE_LSP_WDT0_PCLK>;
> > +                     clock-names = "tclk";
> 
> [Severity: High]
> Will this cause the watchdog to calculate timeouts incorrectly or freeze
> entirely?
> 
> The snps,dw-wdt driver uses the clock named tclk to determine the frequency
> for timeout calculations. Here it is mapped to the APB interface clock
> (EIC7700_CLK_GATE_LSP_WDT0_PCLK).
> 
> Since the ESWIN EIC7700 clock driver defines a separate watchdog reference
> clock (EIC7700_CLK_GATE_NOC_WDREF), providing the faster APB clock
> frequency to the driver might cause it to calculate extremely large tick counts.
> This would result in physical timeouts taking much longer than requested.
> 
> Additionally, if EIC7700_CLK_GATE_NOC_WDREF is not referenced here, could
> the common clock framework's clk_disable_unused late initcall gate it and
> completely freeze the watchdog counter?
> 
> This same clock mapping pattern appears in the wdt1, wdt2, and wdt3 nodes
> introduced below.

Since the watchdog in our hardware is driven directly by the APB clock,
EIC7700_CLK_GATE_LSP_WDT0_PCLK is used as the watchdog reference clock.
Therefore, it is specified as tclk in the device tree.
EIC7700_CLK_GATE_NOC_WDREF is dedicated to the separate NoC watchdog
and is not applicable to this watchdog instance.

Regards,
Pinkesh

> 
> > +                     resets = <&reset EIC7700_RESET_WDT0>;
> > +                     status = "disabled";
> > +             };
> 
> [ ... ]
> 
> --
> Sashiko AI review *
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsashik
> o.dev%2F%23%2Fpatchset%2F20260804104431.1391839-1-
> pinkesh.vaghela%40einfochips.com%3Fpart%3D8&data=05%7C02%7Cpinkesh
> .vaghela%40einfochips.com%7Cfe48de46d29048e4790508def21c5398%7C0be
> b0c359cbb4feb99e5589e415c7944%7C1%7C0%7C639214400661176845%7CU
> nknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAw
> MCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C
> &sdata=vO6SqAlUtK%2F7QM%2FaiH07F5SqH%2BmuSmCjoqk9FyHC4eY%3D&
> reserved=0