Hi Jan,
sorry for the delay. A few comments below.
On 04.09.26 15:01, Jan Kiszka wrote:
> +static int i6300esb_wdt_start(struct udevice *dev, u64 ms, ulong flags)
> +{
> + struct i6300_wdt *wdt = dev_get_priv(dev);
> + uint32_t val;
> +
> + /*
> + * Scale to ns, then divide by 983040 ns, the period of the timer
> + * (30 ns PCI bus tick * 2^15). This is timer 1 (IRQ). Timer 2
> + * (reboot) is set to immediate expiry.
> + */
> + val = ms * 1000000ULL / 983040;
There is no upper limit here. The preload registers are 20 bits wide
(Linux caps the heartbeat at 2046s, 2046 << 9 per timer, just below
2^20), so with your scaling anything above roughly 1030s silently
wraps. Please clamp it. Once Juuso's series ("watchdog: report
effective timeout") is in next, setting max_timeout_ms in uc_plat in
probe would be the preferred way.
> +
> + unlock_timer_regs(wdt);
> + writel(val, ESB_TIMER1_REG(wdt));
> +
> + unlock_timer_regs(wdt);
> + writel(0, ESB_TIMER2_REG(wdt));
> +
> + dm_pci_write_config8(dev, ESB_LOCK_REG, ESB_WDT_ENABLE);
Linux does a reload after writing the preload values
(esb_timer_set_heartbeat()) and again in esb_timer_start() before
enabling. Without it the new value is only picked up with the first
wdt_reset(), so the first period still runs with whatever was
preloaded before. I'd add the reload here, before enabling.
> +static int i6300esb_wdt_stop(struct udevice *dev)
> +{
[...]
> + /* Returns 0 if the timer was disabled, non-zero otherwise */
> + return val & ESB_WDT_ENABLE;
That's taken over from Linux, but in U-Boot the callers expect 0 or a
negative error code. Something like -EBUSY when the enable bit is
still set (locked) would fit better.
> +static int i6300esb_wdt_probe(struct udevice *dev)
> +{
> + struct i6300_wdt *wdt = dev_get_priv(dev);
> + uint8_t val;
> +
> + wdt->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0, 0, 0,
> + PCI_REGION_TYPE, PCI_REGION_MEM);
Please check for NULL here.
Nit: checkpatch --strict prefers u32/u8 over uint32_t/uint8_t.
Thanks,
Stefan
