All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
@ 2018-08-30 14:22 Marek Behún
  2018-08-30 14:22 ` [PATCH 2/2] arm64: dts: marvell: armada-37xx: add nodes to support watchdog Marek Behún
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Marek Behún @ 2018-08-30 14:22 UTC (permalink / raw)
  To: linux-watchdog; +Cc: wim, Marek Behún

This adds support for the CPU watchdog found on Marvell Armada 37xx
SoCs.

There are 4 counters which can be set as CPU watchdog counters.
This driver uses the second counter (ID 1, counting from 0)
(Marvell's Linux also uses second counter by default).
In the future it could be adapted to use other counters, with
definition in the device tree.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/watchdog/armada-37xx-wdt.txt          |  23 ++
 drivers/watchdog/Kconfig                           |  11 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/armada_37xx_wdt.c                 | 353 +++++++++++++++++++++
 4 files changed, 388 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
 create mode 100644 drivers/watchdog/armada_37xx_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt b/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
new file mode 100644
index 000000000000..4ba9e40ad386
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
@@ -0,0 +1,23 @@
+* Armada 37xx CPU Watchdog Timer Controller
+
+Required properties:
+- compatible : must be "marvell,armada-3700-wdt"
+- reg : base physical address of the controller and length of memory mapped
+	region.
+- clocks : the clock feeding the watchdog timer. See clock-bindings.txt
+- marvell,system-controller : reference to syscon node for the CPU Miscellaneous
+	Registers
+
+Example:
+
+	cpu_misc: system-controller@d000 {
+		compatible = "syscon", "simple-mfd";
+		reg = <0xd000 0x1000>;
+	};
+
+	wdt: watchdog-timer@8300 {
+		compatible = "marvell,armada-3700-wdt";
+		reg = <0x8300 0x40>;
+		marvell,system-controller = <&cpu_misc>;
+		clocks = <&xtalclk>;
+	};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5ea8909a41f9..df710ac9a6ff 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -273,6 +273,17 @@ config ARM_SBSA_WATCHDOG
 	  To compile this driver as module, choose M here: The module
 	  will be called sbsa_gwdt.
 
+config ARMADA_37XX_WATCHDOG
+	tristate "Armada 37xx watchdog"
+	depends on ARCH_MVEBU || COMPILE_TEST
+	select MFD_SYSCON
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the watchdog timer found on
+	  Marvell Armada 37xx SoCs.
+	  To compile this driver as a module, choose M here: the
+	  module will be called armada_37xx_wdt.
+
 config ASM9260_WATCHDOG
 	tristate "Alphascale ASM9260 watchdog"
 	depends on MACH_ASM9260 || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index bf92e7bf9ce0..f69cdff5ad7f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
 # ARM Architecture
 obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
 obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
+obj-$(CONFIG_ARMADA_37XX_WATCHDOG) += armada_37xx_wdt.o
 obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
 obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
 obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
diff --git a/drivers/watchdog/armada_37xx_wdt.c b/drivers/watchdog/armada_37xx_wdt.c
new file mode 100644
index 000000000000..83ba4ce40d2f
--- /dev/null
+++ b/drivers/watchdog/armada_37xx_wdt.c
@@ -0,0 +1,353 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Watchdog driver for Marvell Armada 37xx SoCs
+ *
+ * Author: Marek Behun <marek.behun@nic.cz>
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+/*
+ * There are four counters that can be used for watchdog on Armada 37xx.
+ * The addresses for counter control registers are register base plus ID*0x10,
+ * where ID is 0, 1, 2 or 3.
+ * In this driver we use ID 1. Marvell's Linux also uses this ID by default,
+ * and the U-Boot driver written simultaneosly by the same author as this
+ * driver also uses ID 1.
+ * Maybe in the future we could change this driver to support other counters,
+ * depending on the device tree, but I don't think this is necessary.
+ *
+ * Note that CNTR_ID cannot be 3, because the third counter is an increment
+ * counter, and this driver is written to support decrementing counters only.
+ */
+
+/* relative to cpu_misc */
+#define WDT_TIMER_SELECT		0x64
+#define WDT_TIMER_SELECT_MASK		0xf
+#define WDT_TIMER_SELECT_VAL		BIT(CNTR_ID)
+
+/* relative to reg */
+#define CNTR_ID				1
+
+#define CNTR_CTRL			(CNTR_ID * 0x10)
+#define CNTR_CTRL_ENABLE		0x0001
+#define CNTR_CTRL_ACTIVE		0x0002
+#define CNTR_CTRL_MODE_MASK		0x000c
+#define CNTR_CTRL_MODE_ONESHOT		0x0000
+#define CNTR_CTRL_PRESCALE_MASK		0xff00
+#define CNTR_CTRL_PRESCALE_MIN		2
+#define CNTR_CTRL_PRESCALE_SHIFT	8
+
+#define CNTR_COUNT_LOW			(CNTR_CTRL + 0x4)
+#define CNTR_COUNT_HIGH			(CNTR_CTRL + 0x8)
+
+#define WATCHDOG_TIMEOUT		120
+
+static unsigned int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
+			  __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+			   __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct armada_37xx_watchdog {
+	struct watchdog_device wdt;
+	struct regmap *cpu_misc;
+	void __iomem *reg;
+	u64 timeout; /* in clock ticks */
+	unsigned long clk_rate;
+	struct clk *clk;
+};
+
+static u64 get_counter_value(struct armada_37xx_watchdog *dev)
+{
+	u64 val;
+
+	val = readl(dev->reg + CNTR_COUNT_HIGH);
+	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);
+
+	return val;
+}
+
+static void set_counter_value(struct armada_37xx_watchdog *dev)
+{
+	writel(dev->timeout & 0xffffffff, dev->reg + CNTR_COUNT_LOW);
+	writel(dev->timeout >> 32, dev->reg + CNTR_COUNT_HIGH);
+}
+
+static void armada_37xx_wdt_counter_enable(struct armada_37xx_watchdog *dev)
+{
+	u32 reg;
+
+	reg = readl(dev->reg + CNTR_CTRL);
+	reg |= CNTR_CTRL_ENABLE;
+	writel(reg, dev->reg + CNTR_CTRL);
+}
+
+static void armada_37xx_wdt_counter_disable(struct armada_37xx_watchdog *dev)
+{
+	u32 reg;
+
+	reg = readl(dev->reg + CNTR_CTRL);
+	reg &= ~CNTR_CTRL_ENABLE;
+	writel(reg, dev->reg + CNTR_CTRL);
+}
+
+static int armada_37xx_wdt_ping(struct watchdog_device *wdt)
+{
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+
+	armada_37xx_wdt_counter_disable(dev);
+	set_counter_value(dev);
+	armada_37xx_wdt_counter_enable(dev);
+
+	return 0;
+}
+
+static unsigned int armada_37xx_wdt_get_timeleft(struct watchdog_device *wdt)
+{
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+	unsigned int res;
+
+	res = get_counter_value(dev) * CNTR_CTRL_PRESCALE_MIN / dev->clk_rate;
+
+	return res;
+}
+
+static int armada_37xx_wdt_set_timeout(struct watchdog_device *wdt,
+				       unsigned int timeout)
+{
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+
+	wdt->timeout = timeout;
+
+	/*
+	 * Compute the timeout in clock rate. We use smallest possible
+	 * prescaler, which divides the clock rate by 2
+	 * (CNTR_CTRL_PRESCALE_MIN).
+	 */
+	dev->timeout = (u64)dev->clk_rate * timeout / CNTR_CTRL_PRESCALE_MIN;
+
+	return 0;
+}
+
+static bool armada_37xx_wdt_is_running(struct armada_37xx_watchdog *dev)
+{
+	u32 reg;
+
+	regmap_read(dev->cpu_misc, WDT_TIMER_SELECT, &reg);
+	if ((reg & WDT_TIMER_SELECT_MASK) != WDT_TIMER_SELECT_VAL)
+		return false;
+
+	reg = readl(dev->reg + CNTR_CTRL);
+	return !!(reg & CNTR_CTRL_ACTIVE);
+}
+
+static int armada_37xx_wdt_start(struct watchdog_device *wdt)
+{
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+	u32 reg;
+
+	reg = readl(dev->reg + CNTR_CTRL);
+
+	if (reg & CNTR_CTRL_ACTIVE)
+		return -EBUSY;
+
+	/* set mode */
+	reg = (reg & ~CNTR_CTRL_MODE_MASK) | CNTR_CTRL_MODE_ONESHOT;
+
+	/* set prescaler to the min value of 2 */
+	reg &= ~CNTR_CTRL_PRESCALE_MASK;
+	reg |= CNTR_CTRL_PRESCALE_MIN << CNTR_CTRL_PRESCALE_SHIFT;
+
+	writel(reg, dev->reg + CNTR_CTRL);
+
+	set_counter_value(dev);
+
+	regmap_write(dev->cpu_misc, WDT_TIMER_SELECT, WDT_TIMER_SELECT_VAL);
+	armada_37xx_wdt_counter_enable(dev);
+
+	return 0;
+}
+
+static int armada_37xx_wdt_stop(struct watchdog_device *wdt)
+{
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+
+	armada_37xx_wdt_counter_disable(dev);
+	regmap_write(dev->cpu_misc, WDT_TIMER_SELECT, 0);
+
+	return 0;
+}
+
+static const struct watchdog_info armada_37xx_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "Armada 37xx Watchdog",
+};
+
+static const struct watchdog_ops armada_37xx_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = armada_37xx_wdt_start,
+	.stop = armada_37xx_wdt_stop,
+	.ping = armada_37xx_wdt_ping,
+	.set_timeout = armada_37xx_wdt_set_timeout,
+	.get_timeleft = armada_37xx_wdt_get_timeleft,
+};
+
+static int armada_37xx_wdt_probe(struct platform_device *pdev)
+{
+	struct armada_37xx_watchdog *dev;
+	struct resource *res;
+	struct regmap *regmap;
+	int ret;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct armada_37xx_watchdog),
+			   GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->wdt.info = &armada_37xx_wdt_info;
+	dev->wdt.ops = &armada_37xx_wdt_ops;
+	dev->wdt.min_timeout = 1;
+
+	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						 "marvell,system-controller");
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+	dev->cpu_misc = regmap;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	dev->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+
+	/* init clock */
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	dev->clk_rate = clk_get_rate(dev->clk);
+
+	/*
+	 * Since the timeout in seconds is given as 32 bit unsigned int, and
+	 * the counters hold 64 bit values, even after multiplication by clock
+	 * rate the counter can hold timeout of UINT_MAX seconds.
+	 */
+	dev->wdt.min_timeout = 0;
+	dev->wdt.max_timeout = UINT_MAX;
+	dev->wdt.parent = &pdev->dev;
+
+	/* default value, possibly override by module parameter or dtb */
+	dev->wdt.timeout = WATCHDOG_TIMEOUT;
+	watchdog_init_timeout(&dev->wdt, timeout, &pdev->dev);
+
+	platform_set_drvdata(pdev, &dev->wdt);
+	watchdog_set_drvdata(&dev->wdt, dev);
+
+	armada_37xx_wdt_set_timeout(&dev->wdt, dev->wdt.timeout);
+
+	if (armada_37xx_wdt_is_running(dev))
+		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
+	else
+		armada_37xx_wdt_stop(&dev->wdt);
+
+	watchdog_set_nowayout(&dev->wdt, nowayout);
+	ret = watchdog_register_device(&dev->wdt);
+	if (ret)
+		goto disable_clk;
+
+	dev_info(&pdev->dev, "Initial timeout %d sec%s\n",
+		 dev->wdt.timeout, nowayout ? ", nowayout" : "");
+
+	return 0;
+
+disable_clk:
+	clk_disable_unprepare(dev->clk);
+	return ret;
+}
+
+static int armada_37xx_wdt_remove(struct platform_device *pdev)
+{
+	struct watchdog_device *wdt = platform_get_drvdata(pdev);
+	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
+
+	watchdog_unregister_device(wdt);
+	clk_disable_unprepare(dev->clk);
+	return 0;
+}
+
+static void armada_37xx_wdt_shutdown(struct platform_device *pdev)
+{
+	struct watchdog_device *wdt = platform_get_drvdata(pdev);
+
+	armada_37xx_wdt_stop(wdt);
+}
+
+static int __maybe_unused armada_37xx_wdt_suspend(struct device *dev)
+{
+	struct watchdog_device *wdt = dev_get_drvdata(dev);
+
+	return armada_37xx_wdt_stop(wdt);
+}
+
+static int __maybe_unused armada_37xx_wdt_resume(struct device *dev)
+{
+	struct watchdog_device *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(wdt))
+		return armada_37xx_wdt_start(wdt);
+
+	return 0;
+}
+
+static const struct dev_pm_ops armada_37xx_wdt_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(armada_37xx_wdt_suspend,
+				armada_37xx_wdt_resume)
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id armada_37xx_wdt_match[] = {
+	{ .compatible = "marvell,armada-3700-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, armada_37xx_wdt_match);
+#endif
+
+static struct platform_driver armada_37xx_wdt_driver = {
+	.probe		= armada_37xx_wdt_probe,
+	.remove		= armada_37xx_wdt_remove,
+	.shutdown	= armada_37xx_wdt_shutdown,
+	.driver		= {
+		.name	= "armada_37xx_wdt",
+		.of_match_table = of_match_ptr(armada_37xx_wdt_match),
+		.pm = &armada_37xx_wdt_dev_pm_ops,
+	},
+};
+
+module_platform_driver(armada_37xx_wdt_driver);
+
+MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>");
+MODULE_DESCRIPTION("Armada 37xx CPU Watchdog");
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:armada_37xx_wdt");
-- 
2.16.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] arm64: dts: marvell: armada-37xx: add nodes to support watchdog
  2018-08-30 14:22 [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behún
@ 2018-08-30 14:22 ` Marek Behún
  2018-08-30 14:28 ` [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behun
  2018-08-30 16:28 ` Guenter Roeck
  2 siblings, 0 replies; 13+ messages in thread
From: Marek Behún @ 2018-08-30 14:22 UTC (permalink / raw)
  To: linux-watchdog; +Cc: wim, Marek Behún

This adds the system controller node for CPU Miscellaneous Registers
(which is needed for the watchdog node) and the watchdog node.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index d9531e242eb4..048b072080c2 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -80,6 +80,18 @@
 			/* 32M internal register @ 0xd000_0000 */
 			ranges = <0x0 0x0 0xd0000000 0x2000000>;
 
+			wdt: watchdog-timer@8300 {
+				compatible = "marvell,armada-3700-wdt";
+				reg = <0x8300 0x40>;
+				marvell,system-controller = <&cpu_misc>;
+				clocks = <&xtalclk>;
+			};
+
+			cpu_misc: system-controller@d000 {
+				compatible = "syscon", "simple-mfd";
+				reg = <0xd000 0x1000>;
+			};
+
 			spi0: spi@10600 {
 				compatible = "marvell,armada-3700-spi";
 				#address-cells = <1>;
-- 
2.16.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 14:22 [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behún
  2018-08-30 14:22 ` [PATCH 2/2] arm64: dts: marvell: armada-37xx: add nodes to support watchdog Marek Behún
@ 2018-08-30 14:28 ` Marek Behun
  2018-08-30 14:40   ` Guenter Roeck
  2018-08-30 16:28 ` Guenter Roeck
  2 siblings, 1 reply; 13+ messages in thread
From: Marek Behun @ 2018-08-30 14:28 UTC (permalink / raw)
  To: linux-watchdog; +Cc: wim

Hi,

Checkpatch complains about not adding the new .c file to MAINTAINERS.
As I am sending other patches to linux-gpio, which creates a new group
in MAINTAINERS file, to which this file should belong, but if I created
this group also here, it would result in a conflict.
The addition of this file to MAINTAINERS will be done in the patch sent
to linux-gpio. Is this ok, or shall I do it differently?
Thanks.

Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 14:28 ` [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behun
@ 2018-08-30 14:40   ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2018-08-30 14:40 UTC (permalink / raw)
  To: Marek Behun; +Cc: linux-watchdog, wim

On Thu, Aug 30, 2018 at 04:28:28PM +0200, Marek Behun wrote:
> Hi,
> 
> Checkpatch complains about not adding the new .c file to MAINTAINERS.
> As I am sending other patches to linux-gpio, which creates a new group
> in MAINTAINERS file, to which this file should belong, but if I created
> this group also here, it would result in a conflict.
> The addition of this file to MAINTAINERS will be done in the patch sent
> to linux-gpio. Is this ok, or shall I do it differently?
> Thanks.
> 
This is one of the checkpatch complains to ignore.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 14:22 [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behún
  2018-08-30 14:22 ` [PATCH 2/2] arm64: dts: marvell: armada-37xx: add nodes to support watchdog Marek Behún
  2018-08-30 14:28 ` [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behun
@ 2018-08-30 16:28 ` Guenter Roeck
  2018-08-30 18:42   ` Marek Behun
  2 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2018-08-30 16:28 UTC (permalink / raw)
  To: Marek Behún; +Cc: linux-watchdog, wim

On Thu, Aug 30, 2018 at 04:22:42PM +0200, Marek Behún wrote:
> This adds support for the CPU watchdog found on Marvell Armada 37xx
> SoCs.
> 
> There are 4 counters which can be set as CPU watchdog counters.
> This driver uses the second counter (ID 1, counting from 0)
> (Marvell's Linux also uses second counter by default).
> In the future it could be adapted to use other counters, with
> definition in the device tree.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/watchdog/armada-37xx-wdt.txt          |  23 ++

Deviceptree properties need to be in a separate patch and have to be approved by
devicetree maintainers.

>  drivers/watchdog/Kconfig                           |  11 +
>  drivers/watchdog/Makefile                          |   1 +
>  drivers/watchdog/armada_37xx_wdt.c                 | 353 +++++++++++++++++++++
>  4 files changed, 388 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
>  create mode 100644 drivers/watchdog/armada_37xx_wdt.c
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt b/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
> new file mode 100644
> index 000000000000..4ba9e40ad386
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/armada-37xx-wdt.txt
> @@ -0,0 +1,23 @@
> +* Armada 37xx CPU Watchdog Timer Controller
> +
> +Required properties:
> +- compatible : must be "marvell,armada-3700-wdt"
> +- reg : base physical address of the controller and length of memory mapped
> +	region.
> +- clocks : the clock feeding the watchdog timer. See clock-bindings.txt
> +- marvell,system-controller : reference to syscon node for the CPU Miscellaneous
> +	Registers
> +
> +Example:
> +
> +	cpu_misc: system-controller@d000 {
> +		compatible = "syscon", "simple-mfd";
> +		reg = <0xd000 0x1000>;
> +	};
> +
> +	wdt: watchdog-timer@8300 {
> +		compatible = "marvell,armada-3700-wdt";
> +		reg = <0x8300 0x40>;
> +		marvell,system-controller = <&cpu_misc>;
> +		clocks = <&xtalclk>;
> +	};
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 5ea8909a41f9..df710ac9a6ff 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -273,6 +273,17 @@ config ARM_SBSA_WATCHDOG
>  	  To compile this driver as module, choose M here: The module
>  	  will be called sbsa_gwdt.
>  
> +config ARMADA_37XX_WATCHDOG
> +	tristate "Armada 37xx watchdog"
> +	depends on ARCH_MVEBU || COMPILE_TEST

Did you test this with a couple of architectures ?

> +	select MFD_SYSCON
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the watchdog timer found on
> +	  Marvell Armada 37xx SoCs.
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called armada_37xx_wdt.
> +
>  config ASM9260_WATCHDOG
>  	tristate "Alphascale ASM9260 watchdog"
>  	depends on MACH_ASM9260 || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index bf92e7bf9ce0..f69cdff5ad7f 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
>  # ARM Architecture
>  obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
>  obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
> +obj-$(CONFIG_ARMADA_37XX_WATCHDOG) += armada_37xx_wdt.o
>  obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
>  obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
>  obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
> diff --git a/drivers/watchdog/armada_37xx_wdt.c b/drivers/watchdog/armada_37xx_wdt.c
> new file mode 100644
> index 000000000000..83ba4ce40d2f
> --- /dev/null
> +++ b/drivers/watchdog/armada_37xx_wdt.c
> @@ -0,0 +1,353 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Watchdog driver for Marvell Armada 37xx SoCs
> + *
> + * Author: Marek Behun <marek.behun@nic.cz>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +
> +/*
> + * There are four counters that can be used for watchdog on Armada 37xx.
> + * The addresses for counter control registers are register base plus ID*0x10,
> + * where ID is 0, 1, 2 or 3.
> + * In this driver we use ID 1. Marvell's Linux also uses this ID by default,
> + * and the U-Boot driver written simultaneosly by the same author as this
> + * driver also uses ID 1.
> + * Maybe in the future we could change this driver to support other counters,
> + * depending on the device tree, but I don't think this is necessary.
> + *
> + * Note that CNTR_ID cannot be 3, because the third counter is an increment
> + * counter, and this driver is written to support decrementing counters only.
> + */
> +
> +/* relative to cpu_misc */
> +#define WDT_TIMER_SELECT		0x64
> +#define WDT_TIMER_SELECT_MASK		0xf
> +#define WDT_TIMER_SELECT_VAL		BIT(CNTR_ID)
> +
> +/* relative to reg */
> +#define CNTR_ID				1
> +
> +#define CNTR_CTRL			(CNTR_ID * 0x10)
> +#define CNTR_CTRL_ENABLE		0x0001
> +#define CNTR_CTRL_ACTIVE		0x0002
> +#define CNTR_CTRL_MODE_MASK		0x000c
> +#define CNTR_CTRL_MODE_ONESHOT		0x0000
> +#define CNTR_CTRL_PRESCALE_MASK		0xff00
> +#define CNTR_CTRL_PRESCALE_MIN		2
> +#define CNTR_CTRL_PRESCALE_SHIFT	8
> +
> +#define CNTR_COUNT_LOW			(CNTR_CTRL + 0x4)
> +#define CNTR_COUNT_HIGH			(CNTR_CTRL + 0x8)
> +
> +#define WATCHDOG_TIMEOUT		120
> +
> +static unsigned int timeout = WATCHDOG_TIMEOUT;

This defeats the purpose of using watchdog_init_timeout() to get a value
from devicetree. That value will never be used.

> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> +			  __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +			   __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct armada_37xx_watchdog {
> +	struct watchdog_device wdt;
> +	struct regmap *cpu_misc;
> +	void __iomem *reg;
> +	u64 timeout; /* in clock ticks */
> +	unsigned long clk_rate;
> +	struct clk *clk;
> +};
> +
> +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> +{
> +	u64 val;
> +
> +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);

Is this guaranteed to be consistent ? What happens if there is a 32-bit
wrap between those two operations ?

> +
> +	return val;
> +}
> +
> +static void set_counter_value(struct armada_37xx_watchdog *dev)
> +{
> +	writel(dev->timeout & 0xffffffff, dev->reg + CNTR_COUNT_LOW);
> +	writel(dev->timeout >> 32, dev->reg + CNTR_COUNT_HIGH);
> +}
> +
> +static void armada_37xx_wdt_counter_enable(struct armada_37xx_watchdog *dev)
> +{
> +	u32 reg;
> +
> +	reg = readl(dev->reg + CNTR_CTRL);
> +	reg |= CNTR_CTRL_ENABLE;
> +	writel(reg, dev->reg + CNTR_CTRL);
> +}
> +
> +static void armada_37xx_wdt_counter_disable(struct armada_37xx_watchdog *dev)
> +{
> +	u32 reg;
> +
> +	reg = readl(dev->reg + CNTR_CTRL);
> +	reg &= ~CNTR_CTRL_ENABLE;
> +	writel(reg, dev->reg + CNTR_CTRL);
> +}
> +
> +static int armada_37xx_wdt_ping(struct watchdog_device *wdt)
> +{
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +
> +	armada_37xx_wdt_counter_disable(dev);
> +	set_counter_value(dev);
> +	armada_37xx_wdt_counter_enable(dev);
> +

Is it really necessary to stop the watchdog for each ping ? That leaves
the system in a vulnerable state. Or does CNTR_CTRL_ENABLE only enable
writing into the counter register ?

> +	return 0;
> +}
> +
> +static unsigned int armada_37xx_wdt_get_timeleft(struct watchdog_device *wdt)
> +{
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +	unsigned int res;
> +
> +	res = get_counter_value(dev) * CNTR_CTRL_PRESCALE_MIN / dev->clk_rate;
> +
> +	return res;
> +}
> +
> +static int armada_37xx_wdt_set_timeout(struct watchdog_device *wdt,
> +				       unsigned int timeout)
> +{
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +
> +	wdt->timeout = timeout;
> +
> +	/*
> +	 * Compute the timeout in clock rate. We use smallest possible
> +	 * prescaler, which divides the clock rate by 2
> +	 * (CNTR_CTRL_PRESCALE_MIN).
> +	 */
> +	dev->timeout = (u64)dev->clk_rate * timeout / CNTR_CTRL_PRESCALE_MIN;
> +
> +	return 0;
> +}
> +
> +static bool armada_37xx_wdt_is_running(struct armada_37xx_watchdog *dev)
> +{
> +	u32 reg;
> +
> +	regmap_read(dev->cpu_misc, WDT_TIMER_SELECT, &reg);
> +	if ((reg & WDT_TIMER_SELECT_MASK) != WDT_TIMER_SELECT_VAL)
> +		return false;
> +
> +	reg = readl(dev->reg + CNTR_CTRL);
> +	return !!(reg & CNTR_CTRL_ACTIVE);
> +}
> +
> +static int armada_37xx_wdt_start(struct watchdog_device *wdt)
> +{
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +	u32 reg;
> +
> +	reg = readl(dev->reg + CNTR_CTRL);
> +
> +	if (reg & CNTR_CTRL_ACTIVE)
> +		return -EBUSY;

This is highly unusual. What problem are you solving here ?

> +
> +	/* set mode */
> +	reg = (reg & ~CNTR_CTRL_MODE_MASK) | CNTR_CTRL_MODE_ONESHOT;
> +
> +	/* set prescaler to the min value of 2 */
> +	reg &= ~CNTR_CTRL_PRESCALE_MASK;
> +	reg |= CNTR_CTRL_PRESCALE_MIN << CNTR_CTRL_PRESCALE_SHIFT;
> +
> +	writel(reg, dev->reg + CNTR_CTRL);
> +
> +	set_counter_value(dev);
> +
> +	regmap_write(dev->cpu_misc, WDT_TIMER_SELECT, WDT_TIMER_SELECT_VAL);
> +	armada_37xx_wdt_counter_enable(dev);
> +
> +	return 0;
> +}
> +
> +static int armada_37xx_wdt_stop(struct watchdog_device *wdt)
> +{
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +
> +	armada_37xx_wdt_counter_disable(dev);
> +	regmap_write(dev->cpu_misc, WDT_TIMER_SELECT, 0);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info armada_37xx_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "Armada 37xx Watchdog",
> +};
> +
> +static const struct watchdog_ops armada_37xx_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = armada_37xx_wdt_start,
> +	.stop = armada_37xx_wdt_stop,
> +	.ping = armada_37xx_wdt_ping,
> +	.set_timeout = armada_37xx_wdt_set_timeout,
> +	.get_timeleft = armada_37xx_wdt_get_timeleft,
> +};
> +
> +static int armada_37xx_wdt_probe(struct platform_device *pdev)
> +{
> +	struct armada_37xx_watchdog *dev;
> +	struct resource *res;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(struct armada_37xx_watchdog),
> +			   GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->wdt.info = &armada_37xx_wdt_info;
> +	dev->wdt.ops = &armada_37xx_wdt_ops;
> +	dev->wdt.min_timeout = 1;
> +
> +	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +						 "marvell,system-controller");
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +	dev->cpu_misc = regmap;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	dev->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +
> +	/* init clock */
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dev->clk))
> +		return PTR_ERR(dev->clk);
> +
> +	ret = clk_prepare_enable(dev->clk);
> +	if (ret)
> +		return ret;
> +
> +	dev->clk_rate = clk_get_rate(dev->clk);

Some clock drivers can return 0 here, which would result in a division by 0
later. It is prudent to check the return value.

> +
> +	/*
> +	 * Since the timeout in seconds is given as 32 bit unsigned int, and
> +	 * the counters hold 64 bit values, even after multiplication by clock
> +	 * rate the counter can hold timeout of UINT_MAX seconds.
> +	 */
> +	dev->wdt.min_timeout = 0;

That value is set to 1 above, which makes more sense. What is the point of
setting it to 0 here, and what is the impact of setting the actual timeout
to 0 ? Are you sure that doesn't result in an immediate reboot ?

> +	dev->wdt.max_timeout = UINT_MAX;
> +	dev->wdt.parent = &pdev->dev;
> +
> +	/* default value, possibly override by module parameter or dtb */
> +	dev->wdt.timeout = WATCHDOG_TIMEOUT;
> +	watchdog_init_timeout(&dev->wdt, timeout, &pdev->dev);
> +
> +	platform_set_drvdata(pdev, &dev->wdt);
> +	watchdog_set_drvdata(&dev->wdt, dev);
> +
> +	armada_37xx_wdt_set_timeout(&dev->wdt, dev->wdt.timeout);
> +
> +	if (armada_37xx_wdt_is_running(dev))
> +		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
> +	else
> +		armada_37xx_wdt_stop(&dev->wdt);

Why stop it if it isn't running ?

> +
> +	watchdog_set_nowayout(&dev->wdt, nowayout);
> +	ret = watchdog_register_device(&dev->wdt);
> +	if (ret)
> +		goto disable_clk;
> +
> +	dev_info(&pdev->dev, "Initial timeout %d sec%s\n",
> +		 dev->wdt.timeout, nowayout ? ", nowayout" : "");
> +
> +	return 0;
> +
> +disable_clk:
> +	clk_disable_unprepare(dev->clk);
> +	return ret;
> +}
> +
> +static int armada_37xx_wdt_remove(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdt = platform_get_drvdata(pdev);
> +	struct armada_37xx_watchdog *dev = watchdog_get_drvdata(wdt);
> +
> +	watchdog_unregister_device(wdt);
> +	clk_disable_unprepare(dev->clk);
> +	return 0;
> +}
> +
> +static void armada_37xx_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdt = platform_get_drvdata(pdev);
> +
> +	armada_37xx_wdt_stop(wdt);
> +}
> +
> +static int __maybe_unused armada_37xx_wdt_suspend(struct device *dev)
> +{
> +	struct watchdog_device *wdt = dev_get_drvdata(dev);
> +
> +	return armada_37xx_wdt_stop(wdt);
> +}
> +
> +static int __maybe_unused armada_37xx_wdt_resume(struct device *dev)
> +{
> +	struct watchdog_device *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdt))
> +		return armada_37xx_wdt_start(wdt);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops armada_37xx_wdt_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(armada_37xx_wdt_suspend,
> +				armada_37xx_wdt_resume)
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id armada_37xx_wdt_match[] = {
> +	{ .compatible = "marvell,armada-3700-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, armada_37xx_wdt_match);
> +#endif
> +
> +static struct platform_driver armada_37xx_wdt_driver = {
> +	.probe		= armada_37xx_wdt_probe,
> +	.remove		= armada_37xx_wdt_remove,
> +	.shutdown	= armada_37xx_wdt_shutdown,
> +	.driver		= {
> +		.name	= "armada_37xx_wdt",
> +		.of_match_table = of_match_ptr(armada_37xx_wdt_match),
> +		.pm = &armada_37xx_wdt_dev_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(armada_37xx_wdt_driver);
> +
> +MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>");
> +MODULE_DESCRIPTION("Armada 37xx CPU Watchdog");
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:armada_37xx_wdt");
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 16:28 ` Guenter Roeck
@ 2018-08-30 18:42   ` Marek Behun
  2018-08-30 18:50     ` Marek Behun
  2018-08-30 19:07     ` Guenter Roeck
  0 siblings, 2 replies; 13+ messages in thread
From: Marek Behun @ 2018-08-30 18:42 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim

On Thu, 30 Aug 2018 09:28:53 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> >  .../bindings/watchdog/armada-37xx-wdt.txt          |  23 ++  
> 
> Deviceptree properties need to be in a separate patch and have to be
> approved by devicetree maintainers.

OK :) I will divide the patch. Should I add devicetree maintainers to
Cc? Who is then responsible for applying the devicetree property, them
or you?

> > +config ARMADA_37XX_WATCHDOG
> > +	tristate "Armada 37xx watchdog"
> > +	depends on ARCH_MVEBU || COMPILE_TEST  
> 
> Did you test this with a couple of architectures ?

This watchdog si specific to Armada 3720, which currently means
EspressoBin and Turris Mox. I don't think another vendor would create a
SOC with same structure for watchdog as this. I also don't have access
to other boards. This works on EspressoBin and Turris Mox.

> > +static unsigned int timeout = WATCHDOG_TIMEOUT;  
> 
> This defeats the purpose of using watchdog_init_timeout() to get a
> value from devicetree. That value will never be used.

I shall rewrite this in the next version.

> > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > +{
> > +	u64 val;
> > +
> > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);  
> 
> Is this guaranteed to be consistent ? What happens if there is a
> 32-bit wrap between those two operations ?

hmmm. The address is not divisible by 8, so I can't use readq :( what
do you propose?

> > +static int armada_37xx_wdt_ping(struct watchdog_device *wdt)
> > +{
> > +	struct armada_37xx_watchdog *dev =
> > watchdog_get_drvdata(wdt); +
> > +	armada_37xx_wdt_counter_disable(dev);
> > +	set_counter_value(dev);
> > +	armada_37xx_wdt_counter_enable(dev);
> > +  
> 
> Is it really necessary to stop the watchdog for each ping ? That
> leaves the system in a vulnerable state. Or does CNTR_CTRL_ENABLE
> only enable writing into the counter register ?

Well, it probably is possible to do this without stopping the watchdog,
but two counters would need to be used for this. A trigger can be set
to counter 1 to start counting from initial value when counter 2 ended.
I will have to try if it works, though.

> > +static int armada_37xx_wdt_start(struct watchdog_device *wdt)
> > +{
> > +	struct armada_37xx_watchdog *dev =
> > watchdog_get_drvdata(wdt);
> > +	u32 reg;
> > +
> > +	reg = readl(dev->reg + CNTR_CTRL);
> > +
> > +	if (reg & CNTR_CTRL_ACTIVE)
> > +		return -EBUSY;  
> 
> This is highly unusual. What problem are you solving here ?

Hmm, you are right, this should not happen if the rest of this driver
works well so that code is redundant.

> > +	ret = clk_prepare_enable(dev->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev->clk_rate = clk_get_rate(dev->clk);  
> 
> Some clock drivers can return 0 here, which would result in a
> division by 0 later. It is prudent to check the return value.

Yes, I shall do that.

> > +
> > +	/*
> > +	 * Since the timeout in seconds is given as 32 bit
> > unsigned int, and
> > +	 * the counters hold 64 bit values, even after
> > multiplication by clock
> > +	 * rate the counter can hold timeout of UINT_MAX seconds.
> > +	 */
> > +	dev->wdt.min_timeout = 0;  
> 
> That value is set to 1 above, which makes more sense. What is the
> point of setting it to 0 here, and what is the impact of setting the
> actual timeout to 0 ? Are you sure that doesn't result in an
> immediate reboot ?

Yes, setting it to 0 does immediate reset. The = 1 line is redundant,
sorry.

> > +	armada_37xx_wdt_set_timeout(&dev->wdt, dev->wdt.timeout);
> > +
> > +	if (armada_37xx_wdt_is_running(dev))
> > +		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
> > +	else
> > +		armada_37xx_wdt_stop(&dev->wdt);  
> 
> Why stop it if it isn't running ?

There are 4 counters and every one of them can be set to be a watchdog
counter. This driver, as described in the commit message, works with
counter ID 1. The armada_37xx_wdt_stop unsets all 4 counters from
being watchdog counters, so that if something, for example u-boot, set
another counter as watchdog counter, the system would not reboot.

If you think this shouldn't be done I shall take it away, and rewrite
armada_37xx_wdt_stop to only touch counter ID 1.

Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 18:42   ` Marek Behun
@ 2018-08-30 18:50     ` Marek Behun
  2018-08-30 19:12       ` Guenter Roeck
  2018-08-30 19:07     ` Guenter Roeck
  1 sibling, 1 reply; 13+ messages in thread
From: Marek Behun @ 2018-08-30 18:50 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim

> > > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > > +{
> > > +	u64 val;
> > > +
> > > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);    
> > 
> > Is this guaranteed to be consistent ? What happens if there is a
> > 32-bit wrap between those two operations ?  
> 
> hmmm. The address is not divisible by 8, so I can't use readq :( what
> do you propose?

What do you think of this solution?

u64 val;
u32 low1, low2;

low1 = readl(dev->reg + CNTR_COUNT_LOW);
val = readl(dev->reg + CNTR_COUNT_HIGH);
low2 = readl(dev->reg + CNTR_COUNT_LOW);

/*
 * If low jumped in this short time more than 2^31, a wrap probably
 * occured. Read high again.
 */
if (low2 - low1 > 0x80000000)
	val = readl(dev->reg + CNTR_COUNT_HIGH);
val = (val << 32) | low2;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 18:42   ` Marek Behun
  2018-08-30 18:50     ` Marek Behun
@ 2018-08-30 19:07     ` Guenter Roeck
  1 sibling, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2018-08-30 19:07 UTC (permalink / raw)
  To: Marek Behun; +Cc: linux-watchdog, wim

On Thu, Aug 30, 2018 at 08:42:23PM +0200, Marek Behun wrote:
> On Thu, 30 Aug 2018 09:28:53 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > >  .../bindings/watchdog/armada-37xx-wdt.txt          |  23 ++  
> > 
> > Deviceptree properties need to be in a separate patch and have to be
> > approved by devicetree maintainers.
> 
> OK :) I will divide the patch. Should I add devicetree maintainers to
> Cc? Who is then responsible for applying the devicetree property, them
> or you?
> 
I (or rather Wim) will appy it, it just needs a Reviewed-by: from a DT
maintainer.

> > > +config ARMADA_37XX_WATCHDOG
> > > +	tristate "Armada 37xx watchdog"
> > > +	depends on ARCH_MVEBU || COMPILE_TEST  
> > 
> > Did you test this with a couple of architectures ?
> 
> This watchdog si specific to Armada 3720, which currently means
> EspressoBin and Turris Mox. I don't think another vendor would create a
> SOC with same structure for watchdog as this. I also don't have access
> to other boards. This works on EspressoBin and Turris Mox.
> 
I should have said "build test". Did you test-build the driver with other
architectures ?

> > > +static unsigned int timeout = WATCHDOG_TIMEOUT;  
> > 
> > This defeats the purpose of using watchdog_init_timeout() to get a
> > value from devicetree. That value will never be used.
> 
> I shall rewrite this in the next version.
> 
> > > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > > +{
> > > +	u64 val;
> > > +
> > > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);  
> > 
> > Is this guaranteed to be consistent ? What happens if there is a
> > 32-bit wrap between those two operations ?
> 
> hmmm. The address is not divisible by 8, so I can't use readq :( what
> do you propose?
> 

Re-read high after reading low ?

> > > +static int armada_37xx_wdt_ping(struct watchdog_device *wdt)
> > > +{
> > > +	struct armada_37xx_watchdog *dev =
> > > watchdog_get_drvdata(wdt); +
> > > +	armada_37xx_wdt_counter_disable(dev);
> > > +	set_counter_value(dev);
> > > +	armada_37xx_wdt_counter_enable(dev);
> > > +  
> > 
> > Is it really necessary to stop the watchdog for each ping ? That
> > leaves the system in a vulnerable state. Or does CNTR_CTRL_ENABLE
> > only enable writing into the counter register ?
> 
> Well, it probably is possible to do this without stopping the watchdog,
> but two counters would need to be used for this. A trigger can be set
> to counter 1 to start counting from initial value when counter 2 ended.
> I will have to try if it works, though.
> 
> > > +static int armada_37xx_wdt_start(struct watchdog_device *wdt)
> > > +{
> > > +	struct armada_37xx_watchdog *dev =
> > > watchdog_get_drvdata(wdt);
> > > +	u32 reg;
> > > +
> > > +	reg = readl(dev->reg + CNTR_CTRL);
> > > +
> > > +	if (reg & CNTR_CTRL_ACTIVE)
> > > +		return -EBUSY;  
> > 
> > This is highly unusual. What problem are you solving here ?
> 
> Hmm, you are right, this should not happen if the rest of this driver
> works well so that code is redundant.
> 
> > > +	ret = clk_prepare_enable(dev->clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dev->clk_rate = clk_get_rate(dev->clk);  
> > 
> > Some clock drivers can return 0 here, which would result in a
> > division by 0 later. It is prudent to check the return value.
> 
> Yes, I shall do that.
> 
> > > +
> > > +	/*
> > > +	 * Since the timeout in seconds is given as 32 bit
> > > unsigned int, and
> > > +	 * the counters hold 64 bit values, even after
> > > multiplication by clock
> > > +	 * rate the counter can hold timeout of UINT_MAX seconds.
> > > +	 */
> > > +	dev->wdt.min_timeout = 0;  
> > 
> > That value is set to 1 above, which makes more sense. What is the
> > point of setting it to 0 here, and what is the impact of setting the
> > actual timeout to 0 ? Are you sure that doesn't result in an
> > immediate reboot ?
> 
> Yes, setting it to 0 does immediate reset. The = 1 line is redundant,
> sorry.

This does not make sense. If you want to implement a reset function,
do it. Don't mis-use the watchdog API for it.

> 
> > > +	armada_37xx_wdt_set_timeout(&dev->wdt, dev->wdt.timeout);
> > > +
> > > +	if (armada_37xx_wdt_is_running(dev))
> > > +		set_bit(WDOG_HW_RUNNING, &dev->wdt.status);
> > > +	else
> > > +		armada_37xx_wdt_stop(&dev->wdt);  
> > 
> > Why stop it if it isn't running ?
> 
> There are 4 counters and every one of them can be set to be a watchdog
> counter. This driver, as described in the commit message, works with
> counter ID 1. The armada_37xx_wdt_stop unsets all 4 counters from
> being watchdog counters, so that if something, for example u-boot, set
> another counter as watchdog counter, the system would not reboot.
> 
> If you think this shouldn't be done I shall take it away, and rewrite
> armada_37xx_wdt_stop to only touch counter ID 1.
> 

If anything, the counter to be used should be specified in devicetree
instead of being fixed. You should not touch any other counters.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 18:50     ` Marek Behun
@ 2018-08-30 19:12       ` Guenter Roeck
  2018-09-01  0:29         ` Marek Behun
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2018-08-30 19:12 UTC (permalink / raw)
  To: Marek Behun; +Cc: linux-watchdog, wim

On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:
> > > > +static u64 get_counter_value(struct armada_37xx_watchdog *dev)
> > > > +{
> > > > +	u64 val;
> > > > +
> > > > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > > +	val = (val << 32) | readl(dev->reg + CNTR_COUNT_LOW);    
> > > 
> > > Is this guaranteed to be consistent ? What happens if there is a
> > > 32-bit wrap between those two operations ?  
> > 
> > hmmm. The address is not divisible by 8, so I can't use readq :( what
> > do you propose?
> 
> What do you think of this solution?
> 
> u64 val;
> u32 low1, low2;
> 
> low1 = readl(dev->reg + CNTR_COUNT_LOW);
> val = readl(dev->reg + CNTR_COUNT_HIGH);
> low2 = readl(dev->reg + CNTR_COUNT_LOW);
> 
> /*
>  * If low jumped in this short time more than 2^31, a wrap probably
>  * occured. Read high again.
>  */
> if (low2 - low1 > 0x80000000)
> 	val = readl(dev->reg + CNTR_COUNT_HIGH);
> val = (val << 32) | low2;

Yes, that is one option. The other would be to read high again
all the time and repeat reading low if high changed on the second
read of high.

	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
	low = readl(dev->reg + CNTR_COUNT_LOW);
	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
	if (high2 != high1)
		low = readl(dev->reg + CNTR_COUNT_LOW);
	val = (high2 << 32) | low;

There is no ambiguity in this case: We _know_ that
a wrap occurred if high1 and high2 are different.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-08-30 19:12       ` Guenter Roeck
@ 2018-09-01  0:29         ` Marek Behun
  2018-09-01  0:47           ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Behun @ 2018-09-01  0:29 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim

Hello Guenter,
I just read in the specification that software does not need to
do debouncing, because when low is read, high is latched into 32 bit
flip flop so that it can be read correctly.
So the correct way is to read low first, then high, and that's it.
I shall write a comment describing this into new code.
Marek

On Thu, 30 Aug 2018 12:12:26 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:
> > > > > +static u64 get_counter_value(struct armada_37xx_watchdog
> > > > > *dev) +{
> > > > > +	u64 val;
> > > > > +
> > > > > +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > > > > +	val = (val << 32) | readl(dev->reg +
> > > > > CNTR_COUNT_LOW);      
> > > > 
> > > > Is this guaranteed to be consistent ? What happens if there is a
> > > > 32-bit wrap between those two operations ?    
> > > 
> > > hmmm. The address is not divisible by 8, so I can't use
> > > readq :( what do you propose?  
> > 
> > What do you think of this solution?
> > 
> > u64 val;
> > u32 low1, low2;
> > 
> > low1 = readl(dev->reg + CNTR_COUNT_LOW);
> > val = readl(dev->reg + CNTR_COUNT_HIGH);
> > low2 = readl(dev->reg + CNTR_COUNT_LOW);
> > 
> > /*
> >  * If low jumped in this short time more than 2^31, a wrap probably
> >  * occured. Read high again.
> >  */
> > if (low2 - low1 > 0x80000000)
> > 	val = readl(dev->reg + CNTR_COUNT_HIGH);
> > val = (val << 32) | low2;  
> 
> Yes, that is one option. The other would be to read high again
> all the time and repeat reading low if high changed on the second
> read of high.
> 
> 	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
> 	low = readl(dev->reg + CNTR_COUNT_LOW);
> 	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
> 	if (high2 != high1)
> 		low = readl(dev->reg + CNTR_COUNT_LOW);
> 	val = (high2 << 32) | low;
> 
> There is no ambiguity in this case: We _know_ that
> a wrap occurred if high1 and high2 are different.
> 
> Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-09-01  0:29         ` Marek Behun
@ 2018-09-01  0:47           ` Guenter Roeck
  2018-09-02 20:12             ` Marek Behun
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2018-09-01  0:47 UTC (permalink / raw)
  To: Marek Behun; +Cc: linux-watchdog, wim

On 08/31/2018 05:29 PM, Marek Behun wrote:
> Hello Guenter,
> I just read in the specification that software does not need to
> do debouncing, because when low is read, high is latched into 32 bit
> flip flop so that it can be read correctly.
> So the correct way is to read low first, then high, and that's it.
> I shall write a comment describing this into new code.

Excellent. Thanks for checking!

Guenter

> Marek
> 
> On Thu, 30 Aug 2018 12:12:26 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:
>>>>>> +static u64 get_counter_value(struct armada_37xx_watchdog
>>>>>> *dev) +{
>>>>>> +	u64 val;
>>>>>> +
>>>>>> +	val = readl(dev->reg + CNTR_COUNT_HIGH);
>>>>>> +	val = (val << 32) | readl(dev->reg +
>>>>>> CNTR_COUNT_LOW);
>>>>>
>>>>> Is this guaranteed to be consistent ? What happens if there is a
>>>>> 32-bit wrap between those two operations ?
>>>>
>>>> hmmm. The address is not divisible by 8, so I can't use
>>>> readq :( what do you propose?
>>>
>>> What do you think of this solution?
>>>
>>> u64 val;
>>> u32 low1, low2;
>>>
>>> low1 = readl(dev->reg + CNTR_COUNT_LOW);
>>> val = readl(dev->reg + CNTR_COUNT_HIGH);
>>> low2 = readl(dev->reg + CNTR_COUNT_LOW);
>>>
>>> /*
>>>   * If low jumped in this short time more than 2^31, a wrap probably
>>>   * occured. Read high again.
>>>   */
>>> if (low2 - low1 > 0x80000000)
>>> 	val = readl(dev->reg + CNTR_COUNT_HIGH);
>>> val = (val << 32) | low2;
>>
>> Yes, that is one option. The other would be to read high again
>> all the time and repeat reading low if high changed on the second
>> read of high.
>>
>> 	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
>> 	low = readl(dev->reg + CNTR_COUNT_LOW);
>> 	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
>> 	if (high2 != high1)
>> 		low = readl(dev->reg + CNTR_COUNT_LOW);
>> 	val = (high2 << 32) | low;
>>
>> There is no ambiguity in this case: We _know_ that
>> a wrap occurred if high1 and high2 are different.
>>
>> Guenter
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-09-01  0:47           ` Guenter Roeck
@ 2018-09-02 20:12             ` Marek Behun
  2018-09-03  0:55               ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Behun @ 2018-09-02 20:12 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim

Hi Guenter,

One cannot restart the counter to start counting down from initial
value without disabling the counter, but the counter can be made to
restart counting from the initial value by an event which can be set,
and for Counter ID 1 the event can be set to "end count of Counter ID
0".

I have therefore reimplemented the driver so that two counters are
used. Counter ID 1 is used as the watchdog counter, while Counter ID 0
is used to reset Counter ID 1 to start counting from initial value.
Pinging is then done by forcing immediate end count event on Counter ID
0.

I discovered that counters 2 and 3 are enabled by default even before
U-Boot loads on the board, and therefore, in combination with the
previous, decided not to add th possibility to select watchdog counter
as a devicetree property.

What do you think of this?

Marek

On Fri, 31 Aug 2018 17:47:38 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 08/31/2018 05:29 PM, Marek Behun wrote:
> > Hello Guenter,
> > I just read in the specification that software does not need to
> > do debouncing, because when low is read, high is latched into 32 bit
> > flip flop so that it can be read correctly.
> > So the correct way is to read low first, then high, and that's it.
> > I shall write a comment describing this into new code.  
> 
> Excellent. Thanks for checking!
> 
> Guenter
> 
> > Marek
> > 
> > On Thu, 30 Aug 2018 12:12:26 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:  
> >>>>>> +static u64 get_counter_value(struct armada_37xx_watchdog
> >>>>>> *dev) +{
> >>>>>> +	u64 val;
> >>>>>> +
> >>>>>> +	val = readl(dev->reg + CNTR_COUNT_HIGH);
> >>>>>> +	val = (val << 32) | readl(dev->reg +
> >>>>>> CNTR_COUNT_LOW);  
> >>>>>
> >>>>> Is this guaranteed to be consistent ? What happens if there is a
> >>>>> 32-bit wrap between those two operations ?  
> >>>>
> >>>> hmmm. The address is not divisible by 8, so I can't use
> >>>> readq :( what do you propose?  
> >>>
> >>> What do you think of this solution?
> >>>
> >>> u64 val;
> >>> u32 low1, low2;
> >>>
> >>> low1 = readl(dev->reg + CNTR_COUNT_LOW);
> >>> val = readl(dev->reg + CNTR_COUNT_HIGH);
> >>> low2 = readl(dev->reg + CNTR_COUNT_LOW);
> >>>
> >>> /*
> >>>   * If low jumped in this short time more than 2^31, a wrap
> >>> probably
> >>>   * occured. Read high again.
> >>>   */
> >>> if (low2 - low1 > 0x80000000)
> >>> 	val = readl(dev->reg + CNTR_COUNT_HIGH);
> >>> val = (val << 32) | low2;  
> >>
> >> Yes, that is one option. The other would be to read high again
> >> all the time and repeat reading low if high changed on the second
> >> read of high.
> >>
> >> 	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
> >> 	low = readl(dev->reg + CNTR_COUNT_LOW);
> >> 	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
> >> 	if (high2 != high1)
> >> 		low = readl(dev->reg + CNTR_COUNT_LOW);
> >> 	val = (high2 << 32) | low;
> >>
> >> There is no ambiguity in this case: We _know_ that
> >> a wrap occurred if high1 and high2 are different.
> >>
> >> Guenter  
> > 
> >   
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog
  2018-09-02 20:12             ` Marek Behun
@ 2018-09-03  0:55               ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2018-09-03  0:55 UTC (permalink / raw)
  To: Marek Behun; +Cc: linux-watchdog, wim

On 09/02/2018 01:12 PM, Marek Behun wrote:
> Hi Guenter,
> 
> One cannot restart the counter to start counting down from initial
> value without disabling the counter, but the counter can be made to
> restart counting from the initial value by an event which can be set,
> and for Counter ID 1 the event can be set to "end count of Counter ID
> 0".
> 
> I have therefore reimplemented the driver so that two counters are
> used. Counter ID 1 is used as the watchdog counter, while Counter ID 0
> is used to reset Counter ID 1 to start counting from initial value.
> Pinging is then done by forcing immediate end count event on Counter ID
> 0.
> 
> I discovered that counters 2 and 3 are enabled by default even before
> U-Boot loads on the board, and therefore, in combination with the
> previous, decided not to add th possibility to select watchdog counter
> as a devicetree property.
> 
> What do you think of this?
> 

Ok with me. Please add above explanation to the driver.

Thanks,
Guenter

> Marek
> 
> On Fri, 31 Aug 2018 17:47:38 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 08/31/2018 05:29 PM, Marek Behun wrote:
>>> Hello Guenter,
>>> I just read in the specification that software does not need to
>>> do debouncing, because when low is read, high is latched into 32 bit
>>> flip flop so that it can be read correctly.
>>> So the correct way is to read low first, then high, and that's it.
>>> I shall write a comment describing this into new code.
>>
>> Excellent. Thanks for checking!
>>
>> Guenter
>>
>>> Marek
>>>
>>> On Thu, 30 Aug 2018 12:12:26 -0700
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>    
>>>> On Thu, Aug 30, 2018 at 08:50:04PM +0200, Marek Behun wrote:
>>>>>>>> +static u64 get_counter_value(struct armada_37xx_watchdog
>>>>>>>> *dev) +{
>>>>>>>> +	u64 val;
>>>>>>>> +
>>>>>>>> +	val = readl(dev->reg + CNTR_COUNT_HIGH);
>>>>>>>> +	val = (val << 32) | readl(dev->reg +
>>>>>>>> CNTR_COUNT_LOW);
>>>>>>>
>>>>>>> Is this guaranteed to be consistent ? What happens if there is a
>>>>>>> 32-bit wrap between those two operations ?
>>>>>>
>>>>>> hmmm. The address is not divisible by 8, so I can't use
>>>>>> readq :( what do you propose?
>>>>>
>>>>> What do you think of this solution?
>>>>>
>>>>> u64 val;
>>>>> u32 low1, low2;
>>>>>
>>>>> low1 = readl(dev->reg + CNTR_COUNT_LOW);
>>>>> val = readl(dev->reg + CNTR_COUNT_HIGH);
>>>>> low2 = readl(dev->reg + CNTR_COUNT_LOW);
>>>>>
>>>>> /*
>>>>>    * If low jumped in this short time more than 2^31, a wrap
>>>>> probably
>>>>>    * occured. Read high again.
>>>>>    */
>>>>> if (low2 - low1 > 0x80000000)
>>>>> 	val = readl(dev->reg + CNTR_COUNT_HIGH);
>>>>> val = (val << 32) | low2;
>>>>
>>>> Yes, that is one option. The other would be to read high again
>>>> all the time and repeat reading low if high changed on the second
>>>> read of high.
>>>>
>>>> 	high1 = readl(dev->reg + CNTR_COUNT_HIGH);
>>>> 	low = readl(dev->reg + CNTR_COUNT_LOW);
>>>> 	high2 = readl(dev->reg + CNTR_COUNT_HIGH);
>>>> 	if (high2 != high1)
>>>> 		low = readl(dev->reg + CNTR_COUNT_LOW);
>>>> 	val = (high2 << 32) | low;
>>>>
>>>> There is no ambiguity in this case: We _know_ that
>>>> a wrap occurred if high1 and high2 are different.
>>>>
>>>> Guenter
>>>
>>>    
>>
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-09-03  5:13 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-30 14:22 [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behún
2018-08-30 14:22 ` [PATCH 2/2] arm64: dts: marvell: armada-37xx: add nodes to support watchdog Marek Behún
2018-08-30 14:28 ` [PATCH 1/2] watchdog: Add support for Armada 37xx CPU watchdog Marek Behun
2018-08-30 14:40   ` Guenter Roeck
2018-08-30 16:28 ` Guenter Roeck
2018-08-30 18:42   ` Marek Behun
2018-08-30 18:50     ` Marek Behun
2018-08-30 19:12       ` Guenter Roeck
2018-09-01  0:29         ` Marek Behun
2018-09-01  0:47           ` Guenter Roeck
2018-09-02 20:12             ` Marek Behun
2018-09-03  0:55               ` Guenter Roeck
2018-08-30 19:07     ` Guenter Roeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.