[PATCH 0/3] watchdog: mtk_wdt: add support for mt6577

[PATCH 0/3] watchdog: mtk_wdt: add support for mt6577

[Boris Lysov]

This series adds support for mt6577 watchdog to the:
a) mtk_wdt driver - by bringing support for 16-bit I/O
b) mtk_wdt dt-binding - by declaring compatibility with mt6577

Without these changes user would have to disable watchdog by manually
writing a value to the register, otherwise the device will power off.

Accepting these patches will make ground for submitting additional
changes related to the mainline support of mt6577 (and other compatible
SoCs) in future.

Suggested patches successfully pass all `checkpatch.pl` checks, and they
do not interfere with already supported watchdogs.

Boris Lysov (3):
  dt-bindings: watchdog: mediatek: add support for mt6577 SoC
  watchdog: mtk_wdt: add support for 16-bit control registers
  watchdog: mtk_wdt: declare compatibility with mt6577

 .../devicetree/bindings/watchdog/mtk-wdt.txt  |  1 +
 drivers/watchdog/Kconfig                      |  9 +++++
 drivers/watchdog/mtk_wdt.c                    | 35 +++++++++++++------
 3 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH 0/3] watchdog: mtk_wdt: add support for mt6577

[Boris Lysov]

This series adds support for mt6577 watchdog to the:
a) mtk_wdt driver - by bringing support for 16-bit I/O
b) mtk_wdt dt-binding - by declaring compatibility with mt6577

Without these changes user would have to disable watchdog by manually
writing a value to the register, otherwise the device will power off.

Accepting these patches will make ground for submitting additional
changes related to the mainline support of mt6577 (and other compatible
SoCs) in future.

Suggested patches successfully pass all `checkpatch.pl` checks, and they
do not interfere with already supported watchdogs.

Boris Lysov (3):
  dt-bindings: watchdog: mediatek: add support for mt6577 SoC
  watchdog: mtk_wdt: add support for 16-bit control registers
  watchdog: mtk_wdt: declare compatibility with mt6577

 .../devicetree/bindings/watchdog/mtk-wdt.txt  |  1 +
 drivers/watchdog/Kconfig                      |  9 +++++
 drivers/watchdog/mtk_wdt.c                    | 35 +++++++++++++------
 3 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 1/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Boris Lysov]

Add support for Mediatek mt6577 SoC to device tree binding
documentation.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index 4dd36bd3f1ad..7f84ec1b8dc7 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible should contain:
 	"mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
 	"mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
+	"mediatek,mt6577-wdt": for MT6577
 	"mediatek,mt6589-wdt": for MT6589
 	"mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
 	"mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
-- 
2.20.1

[PATCH 1/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Boris Lysov]

Add support for Mediatek mt6577 SoC to device tree binding
documentation.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index 4dd36bd3f1ad..7f84ec1b8dc7 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible should contain:
 	"mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
 	"mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
+	"mediatek,mt6577-wdt": for MT6577
 	"mediatek,mt6589-wdt": for MT6589
 	"mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
 	"mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 1/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Rob Herring]

On Mon, 01 Feb 2021 02:44:23 +0300, Boris Lysov wrote:
> Add support for Mediatek mt6577 SoC to device tree binding
> documentation.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>  Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Rob Herring]

On Mon, 01 Feb 2021 02:44:23 +0300, Boris Lysov wrote:
> Add support for Mediatek mt6577 SoC to device tree binding
> documentation.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>  Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Boris Lysov]

Add support for 16-bit control registers.
Some old Mediatek SoCs such as mt6577 use 16-bit I/O operations
for controlling watchdog. This commit redefines read/write
functions and some values in mtk_wdt driver depending on the
16-bit register support flag in kernel configuration.
By default, driver still uses 32-bit values and I/O functions, so
currently supported devices are unaffected.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/Kconfig   |  9 +++++++++
 drivers/watchdog/mtk_wdt.c | 34 ++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7ff941e71b79..83a4b57ede3f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -865,6 +865,15 @@ config MEDIATEK_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called mtk_wdt.
 
+config MEDIATEK_WATCHDOG_16BIT
+	bool "Support 16-bit control registers"
+	depends on MEDIATEK_WATCHDOG=y
+	help
+	  Some Mediatek SoCs such as mt6577 have 16-bit registers for
+	  controlling watchdog. Newer SoCs usually use 32-bit read/write
+	  operations.
+	  If in doubt, say N.
+
 config DIGICOLOR_WATCHDOG
 	tristate "Conexant Digicolor SoCs watchdog support"
 	depends on ARCH_DIGICOLOR || COMPILE_TEST
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index d6a6393f609d..0ab3cbcf0d93 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -43,13 +43,27 @@
 #define WDT_MODE_IRQ_EN		(1 << 3)
 #define WDT_MODE_AUTO_START	(1 << 4)
 #define WDT_MODE_DUAL_EN	(1 << 6)
-#define WDT_MODE_KEY		0x22000000
 
 #define WDT_SWRST		0x14
 #define WDT_SWRST_KEY		0x1209
 
 #define WDT_SWSYSRST		0x18U
+
+#ifdef CONFIG_MEDIATEK_WATCHDOG_16BIT
+#define WDT_MODE_KEY		0x2200
+#define WDT_SWSYS_RST_KEY	0x8800
+#define mtk_wdt_read(a) readw(a)
+#define mtk_wdt_write(v, a) writew(v, a)
+#define mtk_wdt_ioread(a) ioread16(a)
+#define mtk_wdt_iowrite(v, a) iowrite16(v, a)
+#else
+#define WDT_MODE_KEY		0x22000000
 #define WDT_SWSYS_RST_KEY	0x88000000
+#define mtk_wdt_read(a) readl(a)
+#define mtk_wdt_write(v, a) writel(v, a)
+#define mtk_wdt_ioread(a) ioread32(a)
+#define mtk_wdt_iowrite(v, a) iowrite32(v, a)
+#endif
 
 #define DRV_NAME		"mtk-wdt"
 #define DRV_VERSION		"1.0"
@@ -86,13 +100,13 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	tmp = readl(data->wdt_base + WDT_SWSYSRST);
+	tmp = mtk_wdt_read(data->wdt_base + WDT_SWSYSRST);
 	if (assert)
 		tmp |= BIT(id);
 	else
 		tmp &= ~BIT(id);
 	tmp |= WDT_SWSYS_RST_KEY;
-	writel(tmp, data->wdt_base + WDT_SWSYSRST);
+	mtk_wdt_write(tmp, data->wdt_base + WDT_SWSYSRST);
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -157,7 +171,7 @@ static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
 	wdt_base = mtk_wdt->wdt_base;
 
 	while (1) {
-		writel(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
+		mtk_wdt_write(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
 		mdelay(5);
 	}
 
@@ -169,7 +183,7 @@ static int mtk_wdt_ping(struct watchdog_device *wdt_dev)
 	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
 	void __iomem *wdt_base = mtk_wdt->wdt_base;
 
-	iowrite32(WDT_RST_RELOAD, wdt_base + WDT_RST);
+	mtk_wdt_iowrite(WDT_RST_RELOAD, wdt_base + WDT_RST);
 
 	return 0;
 }
@@ -188,7 +202,7 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
 	 * The clock has 32 KHz
 	 */
 	reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
-	iowrite32(reg, wdt_base + WDT_LENGTH);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_LENGTH);
 
 	mtk_wdt_ping(wdt_dev);
 
@@ -201,10 +215,10 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
 	void __iomem *wdt_base = mtk_wdt->wdt_base;
 	u32 reg;
 
-	reg = readl(wdt_base + WDT_MODE);
+	reg = mtk_wdt_read(wdt_base + WDT_MODE);
 	reg &= ~WDT_MODE_EN;
 	reg |= WDT_MODE_KEY;
-	iowrite32(reg, wdt_base + WDT_MODE);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
 
 	return 0;
 }
@@ -220,10 +234,10 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
 	if (ret < 0)
 		return ret;
 
-	reg = ioread32(wdt_base + WDT_MODE);
+	reg = mtk_wdt_ioread(wdt_base + WDT_MODE);
 	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
 	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
-	iowrite32(reg, wdt_base + WDT_MODE);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
 
 	return 0;
 }
-- 
2.20.1

[PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Boris Lysov]

Add support for 16-bit control registers.
Some old Mediatek SoCs such as mt6577 use 16-bit I/O operations
for controlling watchdog. This commit redefines read/write
functions and some values in mtk_wdt driver depending on the
16-bit register support flag in kernel configuration.
By default, driver still uses 32-bit values and I/O functions, so
currently supported devices are unaffected.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/Kconfig   |  9 +++++++++
 drivers/watchdog/mtk_wdt.c | 34 ++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7ff941e71b79..83a4b57ede3f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -865,6 +865,15 @@ config MEDIATEK_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called mtk_wdt.
 
+config MEDIATEK_WATCHDOG_16BIT
+	bool "Support 16-bit control registers"
+	depends on MEDIATEK_WATCHDOG=y
+	help
+	  Some Mediatek SoCs such as mt6577 have 16-bit registers for
+	  controlling watchdog. Newer SoCs usually use 32-bit read/write
+	  operations.
+	  If in doubt, say N.
+
 config DIGICOLOR_WATCHDOG
 	tristate "Conexant Digicolor SoCs watchdog support"
 	depends on ARCH_DIGICOLOR || COMPILE_TEST
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index d6a6393f609d..0ab3cbcf0d93 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -43,13 +43,27 @@
 #define WDT_MODE_IRQ_EN		(1 << 3)
 #define WDT_MODE_AUTO_START	(1 << 4)
 #define WDT_MODE_DUAL_EN	(1 << 6)
-#define WDT_MODE_KEY		0x22000000
 
 #define WDT_SWRST		0x14
 #define WDT_SWRST_KEY		0x1209
 
 #define WDT_SWSYSRST		0x18U
+
+#ifdef CONFIG_MEDIATEK_WATCHDOG_16BIT
+#define WDT_MODE_KEY		0x2200
+#define WDT_SWSYS_RST_KEY	0x8800
+#define mtk_wdt_read(a) readw(a)
+#define mtk_wdt_write(v, a) writew(v, a)
+#define mtk_wdt_ioread(a) ioread16(a)
+#define mtk_wdt_iowrite(v, a) iowrite16(v, a)
+#else
+#define WDT_MODE_KEY		0x22000000
 #define WDT_SWSYS_RST_KEY	0x88000000
+#define mtk_wdt_read(a) readl(a)
+#define mtk_wdt_write(v, a) writel(v, a)
+#define mtk_wdt_ioread(a) ioread32(a)
+#define mtk_wdt_iowrite(v, a) iowrite32(v, a)
+#endif
 
 #define DRV_NAME		"mtk-wdt"
 #define DRV_VERSION		"1.0"
@@ -86,13 +100,13 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	tmp = readl(data->wdt_base + WDT_SWSYSRST);
+	tmp = mtk_wdt_read(data->wdt_base + WDT_SWSYSRST);
 	if (assert)
 		tmp |= BIT(id);
 	else
 		tmp &= ~BIT(id);
 	tmp |= WDT_SWSYS_RST_KEY;
-	writel(tmp, data->wdt_base + WDT_SWSYSRST);
+	mtk_wdt_write(tmp, data->wdt_base + WDT_SWSYSRST);
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -157,7 +171,7 @@ static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
 	wdt_base = mtk_wdt->wdt_base;
 
 	while (1) {
-		writel(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
+		mtk_wdt_write(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
 		mdelay(5);
 	}
 
@@ -169,7 +183,7 @@ static int mtk_wdt_ping(struct watchdog_device *wdt_dev)
 	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
 	void __iomem *wdt_base = mtk_wdt->wdt_base;
 
-	iowrite32(WDT_RST_RELOAD, wdt_base + WDT_RST);
+	mtk_wdt_iowrite(WDT_RST_RELOAD, wdt_base + WDT_RST);
 
 	return 0;
 }
@@ -188,7 +202,7 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
 	 * The clock has 32 KHz
 	 */
 	reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
-	iowrite32(reg, wdt_base + WDT_LENGTH);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_LENGTH);
 
 	mtk_wdt_ping(wdt_dev);
 
@@ -201,10 +215,10 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
 	void __iomem *wdt_base = mtk_wdt->wdt_base;
 	u32 reg;
 
-	reg = readl(wdt_base + WDT_MODE);
+	reg = mtk_wdt_read(wdt_base + WDT_MODE);
 	reg &= ~WDT_MODE_EN;
 	reg |= WDT_MODE_KEY;
-	iowrite32(reg, wdt_base + WDT_MODE);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
 
 	return 0;
 }
@@ -220,10 +234,10 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
 	if (ret < 0)
 		return ret;
 
-	reg = ioread32(wdt_base + WDT_MODE);
+	reg = mtk_wdt_ioread(wdt_base + WDT_MODE);
 	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
 	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
-	iowrite32(reg, wdt_base + WDT_MODE);
+	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
 
 	return 0;
 }
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Guenter Roeck]

On 1/31/21 3:44 PM, Boris Lysov wrote:
> Add support for 16-bit control registers.
> Some old Mediatek SoCs such as mt6577 use 16-bit I/O operations
> for controlling watchdog. This commit redefines read/write
> functions and some values in mtk_wdt driver depending on the
> 16-bit register support flag in kernel configuration.
> By default, driver still uses 32-bit values and I/O functions, so
> currently supported devices are unaffected.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>

We can't do this. With this flag enabled, the watchdog won't
support other SoCs, and there is nothing that prevents the flag
from being set for those SoCs.

This has to be handled differently, without configuration
flag. Maybe use regmap for register addresses, and use the
compatible string to determine which regmap settings to use,
or use accessor functions in mtk_wdt_dev.

Guenter

> ---
>  drivers/watchdog/Kconfig   |  9 +++++++++
>  drivers/watchdog/mtk_wdt.c | 34 ++++++++++++++++++++++++----------
>  2 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7ff941e71b79..83a4b57ede3f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -865,6 +865,15 @@ config MEDIATEK_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called mtk_wdt.
>  
> +config MEDIATEK_WATCHDOG_16BIT
> +	bool "Support 16-bit control registers"
> +	depends on MEDIATEK_WATCHDOG=y
> +	help
> +	  Some Mediatek SoCs such as mt6577 have 16-bit registers for
> +	  controlling watchdog. Newer SoCs usually use 32-bit read/write
> +	  operations.
> +	  If in doubt, say N.
> +
>  config DIGICOLOR_WATCHDOG
>  	tristate "Conexant Digicolor SoCs watchdog support"
>  	depends on ARCH_DIGICOLOR || COMPILE_TEST
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index d6a6393f609d..0ab3cbcf0d93 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -43,13 +43,27 @@
>  #define WDT_MODE_IRQ_EN		(1 << 3)
>  #define WDT_MODE_AUTO_START	(1 << 4)
>  #define WDT_MODE_DUAL_EN	(1 << 6)
> -#define WDT_MODE_KEY		0x22000000
>  
>  #define WDT_SWRST		0x14
>  #define WDT_SWRST_KEY		0x1209
>  
>  #define WDT_SWSYSRST		0x18U
> +
> +#ifdef CONFIG_MEDIATEK_WATCHDOG_16BIT
> +#define WDT_MODE_KEY		0x2200
> +#define WDT_SWSYS_RST_KEY	0x8800
> +#define mtk_wdt_read(a) readw(a)
> +#define mtk_wdt_write(v, a) writew(v, a)
> +#define mtk_wdt_ioread(a) ioread16(a)
> +#define mtk_wdt_iowrite(v, a) iowrite16(v, a)
> +#else
> +#define WDT_MODE_KEY		0x22000000
>  #define WDT_SWSYS_RST_KEY	0x88000000
> +#define mtk_wdt_read(a) readl(a)
> +#define mtk_wdt_write(v, a) writel(v, a)
> +#define mtk_wdt_ioread(a) ioread32(a)
> +#define mtk_wdt_iowrite(v, a) iowrite32(v, a)
> +#endif
>  
>  #define DRV_NAME		"mtk-wdt"
>  #define DRV_VERSION		"1.0"
> @@ -86,13 +100,13 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>  
>  	spin_lock_irqsave(&data->lock, flags);
>  
> -	tmp = readl(data->wdt_base + WDT_SWSYSRST);
> +	tmp = mtk_wdt_read(data->wdt_base + WDT_SWSYSRST);
>  	if (assert)
>  		tmp |= BIT(id);
>  	else
>  		tmp &= ~BIT(id);
>  	tmp |= WDT_SWSYS_RST_KEY;
> -	writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +	mtk_wdt_write(tmp, data->wdt_base + WDT_SWSYSRST);
>  
>  	spin_unlock_irqrestore(&data->lock, flags);
>  
> @@ -157,7 +171,7 @@ static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
>  	wdt_base = mtk_wdt->wdt_base;
>  
>  	while (1) {
> -		writel(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
> +		mtk_wdt_write(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
>  		mdelay(5);
>  	}
>  
> @@ -169,7 +183,7 @@ static int mtk_wdt_ping(struct watchdog_device *wdt_dev)
>  	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
>  	void __iomem *wdt_base = mtk_wdt->wdt_base;
>  
> -	iowrite32(WDT_RST_RELOAD, wdt_base + WDT_RST);
> +	mtk_wdt_iowrite(WDT_RST_RELOAD, wdt_base + WDT_RST);
>  
>  	return 0;
>  }
> @@ -188,7 +202,7 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  	 * The clock has 32 KHz
>  	 */
>  	reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
> -	iowrite32(reg, wdt_base + WDT_LENGTH);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_LENGTH);
>  
>  	mtk_wdt_ping(wdt_dev);
>  
> @@ -201,10 +215,10 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
>  	void __iomem *wdt_base = mtk_wdt->wdt_base;
>  	u32 reg;
>  
> -	reg = readl(wdt_base + WDT_MODE);
> +	reg = mtk_wdt_read(wdt_base + WDT_MODE);
>  	reg &= ~WDT_MODE_EN;
>  	reg |= WDT_MODE_KEY;
> -	iowrite32(reg, wdt_base + WDT_MODE);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
>  
>  	return 0;
>  }
> @@ -220,10 +234,10 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	reg = ioread32(wdt_base + WDT_MODE);
> +	reg = mtk_wdt_ioread(wdt_base + WDT_MODE);
>  	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>  	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
> -	iowrite32(reg, wdt_base + WDT_MODE);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
>  
>  	return 0;
>  }
> 

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Guenter Roeck]

On 1/31/21 3:44 PM, Boris Lysov wrote:
> Add support for 16-bit control registers.
> Some old Mediatek SoCs such as mt6577 use 16-bit I/O operations
> for controlling watchdog. This commit redefines read/write
> functions and some values in mtk_wdt driver depending on the
> 16-bit register support flag in kernel configuration.
> By default, driver still uses 32-bit values and I/O functions, so
> currently supported devices are unaffected.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>

We can't do this. With this flag enabled, the watchdog won't
support other SoCs, and there is nothing that prevents the flag
from being set for those SoCs.

This has to be handled differently, without configuration
flag. Maybe use regmap for register addresses, and use the
compatible string to determine which regmap settings to use,
or use accessor functions in mtk_wdt_dev.

Guenter

> ---
>  drivers/watchdog/Kconfig   |  9 +++++++++
>  drivers/watchdog/mtk_wdt.c | 34 ++++++++++++++++++++++++----------
>  2 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7ff941e71b79..83a4b57ede3f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -865,6 +865,15 @@ config MEDIATEK_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called mtk_wdt.
>  
> +config MEDIATEK_WATCHDOG_16BIT
> +	bool "Support 16-bit control registers"
> +	depends on MEDIATEK_WATCHDOG=y
> +	help
> +	  Some Mediatek SoCs such as mt6577 have 16-bit registers for
> +	  controlling watchdog. Newer SoCs usually use 32-bit read/write
> +	  operations.
> +	  If in doubt, say N.
> +
>  config DIGICOLOR_WATCHDOG
>  	tristate "Conexant Digicolor SoCs watchdog support"
>  	depends on ARCH_DIGICOLOR || COMPILE_TEST
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index d6a6393f609d..0ab3cbcf0d93 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -43,13 +43,27 @@
>  #define WDT_MODE_IRQ_EN		(1 << 3)
>  #define WDT_MODE_AUTO_START	(1 << 4)
>  #define WDT_MODE_DUAL_EN	(1 << 6)
> -#define WDT_MODE_KEY		0x22000000
>  
>  #define WDT_SWRST		0x14
>  #define WDT_SWRST_KEY		0x1209
>  
>  #define WDT_SWSYSRST		0x18U
> +
> +#ifdef CONFIG_MEDIATEK_WATCHDOG_16BIT
> +#define WDT_MODE_KEY		0x2200
> +#define WDT_SWSYS_RST_KEY	0x8800
> +#define mtk_wdt_read(a) readw(a)
> +#define mtk_wdt_write(v, a) writew(v, a)
> +#define mtk_wdt_ioread(a) ioread16(a)
> +#define mtk_wdt_iowrite(v, a) iowrite16(v, a)
> +#else
> +#define WDT_MODE_KEY		0x22000000
>  #define WDT_SWSYS_RST_KEY	0x88000000
> +#define mtk_wdt_read(a) readl(a)
> +#define mtk_wdt_write(v, a) writel(v, a)
> +#define mtk_wdt_ioread(a) ioread32(a)
> +#define mtk_wdt_iowrite(v, a) iowrite32(v, a)
> +#endif
>  
>  #define DRV_NAME		"mtk-wdt"
>  #define DRV_VERSION		"1.0"
> @@ -86,13 +100,13 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>  
>  	spin_lock_irqsave(&data->lock, flags);
>  
> -	tmp = readl(data->wdt_base + WDT_SWSYSRST);
> +	tmp = mtk_wdt_read(data->wdt_base + WDT_SWSYSRST);
>  	if (assert)
>  		tmp |= BIT(id);
>  	else
>  		tmp &= ~BIT(id);
>  	tmp |= WDT_SWSYS_RST_KEY;
> -	writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +	mtk_wdt_write(tmp, data->wdt_base + WDT_SWSYSRST);
>  
>  	spin_unlock_irqrestore(&data->lock, flags);
>  
> @@ -157,7 +171,7 @@ static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
>  	wdt_base = mtk_wdt->wdt_base;
>  
>  	while (1) {
> -		writel(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
> +		mtk_wdt_write(WDT_SWRST_KEY, wdt_base + WDT_SWRST);
>  		mdelay(5);
>  	}
>  
> @@ -169,7 +183,7 @@ static int mtk_wdt_ping(struct watchdog_device *wdt_dev)
>  	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev);
>  	void __iomem *wdt_base = mtk_wdt->wdt_base;
>  
> -	iowrite32(WDT_RST_RELOAD, wdt_base + WDT_RST);
> +	mtk_wdt_iowrite(WDT_RST_RELOAD, wdt_base + WDT_RST);
>  
>  	return 0;
>  }
> @@ -188,7 +202,7 @@ static int mtk_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  	 * The clock has 32 KHz
>  	 */
>  	reg = WDT_LENGTH_TIMEOUT(timeout << 6) | WDT_LENGTH_KEY;
> -	iowrite32(reg, wdt_base + WDT_LENGTH);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_LENGTH);
>  
>  	mtk_wdt_ping(wdt_dev);
>  
> @@ -201,10 +215,10 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
>  	void __iomem *wdt_base = mtk_wdt->wdt_base;
>  	u32 reg;
>  
> -	reg = readl(wdt_base + WDT_MODE);
> +	reg = mtk_wdt_read(wdt_base + WDT_MODE);
>  	reg &= ~WDT_MODE_EN;
>  	reg |= WDT_MODE_KEY;
> -	iowrite32(reg, wdt_base + WDT_MODE);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
>  
>  	return 0;
>  }
> @@ -220,10 +234,10 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	reg = ioread32(wdt_base + WDT_MODE);
> +	reg = mtk_wdt_ioread(wdt_base + WDT_MODE);
>  	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>  	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
> -	iowrite32(reg, wdt_base + WDT_MODE);
> +	mtk_wdt_iowrite(reg, wdt_base + WDT_MODE);
>  
>  	return 0;
>  }
> 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Boris Lysov]

В Sun, 31 Jan 2021 16:31:09 -0800
Guenter Roeck <linux@roeck-us.net> пишет:

> We can't do this. With this flag enabled, the watchdog won't
> support other SoCs, and there is nothing that prevents the flag
> from being set for those SoCs.
> 
> This has to be handled differently, without configuration
> flag. Maybe use regmap for register addresses, [snip],
> or use accessor functions in mtk_wdt_dev.

Thank you for reviewing my patch!

I will consider suggested fixes, and I will come up with better solution
in V2. I'm beginner developer, and am still learning.

> use the compatible string to determine which regmap settings to use

I think relying on hardcoded "compatible string - settings" pairs in
driver is not good. Whilst most Mediatek watchdogs I've seen use
similar drivers, no one (except Mediatek itself, of course) knows for
sure how many devices use 16-bit mode, and specifying each one in C
code may _theoretically_ bloat it. (well, on the other hand, I've seen
other watchdog drivers with many compatible devices listed in C code,
and they didn't seem bloated at all)

What do you think about implementing a simple boolean flag in
dt-binding for enabling the 16-bit operation mode? Something like
"mediatek,watchdog-16bits" [1] , which the driver would check for in
the `mtk_wdt_probe` and set corresponding regmaps. As result, there
won't be a need for kernel configuration flag, and other watchdogs
would be supported.

Most likely this idea doesn't sound good as I portray it, but I would
still like to hear your opinion about it. Thanks.

References:
[1] Mediatek UART APDMA driver uses similar flag
called `mediatek,dma-33bits`
Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Boris Lysov]

В Sun, 31 Jan 2021 16:31:09 -0800
Guenter Roeck <linux@roeck-us.net> пишет:

> We can't do this. With this flag enabled, the watchdog won't
> support other SoCs, and there is nothing that prevents the flag
> from being set for those SoCs.
> 
> This has to be handled differently, without configuration
> flag. Maybe use regmap for register addresses, [snip],
> or use accessor functions in mtk_wdt_dev.

Thank you for reviewing my patch!

I will consider suggested fixes, and I will come up with better solution
in V2. I'm beginner developer, and am still learning.

> use the compatible string to determine which regmap settings to use

I think relying on hardcoded "compatible string - settings" pairs in
driver is not good. Whilst most Mediatek watchdogs I've seen use
similar drivers, no one (except Mediatek itself, of course) knows for
sure how many devices use 16-bit mode, and specifying each one in C
code may _theoretically_ bloat it. (well, on the other hand, I've seen
other watchdog drivers with many compatible devices listed in C code,
and they didn't seem bloated at all)

What do you think about implementing a simple boolean flag in
dt-binding for enabling the 16-bit operation mode? Something like
"mediatek,watchdog-16bits" [1] , which the driver would check for in
the `mtk_wdt_probe` and set corresponding regmaps. As result, there
won't be a need for kernel configuration flag, and other watchdogs
would be supported.

Most likely this idea doesn't sound good as I portray it, but I would
still like to hear your opinion about it. Thanks.

References:
[1] Mediatek UART APDMA driver uses similar flag
called `mediatek,dma-33bits`
Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Guenter Roeck]

On 2/1/21 5:33 PM, Boris Lysov wrote:
> В Sun, 31 Jan 2021 16:31:09 -0800
> Guenter Roeck <linux@roeck-us.net> пишет:
> 
>> We can't do this. With this flag enabled, the watchdog won't
>> support other SoCs, and there is nothing that prevents the flag
>> from being set for those SoCs.
>>
>> This has to be handled differently, without configuration
>> flag. Maybe use regmap for register addresses, [snip],
>> or use accessor functions in mtk_wdt_dev.
> 
> Thank you for reviewing my patch!
> 
> I will consider suggested fixes, and I will come up with better solution
> in V2. I'm beginner developer, and am still learning.
> 
>> use the compatible string to determine which regmap settings to use
> 
> I think relying on hardcoded "compatible string - settings" pairs in
> driver is not good. Whilst most Mediatek watchdogs I've seen use

So you are saying that the existing mediatek watchdog driver is not good ?
Or that it is good for setting the number of supported reset pins,
but not for setting the register width ?

> similar drivers, no one (except Mediatek itself, of course) knows for
> sure how many devices use 16-bit mode, and specifying each one in C
> code may _theoretically_ bloat it. (well, on the other hand, I've seen
> other watchdog drivers with many compatible devices listed in C code,
> and they didn't seem bloated at all)
> 
> What do you think about implementing a simple boolean flag in
> dt-binding for enabling the 16-bit operation mode? Something like
> "mediatek,watchdog-16bits" [1] , which the driver would check for in
> the `mtk_wdt_probe` and set corresponding regmaps. As result, there
> won't be a need for kernel configuration flag, and other watchdogs
> would be supported.
> 
> Most likely this idea doesn't sound good as I portray it, but I would
> still like to hear your opinion about it. Thanks.
> 

I don't like it at all, I don't see your problem, and mediatek,dma-33bits
seems to be a completely different scope (all it does is to trigger a couple
of additional writes, not control register width). On the other side,
you would have to sell it to dt maintainers, not to me.

Guenter

> References:
> [1] Mediatek UART APDMA driver uses similar flag
> called `mediatek,dma-33bits`
> Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
> 

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Guenter Roeck]

On 2/1/21 5:33 PM, Boris Lysov wrote:
> В Sun, 31 Jan 2021 16:31:09 -0800
> Guenter Roeck <linux@roeck-us.net> пишет:
> 
>> We can't do this. With this flag enabled, the watchdog won't
>> support other SoCs, and there is nothing that prevents the flag
>> from being set for those SoCs.
>>
>> This has to be handled differently, without configuration
>> flag. Maybe use regmap for register addresses, [snip],
>> or use accessor functions in mtk_wdt_dev.
> 
> Thank you for reviewing my patch!
> 
> I will consider suggested fixes, and I will come up with better solution
> in V2. I'm beginner developer, and am still learning.
> 
>> use the compatible string to determine which regmap settings to use
> 
> I think relying on hardcoded "compatible string - settings" pairs in
> driver is not good. Whilst most Mediatek watchdogs I've seen use

So you are saying that the existing mediatek watchdog driver is not good ?
Or that it is good for setting the number of supported reset pins,
but not for setting the register width ?

> similar drivers, no one (except Mediatek itself, of course) knows for
> sure how many devices use 16-bit mode, and specifying each one in C
> code may _theoretically_ bloat it. (well, on the other hand, I've seen
> other watchdog drivers with many compatible devices listed in C code,
> and they didn't seem bloated at all)
> 
> What do you think about implementing a simple boolean flag in
> dt-binding for enabling the 16-bit operation mode? Something like
> "mediatek,watchdog-16bits" [1] , which the driver would check for in
> the `mtk_wdt_probe` and set corresponding regmaps. As result, there
> won't be a need for kernel configuration flag, and other watchdogs
> would be supported.
> 
> Most likely this idea doesn't sound good as I portray it, but I would
> still like to hear your opinion about it. Thanks.
> 

I don't like it at all, I don't see your problem, and mediatek,dma-33bits
seems to be a completely different scope (all it does is to trigger a couple
of additional writes, not control register width). On the other side,
you would have to sell it to dt maintainers, not to me.

Guenter

> References:
> [1] Mediatek UART APDMA driver uses similar flag
> called `mediatek,dma-33bits`
> Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
> 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Matthias Brugger]

On 02/02/2021 02:33, Boris Lysov wrote:
> В Sun, 31 Jan 2021 16:31:09 -0800
> Guenter Roeck <linux@roeck-us.net> пишет:
> 
>> We can't do this. With this flag enabled, the watchdog won't
>> support other SoCs, and there is nothing that prevents the flag
>> from being set for those SoCs.
>>
>> This has to be handled differently, without configuration
>> flag. Maybe use regmap for register addresses, [snip],
>> or use accessor functions in mtk_wdt_dev.
> 
> Thank you for reviewing my patch!
> 
> I will consider suggested fixes, and I will come up with better solution
> in V2. I'm beginner developer, and am still learning.
> 
>> use the compatible string to determine which regmap settings to use
> 
> I think relying on hardcoded "compatible string - settings" pairs in
> driver is not good. Whilst most Mediatek watchdogs I've seen use
> similar drivers, no one (except Mediatek itself, of course) knows for
> sure how many devices use 16-bit mode, and specifying each one in C
> code may _theoretically_ bloat it. (well, on the other hand, I've seen
> other watchdog drivers with many compatible devices listed in C code,
> and they didn't seem bloated at all)

We enable 16 bit access for "mediatek,mt6577-wdt" if we have a new SoC that also
needs 16 bit access, most probably we can just update the binding documentation
by adding the new SoC with a fallback to mt6577:

"mediatek,mt1234-wdt", "mediatek,mt6577-wdt": for MT1234

As no binding to mt1234 is present in the driver, the mt6577 one will be used.

Regards,
Matthias

Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Matthias Brugger]

On 02/02/2021 02:33, Boris Lysov wrote:
> В Sun, 31 Jan 2021 16:31:09 -0800
> Guenter Roeck <linux@roeck-us.net> пишет:
> 
>> We can't do this. With this flag enabled, the watchdog won't
>> support other SoCs, and there is nothing that prevents the flag
>> from being set for those SoCs.
>>
>> This has to be handled differently, without configuration
>> flag. Maybe use regmap for register addresses, [snip],
>> or use accessor functions in mtk_wdt_dev.
> 
> Thank you for reviewing my patch!
> 
> I will consider suggested fixes, and I will come up with better solution
> in V2. I'm beginner developer, and am still learning.
> 
>> use the compatible string to determine which regmap settings to use
> 
> I think relying on hardcoded "compatible string - settings" pairs in
> driver is not good. Whilst most Mediatek watchdogs I've seen use
> similar drivers, no one (except Mediatek itself, of course) knows for
> sure how many devices use 16-bit mode, and specifying each one in C
> code may _theoretically_ bloat it. (well, on the other hand, I've seen
> other watchdog drivers with many compatible devices listed in C code,
> and they didn't seem bloated at all)

We enable 16 bit access for "mediatek,mt6577-wdt" if we have a new SoC that also
needs 16 bit access, most probably we can just update the binding documentation
by adding the new SoC with a fallback to mt6577:

"mediatek,mt1234-wdt", "mediatek,mt6577-wdt": for MT1234

As no binding to mt1234 is present in the driver, the mt6577 one will be used.

Regards,
Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 3/3] watchdog: mtk_wdt: declare compatibility with mt6577

[Boris Lysov]

Support binding the mt6577 watchdog from OF.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 0ab3cbcf0d93..a6abb4e12fce 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -334,6 +334,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
+	{ .compatible = "mediatek,mt6577-wdt" },
 	{ .compatible = "mediatek,mt6589-wdt" },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ /* sentinel */ }
-- 
2.20.1

[PATCH 3/3] watchdog: mtk_wdt: declare compatibility with mt6577

[Boris Lysov]

Support binding the mt6577 watchdog from OF.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 0ab3cbcf0d93..a6abb4e12fce 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -334,6 +334,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
+	{ .compatible = "mediatek,mt6577-wdt" },
 	{ .compatible = "mediatek,mt6589-wdt" },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ /* sentinel */ }
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 0/3] watchdog: mtk_wdt: refactor code to support more watchdogs

[Boris Lysov]

This series aims to refactor existing mtk_wdt driver by making some
constants dependent on a particular SoC. It is done because some mtk
watchdogs, while working in same manner, have slightly different
parameters such as specific register unlock key value and an offset of a
particular register field.

This patch set adds support for mt6577 watchdog.

Accepting these patches will make ground for submitting additional
changes related to the mainline support of mt6577 (and other compatible
SoCs) in future.

Proposed patches successfully pass all `checkpatch.pl` checks and don't
yield compiler warnings.

Resulting code has been thoroughly tested multiple times for hours on
real hardware (mt6577, mt6589) to ensure that proposed changes are
working properly.

Changes since v1 [1]:
- a complete rewrite to get rid of the configuration flags which made
  the watchdog not support other SoCs; suggested [2] by Guenter Roeck.

[1] https://lore.kernel.org/linux-mediatek/20210131234425.9773-1-arzamas-16@mail.ee/
[2] https://lore.kernel.org/linux-mediatek/050f2f8e-9c3c-10e3-05ef-cd84e949b98f@roeck-us.net/

Boris Lysov (3):
  watchdog: mtk_wdt: Refactor code to support more SoCs
  dt-bindings: watchdog: mediatek: add support for mt6577 SoC
  watchdog: mtk_wdt: add support for mt6577

 .../devicetree/bindings/watchdog/mtk-wdt.txt  |  1 +
 drivers/watchdog/mtk_wdt.c                    | 88 +++++++++++++++----
 2 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH v2 0/3] watchdog: mtk_wdt: refactor code to support more watchdogs

[Boris Lysov]

This series aims to refactor existing mtk_wdt driver by making some
constants dependent on a particular SoC. It is done because some mtk
watchdogs, while working in same manner, have slightly different
parameters such as specific register unlock key value and an offset of a
particular register field.

This patch set adds support for mt6577 watchdog.

Accepting these patches will make ground for submitting additional
changes related to the mainline support of mt6577 (and other compatible
SoCs) in future.

Proposed patches successfully pass all `checkpatch.pl` checks and don't
yield compiler warnings.

Resulting code has been thoroughly tested multiple times for hours on
real hardware (mt6577, mt6589) to ensure that proposed changes are
working properly.

Changes since v1 [1]:
- a complete rewrite to get rid of the configuration flags which made
  the watchdog not support other SoCs; suggested [2] by Guenter Roeck.

[1] https://lore.kernel.org/linux-mediatek/20210131234425.9773-1-arzamas-16@mail.ee/
[2] https://lore.kernel.org/linux-mediatek/050f2f8e-9c3c-10e3-05ef-cd84e949b98f@roeck-us.net/

Boris Lysov (3):
  watchdog: mtk_wdt: Refactor code to support more SoCs
  dt-bindings: watchdog: mediatek: add support for mt6577 SoC
  watchdog: mtk_wdt: add support for mt6577

 .../devicetree/bindings/watchdog/mtk-wdt.txt  |  1 +
 drivers/watchdog/mtk_wdt.c                    | 88 +++++++++++++++----
 2 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Boris Lysov]

This patch makes some constants SoC-dependent to support more watchdogs
in the future. It adds shifts of WDT_MODE_KEY and SWSYSRST_KEY to
mtk_wdt_data struct. This is done to bring support for various Mediatek
watchdogs which use same register structure but slightly different field
offsets in the UNLOCK_KEY registers. For example, mt6577 watchdog has
WDT_MODE_KEY and SWSYSRST_KEY at [15:8] instead of currently (and only)
supported [31:24].
Moreover, this patch adds SWSYSRST_KEY value to mtk_wdt_data because this
value also depends on specific SoC watchdog, for example mt6577 uses 0x15
instead of 0x88.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 76 ++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 97ca993bd009..9d3091b12c06 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -44,13 +44,27 @@
 #define WDT_MODE_IRQ_EN		(1 << 3)
 #define WDT_MODE_AUTO_START	(1 << 4)
 #define WDT_MODE_DUAL_EN	(1 << 6)
-#define WDT_MODE_KEY		0x22000000
+#define WDT_MODE_KEY		0x22
 
 #define WDT_SWRST		0x14
 #define WDT_SWRST_KEY		0x1209
-
 #define WDT_SWSYSRST		0x18U
-#define WDT_SWSYS_RST_KEY	0x88000000
+
+#define MT2712_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT2712_SWSYSRST_KEY		0x88
+
+#define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT6589_SWSYSRST_KEY		0x88
+
+#define MT8183_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8183_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8183_SWSYSRST_KEY		0x88
+
+#define MT8192_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8192_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8192_SWSYSRST_KEY		0x88
 
 #define DRV_NAME		"mtk-wdt"
 #define DRV_VERSION		"1.0"
@@ -60,6 +74,7 @@ static unsigned int timeout;
 
 struct mtk_wdt_dev {
 	struct watchdog_device wdt_dev;
+	const struct mtk_wdt_data *data;
 	void __iomem *wdt_base;
 	spinlock_t lock; /* protects WDT_SWSYSRST reg */
 	struct reset_controller_dev rcdev;
@@ -67,18 +82,37 @@ struct mtk_wdt_dev {
 
 struct mtk_wdt_data {
 	int toprgu_sw_rst_num;
+	u8 wdt_mode_key_shift;
+	u8 wdt_swsys_rst_key;
+	u8 wdt_swsys_rst_key_shift;
 };
 
 static const struct mtk_wdt_data mt2712_data = {
-	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT2712_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT2712_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT2712_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
+};
+
+static const struct mtk_wdt_data mt6589_data = {
+	.toprgu_sw_rst_num =		-1,
+	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT6589_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT6589_SWSYSRST_KEY,
 };
 
 static const struct mtk_wdt_data mt8183_data = {
-	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT8183_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT8183_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT8183_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT8183_SWSYSRST_KEY,
 };
 
 static const struct mtk_wdt_data mt8192_data = {
-	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT8192_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT8192_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT8192_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT8192_SWSYSRST_KEY,
 };
 
 static int toprgu_reset_update(struct reset_controller_dev *rcdev,
@@ -86,20 +120,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 {
 	unsigned int tmp;
 	unsigned long flags;
-	struct mtk_wdt_dev *data =
+	struct mtk_wdt_dev *wdev =
 		 container_of(rcdev, struct mtk_wdt_dev, rcdev);
 
-	spin_lock_irqsave(&data->lock, flags);
+	spin_lock_irqsave(&wdev->lock, flags);
 
-	tmp = readl(data->wdt_base + WDT_SWSYSRST);
+	tmp = readl(wdev->wdt_base + WDT_SWSYSRST);
 	if (assert)
 		tmp |= BIT(id);
 	else
 		tmp &= ~BIT(id);
-	tmp |= WDT_SWSYS_RST_KEY;
-	writel(tmp, data->wdt_base + WDT_SWSYSRST);
+	tmp |= wdev->data->wdt_swsys_rst_key << wdev->data->wdt_swsys_rst_key_shift;
+	writel(tmp, wdev->wdt_base + WDT_SWSYSRST);
 
-	spin_unlock_irqrestore(&data->lock, flags);
+	spin_unlock_irqrestore(&wdev->lock, flags);
 
 	return 0;
 }
@@ -221,7 +255,7 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
 
 	reg = readl(wdt_base + WDT_MODE);
 	reg &= ~WDT_MODE_EN;
-	reg |= WDT_MODE_KEY;
+	reg |= WDT_MODE_KEY << mtk_wdt->data->wdt_mode_key_shift;
 	iowrite32(reg, wdt_base + WDT_MODE);
 
 	return 0;
@@ -240,7 +274,7 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
 
 	reg = ioread32(wdt_base + WDT_MODE);
 	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
-	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
+	reg |= (WDT_MODE_EN | (WDT_MODE_KEY << mtk_wdt->data->wdt_mode_key_shift));
 	iowrite32(reg, wdt_base + WDT_MODE);
 
 	return 0;
@@ -266,7 +300,6 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mtk_wdt_dev *mtk_wdt;
-	const struct mtk_wdt_data *wdt_data;
 	int err;
 
 	mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
@@ -279,6 +312,12 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(mtk_wdt->wdt_base))
 		return PTR_ERR(mtk_wdt->wdt_base);
 
+	mtk_wdt->data = of_device_get_match_data(dev);
+	if (!mtk_wdt->data) {
+		dev_err(dev, "watchdog data is not defined\n");
+		return -EINVAL;
+	}
+
 	mtk_wdt->wdt_dev.info = &mtk_wdt_info;
 	mtk_wdt->wdt_dev.ops = &mtk_wdt_ops;
 	mtk_wdt->wdt_dev.timeout = WDT_MAX_TIMEOUT;
@@ -302,10 +341,9 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 	dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
 		 mtk_wdt->wdt_dev.timeout, nowayout);
 
-	wdt_data = of_device_get_match_data(dev);
-	if (wdt_data) {
+	if (mtk_wdt->data->toprgu_sw_rst_num > -1) {
 		err = toprgu_register_reset_controller(pdev,
-						       wdt_data->toprgu_sw_rst_num);
+						       mtk_wdt->data->toprgu_sw_rst_num);
 		if (err)
 			return err;
 	}
@@ -338,7 +376,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
-	{ .compatible = "mediatek,mt6589-wdt" },
+	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ .compatible = "mediatek,mt8192-wdt", .data = &mt8192_data },
 	{ /* sentinel */ }
-- 
2.20.1

[PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Boris Lysov]

This patch makes some constants SoC-dependent to support more watchdogs
in the future. It adds shifts of WDT_MODE_KEY and SWSYSRST_KEY to
mtk_wdt_data struct. This is done to bring support for various Mediatek
watchdogs which use same register structure but slightly different field
offsets in the UNLOCK_KEY registers. For example, mt6577 watchdog has
WDT_MODE_KEY and SWSYSRST_KEY at [15:8] instead of currently (and only)
supported [31:24].
Moreover, this patch adds SWSYSRST_KEY value to mtk_wdt_data because this
value also depends on specific SoC watchdog, for example mt6577 uses 0x15
instead of 0x88.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 76 ++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 97ca993bd009..9d3091b12c06 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -44,13 +44,27 @@
 #define WDT_MODE_IRQ_EN		(1 << 3)
 #define WDT_MODE_AUTO_START	(1 << 4)
 #define WDT_MODE_DUAL_EN	(1 << 6)
-#define WDT_MODE_KEY		0x22000000
+#define WDT_MODE_KEY		0x22
 
 #define WDT_SWRST		0x14
 #define WDT_SWRST_KEY		0x1209
-
 #define WDT_SWSYSRST		0x18U
-#define WDT_SWSYS_RST_KEY	0x88000000
+
+#define MT2712_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT2712_SWSYSRST_KEY		0x88
+
+#define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT6589_SWSYSRST_KEY		0x88
+
+#define MT8183_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8183_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8183_SWSYSRST_KEY		0x88
+
+#define MT8192_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8192_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
+#define MT8192_SWSYSRST_KEY		0x88
 
 #define DRV_NAME		"mtk-wdt"
 #define DRV_VERSION		"1.0"
@@ -60,6 +74,7 @@ static unsigned int timeout;
 
 struct mtk_wdt_dev {
 	struct watchdog_device wdt_dev;
+	const struct mtk_wdt_data *data;
 	void __iomem *wdt_base;
 	spinlock_t lock; /* protects WDT_SWSYSRST reg */
 	struct reset_controller_dev rcdev;
@@ -67,18 +82,37 @@ struct mtk_wdt_dev {
 
 struct mtk_wdt_data {
 	int toprgu_sw_rst_num;
+	u8 wdt_mode_key_shift;
+	u8 wdt_swsys_rst_key;
+	u8 wdt_swsys_rst_key_shift;
 };
 
 static const struct mtk_wdt_data mt2712_data = {
-	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT2712_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT2712_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT2712_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
+};
+
+static const struct mtk_wdt_data mt6589_data = {
+	.toprgu_sw_rst_num =		-1,
+	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT6589_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT6589_SWSYSRST_KEY,
 };
 
 static const struct mtk_wdt_data mt8183_data = {
-	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT8183_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT8183_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT8183_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT8183_SWSYSRST_KEY,
 };
 
 static const struct mtk_wdt_data mt8192_data = {
-	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
+	.toprgu_sw_rst_num =		MT8192_TOPRGU_SW_RST_NUM,
+	.wdt_mode_key_shift =		MT8192_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT8192_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT8192_SWSYSRST_KEY,
 };
 
 static int toprgu_reset_update(struct reset_controller_dev *rcdev,
@@ -86,20 +120,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 {
 	unsigned int tmp;
 	unsigned long flags;
-	struct mtk_wdt_dev *data =
+	struct mtk_wdt_dev *wdev =
 		 container_of(rcdev, struct mtk_wdt_dev, rcdev);
 
-	spin_lock_irqsave(&data->lock, flags);
+	spin_lock_irqsave(&wdev->lock, flags);
 
-	tmp = readl(data->wdt_base + WDT_SWSYSRST);
+	tmp = readl(wdev->wdt_base + WDT_SWSYSRST);
 	if (assert)
 		tmp |= BIT(id);
 	else
 		tmp &= ~BIT(id);
-	tmp |= WDT_SWSYS_RST_KEY;
-	writel(tmp, data->wdt_base + WDT_SWSYSRST);
+	tmp |= wdev->data->wdt_swsys_rst_key << wdev->data->wdt_swsys_rst_key_shift;
+	writel(tmp, wdev->wdt_base + WDT_SWSYSRST);
 
-	spin_unlock_irqrestore(&data->lock, flags);
+	spin_unlock_irqrestore(&wdev->lock, flags);
 
 	return 0;
 }
@@ -221,7 +255,7 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev)
 
 	reg = readl(wdt_base + WDT_MODE);
 	reg &= ~WDT_MODE_EN;
-	reg |= WDT_MODE_KEY;
+	reg |= WDT_MODE_KEY << mtk_wdt->data->wdt_mode_key_shift;
 	iowrite32(reg, wdt_base + WDT_MODE);
 
 	return 0;
@@ -240,7 +274,7 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev)
 
 	reg = ioread32(wdt_base + WDT_MODE);
 	reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
-	reg |= (WDT_MODE_EN | WDT_MODE_KEY);
+	reg |= (WDT_MODE_EN | (WDT_MODE_KEY << mtk_wdt->data->wdt_mode_key_shift));
 	iowrite32(reg, wdt_base + WDT_MODE);
 
 	return 0;
@@ -266,7 +300,6 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mtk_wdt_dev *mtk_wdt;
-	const struct mtk_wdt_data *wdt_data;
 	int err;
 
 	mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
@@ -279,6 +312,12 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(mtk_wdt->wdt_base))
 		return PTR_ERR(mtk_wdt->wdt_base);
 
+	mtk_wdt->data = of_device_get_match_data(dev);
+	if (!mtk_wdt->data) {
+		dev_err(dev, "watchdog data is not defined\n");
+		return -EINVAL;
+	}
+
 	mtk_wdt->wdt_dev.info = &mtk_wdt_info;
 	mtk_wdt->wdt_dev.ops = &mtk_wdt_ops;
 	mtk_wdt->wdt_dev.timeout = WDT_MAX_TIMEOUT;
@@ -302,10 +341,9 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 	dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
 		 mtk_wdt->wdt_dev.timeout, nowayout);
 
-	wdt_data = of_device_get_match_data(dev);
-	if (wdt_data) {
+	if (mtk_wdt->data->toprgu_sw_rst_num > -1) {
 		err = toprgu_register_reset_controller(pdev,
-						       wdt_data->toprgu_sw_rst_num);
+						       mtk_wdt->data->toprgu_sw_rst_num);
 		if (err)
 			return err;
 	}
@@ -338,7 +376,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
-	{ .compatible = "mediatek,mt6589-wdt" },
+	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ .compatible = "mediatek,mt8192-wdt", .data = &mt8192_data },
 	{ /* sentinel */ }
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Guenter Roeck]

On 5/9/21 2:17 PM, Boris Lysov wrote:
> This patch makes some constants SoC-dependent to support more watchdogs
> in the future. It adds shifts of WDT_MODE_KEY and SWSYSRST_KEY to
> mtk_wdt_data struct. This is done to bring support for various Mediatek
> watchdogs which use same register structure but slightly different field
> offsets in the UNLOCK_KEY registers. For example, mt6577 watchdog has
> WDT_MODE_KEY and SWSYSRST_KEY at [15:8] instead of currently (and only)
> supported [31:24].
> Moreover, this patch adds SWSYSRST_KEY value to mtk_wdt_data because this
> value also depends on specific SoC watchdog, for example mt6577 uses 0x15
> instead of 0x88.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>   drivers/watchdog/mtk_wdt.c | 76 ++++++++++++++++++++++++++++----------
>   1 file changed, 57 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 97ca993bd009..9d3091b12c06 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -44,13 +44,27 @@
>   #define WDT_MODE_IRQ_EN		(1 << 3)
>   #define WDT_MODE_AUTO_START	(1 << 4)
>   #define WDT_MODE_DUAL_EN	(1 << 6)
> -#define WDT_MODE_KEY		0x22000000
> +#define WDT_MODE_KEY		0x22
>   
>   #define WDT_SWRST		0x14
>   #define WDT_SWRST_KEY		0x1209
> -
>   #define WDT_SWSYSRST		0x18U
> -#define WDT_SWSYS_RST_KEY	0x88000000
> +
> +#define MT2712_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT2712_SWSYSRST_KEY		0x88
> +
> +#define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT6589_SWSYSRST_KEY		0x88
> +
> +#define MT8183_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8183_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8183_SWSYSRST_KEY		0x88
> +
> +#define MT8192_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8192_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8192_SWSYSRST_KEY		0x88
>   
>   #define DRV_NAME		"mtk-wdt"
>   #define DRV_VERSION		"1.0"
> @@ -60,6 +74,7 @@ static unsigned int timeout;
>   
>   struct mtk_wdt_dev {
>   	struct watchdog_device wdt_dev;
> +	const struct mtk_wdt_data *data;
>   	void __iomem *wdt_base;
>   	spinlock_t lock; /* protects WDT_SWSYSRST reg */
>   	struct reset_controller_dev rcdev;
> @@ -67,18 +82,37 @@ struct mtk_wdt_dev {
>   
>   struct mtk_wdt_data {
>   	int toprgu_sw_rst_num;
> +	u8 wdt_mode_key_shift;
> +	u8 wdt_swsys_rst_key;
> +	u8 wdt_swsys_rst_key_shift;
>   };
>   
>   static const struct mtk_wdt_data mt2712_data = {
> -	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT2712_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT2712_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT2712_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
> +};
> +
> +static const struct mtk_wdt_data mt6589_data = {
> +	.toprgu_sw_rst_num =		-1,
> +	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT6589_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT6589_SWSYSRST_KEY,
>   };
>   
>   static const struct mtk_wdt_data mt8183_data = {
> -	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT8183_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT8183_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT8183_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT8183_SWSYSRST_KEY,
>   };
>   
>   static const struct mtk_wdt_data mt8192_data = {
> -	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT8192_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT8192_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT8192_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT8192_SWSYSRST_KEY,
>   };
>   
>   static int toprgu_reset_update(struct reset_controller_dev *rcdev,
> @@ -86,20 +120,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   {
>   	unsigned int tmp;
>   	unsigned long flags;
> -	struct mtk_wdt_dev *data =
> +	struct mtk_wdt_dev *wdev =

Please do not rename variables. If you dislike that the name matches the name
of the newly introduced structure element, find a different name for that.

Thanks,
Guenter

Re: [PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Guenter Roeck]

On 5/9/21 2:17 PM, Boris Lysov wrote:
> This patch makes some constants SoC-dependent to support more watchdogs
> in the future. It adds shifts of WDT_MODE_KEY and SWSYSRST_KEY to
> mtk_wdt_data struct. This is done to bring support for various Mediatek
> watchdogs which use same register structure but slightly different field
> offsets in the UNLOCK_KEY registers. For example, mt6577 watchdog has
> WDT_MODE_KEY and SWSYSRST_KEY at [15:8] instead of currently (and only)
> supported [31:24].
> Moreover, this patch adds SWSYSRST_KEY value to mtk_wdt_data because this
> value also depends on specific SoC watchdog, for example mt6577 uses 0x15
> instead of 0x88.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>   drivers/watchdog/mtk_wdt.c | 76 ++++++++++++++++++++++++++++----------
>   1 file changed, 57 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 97ca993bd009..9d3091b12c06 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -44,13 +44,27 @@
>   #define WDT_MODE_IRQ_EN		(1 << 3)
>   #define WDT_MODE_AUTO_START	(1 << 4)
>   #define WDT_MODE_DUAL_EN	(1 << 6)
> -#define WDT_MODE_KEY		0x22000000
> +#define WDT_MODE_KEY		0x22
>   
>   #define WDT_SWRST		0x14
>   #define WDT_SWRST_KEY		0x1209
> -
>   #define WDT_SWSYSRST		0x18U
> -#define WDT_SWSYS_RST_KEY	0x88000000
> +
> +#define MT2712_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT2712_SWSYSRST_KEY		0x88
> +
> +#define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT6589_SWSYSRST_KEY		0x88
> +
> +#define MT8183_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8183_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8183_SWSYSRST_KEY		0x88
> +
> +#define MT8192_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8192_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
> +#define MT8192_SWSYSRST_KEY		0x88
>   
>   #define DRV_NAME		"mtk-wdt"
>   #define DRV_VERSION		"1.0"
> @@ -60,6 +74,7 @@ static unsigned int timeout;
>   
>   struct mtk_wdt_dev {
>   	struct watchdog_device wdt_dev;
> +	const struct mtk_wdt_data *data;
>   	void __iomem *wdt_base;
>   	spinlock_t lock; /* protects WDT_SWSYSRST reg */
>   	struct reset_controller_dev rcdev;
> @@ -67,18 +82,37 @@ struct mtk_wdt_dev {
>   
>   struct mtk_wdt_data {
>   	int toprgu_sw_rst_num;
> +	u8 wdt_mode_key_shift;
> +	u8 wdt_swsys_rst_key;
> +	u8 wdt_swsys_rst_key_shift;
>   };
>   
>   static const struct mtk_wdt_data mt2712_data = {
> -	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT2712_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT2712_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT2712_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
> +};
> +
> +static const struct mtk_wdt_data mt6589_data = {
> +	.toprgu_sw_rst_num =		-1,
> +	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT6589_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT6589_SWSYSRST_KEY,
>   };
>   
>   static const struct mtk_wdt_data mt8183_data = {
> -	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT8183_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT8183_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT8183_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT8183_SWSYSRST_KEY,
>   };
>   
>   static const struct mtk_wdt_data mt8192_data = {
> -	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
> +	.toprgu_sw_rst_num =		MT8192_TOPRGU_SW_RST_NUM,
> +	.wdt_mode_key_shift =		MT8192_WDT_MODE_KEY_SHIFT,
> +	.wdt_swsys_rst_key_shift =	MT8192_SWSYSRST_KEY_SHIFT,
> +	.wdt_swsys_rst_key =		MT8192_SWSYSRST_KEY,
>   };
>   
>   static int toprgu_reset_update(struct reset_controller_dev *rcdev,
> @@ -86,20 +120,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   {
>   	unsigned int tmp;
>   	unsigned long flags;
> -	struct mtk_wdt_dev *data =
> +	struct mtk_wdt_dev *wdev =

Please do not rename variables. If you dislike that the name matches the name
of the newly introduced structure element, find a different name for that.

Thanks,
Guenter

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Boris Lysov]

Hello,

On Mon, 10 May 2021 06:21:54 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> > -	struct mtk_wdt_dev *data =
> > +	struct mtk_wdt_dev *wdev =  
> 
> Please do not rename variables. If you dislike that the name matches the name
> of the newly introduced structure element, find a different name for that.
I will fix it in v3.

Re: [PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs

[Boris Lysov]

Hello,

On Mon, 10 May 2021 06:21:54 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> > -	struct mtk_wdt_dev *data =
> > +	struct mtk_wdt_dev *wdev =  
> 
> Please do not rename variables. If you dislike that the name matches the name
> of the newly introduced structure element, find a different name for that.
I will fix it in v3.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 2/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Boris Lysov]

Add support for Mediatek mt6577 SoC to device tree binding
documentation.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index e36ba60de829..f5a5404523a3 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible should contain:
 	"mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
 	"mediatek,mt2712-wdt": for MT2712
+	"mediatek,mt6577-wdt": for MT6577
 	"mediatek,mt6589-wdt": for MT6589
 	"mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
 	"mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
-- 
2.20.1

[PATCH v2 2/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Boris Lysov]

Add support for Mediatek mt6577 SoC to device tree binding
documentation.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index e36ba60de829..f5a5404523a3 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible should contain:
 	"mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
 	"mediatek,mt2712-wdt": for MT2712
+	"mediatek,mt6577-wdt": for MT6577
 	"mediatek,mt6589-wdt": for MT6589
 	"mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
 	"mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 2/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Rob Herring]

On Mon, 10 May 2021 00:17:01 +0300, Boris Lysov wrote:
> Add support for Mediatek mt6577 SoC to device tree binding
> documentation.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>  Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC

[Rob Herring]

On Mon, 10 May 2021 00:17:01 +0300, Boris Lysov wrote:
> Add support for Mediatek mt6577 SoC to device tree binding
> documentation.
> 
> Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
> ---
>  Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 3/3] watchdog: mtk_wdt: add support for mt6577

[Boris Lysov]

This patch adds support for watchdog used by mt6577 and related SoCs such
as mt6575 and mt8317. These watchdogs are known for having shifted WDT_MODE
and SWSYSRST registers and using different SWSYSRST_KEY value.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 9d3091b12c06..830113843ce9 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -54,6 +54,10 @@
 #define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT2712_SWSYSRST_KEY		0x88
 
+#define MT6577_WDT_MODE_KEY_SHIFT	8		// unlock_key [15:8]
+#define MT6577_SWSYSRST_KEY_SHIFT	8		// unlock_key [15:8]
+#define MT6577_SWSYSRST_KEY		0x15
+
 #define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT6589_SWSYSRST_KEY		0x88
@@ -94,6 +98,13 @@ static const struct mtk_wdt_data mt2712_data = {
 	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
 };
 
+static const struct mtk_wdt_data mt6577_data = {
+	.toprgu_sw_rst_num =		-1,
+	.wdt_mode_key_shift =		MT6577_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT6577_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT6577_SWSYSRST_KEY,
+};
+
 static const struct mtk_wdt_data mt6589_data = {
 	.toprgu_sw_rst_num =		-1,
 	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
@@ -376,6 +387,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
+	{ .compatible = "mediatek,mt6577-wdt", .data = &mt6577_data },
 	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ .compatible = "mediatek,mt8192-wdt", .data = &mt8192_data },
-- 
2.20.1

[PATCH v2 3/3] watchdog: mtk_wdt: add support for mt6577

[Boris Lysov]

This patch adds support for watchdog used by mt6577 and related SoCs such
as mt6575 and mt8317. These watchdogs are known for having shifted WDT_MODE
and SWSYSRST registers and using different SWSYSRST_KEY value.

Signed-off-by: Boris Lysov <arzamas-16@mail.ee>
---
 drivers/watchdog/mtk_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 9d3091b12c06..830113843ce9 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -54,6 +54,10 @@
 #define MT2712_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT2712_SWSYSRST_KEY		0x88
 
+#define MT6577_WDT_MODE_KEY_SHIFT	8		// unlock_key [15:8]
+#define MT6577_SWSYSRST_KEY_SHIFT	8		// unlock_key [15:8]
+#define MT6577_SWSYSRST_KEY		0x15
+
 #define MT6589_WDT_MODE_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT6589_SWSYSRST_KEY_SHIFT	24		// unlock_key [31:24]
 #define MT6589_SWSYSRST_KEY		0x88
@@ -94,6 +98,13 @@ static const struct mtk_wdt_data mt2712_data = {
 	.wdt_swsys_rst_key =		MT2712_SWSYSRST_KEY,
 };
 
+static const struct mtk_wdt_data mt6577_data = {
+	.toprgu_sw_rst_num =		-1,
+	.wdt_mode_key_shift =		MT6577_WDT_MODE_KEY_SHIFT,
+	.wdt_swsys_rst_key_shift =	MT6577_SWSYSRST_KEY_SHIFT,
+	.wdt_swsys_rst_key =		MT6577_SWSYSRST_KEY,
+};
+
 static const struct mtk_wdt_data mt6589_data = {
 	.toprgu_sw_rst_num =		-1,
 	.wdt_mode_key_shift =		MT6589_WDT_MODE_KEY_SHIFT,
@@ -376,6 +387,7 @@ static int mtk_wdt_resume(struct device *dev)
 
 static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
+	{ .compatible = "mediatek,mt6577-wdt", .data = &mt6577_data },
 	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ .compatible = "mediatek,mt8192-wdt", .data = &mt8192_data },
-- 
2.20.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek