[PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[PATCH] 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>
---
 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

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..a2a133f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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
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..2a2be40
--- /dev/null
+++ b/drivers/watchdog/arm_sbsa_wdt.c
@@ -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.

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

[Guenter Roeck]

On 04/29/2015 12:33 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>

Comments inline.

Guenter

> ---
>   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
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..a2a133f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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
> 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..2a2be40
> --- /dev/null
> +++ b/drivers/watchdog/arm_sbsa_wdt.c
> @@ -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;
> +};
> +

Why not just use defines instead of all those structures ?

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

Do you also have to reset the counter ?

> +
> +	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;
> +	}
> +
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.

> +	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.

> +	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.

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

Please align continuation lines with '('.

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

"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.

> +
> +	/*
> +	 * 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.

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

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

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

> +	    (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.

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

Really ? Or should there be a -1 somewhere ?
> +
> +	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.

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

0 is not an error (the interrupt number could be 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");
> +
This seems to assume that the watchdog is always supported, which is quite unlikely.

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

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 ?

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

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 ?

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

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

[Ashwin Chaugule]

+ Linaro-acpi

On 29 April 2015 at 15:33, Timur Tabi <timur@codeaurora.org> 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>
> ---
>  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
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..a2a133f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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
> 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..2a2be40
> --- /dev/null
> +++ b/drivers/watchdog/arm_sbsa_wdt.c
> @@ -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.
>

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

[Timur Tabi]

On 04/30/2015 10:30 PM, Guenter Roeck wrote:

>> +/* 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;
>> +};
>> +
>
> 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.

>> +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 ?

No.  Programming a new timeout automatically resets the timer.

>> +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;
>> +    }
>> +
> 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.

>> +    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.
>
>> +    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.

Will fix.

>
>> +        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);
>
> Please align continuation lines with '('.

Will fix.

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

Fair enough.  I thought it would be a handy way to know that the driver 
found the hardware, but I'll move it to a pr_debug().

>> +
>> +    /*
>> +     * 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.

Ok.

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

>> +    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.

>> +        (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.

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.

>
>> +        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);
>
> Really ? Or should there be a -1 somewhere ?

You're right.

>> +
>> +    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.

>> +    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) {
>
> 0 is not an error (the interrupt number could be 0).

Ok.

>> +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.

>> +    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;
>> +    }
>
> 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.

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

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

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

[Timur Tabi]

On 05/01/2015 11:16 AM, Timur Tabi wrote:
>>> +    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.
>
>>> +    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.

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?

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

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

[Guenter Roeck]

On Fri, May 01, 2015 at 12:28:48PM -0500, Timur Tabi wrote:
> On 05/01/2015 11:16 AM, Timur Tabi wrote:
> >>>+    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.
> >
> >>>+    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.
> 
> 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.

Guenter

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

[Timur Tabi]

On 05/01/2015 12:32 PM, Guenter Roeck wrote:
> 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.

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

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

[Guenter Roeck]

On Fri, May 01, 2015 at 11:16:41AM -0500, Timur Tabi wrote:
> On 04/30/2015 10:30 PM, Guenter Roeck wrote:
> 
> >>+/* 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;
> >>+};
> >>+
> >
> >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.
> 
Matter of personal opinion, I guess. Also prone to introducing errors,
and potentially resulting in more complex code. Just my personal opinion,
though, so I'll let that pass.

[ ... ]

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

> >>+    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.
> 
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 ?

> >>+        (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.
> 
> 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.

> >
> >>+        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);
> >
> >Really ? Or should there be a -1 somewhere ?
> 
> You're right.
> 
> >>+
> >>+    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.
> 
> >>+    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) {
> >
> >0 is not an error (the interrupt number could be 0).
> 
> Ok.
> 
> >>+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.
> 
> >>+    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;
> >>+    }
> >
> >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.

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).

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

Thanks,
Guenter

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

[Guenter Roeck]

On Fri, May 01, 2015 at 12:41:59PM -0500, Timur Tabi wrote:
> On 05/01/2015 12:32 PM, Guenter Roeck wrote:
> >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.

Is that really neceessary ?

And then people wonder why maintainers sometimes get grumpy :-(.

Guenter

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

[Timur Tabi]

On 05/01/2015 12:49 PM, Guenter Roeck wrote:

>>> 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!

>> 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.

>> 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.

> 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.

>> 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.

> 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'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 ?

A lot of this code is taken from the GIC driver, which is the closest 
match.  There really isn't much similar, because there's little code 
that supports the ACPI tables as-is.  A lot of the ACPI data is in the 
form of device nodes, which are device-tree like and are probed very 
differently.

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

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

[Timur Tabi]

On 05/01/2015 12:59 PM, Guenter Roeck wrote:
> 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.

No, of course not, it's just that I've written code like this many 
times, and no one else ever complained about my error messages before.

> 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.

Ok, ok!  I'll remove it.  I just trying to understand the rationale.  I 
don't agree with it, but I'll make the change you want.

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

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Arnd Bergmann]

On Friday 01 May 2015 11:16:41 Timur Tabi wrote:
> On 04/30/2015 10:30 PM, Guenter Roeck wrote:
> 
> >> +/* 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;
> >> +};
> >> +
> >
> > 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.

You should use endian-annotations in the structure if you do this,
and mark the members as __le32 or __be32, depending on how the
hardware is defined.

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

	Arnd

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Arnd Bergmann]

On Friday 01 May 2015 13:42:41 Timur Tabi wrote:
> 
> >> 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. 

Nothing prevents you from using the same register set on a platform
that is not SBSA compliant, or using DT on a machine that is SBSA
compliant, and we will definitely see that happen for chips that are
shared between products, e.g. a SBSA+SCPI system in a server targeted
market and the same hardware with devicetree for e.g. networking
equipment.

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.

	Arnd

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

[Guenter Roeck]

On Fri, May 01, 2015 at 01:42:41PM -0500, Timur Tabi wrote:
> On 05/01/2015 12:49 PM, Guenter Roeck wrote:
> 
> >>>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!
> 
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 ?

> >>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.
> 
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.

> >>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.
> 
> >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.
> 
The length field is either 8 bit long or 16 bit. Your code assumes both.

> >>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.
> 
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.

> 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.
> 
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.


> >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.
> 
So ACPI based watchdog support and ACPI_SIG_GTDT / ACPI_GTDT_TYPE_WATCHDOG
is mandatory for arm64 servers which support ACPI ?

Thanks,
Guenter

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

[Timur Tabi]

On 05/01/2015 02:26 PM, Guenter Roeck wrote:

> 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 ?

Sorry, I didn't completely read your email.  I thought you were asking 
why it couldn't be compiled as a module.

The local version of arm_sbsa_wdt_pdev should be deleted.  In fact, I 
could have sworn I fixed that bug already, but apparently not.

>> 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.

So I've modified the code to not display a message if the tables are 
merely absent.  It will still print an error if the table is invalid.

>> 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.

Ah, now I see it.  I will have to look into that and get back to you.

> 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.

Enabling ACPI does not disable DT, because the EFI stub creates a "mini 
DT" that is still used by the kernel.  I forgot what's in it, but I 
think the DT contains a pointer to the ACPI tables.  However, no actual 
device information is in that DT.

> 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.

Well, arm_sbsa_wdt_parse_gtdt() will only be called if the watchdog 
subtable in the GTDT exists.

> 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.

Yes, you're right.  I've modified the driver so that it exits quietly if 
there is no GTDT or watchdog subtable.
> So ACPI based watchdog support and ACPI_SIG_GTDT / ACPI_GTDT_TYPE_WATCHDOG
> is mandatory for arm64 servers which support ACPI ?

Probably not.

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

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

On 05/01/2015 02:24 PM, Arnd Bergmann wrote:
> 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.

> 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.

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

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

On 05/01/2015 02:19 PM, Arnd Bergmann wrote:
> 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.

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

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Guenter Roeck]

On 05/01/2015 01:15 PM, Timur Tabi wrote:
> On 05/01/2015 02:19 PM, Arnd Bergmann wrote:
>> 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.

Guenter

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

Guenter Roeck wrote:
> 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.

> [ 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.

> 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.

> 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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Guenter Roeck]

On 05/01/2015 12:56 PM, Timur Tabi wrote:
> On 05/01/2015 02:24 PM, Arnd Bergmann wrote:
>> 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.
>

I don't think anyone ever claimed that Linux development is free
of politics. Except for master politicians, of course.

>> 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.
>
I am perfectly fine with that. All I am asking for is flexibility on your part.

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.

Thanks,
Guenter

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Guenter Roeck]

On 05/01/2015 04:22 PM, Timur Tabi wrote:
> Guenter Roeck wrote:
>> 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.
>
>> [ 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.
>
>> 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.
>
Maybe ask on the arm  mailing list ?

Guenter

>> 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.
>

Re: [Linaro-acpi] [PATCH] watchdog: introduce the ARM64 SBSA watchdog driver

[Timur Tabi]

Guenter Roeck wrote:
> 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.

There won't be any need to rewrite the driver to add device tree 
support.  All we need to do is add the device tree probe function.

> 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.

Me too.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

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

[Hanjun Guo]

Hi Timur,

I didn't review it in detail since I'm preparing the
ACPI GICv3 related patches and run out of time, but
I noticed a obvious error, comments inline.

On 2015年04月30日 03:33, Timur Tabi wrote:

> +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 acpi_subtable_header can not be used here, because
acpi_subtable_header with the u8 structure length, but
for gtdt watchdog timer structure, its length is u16 defined.

Thanks
Hanjun