[PATCH v2 0/4] watchdog: mt7621-wdt: avoid globals and arch dependencies

[PATCH v2 0/4] 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].

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

Sergio Paracuellos (4):
  dt-bindings: watchdog: mt7621-wdt: add phandle to access system
    controller registers
  mips: dts: ralink: mt7621: add phandle to system controller node for
    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         |   1 +
 drivers/watchdog/Kconfig                      |   2 +
 drivers/watchdog/mt7621_wdt.c                 | 121 ++++++++++++------
 4 files changed, 90 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] 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..cc701e920 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
 
+  ralink,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>;
+      ralink,sysctl = <&sysc>;
     };
-- 
2.25.1

[PATCH v2 2/4] 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..1a1c017d7 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>;
+			ralink,sysctl = <&sysc>;
 		};
 
 		gpio: gpio@600 {
-- 
2.25.1

[PATCH v2 3/4] 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.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/watchdog/mt7621_wdt.c | 104 ++++++++++++++++++++++------------
 1 file changed, 67 insertions(+), 37 deletions(-)

diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
index a8aa3522c..f1c702971 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;
+
+	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
 
-	mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
-	if (!IS_ERR(mt7621_wdt_reset))
-		reset_control_deassert(mt7621_wdt_reset);
+	drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (!IS_ERR(drvdata->rst))
+		reset_control_deassert(drvdata->rst);
 
-	mt7621_wdt_dev.bootstatus = mt7621_wdt_bootcause();
+	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;
 
-	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->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,27 @@ 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);
+	}
+
+	err = devm_watchdog_register_device(dev, &drvdata->wdt);
+	if (err) {
+		dev_err(dev, "Error registering watchdog device\n");
+		return err;
 	}
 
-	return devm_watchdog_register_device(dev, &mt7621_wdt_dev);
+	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 v2 4/4] 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      |  2 ++
 drivers/watchdog/mt7621_wdt.c | 19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b64bc49c7..0759de670 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1865,6 +1865,8 @@ config GXP_WATCHDOG
 config MT7621_WDT
 	tristate "Mediatek SoC watchdog"
 	select WATCHDOG_CORE
+	select REGMAP_MMIO
+	select MFD_SYSCON
 	depends on SOC_MT7620 || SOC_MT7621
 	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 f1c702971..a7480fd2b 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, "ralink,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 v2 1/4] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

[Krzysztof Kozlowski]

On 10/02/2023 13:17, 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..cc701e920 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
>  
> +  ralink,sysctl:

Thanks for the changes. I did not notice it before - isn't Ralink part
of Mediatek now? Also compatible is mediatek, not "ralink"?



Best regards,
Krzysztof

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

[Sergio Paracuellos]

On Fri, Feb 10, 2023 at 2:03 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/02/2023 13:17, 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..cc701e920 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
> >
> > +  ralink,sysctl:
>
> Thanks for the changes. I did not notice it before - isn't Ralink part
> of Mediatek now? Also compatible is mediatek, not "ralink"?

Yes, it is. I was using the same prefix as for the memory controller
syscon phandler inside the 'sysc' node [0].

I have no problems at all in changing this to 'mediatek' if preferred.

Thanks,
    Sergio Paracuellos

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/ralink/mt7621.dtsi#n67
>
>
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 10/02/2023 14:14, Sergio Paracuellos wrote:
> On Fri, Feb 10, 2023 at 2:03 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 10/02/2023 13:17, 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..cc701e920 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
>>>
>>> +  ralink,sysctl:
>>
>> Thanks for the changes. I did not notice it before - isn't Ralink part
>> of Mediatek now? Also compatible is mediatek, not "ralink"?
> 
> Yes, it is. I was using the same prefix as for the memory controller
> syscon phandler inside the 'sysc' node [0].
> 
> I have no problems at all in changing this to 'mediatek' if preferred.
> 

Yes, mediatek as already used in this binding is preferred.

Best regards,
Krzysztof

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

[Sergio Paracuellos]

On Fri, Feb 10, 2023 at 2:35 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/02/2023 14:14, Sergio Paracuellos wrote:
> > On Fri, Feb 10, 2023 at 2:03 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 10/02/2023 13:17, 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..cc701e920 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
> >>>
> >>> +  ralink,sysctl:
> >>
> >> Thanks for the changes. I did not notice it before - isn't Ralink part
> >> of Mediatek now? Also compatible is mediatek, not "ralink"?
> >
> > Yes, it is. I was using the same prefix as for the memory controller
> > syscon phandler inside the 'sysc' node [0].
> >
> > I have no problems at all in changing this to 'mediatek' if preferred.
> >
>
> Yes, mediatek as already used in this binding is preferred.

Sure, I will do it and send v3. Let's wait for some other review
comments in the rest of the patches and will send all them together.

>
> Best regards,
> Krzysztof
>

Thanks,
    Sergio Paracuellos

Re: [PATCH v2 3/4] watchdog: mt7621-wdt: avoid static global declarations

[Guenter Roeck]

On 2/10/23 04:17, Sergio Paracuellos wrote:
> Instead of using static global definitions in driver code, refactor code
> introducing a new watchdog driver data structure and use it along the
> code.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>   drivers/watchdog/mt7621_wdt.c | 104 ++++++++++++++++++++++------------
>   1 file changed, 67 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
> index a8aa3522c..f1c702971 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;
> +
> +	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
>   
> -	mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> -	if (!IS_ERR(mt7621_wdt_reset))
> -		reset_control_deassert(mt7621_wdt_reset);
> +	drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (!IS_ERR(drvdata->rst))
> +		reset_control_deassert(drvdata->rst);
>   
> -	mt7621_wdt_dev.bootstatus = mt7621_wdt_bootcause();
> +	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;
>   
> -	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->bootstatus = mt7621_wdt_bootcause();
> +
> +	watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev);

FWIW, setting ->timeout would have been sufficient. watchdog_init_timeout()
is only really valuable if there is a module parameter, or if the timeout
is set from devicetree. That won't work here, though, because the passed
value takes precedence. Changing that would change functionality and thus would
have to be done in a separate patch, I just wanted to mention it.

> +	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,27 @@ 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);
> +	}
> +
> +	err = devm_watchdog_register_device(dev, &drvdata->wdt);
> +	if (err) {
> +		dev_err(dev, "Error registering watchdog device\n");
> +		return err;
>   	}

This is a functional change. If you want to add an error message,
do it in a separate patch and provide a rationale for it.

>   
> -	return devm_watchdog_register_device(dev, &mt7621_wdt_dev);
> +	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);
>   }

Also FWIW, we have watchdog_stop_on_reboot() for that purpose.
Changing that would be a separate patch, though.

>   static const struct of_device_id mt7621_wdt_match[] = {

Re: [PATCH v2 3/4] watchdog: mt7621-wdt: avoid static global declarations

[Sergio Paracuellos]

Hi Guenter,

Thanks for reviewing this.

On Fri, Feb 10, 2023 at 4:00 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/10/23 04:17, Sergio Paracuellos wrote:
> > Instead of using static global definitions in driver code, refactor code
> > introducing a new watchdog driver data structure and use it along the
> > code.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >   drivers/watchdog/mt7621_wdt.c | 104 ++++++++++++++++++++++------------
> >   1 file changed, 67 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
> > index a8aa3522c..f1c702971 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;
> > +
> > +     drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(drvdata->base))
> > +             return PTR_ERR(drvdata->base);
> >
> > -     mt7621_wdt_reset = devm_reset_control_get_exclusive(dev, NULL);
> > -     if (!IS_ERR(mt7621_wdt_reset))
> > -             reset_control_deassert(mt7621_wdt_reset);
> > +     drvdata->rst = devm_reset_control_get_exclusive(dev, NULL);
> > +     if (!IS_ERR(drvdata->rst))
> > +             reset_control_deassert(drvdata->rst);
> >
> > -     mt7621_wdt_dev.bootstatus = mt7621_wdt_bootcause();
> > +     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;
> >
> > -     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->bootstatus = mt7621_wdt_bootcause();
> > +
> > +     watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev);
>
> FWIW, setting ->timeout would have been sufficient. watchdog_init_timeout()
> is only really valuable if there is a module parameter, or if the timeout
> is set from devicetree. That won't work here, though, because the passed
> value takes precedence. Changing that would change functionality and thus would
> have to be done in a separate patch, I just wanted to mention it.

Thanks for letting me know. By now I only want to maintain the current
functionality and if it really makes sense I will send functionality
changes in new series after this one is applied.

>
> > +     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,27 @@ 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);
> > +     }
> > +
> > +     err = devm_watchdog_register_device(dev, &drvdata->wdt);
> > +     if (err) {
> > +             dev_err(dev, "Error registering watchdog device\n");
> > +             return err;
> >       }
>
> This is a functional change. If you want to add an error message,
> do it in a separate patch and provide a rationale for it.

True, will drop this error message in v3 to maintain current
functionality without any extra changes.

>
> >
> > -     return devm_watchdog_register_device(dev, &mt7621_wdt_dev);
> > +     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);
> >   }
>
> Also FWIW, we have watchdog_stop_on_reboot() for that purpose.
> Changing that would be a separate patch, though.

Thanks. As I have just said above, let's focus only on maintaining
current functionality in this series.

>
> >   static const struct of_device_id mt7621_wdt_match[] = {
>

Thanks,
    Sergio Paracuellos