[PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Sven Peter]

Apple SoCs come with a simple embedded watchdog. This watchdog is also
required in order to reset the SoC.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
new file mode 100644
index 000000000000..e58c56a6fdf6
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/apple,wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SoC Watchdog
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Sven Peter <sven@svenpeter.dev>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,t8103-wdt
+          - apple,t6000-wdt
+      - const: apple,wdt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/apple-aic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    wdt: watchdog@50000000 {
+        compatible = "apple,t8103-wdt", "apple,wdt";
+        reg = <0x50000000 0x4000>;
+        clocks = <&clk>;
+        interrupts = <AIC_IRQ 123 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 5b7a13f706fa..ba480837724d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1749,6 +1749,7 @@ F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
 F:	Documentation/devicetree/bindings/pci/apple,pcie.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+F:	Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/i2c/busses/i2c-pasemi-core.c
 F:	drivers/i2c/busses/i2c-pasemi-platform.c
-- 
2.25.1

[PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Sven Peter]

Add support for the watchdog timer found in Apple SoCs. This driver is
also required to reboot these machines.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 MAINTAINERS                  |   1 +
 drivers/watchdog/Kconfig     |  12 ++
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/apple_wdt.c | 223 +++++++++++++++++++++++++++++++++++
 4 files changed, 237 insertions(+)
 create mode 100644 drivers/watchdog/apple_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ba480837724d..30a1444297a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1756,6 +1756,7 @@ F:	drivers/i2c/busses/i2c-pasemi-platform.c
 F:	drivers/irqchip/irq-apple-aic.c
 F:	drivers/mailbox/apple-mailbox.c
 F:	drivers/pinctrl/pinctrl-apple-gpio.c
+F:	drivers/watchdog/apple_wdt.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 F:	include/dt-bindings/pinctrl/apple.h
 F:	include/linux/apple-mailbox.h
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9d222ba17ec6..170dec880c8f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -976,6 +976,18 @@ config MSC313E_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called msc313e_wdt.
 
+config APPLE_WATCHDOG
+	tristate "Apple SoC watchdog"
+	depends on ARCH_APPLE || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the Watchdog found in Apple
+	  SoCs such as the M1. Next to the common watchdog features this
+	  driver is also required in order to reboot these SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called apple_wdt.
+
 # X86 (i386 + ia64 + x86_64) Architecture
 
 config ACQUIRE_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee97064145b..270a518bd8f3 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
 obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
 obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
 obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
+obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
 
 # X86 (i386 + ia64 + x86_64) Architecture
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/apple_wdt.c b/drivers/watchdog/apple_wdt.c
new file mode 100644
index 000000000000..ff6d98aadffc
--- /dev/null
+++ b/drivers/watchdog/apple_wdt.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SoC Watchdog driver
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+/*
+ * Apple Watchdog MMIO registers
+ *
+ * This HW block has three separate watchdogs. WD0 resets the machine
+ * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
+ * machine reset. WD0 additionally supports a configurable interrupt.
+ *
+ * APPLE_WDT_WDx_CUR_TIME is a simple counter incremented for each tick of the
+ * reference clock. It can also be overwritten to any value.
+ * Whenever APPLE_WDT_CTRL_RESET_EN is set in APPLE_WDT_WDx_CTRL and
+ * APPLE_WDTx_WD1_CUR_TIME >= APPLE_WDTx_WD1_BITE_TIME the entire machine is
+ * reset.
+ * Whenever APPLE_WDT_CTRL_IRQ_EN is set and APPLE_WDTx_WD1_CUR_TIME >=
+ * APPLE_WDTx_WD1_BARK_TIME an interrupt is triggered and
+ * APPLE_WDT_CTRL_IRQ_STATUS is set. The interrupt can be cleared by writing
+ * 1 to APPLE_WDT_CTRL_IRQ_STATUS.
+ */
+#define APPLE_WDT_WD0_CUR_TIME		0x00
+#define APPLE_WDT_WD0_BITE_TIME		0x04
+#define APPLE_WDT_WD0_BARK_TIME		0x08
+#define APPLE_WDT_WD0_CTRL		0x0c
+
+#define APPLE_WDT_WD1_CUR_TIME		0x10
+#define APPLE_WDT_WD1_BITE_TIME		0x14
+#define APPLE_WDT_WD1_CTRL		0x1c
+
+#define APPLE_WDT_WD2_CUR_TIME		0x20
+#define APPLE_WDT_WD2_BITE_TIME		0x24
+#define APPLE_WDT_WD2_CTRL		0x2c
+
+#define APPLE_WDT_CTRL_IRQ_EN		BIT(0)
+#define APPLE_WDT_CTRL_IRQ_STATUS	BIT(1)
+#define APPLE_WDT_CTRL_RESET_EN		BIT(2)
+
+struct apple_wdt {
+	struct watchdog_device wdd;
+	void __iomem *regs;
+	struct clk *clk;
+	u32 clk_rate;
+};
+
+static struct apple_wdt *to_apple_wdt(struct watchdog_device *wdd)
+{
+	return container_of(wdd, struct apple_wdt, wdd);
+}
+
+static int apple_wdt_start(struct watchdog_device *wdd)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
+
+	return 0;
+}
+
+static int apple_wdt_stop(struct watchdog_device *wdd)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
+
+	return 0;
+}
+
+static int apple_wdt_ping(struct watchdog_device *wdd)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+
+	return 0;
+}
+
+static int apple_wdt_set_timeout(struct watchdog_device *wdd, unsigned int s)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+	u64 timeout;
+
+	timeout = mul_u32_u32(wdt->clk_rate, s);
+	if (timeout > U32_MAX)
+		return -EINVAL;
+
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+	writel_relaxed(timeout, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
+
+	wdd->timeout = s;
+
+	return 0;
+}
+
+static unsigned int apple_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+	u32 cur_time, reset_time;
+
+	cur_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+	reset_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_BITE_TIME);
+
+	return (reset_time - cur_time) / wdt->clk_rate;
+}
+
+static int apple_wdt_restart(struct watchdog_device *wdd, unsigned long mode,
+			     void *cmd)
+{
+	struct apple_wdt *wdt = to_apple_wdt(wdd);
+
+	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+
+	/* flush writes and wait for the reset to be triggered */
+	(void)readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
+	mdelay(150);
+
+	return 0;
+}
+
+static struct watchdog_ops apple_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = apple_wdt_start,
+	.stop = apple_wdt_stop,
+	.ping = apple_wdt_ping,
+	.set_timeout = apple_wdt_set_timeout,
+	.get_timeleft = apple_wdt_get_timeleft,
+	.restart = apple_wdt_restart,
+};
+
+static struct watchdog_info apple_wdt_info = {
+	.identity = "Apple SoC Watchdog",
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static int apple_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct apple_wdt *wdt;
+	int ret;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, wdt);
+
+	wdt->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(wdt->regs))
+		return PTR_ERR(wdt->regs);
+
+	/* reset watchdog to safe defaults */
+	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
+	writel_relaxed(U32_MAX, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
+
+	wdt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(wdt->clk))
+		return PTR_ERR(wdt->clk);
+
+	ret = clk_prepare_enable(wdt->clk);
+	if (ret)
+		return ret;
+
+	wdt->clk_rate = clk_get_rate(wdt->clk);
+	wdt->wdd.ops = &apple_wdt_ops;
+	wdt->wdd.info = &apple_wdt_info;
+	wdt->wdd.max_timeout = U32_MAX / wdt->clk_rate;
+	wdt->wdd.timeout = wdt->wdd.max_timeout;
+
+	watchdog_stop_on_unregister(&wdt->wdd);
+	watchdog_set_restart_priority(&wdt->wdd, 128);
+
+	ret = devm_watchdog_register_device(dev, &wdt->wdd);
+	if (ret < 0) {
+		clk_disable_unprepare(wdt->clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int apple_wdt_remove(struct platform_device *pdev)
+{
+	struct apple_wdt *wdt = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(wdt->clk);
+	return 0;
+}
+
+static const struct of_device_id apple_wdt_of_match[] = {
+	{ .compatible = "apple,wdt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apple_wdt_of_match);
+
+static struct platform_driver apple_wdt_driver = {
+	.driver = {
+		.name = "apple-watchdog",
+		.of_match_table = apple_wdt_of_match,
+	},
+	.probe = apple_wdt_probe,
+	.remove = apple_wdt_remove,
+};
+module_platform_driver(apple_wdt_driver);
+
+MODULE_DESCRIPTION("Apple SoC watchdog driver");
+MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
+MODULE_LICENSE("Dual MIT/GPL");
-- 
2.25.1

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Janne Grunau]

On 2021-11-13 10:47:32 +0100, Sven Peter wrote:
> Add support for the watchdog timer found in Apple SoCs. This driver is
> also required to reboot these machines.

Tested on a M1 Max with
compatibe = "apple,t6000-wdt", "apple,wdt";
MacOS 12 on the same machine does not use the watchdog for reset. MacOS 
log output suggests that it uses a "pgmr" call for reset.

Feel free to add
Tested-by: Janne Grunau <j@jannau.net>

Janne

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Sven Peter]

Hi Janne,

On Sat, Nov 13, 2021, at 12:14, Janne Grunau wrote:
> On 2021-11-13 10:47:32 +0100, Sven Peter wrote:
>> Add support for the watchdog timer found in Apple SoCs. This driver is
>> also required to reboot these machines.
>
> Tested on a M1 Max with
> compatibe = "apple,t6000-wdt", "apple,wdt";
> MacOS 12 on the same machine does not use the watchdog for reset. MacOS 
> log output suggests that it uses a "pgmr" call for reset.
>
> Feel free to add
> Tested-by: Janne Grunau <j@jannau.net>

Awesome, thanks for testing this!
I suspect that they added an additional reset path for the Pro/Max variants.
On my M1 macOS seems to only use this watchdog to reset the machine.

And if I understand the linux reset infrastructure correctly it's still fine
to have this as a "fallback" reset notifier even on the Pro/Max and add support
for the proper "pmgr" reset notifier eventually with a higher priority. 


Best,

Sven

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Alyssa Rosenzweig]

> + * This HW block has three separate watchdogs. WD0 resets the machine
> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
> + * machine reset. WD0 additionally supports a configurable interrupt.

Do we have any idea what the difference between WD1 and WD2 is?

Re: [PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Mark Kettenis]

> From: Sven Peter <sven@svenpeter.dev>
> Date: Sat, 13 Nov 2021 10:47:31 +0100
> 
> Apple SoCs come with a simple embedded watchdog. This watchdog is also
> required in order to reset the SoC.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> new file mode 100644
> index 000000000000..e58c56a6fdf6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/apple,wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC Watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"

I believew this should be:

  - $ref: /schemas/watchdog/watchdog.yaml#

with that fixed:

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>


> +
> +maintainers:
> +  - Sven Peter <sven@svenpeter.dev>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-wdt
> +          - apple,t6000-wdt
> +      - const: apple,wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/apple-aic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    wdt: watchdog@50000000 {
> +        compatible = "apple,t8103-wdt", "apple,wdt";
> +        reg = <0x50000000 0x4000>;
> +        clocks = <&clk>;
> +        interrupts = <AIC_IRQ 123 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5b7a13f706fa..ba480837724d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1749,6 +1749,7 @@ F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:	Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
>  F:	Documentation/devicetree/bindings/pci/apple,pcie.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +F:	Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/i2c/busses/i2c-pasemi-core.c
>  F:	drivers/i2c/busses/i2c-pasemi-platform.c
> -- 
> 2.25.1
> 
> 

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Sven Peter]

On Sat, Nov 13, 2021, at 16:24, Alyssa Rosenzweig wrote:
>> + * This HW block has three separate watchdogs. WD0 resets the machine
>> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
>> + * machine reset. WD0 additionally supports a configurable interrupt.
>
> Do we have any idea what the difference between WD1 and WD2 is?

I've never seen macOS write to WD2 when running in our hypervisor and only
found that one when I was looking at the rest of the MMIO region.
From what I can tell it works exactly like WD1.

Sven

Re: [PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Sven Peter]

On Sat, Nov 13, 2021, at 22:07, Mark Kettenis wrote:
>> From: Sven Peter <sven@svenpeter.dev>
>> Date: Sat, 13 Nov 2021 10:47:31 +0100
>> 
>> Apple SoCs come with a simple embedded watchdog. This watchdog is also
>> required in order to reset the SoC.
>> 
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>> ---
>>  .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
>> new file mode 100644
>> index 000000000000..e58c56a6fdf6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/watchdog/apple,wdt.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Apple SoC Watchdog
>> +
>> +allOf:
>> +  - $ref: "watchdog.yaml#"
>
> I believew this should be:
>
>   - $ref: /schemas/watchdog/watchdog.yaml#
>
> with that fixed:
>
> Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
>

Thanks for the review!
Almost all the other YAML watchdog bindings just use '$ref: "watchdog.yaml#"'.
Only arm,sp805.yaml uses '$ref: /schemas/watchdog/watchdog.yaml#'.


Sven
 

Re: [PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Mark Kettenis]

> Date: Sun, 14 Nov 2021 12:40:48 +0100
> From: "Sven Peter" <sven@svenpeter.dev>
> 
> On Sat, Nov 13, 2021, at 22:07, Mark Kettenis wrote:
> >> From: Sven Peter <sven@svenpeter.dev>
> >> Date: Sat, 13 Nov 2021 10:47:31 +0100
> >> 
> >> Apple SoCs come with a simple embedded watchdog. This watchdog is also
> >> required in order to reset the SoC.
> >> 
> >> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> >> ---
> >>  .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
> >>  MAINTAINERS                                   |  1 +
> >>  2 files changed, 53 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> >> 
> >> diff --git a/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> >> new file mode 100644
> >> index 000000000000..e58c56a6fdf6
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> >> @@ -0,0 +1,52 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/watchdog/apple,wdt.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Apple SoC Watchdog
> >> +
> >> +allOf:
> >> +  - $ref: "watchdog.yaml#"
> >
> > I believew this should be:
> >
> >   - $ref: /schemas/watchdog/watchdog.yaml#
> >
> > with that fixed:
> >
> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> >
> 
> Thanks for the review!
> Almost all the other YAML watchdog bindings just use '$ref: "watchdog.yaml#"'.
> Only arm,sp805.yaml uses '$ref: /schemas/watchdog/watchdog.yaml#'.

Hmm, maybe it is ok since the file can be found in the same directory.
Guess it is up to robh then.  Feel free to keep my Reviewed-by
regardless.

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Guenter Roeck]

On 11/13/21 1:47 AM, Sven Peter wrote:
> Add support for the watchdog timer found in Apple SoCs. This driver is
> also required to reboot these machines.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>   MAINTAINERS                  |   1 +
>   drivers/watchdog/Kconfig     |  12 ++
>   drivers/watchdog/Makefile    |   1 +
>   drivers/watchdog/apple_wdt.c | 223 +++++++++++++++++++++++++++++++++++
>   4 files changed, 237 insertions(+)
>   create mode 100644 drivers/watchdog/apple_wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ba480837724d..30a1444297a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1756,6 +1756,7 @@ F:	drivers/i2c/busses/i2c-pasemi-platform.c
>   F:	drivers/irqchip/irq-apple-aic.c
>   F:	drivers/mailbox/apple-mailbox.c
>   F:	drivers/pinctrl/pinctrl-apple-gpio.c
> +F:	drivers/watchdog/apple_wdt.c
>   F:	include/dt-bindings/interrupt-controller/apple-aic.h
>   F:	include/dt-bindings/pinctrl/apple.h
>   F:	include/linux/apple-mailbox.h
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9d222ba17ec6..170dec880c8f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -976,6 +976,18 @@ config MSC313E_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called msc313e_wdt.
>   
> +config APPLE_WATCHDOG
> +	tristate "Apple SoC watchdog"
> +	depends on ARCH_APPLE || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the Watchdog found in Apple
> +	  SoCs such as the M1. Next to the common watchdog features this
> +	  driver is also required in order to reboot these SoCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called apple_wdt.
> +
>   # X86 (i386 + ia64 + x86_64) Architecture
>   
>   config ACQUIRE_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 2ee97064145b..270a518bd8f3 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
>   obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
>   obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
>   obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
> +obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
>   
>   # X86 (i386 + ia64 + x86_64) Architecture
>   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> diff --git a/drivers/watchdog/apple_wdt.c b/drivers/watchdog/apple_wdt.c
> new file mode 100644
> index 000000000000..ff6d98aadffc
> --- /dev/null
> +++ b/drivers/watchdog/apple_wdt.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SoC Watchdog driver
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +/*
> + * Apple Watchdog MMIO registers
> + *
> + * This HW block has three separate watchdogs. WD0 resets the machine
> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
> + * machine reset. WD0 additionally supports a configurable interrupt.
> + *
> + * APPLE_WDT_WDx_CUR_TIME is a simple counter incremented for each tick of the
> + * reference clock. It can also be overwritten to any value.
> + * Whenever APPLE_WDT_CTRL_RESET_EN is set in APPLE_WDT_WDx_CTRL and
> + * APPLE_WDTx_WD1_CUR_TIME >= APPLE_WDTx_WD1_BITE_TIME the entire machine is
> + * reset.
> + * Whenever APPLE_WDT_CTRL_IRQ_EN is set and APPLE_WDTx_WD1_CUR_TIME >=
> + * APPLE_WDTx_WD1_BARK_TIME an interrupt is triggered and
> + * APPLE_WDT_CTRL_IRQ_STATUS is set. The interrupt can be cleared by writing
> + * 1 to APPLE_WDT_CTRL_IRQ_STATUS.

This information is useless since it is not used. It might make sense to add a note
along the line of "This information can be used to implement pretimeout support
at a later time".

> + */
> +#define APPLE_WDT_WD0_CUR_TIME		0x00
> +#define APPLE_WDT_WD0_BITE_TIME		0x04
> +#define APPLE_WDT_WD0_BARK_TIME		0x08
> +#define APPLE_WDT_WD0_CTRL		0x0c
> +
> +#define APPLE_WDT_WD1_CUR_TIME		0x10
> +#define APPLE_WDT_WD1_BITE_TIME		0x14
> +#define APPLE_WDT_WD1_CTRL		0x1c
> +
> +#define APPLE_WDT_WD2_CUR_TIME		0x20
> +#define APPLE_WDT_WD2_BITE_TIME		0x24
> +#define APPLE_WDT_WD2_CTRL		0x2c
> +
> +#define APPLE_WDT_CTRL_IRQ_EN		BIT(0)
> +#define APPLE_WDT_CTRL_IRQ_STATUS	BIT(1)
> +#define APPLE_WDT_CTRL_RESET_EN		BIT(2)

Include linux/bits.h

> +
> +struct apple_wdt {
> +	struct watchdog_device wdd;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	u32 clk_rate;

clk_get_rate returns unsigned long. Any reason to use u32 ?

> +};
> +
> +static struct apple_wdt *to_apple_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct apple_wdt, wdd);
> +}
> +
> +static int apple_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
> +
> +	return 0;
> +}
> +
> +static int apple_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
> +
> +	return 0;
> +}
> +
> +static int apple_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +
> +	return 0;
> +}
> +
> +static int apple_wdt_set_timeout(struct watchdog_device *wdd, unsigned int s)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +	u64 timeout;
> +
> +	timeout = mul_u32_u32(wdt->clk_rate, s);
> +	if (timeout > U32_MAX)
> +		return -EINVAL;
> +

With
	max_timeout = U32_MAX / wdt->clk_rate;

the result of the above multiply operation is never larger than U32_MAX,
and both the error check and the use of mul_u32_u32() are unnecessary.

Just use
	u32 timeout = s * wdt->clk_rate;


> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +	writel_relaxed(timeout, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
> +
> +	wdd->timeout = s;
> +
> +	return 0;
> +}
> +
> +static unsigned int apple_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +	u32 cur_time, reset_time;
> +
> +	cur_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +	reset_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_BITE_TIME);
> +
> +	return (reset_time - cur_time) / wdt->clk_rate;
> +}
> +
> +static int apple_wdt_restart(struct watchdog_device *wdd, unsigned long mode,
> +			     void *cmd)
> +{
> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
> +
> +	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +
> +	/* flush writes and wait for the reset to be triggered */
> +	(void)readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
> +	mdelay(150);

Can this magic number be explained ?

> +
> +	return 0;
> +}
> +
> +static struct watchdog_ops apple_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = apple_wdt_start,
> +	.stop = apple_wdt_stop,
> +	.ping = apple_wdt_ping,
> +	.set_timeout = apple_wdt_set_timeout,
> +	.get_timeleft = apple_wdt_get_timeleft,
> +	.restart = apple_wdt_restart,
> +};
> +
> +static struct watchdog_info apple_wdt_info = {
> +	.identity = "Apple SoC Watchdog",
> +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static int apple_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct apple_wdt *wdt;
> +	int ret;
> +
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, wdt);
> +
> +	wdt->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(wdt->regs))
> +		return PTR_ERR(wdt->regs);
> +
> +	/* reset watchdog to safe defaults */
> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
> +	writel_relaxed(U32_MAX, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
> +
> +	wdt->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(wdt->clk))
> +		return PTR_ERR(wdt->clk);
> +
> +	ret = clk_prepare_enable(wdt->clk);
> +	if (ret)
> +		return ret;
> +
> +	wdt->clk_rate = clk_get_rate(wdt->clk);

clk_get_rate() can, at least in theory, return 0.

> +	wdt->wdd.ops = &apple_wdt_ops;
> +	wdt->wdd.info = &apple_wdt_info;
> +	wdt->wdd.max_timeout = U32_MAX / wdt->clk_rate;
> +	wdt->wdd.timeout = wdt->wdd.max_timeout;

It is is quite unusual to set the default timeout to the maximum.
Normally drivers configure 30 or 60 seconds. Are you sure
this is what you want ?

> +
> +	watchdog_stop_on_unregister(&wdt->wdd);
> +	watchdog_set_restart_priority(&wdt->wdd, 128);
> +
> +	ret = devm_watchdog_register_device(dev, &wdt->wdd); > +	if (ret < 0) {
> +		clk_disable_unprepare(wdt->clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int apple_wdt_remove(struct platform_device *pdev)
> +{
> +	struct apple_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(wdt->clk);

clk_disable_unprepare() must be called after unregistering the watchdog.
Please use devm_add_action_or_reset() in the probe function to handle
clk_disable_unprepare() - see many other watchdog drivers for examples.

> +	return 0;
> +}
> +
> +static const struct of_device_id apple_wdt_of_match[] = {
> +	{ .compatible = "apple,wdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, apple_wdt_of_match);
> +
> +static struct platform_driver apple_wdt_driver = {
> +	.driver = {
> +		.name = "apple-watchdog",
> +		.of_match_table = apple_wdt_of_match,
> +	},
> +	.probe = apple_wdt_probe,
> +	.remove = apple_wdt_remove,
> +};
> +module_platform_driver(apple_wdt_driver);
> +
> +MODULE_DESCRIPTION("Apple SoC watchdog driver");
> +MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
> +MODULE_LICENSE("Dual MIT/GPL");
> 

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Alyssa Rosenzweig]

> >> + * This HW block has three separate watchdogs. WD0 resets the machine
> >> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
> >> + * machine reset. WD0 additionally supports a configurable interrupt.
> >
> > Do we have any idea what the difference between WD1 and WD2 is?
> 
> I've never seen macOS write to WD2 when running in our hypervisor and only
> found that one when I was looking at the rest of the MMIO region.
> >From what I can tell it works exactly like WD1.

Makes sense, thanks.

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Sven Peter]

Thanks a lot for the quick review!

On Sun, Nov 14, 2021, at 13:42, Guenter Roeck wrote:
> On 11/13/21 1:47 AM, Sven Peter wrote:
>> Add support for the watchdog timer found in Apple SoCs. This driver is
>> also required to reboot these machines.
>> 
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>> ---
>>   MAINTAINERS                  |   1 +
>>   drivers/watchdog/Kconfig     |  12 ++
>>   drivers/watchdog/Makefile    |   1 +
>>   drivers/watchdog/apple_wdt.c | 223 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 237 insertions(+)
>>   create mode 100644 drivers/watchdog/apple_wdt.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ba480837724d..30a1444297a1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1756,6 +1756,7 @@ F:	drivers/i2c/busses/i2c-pasemi-platform.c
>>   F:	drivers/irqchip/irq-apple-aic.c
>>   F:	drivers/mailbox/apple-mailbox.c
>>   F:	drivers/pinctrl/pinctrl-apple-gpio.c
>> +F:	drivers/watchdog/apple_wdt.c
>>   F:	include/dt-bindings/interrupt-controller/apple-aic.h
>>   F:	include/dt-bindings/pinctrl/apple.h
>>   F:	include/linux/apple-mailbox.h
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9d222ba17ec6..170dec880c8f 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -976,6 +976,18 @@ config MSC313E_WATCHDOG
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called msc313e_wdt.
>>   
>> +config APPLE_WATCHDOG
>> +	tristate "Apple SoC watchdog"
>> +	depends on ARCH_APPLE || COMPILE_TEST
>> +	select WATCHDOG_CORE
>> +	help
>> +	  Say Y here to include support for the Watchdog found in Apple
>> +	  SoCs such as the M1. Next to the common watchdog features this
>> +	  driver is also required in order to reboot these SoCs.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called apple_wdt.
>> +
>>   # X86 (i386 + ia64 + x86_64) Architecture
>>   
>>   config ACQUIRE_WDT
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 2ee97064145b..270a518bd8f3 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -93,6 +93,7 @@ obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
>>   obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
>>   obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
>>   obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>> +obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
>>   
>>   # X86 (i386 + ia64 + x86_64) Architecture
>>   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
>> diff --git a/drivers/watchdog/apple_wdt.c b/drivers/watchdog/apple_wdt.c
>> new file mode 100644
>> index 000000000000..ff6d98aadffc
>> --- /dev/null
>> +++ b/drivers/watchdog/apple_wdt.c
>> @@ -0,0 +1,223 @@
>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +/*
>> + * Apple SoC Watchdog driver
>> + *
>> + * Copyright (C) The Asahi Linux Contributors
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/limits.h>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/watchdog.h>
>> +
>> +/*
>> + * Apple Watchdog MMIO registers
>> + *
>> + * This HW block has three separate watchdogs. WD0 resets the machine
>> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
>> + * machine reset. WD0 additionally supports a configurable interrupt.
>> + *
>> + * APPLE_WDT_WDx_CUR_TIME is a simple counter incremented for each tick of the
>> + * reference clock. It can also be overwritten to any value.
>> + * Whenever APPLE_WDT_CTRL_RESET_EN is set in APPLE_WDT_WDx_CTRL and
>> + * APPLE_WDTx_WD1_CUR_TIME >= APPLE_WDTx_WD1_BITE_TIME the entire machine is
>> + * reset.
>> + * Whenever APPLE_WDT_CTRL_IRQ_EN is set and APPLE_WDTx_WD1_CUR_TIME >=
>> + * APPLE_WDTx_WD1_BARK_TIME an interrupt is triggered and
>> + * APPLE_WDT_CTRL_IRQ_STATUS is set. The interrupt can be cleared by writing
>> + * 1 to APPLE_WDT_CTRL_IRQ_STATUS.
>
> This information is useless since it is not used. It might make sense 
> to add a note
> along the line of "This information can be used to implement pretimeout 
> support
> at a later time".

Sure, I'll add that line.

>
>> + */
>> +#define APPLE_WDT_WD0_CUR_TIME		0x00
>> +#define APPLE_WDT_WD0_BITE_TIME		0x04
>> +#define APPLE_WDT_WD0_BARK_TIME		0x08
>> +#define APPLE_WDT_WD0_CTRL		0x0c
>> +
>> +#define APPLE_WDT_WD1_CUR_TIME		0x10
>> +#define APPLE_WDT_WD1_BITE_TIME		0x14
>> +#define APPLE_WDT_WD1_CTRL		0x1c
>> +
>> +#define APPLE_WDT_WD2_CUR_TIME		0x20
>> +#define APPLE_WDT_WD2_BITE_TIME		0x24
>> +#define APPLE_WDT_WD2_CTRL		0x2c
>> +
>> +#define APPLE_WDT_CTRL_IRQ_EN		BIT(0)
>> +#define APPLE_WDT_CTRL_IRQ_STATUS	BIT(1)
>> +#define APPLE_WDT_CTRL_RESET_EN		BIT(2)
>
> Include linux/bits.h

Done.

>
>> +
>> +struct apple_wdt {
>> +	struct watchdog_device wdd;
>> +	void __iomem *regs;
>> +	struct clk *clk;
>> +	u32 clk_rate;
>
> clk_get_rate returns unsigned long. Any reason to use u32 ?

No, just a (bad) habit. I'll switch it to unsigned long.

>
>> +};
>> +
>> +static struct apple_wdt *to_apple_wdt(struct watchdog_device *wdd)
>> +{
>> +	return container_of(wdd, struct apple_wdt, wdd);
>> +}
>> +
>> +static int apple_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int apple_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int apple_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +
>> +	return 0;
>> +}
>> +
>> +static int apple_wdt_set_timeout(struct watchdog_device *wdd, unsigned int s)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +	u64 timeout;
>> +
>> +	timeout = mul_u32_u32(wdt->clk_rate, s);
>> +	if (timeout > U32_MAX)
>> +		return -EINVAL;
>> +
>
> With
> 	max_timeout = U32_MAX / wdt->clk_rate;
>
> the result of the above multiply operation is never larger than U32_MAX,
> and both the error check and the use of mul_u32_u32() are unnecessary.
>
> Just use
> 	u32 timeout = s * wdt->clk_rate;

Good point, will do that.

>
>
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +	writel_relaxed(timeout, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
>> +
>> +	wdd->timeout = s;
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int apple_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +	u32 cur_time, reset_time;
>> +
>> +	cur_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +	reset_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_BITE_TIME);
>> +
>> +	return (reset_time - cur_time) / wdt->clk_rate;
>> +}
>> +
>> +static int apple_wdt_restart(struct watchdog_device *wdd, unsigned long mode,
>> +			     void *cmd)
>> +{
>> +	struct apple_wdt *wdt = to_apple_wdt(wdd);
>> +
>> +	writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL);
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +
>> +	/* flush writes and wait for the reset to be triggered */
>> +	(void)readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME);
>> +	mdelay(150);
>
> Can this magic number be explained ?

I actually should just need to wait for a single tick of the watchdog
clock here before it resets the SoC so something like

    while (readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME) == 0);

should work without requiring any magic number.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static struct watchdog_ops apple_wdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = apple_wdt_start,
>> +	.stop = apple_wdt_stop,
>> +	.ping = apple_wdt_ping,
>> +	.set_timeout = apple_wdt_set_timeout,
>> +	.get_timeleft = apple_wdt_get_timeleft,
>> +	.restart = apple_wdt_restart,
>> +};
>> +
>> +static struct watchdog_info apple_wdt_info = {
>> +	.identity = "Apple SoC Watchdog",
>> +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
>> +};
>> +
>> +static int apple_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct apple_wdt *wdt;
>> +	int ret;
>> +
>> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> +	if (!wdt)
>> +		return -ENOMEM;
>> +	platform_set_drvdata(pdev, wdt);
>> +
>> +	wdt->regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(wdt->regs))
>> +		return PTR_ERR(wdt->regs);
>> +
>> +	/* reset watchdog to safe defaults */
>> +	writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL);
>> +	writel_relaxed(U32_MAX, wdt->regs + APPLE_WDT_WD1_BITE_TIME);
>> +
>> +	wdt->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(wdt->clk))
>> +		return PTR_ERR(wdt->clk);
>> +
>> +	ret = clk_prepare_enable(wdt->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	wdt->clk_rate = clk_get_rate(wdt->clk);
>
> clk_get_rate() can, at least in theory, return 0.

Good catch, will add a check for that.

>
>> +	wdt->wdd.ops = &apple_wdt_ops;
>> +	wdt->wdd.info = &apple_wdt_info;
>> +	wdt->wdd.max_timeout = U32_MAX / wdt->clk_rate;
>> +	wdt->wdd.timeout = wdt->wdd.max_timeout;
>
> It is is quite unusual to set the default timeout to the maximum.
> Normally drivers configure 30 or 60 seconds. Are you sure
> this is what you want ?

Didn't really think about this at all. I'll setup a default timeout of 30s
by calling apple_wdt_set_timeout here.

>
>> +
>> +	watchdog_stop_on_unregister(&wdt->wdd);
>> +	watchdog_set_restart_priority(&wdt->wdd, 128);
>> +
>> +	ret = devm_watchdog_register_device(dev, &wdt->wdd); > +	if (ret < 0) {
>> +		clk_disable_unprepare(wdt->clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int apple_wdt_remove(struct platform_device *pdev)
>> +{
>> +	struct apple_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +	clk_disable_unprepare(wdt->clk);
>
> clk_disable_unprepare() must be called after unregistering the watchdog.
> Please use devm_add_action_or_reset() in the probe function to handle
> clk_disable_unprepare() - see many other watchdog drivers for examples.

Sure, will do that.

>
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id apple_wdt_of_match[] = {
>> +	{ .compatible = "apple,wdt" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, apple_wdt_of_match);
>> +
>> +static struct platform_driver apple_wdt_driver = {
>> +	.driver = {
>> +		.name = "apple-watchdog",
>> +		.of_match_table = apple_wdt_of_match,
>> +	},
>> +	.probe = apple_wdt_probe,
>> +	.remove = apple_wdt_remove,
>> +};
>> +module_platform_driver(apple_wdt_driver);
>> +
>> +MODULE_DESCRIPTION("Apple SoC watchdog driver");
>> +MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
>> +MODULE_LICENSE("Dual MIT/GPL");
>>


Thanks,

Sven

Re: [PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Rob Herring]

On Sun, Nov 14, 2021 at 12:58:20PM +0100, Mark Kettenis wrote:
> > Date: Sun, 14 Nov 2021 12:40:48 +0100
> > From: "Sven Peter" <sven@svenpeter.dev>
> > 
> > On Sat, Nov 13, 2021, at 22:07, Mark Kettenis wrote:
> > >> From: Sven Peter <sven@svenpeter.dev>
> > >> Date: Sat, 13 Nov 2021 10:47:31 +0100
> > >> 
> > >> Apple SoCs come with a simple embedded watchdog. This watchdog is also
> > >> required in order to reset the SoC.
> > >> 
> > >> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > >> ---
> > >>  .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
> > >>  MAINTAINERS                                   |  1 +
> > >>  2 files changed, 53 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> > >> 
> > >> diff --git a/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> > >> new file mode 100644
> > >> index 000000000000..e58c56a6fdf6
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> > >> @@ -0,0 +1,52 @@
> > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/watchdog/apple,wdt.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: Apple SoC Watchdog
> > >> +
> > >> +allOf:
> > >> +  - $ref: "watchdog.yaml#"
> > >
> > > I believew this should be:
> > >
> > >   - $ref: /schemas/watchdog/watchdog.yaml#
> > >
> > > with that fixed:
> > >
> > > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> > >
> > 
> > Thanks for the review!
> > Almost all the other YAML watchdog bindings just use '$ref: "watchdog.yaml#"'.
> > Only arm,sp805.yaml uses '$ref: /schemas/watchdog/watchdog.yaml#'.
> 
> Hmm, maybe it is ok since the file can be found in the same directory.
> Guess it is up to robh then.  Feel free to keep my Reviewed-by
> regardless.

Either way is fine. It's just ../ paths I want to avoid.

Rob

Re: [PATCH 1/2] dt-bindings: watchdog: Add Apple Watchdog

[Rob Herring]

On Sat, 13 Nov 2021 10:47:31 +0100, Sven Peter wrote:
> Apple SoCs come with a simple embedded watchdog. This watchdog is also
> required in order to reset the SoC.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  .../bindings/watchdog/apple,wdt.yaml          | 52 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Hector Martin]

On 14/11/2021 22.18, Alyssa Rosenzweig wrote:
>>>> + * This HW block has three separate watchdogs. WD0 resets the machine
>>>> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
>>>> + * machine reset. WD0 additionally supports a configurable interrupt.
>>>
>>> Do we have any idea what the difference between WD1 and WD2 is?
>>
>> I've never seen macOS write to WD2 when running in our hypervisor and only
>> found that one when I was looking at the rest of the MMIO region.
>> >From what I can tell it works exactly like WD1.
> 
> Makes sense, thanks.
> 

Are any of these watchdogs active when we boot, and are we leaving them 
like that? I'm pretty sure at least some of the coprocessors have their 
own watchdog (SMC...), which might be one of these. We should make sure 
we don't clobber that.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver

[Sven Peter]

On Thu, Dec 2, 2021, at 06:14, Hector Martin wrote:
> On 14/11/2021 22.18, Alyssa Rosenzweig wrote:
>>>>> + * This HW block has three separate watchdogs. WD0 resets the machine
>>>>> + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal
>>>>> + * machine reset. WD0 additionally supports a configurable interrupt.
>>>>
>>>> Do we have any idea what the difference between WD1 and WD2 is?
>>>
>>> I've never seen macOS write to WD2 when running in our hypervisor and only
>>> found that one when I was looking at the rest of the MMIO region.
>>> >From what I can tell it works exactly like WD1.
>> 
>> Makes sense, thanks.
>> 
>
> Are any of these watchdogs active when we boot, and are we leaving them 
> like that? I'm pretty sure at least some of the coprocessors have their 
> own watchdog (SMC...), which might be one of these. We should make sure 
> we don't clobber that.

That's what I thought at first as well but they are all disabled except for
WD1 which we disable in m1n1.
I don't touch WD0 or WD2 in this driver anyway and v2 also checks if WD1 is
running (because it might've been started by u-boot) and makes sure the
watchdog core is aware and keeps pinging it.


Sven