watchdog: introduce the ARM64 SBSA watchdog driver

The ARM Server Base System Architecture is a specification for ARM-based server systems. Among other things, it defines the behavior and register interface for a watchdog timer.

Signed-off-by Timur Tabi <[email protected]>

---
  drivers/watchdog/Kconfig        |  10 ++
  drivers/watchdog/Makefile       |   1 +
  drivers/watchdog/arm_sbsa_wdt.c | 368 ++++++++++++++++++++++++++++++++++++++++
  3 files changed, 379 insertions(+)

create mode 100644 drivers/watchdog/arm_sbsa_wdt.c

drivers/watchdog/Kconfig +10 -0

index e5e7c55..a2a133f 100644
@@ -514,6 +514,16 @@ config MEDIATEK_WATCHDOG
  	  To compile this driver as a module, choose M here: the
  	  module will be called mtk_wdt.
 
+config ARM_SBSA_WDT
+	bool "ARM Server Base System Architecture watchdog"
+	depends on ACPI
+	depends on ARM64
+	depends on ARM_ARCH_TIMER
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include watchdog timer support for ARM Server Base
+	  System Architecture (SBSA) systems.
+
  # AVR32 Architecture
 
  config AT32AP700X_WDT

drivers/watchdog/Makefile +1 -0

index 5c19294..063ab8c 100644
@@ -64,6 +64,7 @@ obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
  obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
  obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
  obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
+obj-$(CONFIG_ARM_SBSA_WDT) += arm_sbsa_wdt.o
 
  # AVR32 Architecture
  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o

drivers/watchdog/arm_sbsa_wdt.c new file +51 -0

new file mode 100644
index 0000000..2a2be40
@@ -0,0 +1,368 @@
+/*
+ * Watchdog driver for SBSA-compliant watchdog timers
+ *
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * ARM Server Base System Architecture watchdog driver.
+ *
+ * Register descriptions are taken from the ARM Server Base System
+ * Architecture document (ARM-DEN-0029)
+ */
+
+#define DRV_NAME "arm-sbsa-wdt"
+#define pr_fmt(fmt) DRV_NAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/io.h>
+
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/reboot.h>
+
+#include <clocksource/arm_arch_timer.h>
+#include <linux/acpi.h>
+
+/* Watchdog Interface Identification Registers */
+struct arm_sbsa_watchdog_ident {
+	uint32_t w_iidr;	/* Watchdog Interface Identification Register */
+	uint8_t res2[0xFE8 - 0xFD0];
+	uint32_t w_pidr2;	/* Peripheral ID2 Register */
+};
+
+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+	uint32_t wrr;		/* Watchdog Refresh Register */
+	uint8_t res1[0xFCC - 0x004];
+	struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */

drivers/watchdog/arm_sbsa_wdt.c new file +25 -0

+struct arm_sbsa_watchdog_data {
+	struct watchdog_device wdev;
+	struct arm_sbsa_watchdog_refresh __iomem *refresh;
+	struct arm_sbsa_watchdog_control __iomem *control;
+};
+
+static int arm_sbsa_wdt_start(struct watchdog_device *wdev)
+{
+	struct arm_sbsa_watchdog_data *data =
+		container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+	/* Writing to the control register will also reset the counter */
+	writel(1, &data->control->wcs);
+
+	return 0;
+}
+
+static int arm_sbsa_wdt_stop(struct watchdog_device *wdev)
+{
+	struct arm_sbsa_watchdog_data *data =
+		container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+	writel(0, &data->control->wcs);
+
+	return 0;

drivers/watchdog/arm_sbsa_wdt.c new file +66 -0

+
+	return 0;
+}
+
+static int arm_sbsa_wdt_ping(struct watchdog_device *wdev)
+{
+	struct arm_sbsa_watchdog_data *data =
+		container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+	writel(1, &data->refresh->wrr);
+
+	return 0;
+}
+
+static unsigned int arm_sbsa_wdt_status(struct watchdog_device *wdev)
+{
+	struct arm_sbsa_watchdog_data *data =
+		container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+	return (readl(&data->control->wcs) & 1) << WDOG_ACTIVE;
+}
+
+static unsigned int arm_sbsa_wdt_timeleft(struct watchdog_device *wdev)
+{
+	struct arm_sbsa_watchdog_data *data =
+		container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+	uint64_t diff = readq(&data->control->wcv) - arch_timer_read_counter();
+
+	return diff / arch_timer_get_rate();
+}
+
+static struct watchdog_info arm_sbsa_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "ARM SBSA watchdog",
+};
+
+static struct watchdog_ops arm_sbsa_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = arm_sbsa_wdt_start,
+	.stop = arm_sbsa_wdt_stop,
+	.ping = arm_sbsa_wdt_ping,
+	.set_timeout = arm_sbsa_wdt_set_timeout,
+	.status = arm_sbsa_wdt_status,
+	.get_timeleft = arm_sbsa_wdt_timeleft,
+};
+
+static irqreturn_t arm_sbsa_wdt_interrupt(int irq, void *p)
+{
+	/*
+	 * The WS0 interrupt occurs after the first timeout, so we attempt
+	 * a manual reboot.  If this doesn't work, the WS1 timeout will
+	 * cause a hardware reset.
+	 */
+	emergency_restart();
+
+	return IRQ_HANDLED;
+}
+
+static int __init arm_sbsa_wdt_probe(struct platform_device *pdev)
+{
+	struct arm_sbsa_watchdog_data *data;
+	struct resource *res;
+	uint32_t iidr;
+	int ret;
+

drivers/watchdog/arm_sbsa_wdt.c new file +4 -0

+		dev_err(&pdev->dev, "could not get interrupt\n");
+		return -ENOMEM;
+	}
+

drivers/watchdog/arm_sbsa_wdt.c new file +13 -0

+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not request irq %u (ret=%i)\n",
+			(unsigned int)res->start, ret);
+		return ret;
+	}
+
+	data->wdev.info = &arm_sbsa_wdt_info;
+	data->wdev.ops = &arm_sbsa_wdt_ops;
+	data->wdev.min_timeout = 1;
+	data->wdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	/* Calculate the maximum timeout in seconds that we can support */
+	data->wdev.max_timeout = U32_MAX / arch_timer_get_rate();

drivers/watchdog/arm_sbsa_wdt.c new file +11 -0

+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static int __exit arm_sbsa_wdt_remove(struct platform_device *pdev)
+{
+	struct arm_sbsa_watchdog_data *data = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&data->wdev);
+

drivers/watchdog/arm_sbsa_wdt.c new file +7 -0

+		pr_err("malformed subtable entry\n");
+		return -EINVAL;
+	}
+
+	if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
+		pr_err("invalid control or refresh address\n");
+		return -ENXIO;

drivers/watchdog/arm_sbsa_wdt.c new file +40 -0

+		pr_err("could not obtain interrupt\n");
+		goto error_platform;
+	}
+
+	res[2].name = "irq";
+	res[2].flags = IORESOURCE_IRQ;
+	res[2].start = ret;
+	res[2].end = res[2].start;
+
+	ret = platform_device_add_resources(arm_sbsa_wdt_pdev, res,
+		ARRAY_SIZE(res));
+	if (ret) {
+		pr_err("could not add platform resources\n");
+		goto error_acpi;
+	}
+
+	ret = platform_device_add(arm_sbsa_wdt_pdev);
+	if (ret) {
+		pr_err("could not add platform device\n");
+		goto error_acpi;
+	}
+
+	return 0;
+
+error_acpi:
+	acpi_unregister_gsi(res[2].start);
+
+error_platform:
+	platform_device_put(arm_sbsa_wdt_pdev);
+
+	return ret;
+}
+
+static struct platform_driver arm_sbsa_wdt_driver = {
+	.remove = __exit_p(arm_sbsa_wdt_remove),
+	.driver = {
+		   .name = DRV_NAME,
+		   .owner = THIS_MODULE,
+	},
+};

drivers/watchdog/arm_sbsa_wdt.c new file +13 -0

+}
+
+static void __exit arm_sbsa_wdt_exit(void)
+{
+	platform_device_unregister(arm_sbsa_wdt_pdev);
+	platform_driver_unregister(&arm_sbsa_wdt_driver);
+}
+
+module_init(arm_sbsa_wdt_init);
+module_exit(arm_sbsa_wdt_exit);
+
+MODULE_DESCRIPTION("ARM Server Base System Architecture Watchdog Driver");
+MODULE_LICENSE("GPL v2");

Quoted 12 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+    uint32_t wrr;        /* Watchdog Refresh Register */
+    uint8_t res1[0xFCC - 0x004];
+    struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */
+struct arm_sbsa_watchdog_control {
+    uint32_t wcs;
+    uint32_t res1;
+    uint32_t wor;

drivers/watchdog/arm_sbsa_wdt.c line 61

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    uint32_t res2;
+    uint64_t wcv;
+    uint8_t res3[0xFCC - 0x018];
+    struct arm_sbsa_watchdog_ident ident;
+};
+

Why not just use defines instead of all those structures ?

drivers/watchdog/arm_sbsa_wdt.c line 96

Quoted 8 lines

drivers/watchdog/arm_sbsa_wdt.c new file +8 -0

+static int arm_sbsa_wdt_set_timeout(struct watchdog_device *wdev,
+    unsigned int timeout)
+{
+    struct arm_sbsa_watchdog_data *data =
+        container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+    wdev->timeout = timeout;
+    writel(arch_timer_get_rate() * wdev->timeout, &data->control->wor);

Do you also have to reset the counter ?

Quoted 12 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+static int __init arm_sbsa_wdt_probe(struct platform_device *pdev)
+{
+    struct arm_sbsa_watchdog_data *data;
+    struct resource *res;
+    uint32_t iidr;
+    int ret;
+
+    data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

drivers/watchdog/arm_sbsa_wdt.c line 172

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get control address\n");
+        return -ENOMEM;
+    }
+

devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.

drivers/watchdog/arm_sbsa_wdt.c line 181

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+    data->control = devm_ioremap_resource(&pdev->dev, res);
+    if (!data->control)
+        return -ENOMEM;
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get refresh address\n");
+        return -ENOMEM;
+    }

Same here.

drivers/watchdog/arm_sbsa_wdt.c line 187

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    data->refresh = devm_ioremap_resource(&pdev->dev, res);
+    if (!data->refresh)
+        return -ENOMEM;
+
+    res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+    if (!res || !res->start) {

res->start checking is unnecessary. On a side note, irq == 0 might be a valid
interrupt number.

Quoted 11 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+        dev_err(&pdev->dev, "could not get interrupt\n");
+        return -ENOMEM;
+    }
+
+    iidr = readl(&data->control->ident.w_iidr);
+
+    /* We only support architecture version 0 */
+    if (((iidr >> 16) & 0xf) != 0) {
+        dev_info(&pdev->dev, "only architecture version 0 is

supported\n");

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +4 -0

+        return -ENODEV;
+    }
+
+    ret = devm_request_irq(&pdev->dev, res->start,

arm_sbsa_wdt_interrupt,
+ 0, DRV_NAME, NULL);

Please align continuation lines with '('.

Quoted 19 lines

drivers/watchdog/arm_sbsa_wdt.c new file +19 -0

+    if (ret < 0) {
+        dev_err(&pdev->dev, "could not request irq %u (ret=%i)\n",
+            (unsigned int)res->start, ret);
+        return ret;
+    }
+
+    data->wdev.info = &arm_sbsa_wdt_info;
+    data->wdev.ops = &arm_sbsa_wdt_ops;
+    data->wdev.min_timeout = 1;
+    data->wdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+    /* Calculate the maximum timeout in seconds that we can support */
+    data->wdev.max_timeout = U32_MAX / arch_timer_get_rate();
+
+    ret = watchdog_register_device(&data->wdev);
+    if (ret < 0) {
+        dev_err(&pdev->dev,
+            "could not register watchdog device (ret=%i)\n", ret);
+        return ret;

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+    }
+
+    dev_info(&pdev->dev, "implementer code is %03x, max timeout is %u

seconds\n",
+ (iidr & 0xf00) >> 1 | (iidr & 0x7f), data->wdev.max_timeout);

"Implementer code" sounds very much like a debug message to me. It means nothing to me, and it won't mean anything to a user.

drivers/watchdog/arm_sbsa_wdt.c line 231

Quoted 7 lines

drivers/watchdog/arm_sbsa_wdt.c new file +7 -0

+
+    /*
+     * Bits [15:12] are an implementation-defined revision number
+     * for the component.
+     */
+    arm_sbsa_wdt_info.firmware_version = (iidr >> 12) & 0xf;
+

It might make sense to set that prior to registering the driver.

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+static struct platform_device *arm_sbsa_wdt_pdev;
+
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header

*header,
+ const unsigned long end)

Quoted 4 lines

(quoted diff) +2 -0

+{
+    struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog

*)header;
+ struct platform_device *arm_sbsa_wdt_pdev;

That is an interesting one. Makes me wonder if you ever tried to unload this driver.
Did you ?

drivers/watchdog/arm_sbsa_wdt.c line 257

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+    struct resource res[3] = {};
+    int trigger, polarity;
+    int ret;
+
+    if (!header ||

That error check is kind of weird. Sure, 0 is an invalid address, but so are many other
addresses. Is there any reason to believe that acpi_get_table_with_size would return
a NULL pointer (but not some other invalid address) ?

drivers/watchdog/arm_sbsa_wdt.c line 259

Quoted 2 lines

drivers/watchdog/arm_sbsa_wdt.c new file +2 -0

+        (unsigned long)header + sizeof(*wdg) > end ||
+        header->length < sizeof(*wdg)) {

So I really wonder how this is supposed to work.

struct acpi_subtable_header {
         u8 type;
         u8 length;
};

struct acpi_gtdt_watchdog {
         struct acpi_gtdt_header header;

...

struct acpi_gtdt_header {
         u8 type;
         u16 length;
};

Either the length is 16 bit or 8 bit. But both ???
Or is it just luck and the value happens to be stored as little endian value and the length is less than 256 bytes ?

I understand this seems to be copied from BAD_MADT_ENTRY or similar code, but it is still odd.

Quoted 13 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+        pr_err("malformed subtable entry\n");
+        return -EINVAL;
+    }
+
+    if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
+        pr_err("invalid control or refresh address\n");
+        return -ENXIO;
+    }
+
+    arm_sbsa_wdt_pdev = platform_device_alloc(DRV_NAME, 0);
+    if (!arm_sbsa_wdt_pdev)
+        return -ENOMEM;

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+
+    res[0].name = "control";
+    res[0].flags = IORESOURCE_MEM;
+    res[0].start = wdg->control_frame_address;
+    res[0].end = res[0].start + sizeof(struct

arm_sbsa_watchdog_control);

Really ? Or should there be a -1 somewhere ?

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+
+    res[1].name = "refresh";
+    res[1].flags = IORESOURCE_MEM;
+    res[1].start = wdg->refresh_frame_address;
+    res[1].end = res[1].start + sizeof(struct

arm_sbsa_watchdog_refresh);

Same here.

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+    trigger = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_MODE) ?
+        ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+
+    polarity = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_POLARITY) ?
+        ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;

drivers/watchdog/arm_sbsa_wdt.c line 292

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+
+    /* This should be the WS0 interrupt */
+    ret = acpi_register_gsi(&arm_sbsa_wdt_pdev->dev,

wdg->timer_interrupt,

drivers/watchdog/arm_sbsa_wdt.c new file +2 -0

+        trigger, polarity);
+    if (ret <= 0) {

0 is not an error (the interrupt number could be 0).

drivers/watchdog/arm_sbsa_wdt.c line 342

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+static int __init arm_sbsa_wdt_init(void)
+{
+    struct acpi_table_header *table;
+    acpi_size tbl_size;
+    acpi_status status;
+    int count;
+
+    pr_info("ARM Server Base System Architecture watchdog driver\n");
+

This seems to assume that the watchdog is always supported, which is quite unlikely.

drivers/watchdog/arm_sbsa_wdt.c line 347

Quoted 6 lines

+ status = acpi_get_table_with_size(ACPI_SIG_GTDT, 0, &table,
&tbl_size);

drivers/watchdog/arm_sbsa_wdt.c new file +4 -0

+    if (ACPI_FAILURE(status)) {
+        pr_err("cannot locate GTDT table\n");
+        return -EINVAL;
+    }

I am kind of completely unhappy with the noisiness here and below. Is this how acpi detection is supposed to happen ?
And is it really an _error_ if the device isn't there, or does it just mean that the device isn't there ?

Quoted 10 lines

+ count = acpi_parse_entries(ACPI_SIG_GTDT, sizeof(struct
acpi_table_gtdt),

drivers/watchdog/arm_sbsa_wdt.c new file +7 -0

+        arm_sbsa_wdt_parse_gtdt, table, ACPI_GTDT_TYPE_WATCHDOG, 0);
+    if (count <= 0) {
+        pr_err("no watchdog subtables found\n");
+        return -EINVAL;
+    }
+
+    return platform_driver_probe(&arm_sbsa_wdt_driver,

arm_sbsa_wdt_probe);

Another oddity. Is this how acpi device instantiation is supposed to happen ?

I thought there are functions to register acpi drivers, such as acpi_bus_register_driver().
Why doesn't this work here ?

Quoted 6 lines

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get control address\n");
+        return -ENOMEM;
+    }
+

Quoted 3 lines

devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.

Will fix.

Quoted 10 lines

drivers/watchdog/arm_sbsa_wdt.c line 181

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+    data->control = devm_ioremap_resource(&pdev->dev, res);
+    if (!data->control)
+        return -ENOMEM;
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get refresh address\n");
+        return -ENOMEM;
+    }

Same here.

Quoted 21 lines · Timur Tabi

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c line 172

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get control address\n");
+        return -ENOMEM;
+    }
+

devm_ioremap_resource already prints an error message if res is NULL. And res->start can not be 0 unless there is a severe infrastructure problem.

Will fix.

Quoted 9 lines

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+    data->control = devm_ioremap_resource(&pdev->dev, res);
+    if (!data->control)
+        return -ENOMEM;
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+    if (!res || !res->start) {
+        dev_err(&pdev->dev, "could not get refresh address\n");
+        return -ENOMEM;
+    }

Quoted 1 line

Quoted 1 line

Same here.

So I must be missing something here. I'm only printing an error message if platform_get_resource() fails. I'm not printing a message if devm_ioremap_resource() fails. Are you saying that I should not print an error message if platform_get_resource() fails? What's wrong with that?

devm_ioremap_resource already prints a message. For this reason, elsewhere in the kernel the check for !res before calling devm_ioremap_resource is being removed, leaving the error handling to devm_ioremap_resource. I would suggest to do the same here.

Quoted 4 lines · Guenter Roeck

devm_ioremap_resource already prints a message. For this reason, elsewhere in the kernel the check for !res before calling devm_ioremap_resource is being removed, leaving the error handling to devm_ioremap_resource. I would suggest to do the same here.

I see what you're saying, but I leave the error handling to devm_ioremap_resource(), then no one will know whether it's because the control address or the refresh address is missing. The user will just see "invalid resource" and won't what resource is actually invalid.

You are saying that pretty much everyone in the kernel is, in your opinion, doing the Wrong Thing (tm) and you insist in doing it differently.

As maintainer, I have seen lots of patches which remove this very same error checking as unnecessary. If we accept your code, we can be all but sure to see such a patch at some point, probably right after your patch was accepted and shows up in linux-next. So besides arguing about something we should not have to argue about in the first place, you are trying to create even more maintainer work going forward.

Quoted 12 lines · Guenter Roeck

Quoted 12 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+    uint32_t wrr;        /* Watchdog Refresh Register */
+    uint8_t res1[0xFCC - 0x004];
+    struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */
+struct arm_sbsa_watchdog_control {
+    uint32_t wcs;
+    uint32_t res1;
+    uint32_t wor;

Quoted 8 lines

drivers/watchdog/arm_sbsa_wdt.c line 61

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    uint32_t res2;
+    uint64_t wcv;
+    uint8_t res3[0xFCC - 0x018];
+    struct arm_sbsa_watchdog_ident ident;
+};
+

Why not just use defines instead of all those structures ?

I like structures. I think hardware register blocks should be defined with structures that provide type checking.

Quoted 5 lines

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+static struct platform_device *arm_sbsa_wdt_pdev;
+
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header

*header,
+ const unsigned long end)

Quoted 8 lines

Quoted 4 lines

(quoted diff) +2 -0

+{
+    struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog

*)header;
+ struct platform_device *arm_sbsa_wdt_pdev;

That is an interesting one. Makes me wonder if you ever tried to unload this driver.
Did you ?

The driver can only be compiled in-kernel.

Quoted 11 lines

drivers/watchdog/arm_sbsa_wdt.c line 257

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+    struct resource res[3] = {};
+    int trigger, polarity;
+    int ret;
+
+    if (!header ||

That error check is kind of weird. Sure, 0 is an invalid address, but so are many other
addresses. Is there any reason to believe that acpi_get_table_with_size would return
a NULL pointer (but not some other invalid address) ?

If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.

Quoted 23 lines

drivers/watchdog/arm_sbsa_wdt.c line 259

Quoted 2 lines

drivers/watchdog/arm_sbsa_wdt.c new file +2 -0

+        (unsigned long)header + sizeof(*wdg) > end ||
+        header->length < sizeof(*wdg)) {

So I really wonder how this is supposed to work.

struct acpi_subtable_header {
        u8 type;
        u8 length;
};

struct acpi_gtdt_watchdog {
        struct acpi_gtdt_header header;

...

struct acpi_gtdt_header {
        u8 type;
        u16 length;
};

Either the length is 16 bit or 8 bit. But both ???
Or is it just luck and the value happens to be stored as little endian value and the length is less than 256 bytes ?

Quoted 2 lines

I understand this seems to be copied from BAD_MADT_ENTRY or similar code, but it is still odd.

It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.

Quoted 13 lines

Quoted 13 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+        pr_err("malformed subtable entry\n");
+        return -EINVAL;
+    }
+
+    if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
+        pr_err("invalid control or refresh address\n");
+        return -ENXIO;
+    }
+
+    arm_sbsa_wdt_pdev = platform_device_alloc(DRV_NAME, 0);
+    if (!arm_sbsa_wdt_pdev)
+        return -ENOMEM;

Quoted 8 lines

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+
+    res[0].name = "control";
+    res[0].flags = IORESOURCE_MEM;
+    res[0].start = wdg->control_frame_address;
+    res[0].end = res[0].start + sizeof(struct

arm_sbsa_watchdog_control);

Really ? Or should there be a -1 somewhere ?

You're right.

Quoted 8 lines

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+
+    res[1].name = "refresh";
+    res[1].flags = IORESOURCE_MEM;
+    res[1].start = wdg->refresh_frame_address;
+    res[1].end = res[1].start + sizeof(struct

arm_sbsa_watchdog_refresh);

Same here.

Ok.

Quoted 5 lines

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+    trigger = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_MODE) ?
+        ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+
+    polarity = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_POLARITY) ?
+        ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;

Quoted 8 lines

drivers/watchdog/arm_sbsa_wdt.c line 292

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+
+    /* This should be the WS0 interrupt */
+    ret = acpi_register_gsi(&arm_sbsa_wdt_pdev->dev,

wdg->timer_interrupt,

drivers/watchdog/arm_sbsa_wdt.c new file +2 -0

+        trigger, polarity);
+    if (ret <= 0) {

0 is not an error (the interrupt number could be 0).

Ok.

Quoted 11 lines

drivers/watchdog/arm_sbsa_wdt.c line 342

Quoted 9 lines

drivers/watchdog/arm_sbsa_wdt.c new file +9 -0

+static int __init arm_sbsa_wdt_init(void)
+{
+    struct acpi_table_header *table;
+    acpi_size tbl_size;
+    acpi_status status;
+    int count;
+
+    pr_info("ARM Server Base System Architecture watchdog driver\n");
+

This seems to assume that the watchdog is always supported, which is quite unlikely.

I'll make it a pr_debug.

Quoted 7 lines

Quoted 6 lines

+ status = acpi_get_table_with_size(ACPI_SIG_GTDT, 0, &table,
&tbl_size);

drivers/watchdog/arm_sbsa_wdt.c new file +4 -0

+    if (ACPI_FAILURE(status)) {
+        pr_err("cannot locate GTDT table\n");
+        return -EINVAL;
+    }

Quoted 4 lines

I am kind of completely unhappy with the noisiness here and below. Is this how acpi detection is supposed to happen ?
And is it really an _error_ if the device isn't there, or does it just mean that the device isn't there ?

I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.

However, I'm not an ACPI expert by any means. I'm hoping that someone who knows a lot more than I do will review it. This driver was reviewed and approved by some of our internal ACPI experts, so I presume that it is correct.

Quoted 11 lines

Quoted 10 lines

+ count = acpi_parse_entries(ACPI_SIG_GTDT, sizeof(struct
acpi_table_gtdt),

drivers/watchdog/arm_sbsa_wdt.c new file +7 -0

+        arm_sbsa_wdt_parse_gtdt, table, ACPI_GTDT_TYPE_WATCHDOG, 0);
+    if (count <= 0) {
+        pr_err("no watchdog subtables found\n");
+        return -EINVAL;
+    }
+
+    return platform_driver_probe(&arm_sbsa_wdt_driver,

arm_sbsa_wdt_probe);

Quoted 6 lines

Another oddity. Is this how acpi device instantiation is supposed to happen ?

I thought there are functions to register acpi drivers, such as acpi_bus_register_driver().
Why doesn't this work here ?

I can't use acpi_bus_register_driver() because there are no ACPI IDs to probe on. Watchdog information is stored as a subtable of the GTDT table, and there's nothing in the ACPI layer that automatically creates platforms from subtables. I have to create the platform from scratch, which is why the code looks so messy.

Quoted 6 lines

Quoted 3 lines

That is an interesting one. Makes me wonder if you ever tried to unload this driver.
Did you ?

The driver can only be compiled in-kernel.

And that makes it valid to have both a global and a local variable named arm_sbsa_wdt_pdev ? Please explain the rationale.

Quoted 4 lines

If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.

How would it be uninitialized ? A quick glance to other code seems to suggest that this isn't needed elsewhere. Why is it needed here ?

Quoted 3 lines

It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.

That doesn't explain if length is supposed to be 16 or 8 bit, if the length is supposed to be stored as big or little endian value, and what would happen if it was stored as big endian value on some system and is 16 bit wide.

If the length is in fact 16 bit, you could just check its value directly, instead of doing all the typecasting. And if it is 16 bit and has a fixed endianness (not host byte order), you should convert it to host byte order before validating it. Alternatively, something appears to be wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog.

Quoted 2 lines

I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.

ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?

Assuming that there can be an image which boots both with ACPI and devicetree based configurations, I can understand that this driver would only load on ACPI based arm64 systems, but that doesn't mean it should dump an error message if the system does not use ACPI (or if its ACPI tables do not include an entry for the watchdog, for that matter).

Quoted 6 lines

I can't use acpi_bus_register_driver() because there are no ACPI IDs to probe on. Watchdog information is stored as a subtable of the GTDT table, and there's nothing in the ACPI layer that automatically creates platforms from subtables. I have to create the platform from scratch, which is why the code looks so messy.

Weird. I'll really want to know from some ACPI experts if this is really how ACPI drivers are supposed to be instantiated. Can you give me some other examples where this is done ?

Quoted 6 lines

Quoted 3 lines

I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.

ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?

Yes, ACPI for ARM64 *servers* is mandatory. ARM64 servers are not supposed to use device tree.

And since this is a driver for SBSA systems (SBSA = Server Base System Architecture), this driver will only ever be used on an ARM64 server system with ACPI and no device tree at all.

No, that is not a reasonable assumption to make. All the ARM64 server systems we have today are using DT only and we will of course keep supporting systems like that.

This means we need a DT binding for the driver, and the ACPI portion that creates the platform device should be split out from the main driver.

Quoted 3 lines · Arnd Bergmann

No, that is not a reasonable assumption to make. All the ARM64 server systems we have today are using DT only and we will of course keep supporting systems like that.

This is what drives me crazy about Linux kernel development. I've been doing this for 15 years, and I still get random emails that contradict everything I've been told to date.

I was repeatedly told over the past year by multiple people that ARM64 server == ACPI, no ifs, ands, or buts.

Quoted 3 lines

This means we need a DT binding for the driver, and the ACPI portion that creates the platform device should be split out from the main driver.

Well, someone who actually has a DT-based ARM64 server system (which is not me) should come up with such a definition.

I'm hoping that my driver can be accepted without DT support, and that someone else who is interested in running my driver on an SBSA device tree platform can add what's missing.

Even if you (and/or others) may personally believe that ACPI _is_ the center of the universe, please be open to the possibility that others may have a different opinion and/or different requirements. We should not have to completely rewrite the driver to add devicetree support later on.

Note we still have to figure out how to instantiate the device based on the acpi data. I have no idea how that should be done, but it seems odd if it is really supposed to be done as you have implemented it. Maybe that is the case, but if so I would like to get a confirmation from an acpi maintainer.

Quoted 8 lines · Guenter Roeck

Quoted 6 lines

Quoted 3 lines

That is an interesting one. Makes me wonder if you ever tried to unload this driver.
Did you ?

The driver can only be compiled in-kernel.

And that makes it valid to have both a global and a local variable named arm_sbsa_wdt_pdev ? Please explain the rationale.

Because the driver has to create the ACPI platform device, it needs to call functions that are not exported:

ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!

Quoted 6 lines

Quoted 4 lines

If the table is uninitialized, then all the values are probably zero. I was trying to come up with something. These are the only tests I could come up with I know work.

How would it be uninitialized ? A quick glance to other code seems to suggest that this isn't needed elsewhere. Why is it needed here ?

I don't have a good answer to that, except that when I wrote this code, I wanted to add some error checking for the ACPI tables, and this is what I came up with.

If you're telling me that the code is wrong and it will generate false positives, then I can fix it. But if you're telling me that you don't understand why I'm doing some error checking on the ACPI tables that I'm parsing, well, I don't understand what could be wrong with that.

Quoted 6 lines

Quoted 3 lines

It's the best I could come up with. Sure it seems weird, but it works, and since it's copied from BAD_MADT_ENTRY, I figured it was acceptable.

That doesn't explain if length is supposed to be 16 or 8 bit, if the length is supposed to be stored as big or little endian value, and what would happen if it was stored as big endian value on some system and is 16 bit wide.

All lengths and endianness of the fields in the ACPI structures are already defined in the ACPI spec, so that stuff is fixed and already known. I'm not sure what you're getting at. I'm just doing some basic error checking, no different than any of the code that calls BAD_MADT_ENTRY does.

In fact, I suspect that the only reason that BAD_GTDT_ENTRY does not yet exist is because there isn't much support for ACPI timers in the kernel yet.

Quoted 5 lines

If the length is in fact 16 bit, you could just check its value directly, instead of doing all the typecasting. And if it is 16 bit and has a fixed endianness (not host byte order), you should convert it to host byte order before validating it. Alternatively, something appears to be wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog.

I don't understand what you mean when you say something is wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog. These structures look perfectly fine to me, and they work. My driver successfully loads and parses the ACPI tables on a real ARM64 server system.

Quoted 5 lines

Quoted 2 lines

I think the GTDT is required. Most likely, the kernel will fail to boot long before we get to this point if the GTDT is missing or corrupt.

ACPI, the center of the universe ;-). Is ACPI support on arm64 now mandatory ? I thought it also supports devicetree ?

Yes, ACPI for ARM64 *servers* is mandatory. ARM64 servers are not supposed to use device tree.

And since this is a driver for SBSA systems (SBSA = Server Base System Architecture), this driver will only ever be used on an ARM64 server system with ACPI and no device tree at all.

Quoted 5 lines

Assuming that there can be an image which boots both with ACPI and devicetree based configurations, I can understand that this driver would only load on ACPI based arm64 systems, but that doesn't mean it should dump an error message if the system does not use ACPI (or if its ACPI tables do not include an entry for the watchdog, for that matter).

I can have the driver silently exit if the GTDT table is missing. However, if it does exist, then all the other error messages are valid.

I can understand why that mandates that the driver has to be built into the kernel, at least if you and/or the arm64 maintainers don't want to export those functions. However, I still don't understand why you would need arm_sbsa_wdt_pdev declared twice.

What am I missing here ?

Quoted 5 lines

If you're telling me that the code is wrong and it will generate false positives, then I can fix it. But if you're telling me that you don't understand why I'm doing some error checking on the ACPI tables that I'm parsing, well, I don't understand what could be wrong with that.

I understand the checking. I don't understand why you think you need an error message instead of just returning ENODEV if the tables are not there.

Quoted 5 lines

I don't understand what you mean when you say something is wrong with acpi_gtdt_header and/or with acpi_gtdt_watchdog. These structures look perfectly fine to me, and they work. My driver successfully loads and parses the ACPI tables on a real ARM64 server system.

The length field is either 8 bit long or 16 bit. Your code assumes both.

Question here is if enabling ACPI support disables devcietree, and if enabling devicetree disables ACPI. If both can be enabled, and if an image can be built which supports both, this would result in stray and unexpected error messages if the image is loaded on a system supporting devicetree.

If both ACPI and OF can be enabled, and if you want the error messages, I would expect to see an additional dependency on !OF.

Question though is if the driver can (and will) be built into an image which can run on other systems. There is no SBSA dependency, after all, only an ACPI dependency. If there is no "silent" means for the driver to fail instantiation, and if the driver is always loaded if present in an image, I would expect that it must only be built into an image if that image is built specifically to _only_ run on SBSA systems.

So ACPI based watchdog support and ACPI_SIG_GTDT / ACPI_GTDT_TYPE_WATCHDOG is mandatory for arm64 servers which support ACPI ?

Quoted 12 lines · Guenter Roeck

Quoted 12 lines

drivers/watchdog/arm_sbsa_wdt.c new file +12 -0

+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+    uint32_t wrr;        /* Watchdog Refresh Register */
+    uint8_t res1[0xFCC - 0x004];
+    struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */
+struct arm_sbsa_watchdog_control {
+    uint32_t wcs;
+    uint32_t res1;
+    uint32_t wor;

Quoted 8 lines

drivers/watchdog/arm_sbsa_wdt.c line 61

Quoted 6 lines

drivers/watchdog/arm_sbsa_wdt.c new file +6 -0

+    uint32_t res2;
+    uint64_t wcv;
+    uint8_t res3[0xFCC - 0x018];
+    struct arm_sbsa_watchdog_ident ident;
+};
+

Why not just use defines instead of all those structures ?

I like structures. I think hardware register blocks should be defined with structures that provide type checking.

Quoted 5 lines

Quoted 5 lines

drivers/watchdog/arm_sbsa_wdt.c new file +3 -0

+static struct platform_device *arm_sbsa_wdt_pdev;
+
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header

*header,
+ const unsigned long end)

Quoted 8 lines

Quoted 4 lines

(quoted diff) +2 -0

+{
+    struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog

*)header;
+ struct platform_device *arm_sbsa_wdt_pdev;

That is an interesting one. Makes me wonder if you ever tried to unload this driver.
Did you ?

The driver can only be compiled in-kernel.

That should be fixed, we want normal drivers to be loadable modules.

Quoted 1 line · Arnd Bergmann

That should be fixed, we want normal drivers to be loadable modules.

The driver references three functions that are not exported:

ERROR: "acpi_parse_entries" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
ERROR: "arch_timer_get_rate" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!
ERROR: "arch_timer_read_counter" [drivers/watchdog/arm_sbsa_wdt.ko] undefined!

I know what you're going to say. You want me to move the ACPI platform code into some ACPI platform file, maybe /arch/arm64/kernel/acpi.c. My concern is that there is currently no code for ACPI timers, so I don't have much of a starting point. I'd hate to have to define support for all of ACPI timers just to get my driver merged.

Not really, at least not me. However, there is no other caller of arch_timer_get_rate, except for the call setting arch_timer_rate. This suggests that it might not be such a good idea to call arch_timer_get_rate() and arch_timer_read_counter() in the first place.

[ On a side note, arch_timer_get_rate() can return 0 if ARM_ARCH_TIMER
is not configured. You'll need to check for that. ]

It is hard to imagine that the watchdog would be the only driver which needs this clock. Isn't there some clock API call that can be used instead ?

As for acpi_parse_entries, we will need some feedback from the acpi maintainers. If the intent is that this function can be called from drivers, it should be exported.

Quoted 4 lines · Guenter Roeck

Not really, at least not me. However, there is no other caller of arch_timer_get_rate, except for the call setting arch_timer_rate. This suggests that it might not be such a good idea to call arch_timer_get_rate() and arch_timer_read_counter() in the first place.

I don't know of any other way to convert seconds into watchdog ticks.

Quoted 2 lines

[ On a side note, arch_timer_get_rate() can return 0 if ARM_ARCH_TIMER
is not configured. You'll need to check for that. ]

Check the Kconfig for this driver. It requires ARM_ARCH_TIMER.

Quoted 3 lines

It is hard to imagine that the watchdog would be the only driver which needs this clock. Isn't there some clock API call that can be used instead ?

If there is, I'd love to know it. There's no 'device' for this driver, so I don't think the clk API will work.

Quoted 3 lines

As for acpi_parse_entries, we will need some feedback from the acpi maintainers. If the intent is that this function can be called from drivers, it should be exported.

Agreed.

The ARM Server Base System Architecture is a specification for ARM-based server systems. Among other things, it defines the behavior and register interface for a watchdog timer.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/watchdog/Kconfig        |  10 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/arm_sbsa_wdt.c | 368 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 379 insertions(+)
 create mode 100644 drivers/watchdog/arm_sbsa_wdt.c

drivers/watchdog/Kconfig +10 -0

index e5e7c55..a2a133f 100644
@@ -514,6 +514,16 @@ config MEDIATEK_WATCHDOG
          To compile this driver as a module, choose M here: the
          module will be called mtk_wdt.
 
+config ARM_SBSA_WDT
+       bool "ARM Server Base System Architecture watchdog"
+       depends on ACPI
+       depends on ARM64
+       depends on ARM_ARCH_TIMER
+       select WATCHDOG_CORE
+       help
+         Say Y here to include watchdog timer support for ARM Server Base
+         System Architecture (SBSA) systems.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT

drivers/watchdog/Makefile +1 -0

index 5c19294..063ab8c 100644
@@ -64,6 +64,7 @@ obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
+obj-$(CONFIG_ARM_SBSA_WDT) += arm_sbsa_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o

drivers/watchdog/arm_sbsa_wdt.c new file +368 -1

new file mode 100644
index 0000000..2a2be40
@@ -0,0 +1,368 @@
+/*
+ * Watchdog driver for SBSA-compliant watchdog timers
+ *
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * ARM Server Base System Architecture watchdog driver.
+ *
+ * Register descriptions are taken from the ARM Server Base System
+ * Architecture document (ARM-DEN-0029)
+ */
+
+#define DRV_NAME "arm-sbsa-wdt"
+#define pr_fmt(fmt) DRV_NAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/io.h>
+
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/reboot.h>
+
+#include <clocksource/arm_arch_timer.h>
+#include <linux/acpi.h>
+
+/* Watchdog Interface Identification Registers */
+struct arm_sbsa_watchdog_ident {
+       uint32_t w_iidr;        /* Watchdog Interface Identification Register */
+       uint8_t res2[0xFE8 - 0xFD0];
+       uint32_t w_pidr2;       /* Peripheral ID2 Register */
+};
+
+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+       uint32_t wrr;           /* Watchdog Refresh Register */
+       uint8_t res1[0xFCC - 0x004];
+       struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */
+struct arm_sbsa_watchdog_control {
+       uint32_t wcs;
+       uint32_t res1;
+       uint32_t wor;
+       uint32_t res2;
+       uint64_t wcv;
+       uint8_t res3[0xFCC - 0x018];
+       struct arm_sbsa_watchdog_ident ident;
+};
+
+struct arm_sbsa_watchdog_data {
+       struct watchdog_device wdev;
+       struct arm_sbsa_watchdog_refresh __iomem *refresh;
+       struct arm_sbsa_watchdog_control __iomem *control;
+};
+
+static int arm_sbsa_wdt_start(struct watchdog_device *wdev)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       /* Writing to the control register will also reset the counter */
+       writel(1, &data->control->wcs);
+
+       return 0;
+}
+
+static int arm_sbsa_wdt_stop(struct watchdog_device *wdev)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       writel(0, &data->control->wcs);
+
+       return 0;
+}
+
+static int arm_sbsa_wdt_set_timeout(struct watchdog_device *wdev,
+       unsigned int timeout)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       wdev->timeout = timeout;
+       writel(arch_timer_get_rate() * wdev->timeout, &data->control->wor);
+
+       return 0;
+}
+
+static int arm_sbsa_wdt_ping(struct watchdog_device *wdev)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       writel(1, &data->refresh->wrr);
+
+       return 0;
+}
+
+static unsigned int arm_sbsa_wdt_status(struct watchdog_device *wdev)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       return (readl(&data->control->wcs) & 1) << WDOG_ACTIVE;
+}
+
+static unsigned int arm_sbsa_wdt_timeleft(struct watchdog_device *wdev)
+{
+       struct arm_sbsa_watchdog_data *data =
+               container_of(wdev, struct arm_sbsa_watchdog_data, wdev);
+
+       uint64_t diff = readq(&data->control->wcv) - arch_timer_read_counter();
+
+       return diff / arch_timer_get_rate();
+}
+
+static struct watchdog_info arm_sbsa_wdt_info = {
+       .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+       .identity = "ARM SBSA watchdog",
+};
+
+static struct watchdog_ops arm_sbsa_wdt_ops = {
+       .owner = THIS_MODULE,
+       .start = arm_sbsa_wdt_start,
+       .stop = arm_sbsa_wdt_stop,
+       .ping = arm_sbsa_wdt_ping,
+       .set_timeout = arm_sbsa_wdt_set_timeout,
+       .status = arm_sbsa_wdt_status,
+       .get_timeleft = arm_sbsa_wdt_timeleft,
+};
+
+static irqreturn_t arm_sbsa_wdt_interrupt(int irq, void *p)
+{
+       /*
+        * The WS0 interrupt occurs after the first timeout, so we attempt
+        * a manual reboot.  If this doesn't work, the WS1 timeout will
+        * cause a hardware reset.
+        */
+       emergency_restart();
+
+       return IRQ_HANDLED;
+}
+
+static int __init arm_sbsa_wdt_probe(struct platform_device *pdev)
+{
+       struct arm_sbsa_watchdog_data *data;
+       struct resource *res;
+       uint32_t iidr;
+       int ret;
+
+       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!res || !res->start) {
+               dev_err(&pdev->dev, "could not get control address\n");
+               return -ENOMEM;
+       }
+
+       data->control = devm_ioremap_resource(&pdev->dev, res);
+       if (!data->control)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+       if (!res || !res->start) {
+               dev_err(&pdev->dev, "could not get refresh address\n");
+               return -ENOMEM;
+       }
+       data->refresh = devm_ioremap_resource(&pdev->dev, res);
+       if (!data->refresh)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+       if (!res || !res->start) {
+               dev_err(&pdev->dev, "could not get interrupt\n");
+               return -ENOMEM;
+       }
+
+       iidr = readl(&data->control->ident.w_iidr);
+
+       /* We only support architecture version 0 */
+       if (((iidr >> 16) & 0xf) != 0) {
+               dev_info(&pdev->dev, "only architecture version 0 is supported\n");
+               return -ENODEV;
+       }
+
+       ret = devm_request_irq(&pdev->dev, res->start, arm_sbsa_wdt_interrupt,
+               0, DRV_NAME, NULL);
+       if (ret < 0) {
+               dev_err(&pdev->dev, "could not request irq %u (ret=%i)\n",
+                       (unsigned int)res->start, ret);
+               return ret;
+       }
+
+       data->wdev.info = &arm_sbsa_wdt_info;
+       data->wdev.ops = &arm_sbsa_wdt_ops;
+       data->wdev.min_timeout = 1;
+       data->wdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+       /* Calculate the maximum timeout in seconds that we can support */
+       data->wdev.max_timeout = U32_MAX / arch_timer_get_rate();
+
+       ret = watchdog_register_device(&data->wdev);
+       if (ret < 0) {
+               dev_err(&pdev->dev,
+                       "could not register watchdog device (ret=%i)\n", ret);
+               return ret;
+       }
+
+       dev_info(&pdev->dev, "implementer code is %03x, max timeout is %u seconds\n",
+               (iidr & 0xf00) >> 1 | (iidr & 0x7f), data->wdev.max_timeout);
+
+       /*
+        * Bits [15:12] are an implementation-defined revision number
+        * for the component.
+        */
+       arm_sbsa_wdt_info.firmware_version = (iidr >> 12) & 0xf;
+
+       platform_set_drvdata(pdev, data);
+
+       return 0;
+}
+
+static int __exit arm_sbsa_wdt_remove(struct platform_device *pdev)
+{
+       struct arm_sbsa_watchdog_data *data = platform_get_drvdata(pdev);
+
+       watchdog_unregister_device(&data->wdev);
+
+       return 0;
+}
+
+static struct platform_device *arm_sbsa_wdt_pdev;
+
+static int __init arm_sbsa_wdt_parse_gtdt(struct acpi_subtable_header *header,
+       const unsigned long end)
+{
+       struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog *)header;
+       struct platform_device *arm_sbsa_wdt_pdev;
+       struct resource res[3] = {};
+       int trigger, polarity;
+       int ret;
+
+       if (!header ||
+           (unsigned long)header + sizeof(*wdg) > end ||
+           header->length < sizeof(*wdg)) {
+               pr_err("malformed subtable entry\n");
+               return -EINVAL;
+       }
+
+       if (!wdg->control_frame_address || !wdg->refresh_frame_address) {
+               pr_err("invalid control or refresh address\n");
+               return -ENXIO;
+       }
+
+       arm_sbsa_wdt_pdev = platform_device_alloc(DRV_NAME, 0);
+       if (!arm_sbsa_wdt_pdev)
+               return -ENOMEM;
+
+       res[0].name = "control";
+       res[0].flags = IORESOURCE_MEM;
+       res[0].start = wdg->control_frame_address;
+       res[0].end = res[0].start + sizeof(struct arm_sbsa_watchdog_control);
+
+       res[1].name = "refresh";
+       res[1].flags = IORESOURCE_MEM;
+       res[1].start = wdg->refresh_frame_address;
+       res[1].end = res[1].start + sizeof(struct arm_sbsa_watchdog_refresh);
+
+       trigger = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_MODE) ?
+               ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+
+       polarity = (wdg->timer_flags & ACPI_GTDT_WATCHDOG_IRQ_POLARITY) ?
+               ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+
+       /* This should be the WS0 interrupt */
+       ret = acpi_register_gsi(&arm_sbsa_wdt_pdev->dev, wdg->timer_interrupt,
+               trigger, polarity);
+       if (ret <= 0) {
+               pr_err("could not obtain interrupt\n");
+               goto error_platform;
+       }
+
+       res[2].name = "irq";
+       res[2].flags = IORESOURCE_IRQ;
+       res[2].start = ret;
+       res[2].end = res[2].start;
+
+       ret = platform_device_add_resources(arm_sbsa_wdt_pdev, res,
+               ARRAY_SIZE(res));
+       if (ret) {
+               pr_err("could not add platform resources\n");
+               goto error_acpi;
+       }
+
+       ret = platform_device_add(arm_sbsa_wdt_pdev);
+       if (ret) {
+               pr_err("could not add platform device\n");
+               goto error_acpi;
+       }
+
+       return 0;
+
+error_acpi:
+       acpi_unregister_gsi(res[2].start);
+
+error_platform:
+       platform_device_put(arm_sbsa_wdt_pdev);
+
+       return ret;
+}
+
+static struct platform_driver arm_sbsa_wdt_driver = {
+       .remove = __exit_p(arm_sbsa_wdt_remove),
+       .driver = {
+                  .name = DRV_NAME,
+                  .owner = THIS_MODULE,
+       },
+};
+
+static int __init arm_sbsa_wdt_init(void)
+{
+       struct acpi_table_header *table;
+       acpi_size tbl_size;
+       acpi_status status;
+       int count;
+
+       pr_info("ARM Server Base System Architecture watchdog driver\n");
+
+       status = acpi_get_table_with_size(ACPI_SIG_GTDT, 0, &table, &tbl_size);
+       if (ACPI_FAILURE(status)) {
+               pr_err("cannot locate GTDT table\n");
+               return -EINVAL;
+       }
+       count = acpi_parse_entries(ACPI_SIG_GTDT, sizeof(struct acpi_table_gtdt),
+               arm_sbsa_wdt_parse_gtdt, table, ACPI_GTDT_TYPE_WATCHDOG, 0);
+       if (count <= 0) {
+               pr_err("no watchdog subtables found\n");
+               return -EINVAL;
+       }
+
+       return platform_driver_probe(&arm_sbsa_wdt_driver, arm_sbsa_wdt_probe);
+}
+
+static void __exit arm_sbsa_wdt_exit(void)
+{
+       platform_device_unregister(arm_sbsa_wdt_pdev);
+       platform_driver_unregister(&arm_sbsa_wdt_driver);
+}
+
+module_init(arm_sbsa_wdt_init);
+module_exit(arm_sbsa_wdt_exit);
+
+MODULE_DESCRIPTION("ARM Server Base System Architecture Watchdog Driver");
+MODULE_LICENSE("GPL v2");
--

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

drivers/watchdog/arm_sbsa_wdt.c new file +5 -0

+static int __exit arm_sbsa_wdt_remove(struct platform_device *pdev)
+{
+	struct arm_sbsa_watchdog_data *data = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&data->wdev);