[PATCH 1/3] dt-bindings: watchdog: convert Broadcom's WDT to the json-schema

[PATCH 1/3] dt-bindings: watchdog: convert Broadcom's WDT to the json-schema

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This helps validating DTS files. While at it also rename it to bcm63xx.
It's the first SoC with that hardware block and it's a naming schema
used for other Broadcom blocks. It's common for some (originally)
bcm63xx blocks to be reused in newer SoCs (like 7038 or 4908).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/watchdog/brcm,bcm63xx-wdt.yaml   | 38 +++++++++++++++++++
 .../bindings/watchdog/brcm,bcm7038-wdt.txt    | 19 ----------
 2 files changed, 38 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
 delete mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
new file mode 100644
index 000000000000..9d468026550f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/brcm,bcm63xx-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: BCM63xx watchdog timer
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+properties:
+  compatible:
+    const: brcm,bcm7038-wdt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: >
+      The clock running the watchdog. If no clock is found the driver will
+      default to 27000000 Hz.
+
+unevaluatedProperties: false
+
+required:
+  - reg
+
+examples:
+  - |
+    watchdog@f040a7e8 {
+      compatible = "brcm,bcm7038-wdt";
+      reg = <0xf040a7e8 0x16>;
+      clocks = <&upg_fixed>;
+    };
diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
deleted file mode 100644
index 84122270be8f..000000000000
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
+++ /dev/null
@@ -1,19 +0,0 @@
-BCM7038 Watchdog timer
-
-Required properties:
-
-- compatible : should be "brcm,bcm7038-wdt"
-- reg : Specifies base physical address and size of the registers.
-
-Optional properties:
-
-- clocks: The clock running the watchdog. If no clock is found the
-	  driver will default to 27000000 Hz.
-
-Example:
-
-watchdog@f040a7e8 {
-	compatible = "brcm,bcm7038-wdt";
-	clocks = <&upg_fixed>;
-	reg = <0xf040a7e8 0x16>;
-};
-- 
2.31.1

[PATCH 2/3] dt-bindings: watchdog: brcm,bcm63xx-wdt: add BCM4908 compatibility

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

BCM4908 is another SoC with the same hardware block. It just uses a
slightly different registers layout.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml        | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
index 9d468026550f..4391af0c9698 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm63xx-wdt.yaml
@@ -14,7 +14,9 @@ maintainers:
 
 properties:
   compatible:
-    const: brcm,bcm7038-wdt
+    enum:
+      - brcm,bcm4908-wdt
+      - brcm,bcm7038-wdt
 
   reg:
     maxItems: 1
-- 
2.31.1

[PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Hardware supported by this driver goes back to the old bcm63xx days. It
was then reused in BCM7038 and later also in BCM4908.

Depending on SoC model registers layout differs a bit. This commit
introduces support for per-chipset registers offsets & adds BCM4908
layout.

Later on BCM63xx SoCs support should be added too (probably as platform
devices due to missing DT). Eventually this driver should replace
bcm63xx_wdt.c.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/watchdog/Kconfig       |  2 +-
 drivers/watchdog/bcm7038_wdt.c | 89 ++++++++++++++++++++++++++--------
 2 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bf59faeb3de1..324aa942b182 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1756,7 +1756,7 @@ config BCM7038_WDT
 	tristate "BCM7038 Watchdog"
 	select WATCHDOG_CORE
 	depends on HAS_IOMEM
-	depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST
+	depends on ARCH_BCM4908 || ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST
 	help
 	 Watchdog driver for the built-in hardware in Broadcom 7038 and
 	 later SoCs used in set-top boxes.  BCM7038 was made public
diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index acaaa0005d5b..352689f869c1 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -9,6 +9,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/watchdog.h>
@@ -18,14 +19,17 @@
 #define WDT_STOP_1		0xee00
 #define WDT_STOP_2		0x00ee
 
-#define WDT_TIMEOUT_REG		0x0
-#define WDT_CMD_REG		0x4
-
 #define WDT_MIN_TIMEOUT		1 /* seconds */
 #define WDT_DEFAULT_TIMEOUT	30 /* seconds */
 #define WDT_DEFAULT_RATE	27000000
 
+enum bcm63xx_wdt_soc {
+	BCM63XX_WDT_SOC_BCM4908,
+	BCM63XX_WDT_SOC_BCM7038,
+};
+
 struct bcm7038_watchdog {
+	enum bcm63xx_wdt_soc	soc;
 	void __iomem		*base;
 	struct watchdog_device	wdd;
 	u32			rate;
@@ -34,8 +38,52 @@ struct bcm7038_watchdog {
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
-static inline void bcm7038_wdt_write(u32 value, void __iomem *addr)
+static const struct of_device_id bcm7038_wdt_match[] = {
+	{ .compatible = "brcm,bcm4908-wdt", .data = (const void *)BCM63XX_WDT_SOC_BCM4908, },
+	{ .compatible = "brcm,bcm7038-wdt", .data = (const void *)BCM63XX_WDT_SOC_BCM7038, },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);
+
+enum bcm7038_wdt_regs {
+	BCM63XX_WDT_REG_DEFVAL = 0,
+	BCM63XX_WDT_REG_CTL,
+	BCM63XX_WDT_REG_SOFTRESET,
+};
+
+static const u16 bcm7038_wdt_regs_bcm4908[] = {
+	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
+	[BCM63XX_WDT_REG_CTL]		= 0x2c,
+	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
+};
+
+static const u16 bcm7038_wdt_regs_bcm7038[] = {
+	[BCM63XX_WDT_REG_DEFVAL]	= 0x00,
+	[BCM63XX_WDT_REG_CTL]		= 0x04,
+};
+
+static void __iomem *bcm7038_wdt_reg_addr(struct watchdog_device *wdog,
+					  enum bcm7038_wdt_regs reg)
 {
+	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
+	void __iomem *addr = wdt->base;
+
+	switch (wdt->soc) {
+	case BCM63XX_WDT_SOC_BCM4908:
+		return addr + bcm7038_wdt_regs_bcm4908[reg];
+	case BCM63XX_WDT_SOC_BCM7038:
+		return addr + bcm7038_wdt_regs_bcm7038[reg];
+	default:
+		WARN_ON(1);
+		return NULL;
+	}
+}
+
+static void bcm7038_wdt_write(struct watchdog_device *wdog,
+			      enum bcm7038_wdt_regs reg, u32 value)
+{
+	void __iomem *addr = bcm7038_wdt_reg_addr(wdog, reg);
+
 	/* MIPS chips strapped for BE will automagically configure the
 	 * peripheral registers for CPU-native byte order.
 	 */
@@ -45,8 +93,11 @@ static inline void bcm7038_wdt_write(u32 value, void __iomem *addr)
 		writel_relaxed(value, addr);
 }
 
-static inline u32 bcm7038_wdt_read(void __iomem *addr)
+static inline u32 bcm7038_wdt_read(struct watchdog_device *wdog,
+				   enum bcm7038_wdt_regs reg)
 {
+	void __iomem *addr = bcm7038_wdt_reg_addr(wdog, reg);
+
 	if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
 		return __raw_readl(addr);
 	else
@@ -60,15 +111,13 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
 
 	timeout = wdt->rate * wdog->timeout;
 
-	bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
+	bcm7038_wdt_write(wdog, BCM63XX_WDT_REG_DEFVAL, timeout);
 }
 
 static int bcm7038_wdt_ping(struct watchdog_device *wdog)
 {
-	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
-
-	bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
-	bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(wdog, BCM63XX_WDT_REG_CTL, WDT_START_1);
+	bcm7038_wdt_write(wdog, BCM63XX_WDT_REG_CTL, WDT_START_2);
 
 	return 0;
 }
@@ -83,10 +132,8 @@ static int bcm7038_wdt_start(struct watchdog_device *wdog)
 
 static int bcm7038_wdt_stop(struct watchdog_device *wdog)
 {
-	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
-
-	bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
-	bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
+	bcm7038_wdt_write(wdog, BCM63XX_WDT_REG_CTL, WDT_STOP_1);
+	bcm7038_wdt_write(wdog, BCM63XX_WDT_REG_CTL, WDT_STOP_2);
 
 	return 0;
 }
@@ -107,7 +154,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
 	struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
 	u32 time_left;
 
-	time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
+	time_left = bcm7038_wdt_read(wdog, BCM63XX_WDT_REG_CTL);
 
 	return time_left / wdt->rate;
 }
@@ -134,6 +181,7 @@ static void bcm7038_clk_disable_unprepare(void *data)
 static int bcm7038_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
 	struct bcm7038_watchdog *wdt;
 	int err;
 
@@ -143,6 +191,11 @@ static int bcm7038_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, wdt);
 
+	of_id = of_match_device(bcm7038_wdt_match, dev);
+	if (!of_id)
+		return -EINVAL;
+	wdt->soc = (enum bcm63xx_wdt_soc)of_id->data;
+
 	wdt->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(wdt->base))
 		return PTR_ERR(wdt->base);
@@ -211,12 +264,6 @@ static int bcm7038_wdt_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(bcm7038_wdt_pm_ops, bcm7038_wdt_suspend,
 			 bcm7038_wdt_resume);
 
-static const struct of_device_id bcm7038_wdt_match[] = {
-	{ .compatible = "brcm,bcm7038-wdt" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, bcm7038_wdt_match);
-
 static struct platform_driver bcm7038_wdt_driver = {
 	.probe		= bcm7038_wdt_probe,
 	.driver		= {
-- 
2.31.1

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Florian Fainelli]

On 10/28/21 2:30 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Hardware supported by this driver goes back to the old bcm63xx days. It
> was then reused in BCM7038 and later also in BCM4908.
> 
> Depending on SoC model registers layout differs a bit. This commit
> introduces support for per-chipset registers offsets & adds BCM4908
> layout.
> 
> Later on BCM63xx SoCs support should be added too (probably as platform
> devices due to missing DT). Eventually this driver should replace
> bcm63xx_wdt.c.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---

[snip]

> +
> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,

I don't understand what you are doing here and why you are not
offsetting the "reg" property appropriately when you create your
bcm4908-wdt Device Tree node such that the base starts at 0, and the
existing driver becomes usable as-is. This does not make any sense to me
when it is obviously the simplest way to make the driver "accept" the
resource being passed.

I am going to send my patch series converting the bcm63xx_wdt.c driver
over to bcm7038_wdt.c, feel free to use or discard it.
-- 
Florian

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Guenter Roeck]

On 10/28/21 9:29 AM, Florian Fainelli wrote:
> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Hardware supported by this driver goes back to the old bcm63xx days. It
>> was then reused in BCM7038 and later also in BCM4908.
>>
>> Depending on SoC model registers layout differs a bit. This commit
>> introduces support for per-chipset registers offsets & adds BCM4908
>> layout.
>>
>> Later on BCM63xx SoCs support should be added too (probably as platform
>> devices due to missing DT). Eventually this driver should replace
>> bcm63xx_wdt.c.
>>
Seems unrelated / irrelevant in this commit log, except maybe after '---'.

>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
> 
> [snip]
> 
>> +
>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,

REG_DEFVAL is an odd name for this register. I can see that the
bcm63xx driver uses it, but in reality it seems to be the timeout
value, not some default value, only the bcm63xx driver doesn't
seem to use it properly. I think REG_TIMEOUT or similar would
be a much better name.

>> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
>> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
> 
> I don't understand what you are doing here and why you are not
> offsetting the "reg" property appropriately when you create your
> bcm4908-wdt Device Tree node such that the base starts at 0, and the
> existing driver becomes usable as-is. This does not make any sense to me
> when it is obviously the simplest way to make the driver "accept" the
> resource being passed.
> 

Agreed. This adds a lot of complexity that could be avoided.

> I am going to send my patch series converting the bcm63xx_wdt.c driver
> over to bcm7038_wdt.c, feel free to use or discard it.
> 
For my part I am very much looking forward to it.

Guenter

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rafał Miłecki]

On 28.10.2021 18:57, Guenter Roeck wrote:
> On 10/28/21 9:29 AM, Florian Fainelli wrote:
>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>> was then reused in BCM7038 and later also in BCM4908.
>>>
>>> Depending on SoC model registers layout differs a bit. This commit
>>> introduces support for per-chipset registers offsets & adds BCM4908
>>> layout.
>>>
>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>> devices due to missing DT). Eventually this driver should replace
>>> bcm63xx_wdt.c.
>>>
> Seems unrelated / irrelevant in this commit log, except maybe after '---'.
> 
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>
>> [snip]
>>
>>> +
>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
> 
> REG_DEFVAL is an odd name for this register. I can see that the
> bcm63xx driver uses it, but in reality it seems to be the timeout
> value, not some default value, only the bcm63xx driver doesn't
> seem to use it properly. I think REG_TIMEOUT or similar would
> be a much better name.

I used name used in Broadcom's SDK (and as I guess also in their
documentation too).

Take a look at this BCM60333 example:

typedef struct Timer {
	uint32	TimerInts;		/* 0x00 */
	uint32	TimerCtl0;		/* 0x04 */
	uint32	TimerCtl1;		/* 0x08 */
	uint32	TimerCtl2;		/* 0x0c */
	uint32	TimerCnt0;		/* 0x10 */
	uint32	TimerCnt1;		/* 0x14 */
	uint32	TimerCnt2;		/* 0x18 */
	uint32	WatchDogDefCount;	/* 0x1c */
	uint32	WatchDogCtl;		/* 0x20 */
	uint32	WDResetCount;		/* 0x24 */
} Timer;

I got impression that Linux driver registers names usually follow what
is used in hardware documentation.

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Guenter Roeck]

On 10/29/21 5:15 AM, Rafał Miłecki wrote:
> On 28.10.2021 18:57, Guenter Roeck wrote:
>> On 10/28/21 9:29 AM, Florian Fainelli wrote:
>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>
>>>> Depending on SoC model registers layout differs a bit. This commit
>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>> layout.
>>>>
>>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>>> devices due to missing DT). Eventually this driver should replace
>>>> bcm63xx_wdt.c.
>>>>
>> Seems unrelated / irrelevant in this commit log, except maybe after '---'.
>>
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +
>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>
>> REG_DEFVAL is an odd name for this register. I can see that the
>> bcm63xx driver uses it, but in reality it seems to be the timeout
>> value, not some default value, only the bcm63xx driver doesn't
>> seem to use it properly. I think REG_TIMEOUT or similar would
>> be a much better name.
> 
> I used name used in Broadcom's SDK (and as I guess also in their
> documentation too).
> 
> Take a look at this BCM60333 example:
> 
> typedef struct Timer {
>      uint32    TimerInts;        /* 0x00 */
>      uint32    TimerCtl0;        /* 0x04 */
>      uint32    TimerCtl1;        /* 0x08 */
>      uint32    TimerCtl2;        /* 0x0c */
>      uint32    TimerCnt0;        /* 0x10 */
>      uint32    TimerCnt1;        /* 0x14 */
>      uint32    TimerCnt2;        /* 0x18 */
>      uint32    WatchDogDefCount;    /* 0x1c */
>      uint32    WatchDogCtl;        /* 0x20 */
>      uint32    WDResetCount;        /* 0x24 */
> } Timer;
> 
> I got impression that Linux driver registers names usually follow what
> is used in hardware documentation.

Still, the key part of the register name is "Count", not "Def",
and there is no "val" in there.

Guenter

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rafał Miłecki]

On 29.10.2021 16:15, Guenter Roeck wrote:
> On 10/29/21 5:15 AM, Rafał Miłecki wrote:
>> On 28.10.2021 18:57, Guenter Roeck wrote:
>>> On 10/28/21 9:29 AM, Florian Fainelli wrote:
>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>
>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>> layout.
>>>>>
>>>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>>>> devices due to missing DT). Eventually this driver should replace
>>>>> bcm63xx_wdt.c.
>>>>>
>>> Seems unrelated / irrelevant in this commit log, except maybe after '---'.
>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +
>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>>
>>> REG_DEFVAL is an odd name for this register. I can see that the
>>> bcm63xx driver uses it, but in reality it seems to be the timeout
>>> value, not some default value, only the bcm63xx driver doesn't
>>> seem to use it properly. I think REG_TIMEOUT or similar would
>>> be a much better name.
>>
>> I used name used in Broadcom's SDK (and as I guess also in their
>> documentation too).
>>
>> Take a look at this BCM60333 example:
>>
>> typedef struct Timer {
>>      uint32    TimerInts;        /* 0x00 */
>>      uint32    TimerCtl0;        /* 0x04 */
>>      uint32    TimerCtl1;        /* 0x08 */
>>      uint32    TimerCtl2;        /* 0x0c */
>>      uint32    TimerCnt0;        /* 0x10 */
>>      uint32    TimerCnt1;        /* 0x14 */
>>      uint32    TimerCnt2;        /* 0x18 */
>>      uint32    WatchDogDefCount;    /* 0x1c */
>>      uint32    WatchDogCtl;        /* 0x20 */
>>      uint32    WDResetCount;        /* 0x24 */
>> } Timer;
>>
>> I got impression that Linux driver registers names usually follow what
>> is used in hardware documentation.
> 
> Still, the key part of the register name is "Count", not "Def",
> and there is no "val" in there.

Absolutely right. No idea where did I take it from and how did I miss that.

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rafał Miłecki]

[Rob: please kindly comment on this]

On 28.10.2021 18:29, Florian Fainelli wrote:
> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Hardware supported by this driver goes back to the old bcm63xx days. It
>> was then reused in BCM7038 and later also in BCM4908.
>>
>> Depending on SoC model registers layout differs a bit. This commit
>> introduces support for per-chipset registers offsets & adds BCM4908
>> layout.
>>
>> Later on BCM63xx SoCs support should be added too (probably as platform
>> devices due to missing DT). Eventually this driver should replace
>> bcm63xx_wdt.c.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
> 
> [snip]
> 
>> +
>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
>> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
>> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
> 
> I don't understand what you are doing here and why you are not
> offsetting the "reg" property appropriately when you create your
> bcm4908-wdt Device Tree node such that the base starts at 0, and the
> existing driver becomes usable as-is. This does not make any sense to me
> when it is obviously the simplest way to make the driver "accept" the
> resource being passed.

I believe that DT binding should cover the whole hardware block and
describe it (here: use proper compatible to allow recognizing block
variant).

That's because (as far as I understand) DT should be used to describe
hardware as closely as possible. I think it shouldn't be adjusted to
make mapping match Linux's driver implementation.


So if:
1. Hardware block is mapped at 0xff800400
2. It has interesting registers at 0xff800428 and 0xff80042c

I think mapping should use:
reg = <0xff800400 0x3c>;
even if we don't use the first N registers.

That way, at some point, you can extend Linux (or whatever) driver to
use extra registers without reworking the whole binding. That's why I
think we need to map whole hardware block & handle different registers
layouts in a driver.


Now, that is something I learnt from various DT discussions but I still
may got it wrong. I'd like to ask Rob to comment on this.


Let me also paste my summary of BCM4908's block I extracted from
Broadcom's header:

typedef struct Timer {
	uint32	TimerCtl0;		/* 0x00 */
	uint32	TimerCtl1;		/* 0x04 */
	uint32	TimerCtl2;		/* 0x08 */
	uint32	TimerCtl3;		/* 0x0c */
	uint32	TimerCnt0;		/* 0x10 */
	uint32	TimerCnt1;		/* 0x14 */
	uint32	TimerCnt2;		/* 0x18 */
	uint32	TimerCnt3;		/* 0x1c */
	uint32	TimerMask;		/* 0x20 */
	uint32	TimerInts;		/* 0x24 */
	uint32	WatchDogDefCount;	/* 0x28 */
	uint32	WatchDogCtl;		/* 0x2c */
	uint32	WDResetCount;		/* 0x30 */
	uint32	SoftRst;		/* 0x34 */
	uint32	ResetStatus;		/* 0x38 */
	uint32	ResetReason;		/* 0x3c */
	uint32	spare[3];		/* 0x40-0x4b */
};

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rob Herring]

On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
> [Rob: please kindly comment on this]
> 
> On 28.10.2021 18:29, Florian Fainelli wrote:
> > On 10/28/21 2:30 AM, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > Hardware supported by this driver goes back to the old bcm63xx days. It
> > > was then reused in BCM7038 and later also in BCM4908.
> > > 
> > > Depending on SoC model registers layout differs a bit. This commit
> > > introduces support for per-chipset registers offsets & adds BCM4908
> > > layout.
> > > 
> > > Later on BCM63xx SoCs support should be added too (probably as platform
> > > devices due to missing DT). Eventually this driver should replace
> > > bcm63xx_wdt.c.
> > > 
> > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > > ---
> > 
> > [snip]
> > 
> > > +
> > > +static const u16 bcm7038_wdt_regs_bcm4908[] = {
> > > +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
> > > +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
> > > +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
> > 
> > I don't understand what you are doing here and why you are not
> > offsetting the "reg" property appropriately when you create your
> > bcm4908-wdt Device Tree node such that the base starts at 0, and the
> > existing driver becomes usable as-is. This does not make any sense to me
> > when it is obviously the simplest way to make the driver "accept" the
> > resource being passed.
> 
> I believe that DT binding should cover the whole hardware block and
> describe it (here: use proper compatible to allow recognizing block
> variant).
> 
> That's because (as far as I understand) DT should be used to describe
> hardware as closely as possible. I think it shouldn't be adjusted to
> make mapping match Linux's driver implementation.
> 
> 
> So if:
> 1. Hardware block is mapped at 0xff800400
> 2. It has interesting registers at 0xff800428 and 0xff80042c
> 
> I think mapping should use:
> reg = <0xff800400 0x3c>;
> even if we don't use the first N registers.
> 
> That way, at some point, you can extend Linux (or whatever) driver to
> use extra registers without reworking the whole binding. That's why I
> think we need to map whole hardware block & handle different registers
> layouts in a driver.

Yes, that's the correct thing to do.

The question is whether you'd need sub nodes for the other functions. 
Folks tend to want to have sub nodes for convenience which isn't really 
needed and then requires a DT update ('cause they add nodes as adding 
drivers).

Based on the registers, you really don't need sub nodes here.

Rob

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Florian Fainelli]

On 10/29/21 6:03 AM, Rob Herring wrote:
> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>> [Rob: please kindly comment on this]
>>
>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>
>>>> Depending on SoC model registers layout differs a bit. This commit
>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>> layout.
>>>>
>>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>>> devices due to missing DT). Eventually this driver should replace
>>>> bcm63xx_wdt.c.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +
>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
>>>> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
>>>> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
>>>
>>> I don't understand what you are doing here and why you are not
>>> offsetting the "reg" property appropriately when you create your
>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>> existing driver becomes usable as-is. This does not make any sense to me
>>> when it is obviously the simplest way to make the driver "accept" the
>>> resource being passed.
>>
>> I believe that DT binding should cover the whole hardware block and
>> describe it (here: use proper compatible to allow recognizing block
>> variant).
>>
>> That's because (as far as I understand) DT should be used to describe
>> hardware as closely as possible. I think it shouldn't be adjusted to
>> make mapping match Linux's driver implementation.
>>
>>
>> So if:
>> 1. Hardware block is mapped at 0xff800400
>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>
>> I think mapping should use:
>> reg = <0xff800400 0x3c>;
>> even if we don't use the first N registers.
>>
>> That way, at some point, you can extend Linux (or whatever) driver to
>> use extra registers without reworking the whole binding. That's why I
>> think we need to map whole hardware block & handle different registers
>> layouts in a driver.
> 
> Yes, that's the correct thing to do.

So in the future if we happen to want to manage the hardware timers in
that block, they would be part of the watchdog driver? I am fairly sure
they won't be, so you will be creating a composite driver/MFD to
separate out the functions, more likely. So you might as well create
sub-nodes.

> 
> The question is whether you'd need sub nodes for the other functions. 
> Folks tend to want to have sub nodes for convenience which isn't really 
> needed and then requires a DT update ('cause they add nodes as adding 
> drivers).

Sorry but not, this is getting completely ridiculous, the

> 
> Based on the registers, you really don't need sub nodes here.

I sort of disagree here, the watchdog is a part of a sundry timer block
here, but it is logically broken up into different parts and if if I
were to imagine how this would map into different drivers, I can easily
see that we would have:

- a driver to manage the timer interrupt controller
- a driver to manage each of the 3 hardware timers, be they clockevent
or else
- a driver to manage the watchdog

The simplest way to get there, and also because these same timer blocks
are actually spread in other parts of STB chips just like the watchdog
is, but in a different layout where they stand on their own was the main
drive for defining the bcm7038_wdt binding the way it was.

Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
driver and this is better than the previous patch set, but I really do
not see why having the watchdog driver not manage the *exact* subset of
the register space needed (starting at 0x28) is not being done.
--
Florian

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rafał Miłecki]

On 29.10.2021 18:45, Florian Fainelli wrote:
> On 10/29/21 6:03 AM, Rob Herring wrote:
>> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>>> [Rob: please kindly comment on this]
>>>
>>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>
>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>> layout.
>>>>>
>>>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>>>> devices due to missing DT). Eventually this driver should replace
>>>>> bcm63xx_wdt.c.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +
>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
>>>>> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
>>>>> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
>>>>
>>>> I don't understand what you are doing here and why you are not
>>>> offsetting the "reg" property appropriately when you create your
>>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>>> existing driver becomes usable as-is. This does not make any sense to me
>>>> when it is obviously the simplest way to make the driver "accept" the
>>>> resource being passed.
>>>
>>> I believe that DT binding should cover the whole hardware block and
>>> describe it (here: use proper compatible to allow recognizing block
>>> variant).
>>>
>>> That's because (as far as I understand) DT should be used to describe
>>> hardware as closely as possible. I think it shouldn't be adjusted to
>>> make mapping match Linux's driver implementation.
>>>
>>>
>>> So if:
>>> 1. Hardware block is mapped at 0xff800400
>>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>>
>>> I think mapping should use:
>>> reg = <0xff800400 0x3c>;
>>> even if we don't use the first N registers.
>>>
>>> That way, at some point, you can extend Linux (or whatever) driver to
>>> use extra registers without reworking the whole binding. That's why I
>>> think we need to map whole hardware block & handle different registers
>>> layouts in a driver.
>>
>> Yes, that's the correct thing to do.
> 
> So in the future if we happen to want to manage the hardware timers in
> that block, they would be part of the watchdog driver? I am fairly sure
> they won't be, so you will be creating a composite driver/MFD to
> separate out the functions, more likely. So you might as well create
> sub-nodes.
> 
>>
>> The question is whether you'd need sub nodes for the other functions.
>> Folks tend to want to have sub nodes for convenience which isn't really
>> needed and then requires a DT update ('cause they add nodes as adding
>> drivers).
> 
> Sorry but not, this is getting completely ridiculous, the
> 
>>
>> Based on the registers, you really don't need sub nodes here.
> 
> I sort of disagree here, the watchdog is a part of a sundry timer block
> here, but it is logically broken up into different parts and if if I
> were to imagine how this would map into different drivers, I can easily
> see that we would have:
> 
> - a driver to manage the timer interrupt controller
> - a driver to manage each of the 3 hardware timers, be they clockevent
> or else
> - a driver to manage the watchdog
> 
> The simplest way to get there, and also because these same timer blocks
> are actually spread in other parts of STB chips just like the watchdog
> is, but in a different layout where they stand on their own was the main
> drive for defining the bcm7038_wdt binding the way it was.
> 
> Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
> driver and this is better than the previous patch set, but I really do
> not see why having the watchdog driver not manage the *exact* subset of
> the register space needed (starting at 0x28) is not being done.

Sure, let's get this discussed before pushing my patches. I'm happy to
have you and Rob involved here. Once we get into agreement we can decide
how to proceed.

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Guenter Roeck]

On 10/29/21 9:45 AM, Florian Fainelli wrote:
> On 10/29/21 6:03 AM, Rob Herring wrote:
>> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>>> [Rob: please kindly comment on this]
>>>
>>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Hardware supported by this driver goes back to the old bcm63xx days. It
>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>
>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>> layout.
>>>>>
>>>>> Later on BCM63xx SoCs support should be added too (probably as platform
>>>>> devices due to missing DT). Eventually this driver should replace
>>>>> bcm63xx_wdt.c.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +
>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>> +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
>>>>> +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
>>>>> +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
>>>>
>>>> I don't understand what you are doing here and why you are not
>>>> offsetting the "reg" property appropriately when you create your
>>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>>> existing driver becomes usable as-is. This does not make any sense to me
>>>> when it is obviously the simplest way to make the driver "accept" the
>>>> resource being passed.
>>>
>>> I believe that DT binding should cover the whole hardware block and
>>> describe it (here: use proper compatible to allow recognizing block
>>> variant).
>>>
>>> That's because (as far as I understand) DT should be used to describe
>>> hardware as closely as possible. I think it shouldn't be adjusted to
>>> make mapping match Linux's driver implementation.
>>>
>>>
>>> So if:
>>> 1. Hardware block is mapped at 0xff800400
>>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>>
>>> I think mapping should use:
>>> reg = <0xff800400 0x3c>;
>>> even if we don't use the first N registers.
>>>
>>> That way, at some point, you can extend Linux (or whatever) driver to
>>> use extra registers without reworking the whole binding. That's why I
>>> think we need to map whole hardware block & handle different registers
>>> layouts in a driver.
>>
>> Yes, that's the correct thing to do.
> 
> So in the future if we happen to want to manage the hardware timers in
> that block, they would be part of the watchdog driver? I am fairly sure
> they won't be, so you will be creating a composite driver/MFD to
> separate out the functions, more likely. So you might as well create
> sub-nodes.
> 
>>
>> The question is whether you'd need sub nodes for the other functions.
>> Folks tend to want to have sub nodes for convenience which isn't really
>> needed and then requires a DT update ('cause they add nodes as adding
>> drivers).
> 
> Sorry but not, this is getting completely ridiculous, the
> 
>>
>> Based on the registers, you really don't need sub nodes here.
> 
> I sort of disagree here, the watchdog is a part of a sundry timer block
> here, but it is logically broken up into different parts and if if I
> were to imagine how this would map into different drivers, I can easily
> see that we would have:
> 
> - a driver to manage the timer interrupt controller
> - a driver to manage each of the 3 hardware timers, be they clockevent
> or else
> - a driver to manage the watchdog
> 
> The simplest way to get there, and also because these same timer blocks
> are actually spread in other parts of STB chips just like the watchdog
> is, but in a different layout where they stand on their own was the main
> drive for defining the bcm7038_wdt binding the way it was.
> 
> Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
> driver and this is better than the previous patch set, but I really do
> not see why having the watchdog driver not manage the *exact* subset of
> the register space needed (starting at 0x28) is not being done.

Agreed, especially since other sub-devices of bcm4908 are alredy modeled
this way. See arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi.
At this point, before accepting anything, I'll want to have an explanation
how and why the watchdog interface is handled differently than, say,
its reset controller. Also, I'd like to understand the memory region
assigned to bcm7038, which happens to be something like:

     compatible = "brcm,bcm7038-wdt";
         reg = <0xf040a7e8 0x16>;

because it seems unlikely that this is a chip subsystem that just happens
to start at such an odd boundary. More specifically, I see in actual
.dtsi files data such as:

                 watchdog: watchdog@4066a8 {
                         clocks = <&upg_clk>;
                         compatible = "brcm,bcm7038-wdt";
                         reg = <0x4066a8 0x14>;
                         status = "disabled";
                 };
...
                 timers: timer@406680 {
                         compatible = "brcm,brcmstb-timers";
                         reg = <0x406680 0x40>;
                 };

So there happen to be timers in the same region, and the offset
between timer and watchdog registers is 0x28. Coincidentally, that
just happens to be the extra offset defined in this patch for the
bcm4908 watchdog. Really ? Sorry, this sounds very inconsistent
and arbitrary to me.

Overall, I suspect I'll have to see datasheets if we really end up
having different offsets for each chip, because I'll want to confirm
that the watchdog subsystem isn't treated differently than other
subsystems, and that the offset calculations are appropriate and
consistent across the different chips.

Guenter

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Florian Fainelli]

On 10/29/21 10:43 AM, Guenter Roeck wrote:
> On 10/29/21 9:45 AM, Florian Fainelli wrote:
>> On 10/29/21 6:03 AM, Rob Herring wrote:
>>> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>>>> [Rob: please kindly comment on this]
>>>>
>>>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> Hardware supported by this driver goes back to the old bcm63xx
>>>>>> days. It
>>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>>
>>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>>> layout.
>>>>>>
>>>>>> Later on BCM63xx SoCs support should be added too (probably as
>>>>>> platform
>>>>>> devices due to missing DT). Eventually this driver should replace
>>>>>> bcm63xx_wdt.c.
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>> ---
>>>>>
>>>>> [snip]
>>>>>
>>>>>> +
>>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>>>>> +    [BCM63XX_WDT_REG_CTL]        = 0x2c,
>>>>>> +    [BCM63XX_WDT_REG_SOFTRESET]    = 0x34,
>>>>>
>>>>> I don't understand what you are doing here and why you are not
>>>>> offsetting the "reg" property appropriately when you create your
>>>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>>>> existing driver becomes usable as-is. This does not make any sense
>>>>> to me
>>>>> when it is obviously the simplest way to make the driver "accept" the
>>>>> resource being passed.
>>>>
>>>> I believe that DT binding should cover the whole hardware block and
>>>> describe it (here: use proper compatible to allow recognizing block
>>>> variant).
>>>>
>>>> That's because (as far as I understand) DT should be used to describe
>>>> hardware as closely as possible. I think it shouldn't be adjusted to
>>>> make mapping match Linux's driver implementation.
>>>>
>>>>
>>>> So if:
>>>> 1. Hardware block is mapped at 0xff800400
>>>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>>>
>>>> I think mapping should use:
>>>> reg = <0xff800400 0x3c>;
>>>> even if we don't use the first N registers.
>>>>
>>>> That way, at some point, you can extend Linux (or whatever) driver to
>>>> use extra registers without reworking the whole binding. That's why I
>>>> think we need to map whole hardware block & handle different registers
>>>> layouts in a driver.
>>>
>>> Yes, that's the correct thing to do.
>>
>> So in the future if we happen to want to manage the hardware timers in
>> that block, they would be part of the watchdog driver? I am fairly sure
>> they won't be, so you will be creating a composite driver/MFD to
>> separate out the functions, more likely. So you might as well create
>> sub-nodes.
>>
>>>
>>> The question is whether you'd need sub nodes for the other functions.
>>> Folks tend to want to have sub nodes for convenience which isn't really
>>> needed and then requires a DT update ('cause they add nodes as adding
>>> drivers).
>>
>> Sorry but not, this is getting completely ridiculous, the
>>
>>>
>>> Based on the registers, you really don't need sub nodes here.
>>
>> I sort of disagree here, the watchdog is a part of a sundry timer block
>> here, but it is logically broken up into different parts and if if I
>> were to imagine how this would map into different drivers, I can easily
>> see that we would have:
>>
>> - a driver to manage the timer interrupt controller
>> - a driver to manage each of the 3 hardware timers, be they clockevent
>> or else
>> - a driver to manage the watchdog
>>
>> The simplest way to get there, and also because these same timer blocks
>> are actually spread in other parts of STB chips just like the watchdog
>> is, but in a different layout where they stand on their own was the main
>> drive for defining the bcm7038_wdt binding the way it was.
>>
>> Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
>> driver and this is better than the previous patch set, but I really do
>> not see why having the watchdog driver not manage the *exact* subset of
>> the register space needed (starting at 0x28) is not being done.
> 
> Agreed, especially since other sub-devices of bcm4908 are alredy modeled
> this way. See arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi.
> At this point, before accepting anything, I'll want to have an explanation
> how and why the watchdog interface is handled differently than, say,
> its reset controller. Also, I'd like to understand the memory region
> assigned to bcm7038, which happens to be something like:
> 
>     compatible = "brcm,bcm7038-wdt";
>         reg = <0xf040a7e8 0x16>;
> 
> because it seems unlikely that this is a chip subsystem that just happens
> to start at such an odd boundary. More specifically, I see in actual
> .dtsi files data such as:
> 
>                 watchdog: watchdog@4066a8 {
>                         clocks = <&upg_clk>;
>                         compatible = "brcm,bcm7038-wdt";
>                         reg = <0x4066a8 0x14>;
>                         status = "disabled";
>                 };
> ...
>                 timers: timer@406680 {
>                         compatible = "brcm,brcmstb-timers";
>                         reg = <0x406680 0x40>;
>                 };
> 
> So there happen to be timers in the same region, and the offset
> between timer and watchdog registers is 0x28. Coincidentally, that
> just happens to be the extra offset defined in this patch for the
> bcm4908 watchdog. Really ? Sorry, this sounds very inconsistent
> and arbitrary to me.

To Rafal's defense, we could have defined the bcm7038-wdt binding such
that the watchdog would have been at 0x28 from the beginning of the
timer block, but as I wrote earlier, that same watchdog which is really
just 8 bytes worth of register is sometimes instantiated on its own
without the rest of the timer block. This is not visible in a DTS that
is upstream but it does happen in some of the Cable Modem chips. That
was the main motivation for defining the binding the way it was, such
that we could just map those 8 bytes wherever they are.

> 
> Overall, I suspect I'll have to see datasheets if we really end up
> having different offsets for each chip, because I'll want to confirm
> that the watchdog subsystem isn't treated differently than other
> subsystems, and that the offset calculations are appropriate and
> consistent across the different chips.

Datasheets are not public however sharing the structures documenting the
register layout is something that is possible. For consistency, if we do
let 4908 define the watchdog to include that 0x28 offset, then we are
not mapping just the watchdog, but the entire timer block, which then
raises the question of what happens to the timer interrupt enable/status
and timer registers, how do we end-up sub-dividing that register space
in a logical manner.
-- 
Florian

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Guenter Roeck]

On 10/29/21 10:53 AM, Florian Fainelli wrote:
> On 10/29/21 10:43 AM, Guenter Roeck wrote:
>> On 10/29/21 9:45 AM, Florian Fainelli wrote:
>>> On 10/29/21 6:03 AM, Rob Herring wrote:
>>>> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>>>>> [Rob: please kindly comment on this]
>>>>>
>>>>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>>
>>>>>>> Hardware supported by this driver goes back to the old bcm63xx
>>>>>>> days. It
>>>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>>>
>>>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>>>> layout.
>>>>>>>
>>>>>>> Later on BCM63xx SoCs support should be added too (probably as
>>>>>>> platform
>>>>>>> devices due to missing DT). Eventually this driver should replace
>>>>>>> bcm63xx_wdt.c.
>>>>>>>
>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>>> ---
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> +
>>>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>>>>>> +    [BCM63XX_WDT_REG_CTL]        = 0x2c,
>>>>>>> +    [BCM63XX_WDT_REG_SOFTRESET]    = 0x34,
>>>>>>
>>>>>> I don't understand what you are doing here and why you are not
>>>>>> offsetting the "reg" property appropriately when you create your
>>>>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>>>>> existing driver becomes usable as-is. This does not make any sense
>>>>>> to me
>>>>>> when it is obviously the simplest way to make the driver "accept" the
>>>>>> resource being passed.
>>>>>
>>>>> I believe that DT binding should cover the whole hardware block and
>>>>> describe it (here: use proper compatible to allow recognizing block
>>>>> variant).
>>>>>
>>>>> That's because (as far as I understand) DT should be used to describe
>>>>> hardware as closely as possible. I think it shouldn't be adjusted to
>>>>> make mapping match Linux's driver implementation.
>>>>>
>>>>>
>>>>> So if:
>>>>> 1. Hardware block is mapped at 0xff800400
>>>>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>>>>
>>>>> I think mapping should use:
>>>>> reg = <0xff800400 0x3c>;
>>>>> even if we don't use the first N registers.
>>>>>
>>>>> That way, at some point, you can extend Linux (or whatever) driver to
>>>>> use extra registers without reworking the whole binding. That's why I
>>>>> think we need to map whole hardware block & handle different registers
>>>>> layouts in a driver.
>>>>
>>>> Yes, that's the correct thing to do.
>>>
>>> So in the future if we happen to want to manage the hardware timers in
>>> that block, they would be part of the watchdog driver? I am fairly sure
>>> they won't be, so you will be creating a composite driver/MFD to
>>> separate out the functions, more likely. So you might as well create
>>> sub-nodes.
>>>
>>>>
>>>> The question is whether you'd need sub nodes for the other functions.
>>>> Folks tend to want to have sub nodes for convenience which isn't really
>>>> needed and then requires a DT update ('cause they add nodes as adding
>>>> drivers).
>>>
>>> Sorry but not, this is getting completely ridiculous, the
>>>
>>>>
>>>> Based on the registers, you really don't need sub nodes here.
>>>
>>> I sort of disagree here, the watchdog is a part of a sundry timer block
>>> here, but it is logically broken up into different parts and if if I
>>> were to imagine how this would map into different drivers, I can easily
>>> see that we would have:
>>>
>>> - a driver to manage the timer interrupt controller
>>> - a driver to manage each of the 3 hardware timers, be they clockevent
>>> or else
>>> - a driver to manage the watchdog
>>>
>>> The simplest way to get there, and also because these same timer blocks
>>> are actually spread in other parts of STB chips just like the watchdog
>>> is, but in a different layout where they stand on their own was the main
>>> drive for defining the bcm7038_wdt binding the way it was.
>>>
>>> Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
>>> driver and this is better than the previous patch set, but I really do
>>> not see why having the watchdog driver not manage the *exact* subset of
>>> the register space needed (starting at 0x28) is not being done.
>>
>> Agreed, especially since other sub-devices of bcm4908 are alredy modeled
>> this way. See arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi.
>> At this point, before accepting anything, I'll want to have an explanation
>> how and why the watchdog interface is handled differently than, say,
>> its reset controller. Also, I'd like to understand the memory region
>> assigned to bcm7038, which happens to be something like:
>>
>>      compatible = "brcm,bcm7038-wdt";
>>          reg = <0xf040a7e8 0x16>;
>>
>> because it seems unlikely that this is a chip subsystem that just happens
>> to start at such an odd boundary. More specifically, I see in actual
>> .dtsi files data such as:
>>
>>                  watchdog: watchdog@4066a8 {
>>                          clocks = <&upg_clk>;
>>                          compatible = "brcm,bcm7038-wdt";
>>                          reg = <0x4066a8 0x14>;
>>                          status = "disabled";
>>                  };
>> ...
>>                  timers: timer@406680 {
>>                          compatible = "brcm,brcmstb-timers";
>>                          reg = <0x406680 0x40>;
>>                  };
>>
>> So there happen to be timers in the same region, and the offset
>> between timer and watchdog registers is 0x28. Coincidentally, that
>> just happens to be the extra offset defined in this patch for the
>> bcm4908 watchdog. Really ? Sorry, this sounds very inconsistent
>> and arbitrary to me.
> 
> To Rafal's defense, we could have defined the bcm7038-wdt binding such
> that the watchdog would have been at 0x28 from the beginning of the
> timer block, but as I wrote earlier, that same watchdog which is really
> just 8 bytes worth of register is sometimes instantiated on its own
> without the rest of the timer block. This is not visible in a DTS that
> is upstream but it does happen in some of the Cable Modem chips. That
> was the main motivation for defining the binding the way it was, such
> that we could just map those 8 bytes wherever they are.
> 
>>
>> Overall, I suspect I'll have to see datasheets if we really end up
>> having different offsets for each chip, because I'll want to confirm
>> that the watchdog subsystem isn't treated differently than other
>> subsystems, and that the offset calculations are appropriate and
>> consistent across the different chips.
> 
> Datasheets are not public however sharing the structures documenting the
> register layout is something that is possible. For consistency, if we do
> let 4908 define the watchdog to include that 0x28 offset, then we are
> not mapping just the watchdog, but the entire timer block, which then
> raises the question of what happens to the timer interrupt enable/status
> and timer registers, how do we end-up sub-dividing that register space
> in a logical manner.
> 

I can only see me accepting that if it is consistent across chips.
I am perfectly fine with using that offset and including the timer region,
but then you'd have to change the code and devicetree files for existing
chips as well to be consistent across chips. I can not see that happening.

To me it seems that the rules for assigning memory regions in devicetree
files are being changed, and the brunt of those changes is pushed into
driver code, making it messy (plus, it would create messy devicetree files,
some including the offset and some not). I do not think this is a good idea,
and I strongly object to it. I don't mind if rules are changed, but please
do it for new chips and bindings, chips which require new drivers, not
when adding devices to existing bindings.

Guenter

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Florian Fainelli]

On 10/29/21 11:10 AM, Guenter Roeck wrote:
> On 10/29/21 10:53 AM, Florian Fainelli wrote:
>> On 10/29/21 10:43 AM, Guenter Roeck wrote:
>>> On 10/29/21 9:45 AM, Florian Fainelli wrote:
>>>> On 10/29/21 6:03 AM, Rob Herring wrote:
>>>>> On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
>>>>>> [Rob: please kindly comment on this]
>>>>>>
>>>>>> On 28.10.2021 18:29, Florian Fainelli wrote:
>>>>>>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
>>>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>>>
>>>>>>>> Hardware supported by this driver goes back to the old bcm63xx
>>>>>>>> days. It
>>>>>>>> was then reused in BCM7038 and later also in BCM4908.
>>>>>>>>
>>>>>>>> Depending on SoC model registers layout differs a bit. This commit
>>>>>>>> introduces support for per-chipset registers offsets & adds BCM4908
>>>>>>>> layout.
>>>>>>>>
>>>>>>>> Later on BCM63xx SoCs support should be added too (probably as
>>>>>>>> platform
>>>>>>>> devices due to missing DT). Eventually this driver should replace
>>>>>>>> bcm63xx_wdt.c.
>>>>>>>>
>>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>>>> ---
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>> +
>>>>>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>>>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>>>>>>> +    [BCM63XX_WDT_REG_CTL]        = 0x2c,
>>>>>>>> +    [BCM63XX_WDT_REG_SOFTRESET]    = 0x34,
>>>>>>>
>>>>>>> I don't understand what you are doing here and why you are not
>>>>>>> offsetting the "reg" property appropriately when you create your
>>>>>>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
>>>>>>> existing driver becomes usable as-is. This does not make any sense
>>>>>>> to me
>>>>>>> when it is obviously the simplest way to make the driver "accept"
>>>>>>> the
>>>>>>> resource being passed.
>>>>>>
>>>>>> I believe that DT binding should cover the whole hardware block and
>>>>>> describe it (here: use proper compatible to allow recognizing block
>>>>>> variant).
>>>>>>
>>>>>> That's because (as far as I understand) DT should be used to describe
>>>>>> hardware as closely as possible. I think it shouldn't be adjusted to
>>>>>> make mapping match Linux's driver implementation.
>>>>>>
>>>>>>
>>>>>> So if:
>>>>>> 1. Hardware block is mapped at 0xff800400
>>>>>> 2. It has interesting registers at 0xff800428 and 0xff80042c
>>>>>>
>>>>>> I think mapping should use:
>>>>>> reg = <0xff800400 0x3c>;
>>>>>> even if we don't use the first N registers.
>>>>>>
>>>>>> That way, at some point, you can extend Linux (or whatever) driver to
>>>>>> use extra registers without reworking the whole binding. That's why I
>>>>>> think we need to map whole hardware block & handle different
>>>>>> registers
>>>>>> layouts in a driver.
>>>>>
>>>>> Yes, that's the correct thing to do.
>>>>
>>>> So in the future if we happen to want to manage the hardware timers in
>>>> that block, they would be part of the watchdog driver? I am fairly sure
>>>> they won't be, so you will be creating a composite driver/MFD to
>>>> separate out the functions, more likely. So you might as well create
>>>> sub-nodes.
>>>>
>>>>>
>>>>> The question is whether you'd need sub nodes for the other functions.
>>>>> Folks tend to want to have sub nodes for convenience which isn't
>>>>> really
>>>>> needed and then requires a DT update ('cause they add nodes as adding
>>>>> drivers).
>>>>
>>>> Sorry but not, this is getting completely ridiculous, the
>>>>
>>>>>
>>>>> Based on the registers, you really don't need sub nodes here.
>>>>
>>>> I sort of disagree here, the watchdog is a part of a sundry timer block
>>>> here, but it is logically broken up into different parts and if if I
>>>> were to imagine how this would map into different drivers, I can easily
>>>> see that we would have:
>>>>
>>>> - a driver to manage the timer interrupt controller
>>>> - a driver to manage each of the 3 hardware timers, be they clockevent
>>>> or else
>>>> - a driver to manage the watchdog
>>>>
>>>> The simplest way to get there, and also because these same timer blocks
>>>> are actually spread in other parts of STB chips just like the watchdog
>>>> is, but in a different layout where they stand on their own was the
>>>> main
>>>> drive for defining the bcm7038_wdt binding the way it was.
>>>>
>>>> Rafal, I appreciate that you are trying to leverage the bcm7038_wdt
>>>> driver and this is better than the previous patch set, but I really do
>>>> not see why having the watchdog driver not manage the *exact* subset of
>>>> the register space needed (starting at 0x28) is not being done.
>>>
>>> Agreed, especially since other sub-devices of bcm4908 are alredy modeled
>>> this way. See arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi.
>>> At this point, before accepting anything, I'll want to have an
>>> explanation
>>> how and why the watchdog interface is handled differently than, say,
>>> its reset controller. Also, I'd like to understand the memory region
>>> assigned to bcm7038, which happens to be something like:
>>>
>>>      compatible = "brcm,bcm7038-wdt";
>>>          reg = <0xf040a7e8 0x16>;
>>>
>>> because it seems unlikely that this is a chip subsystem that just
>>> happens
>>> to start at such an odd boundary. More specifically, I see in actual
>>> .dtsi files data such as:
>>>
>>>                  watchdog: watchdog@4066a8 {
>>>                          clocks = <&upg_clk>;
>>>                          compatible = "brcm,bcm7038-wdt";
>>>                          reg = <0x4066a8 0x14>;
>>>                          status = "disabled";
>>>                  };
>>> ...
>>>                  timers: timer@406680 {
>>>                          compatible = "brcm,brcmstb-timers";
>>>                          reg = <0x406680 0x40>;
>>>                  };
>>>
>>> So there happen to be timers in the same region, and the offset
>>> between timer and watchdog registers is 0x28. Coincidentally, that
>>> just happens to be the extra offset defined in this patch for the
>>> bcm4908 watchdog. Really ? Sorry, this sounds very inconsistent
>>> and arbitrary to me.
>>
>> To Rafal's defense, we could have defined the bcm7038-wdt binding such
>> that the watchdog would have been at 0x28 from the beginning of the
>> timer block, but as I wrote earlier, that same watchdog which is really
>> just 8 bytes worth of register is sometimes instantiated on its own
>> without the rest of the timer block. This is not visible in a DTS that
>> is upstream but it does happen in some of the Cable Modem chips. That
>> was the main motivation for defining the binding the way it was, such
>> that we could just map those 8 bytes wherever they are.
>>
>>>
>>> Overall, I suspect I'll have to see datasheets if we really end up
>>> having different offsets for each chip, because I'll want to confirm
>>> that the watchdog subsystem isn't treated differently than other
>>> subsystems, and that the offset calculations are appropriate and
>>> consistent across the different chips.
>>
>> Datasheets are not public however sharing the structures documenting the
>> register layout is something that is possible. For consistency, if we do
>> let 4908 define the watchdog to include that 0x28 offset, then we are
>> not mapping just the watchdog, but the entire timer block, which then
>> raises the question of what happens to the timer interrupt enable/status
>> and timer registers, how do we end-up sub-dividing that register space
>> in a logical manner.
>>
> 
> I can only see me accepting that if it is consistent across chips.
> I am perfectly fine with using that offset and including the timer region,
> but then you'd have to change the code and devicetree files for existing
> chips as well to be consistent across chips. I can not see that happening.
> 
> To me it seems that the rules for assigning memory regions in devicetree
> files are being changed, and the brunt of those changes is pushed into
> driver code, making it messy (plus, it would create messy devicetree files,
> some including the offset and some not). I do not think this is a good
> idea,
> and I strongly object to it. I don't mind if rules are changed, but please
> do it for new chips and bindings, chips which require new drivers, not
> when adding devices to existing bindings.

Then I believe we are in agreement, the binding is defined the way it is
for the bcm7038-wdt such that the "reg" property must be taking the 0x28
offset from the beginning of the timer block into account. Therefore
4908 must conform and do the same thing for consistency.

If this is not a good practice, then I will keep that in mind and we
will try not to repeat that bad pattern.
-- 
Florian

Re: [PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rob Herring]

On Fri, Oct 29, 2021 at 11:45 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 10/29/21 6:03 AM, Rob Herring wrote:
> > On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
> >> [Rob: please kindly comment on this]
> >>
> >> On 28.10.2021 18:29, Florian Fainelli wrote:
> >>> On 10/28/21 2:30 AM, Rafał Miłecki wrote:
> >>>> From: Rafał Miłecki <rafal@milecki.pl>
> >>>>
> >>>> Hardware supported by this driver goes back to the old bcm63xx days. It
> >>>> was then reused in BCM7038 and later also in BCM4908.
> >>>>
> >>>> Depending on SoC model registers layout differs a bit. This commit
> >>>> introduces support for per-chipset registers offsets & adds BCM4908
> >>>> layout.
> >>>>
> >>>> Later on BCM63xx SoCs support should be added too (probably as platform
> >>>> devices due to missing DT). Eventually this driver should replace
> >>>> bcm63xx_wdt.c.
> >>>>
> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >>>> ---
> >>>
> >>> [snip]
> >>>
> >>>> +
> >>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
> >>>> +  [BCM63XX_WDT_REG_DEFVAL]        = 0x28,
> >>>> +  [BCM63XX_WDT_REG_CTL]           = 0x2c,
> >>>> +  [BCM63XX_WDT_REG_SOFTRESET]     = 0x34,
> >>>
> >>> I don't understand what you are doing here and why you are not
> >>> offsetting the "reg" property appropriately when you create your
> >>> bcm4908-wdt Device Tree node such that the base starts at 0, and the
> >>> existing driver becomes usable as-is. This does not make any sense to me
> >>> when it is obviously the simplest way to make the driver "accept" the
> >>> resource being passed.
> >>
> >> I believe that DT binding should cover the whole hardware block and
> >> describe it (here: use proper compatible to allow recognizing block
> >> variant).
> >>
> >> That's because (as far as I understand) DT should be used to describe
> >> hardware as closely as possible. I think it shouldn't be adjusted to
> >> make mapping match Linux's driver implementation.
> >>
> >>
> >> So if:
> >> 1. Hardware block is mapped at 0xff800400
> >> 2. It has interesting registers at 0xff800428 and 0xff80042c
> >>
> >> I think mapping should use:
> >> reg = <0xff800400 0x3c>;
> >> even if we don't use the first N registers.
> >>
> >> That way, at some point, you can extend Linux (or whatever) driver to
> >> use extra registers without reworking the whole binding. That's why I
> >> think we need to map whole hardware block & handle different registers
> >> layouts in a driver.
> >
> > Yes, that's the correct thing to do.
>
> So in the future if we happen to want to manage the hardware timers in
> that block, they would be part of the watchdog driver? I am fairly sure
> they won't be, so you will be creating a composite driver/MFD to
> separate out the functions, more likely. So you might as well create
> sub-nodes.

There is no requirement that an MFD have child nodes. They are done
both ways. If you need some internal kernel restructuring, then I
don't care (as DT maintainer).

We very commonly have a single node that's both clock and reset
provider for example. It's primarily when the sub blocks consume
different DT resources that you need sub-nodes.

> > The question is whether you'd need sub nodes for the other functions.
> > Folks tend to want to have sub nodes for convenience which isn't really
> > needed and then requires a DT update ('cause they add nodes as adding
> > drivers).
>
> Sorry but not, this is getting completely ridiculous, the

Huh?

>
> >
> > Based on the registers, you really don't need sub nodes here.
>
> I sort of disagree here, the watchdog is a part of a sundry timer block
> here, but it is logically broken up into different parts and if if I
> were to imagine how this would map into different drivers, I can easily
> see that we would have:
>
> - a driver to manage the timer interrupt controller
> - a driver to manage each of the 3 hardware timers, be they clockevent
> or else
> - a driver to manage the watchdog

You know the h/w better than me. I was giving my opinion based only on
the limited information presented.

> The simplest way to get there, and also because these same timer blocks
> are actually spread in other parts of STB chips just like the watchdog
> is, but in a different layout where they stand on their own was the main
> drive for defining the bcm7038_wdt binding the way it was.

A sub-block reused in different blocks is a decent reason for sub-nodes.

Most important for me is that the binding be complete and not have to
change in an incompatible way in the future. The more detailed you
make the binding, the harder it will be to get right. It's the same
reason we moved away from doing a clock per node for clock trees. So,
if you want child nodes, then you need to define all of them.

Rob

Re: [PATCH 1/3] dt-bindings: watchdog: convert Broadcom's WDT to the json-schema

[Florian Fainelli]

On 10/28/21 2:30 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This helps validating DTS files. While at it also rename it to bcm63xx.
> It's the first SoC with that hardware block and it's a naming schema
> used for other Broadcom blocks. It's common for some (originally)
> bcm63xx blocks to be reused in newer SoCs (like 7038 or 4908).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

I would prefer you do not rename the binding especially as I am about to
remove the bcm63xx_wdt.c driver, and so having a binding name that sort
of matches the driver names makes it a lot easier to keep all of your
eggs in the same basket. We can argue whether 63xx came before 7xxx in
terms of time line, but that still does not justify the rename IMHO.

I realized that we are missing a 7038 pattern in the MAINTAINERS file
for the BCM7XXX entry, and that will be fixed shortly after sending this
email.

Please add Justin Chen (original author) and myself as maintainers for
this binding.
-- 
Florian