[PATCH v4 0/5] watchdog: mt7621-wdt: avoid globals and arch dependencies

[PATCH v4 0/5] watchdog: mt7621-wdt: avoid globals and arch dependencies

[Sergio Paracuellos]

Hi all,

This series make an update in the MT7621 SoC's watchdog driver code. This
SoC already provides a system controller node to access reset status
register needed for the watchdog. Instead of using MIPS architecture
dependent operations in header 'asm/mach-ralink/ralink_regs.h' use
a phandle to the system controller node and use it through regmap APIs
from the code. Driver is also using some globals that are not needed at
all if a driver data structure is used along the code. Hence, add all
new needed stuff inside a new driver data structure. With this changes
driver can be properly compile tested.

Thanks in advance for reviewing this!

v1 of this series here [0].
v2 os thise series here [1].
v3 os thise series here [2].

Cahnges in v4:
    - Add a patch to fix a watchdog node warning with 'make dtbs_check'
      because of a wrong node name.
    - Collect Guenter 'Reviewed-by' tags for watchdog driver code.
    - Add a missing 'COMPILE_TEST' to Kconfig which was lost when driver
      code was split in two patches in v2.

Changes in v3:
    - rename phandler from 'ralink,sysctl' into 'mediatek,sysctl'.
    - Drop error message added in PATCH 3 that modifies functionality
      and we only want to maintain current functionaloty by now.

Changes in v2:
    - Remove no needed compatible 'syscon' from bindings.
    - Rewrite new syscon phandle description in bindings.
    - Remove 'syscon' from compatible in 'mt7621.dtsi'.
    - Split PATCH 3 into two different patches:
        - PATCH 3: removes static globals using a driver data structure.
        - PATCH 4: remove ralink architecture dependent includes and code.

Best regards,
    Sergio Paracuellos

[0]: https://lore.kernel.org/linux-watchdog/20230210065621.598120-1-sergio.paracuellos@gmail.com/T/#t
[1]: https://lore.kernel.org/linux-watchdog/20230210121735.639089-1-sergio.paracuellos@gmail.com/T/#t
[2]: https://lore.kernel.org/linux-watchdog/20230210173841.705783-1-sergio.paracuellos@gmail.com/T/#t

Sergio Paracuellos (5):
  dt-bindings: watchdog: mt7621-wdt: add phandle to access system
    controller registers
  mips: dts: ralink: mt7621: add phandle to system controller node for
    watchdog
  mips: dts: ralink: mt7621: rename watchdog node from 'wdt' into
    'watchdog'
  watchdog: mt7621-wdt: avoid static global declarations
  watchdog: mt7621-wdt: avoid ralink architecture dependent code

 .../watchdog/mediatek,mt7621-wdt.yaml         |   7 ++
 arch/mips/boot/dts/ralink/mt7621.dtsi         |   3 +-
 drivers/watchdog/Kconfig                      |   4 +-
 drivers/watchdog/mt7621_wdt.c                 | 119 ++++++++++++------
 4 files changed, 90 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

MT7621 SoC provides a system controller node for accessing to some registers.
Add a phandle in this node to avoid using MIPS related arch operations and
includes in watchdog driver code.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml  | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
index b2b17fdf4..a668d0c2f 100644
--- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
@@ -19,6 +19,12 @@ properties:
   reg:
     maxItems: 1
 
+  mediatek,sysctl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to system controller 'sysc' syscon node which
+      controls system registers
+
 required:
   - compatible
   - reg
@@ -30,4 +36,5 @@ examples:
     watchdog@100 {
       compatible = "mediatek,mt7621-wdt";
       reg = <0x100 0x100>;
+      mediatek,sysctl = <&sysc>;
     };
-- 
2.25.1

[PATCH v4 2/5] mips: dts: ralink: mt7621: add phandle to system controller node for watchdog

[Sergio Paracuellos]

To allow to access system controller registers from watchdog driver code
add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
operations in driver code.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/boot/dts/ralink/mt7621.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 5ca40fd21..764916eaf 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -73,6 +73,7 @@ sysc: syscon@0 {
 		wdt: wdt@100 {
 			compatible = "mediatek,mt7621-wdt";
 			reg = <0x100 0x100>;
+			mediatek,sysctl = <&sysc>;
 		};
 
 		gpio: gpio@600 {
-- 
2.25.1

[PATCH v4 3/5] mips: dts: ralink: mt7621: rename watchdog node from 'wdt' into 'watchdog'

[Sergio Paracuellos]

Watchdog nodes must use 'watchdog' for node name. When a 'make dtbs_check'
is performed the following warning appears:

wdt@100: $nodename:0: 'wdt@100' does not match '^watchdog(@.*|-[0-9a-f])?$'

Fix this warning up properly renaming the node into 'watchdog'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/boot/dts/ralink/mt7621.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 764916eaf..3d16beb77 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -70,7 +70,7 @@ sysc: syscon@0 {
 					     "250m", "270m";
 		};
 
-		wdt: wdt@100 {
+		wdt: watchdog@100 {
 			compatible = "mediatek,mt7621-wdt";
 			reg = <0x100 0x100>;
 			mediatek,sysctl = <&sysc>;
-- 
2.25.1

[PATCH v4 4/5] watchdog: mt7621-wdt: avoid static global declarations

[Sergio Paracuellos]

Instead of using static global definitions in driver code, refactor code
introducing a new watchdog driver data structure and use it along the
code.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/watchdog/mt7621_wdt.c | 102 ++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 37 deletions(-)

diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
index a8aa3522c..40fb2c9ba 100644
--- a/drivers/watchdog/mt7621_wdt.c
+++ b/drivers/watchdog/mt7621_wdt.c
@@ -31,8 +31,11 @@
 #define TMR1CTL_RESTART			BIT(9)
 #define TMR1CTL_PRESCALE_SHIFT		16
 
-static void __iomem *mt7621_wdt_base;
-static struct reset_control *mt7621_wdt_reset;
+struct mt7621_wdt_data {
+	void __iomem *base;
+	struct reset_control *rst;
+	struct watchdog_device wdt;
+};
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -40,27 +43,31 @@ MODULE_PARM_DESC(nowayout,
 		 "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static inline void rt_wdt_w32(unsigned reg, u32 val)
+static inline void rt_wdt_w32(void __iomem *base, unsigned reg, u32 val)
 {
-	iowrite32(val, mt7621_wdt_base + reg);
+	iowrite32(val, base + reg);
 }
 
-static inline u32 rt_wdt_r32(unsigned reg)
+static inline u32 rt_wdt_r32(void __iomem *base, unsigned reg)
 {
-	return ioread32(mt7621_wdt_base + reg);
+	return ioread32(base + reg);
 }
 
 static int mt7621_wdt_ping(struct watchdog_device *w)
 {
-	rt_wdt_w32(TIMER_REG_TMRSTAT, TMR1CTL_RESTART);
+	struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
+	rt_wdt_w32(drvdata->base, TIMER_REG_TMRSTAT, TMR1CTL_RESTART);
 
 	return 0;
 }
 
 static int mt7621_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
 {
+	struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
 	w->timeout = t;
-	rt_wdt_w32(TIMER_REG_TMR1LOAD, t * 1000);
+	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1LOAD, t * 1000);
 	mt7621_wdt_ping(w);
 
 	return 0;
@@ -68,29 +75,31 @@ static int mt7621_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
 
 static int mt7621_wdt_start(struct watchdog_device *w)
 {
+	struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
 	u32 t;
 
 	/* set the prescaler to 1ms == 1000us */
-	rt_wdt_w32(TIMER_REG_TMR1CTL, 1000 << TMR1CTL_PRESCALE_SHIFT);
+	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, 1000 << TMR1CTL_PRESCALE_SHIFT);
 
 	mt7621_wdt_set_timeout(w, w->timeout);
 
-	t = rt_wdt_r32(TIMER_REG_TMR1CTL);
+	t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
 	t |= TMR1CTL_ENABLE;
-	rt_wdt_w32(TIMER_REG_TMR1CTL, t);
+	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
 
 	return 0;
 }
 
 static int mt7621_wdt_stop(struct watchdog_device *w)
 {
+	struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
 	u32 t;
 
 	mt7621_wdt_ping(w);
 
-	t = rt_wdt_r32(TIMER_REG_TMR1CTL);
+	t = rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL);
 	t &= ~TMR1CTL_ENABLE;
-	rt_wdt_w32(TIMER_REG_TMR1CTL, t);
+	rt_wdt_w32(drvdata->base, TIMER_REG_TMR1CTL, t);
 
 	return 0;
 }
@@ -105,7 +114,9 @@ static int mt7621_wdt_bootcause(void)
 
 static int mt7621_wdt_is_running(struct watchdog_device *w)
 {
-	return !!(rt_wdt_r32(TIMER_REG_TMR1CTL) & TMR1CTL_ENABLE);
+	struct mt7621_wdt_data *drvdata = watchdog_get_drvdata(w);
+
+	return !!(rt_wdt_r32(drvdata->base, TIMER_REG_TMR1CTL) & TMR1CTL_ENABLE);
 }
 
 static const struct watchdog_info mt7621_wdt_info = {
@@ -121,30 +132,39 @@ static const struct watchdog_ops mt7621_wdt_ops = {
 	.set_timeout = mt7621_wdt_set_timeout,
 };
 
-static struct watchdog_device mt7621_wdt_dev = {
-	.info = &mt7621_wdt_info,
-	.ops = &mt7621_wdt_ops,
-	.min_timeout = 1,
-	.max_timeout = 0xfffful / 1000,
-};
-
 static int mt7621_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	mt7621_wdt_base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(mt7621_wdt_base))
-		return PTR_ERR(mt7621_wdt_base);
+	struct watchdog_device *mt7621_wdt;
+	struct mt7621_wdt_data *drvdata;
+	int err;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
 
-	mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
-	if (!IS_ERR(mt7621_wdt_reset))
-		reset_control_deassert(mt7621_wdt_reset);
+	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
 
-	mt7621_wdt_dev.bootstatus = mt7621_wdt_bootcause();
+	drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (!IS_ERR(drvdata->rst))
+		reset_control_deassert(drvdata->rst);
 
-	watchdog_init_timeout(&mt7621_wdt_dev, mt7621_wdt_dev.max_timeout,
-			      dev);
-	watchdog_set_nowayout(&mt7621_wdt_dev, nowayout);
-	if (mt7621_wdt_is_running(&mt7621_wdt_dev)) {
+	mt7621_wdt = &drvdata->wdt;
+	mt7621_wdt->info = &mt7621_wdt_info;
+	mt7621_wdt->ops = &mt7621_wdt_ops;
+	mt7621_wdt->min_timeout = 1;
+	mt7621_wdt->max_timeout = 0xfffful / 1000;
+	mt7621_wdt->parent = dev;
+
+	mt7621_wdt->bootstatus = mt7621_wdt_bootcause();
+
+	watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev);
+	watchdog_set_nowayout(mt7621_wdt, nowayout);
+	watchdog_set_drvdata(mt7621_wdt, drvdata);
+
+	if (mt7621_wdt_is_running(mt7621_wdt)) {
 		/*
 		 * Make sure to apply timeout from watchdog core, taking
 		 * the prescaler of this driver here into account (the
@@ -154,17 +174,25 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
 		 * we first disable the watchdog, set the new prescaler
 		 * and timeout, and then re-enable the watchdog.
 		 */
-		mt7621_wdt_stop(&mt7621_wdt_dev);
-		mt7621_wdt_start(&mt7621_wdt_dev);
-		set_bit(WDOG_HW_RUNNING, &mt7621_wdt_dev.status);
+		mt7621_wdt_stop(mt7621_wdt);
+		mt7621_wdt_start(mt7621_wdt);
+		set_bit(WDOG_HW_RUNNING, &mt7621_wdt->status);
 	}
 
-	return devm_watchdog_register_device(dev, &mt7621_wdt_dev);
+	err = devm_watchdog_register_device(dev, &drvdata->wdt);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, drvdata);
+
+	return 0;
 }
 
 static void mt7621_wdt_shutdown(struct platform_device *pdev)
 {
-	mt7621_wdt_stop(&mt7621_wdt_dev);
+	struct mt7621_wdt_data *drvdata = platform_get_drvdata(pdev);
+
+	mt7621_wdt_stop(&drvdata->wdt);
 }
 
 static const struct of_device_id mt7621_wdt_match[] = {
-- 
2.25.1

[PATCH v4 5/5] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Sergio Paracuellos]

MT7621 SoC has a system controller node. Watchdog need to access to reset
status register. Ralink architecture and related driver are old and from
the beggining they are using some architecture dependent operations for
accessing this shared registers through 'asm/mach-ralink/ralink_regs.h'
header file. However this is not ideal from a driver perspective which can
just access to the system controller registers in an arch independent way
using regmap syscon APIs. Update Kconfig accordingly to select new added
dependencies and allow driver to be compile tested.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/watchdog/Kconfig      |  4 +++-
 drivers/watchdog/mt7621_wdt.c | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b64bc49c7..cf752ad64 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1865,7 +1865,9 @@ config GXP_WATCHDOG
 config MT7621_WDT
 	tristate "Mediatek SoC watchdog"
 	select WATCHDOG_CORE
-	depends on SOC_MT7620 || SOC_MT7621
+	select REGMAP_MMIO
+	select MFD_SYSCON
+	depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
 	help
 	  Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer.
 
diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
index 40fb2c9ba..9ed07c187 100644
--- a/drivers/watchdog/mt7621_wdt.c
+++ b/drivers/watchdog/mt7621_wdt.c
@@ -15,8 +15,8 @@
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
 #include <linux/mod_devicetable.h>
-
-#include <asm/mach-ralink/ralink_regs.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #define SYSC_RSTSTAT			0x38
 #define WDT_RST_CAUSE			BIT(1)
@@ -34,6 +34,7 @@
 struct mt7621_wdt_data {
 	void __iomem *base;
 	struct reset_control *rst;
+	struct regmap *sysc;
 	struct watchdog_device wdt;
 };
 
@@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w)
 	return 0;
 }
 
-static int mt7621_wdt_bootcause(void)
+static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d)
 {
-	if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE)
+	u32 val;
+
+	regmap_read(d->sysc, SYSC_RSTSTAT, &val);
+	if (val & WDT_RST_CAUSE)
 		return WDIOF_CARDRESET;
 
 	return 0;
@@ -134,6 +138,7 @@ static const struct watchdog_ops mt7621_wdt_ops = {
 
 static int mt7621_wdt_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
 	struct watchdog_device *mt7621_wdt;
 	struct mt7621_wdt_data *drvdata;
@@ -143,6 +148,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
+	drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "mediatek,sysctl");
+	if (IS_ERR(drvdata->sysc))
+		return PTR_ERR(drvdata->sysc);
+
 	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(drvdata->base))
 		return PTR_ERR(drvdata->base);
@@ -158,7 +167,7 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
 	mt7621_wdt->max_timeout = 0xfffful / 1000;
 	mt7621_wdt->parent = dev;
 
-	mt7621_wdt->bootstatus = mt7621_wdt_bootcause();
+	mt7621_wdt->bootstatus = mt7621_wdt_bootcause(drvdata);
 
 	watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev);
 	watchdog_set_nowayout(mt7621_wdt, nowayout);
-- 
2.25.1

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Arınç ÜNAL]

Is this mediatek,sysctl property required after your changes on the 
watchdog code?

Arınç

On 11.02.2023 10:33, Sergio Paracuellos wrote:
> MT7621 SoC provides a system controller node for accessing to some registers.
> Add a phandle in this node to avoid using MIPS related arch operations and
> includes in watchdog driver code.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>   .../devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml  | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> index b2b17fdf4..a668d0c2f 100644
> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> @@ -19,6 +19,12 @@ properties:
>     reg:
>       maxItems: 1
>   
> +  mediatek,sysctl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to system controller 'sysc' syscon node which
> +      controls system registers
> +
>   required:
>     - compatible
>     - reg
> @@ -30,4 +36,5 @@ examples:
>       watchdog@100 {
>         compatible = "mediatek,mt7621-wdt";
>         reg = <0x100 0x100>;
> +      mediatek,sysctl = <&sysc>;
>       };

Re: [PATCH v4 2/5] mips: dts: ralink: mt7621: add phandle to system controller node for watchdog

[Arınç ÜNAL]

On 11.02.2023 10:33, Sergio Paracuellos wrote:
> To allow to access system controller registers from watchdog driver code
> add a phandle in the watchdog 'wdt' node. This avoid using arch dependent
> operations in driver code.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Reviewed-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Thanks.
Arınç

Re: [PATCH v4 3/5] mips: dts: ralink: mt7621: rename watchdog node from 'wdt' into 'watchdog'

[Arınç ÜNAL]

On 11.02.2023 10:33, Sergio Paracuellos wrote:
> Watchdog nodes must use 'watchdog' for node name. When a 'make dtbs_check'
> is performed the following warning appears:
> 
> wdt@100: $nodename:0: 'wdt@100' does not match '^watchdog(@.*|-[0-9a-f])?$'
> 
> Fix this warning up properly renaming the node into 'watchdog'.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Reviewed-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Thanks.
Arınç

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> Is this mediatek,sysctl property required after your changes on the
> watchdog code?

I don't really understand the question :-) Yes, it is. Since we have
introduced a new phandle in the watchdog node to be able to access the
reset status register through the 'sysc' syscon node.
We need the bindings to be aligned with the mt7621.dtsi file and we
are getting the syscon regmap handler via
'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.

Thanks,
    Sergio Paracuellos
>
> Arınç
>
> On 11.02.2023 10:33, Sergio Paracuellos wrote:
> > MT7621 SoC provides a system controller node for accessing to some registers.
> > Add a phandle in this node to avoid using MIPS related arch operations and
> > includes in watchdog driver code.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >   .../devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml  | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > index b2b17fdf4..a668d0c2f 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml
> > @@ -19,6 +19,12 @@ properties:
> >     reg:
> >       maxItems: 1
> >
> > +  mediatek,sysctl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle to system controller 'sysc' syscon node which
> > +      controls system registers
> > +
> >   required:
> >     - compatible
> >     - reg
> > @@ -30,4 +36,5 @@ examples:
> >       watchdog@100 {
> >         compatible = "mediatek,mt7621-wdt";
> >         reg = <0x100 0x100>;
> > +      mediatek,sysctl = <&sysc>;
> >       };

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Arınç ÜNAL]

On 11.02.2023 13:41, Sergio Paracuellos wrote:
> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> Is this mediatek,sysctl property required after your changes on the
>> watchdog code?
> 
> I don't really understand the question :-) Yes, it is. Since we have
> introduced a new phandle in the watchdog node to be able to access the
> reset status register through the 'sysc' syscon node.
> We need the bindings to be aligned with the mt7621.dtsi file and we
> are getting the syscon regmap handler via
> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.

I believe you need to put mediatek,sysctl under "required:".

Arınç

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> Is this mediatek,sysctl property required after your changes on the
> >> watchdog code?
> >
> > I don't really understand the question :-) Yes, it is. Since we have
> > introduced a new phandle in the watchdog node to be able to access the
> > reset status register through the 'sysc' syscon node.
> > We need the bindings to be aligned with the mt7621.dtsi file and we
> > are getting the syscon regmap handler via
> > 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>
> I believe you need to put mediatek,sysctl under "required:".

Ah, I understood your question now :-). You meant 'required' property.
I need more coffee, I guess :-). I am not sure if you can add
properties as required after bindings are already mainlined for
compatibility issues. The problem with this SoC is that drivers become
mainlined before the device tree was so if things are properly fixed
now this kind of issues appear.  Let's see Krzysztof and Rob comments
for this.

Thanks,
    Sergio Paracuellos
>
> Arınç

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Krzysztof Kozlowski]

On 11/02/2023 12:01, Sergio Paracuellos wrote:
> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> Is this mediatek,sysctl property required after your changes on the
>>>> watchdog code?
>>>
>>> I don't really understand the question :-) Yes, it is. Since we have
>>> introduced a new phandle in the watchdog node to be able to access the
>>> reset status register through the 'sysc' syscon node.
>>> We need the bindings to be aligned with the mt7621.dtsi file and we
>>> are getting the syscon regmap handler via
>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>>
>> I believe you need to put mediatek,sysctl under "required:".
> 
> Ah, I understood your question now :-). You meant 'required' property.
> I need more coffee, I guess :-). I am not sure if you can add
> properties as required after bindings are already mainlined for
> compatibility issues. The problem with this SoC is that drivers become
> mainlined before the device tree was so if things are properly fixed
> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> for this.

If your driver fails to probe without mediatek,sysctl, you already made
it required (thus broke the ABI) regardless what dt-binding is saying.
In such case you should update dt-binding to reflect reality.

Now ABI break is different case. Usually you should not break it without
valid reasons (e.g. it was never working before). Your commit msg
suggests that you only improve the code, thus ABI break is not really
justified. In such case - binding is correct, driver should be reworked
to accept DTS without the new property.

Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Krzysztof Kozlowski]

On 11/02/2023 08:33, Sergio Paracuellos wrote:
> MT7621 SoC provides a system controller node for accessing to some registers.
> Add a phandle in this node to avoid using MIPS related arch operations and
> includes in watchdog driver code.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> >>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> Is this mediatek,sysctl property required after your changes on the
> >>>> watchdog code?
> >>>
> >>> I don't really understand the question :-) Yes, it is. Since we have
> >>> introduced a new phandle in the watchdog node to be able to access the
> >>> reset status register through the 'sysc' syscon node.
> >>> We need the bindings to be aligned with the mt7621.dtsi file and we
> >>> are getting the syscon regmap handler via
> >>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> >>
> >> I believe you need to put mediatek,sysctl under "required:".
> >
> > Ah, I understood your question now :-). You meant 'required' property.
> > I need more coffee, I guess :-). I am not sure if you can add
> > properties as required after bindings are already mainlined for
> > compatibility issues. The problem with this SoC is that drivers become
> > mainlined before the device tree was so if things are properly fixed
> > now this kind of issues appear.  Let's see Krzysztof and Rob comments
> > for this.
>
> If your driver fails to probe without mediatek,sysctl, you already made
> it required (thus broke the ABI) regardless what dt-binding is saying.
> In such case you should update dt-binding to reflect reality.
>
> Now ABI break is different case. Usually you should not break it without
> valid reasons (e.g. it was never working before). Your commit msg
> suggests that you only improve the code, thus ABI break is not really
> justified. In such case - binding is correct, driver should be reworked
> to accept DTS without the new property.

Thanks for clarification, Krzysztof. Ok, so if this is the case I need
to add this property required (as Arinc was properly pointing out in
previous mail) since without it the driver is going to fail on probe
(PATCH 5 of the series). I understand the "it was never working
before" argument reason for ABI breaks. What happens if the old driver
code was not ideal and totally dependent on architecture specific
operations when this could be totally avoided and properly make arch
independent agnostic drivers? This driver was added in 2016 [0]. There
was not a device tree file in the kernel for this SoC mainlined until
2022 [1]. I also personally migrated this watchdog binding in 2022
from text to YAML and maintained it without changes [2]. When this was
mainlined not all drivers were properly reviewed and the current code
was just maintained as it is. Most users of this SoC are in the
openWRT community where the dtsi of the mainline is not used yet and
they maintain their own mt7621.dtsi files. Also, when a new version of
the openWRT selected kernel is added they also modify and align with
its mt7621.dtsi file without maintaining previous dtb's. If "make the
driver arch independent to be able to be compile tested" and this kind
of arguments are not valid at all I need to know because I have
started to review driver code for this SoC and other drivers also have
the same arch dependency that ideally should be avoided in the same
way. This at the end means to break the ABI again in the future for
those drivers / bindings. So I can just let them be as it is and not
provide any change at all and continue without being compile tested
and other beneficial features to detect future driver breakage.

Thanks in advance for clarification.

Best regards,
    Sergio Paracuellos

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/watchdog/mt7621_wdt.c?id=ab3f09fe16d158cb4f84e558c61ec5d6d601f2e0
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/mips/boot/dts/ralink/mt7621.dtsi?id=7a6ee0bbab2551d7189ce0f5e625fef4d612ebea
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml?id=9023e05b7a5809593a7ea09896eee0bbb6ae1685

>
> Best regards,
> Krzysztof
>

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Guenter Roeck]

On 2/12/23 00:13, Sergio Paracuellos wrote:
> On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/02/2023 12:01, Sergio Paracuellos wrote:
>>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
>>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>
>>>>>> Is this mediatek,sysctl property required after your changes on the
>>>>>> watchdog code?
>>>>>
>>>>> I don't really understand the question :-) Yes, it is. Since we have
>>>>> introduced a new phandle in the watchdog node to be able to access the
>>>>> reset status register through the 'sysc' syscon node.
>>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
>>>>> are getting the syscon regmap handler via
>>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>>>>
>>>> I believe you need to put mediatek,sysctl under "required:".
>>>
>>> Ah, I understood your question now :-). You meant 'required' property.
>>> I need more coffee, I guess :-). I am not sure if you can add
>>> properties as required after bindings are already mainlined for
>>> compatibility issues. The problem with this SoC is that drivers become
>>> mainlined before the device tree was so if things are properly fixed
>>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
>>> for this.
>>
>> If your driver fails to probe without mediatek,sysctl, you already made
>> it required (thus broke the ABI) regardless what dt-binding is saying.
>> In such case you should update dt-binding to reflect reality.
>>
>> Now ABI break is different case. Usually you should not break it without
>> valid reasons (e.g. it was never working before). Your commit msg
>> suggests that you only improve the code, thus ABI break is not really
>> justified. In such case - binding is correct, driver should be reworked
>> to accept DTS without the new property.
> 
> Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> to add this property required (as Arinc was properly pointing out in
> previous mail) since without it the driver is going to fail on probe
> (PATCH 5 of the series). I understand the "it was never working
> before" argument reason for ABI breaks. What happens if the old driver
> code was not ideal and totally dependent on architecture specific
> operations when this could be totally avoided and properly make arch
> independent agnostic drivers? This driver was added in 2016 [0]. There
> was not a device tree file in the kernel for this SoC mainlined until
> 2022 [1]. I also personally migrated this watchdog binding in 2022
> from text to YAML and maintained it without changes [2]. When this was
> mainlined not all drivers were properly reviewed and the current code
> was just maintained as it is. Most users of this SoC are in the
> openWRT community where the dtsi of the mainline is not used yet and
> they maintain their own mt7621.dtsi files. Also, when a new version of
> the openWRT selected kernel is added they also modify and align with
> its mt7621.dtsi file without maintaining previous dtb's. If "make the
> driver arch independent to be able to be compile tested" and this kind
> of arguments are not valid at all I need to know because I have
> started to review driver code for this SoC and other drivers also have
> the same arch dependency that ideally should be avoided in the same
> way. This at the end means to break the ABI again in the future for
> those drivers / bindings. So I can just let them be as it is and not
> provide any change at all and continue without being compile tested
> and other beneficial features to detect future driver breakage.
> 

Problem is that there are (presumably) shipped systems out there with
the old devicetree file. The watchdog driver would no longer instantiate
on those systems.

Guenter

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Krzysztof Kozlowski]

On 12/02/2023 09:13, Sergio Paracuellos wrote:
> On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/02/2023 12:01, Sergio Paracuellos wrote:
>>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
>>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>
>>>>>> Is this mediatek,sysctl property required after your changes on the
>>>>>> watchdog code?
>>>>>
>>>>> I don't really understand the question :-) Yes, it is. Since we have
>>>>> introduced a new phandle in the watchdog node to be able to access the
>>>>> reset status register through the 'sysc' syscon node.
>>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
>>>>> are getting the syscon regmap handler via
>>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>>>>
>>>> I believe you need to put mediatek,sysctl under "required:".
>>>
>>> Ah, I understood your question now :-). You meant 'required' property.
>>> I need more coffee, I guess :-). I am not sure if you can add
>>> properties as required after bindings are already mainlined for
>>> compatibility issues. The problem with this SoC is that drivers become
>>> mainlined before the device tree was so if things are properly fixed
>>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
>>> for this.
>>
>> If your driver fails to probe without mediatek,sysctl, you already made
>> it required (thus broke the ABI) regardless what dt-binding is saying.
>> In such case you should update dt-binding to reflect reality.
>>
>> Now ABI break is different case. Usually you should not break it without
>> valid reasons (e.g. it was never working before). Your commit msg
>> suggests that you only improve the code, thus ABI break is not really
>> justified. In such case - binding is correct, driver should be reworked
>> to accept DTS without the new property.
> 
> Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> to add this property required (as Arinc was properly pointing out in
> previous mail) since without it the driver is going to fail on probe
> (PATCH 5 of the series). I understand the "it was never working
> before" argument reason for ABI breaks. What happens if the old driver
> code was not ideal and totally dependent on architecture specific
> operations when this could be totally avoided and properly make arch
> independent agnostic drivers?

It's just an improvement and improvements should be incremental and not
break ABI.

> This driver was added in 2016 [0]. There
> was not a device tree file in the kernel for this SoC mainlined until
> 2022 [1]. 

2022 march was almost a year ago, so there were kernel releases with
this ABI.

Also, what about all out of tree DTS? What about other operating
systems, bootloaders, firmwares etc?


Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Mon, Feb 13, 2023 at 9:38 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 12/02/2023 09:13, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>>>
> >>>>>> Is this mediatek,sysctl property required after your changes on the
> >>>>>> watchdog code?
> >>>>>
> >>>>> I don't really understand the question :-) Yes, it is. Since we have
> >>>>> introduced a new phandle in the watchdog node to be able to access the
> >>>>> reset status register through the 'sysc' syscon node.
> >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
> >>>>> are getting the syscon regmap handler via
> >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> >>>>
> >>>> I believe you need to put mediatek,sysctl under "required:".
> >>>
> >>> Ah, I understood your question now :-). You meant 'required' property.
> >>> I need more coffee, I guess :-). I am not sure if you can add
> >>> properties as required after bindings are already mainlined for
> >>> compatibility issues. The problem with this SoC is that drivers become
> >>> mainlined before the device tree was so if things are properly fixed
> >>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> >>> for this.
> >>
> >> If your driver fails to probe without mediatek,sysctl, you already made
> >> it required (thus broke the ABI) regardless what dt-binding is saying.
> >> In such case you should update dt-binding to reflect reality.
> >>
> >> Now ABI break is different case. Usually you should not break it without
> >> valid reasons (e.g. it was never working before). Your commit msg
> >> suggests that you only improve the code, thus ABI break is not really
> >> justified. In such case - binding is correct, driver should be reworked
> >> to accept DTS without the new property.
> >
> > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> > to add this property required (as Arinc was properly pointing out in
> > previous mail) since without it the driver is going to fail on probe
> > (PATCH 5 of the series). I understand the "it was never working
> > before" argument reason for ABI breaks. What happens if the old driver
> > code was not ideal and totally dependent on architecture specific
> > operations when this could be totally avoided and properly make arch
> > independent agnostic drivers?
>
> It's just an improvement and improvements should be incremental and not
> break ABI.

Understood.

>
> > This driver was added in 2016 [0]. There
> > was not a device tree file in the kernel for this SoC mainlined until
> > 2022 [1].
>
> 2022 march was almost a year ago, so there were kernel releases with
> this ABI.
>
> Also, what about all out of tree DTS? What about other operating
> systems, bootloaders, firmwares etc?

Pretty clear, thanks. So I guess I have to drop all the changes that
are breaking ABI and just maintain those that make no real changes in
bindings.

>
>
> Best regards,
> Krzysztof

Thanks,
    Sergio Paracuellos
>

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/12/23 00:13, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>>>
> >>>>>> Is this mediatek,sysctl property required after your changes on the
> >>>>>> watchdog code?
> >>>>>
> >>>>> I don't really understand the question :-) Yes, it is. Since we have
> >>>>> introduced a new phandle in the watchdog node to be able to access the
> >>>>> reset status register through the 'sysc' syscon node.
> >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
> >>>>> are getting the syscon regmap handler via
> >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> >>>>
> >>>> I believe you need to put mediatek,sysctl under "required:".
> >>>
> >>> Ah, I understood your question now :-). You meant 'required' property.
> >>> I need more coffee, I guess :-). I am not sure if you can add
> >>> properties as required after bindings are already mainlined for
> >>> compatibility issues. The problem with this SoC is that drivers become
> >>> mainlined before the device tree was so if things are properly fixed
> >>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> >>> for this.
> >>
> >> If your driver fails to probe without mediatek,sysctl, you already made
> >> it required (thus broke the ABI) regardless what dt-binding is saying.
> >> In such case you should update dt-binding to reflect reality.
> >>
> >> Now ABI break is different case. Usually you should not break it without
> >> valid reasons (e.g. it was never working before). Your commit msg
> >> suggests that you only improve the code, thus ABI break is not really
> >> justified. In such case - binding is correct, driver should be reworked
> >> to accept DTS without the new property.
> >
> > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> > to add this property required (as Arinc was properly pointing out in
> > previous mail) since without it the driver is going to fail on probe
> > (PATCH 5 of the series). I understand the "it was never working
> > before" argument reason for ABI breaks. What happens if the old driver
> > code was not ideal and totally dependent on architecture specific
> > operations when this could be totally avoided and properly make arch
> > independent agnostic drivers? This driver was added in 2016 [0]. There
> > was not a device tree file in the kernel for this SoC mainlined until
> > 2022 [1]. I also personally migrated this watchdog binding in 2022
> > from text to YAML and maintained it without changes [2]. When this was
> > mainlined not all drivers were properly reviewed and the current code
> > was just maintained as it is. Most users of this SoC are in the
> > openWRT community where the dtsi of the mainline is not used yet and
> > they maintain their own mt7621.dtsi files. Also, when a new version of
> > the openWRT selected kernel is added they also modify and align with
> > its mt7621.dtsi file without maintaining previous dtb's. If "make the
> > driver arch independent to be able to be compile tested" and this kind
> > of arguments are not valid at all I need to know because I have
> > started to review driver code for this SoC and other drivers also have
> > the same arch dependency that ideally should be avoided in the same
> > way. This at the end means to break the ABI again in the future for
> > those drivers / bindings. So I can just let them be as it is and not
> > provide any change at all and continue without being compile tested
> > and other beneficial features to detect future driver breakage.
> >
>
> Problem is that there are (presumably) shipped systems out there with
> the old devicetree file. The watchdog driver would no longer instantiate
> on those systems.

Ok, I will maintain only the PATCH that changes the driver to not use
globals and send v5.

>
> Guenter
>

Thanks,
    Sergio Paracuellos

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Guenter Roeck]

On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote:
> On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 2/12/23 00:13, Sergio Paracuellos wrote:
> > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> > >>>>
> > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> > >>>>>>
> > >>>>>> Is this mediatek,sysctl property required after your changes on the
> > >>>>>> watchdog code?
> > >>>>>
> > >>>>> I don't really understand the question :-) Yes, it is. Since we have
> > >>>>> introduced a new phandle in the watchdog node to be able to access the
> > >>>>> reset status register through the 'sysc' syscon node.
> > >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
> > >>>>> are getting the syscon regmap handler via
> > >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> > >>>>
> > >>>> I believe you need to put mediatek,sysctl under "required:".
> > >>>
> > >>> Ah, I understood your question now :-). You meant 'required' property.
> > >>> I need more coffee, I guess :-). I am not sure if you can add
> > >>> properties as required after bindings are already mainlined for
> > >>> compatibility issues. The problem with this SoC is that drivers become
> > >>> mainlined before the device tree was so if things are properly fixed
> > >>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> > >>> for this.
> > >>
> > >> If your driver fails to probe without mediatek,sysctl, you already made
> > >> it required (thus broke the ABI) regardless what dt-binding is saying.
> > >> In such case you should update dt-binding to reflect reality.
> > >>
> > >> Now ABI break is different case. Usually you should not break it without
> > >> valid reasons (e.g. it was never working before). Your commit msg
> > >> suggests that you only improve the code, thus ABI break is not really
> > >> justified. In such case - binding is correct, driver should be reworked
> > >> to accept DTS without the new property.
> > >
> > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> > > to add this property required (as Arinc was properly pointing out in
> > > previous mail) since without it the driver is going to fail on probe
> > > (PATCH 5 of the series). I understand the "it was never working
> > > before" argument reason for ABI breaks. What happens if the old driver
> > > code was not ideal and totally dependent on architecture specific
> > > operations when this could be totally avoided and properly make arch
> > > independent agnostic drivers? This driver was added in 2016 [0]. There
> > > was not a device tree file in the kernel for this SoC mainlined until
> > > 2022 [1]. I also personally migrated this watchdog binding in 2022
> > > from text to YAML and maintained it without changes [2]. When this was
> > > mainlined not all drivers were properly reviewed and the current code
> > > was just maintained as it is. Most users of this SoC are in the
> > > openWRT community where the dtsi of the mainline is not used yet and
> > > they maintain their own mt7621.dtsi files. Also, when a new version of
> > > the openWRT selected kernel is added they also modify and align with
> > > its mt7621.dtsi file without maintaining previous dtb's. If "make the
> > > driver arch independent to be able to be compile tested" and this kind
> > > of arguments are not valid at all I need to know because I have
> > > started to review driver code for this SoC and other drivers also have
> > > the same arch dependency that ideally should be avoided in the same
> > > way. This at the end means to break the ABI again in the future for
> > > those drivers / bindings. So I can just let them be as it is and not
> > > provide any change at all and continue without being compile tested
> > > and other beneficial features to detect future driver breakage.
> > >
> >
> > Problem is that there are (presumably) shipped systems out there with
> > the old devicetree file. The watchdog driver would no longer instantiate
> > on those systems.
> 
> Ok, I will maintain only the PATCH that changes the driver to not use
> globals and send v5.
> 

Other options might be to search for the "syscon" node name or to search
for the "mediatek,mt7621-sysc" compatible.

Guenter

> >
> > Guenter
> >
> 
> Thanks,
>     Sergio Paracuellos

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Sergio Paracuellos]

On Mon, Feb 13, 2023 at 8:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote:
> > On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 2/12/23 00:13, Sergio Paracuellos wrote:
> > > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >>
> > > >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> > > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> > > >>>>
> > > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> > > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> > > >>>>>>
> > > >>>>>> Is this mediatek,sysctl property required after your changes on the
> > > >>>>>> watchdog code?
> > > >>>>>
> > > >>>>> I don't really understand the question :-) Yes, it is. Since we have
> > > >>>>> introduced a new phandle in the watchdog node to be able to access the
> > > >>>>> reset status register through the 'sysc' syscon node.
> > > >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
> > > >>>>> are getting the syscon regmap handler via
> > > >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> > > >>>>
> > > >>>> I believe you need to put mediatek,sysctl under "required:".
> > > >>>
> > > >>> Ah, I understood your question now :-). You meant 'required' property.
> > > >>> I need more coffee, I guess :-). I am not sure if you can add
> > > >>> properties as required after bindings are already mainlined for
> > > >>> compatibility issues. The problem with this SoC is that drivers become
> > > >>> mainlined before the device tree was so if things are properly fixed
> > > >>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> > > >>> for this.
> > > >>
> > > >> If your driver fails to probe without mediatek,sysctl, you already made
> > > >> it required (thus broke the ABI) regardless what dt-binding is saying.
> > > >> In such case you should update dt-binding to reflect reality.
> > > >>
> > > >> Now ABI break is different case. Usually you should not break it without
> > > >> valid reasons (e.g. it was never working before). Your commit msg
> > > >> suggests that you only improve the code, thus ABI break is not really
> > > >> justified. In such case - binding is correct, driver should be reworked
> > > >> to accept DTS without the new property.
> > > >
> > > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> > > > to add this property required (as Arinc was properly pointing out in
> > > > previous mail) since without it the driver is going to fail on probe
> > > > (PATCH 5 of the series). I understand the "it was never working
> > > > before" argument reason for ABI breaks. What happens if the old driver
> > > > code was not ideal and totally dependent on architecture specific
> > > > operations when this could be totally avoided and properly make arch
> > > > independent agnostic drivers? This driver was added in 2016 [0]. There
> > > > was not a device tree file in the kernel for this SoC mainlined until
> > > > 2022 [1]. I also personally migrated this watchdog binding in 2022
> > > > from text to YAML and maintained it without changes [2]. When this was
> > > > mainlined not all drivers were properly reviewed and the current code
> > > > was just maintained as it is. Most users of this SoC are in the
> > > > openWRT community where the dtsi of the mainline is not used yet and
> > > > they maintain their own mt7621.dtsi files. Also, when a new version of
> > > > the openWRT selected kernel is added they also modify and align with
> > > > its mt7621.dtsi file without maintaining previous dtb's. If "make the
> > > > driver arch independent to be able to be compile tested" and this kind
> > > > of arguments are not valid at all I need to know because I have
> > > > started to review driver code for this SoC and other drivers also have
> > > > the same arch dependency that ideally should be avoided in the same
> > > > way. This at the end means to break the ABI again in the future for
> > > > those drivers / bindings. So I can just let them be as it is and not
> > > > provide any change at all and continue without being compile tested
> > > > and other beneficial features to detect future driver breakage.
> > > >
> > >
> > > Problem is that there are (presumably) shipped systems out there with
> > > the old devicetree file. The watchdog driver would no longer instantiate
> > > on those systems.
> >
> > Ok, I will maintain only the PATCH that changes the driver to not use
> > globals and send v5.
> >
>
> Other options might be to search for the "syscon" node name or to search
> for the "mediatek,mt7621-sysc" compatible.

Thanks for the hint. I didn't know about
'syscon_regmap_lookup_by_compatible()'. I will use this to avoid DTB
ABI breakage and allow the driver to be selected for COMPILE_TEST..

Thanks, Guenter.

Best regards,
    Sergio Paracuellos

>
> Guenter
>
> > >
> > > Guenter
> > >
> >
> > Thanks,
> >     Sergio Paracuellos

Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Guenter Roeck]

On 2/13/23 11:57, Sergio Paracuellos wrote:
> On Mon, Feb 13, 2023 at 8:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote:
>>> On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 2/12/23 00:13, Sergio Paracuellos wrote:
>>>>> On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>
>>>>>> On 11/02/2023 12:01, Sergio Paracuellos wrote:
>>>>>>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>>>
>>>>>>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
>>>>>>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Is this mediatek,sysctl property required after your changes on the
>>>>>>>>>> watchdog code?
>>>>>>>>>
>>>>>>>>> I don't really understand the question :-) Yes, it is. Since we have
>>>>>>>>> introduced a new phandle in the watchdog node to be able to access the
>>>>>>>>> reset status register through the 'sysc' syscon node.
>>>>>>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
>>>>>>>>> are getting the syscon regmap handler via
>>>>>>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>>>>>>>>
>>>>>>>> I believe you need to put mediatek,sysctl under "required:".
>>>>>>>
>>>>>>> Ah, I understood your question now :-). You meant 'required' property.
>>>>>>> I need more coffee, I guess :-). I am not sure if you can add
>>>>>>> properties as required after bindings are already mainlined for
>>>>>>> compatibility issues. The problem with this SoC is that drivers become
>>>>>>> mainlined before the device tree was so if things are properly fixed
>>>>>>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
>>>>>>> for this.
>>>>>>
>>>>>> If your driver fails to probe without mediatek,sysctl, you already made
>>>>>> it required (thus broke the ABI) regardless what dt-binding is saying.
>>>>>> In such case you should update dt-binding to reflect reality.
>>>>>>
>>>>>> Now ABI break is different case. Usually you should not break it without
>>>>>> valid reasons (e.g. it was never working before). Your commit msg
>>>>>> suggests that you only improve the code, thus ABI break is not really
>>>>>> justified. In such case - binding is correct, driver should be reworked
>>>>>> to accept DTS without the new property.
>>>>>
>>>>> Thanks for clarification, Krzysztof. Ok, so if this is the case I need
>>>>> to add this property required (as Arinc was properly pointing out in
>>>>> previous mail) since without it the driver is going to fail on probe
>>>>> (PATCH 5 of the series). I understand the "it was never working
>>>>> before" argument reason for ABI breaks. What happens if the old driver
>>>>> code was not ideal and totally dependent on architecture specific
>>>>> operations when this could be totally avoided and properly make arch
>>>>> independent agnostic drivers? This driver was added in 2016 [0]. There
>>>>> was not a device tree file in the kernel for this SoC mainlined until
>>>>> 2022 [1]. I also personally migrated this watchdog binding in 2022
>>>>> from text to YAML and maintained it without changes [2]. When this was
>>>>> mainlined not all drivers were properly reviewed and the current code
>>>>> was just maintained as it is. Most users of this SoC are in the
>>>>> openWRT community where the dtsi of the mainline is not used yet and
>>>>> they maintain their own mt7621.dtsi files. Also, when a new version of
>>>>> the openWRT selected kernel is added they also modify and align with
>>>>> its mt7621.dtsi file without maintaining previous dtb's. If "make the
>>>>> driver arch independent to be able to be compile tested" and this kind
>>>>> of arguments are not valid at all I need to know because I have
>>>>> started to review driver code for this SoC and other drivers also have
>>>>> the same arch dependency that ideally should be avoided in the same
>>>>> way. This at the end means to break the ABI again in the future for
>>>>> those drivers / bindings. So I can just let them be as it is and not
>>>>> provide any change at all and continue without being compile tested
>>>>> and other beneficial features to detect future driver breakage.
>>>>>
>>>>
>>>> Problem is that there are (presumably) shipped systems out there with
>>>> the old devicetree file. The watchdog driver would no longer instantiate
>>>> on those systems.
>>>
>>> Ok, I will maintain only the PATCH that changes the driver to not use
>>> globals and send v5.
>>>
>>
>> Other options might be to search for the "syscon" node name or to search
>> for the "mediatek,mt7621-sysc" compatible.
> 
> Thanks for the hint. I didn't know about
> 'syscon_regmap_lookup_by_compatible()'. I will use this to avoid DTB
> ABI breakage and allow the driver to be selected for COMPILE_TEST..
> 

I didn't know about that one either. I thought about of_find_compatible_node()
or of_find_node_by_name(). syscon_regmap_lookup_by_compatible() is widely used,
though, so it seems to be a much better option.

Thanks,
Guenter