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

Hi Sashiko,

> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s)
> to consider:
> - [High] Incorrect timer reference clock mapping for watchdog nodes
> --
>=20
> --- Patch [8]: [PATCH v3 8/8] riscv: dts: eswin: add watchdog support ---
> commit bfe09d553ee566b62eb6947526013fd32a57d405
> Author: Pritesh Patel <[email protected]>
>=20
> riscv: dts: eswin: add watchdog support
>=20
> This commit adds watchdog nodes for the ESWIN EIC7700 SoC and enables
> them for the HiFive Premier P550 board.
>=20
> [ ... ]
>=20
> > 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 =3D "snps,dw-wdt";
> > +                     reg =3D <0x0 0x50800000 0x0 0x4000>;
> > +                     interrupts =3D <87>;
> > +                     clocks =3D <&clk EIC7700_CLK_GATE_LSP_WDT0_PCLK>;
> > +                     clock-names =3D "tclk";
>=20
> [Severity: High]
> Will this cause the watchdog to calculate timeouts incorrectly or freeze
> entirely?
>=20
> The snps,dw-wdt driver uses the clock named tclk to determine the frequen=
cy
> for timeout calculations. Here it is mapped to the APB interface clock
> (EIC7700_CLK_GATE_LSP_WDT0_PCLK).
>=20
> Since the ESWIN EIC7700 clock driver defines a separate watchdog referenc=
e
> 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.
>=20
> 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?
>=20
> 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

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