[PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Christian Marangi]

Document watchdog for Airoha EN7581. This SoC implement a simple
watchdog that supports a max timeout of 28 seconds.

The watchdog ticks on half the BUS clock and require the BUS frequency
to be provided.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/watchdog/airoha,en7581-wdt.yaml  | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/airoha,en7581-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/airoha,en7581-wdt.yaml b/Documentation/devicetree/bindings/watchdog/airoha,en7581-wdt.yaml
new file mode 100644
index 000000000000..47210a5990ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/airoha,en7581-wdt.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/airoha,en7581-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Watchdog Timer
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+
+allOf:
+  - $ref: watchdog.yaml#
+
+properties:
+  compatible:
+    const: airoha,en7581-wdt
+
+  reg:
+    maxItems: 1
+
+  clock-frequency:
+    description: BUS frequency in Hz (timer ticks at half the BUS freq)
+    const: 300000000
+
+required:
+  - compatible
+  - reg
+  - clock-frequency
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    watchdog@1fbf0100 {
+        compatible = "airoha,en7581-wdt";
+        reg = <0x1fbf0100 0x3c>;
+        clock-frequency = <300000000>;
+    };
-- 
2.45.2

[PATCH 2/2] watchdog: Add support for Airoha EN7851 watchdog

[Christian Marangi]

Add support for Airoha EN7851 watchdog. This is a very basic watchdog
with no pretimeout support, max timeout is 28 seconds and it ticks based
on half the SoC bus clock.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/watchdog/Kconfig      |   8 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/airoha_wdt.c | 214 ++++++++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+)
 create mode 100644 drivers/watchdog/airoha_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 85eea38dbdf4..be4616d18b29 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -394,6 +394,14 @@ config SL28CPLD_WATCHDOG
 
 # ARM Architecture
 
+config AIROHA_WATCHDOG
+	tristate "Airoha EN7581 Watchdog"
+	depends on ARCH_AIROHA || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Watchdog timer embedded into Airoha SoC. This will reboot your
+	  system when the timeout is reached.
+
 config ARM_SP805_WATCHDOG
 	tristate "ARM SP805 Watchdog"
 	depends on (ARM || ARM64 || COMPILE_TEST) && ARM_AMBA
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2d1117564f5b..32a0f689a9be 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
 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_AIROHA_WATCHDOG) += airoha_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/airoha_wdt.c b/drivers/watchdog/airoha_wdt.c
new file mode 100644
index 000000000000..3bbcd7c1f2c4
--- /dev/null
+++ b/drivers/watchdog/airoha_wdt.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *	Airoha Watchdog Driver
+ *
+ *	Copyright (c) 2024, AIROHA  All rights reserved.
+ *
+ *	Mayur Kumar <mayur.kumar@airoha.com>
+ *	Christian Marangi <ansuelsmth@gmail.com>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/math.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+/* Base address of timer and watchdog registers */
+#define TIMER_CTRL			0x0
+#define   WDT_ENABLE			BIT(25)
+#define   WDT_TIMER_INTERRUPT		BIT(21)
+/* Timer3 is used as Watchdog Timer */
+#define   WDT_TIMER_ENABLE		BIT(5)
+#define WDT_TIMER_LOAD_VALUE		0x2c
+#define WDT_TIMER_CUR_VALUE		0x30
+#define  WDT_TIMER_VAL			GENMASK(31, 0)
+#define WDT_RELOAD			0x38
+#define   WDT_RLD			BIT(0)
+
+/* Airoha watchdog structure description */
+struct airoha_wdt_desc {
+	struct watchdog_device wdog_dev;
+	unsigned int wdt_freq;
+	void __iomem *base;
+};
+
+#define WDT_HEARTBEAT			24
+static int heartbeat = WDT_HEARTBEAT;
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. (default="
+		 __MODULE_STRING(WDT_HEARTBEAT) ")");
+
+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) ")");
+
+static int airoha_wdt_start(struct watchdog_device *wdog_dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = watchdog_get_drvdata(wdog_dev);
+	u32 val;
+
+	val = readl(airoha_wdt->base + TIMER_CTRL);
+	val |= (WDT_TIMER_ENABLE | WDT_ENABLE | WDT_TIMER_INTERRUPT);
+	writel(val, airoha_wdt->base + TIMER_CTRL);
+	val = wdog_dev->timeout * airoha_wdt->wdt_freq;
+	writel(val, airoha_wdt->base + WDT_TIMER_LOAD_VALUE);
+
+	return 0;
+}
+
+static int airoha_wdt_stop(struct watchdog_device *wdog_dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = watchdog_get_drvdata(wdog_dev);
+	u32 val;
+
+	val = readl(airoha_wdt->base + TIMER_CTRL);
+	val &= (~WDT_ENABLE & ~WDT_TIMER_ENABLE);
+	writel(val, airoha_wdt->base + TIMER_CTRL);
+
+	return 0;
+}
+
+static int airoha_wdt_ping(struct watchdog_device *wdog_dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = watchdog_get_drvdata(wdog_dev);
+	u32 val;
+
+	val = readl(airoha_wdt->base + WDT_RELOAD);
+	val |= WDT_RLD;
+	writel(val, airoha_wdt->base + WDT_RELOAD);
+
+	return 0;
+}
+
+static int airoha_wdt_set_timeout(struct watchdog_device *wdog_dev, unsigned int timeout)
+{
+	wdog_dev->timeout = timeout;
+
+	if (watchdog_active(wdog_dev)) {
+		airoha_wdt_stop(wdog_dev);
+		return airoha_wdt_start(wdog_dev);
+	}
+
+	return 0;
+}
+
+static unsigned int airoha_wdt_get_timeleft(struct watchdog_device *wdog_dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = watchdog_get_drvdata(wdog_dev);
+	u32 val;
+
+	val = readl(airoha_wdt->base + WDT_TIMER_CUR_VALUE);
+	return DIV_ROUND_UP(val, airoha_wdt->wdt_freq);
+}
+
+static const struct watchdog_info airoha_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+	.identity = "Airoha Watchdog",
+};
+
+static const struct watchdog_ops airoha_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = airoha_wdt_start,
+	.stop = airoha_wdt_stop,
+	.ping = airoha_wdt_ping,
+	.set_timeout = airoha_wdt_set_timeout,
+	.get_timeleft = airoha_wdt_get_timeleft,
+};
+
+static int airoha_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdog_dev;
+	struct airoha_wdt_desc *airoha_wdt;
+	int ret;
+
+	airoha_wdt = devm_kzalloc(dev, sizeof(*airoha_wdt), GFP_KERNEL);
+	if (!airoha_wdt)
+		return -ENOMEM;
+
+	airoha_wdt->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(airoha_wdt->base))
+		return PTR_ERR(airoha_wdt->base);
+
+	ret = device_property_read_u32(dev, "clock-frequency",
+				       &airoha_wdt->wdt_freq);
+	if (ret)
+		return -EINVAL;
+
+	/* Watchdog ticks at half the bus rate */
+	airoha_wdt->wdt_freq /= 2;
+
+	/* Initialize struct watchdog device */
+	wdog_dev = &airoha_wdt->wdog_dev;
+	wdog_dev->timeout = heartbeat;
+	wdog_dev->info = &airoha_wdt_info;
+	wdog_dev->ops = &airoha_wdt_ops;
+	/* Bus 300MHz, watchdog 150MHz, 28 seconds */
+	wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
+	wdog_dev->parent = dev;
+
+	watchdog_set_drvdata(wdog_dev, airoha_wdt);
+	watchdog_set_nowayout(wdog_dev, nowayout);
+	watchdog_stop_on_unregister(wdog_dev);
+
+	ret = devm_watchdog_register_device(dev, wdog_dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, airoha_wdt);
+	return 0;
+}
+
+static int airoha_wdt_suspend(struct device *dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&airoha_wdt->wdog_dev))
+		airoha_wdt_stop(&airoha_wdt->wdog_dev);
+
+	return 0;
+}
+
+static int airoha_wdt_resume(struct device *dev)
+{
+	struct airoha_wdt_desc *airoha_wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&airoha_wdt->wdog_dev)) {
+		airoha_wdt_start(&airoha_wdt->wdog_dev);
+		airoha_wdt_ping(&airoha_wdt->wdog_dev);
+	}
+	return 0;
+}
+
+static const struct of_device_id airoha_wdt_of_match[] = {
+	{ .compatible = "airoha,en7581-wdt", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, airoha_wdt_of_match);
+
+static DEFINE_SIMPLE_DEV_PM_OPS(airoha_wdt_pm_ops, airoha_wdt_suspend, airoha_wdt_resume);
+
+static struct platform_driver airoha_wdt_driver = {
+	.probe = airoha_wdt_probe,
+	.driver = {
+		.name = "airoha-wdt",
+		.pm = pm_sleep_ptr(&airoha_wdt_pm_ops),
+		.of_match_table = airoha_wdt_of_match,
+	},
+};
+
+module_platform_driver(airoha_wdt_driver);
+
+MODULE_AUTHOR("Mayur Kumar <mayur.kumar@airoha.com>");
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_DESCRIPTION("Airoha Watchdog Driver");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Krzysztof Kozlowski]

On 19/09/2024 14:26, Christian Marangi wrote:
> Document watchdog for Airoha EN7581. This SoC implement a simple
> watchdog that supports a max timeout of 28 seconds.
> 
> The watchdog ticks on half the BUS clock and require the BUS frequency
> to be provided.

Clock provider should implement clk_get_rate()...

> 

...

> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +allOf:
> +  - $ref: watchdog.yaml#
> +
> +properties:
> +  compatible:
> +    const: airoha,en7581-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: BUS frequency in Hz (timer ticks at half the BUS freq)
> +    const: 300000000

Which bus frequency? Aren't you missing here clock input?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Christian Marangi]

On Thu, Sep 19, 2024 at 02:35:02PM +0200, Krzysztof Kozlowski wrote:
> On 19/09/2024 14:26, Christian Marangi wrote:
> > Document watchdog for Airoha EN7581. This SoC implement a simple
> > watchdog that supports a max timeout of 28 seconds.
> > 
> > The watchdog ticks on half the BUS clock and require the BUS frequency
> > to be provided.
> 
> Clock provider should implement clk_get_rate()...
>

The BUS clock is internal and not exposed to the system hence
clk_get_rate is not possible saddly.

> > 
> 
> ...
> 
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +allOf:
> > +  - $ref: watchdog.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: airoha,en7581-wdt
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: BUS frequency in Hz (timer ticks at half the BUS freq)
> > +    const: 300000000
> 
> Which bus frequency? Aren't you missing here clock input?

I'm putting here property to describe the internal clock to what the
watchdog is attached. Should I drop this and just hardcode it
internally to the driver or maybe declare the clock to be 150000000
directly?

Tick frequency is already not well defined so I tought it was a good
idea to describe it in DT.

-- 
	Ansuel

Re: [PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Krzysztof Kozlowski]

On 19/09/2024 14:39, Christian Marangi wrote:
> On Thu, Sep 19, 2024 at 02:35:02PM +0200, Krzysztof Kozlowski wrote:
>> On 19/09/2024 14:26, Christian Marangi wrote:
>>> Document watchdog for Airoha EN7581. This SoC implement a simple
>>> watchdog that supports a max timeout of 28 seconds.
>>>
>>> The watchdog ticks on half the BUS clock and require the BUS frequency
>>> to be provided.
>>
>> Clock provider should implement clk_get_rate()...
>>
> 
> The BUS clock is internal and not exposed to the system hence
> clk_get_rate is not possible saddly.
> 
>>>
>>
>> ...
>>
>>> +maintainers:
>>> +  - Christian Marangi <ansuelsmth@gmail.com>
>>> +
>>> +allOf:
>>> +  - $ref: watchdog.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: airoha,en7581-wdt
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency:
>>> +    description: BUS frequency in Hz (timer ticks at half the BUS freq)
>>> +    const: 300000000
>>
>> Which bus frequency? Aren't you missing here clock input?
> 
> I'm putting here property to describe the internal clock to what the
> watchdog is attached. Should I drop this and just hardcode it
> internally to the driver or maybe declare the clock to be 150000000
> directly?

If this stays, then please mention "internal watchdog bus frequency".

If this is internal and it is part of an SoC (so not board!) why would
we need it in DT? I would imagine this is fixed per SoC, thus deduced
from the compatible.

clock-frequency property is legacy and in general discouraged. This
might be an exception, but for that I would like to see more of
explanations.

> 
> Tick frequency is already not well defined so I tought it was a good
> idea to describe it in DT.
> 

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Guenter Roeck]

On 9/19/24 05:39, Christian Marangi wrote:
> On Thu, Sep 19, 2024 at 02:35:02PM +0200, Krzysztof Kozlowski wrote:
>> On 19/09/2024 14:26, Christian Marangi wrote:
>>> Document watchdog for Airoha EN7581. This SoC implement a simple
>>> watchdog that supports a max timeout of 28 seconds.
>>>
>>> The watchdog ticks on half the BUS clock and require the BUS frequency
>>> to be provided.
>>
>> Clock provider should implement clk_get_rate()...
>>
> 
> The BUS clock is internal and not exposed to the system hence
> clk_get_rate is not possible saddly.
> 
>>>
>>
>> ...
>>
>>> +maintainers:
>>> +  - Christian Marangi <ansuelsmth@gmail.com>
>>> +
>>> +allOf:
>>> +  - $ref: watchdog.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: airoha,en7581-wdt
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency:
>>> +    description: BUS frequency in Hz (timer ticks at half the BUS freq)
>>> +    const: 300000000
>>
>> Which bus frequency? Aren't you missing here clock input?
> 
> I'm putting here property to describe the internal clock to what the
> watchdog is attached. Should I drop this and just hardcode it
> internally to the driver or maybe declare the clock to be 150000000
> directly?
> 
> Tick frequency is already not well defined so I tought it was a good
> idea to describe it in DT.
> 

If the value is a constant it should not be in devicetree.

Guenter

Re: [PATCH 2/2] watchdog: Add support for Airoha EN7851 watchdog

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v6.11 next-20240920]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/watchdog-Add-support-for-Airoha-EN7851-watchdog/20240919-203006
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240919122759.10456-2-ansuelsmth%40gmail.com
patch subject: [PATCH 2/2] watchdog: Add support for Airoha EN7851 watchdog
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240922/202409220008.Y6HbRYac-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240922/202409220008.Y6HbRYac-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409220008.Y6HbRYac-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/watchdog/airoha_wdt.c: In function 'airoha_wdt_probe':
>> drivers/watchdog/airoha_wdt.c:155:33: error: implicit declaration of function 'FIELD_MAX' [-Wimplicit-function-declaration]
     155 |         wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
         |                                 ^~~~~~~~~


vim +/FIELD_MAX +155 drivers/watchdog/airoha_wdt.c

   125	
   126	static int airoha_wdt_probe(struct platform_device *pdev)
   127	{
   128		struct device *dev = &pdev->dev;
   129		struct watchdog_device *wdog_dev;
   130		struct airoha_wdt_desc *airoha_wdt;
   131		int ret;
   132	
   133		airoha_wdt = devm_kzalloc(dev, sizeof(*airoha_wdt), GFP_KERNEL);
   134		if (!airoha_wdt)
   135			return -ENOMEM;
   136	
   137		airoha_wdt->base = devm_platform_ioremap_resource(pdev, 0);
   138		if (IS_ERR(airoha_wdt->base))
   139			return PTR_ERR(airoha_wdt->base);
   140	
   141		ret = device_property_read_u32(dev, "clock-frequency",
   142					       &airoha_wdt->wdt_freq);
   143		if (ret)
   144			return -EINVAL;
   145	
   146		/* Watchdog ticks at half the bus rate */
   147		airoha_wdt->wdt_freq /= 2;
   148	
   149		/* Initialize struct watchdog device */
   150		wdog_dev = &airoha_wdt->wdog_dev;
   151		wdog_dev->timeout = heartbeat;
   152		wdog_dev->info = &airoha_wdt_info;
   153		wdog_dev->ops = &airoha_wdt_ops;
   154		/* Bus 300MHz, watchdog 150MHz, 28 seconds */
 > 155		wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
   156		wdog_dev->parent = dev;
   157	
   158		watchdog_set_drvdata(wdog_dev, airoha_wdt);
   159		watchdog_set_nowayout(wdog_dev, nowayout);
   160		watchdog_stop_on_unregister(wdog_dev);
   161	
   162		ret = devm_watchdog_register_device(dev, wdog_dev);
   163		if (ret)
   164			return ret;
   165	
   166		platform_set_drvdata(pdev, airoha_wdt);
   167		return 0;
   168	}
   169	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] watchdog: Add support for Airoha EN7851 watchdog

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v6.11 next-20240920]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/watchdog-Add-support-for-Airoha-EN7851-watchdog/20240919-203006
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240919122759.10456-2-ansuelsmth%40gmail.com
patch subject: [PATCH 2/2] watchdog: Add support for Airoha EN7851 watchdog
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240922/202409221503.OVlRRuM7-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240922/202409221503.OVlRRuM7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409221503.OVlRRuM7-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/watchdog/airoha_wdt.c:17:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/watchdog/airoha_wdt.c:17:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/watchdog/airoha_wdt.c:17:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/watchdog/airoha_wdt.c:155:26: error: call to undeclared function 'FIELD_MAX'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     155 |         wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
         |                                 ^
   6 warnings and 1 error generated.


vim +/FIELD_MAX +155 drivers/watchdog/airoha_wdt.c

   125	
   126	static int airoha_wdt_probe(struct platform_device *pdev)
   127	{
   128		struct device *dev = &pdev->dev;
   129		struct watchdog_device *wdog_dev;
   130		struct airoha_wdt_desc *airoha_wdt;
   131		int ret;
   132	
   133		airoha_wdt = devm_kzalloc(dev, sizeof(*airoha_wdt), GFP_KERNEL);
   134		if (!airoha_wdt)
   135			return -ENOMEM;
   136	
   137		airoha_wdt->base = devm_platform_ioremap_resource(pdev, 0);
   138		if (IS_ERR(airoha_wdt->base))
   139			return PTR_ERR(airoha_wdt->base);
   140	
   141		ret = device_property_read_u32(dev, "clock-frequency",
   142					       &airoha_wdt->wdt_freq);
   143		if (ret)
   144			return -EINVAL;
   145	
   146		/* Watchdog ticks at half the bus rate */
   147		airoha_wdt->wdt_freq /= 2;
   148	
   149		/* Initialize struct watchdog device */
   150		wdog_dev = &airoha_wdt->wdog_dev;
   151		wdog_dev->timeout = heartbeat;
   152		wdog_dev->info = &airoha_wdt_info;
   153		wdog_dev->ops = &airoha_wdt_ops;
   154		/* Bus 300MHz, watchdog 150MHz, 28 seconds */
 > 155		wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
   156		wdog_dev->parent = dev;
   157	
   158		watchdog_set_drvdata(wdog_dev, airoha_wdt);
   159		watchdog_set_nowayout(wdog_dev, nowayout);
   160		watchdog_stop_on_unregister(wdog_dev);
   161	
   162		ret = devm_watchdog_register_device(dev, wdog_dev);
   163		if (ret)
   164			return ret;
   165	
   166		platform_set_drvdata(pdev, airoha_wdt);
   167		return 0;
   168	}
   169	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] dt-bindings: watchdog: airoha: document watchdog for Airoha EN7581

[Christian Marangi]

On Thu, Sep 19, 2024 at 02:42:33PM +0200, Krzysztof Kozlowski wrote:
> On 19/09/2024 14:39, Christian Marangi wrote:
> > On Thu, Sep 19, 2024 at 02:35:02PM +0200, Krzysztof Kozlowski wrote:
> >> On 19/09/2024 14:26, Christian Marangi wrote:
> >>> Document watchdog for Airoha EN7581. This SoC implement a simple
> >>> watchdog that supports a max timeout of 28 seconds.
> >>>
> >>> The watchdog ticks on half the BUS clock and require the BUS frequency
> >>> to be provided.
> >>
> >> Clock provider should implement clk_get_rate()...
> >>
> > 
> > The BUS clock is internal and not exposed to the system hence
> > clk_get_rate is not possible saddly.
> > 
> >>>
> >>
> >> ...
> >>
> >>> +maintainers:
> >>> +  - Christian Marangi <ansuelsmth@gmail.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: watchdog.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: airoha,en7581-wdt
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clock-frequency:
> >>> +    description: BUS frequency in Hz (timer ticks at half the BUS freq)
> >>> +    const: 300000000
> >>
> >> Which bus frequency? Aren't you missing here clock input?
> > 
> > I'm putting here property to describe the internal clock to what the
> > watchdog is attached. Should I drop this and just hardcode it
> > internally to the driver or maybe declare the clock to be 150000000
> > directly?
> 
> If this stays, then please mention "internal watchdog bus frequency".
> 
> If this is internal and it is part of an SoC (so not board!) why would
> we need it in DT? I would imagine this is fixed per SoC, thus deduced
> from the compatible.
> 
> clock-frequency property is legacy and in general discouraged. This
> might be an exception, but for that I would like to see more of
> explanations.
>

Ok it took a while but finally I got my answer. The Documentation had a
mistake and conflicting info. (one bus was said running at 250Mhz instead
of 300Mhz) With this error fixed I can correctly attach a clock and drop
this stupiud thing. Win-Win for everyone!

> > 
> > Tick frequency is already not well defined so I tought it was a good
> > idea to describe it in DT.
> > 
> 

-- 
	Ansuel