[PATCH] [v3] watchdog: introduce the ARM64 SBSA watchdog driver

[PATCH] [v3] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

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 <timur@codeaurora.org>
---

[v3]
removed cpu_to_le32
used __maybe_unused instead of #ifdef

 drivers/watchdog/Kconfig        |   9 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/arm_sbsa_wdt.c | 296 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 306 insertions(+)
 create mode 100644 drivers/watchdog/arm_sbsa_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..b6e1cb4 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -514,6 +514,15 @@ config MEDIATEK_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called mtk_wdt.
 
+config ARM_SBSA_WDT
+	tristate "ARM Server Base System Architecture watchdog"
+	depends on ARM64 || COMPILE_TEST
+	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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c19294..063ab8c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
diff --git a/drivers/watchdog/arm_sbsa_wdt.c b/drivers/watchdog/arm_sbsa_wdt.c
new file mode 100644
index 0000000..0b35598
--- /dev/null
+++ b/drivers/watchdog/arm_sbsa_wdt.c
@@ -0,0 +1,296 @@
+/*
+ * 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 pr_fmt(fmt) "sbsa-gwdt: " fmt
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/device.h>
+#include <linux/io.h>
+
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/reboot.h>
+
+#include <asm/arch_timer.h>
+#include <linux/acpi.h>
+
+/* Watchdog Interface Identification Registers */
+struct arm_sbsa_watchdog_ident {
+	__le32 w_iidr;	/* Watchdog Interface Identification Register */
+	uint8_t res2[0xFE8 - 0xFD0];
+	__le32 w_pidr2;	/* Peripheral ID2 Register */
+};
+
+/* Watchdog Refresh Frame */
+struct arm_sbsa_watchdog_refresh {
+	__le32 wrr;		/* Watchdog Refresh Register */
+	uint8_t res1[0xFCC - 0x004];
+	struct arm_sbsa_watchdog_ident ident;
+};
+
+/* Watchdog Control Frame */
+struct arm_sbsa_watchdog_control {
+	__le32 wcs;
+	__le32 res1;
+	__le32 wor;
+	__le32 res2;
+	__le64 wcv;
+	uint8_t res3[0xFCC - 0x018];
+	struct arm_sbsa_watchdog_ident ident;
+};
+
+struct arm_sbsa_watchdog_data {
+	struct watchdog_device wdev;
+	unsigned int pm_status;
+	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_cntfrq() * 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_counter_get_cntvct();
+
+	return diff / arch_timer_get_cntfrq();
+}
+
+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.
+	 */
+	pr_crit("Initiating system reboot\n");
+	emergency_restart();
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Disable watchdog if it is active during suspend
+ */
+static int __maybe_unused arm_sbsa_wdt_suspend(struct device *dev)
+{
+	struct arm_sbsa_watchdog_data *data = dev_get_drvdata(dev);
+
+	data->pm_status = readl(&data->control->wcs) & 1;
+
+	if (data->pm_status)
+		writel(0, &data->control->wcs);
+
+	return 0;
+}
+
+/*
+ * Enable watchdog and configure it if necessary
+ */
+static int __maybe_unused arm_sbsa_wdt_resume(struct device *dev)
+{
+	struct arm_sbsa_watchdog_data *data = dev_get_drvdata(dev);
+
+	if (data->pm_status)
+		writel(1, &data->control->wcs);
+
+	return 0;
+}
+
+static const struct dev_pm_ops arm_sbsa_wdt_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(arm_sbsa_wdt_suspend, arm_sbsa_wdt_resume)
+};
+
+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 int __init arm_sbsa_wdt_probe(struct platform_device *pdev)
+{
+	struct arm_sbsa_watchdog_data *data;
+	struct resource *res;
+	uint32_t iidr;
+	int irq, ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
+	data->control = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->control))
+		return PTR_ERR(data->control);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh");
+	data->refresh = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->refresh))
+		return PTR_ERR(data->refresh);
+
+	/* We only support architecture version 0 */
+	iidr = readl(&data->control->ident.w_iidr);
+	if (((iidr >> 16) & 0xf) != 0) {
+		dev_info(&pdev->dev,
+			 "only architecture version 0 is supported\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_irq_byname(pdev, "ws0");
+	if (irq < 0) {
+		dev_err(&pdev->dev, "could not get interrupt\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, arm_sbsa_wdt_interrupt,
+			       0, pdev->name, NULL);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not request irq %i (ret=%i)\n",
+			irq, 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_cntfrq();
+
+	watchdog_set_drvdata(&data->wdev, data);
+
+	/*
+	 * Bits [15:12] are an implementation-defined revision number
+	 * for the component.
+	 */
+	arm_sbsa_wdt_info.firmware_version = (iidr >> 12) & 0xf;
+
+	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_dbg(&pdev->dev, "implementer code is %03x\n",
+		 (iidr & 0xf00) >> 1 | (iidr & 0x7f));
+	dev_info(&pdev->dev, "maximum timeout is %u seconds\n",
+		 data->wdev.max_timeout);
+
+	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;
+}
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id arm_sbsa_wdt_of_match[] = {
+	{ .compatible = "arm,sbsa-gwdt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, arm_sbsa_wdt_of_match);
+#endif
+
+static struct platform_driver arm_sbsa_wdt_driver = {
+	.driver = {
+		.name = "sbsa-gwdt",
+		.owner = THIS_MODULE,
+		.pm = &arm_sbsa_wdt_pm_ops,
+		.of_match_table = of_match_ptr(arm_sbsa_wdt_of_match),
+	},
+	.probe = arm_sbsa_wdt_probe,
+	.remove = __exit_p(arm_sbsa_wdt_remove),
+};
+
+module_platform_driver(arm_sbsa_wdt_driver);
+
+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.

Re: [PATCH] [v3] watchdog: introduce the ARM64 SBSA watchdog driver

[Guenter Roeck]

On 05/15/2015 06:35 PM, Timur Tabi wrote:
> 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 <timur@codeaurora.org>
> ---
>
> [v3]
> removed cpu_to_le32
> used __maybe_unused instead of #ifdef
>
>   drivers/watchdog/Kconfig        |   9 ++
>   drivers/watchdog/Makefile       |   1 +
>   drivers/watchdog/arm_sbsa_wdt.c | 296 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 306 insertions(+)
>   create mode 100644 drivers/watchdog/arm_sbsa_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..b6e1cb4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -514,6 +514,15 @@ config MEDIATEK_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called mtk_wdt.
>
> +config ARM_SBSA_WDT
> +	tristate "ARM Server Base System Architecture watchdog"
> +	depends on ARM64 || COMPILE_TEST

COMPILE_TEST doesn't work with the current code - see below.

> +	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
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c19294..063ab8c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -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
> diff --git a/drivers/watchdog/arm_sbsa_wdt.c b/drivers/watchdog/arm_sbsa_wdt.c
> new file mode 100644
> index 0000000..0b35598
> --- /dev/null
> +++ b/drivers/watchdog/arm_sbsa_wdt.c
> @@ -0,0 +1,296 @@
> +/*
> + * 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 pr_fmt(fmt) "sbsa-gwdt: " fmt
> +
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
> +#include <linux/reboot.h>
> +
> +#include <asm/arch_timer.h>

Only exists for arm and arm64, so COMPILE_TEST isn't really very useful
(also see readq problem below).

I still don't understand why you can not use the standard arch_sys_counter
provided for arm but have to call arm-internal functions directly.
That seems quite broken to me.

> +#include <linux/acpi.h>
> +
> +/* Watchdog Interface Identification Registers */
> +struct arm_sbsa_watchdog_ident {
> +	__le32 w_iidr;	/* Watchdog Interface Identification Register */
> +	uint8_t res2[0xFE8 - 0xFD0];
> +	__le32 w_pidr2;	/* Peripheral ID2 Register */

Does this register have any use ?

> +};
> +
> +/* Watchdog Refresh Frame */
> +struct arm_sbsa_watchdog_refresh {
> +	__le32 wrr;		/* Watchdog Refresh Register */
> +	uint8_t res1[0xFCC - 0x004];
> +	struct arm_sbsa_watchdog_ident ident;
> +};
> +
> +/* Watchdog Control Frame */
> +struct arm_sbsa_watchdog_control {
> +	__le32 wcs;
> +	__le32 res1;
> +	__le32 wor;
> +	__le32 res2;
> +	__le64 wcv;
> +	uint8_t res3[0xFCC - 0x018];
> +	struct arm_sbsa_watchdog_ident ident;
> +};
> +
> +struct arm_sbsa_watchdog_data {
> +	struct watchdog_device wdev;
> +	unsigned int pm_status;

You can use bool here.

> +	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);
> +

Is it ok to always overwrite the entire wcs register ?

> +	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_cntfrq() * 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_counter_get_cntvct();
> +
Due to this readq the code doesn't compile on arm,
nor on any other architecture which does not implement readq.

> +	return diff / arch_timer_get_cntfrq();

.. and as mentioned before this won't compile on 32 bit targets.

> +}
> +
> +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.
> +	 */
> +	pr_crit("Initiating system reboot\n");
> +	emergency_restart();
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Disable watchdog if it is active during suspend
> + */
> +static int __maybe_unused arm_sbsa_wdt_suspend(struct device *dev)
> +{
> +	struct arm_sbsa_watchdog_data *data = dev_get_drvdata(dev);
> +
> +	data->pm_status = readl(&data->control->wcs) & 1;
> +
> +	if (data->pm_status)
> +		writel(0, &data->control->wcs);
> +
> +	return 0;
> +}
> +
> +/*
> + * Enable watchdog and configure it if necessary
> + */
> +static int __maybe_unused arm_sbsa_wdt_resume(struct device *dev)
> +{
> +	struct arm_sbsa_watchdog_data *data = dev_get_drvdata(dev);
> +
> +	if (data->pm_status)
> +		writel(1, &data->control->wcs);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops arm_sbsa_wdt_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(arm_sbsa_wdt_suspend, arm_sbsa_wdt_resume)
> +};
> +
> +static struct watchdog_info arm_sbsa_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | _WDIOF_KEEPALIVEPING,

	WDIOF_MAGICCLOSE ?

> +	.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 int __init arm_sbsa_wdt_probe(struct platform_device *pdev)
> +{
> +	struct arm_sbsa_watchdog_data *data;
> +	struct resource *res;
> +	uint32_t iidr;
> +	int irq, ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
> +	data->control = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->control))
> +		return PTR_ERR(data->control);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh");
> +	data->refresh = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->refresh))
> +		return PTR_ERR(data->refresh);
> +
> +	/* We only support architecture version 0 */
> +	iidr = readl(&data->control->ident.w_iidr);
> +	if (((iidr >> 16) & 0xf) != 0) {
> +		dev_info(&pdev->dev,
> +			 "only architecture version 0 is supported\n");

dev_err ?
It might make sense to display the detected version as well.

> +		return -ENODEV;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "ws0");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "could not get interrupt\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, arm_sbsa_wdt_interrupt,
> +			       0, pdev->name, NULL);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not request irq %i (ret=%i)\n",
> +			irq, 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_cntfrq();
> +
> +	watchdog_set_drvdata(&data->wdev, data);
> +
You don't ever call watchdog_get_drvdata(), so this is unnecessary.

> +	/*
> +	 * Bits [15:12] are an implementation-defined revision number
> +	 * for the component.
> +	 */
> +	arm_sbsa_wdt_info.firmware_version = (iidr >> 12) & 0xf;
> +
> +	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_dbg(&pdev->dev, "implementer code is %03x\n",
> +		 (iidr & 0xf00) >> 1 | (iidr & 0x7f));
> +	dev_info(&pdev->dev, "maximum timeout is %u seconds\n",
> +		 data->wdev.max_timeout);
> +
> +	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;
> +}
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id arm_sbsa_wdt_of_match[] = {
> +	{ .compatible = "arm,sbsa-gwdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, arm_sbsa_wdt_of_match);
> +#endif
> +
> +static struct platform_driver arm_sbsa_wdt_driver = {
> +	.driver = {
> +		.name = "sbsa-gwdt",
> +		.owner = THIS_MODULE,
> +		.pm = &arm_sbsa_wdt_pm_ops,
> +		.of_match_table = of_match_ptr(arm_sbsa_wdt_of_match),
> +	},
> +	.probe = arm_sbsa_wdt_probe,
> +	.remove = __exit_p(arm_sbsa_wdt_remove),
> +};
> +
> +module_platform_driver(arm_sbsa_wdt_driver);
> +
> +MODULE_DESCRIPTION("ARM Server Base System Architecture Watchdog Driver");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH] [v3] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

Guenter Roeck wrote:

>> +config ARM_SBSA_WDT
>> +    tristate "ARM Server Base System Architecture watchdog"
>> +    depends on ARM64 || COMPILE_TEST
>
> COMPILE_TEST doesn't work with the current code - see below.

Ok, I'll remove it.  I never liked it anyway.

> I still don't understand why you can not use the standard arch_sys_counter
> provided for arm but have to call arm-internal functions directly.
> That seems quite broken to me.

I tried to use clk_get_sys(), but that failed because there are no 
clocks defined on my platform (the 'clocks' list in clkdev.c is empty). 
  I don't think the clock API is fully functional on an ARM ACPI system.

I was originally using functions like arch_timer_read_counter(), but 
that function is not exported, so I couldn't compile my driver as module 
if I used it.

>> +#include <linux/acpi.h>
>> +
>> +/* Watchdog Interface Identification Registers */
>> +struct arm_sbsa_watchdog_ident {
>> +    __le32 w_iidr;    /* Watchdog Interface Identification Register */
>> +    uint8_t res2[0xFE8 - 0xFD0];
>> +    __le32 w_pidr2;    /* Peripheral ID2 Register */
>
> Does this register have any use ?

I don't use it in this driver, but it is part of the hardware.

>> +struct arm_sbsa_watchdog_data {
>> +    struct watchdog_device wdev;
>> +    unsigned int pm_status;
>
> You can use bool here.

Ok.

>> +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);
>> +
>
> Is it ok to always overwrite the entire wcs register ?

Yes.  Writes to bits 1-31 have no affect.

>> +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_counter_get_cntvct();
>> +
> Due to this readq the code doesn't compile on arm,
> nor on any other architecture which does not implement readq.

>> +    return diff / arch_timer_get_cntfrq();
>
> .. and as mentioned before this won't compile on 32 bit targets.

That's why I have in my Kconfig:

	depends on ARM64

It will never be compiled on a 32-bit ARM platform.

>> +static struct watchdog_info arm_sbsa_wdt_info = {
>> +    .options = WDIOF_SETTIMEOUT | _WDIOF_KEEPALIVEPING,
>
>      WDIOF_MAGICCLOSE ?

Is it preferred to enable this feature?  It seems like a policy setting 
that shouldn't be defined by the driver.

>> +    /* We only support architecture version 0 */
>> +    iidr = readl(&data->control->ident.w_iidr);
>> +    if (((iidr >> 16) & 0xf) != 0) {
>> +        dev_info(&pdev->dev,
>> +             "only architecture version 0 is supported\n");
>
> dev_err ?
> It might make sense to display the detected version as well.

Ok.

>> +    watchdog_set_drvdata(&data->wdev, data);
>> +
> You don't ever call watchdog_get_drvdata(), so this is unnecessary.

Ok

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