| Message ID | 20210131234425.9773-3-arzamas-16@mail.ee (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | watchdog: mtk_wdt: add support for mt6577 | expand |
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; > } >
В 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
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 >
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 --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; }
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(-)