Hi Greg, Thanks for the review.
> We should use a vendor prefix for this property "adi,sec" so that it > will be the same as what the kernel will require. That way we will be > able to switch to OF_UPSTREAM without a driver change once support > lands in the kernel. Makes sense. I'll rename it to "adi,secid" (the prefixed form of the current name) in v2 so it matches what the kernel binding will use. > I think you should just read it directly into the priv struct without > a temporary, i.e. dev_read_u32(dev, "secid", &priv->secid). Agreed, will do that in v2. > Also, what do you think about using dev_read_u32_default() with a > default of 3 (appropriately documented) so that any users with > modified or custom board device trees will not be surprised by this if > they update to the latest? Fair point on not surprising existing users. My only hesitation is that the SEC ID isn't really a value with a sensible universal default. It's a fixed per-SoC hardware constant. It's 3 on the current SC5xx parts, but on the upcoming ADSP SoC the watchdog maps to different SEC ID, for example. Defaulting to 3 keeps existing SC5xx device trees working (they all want 3 anyway), so it does address the custom-DT case you raise. The trade-off is that a future SoC whose DT omits the property would then probe "successfully" while programming the wrong SEC source and silently breaking the watchdog->reset path, rather than failing at probe. I'd lean slightly towards keeping it required so a misconfiguration is caught early. But I don't feel strongly, if you'd prefer the default for the custom-DT case, I'm okay to use dev_read_u32_default() with a comment documenting that 3 is the SC5xx value. Which would you prefer? Thanks, Ozan On Thu, Jul 30, 2026 at 7:44 PM Greg Malysa <[email protected]> wrote: > > Hi Ozan, > > On Thu, Jul 30, 2026 at 12:38 PM Ozan Durgut <[email protected]> wrote: > > > > From: UtsavAgarwalADI <[email protected]> > > > > The ADI watchdog drives a reset through a Security (SEC) controller > > fault source. The source index was hardcoded to 3 in adi_wdt_start(), > > which is only correct for the SC5xx parts that happen to use that > > index. > > > > Read the index from a new "secid" device tree property so boards that > > route the watchdog fault to a different SEC source can configure it. > > > > Set secid = <3> in the SC57x, SC58x, SC594 and SC598 device trees to > > match the value they relied on before. > > > > Co-developed-by: Ozan Durgut <[email protected]> > > Signed-off-by: Ozan Durgut <[email protected]> > > Signed-off-by: UtsavAgarwalADI <[email protected]> > > --- > > arch/arm/dts/sc57x.dtsi | 1 + > > arch/arm/dts/sc58x.dtsi | 1 + > > arch/arm/dts/sc594-som.dtsi | 1 + > > arch/arm/dts/sc598-som.dtsi | 1 + > > drivers/watchdog/adi_wdt.c | 9 ++++++++- > > 5 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/dts/sc57x.dtsi b/arch/arm/dts/sc57x.dtsi > > index e4cc612959f..794e80c3cf1 100644 > > --- a/arch/arm/dts/sc57x.dtsi > > +++ b/arch/arm/dts/sc57x.dtsi > > @@ -113,6 +113,7 @@ > > > > &wdog { > > clocks = <&clk ADSP_SC57X_CLK_CGU0_SCLK0>; > > + secid = <3>; > > }; > > > > ð0 { > > diff --git a/arch/arm/dts/sc58x.dtsi b/arch/arm/dts/sc58x.dtsi > > index 7b07589de47..d4008ba7988 100644 > > --- a/arch/arm/dts/sc58x.dtsi > > +++ b/arch/arm/dts/sc58x.dtsi > > @@ -158,6 +158,7 @@ > > > > &wdog { > > clocks = <&clk ADSP_SC58X_CLK_CGU0_SCLK0>; > > + secid = <3>; > > }; > > > > ð0 { > > diff --git a/arch/arm/dts/sc594-som.dtsi b/arch/arm/dts/sc594-som.dtsi > > index 1c2adc601dd..7a7167fbbba 100644 > > --- a/arch/arm/dts/sc594-som.dtsi > > +++ b/arch/arm/dts/sc594-som.dtsi > > @@ -67,6 +67,7 @@ > > > > &wdog { > > clocks = <&clk ADSP_SC594_CLK_CGU0_SCLK0>; > > + secid = <3>; > > }; > > > > &i2c0 { > > diff --git a/arch/arm/dts/sc598-som.dtsi b/arch/arm/dts/sc598-som.dtsi > > index bc212ef25cb..bca3ff765d6 100644 > > --- a/arch/arm/dts/sc598-som.dtsi > > +++ b/arch/arm/dts/sc598-som.dtsi > > @@ -124,6 +124,7 @@ > > > > &wdog { > > clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>; > > + secid = <3>; > > }; > > > > We should use a vendor prefix for this property "adi,sec" so that it > will be the same as what the kernel will require. That way we will be > able to switch to OF_UPSTREAM without a driver change once support > lands in the kernel. > > > &i2c0 { > > diff --git a/drivers/watchdog/adi_wdt.c b/drivers/watchdog/adi_wdt.c > > index 7d7cf98b55e..8e7ff958121 100644 > > --- a/drivers/watchdog/adi_wdt.c > > +++ b/drivers/watchdog/adi_wdt.c > > @@ -36,6 +36,7 @@ struct adi_wdt_priv { > > void __iomem *rcu_base; > > void __iomem *sec_base; > > void __iomem *wdt_base; > > + u32 secid; > > struct clk clock; > > }; > > > > @@ -72,7 +73,7 @@ static int adi_wdt_start(struct udevice *dev, u64 > > timeout_ms, ulong flags) > > iowrite32(0xc1, priv->sec_base + SEC_FCTL); > > > > /* enable SEC fault source for watchdog0 */ > > - setbits_32(priv->sec_base + SEC_SCTL0 + (3*8), 0x6); > > + setbits_32(priv->sec_base + SEC_SCTL0 + (priv->secid * 8), 0x6); > > > > /* Enable SYSCD_RESETb input */ > > iowrite32(0x100, priv->rcu_base + RCU_CTL); > > @@ -95,6 +96,7 @@ static int adi_wdt_probe(struct udevice *dev) > > struct adi_wdt_priv *priv = dev_get_priv(dev); > > int ret; > > struct resource res; > > + u32 secid; > > > > ret = dev_read_resource_byname(dev, "rcu", &res); > > if (ret) > > @@ -111,6 +113,11 @@ static int adi_wdt_probe(struct udevice *dev) > > return ret; > > priv->wdt_base = devm_ioremap(dev, res.start, resource_size(&res)); > > > > + ret = dev_read_u32(dev, "secid", &secid); > > + if (ret) > > + return ret; > > + priv->secid = secid; > > + > > I think you should just read it directly into the priv struct without > a temporary, i.e. dev_read_u32(dev, "secid", &priv->secid). > > Also, what do you think about using dev_read_u32_default() with a > default of 3 (appropriately documented) so that any users with > modified or custom board device trees will not be surprised by this if > they update to the latest? > > > ret = clk_get_by_name(dev, "sclk0", &priv->clock); > > if (ret < 0) { > > printf("Can't get WDT clk: %d\n", ret); > > -- > > 2.43.0 > > > > Thanks, > Greg
