[PATCH v6 0/3] watchdog: mt7621-wdt: avoid globals and arch dependencies

[PATCH v6 0/3] 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' get
node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
regmap APIS. 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 of this series here [1].
v3 of this series here [2].
v4 of this series here [3].
v5 of this series here [4].

Changes in v6:
    - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
    - Add COMPILE_TEST and dependencies to Kconfig.
    - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
      warning fix.

Changes in v5:
    - Drop patches related with device tree ABI breakage and only
      maintain the rest.
    - Collect Arinc 'Reviewed-by' tag for watchdog node warning fix.

Changes 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
[3]: https://lore.kernel.org/linux-watchdog/20230211073357.755893-1-sergio.paracuellos@gmail.com/T/#t
[4]: https://lore.kernel.org/linux-watchdog/20230213120638.850612-1-sergio.paracuellos@gmail.com/T/#t

Sergio Paracuellos (2):

Sergio Paracuellos (3):
  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

 arch/mips/boot/dts/ralink/mt7621.dtsi |   2 +-
 drivers/watchdog/Kconfig              |   4 +-
 drivers/watchdog/mt7621_wdt.c         | 118 +++++++++++++++++---------
 3 files changed, 81 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH v6 1/3] 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'.

Reviewed-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 5ca40fd21..ac818fd72 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>;
 		};
-- 
2.25.1

[PATCH v6 2/3] 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 v6 3/3] 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.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/watchdog/Kconfig      |  4 +++-
 drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
 2 files changed, 16 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..22e979bdd 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;
@@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
+	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
+	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 +166,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 v6 0/3] watchdog: mt7621-wdt: avoid globals and arch dependencies

[Arınç ÜNAL]

On 13.02.2023 23:05, Sergio Paracuellos wrote:
> 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' get
> node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
> regmap APIS. 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 of this series here [1].
> v3 of this series here [2].
> v4 of this series here [3].
> v5 of this series here [4].
> 
> Changes in v6:
>      - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
>      - Add COMPILE_TEST and dependencies to Kconfig.
>      - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
>        warning fix.
> 
> Changes in v5:
>      - Drop patches related with device tree ABI breakage and only
>        maintain the rest.

This makes me regret taking mt7621-dts out of drivers/staging. Clearly 
there were improvements to be made on the MT7621 drivers that would 
affect the devicetree binding. I don't think one would bat an eye to 
make dt-binding changes on a devicetree on drivers/staging.

I also want to make a similar change that would break the ABI. I want to 
split the MediaTek MIPS (currently called Ralink) pinctrl subdrivers 
further, namely mt7620.c, to split MT7628/MT7688 pinmux data from 
MT7620, so I can properly document which function can be given to which 
group(s) for the MT7628/MT7688 SoCs.

This means I've got to introduce a new compatible string for the MT76X8 
SoCs which would cause the pinctrl driver to stop working with an old DT 
for the MT76X8 SoCs. By the nature of the change, I can't make a way 
around with the code like Sergio did in v6.

Arınç

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 13/02/2023 21:05, Sergio Paracuellos wrote:
> 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.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/watchdog/Kconfig      |  4 +++-
>  drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>  2 files changed, 16 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..22e979bdd 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;
> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>  	if (!drvdata)
>  		return -ENOMEM;
>  
> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
> +	if (IS_ERR(drvdata->sysc))
> +		return PTR_ERR(drvdata->sysc);

This should be the backup/error path for original code using syscon
property. Looking up by compatible is really not portable/re-usable.

Best regards,
Krzysztof

Re: [PATCH v6 0/3] watchdog: mt7621-wdt: avoid globals and arch dependencies

[Arınç ÜNAL]

On 14.02.2023 11:12, Arınç ÜNAL wrote:
> On 13.02.2023 23:05, Sergio Paracuellos wrote:
>> 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' get
>> node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
>> regmap APIS. 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 of this series here [1].
>> v3 of this series here [2].
>> v4 of this series here [3].
>> v5 of this series here [4].
>>
>> Changes in v6:
>>      - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
>>      - Add COMPILE_TEST and dependencies to Kconfig.
>>      - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
>>        warning fix.
>>
>> Changes in v5:
>>      - Drop patches related with device tree ABI breakage and only
>>        maintain the rest.
> 
> This makes me regret taking mt7621-dts out of drivers/staging. Clearly 
> there were improvements to be made on the MT7621 drivers that would 
> affect the devicetree binding. I don't think one would bat an eye to 
> make dt-binding changes on a devicetree on drivers/staging.
> 
> I also want to make a similar change that would break the ABI. I want to 
> split the MediaTek MIPS (currently called Ralink) pinctrl subdrivers 
> further, namely mt7620.c, to split MT7628/MT7688 pinmux data from 
> MT7620, so I can properly document which function can be given to which 
> group(s) for the MT7628/MT7688 SoCs.
> 
> This means I've got to introduce a new compatible string for the MT76X8 
> SoCs which would cause the pinctrl driver to stop working with an old DT 
> for the MT76X8 SoCs. By the nature of the change, I can't make a way 
> around with the code like Sergio did in v6.

Now that I think about it, I think there's a way I can introduce a new 
compatible string for the sake of better documentation, and keep the 
driver working with old DTs. I'd not like to convolute this series with 
this conversation. I'll send a mail to the related mailing list when the 
time is right.

Arınç

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Sergio Paracuellos]

On Tue, Feb 14, 2023 at 9:31 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/02/2023 21:05, Sergio Paracuellos wrote:
> > 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.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/watchdog/Kconfig      |  4 +++-
> >  drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
> >  2 files changed, 16 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..22e979bdd 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;
> > @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
> >       if (!drvdata)
> >               return -ENOMEM;
> >
> > +     drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
> > +     if (IS_ERR(drvdata->sysc))
> > +             return PTR_ERR(drvdata->sysc);
>
> This should be the backup/error path for original code using syscon
> property. Looking up by compatible is really not portable/re-usable.

I can change the code in the way you are pointing out here but...
Why is it not re-usable? Compatible is not supposed to be changed
since in other cases the DTB ABI will be broken. I am totally lost
about what is an ABI breakage, then.

Thanks in advance for clarification.

Best regards,
    Sergio Paracuellos

>
> Best regards,
> Krzysztof
>

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 14/02/2023 09:54, Sergio Paracuellos wrote:
>>>       return 0;
>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>       if (!drvdata)
>>>               return -ENOMEM;
>>>
>>> +     drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>> +     if (IS_ERR(drvdata->sysc))
>>> +             return PTR_ERR(drvdata->sysc);
>>
>> This should be the backup/error path for original code using syscon
>> property. Looking up by compatible is really not portable/re-usable.
> 
> I can change the code in the way you are pointing out here but...
> Why is it not re-usable? Compatible is not supposed to be changed
> since in other cases the DTB ABI will be broken. I am totally lost
> about what is an ABI breakage, then.

How do you use it on other platform?

Best regards,
Krzysztof

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Sergio Paracuellos]

On Tue, Feb 14, 2023 at 10:33 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/02/2023 09:54, Sergio Paracuellos wrote:
> >>>       return 0;
> >>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
> >>>       if (!drvdata)
> >>>               return -ENOMEM;
> >>>
> >>> +     drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
> >>> +     if (IS_ERR(drvdata->sysc))
> >>> +             return PTR_ERR(drvdata->sysc);
> >>
> >> This should be the backup/error path for original code using syscon
> >> property. Looking up by compatible is really not portable/re-usable.
> >
> > I can change the code in the way you are pointing out here but...
> > Why is it not re-usable? Compatible is not supposed to be changed
> > since in other cases the DTB ABI will be broken. I am totally lost
> > about what is an ABI breakage, then.
>
> How do you use it on other platform?

I see, thanks.

So if I am understanding you correctly I have to maintain the
'mediatek,sysctl' phandle and use it in the first instance and if it
fails I have to use regmap_loopup_by_compatible() stuff, right?
Something like:

...

drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "mediatek,sysctl");
if (IS_ERR(drvdata->sysc)) {
         drvdata->sysc =
syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
         if (IS_ERR(drvdata->sysc))
              return PTR_ERR(drvdata->sysc);
}

....

So in that case, we can add the new phandle to the bindings without
any kind of ABI breakage and we can use old dtbs using new watchdog
driver code.

I really hope I finally understood this properly. Thanks for your
patience, Krzysztof.

Best regards,
    Sergio Paracuellos

>
> Best regards,
> Krzysztof
>

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 14/02/2023 10:50, Sergio Paracuellos wrote:
> On Tue, Feb 14, 2023 at 10:33 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/02/2023 09:54, Sergio Paracuellos wrote:
>>>>>       return 0;
>>>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>>>       if (!drvdata)
>>>>>               return -ENOMEM;
>>>>>
>>>>> +     drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>>>> +     if (IS_ERR(drvdata->sysc))
>>>>> +             return PTR_ERR(drvdata->sysc);
>>>>
>>>> This should be the backup/error path for original code using syscon
>>>> property. Looking up by compatible is really not portable/re-usable.
>>>
>>> I can change the code in the way you are pointing out here but...
>>> Why is it not re-usable? Compatible is not supposed to be changed
>>> since in other cases the DTB ABI will be broken. I am totally lost
>>> about what is an ABI breakage, then.
>>
>> How do you use it on other platform?
> 
> I see, thanks.
> 
> So if I am understanding you correctly I have to maintain the
> 'mediatek,sysctl' phandle and use it in the first instance and if it
> fails I have to use regmap_loopup_by_compatible() stuff, right?
> Something like:
> 
> ...
> 
> drvdata->sysc = syscon_regmap_lookup_by_phandle(np, "mediatek,sysctl");
> if (IS_ERR(drvdata->sysc)) {
>          drvdata->sysc =
> syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>          if (IS_ERR(drvdata->sysc))
>               return PTR_ERR(drvdata->sysc);
> }
> 
> ....
> 
> So in that case, we can add the new phandle to the bindings without
> any kind of ABI breakage and we can use old dtbs using new watchdog
> driver code.
> 

Yes.

Best regards,
Krzysztof

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Guenter Roeck]

On 2/14/23 00:31, Krzysztof Kozlowski wrote:
> On 13/02/2023 21:05, Sergio Paracuellos wrote:
>> 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.
>>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> ---
>>   drivers/watchdog/Kconfig      |  4 +++-
>>   drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>>   2 files changed, 16 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..22e979bdd 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;
>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>   	if (!drvdata)
>>   		return -ENOMEM;
>>   
>> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>> +	if (IS_ERR(drvdata->sysc))
>> +		return PTR_ERR(drvdata->sysc);
> 
> This should be the backup/error path for original code using syscon
> property. Looking up by compatible is really not portable/re-usable.
> 

I really disagree here.

$ git grep syscon_regmap_lookup_by_compatible | wc
      90     326    8940

I have not yet reviewed this code, but I do not accept this argument against it.

Guenter

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 14/02/2023 13:10, Guenter Roeck wrote:
> On 2/14/23 00:31, Krzysztof Kozlowski wrote:
>> On 13/02/2023 21:05, Sergio Paracuellos wrote:
>>> 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.
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>> ---
>>>   drivers/watchdog/Kconfig      |  4 +++-
>>>   drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>>>   2 files changed, 16 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..22e979bdd 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;
>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>   	if (!drvdata)
>>>   		return -ENOMEM;
>>>   
>>> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>> +	if (IS_ERR(drvdata->sysc))
>>> +		return PTR_ERR(drvdata->sysc);
>>
>> This should be the backup/error path for original code using syscon
>> property. Looking up by compatible is really not portable/re-usable.
>>
> 
> I really disagree here.
> 
> $ git grep syscon_regmap_lookup_by_compatible | wc
>       90     326    8940
> 
> I have not yet reviewed this code, but I do not accept this argument against it.

First, argument that bad pattern is being used is not an argument to
keep it and repeat it.

Second, we already had examples that:
1. Author used syscon_regmap_lookup_by_compatible() and assumed "we will
never add new variant/soc".
2. Then turns out that new variants are obviously added and
syscon_regmap_lookup_by_compatible() stops scaling.

Whether any new variant/compatible/platform can appear for this watchdog
- I don't know.

Third, with syscon_regmap_lookup_by_compatible() you have undocumented
(not in the binding) dependency between blocks which:
a. stops any reusability,
b. affects device links and probe ordering (simply - there is no, device
must defer probe),
c. is simply undocumented.

The usage of syscon_regmap_lookup_by_compatible() has clear drawbacks
thus new code should rather use syscon phandles which solve all of above.

Best regards,
Krzysztof

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Guenter Roeck]

On 2/14/23 04:15, Krzysztof Kozlowski wrote:
> On 14/02/2023 13:10, Guenter Roeck wrote:
>> On 2/14/23 00:31, Krzysztof Kozlowski wrote:
>>> On 13/02/2023 21:05, Sergio Paracuellos wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>> ---
>>>>    drivers/watchdog/Kconfig      |  4 +++-
>>>>    drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>>>>    2 files changed, 16 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..22e979bdd 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;
>>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>>    	if (!drvdata)
>>>>    		return -ENOMEM;
>>>>    
>>>> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>>> +	if (IS_ERR(drvdata->sysc))
>>>> +		return PTR_ERR(drvdata->sysc);
>>>
>>> This should be the backup/error path for original code using syscon
>>> property. Looking up by compatible is really not portable/re-usable.
>>>
>>
>> I really disagree here.
>>
>> $ git grep syscon_regmap_lookup_by_compatible | wc
>>        90     326    8940
>>
>> I have not yet reviewed this code, but I do not accept this argument against it.
> 
> First, argument that bad pattern is being used is not an argument to
> keep it and repeat it.
> 
> Second, we already had examples that:
> 1. Author used syscon_regmap_lookup_by_compatible() and assumed "we will
> never add new variant/soc".
> 2. Then turns out that new variants are obviously added and
> syscon_regmap_lookup_by_compatible() stops scaling.
> 

Your arguments only applies if 1) there is ever going to be a new Mediatek/Ralink
chip with the same watchdog core, and 2) that chip also has the same watchdog
related registers in its syscon node.

Both is highly unlikely to happen. There already is a more generic watchdog
driver for later Mediatek chips (mtk_wdt.c). Even if Mediatek/Ralink ever
decides to revive this specific watchdog core for whatever reason,
the time to make the code more generic would have been at that time, at the same
time ensuring that the more generic code actually works and does have the
same syscon registers to obtain the boot status.

> Whether any new variant/compatible/platform can appear for this watchdog
> - I don't know.
> 
> Third, with syscon_regmap_lookup_by_compatible() you have undocumented
> (not in the binding) dependency between blocks which:
> a. stops any reusability,
> b. affects device links and probe ordering (simply - there is no, device
> must defer probe),
> c. is simply undocumented.
> 
> The usage of syscon_regmap_lookup_by_compatible() has clear drawbacks
> thus new code should rather use syscon phandles which solve all of above.
> 

"new code". Exactly. This isn't new code.

In order to make progress, I'll accept v7, assuming its updated devicetree
description is going to be accepted, but my argument still stands.

Guenter

Re: [PATCH v6 3/3] watchdog: mt7621-wdt: avoid ralink architecture dependent code

[Krzysztof Kozlowski]

On 14/02/2023 16:07, Guenter Roeck wrote:
> On 2/14/23 04:15, Krzysztof Kozlowski wrote:
>> On 14/02/2023 13:10, Guenter Roeck wrote:
>>> On 2/14/23 00:31, Krzysztof Kozlowski wrote:
>>>> On 13/02/2023 21:05, Sergio Paracuellos wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>> ---
>>>>>    drivers/watchdog/Kconfig      |  4 +++-
>>>>>    drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++-----
>>>>>    2 files changed, 16 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..22e979bdd 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;
>>>>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev)
>>>>>    	if (!drvdata)
>>>>>    		return -ENOMEM;
>>>>>    
>>>>> +	drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc");
>>>>> +	if (IS_ERR(drvdata->sysc))
>>>>> +		return PTR_ERR(drvdata->sysc);
>>>>
>>>> This should be the backup/error path for original code using syscon
>>>> property. Looking up by compatible is really not portable/re-usable.
>>>>
>>>
>>> I really disagree here.
>>>
>>> $ git grep syscon_regmap_lookup_by_compatible | wc
>>>        90     326    8940
>>>
>>> I have not yet reviewed this code, but I do not accept this argument against it.
>>
>> First, argument that bad pattern is being used is not an argument to
>> keep it and repeat it.
>>
>> Second, we already had examples that:
>> 1. Author used syscon_regmap_lookup_by_compatible() and assumed "we will
>> never add new variant/soc".
>> 2. Then turns out that new variants are obviously added and
>> syscon_regmap_lookup_by_compatible() stops scaling.
>>
> 
> Your arguments only applies if 1) there is ever going to be a new Mediatek/Ralink
> chip with the same watchdog core, and 2) that chip also has the same watchdog
> related registers in its syscon node.
> 
> Both is highly unlikely to happen. There already is a more generic watchdog
> driver for later Mediatek chips (mtk_wdt.c). Even if Mediatek/Ralink ever
> decides to revive this specific watchdog core for whatever reason,
> the time to make the code more generic would have been at that time, at the same
> time ensuring that the more generic code actually works and does have the
> same syscon registers to obtain the boot status.

Sure. In other cases we experienced, people also claimed that this will
not happen. But it happened... I cannot judge whether this will happen
here or not, thus I give generic guideline. If you (singular and plural)
are sure this will not happen, then the code is fine.

> 
>> Whether any new variant/compatible/platform can appear for this watchdog
>> - I don't know.
>>
>> Third, with syscon_regmap_lookup_by_compatible() you have undocumented
>> (not in the binding) dependency between blocks which:
>> a. stops any reusability,
>> b. affects device links and probe ordering (simply - there is no, device
>> must defer probe),
>> c. is simply undocumented.
>>
>> The usage of syscon_regmap_lookup_by_compatible() has clear drawbacks
>> thus new code should rather use syscon phandles which solve all of above.
>>
> 
> "new code". Exactly. This isn't new code.

New code not as "new driver" but new code adding syscon usage. Syscon is
new here so we have flexibility to make it differently, and IMHO, one
way is better than other (for the reasons I stated).

> 
> In order to make progress, I'll accept v7, assuming its updated devicetree
> description is going to be accepted, but my argument still stands.


Best regards,
Krzysztof