Re: [PATCH 2/2] watchdog: Add Aspeed watchdog driver

[Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>]

Hey Guenter,

Thanks for the review.

On Tue, May 10, 2016 at 12:18 AM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> +#define WDT_STATUS             0x00
>> +#define WDT_RELOAD_VALUE       0x04
>> +#define WDT_RESTART            0x08
>> +#define WDT_CTRL               0x0C
>> +#define   WDT_CTRL_RESET_MODE_SOC      (0x00 << 5)
>> +#define   WDT_CTRL_RESET_MODE_FULL_CHIP        (0x01 << 5)
>> +#define          WDT_CTRL_1MHZ_CLK             BIT(4)
>> +#define          WDT_CTRL_WDT_EXT              BIT(3)
>> +#define          WDT_CTRL_WDT_INTR             BIT(2)
>> +#define   WDT_CTRL_RESET_SYSTEM                BIT(1)
>> +#define   WDT_CTRL_ENABLE              BIT(0)
>> +
>> +#define        WDT_RESTART_MAGIC       0x4755
>
>
> Please don't use tabs after #define.

Oops, typo. Thanks.

>
>> +
>> +static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
>> +{
>> +       return container_of(wdd, struct aspeed_wdt, wdd);
>> +}
>> +
>> +
>
> Please no double empty lines.
>
>> +static void aspeed_wdt_enable(struct aspeed_wdt *wdt, int count)
>> +{
>> +       wdt->ctrl |= WDT_CTRL_ENABLE;
>> +
>> +       writel(0, wdt->base + WDT_CTRL);
>> +       writel(count, wdt->base + WDT_RELOAD_VALUE);
>> +       writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
>> +       writel(wdt->ctrl, wdt->base + WDT_CTRL);
>
>
> Is it really necessary to write 0 into the ctrl register when enabling
> the watchdog ? Problem with it is two-fold: It changes the clock rate
> for a brief period of time, and it disables the watchdog while updating
> the timeout. Both is undesirable unless really needed.

In general I agree. However, the datasheet explicitly spells out these
four steps in this order. Writing zero is a shortcut; I could write
ctrl with the enable bit set to zero.

We've been using the driver in this fashion for the past year.

>> +}
>> +
>> +static int aspeed_wdt_start(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       dev_dbg(wdd->parent, "starting with timeout of %d (rate %lu)\n",
>> +                       wdd->timeout, wdt->rate);
>
>
> Are those dev_dbg() really useful ?

I did use them in writing the driver. They can go.

>
>> +       aspeed_wdt_enable(wdt, wdd->timeout * wdt->rate);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       wdt->ctrl &= ~WDT_CTRL_ENABLE;
>> +       writel(wdt->ctrl, wdt->base + WDT_CTRL);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       dev_dbg(wdd->parent, "ping\n");
>> +       writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>> +                                 unsigned int timeout)
>> +{
>> +       dev_dbg(wdd->parent, "timeout set to %u\n", timeout);
>> +       wdd->timeout = timeout;
>> +
>> +       return aspeed_wdt_start(wdd);
>> +}
>> +
>> +static int aspeed_wdt_restart(struct watchdog_device *wdd,
>> +                             unsigned long action, void *data)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       aspeed_wdt_enable(wdt, 128 * wdt->rate / 1000);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct watchdog_ops aspeed_wdt_ops = {
>> +       .start          = aspeed_wdt_start,
>> +       .stop           = aspeed_wdt_stop,
>> +       .ping           = aspeed_wdt_ping,
>> +       .set_timeout    = aspeed_wdt_set_timeout,
>> +       .restart        = aspeed_wdt_restart,
>> +       .owner          = THIS_MODULE,
>> +};
>> +
>> +static const struct watchdog_info aspeed_wdt_info = {
>> +       .options        = WDIOF_KEEPALIVEPING
>> +                       | WDIOF_MAGICCLOSE
>> +                       | WDIOF_SETTIMEOUT,
>> +       .identity       = KBUILD_MODNAME,
>> +};
>> +
>> +static int aspeed_wdt_remove(struct platform_device *pdev)
>> +{
>> +       struct aspeed_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +       watchdog_unregister_device(&wdt->wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_probe(struct platform_device *pdev)
>> +{
>> +       struct aspeed_wdt *wdt;
>> +       struct resource *res;
>> +       int ret;
>> +
>> +       wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>> +       if (!wdt)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       wdt->base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(wdt->base))
>> +               return PTR_ERR(wdt->base);
>> +
>> +       /*
>> +        * The ast2400 wdt can run at PCLK, or 1MHz. The ast2500 only
>> +        * runs at 1MHz. We chose to always run at 1MHz, as there's no
>> +        * good reason to have a faster watchdog counter.
>> +        */
>> +       wdt->rate = 1000000;
>
>
> Why not just use a define ?

I will add one.

The comment is informative though for people who read the ast2400
datasheet and wonder why we don't provide the option clocking with
pclk, I will leave it in.

>> +       wdt->wdd.info = &aspeed_wdt_info;
>> +       wdt->wdd.ops = &aspeed_wdt_ops;
>> +       wdt->wdd.min_timeout = 1;
>> +       wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
>> +       wdt->wdd.parent = &pdev->dev;
>> +
>> +       wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>
>
> Pretty much the same here. Since max_timeout is a constant,
> it is well known that it is larger than 30 seconds.
> Might as well use defines for max_timeout and timeout.

Ok.

>
>> +       watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>> +
>> +       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>
>
> Only reset tha SoC ? Are you sure this is what you want ?

I think this is left over from experiments when tracking down a reset
bug in u-boot. I will confirm and change it back to FULL_CHIP.

>
>> +               WDT_CTRL_1MHZ_CLK |
>> +               WDT_CTRL_RESET_SYSTEM;
>> +
>> +       /* Ensure hardware is in consistent state */
>> +       aspeed_wdt_stop(&wdt->wdd);
>> +
>
> An alternative might be to check if the watchdog is running, and inform
> the watchdog core about it so it can generate heartbeats as required
> until user space opens the watchdog device. This would ensure that the
> system is protected during boot.

I will implement this.

> ctrl is really static except for the enable flag. Not really sure
> if having a variable for it has any real benefits - you might as well
> just read the register and update the enable flag instead as needed when
> starting or stopping the watchdog. Is there a reason for not doing that ?

Not really. I find it cleaner to keep a copy of the register instead
of read-modify-write, but if you feel strongly about it I can change.

>
>> +       ret = watchdog_register_device(&wdt->wdd);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to register\n");
>> +               goto err;
>
>
> Unnecessary goto. Just return.

Ok.

>
>
>> +       }
>> +
>> +       dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
>> +                wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
>> +
>> +       platform_set_drvdata(pdev, wdt);
>> +err:
>> +       return ret;
>> +}
>> +
>> +static struct platform_driver aspeed_watchdog_driver = {
>> +       .probe = aspeed_wdt_probe,
>> +       .remove = aspeed_wdt_remove,
>> +       .driver = {
>> +               .name = KBUILD_MODNAME,
>> +               .of_match_table = of_match_ptr(aspeed_wdt_of_table),
>> +       },
>> +};
>> +module_platform_driver(aspeed_watchdog_driver);
>> +
>> +MODULE_DESCRIPTION("Aspeed Watchdog Driver");
>> +MODULE_LICENSE("GPL");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html