Re: [PATCH v6 08/24] watchdog: jz4740: Use WDT clock provided by TCU driver
On Thu, Aug 09, 2018 at 11:43:58PM +0200, Paul Cercueil wrote: > Instead of requesting the "ext" clock and handling the watchdog clock > divider and gating in the watchdog driver, we now request and use the > "wdt" clock that is supplied by the ingenic-timer "TCU" driver. > > The major benefit is that the watchdog's clock rate and parent can now > be specified from within devicetree, instead of hardcoded in the driver. > > Also, this driver won't poke anymore into the TCU registers to > enable/disable the clock, as this is now handled by the TCU driver. > > On the bad side, we break the ABI with devicetree - as we now request a > different clock. In this very specific case it is still okay, as every > Ingenic JZ47xx-based board out there compile the devicetree within the > kernel; so it's still time to push breaking changes, in order to get a > clean devicetree that won't break once it musn't. > > Signed-off-by: Paul Cercueil Reviewed-by: Guenter Roeck > --- > drivers/watchdog/Kconfig | 2 + > drivers/watchdog/jz4740_wdt.c | 86 > +-- > 2 files changed, 36 insertions(+), 52 deletions(-) > > v5: New patch > > v6: - Split regmap change to new patch 09/24 > - The code now sets the WDT clock to the smallest rate possible and >calculates the maximum timeout from that > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9af07fd92763..834222abbbdb 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1476,7 +1476,9 @@ config INDYDOG > config JZ4740_WDT > tristate "Ingenic jz4740 SoC hardware watchdog" > depends on MACH_JZ4740 || MACH_JZ4780 > + depends on COMMON_CLK > select WATCHDOG_CORE > + select INGENIC_TIMER > help > Hardware driver for the built-in watchdog timer on Ingenic jz4740 > SoCs. > > diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c > index ec4d99a830ba..1d504ecf45e1 100644 > --- a/drivers/watchdog/jz4740_wdt.c > +++ b/drivers/watchdog/jz4740_wdt.c > @@ -26,25 +26,9 @@ > #include > #include > > -#include > - > #define JZ_REG_WDT_TIMER_DATA 0x0 > #define JZ_REG_WDT_COUNTER_ENABLE 0x4 > #define JZ_REG_WDT_TIMER_COUNTER 0x8 > -#define JZ_REG_WDT_TIMER_CONTROL 0xC > - > -#define JZ_WDT_CLOCK_PCLK 0x1 > -#define JZ_WDT_CLOCK_RTC 0x2 > -#define JZ_WDT_CLOCK_EXT 0x4 > - > -#define JZ_WDT_CLOCK_DIV_SHIFT 3 > - > -#define JZ_WDT_CLOCK_DIV_1(0 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_4(1 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_16 (2 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_64 (3 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_256 (4 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_1024 (5 << JZ_WDT_CLOCK_DIV_SHIFT) > > #define DEFAULT_HEARTBEAT 5 > #define MAX_HEARTBEAT 2048 > @@ -65,7 +49,8 @@ MODULE_PARM_DESC(heartbeat, > struct jz4740_wdt_drvdata { > struct watchdog_device wdt; > void __iomem *base; > - struct clk *rtc_clk; > + struct clk *clk; > + unsigned long clk_rate; > }; > > static int jz4740_wdt_ping(struct watchdog_device *wdt_dev) > @@ -80,31 +65,12 @@ static int jz4740_wdt_set_timeout(struct watchdog_device > *wdt_dev, > unsigned int new_timeout) > { > struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev); > - unsigned int rtc_clk_rate; > - unsigned int timeout_value; > - unsigned short clock_div = JZ_WDT_CLOCK_DIV_1; > - > - rtc_clk_rate = clk_get_rate(drvdata->rtc_clk); > - > - timeout_value = rtc_clk_rate * new_timeout; > - while (timeout_value > 0x) { > - if (clock_div == JZ_WDT_CLOCK_DIV_1024) { > - /* Requested timeout too high; > - * use highest possible value. */ > - timeout_value = 0x; > - break; > - } > - timeout_value >>= 2; > - clock_div += (1 << JZ_WDT_CLOCK_DIV_SHIFT); > - } > + u16 timeout_value = (u16)(drvdata->clk_rate * new_timeout); > > writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE); > - writew(clock_div, drvdata->base + JZ_REG_WDT_TIMER_CONTROL); > > writew((u16)timeout_value, drvdata->base + JZ_REG_WDT_TIMER_DATA); > writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER); > - writew(clock_div | JZ_WDT_CLOCK_RTC, > - drvdata->base + JZ_REG_WDT_TIMER_CONTROL); > > writeb(0x1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE); > > @@ -114,7 +80,13 @@ static int jz4740_wdt_set_timeout(struct watchdog_device > *wdt_dev, > > static int jz4740_wdt_start(struct watchdog_device *wdt_dev) > { > - jz4740_timer_enable_watchdog(); > + struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev); > + int ret; > + > + ret = clk_prepare_enable(drvdata->clk); > + if (ret) > +
Re: [PATCH v6 08/24] watchdog: jz4740: Use WDT clock provided by TCU driver
On Thu, Aug 09, 2018 at 11:43:58PM +0200, Paul Cercueil wrote: > Instead of requesting the "ext" clock and handling the watchdog clock > divider and gating in the watchdog driver, we now request and use the > "wdt" clock that is supplied by the ingenic-timer "TCU" driver. > > The major benefit is that the watchdog's clock rate and parent can now > be specified from within devicetree, instead of hardcoded in the driver. > > Also, this driver won't poke anymore into the TCU registers to > enable/disable the clock, as this is now handled by the TCU driver. > > On the bad side, we break the ABI with devicetree - as we now request a > different clock. In this very specific case it is still okay, as every > Ingenic JZ47xx-based board out there compile the devicetree within the > kernel; so it's still time to push breaking changes, in order to get a > clean devicetree that won't break once it musn't. > > Signed-off-by: Paul Cercueil Reviewed-by: Guenter Roeck > --- > drivers/watchdog/Kconfig | 2 + > drivers/watchdog/jz4740_wdt.c | 86 > +-- > 2 files changed, 36 insertions(+), 52 deletions(-) > > v5: New patch > > v6: - Split regmap change to new patch 09/24 > - The code now sets the WDT clock to the smallest rate possible and >calculates the maximum timeout from that > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9af07fd92763..834222abbbdb 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1476,7 +1476,9 @@ config INDYDOG > config JZ4740_WDT > tristate "Ingenic jz4740 SoC hardware watchdog" > depends on MACH_JZ4740 || MACH_JZ4780 > + depends on COMMON_CLK > select WATCHDOG_CORE > + select INGENIC_TIMER > help > Hardware driver for the built-in watchdog timer on Ingenic jz4740 > SoCs. > > diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c > index ec4d99a830ba..1d504ecf45e1 100644 > --- a/drivers/watchdog/jz4740_wdt.c > +++ b/drivers/watchdog/jz4740_wdt.c > @@ -26,25 +26,9 @@ > #include > #include > > -#include > - > #define JZ_REG_WDT_TIMER_DATA 0x0 > #define JZ_REG_WDT_COUNTER_ENABLE 0x4 > #define JZ_REG_WDT_TIMER_COUNTER 0x8 > -#define JZ_REG_WDT_TIMER_CONTROL 0xC > - > -#define JZ_WDT_CLOCK_PCLK 0x1 > -#define JZ_WDT_CLOCK_RTC 0x2 > -#define JZ_WDT_CLOCK_EXT 0x4 > - > -#define JZ_WDT_CLOCK_DIV_SHIFT 3 > - > -#define JZ_WDT_CLOCK_DIV_1(0 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_4(1 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_16 (2 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_64 (3 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_256 (4 << JZ_WDT_CLOCK_DIV_SHIFT) > -#define JZ_WDT_CLOCK_DIV_1024 (5 << JZ_WDT_CLOCK_DIV_SHIFT) > > #define DEFAULT_HEARTBEAT 5 > #define MAX_HEARTBEAT 2048 > @@ -65,7 +49,8 @@ MODULE_PARM_DESC(heartbeat, > struct jz4740_wdt_drvdata { > struct watchdog_device wdt; > void __iomem *base; > - struct clk *rtc_clk; > + struct clk *clk; > + unsigned long clk_rate; > }; > > static int jz4740_wdt_ping(struct watchdog_device *wdt_dev) > @@ -80,31 +65,12 @@ static int jz4740_wdt_set_timeout(struct watchdog_device > *wdt_dev, > unsigned int new_timeout) > { > struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev); > - unsigned int rtc_clk_rate; > - unsigned int timeout_value; > - unsigned short clock_div = JZ_WDT_CLOCK_DIV_1; > - > - rtc_clk_rate = clk_get_rate(drvdata->rtc_clk); > - > - timeout_value = rtc_clk_rate * new_timeout; > - while (timeout_value > 0x) { > - if (clock_div == JZ_WDT_CLOCK_DIV_1024) { > - /* Requested timeout too high; > - * use highest possible value. */ > - timeout_value = 0x; > - break; > - } > - timeout_value >>= 2; > - clock_div += (1 << JZ_WDT_CLOCK_DIV_SHIFT); > - } > + u16 timeout_value = (u16)(drvdata->clk_rate * new_timeout); > > writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE); > - writew(clock_div, drvdata->base + JZ_REG_WDT_TIMER_CONTROL); > > writew((u16)timeout_value, drvdata->base + JZ_REG_WDT_TIMER_DATA); > writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER); > - writew(clock_div | JZ_WDT_CLOCK_RTC, > - drvdata->base + JZ_REG_WDT_TIMER_CONTROL); > > writeb(0x1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE); > > @@ -114,7 +80,13 @@ static int jz4740_wdt_set_timeout(struct watchdog_device > *wdt_dev, > > static int jz4740_wdt_start(struct watchdog_device *wdt_dev) > { > - jz4740_timer_enable_watchdog(); > + struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev); > + int ret; > + > + ret = clk_prepare_enable(drvdata->clk); > + if (ret) > +