diff mbox series

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

Message ID 20210131234425.9773-3-arzamas-16@mail.ee (mailing list archive)
State New
Headers show
Series watchdog: mtk_wdt: add support for mt6577 | expand

Commit Message

Boris Lysov Jan. 31, 2021, 11:44 p.m. UTC
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(-)

Comments

Guenter Roeck Feb. 1, 2021, 12:31 a.m. UTC | #1
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;
>  }
>
Boris Lysov Feb. 2, 2021, 1:33 a.m. UTC | #2
В 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
Guenter Roeck Feb. 2, 2021, 1:55 a.m. UTC | #3
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
>
Matthias Brugger Feb. 10, 2021, 11:37 a.m. UTC | #4
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
diff mbox series

Patch

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;
 }