Hi Guenter,
Thank you for reviewing this patch.
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Thursday, 5 November, 2020 5:18 AM
> Subject: Re: [PATCH v1 1/2] watchdog: Add watchdog driver for Intel Keembay
> Soc
>
> On Mon, Nov 02, 2020 at 01:23:11PM +0800,
> vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
> > +
> > +static void keembay_wdt_set_timeout_reg(struct watchdog_device *wdog,
> bool ping)
> > +{
> > + struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > + u32 th_val = 0;
> > +
> > + if (ping)
> > + keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout *
> wdt->rate);
> > +
> > + if (wdog->pretimeout)
> > + th_val = wdog->timeout - wdog->pretimeout;
> > +
> > + keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val * wdt-
> >rate);
> > +
> > + if (ping)
> > + keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout *
> wdt->rate);
> > +}
> > +
> > +static int keembay_wdt_start(struct watchdog_device *wdog)
> > +{
> > + struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > +
> > + keembay_wdt_set_timeout_reg(wdog, 0);
>
> The second parameter is bool, so please use true/false.
Yes. Let me incorporate the change in the next version.
> Anyway, why does starting the watchdog require setting the pretimeout
> register, but not setting the timeout register ?
>
Good catch. Let me fix the sequence in the next version.
> > + keembay_wdt_writel(wdt, TIM_WDOG_EN, 1);
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_wdt_stop(struct watchdog_device *wdog)
> > +{
> > + struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > +
> > + keembay_wdt_writel(wdt, TIM_WDOG_EN, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_wdt_ping(struct watchdog_device *wdog)
> > +{
> > + keembay_wdt_set_timeout_reg(wdog, 1);
>
> Each ping requires setting the timeout twice, plus setting the
> pretimeout ? Really ?
>
Not really twice.
TIM_WATCHDOG has to be updated for both true and false conditions
at the argument 2. Let me fix that change in the next version.
> Is this document somewhere. I think I'll want to confirm this with
> the specification. The TIM_WATCHDOG register seems to need a lot
> of updates, and I'd like to have some confirmation that this is
> really necessary.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_wdt_set_timeout(struct watchdog_device *wdog, u32 t)
> > +{
> > + u32 actual = min(t, wdog->max_timeout);
> > +
> min() is unnecessary.
>
OK. I will use t directly in the next version.
> > + wdog->timeout = actual;
> > + keembay_wdt_set_timeout_reg(wdog, 0);
>
> Why does setting the timeout require setting the pretimeout register,
> but not setting the timeout register ?
>
Let me fix the sequence in the next version.
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_wdt_set_pretimeout(struct watchdog_device *wdog,
> u32 t)
> > +{
> > + if (t < wdog->min_timeout || t >= wdog->timeout)
> > + return -EINVAL;
>
> Validated by watchdog core.
>
Sure. I will remove this check in the next version.
> > +
> > + wdog->pretimeout = t;
> > + keembay_wdt_set_timeout_reg(wdog, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int keembay_wdt_get_timeleft(struct watchdog_device
> *wdog)
> > +{
> > + struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > +
> > + return keembay_wdt_readl(wdt, TIM_WATCHDOG) / wdt->rate;
> > +}
> > +
> > +/*
> > + * SMC call is used to clear the interrupt bits, because the
> TIM_GEN_CONFIG
> > + * register is in the secure bank.
> > + */
> > +static irqreturn_t keembay_wdt_to_isr(int irq, void *dev_id)
> > +{
> > + struct keembay_wdt *wdt = dev_id;
> > + struct arm_smccc_res res;
> > +
> > + keembay_wdt_writel(wdt, TIM_WATCHDOG, 1);
> > + arm_smccc_smc(WDT_ISR_CLEAR, WDT_ISR_MASK, 0, 0, 0, 0, 0, 0,
> &res);
> > + dev_crit(wdt->wdd.parent, "Intel Keem Bay non-sec wdt timeout.\n");
> > + emergency_restart();
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t keembay_wdt_th_isr(int irq, void *dev_id)
> > +{
> > + struct keembay_wdt *wdt = dev_id;
> > + struct arm_smccc_res res;
> > + u32 th_val = 0;
> > +
> > + if (wdt->wdd.pretimeout)
> > + th_val = wdt->wdd.timeout - wdt->wdd.pretimeout;
> > +
> > + keembay_wdt_writel(wdt, TIM_WATCHDOG, th_val * wdt->rate + 1);
> > + arm_smccc_smc(WDT_ISR_CLEAR, WDT_ISR_MASK, 0, 0, 0, 0, 0, 0,
> &res);
> > + dev_crit(wdt->wdd.parent, "Intel Keem Bay non-sec wdt pre-
> timeout.\n");
> > + watchdog_notify_pretimeout(&wdt->wdd);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct watchdog_info keembay_wdt_info = {
> > + .identity = "Intel Keem Bay Watchdog Timer",
> > + .options = WDIOF_SETTIMEOUT |
> > + WDIOF_PRETIMEOUT |
> > + WDIOF_MAGICCLOSE |
> > + WDIOF_KEEPALIVEPING,
> > +};
> > +
> > +static const struct watchdog_ops keembay_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = keembay_wdt_start,
> > + .stop = keembay_wdt_stop,
> > + .ping = keembay_wdt_ping,
> > + .set_timeout = keembay_wdt_set_timeout,
> > + .set_pretimeout = keembay_wdt_set_pretimeout,
> > + .get_timeleft = keembay_wdt_get_timeleft,
> > +};
> > +
> > +static int keembay_wdt_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct keembay_wdt *wdt;
> > + int ret;
> > +
> > + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> > + if (!wdt)
> > + return -ENOMEM;
> > +
> > + wdt->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(wdt->base))
> > + return PTR_ERR(wdt->base);
> > +
> > + /* we do not need to enable the clock as it is enabled by default */
> > + wdt->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(wdt->clk))
> > + return dev_err_probe(dev, PTR_ERR(wdt->clk), "Failed to get
> clock\n");
> > +
> > + wdt->rate = clk_get_rate(wdt->clk);
> > + if (!wdt->rate)
> > + return dev_err_probe(dev, -EINVAL, "Failed to get clock
> rate\n");
> > +
> > + wdt->th_irq = platform_get_irq_byname(pdev, "threshold");
> > + if (wdt->th_irq < 0)
> > + return dev_err_probe(dev, wdt->th_irq, "Failed to get IRQ for
> threshold\n");
> > +
> > + ret = devm_request_irq(dev, wdt->th_irq, keembay_wdt_th_isr, 0,
> > + "keembay-wdt", wdt);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to request IRQ for
> threshold\n");
> > +
> > + wdt->to_irq = platform_get_irq_byname(pdev, "timeout");
> > + if (wdt->to_irq < 0)
> > + return dev_err_probe(dev, wdt->to_irq, "Failed to get IRQ for
> timeout\n");
> > +
> > + ret = devm_request_irq(dev, wdt->to_irq, keembay_wdt_to_isr, 0,
> > + "keembay-wdt", wdt);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to request IRQ for
> timeout\n");
> > +
> > + wdt->wdd.parent = dev;
> > + wdt->wdd.info = &keembay_wdt_info;
> > + wdt->wdd.ops = &keembay_wdt_ops;
> > + wdt->wdd.min_timeout = WDT_LOAD_MIN;
> > + wdt->wdd.max_timeout = WDT_LOAD_MAX / wdt->rate;
> > + wdt->wdd.timeout = WDT_TIMEOUT;
> > +
> > + watchdog_set_drvdata(&wdt->wdd, wdt);
> > + watchdog_set_nowayout(&wdt->wdd, nowayout);
> > + watchdog_init_timeout(&wdt->wdd, timeout, dev);
> > + keembay_wdt_set_timeout(&wdt->wdd, wdt->wdd.timeout);
> > +
> > + ret = watchdog_register_device(&wdt->wdd);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to register watchdog
> device.\n");
> > +
> > + platform_set_drvdata(pdev, wdt);
> > + dev_info(dev, "Initial timeout %d sec%s.\n",
> > + wdt->wdd.timeout, nowayout ? ", nowayout" : "");
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused keembay_wdt_suspend(struct device *dev)
> > +{
> > + struct keembay_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(&wdt->wdd))
> > + return keembay_wdt_stop(&wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused keembay_wdt_resume(struct device *dev)
> > +{
> > + struct keembay_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(&wdt->wdd))
> > + return keembay_wdt_start(&wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(keembay_wdt_pm_ops,
> keembay_wdt_suspend,
> > + keembay_wdt_resume);
> > +
> > +static const struct of_device_id keembay_wdt_match[] = {
> > + { .compatible = "intel,keembay-wdt" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, keembay_wdt_match);
> > +
> > +static struct platform_driver keembay_wdt_driver = {
> > + .probe = keembay_wdt_probe,
>
> No remove function ? What happens if someone unloads
> the driver ?
>
Yes it is a problem.
Let me add the support in the next version.
I would like to go with resource managed watchdog registration to fulfil this need.
devm_watchdog_register_device()
> > + .driver = {
> > + .name = "keembay_wdt",
> > + .of_match_table = keembay_wdt_match,
> > + .pm = &keembay_wdt_pm_ops,
> > + },
> > +};
> > +
> > +module_platform_driver(keembay_wdt_driver);
> > +
> > +MODULE_DESCRIPTION("Intel Keem Bay SoC watchdog driver");
> > +MODULE_AUTHOR("Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
Thanks,
Vijay
