* [PATCH 0/2] watchdog: sunxi: Add A31 watchdog support
@ 2014-09-16 14:42 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: Wim Van Sebroeck, Maxime Ripard
Cc: Chen-Yu Tsai, Guenter Roeck, linux-watchdog, linux-arm-kernel
Hi,
This series adds support for the watchdog hardware found in Allwinner A31
and later SoCs. These new chips have multiple watchdogs, with separate
interrupt lines and registers, but the basic layout and operation is the
same. This series is based on Guenter's restart handler series.
Patch 1 prepares the sunxi-wdt driver for varying register offsets tied to
compatible strings.
Patch 2 adds the sun6i compatible string with accompanying register offset
data.
I tested this series on my Cubieboard (sun4i) and A23 tablet (sun8i), which
has the same watchdog hardware as the A31 (sun6i). This covers the 2 classes
of hardware supported.
After this series is merged, we can remove the power/reset driver for sun6i.
Cheers
ChenYu
Chen-Yu Tsai (2):
watchdog: sunxi: support parameterized compatible strings
watchdog: sunxi: Add A31 watchdog support
drivers/watchdog/sunxi_wdt.c | 111 +++++++++++++++++++++++++++++++++----------
1 file changed, 86 insertions(+), 25 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/2] watchdog: sunxi: Add A31 watchdog support
@ 2014-09-16 14:42 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
This series adds support for the watchdog hardware found in Allwinner A31
and later SoCs. These new chips have multiple watchdogs, with separate
interrupt lines and registers, but the basic layout and operation is the
same. This series is based on Guenter's restart handler series.
Patch 1 prepares the sunxi-wdt driver for varying register offsets tied to
compatible strings.
Patch 2 adds the sun6i compatible string with accompanying register offset
data.
I tested this series on my Cubieboard (sun4i) and A23 tablet (sun8i), which
has the same watchdog hardware as the A31 (sun6i). This covers the 2 classes
of hardware supported.
After this series is merged, we can remove the power/reset driver for sun6i.
Cheers
ChenYu
Chen-Yu Tsai (2):
watchdog: sunxi: support parameterized compatible strings
watchdog: sunxi: Add A31 watchdog support
drivers/watchdog/sunxi_wdt.c | 111 +++++++++++++++++++++++++++++++++----------
1 file changed, 86 insertions(+), 25 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
2014-09-16 14:42 ` Chen-Yu Tsai
@ 2014-09-16 14:42 ` Chen-Yu Tsai
-1 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: Wim Van Sebroeck, Maxime Ripard
Cc: Chen-Yu Tsai, Guenter Roeck, linux-watchdog, linux-arm-kernel
This patch adds support for hardware parameters tied to compatible
strings, so similar hardware can reuse the driver.
This will be used to support the newer watchdog found in A31 and
later SoCs. Differences in the new hardware include separate
interrupt lines for each watchdog, and corresponding interrupt
control/status registers. Watchdog control registers were also
slightly rearranged.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/watchdog/sunxi_wdt.c | 101 ++++++++++++++++++++++++++++++++-----------
1 file changed, 76 insertions(+), 25 deletions(-)
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index 480bb55..a1f7113 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -23,6 +23,7 @@
#include <linux/moduleparam.h>
#include <linux/notifier.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
#include <linux/types.h>
@@ -30,15 +31,11 @@
#define WDT_MAX_TIMEOUT 16
#define WDT_MIN_TIMEOUT 1
-#define WDT_MODE_TIMEOUT(n) ((n) << 3)
-#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
+#define WDT_TIMEOUT_MASK 0x0F
-#define WDT_CTRL 0x00
#define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
-#define WDT_MODE 0x04
#define WDT_MODE_EN (1 << 0)
-#define WDT_MODE_RST_EN (1 << 1)
#define DRV_NAME "sunxi-wdt"
#define DRV_VERSION "1.0"
@@ -46,15 +43,29 @@
static bool nowayout = WATCHDOG_NOWAYOUT;
static unsigned int timeout = WDT_MAX_TIMEOUT;
+/*
+ * This structure stores the register offsets for different variants
+ * of Allwinner's watchdog hardware.
+ */
+struct sunxi_wdt_reg {
+ u8 wdt_ctrl;
+ u8 wdt_cfg;
+ u8 wdt_mode;
+ u8 wdt_timeout_shift;
+ u8 wdt_reset_mask;
+ u8 wdt_reset_val;
+};
+
struct sunxi_wdt_dev {
struct watchdog_device wdt_dev;
void __iomem *wdt_base;
+ const struct sunxi_wdt_reg *wdt_regs;
struct notifier_block restart_handler;
};
/*
* wdt_timeout_map maps the watchdog timer interval value in seconds to
- * the value of the register WDT_MODE bit 3:6
+ * the value of the register WDT_MODE at bits .wdt_timeout_shift ~ +3
*
* [timeout seconds] = register value
*
@@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block *this, unsigned long mode,
struct sunxi_wdt_dev,
restart_handler);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
+ u32 val;
+
+ /* Set system reset function */
+ val = readl(wdt_base + regs->wdt_cfg);
+ val &= ~(regs->wdt_reset_mask);
+ val |= regs->wdt_reset_val;
+ writel(val, wdt_base + regs->wdt_cfg);
- /* Enable timer and set reset bit in the watchdog */
- writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
+ /* Set lowest timeout and enable watchdog */
+ val = readl(wdt_base + regs->wdt_mode);
+ val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
+ val |= WDT_MODE_EN;
+ writel(val, wdt_base + regs->wdt_mode);
/*
* Restart the watchdog. The default (and lowest) interval
* value for the watchdog is 0.5s.
*/
- writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
+ writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
while (1) {
mdelay(5);
- writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
+ val = readl(wdt_base + regs->wdt_mode);
+ val |= WDT_MODE_EN;
+ writel(val, wdt_base + regs->wdt_mode);
}
return NOTIFY_DONE;
}
@@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
- iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
+ writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
return 0;
}
@@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
u32 reg;
if (wdt_timeout_map[timeout] == 0)
@@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
sunxi_wdt->wdt_dev.timeout = timeout;
- reg = ioread32(wdt_base + WDT_MODE);
- reg &= ~WDT_TIMEOUT_MASK;
- reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
- iowrite32(reg, wdt_base + WDT_MODE);
+ reg = readl(wdt_base + regs->wdt_mode);
+ reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
+ reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
+ writel(reg, wdt_base + regs->wdt_mode);
sunxi_wdt_ping(wdt_dev);
@@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
- iowrite32(0, wdt_base + WDT_MODE);
+ writel(0, wdt_base + regs->wdt_mode);
return 0;
}
@@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
u32 reg;
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
int ret;
ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
@@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
if (ret < 0)
return ret;
- reg = ioread32(wdt_base + WDT_MODE);
- reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
- iowrite32(reg, wdt_base + WDT_MODE);
+ /* Set system reset function */
+ reg = readl(wdt_base + regs->wdt_cfg);
+ reg &= ~(regs->wdt_reset_mask);
+ reg |= ~(regs->wdt_reset_val);
+ writel(reg, wdt_base + regs->wdt_cfg);
+
+ /* Enable watchdog */
+ reg = readl(wdt_base + regs->wdt_mode);
+ reg |= WDT_MODE_EN;
+ writel(reg, wdt_base + regs->wdt_mode);
return 0;
}
@@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
.set_timeout = sunxi_wdt_set_timeout,
};
+static const struct sunxi_wdt_reg sun4i_wdt_reg = {
+ .wdt_ctrl = 0x00,
+ .wdt_cfg = 0x04,
+ .wdt_mode = 0x04,
+ .wdt_timeout_shift = 3,
+ .wdt_reset_mask = 0x02,
+ .wdt_reset_val = 0x02,
+};
+
+static const struct of_device_id sunxi_wdt_dt_ids[] = {
+ { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
+
static int sunxi_wdt_probe(struct platform_device *pdev)
{
struct sunxi_wdt_dev *sunxi_wdt;
+ const struct of_device_id *device;
struct resource *res;
int err;
@@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sunxi_wdt);
+ device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
+ if (!device)
+ return -ENODEV;
+
+ sunxi_wdt->wdt_regs = device->data;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(sunxi_wdt->wdt_base))
@@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device *pdev)
sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
}
-static const struct of_device_id sunxi_wdt_dt_ids[] = {
- { .compatible = "allwinner,sun4i-a10-wdt" },
- { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
-
static struct platform_driver sunxi_wdt_driver = {
.probe = sunxi_wdt_probe,
.remove = sunxi_wdt_remove,
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
@ 2014-09-16 14:42 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for hardware parameters tied to compatible
strings, so similar hardware can reuse the driver.
This will be used to support the newer watchdog found in A31 and
later SoCs. Differences in the new hardware include separate
interrupt lines for each watchdog, and corresponding interrupt
control/status registers. Watchdog control registers were also
slightly rearranged.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/watchdog/sunxi_wdt.c | 101 ++++++++++++++++++++++++++++++++-----------
1 file changed, 76 insertions(+), 25 deletions(-)
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index 480bb55..a1f7113 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -23,6 +23,7 @@
#include <linux/moduleparam.h>
#include <linux/notifier.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
#include <linux/types.h>
@@ -30,15 +31,11 @@
#define WDT_MAX_TIMEOUT 16
#define WDT_MIN_TIMEOUT 1
-#define WDT_MODE_TIMEOUT(n) ((n) << 3)
-#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
+#define WDT_TIMEOUT_MASK 0x0F
-#define WDT_CTRL 0x00
#define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
-#define WDT_MODE 0x04
#define WDT_MODE_EN (1 << 0)
-#define WDT_MODE_RST_EN (1 << 1)
#define DRV_NAME "sunxi-wdt"
#define DRV_VERSION "1.0"
@@ -46,15 +43,29 @@
static bool nowayout = WATCHDOG_NOWAYOUT;
static unsigned int timeout = WDT_MAX_TIMEOUT;
+/*
+ * This structure stores the register offsets for different variants
+ * of Allwinner's watchdog hardware.
+ */
+struct sunxi_wdt_reg {
+ u8 wdt_ctrl;
+ u8 wdt_cfg;
+ u8 wdt_mode;
+ u8 wdt_timeout_shift;
+ u8 wdt_reset_mask;
+ u8 wdt_reset_val;
+};
+
struct sunxi_wdt_dev {
struct watchdog_device wdt_dev;
void __iomem *wdt_base;
+ const struct sunxi_wdt_reg *wdt_regs;
struct notifier_block restart_handler;
};
/*
* wdt_timeout_map maps the watchdog timer interval value in seconds to
- * the value of the register WDT_MODE bit 3:6
+ * the value of the register WDT_MODE@bits .wdt_timeout_shift ~ +3
*
* [timeout seconds] = register value
*
@@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block *this, unsigned long mode,
struct sunxi_wdt_dev,
restart_handler);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
+ u32 val;
+
+ /* Set system reset function */
+ val = readl(wdt_base + regs->wdt_cfg);
+ val &= ~(regs->wdt_reset_mask);
+ val |= regs->wdt_reset_val;
+ writel(val, wdt_base + regs->wdt_cfg);
- /* Enable timer and set reset bit in the watchdog */
- writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
+ /* Set lowest timeout and enable watchdog */
+ val = readl(wdt_base + regs->wdt_mode);
+ val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
+ val |= WDT_MODE_EN;
+ writel(val, wdt_base + regs->wdt_mode);
/*
* Restart the watchdog. The default (and lowest) interval
* value for the watchdog is 0.5s.
*/
- writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
+ writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
while (1) {
mdelay(5);
- writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
+ val = readl(wdt_base + regs->wdt_mode);
+ val |= WDT_MODE_EN;
+ writel(val, wdt_base + regs->wdt_mode);
}
return NOTIFY_DONE;
}
@@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
- iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
+ writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
return 0;
}
@@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
u32 reg;
if (wdt_timeout_map[timeout] == 0)
@@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
sunxi_wdt->wdt_dev.timeout = timeout;
- reg = ioread32(wdt_base + WDT_MODE);
- reg &= ~WDT_TIMEOUT_MASK;
- reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
- iowrite32(reg, wdt_base + WDT_MODE);
+ reg = readl(wdt_base + regs->wdt_mode);
+ reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
+ reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
+ writel(reg, wdt_base + regs->wdt_mode);
sunxi_wdt_ping(wdt_dev);
@@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
{
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
- iowrite32(0, wdt_base + WDT_MODE);
+ writel(0, wdt_base + regs->wdt_mode);
return 0;
}
@@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
u32 reg;
struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
void __iomem *wdt_base = sunxi_wdt->wdt_base;
+ const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
int ret;
ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
@@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
if (ret < 0)
return ret;
- reg = ioread32(wdt_base + WDT_MODE);
- reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
- iowrite32(reg, wdt_base + WDT_MODE);
+ /* Set system reset function */
+ reg = readl(wdt_base + regs->wdt_cfg);
+ reg &= ~(regs->wdt_reset_mask);
+ reg |= ~(regs->wdt_reset_val);
+ writel(reg, wdt_base + regs->wdt_cfg);
+
+ /* Enable watchdog */
+ reg = readl(wdt_base + regs->wdt_mode);
+ reg |= WDT_MODE_EN;
+ writel(reg, wdt_base + regs->wdt_mode);
return 0;
}
@@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
.set_timeout = sunxi_wdt_set_timeout,
};
+static const struct sunxi_wdt_reg sun4i_wdt_reg = {
+ .wdt_ctrl = 0x00,
+ .wdt_cfg = 0x04,
+ .wdt_mode = 0x04,
+ .wdt_timeout_shift = 3,
+ .wdt_reset_mask = 0x02,
+ .wdt_reset_val = 0x02,
+};
+
+static const struct of_device_id sunxi_wdt_dt_ids[] = {
+ { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
+
static int sunxi_wdt_probe(struct platform_device *pdev)
{
struct sunxi_wdt_dev *sunxi_wdt;
+ const struct of_device_id *device;
struct resource *res;
int err;
@@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sunxi_wdt);
+ device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
+ if (!device)
+ return -ENODEV;
+
+ sunxi_wdt->wdt_regs = device->data;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(sunxi_wdt->wdt_base))
@@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device *pdev)
sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
}
-static const struct of_device_id sunxi_wdt_dt_ids[] = {
- { .compatible = "allwinner,sun4i-a10-wdt" },
- { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
-
static struct platform_driver sunxi_wdt_driver = {
.probe = sunxi_wdt_probe,
.remove = sunxi_wdt_remove,
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] watchdog: sunxi: Add A31 watchdog support
2014-09-16 14:42 ` Chen-Yu Tsai
@ 2014-09-16 14:42 ` Chen-Yu Tsai
-1 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: Wim Van Sebroeck, Maxime Ripard
Cc: Chen-Yu Tsai, Guenter Roeck, linux-watchdog, linux-arm-kernel
This patch adds support for the watchdog hardware found in A31 and
newer SoCs. This new hardware has registers at different offsets, and
the system reset control has been split out of the "mode" register
into a new "configuration" register.
Differences not supported by this driver include separate interrupt
lines for each watchdog, instead of sharing an interrupt line and
registers with the timer block.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/watchdog/sunxi_wdt.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index a1f7113..b62301e 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -219,8 +219,18 @@ static const struct sunxi_wdt_reg sun4i_wdt_reg = {
.wdt_reset_val = 0x02,
};
+static const struct sunxi_wdt_reg sun6i_wdt_reg = {
+ .wdt_ctrl = 0x10,
+ .wdt_cfg = 0x14,
+ .wdt_mode = 0x18,
+ .wdt_timeout_shift = 4,
+ .wdt_reset_mask = 0x03,
+ .wdt_reset_val = 0x01,
+};
+
static const struct of_device_id sunxi_wdt_dt_ids[] = {
{ .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
+ { .compatible = "allwinner,sun6i-a31-wdt", .data = &sun6i_wdt_reg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] watchdog: sunxi: Add A31 watchdog support
@ 2014-09-16 14:42 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-16 14:42 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for the watchdog hardware found in A31 and
newer SoCs. This new hardware has registers at different offsets, and
the system reset control has been split out of the "mode" register
into a new "configuration" register.
Differences not supported by this driver include separate interrupt
lines for each watchdog, instead of sharing an interrupt line and
registers with the timer block.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/watchdog/sunxi_wdt.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index a1f7113..b62301e 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -219,8 +219,18 @@ static const struct sunxi_wdt_reg sun4i_wdt_reg = {
.wdt_reset_val = 0x02,
};
+static const struct sunxi_wdt_reg sun6i_wdt_reg = {
+ .wdt_ctrl = 0x10,
+ .wdt_cfg = 0x14,
+ .wdt_mode = 0x18,
+ .wdt_timeout_shift = 4,
+ .wdt_reset_mask = 0x03,
+ .wdt_reset_val = 0x01,
+};
+
static const struct of_device_id sunxi_wdt_dt_ids[] = {
{ .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
+ { .compatible = "allwinner,sun6i-a31-wdt", .data = &sun6i_wdt_reg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
2014-09-16 14:42 ` Chen-Yu Tsai
@ 2014-09-19 4:54 ` Guenter Roeck
-1 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2014-09-19 4:54 UTC (permalink / raw)
To: Chen-Yu Tsai, Wim Van Sebroeck, Maxime Ripard
Cc: linux-watchdog, linux-arm-kernel
On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
> This patch adds support for hardware parameters tied to compatible
> strings, so similar hardware can reuse the driver.
>
> This will be used to support the newer watchdog found in A31 and
> later SoCs. Differences in the new hardware include separate
> interrupt lines for each watchdog, and corresponding interrupt
> control/status registers. Watchdog control registers were also
> slightly rearranged.
>
You might also mention that you replace some iowrite32 with writel.
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Have you considered using regmap ? Seems to be an ideal candidate.
> ---
> drivers/watchdog/sunxi_wdt.c | 101 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 76 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index 480bb55..a1f7113 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -23,6 +23,7 @@
> #include <linux/moduleparam.h>
> #include <linux/notifier.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> #include <linux/types.h>
> @@ -30,15 +31,11 @@
>
> #define WDT_MAX_TIMEOUT 16
> #define WDT_MIN_TIMEOUT 1
> -#define WDT_MODE_TIMEOUT(n) ((n) << 3)
> -#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
> +#define WDT_TIMEOUT_MASK 0x0F
>
> -#define WDT_CTRL 0x00
> #define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
>
> -#define WDT_MODE 0x04
> #define WDT_MODE_EN (1 << 0)
> -#define WDT_MODE_RST_EN (1 << 1)
>
> #define DRV_NAME "sunxi-wdt"
> #define DRV_VERSION "1.0"
> @@ -46,15 +43,29 @@
> static bool nowayout = WATCHDOG_NOWAYOUT;
> static unsigned int timeout = WDT_MAX_TIMEOUT;
>
> +/*
> + * This structure stores the register offsets for different variants
> + * of Allwinner's watchdog hardware.
> + */
> +struct sunxi_wdt_reg {
> + u8 wdt_ctrl;
> + u8 wdt_cfg;
> + u8 wdt_mode;
> + u8 wdt_timeout_shift;
> + u8 wdt_reset_mask;
> + u8 wdt_reset_val;
> +};
> +
> struct sunxi_wdt_dev {
> struct watchdog_device wdt_dev;
> void __iomem *wdt_base;
> + const struct sunxi_wdt_reg *wdt_regs;
> struct notifier_block restart_handler;
> };
>
> /*
> * wdt_timeout_map maps the watchdog timer interval value in seconds to
> - * the value of the register WDT_MODE bit 3:6
> + * the value of the register WDT_MODE at bits .wdt_timeout_shift ~ +3
> *
> * [timeout seconds] = register value
> *
> @@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block *this, unsigned long mode,
> struct sunxi_wdt_dev,
> restart_handler);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> + u32 val;
> +
> + /* Set system reset function */
> + val = readl(wdt_base + regs->wdt_cfg);
> + val &= ~(regs->wdt_reset_mask);
> + val |= regs->wdt_reset_val;
> + writel(val, wdt_base + regs->wdt_cfg);
>
> - /* Enable timer and set reset bit in the watchdog */
> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> + /* Set lowest timeout and enable watchdog */
> + val = readl(wdt_base + regs->wdt_mode);
> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> + val |= WDT_MODE_EN;
I think it would make sense to also define WDT_MODE_EN and
WDT_TIMEOUT_MASK as configurable, even if not currently needed.
It is odd to have the shift configurable but not the mask,
and to have the mode register configurable but not the mode value.
> + writel(val, wdt_base + regs->wdt_mode);
>
> /*
> * Restart the watchdog. The default (and lowest) interval
> * value for the watchdog is 0.5s.
> */
> - writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>
> while (1) {
> mdelay(5);
> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> + val = readl(wdt_base + regs->wdt_mode);
> + val |= WDT_MODE_EN;
> + writel(val, wdt_base + regs->wdt_mode);
> }
> return NOTIFY_DONE;
> }
> @@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>
> - iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>
> return 0;
> }
> @@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> u32 reg;
>
> if (wdt_timeout_map[timeout] == 0)
> @@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>
> sunxi_wdt->wdt_dev.timeout = timeout;
>
> - reg = ioread32(wdt_base + WDT_MODE);
> - reg &= ~WDT_TIMEOUT_MASK;
> - reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
> - iowrite32(reg, wdt_base + WDT_MODE);
> + reg = readl(wdt_base + regs->wdt_mode);
> + reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> + reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
> + writel(reg, wdt_base + regs->wdt_mode);
>
> sunxi_wdt_ping(wdt_dev);
>
> @@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>
> - iowrite32(0, wdt_base + WDT_MODE);
> + writel(0, wdt_base + regs->wdt_mode);
>
> return 0;
> }
> @@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> u32 reg;
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> int ret;
>
> ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
> @@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> if (ret < 0)
> return ret;
>
> - reg = ioread32(wdt_base + WDT_MODE);
> - reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
> - iowrite32(reg, wdt_base + WDT_MODE);
> + /* Set system reset function */
> + reg = readl(wdt_base + regs->wdt_cfg);
> + reg &= ~(regs->wdt_reset_mask);
> + reg |= ~(regs->wdt_reset_val);
> + writel(reg, wdt_base + regs->wdt_cfg);
> +
> + /* Enable watchdog */
> + reg = readl(wdt_base + regs->wdt_mode);
> + reg |= WDT_MODE_EN;
> + writel(reg, wdt_base + regs->wdt_mode);
>
> return 0;
> }
> @@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
> .set_timeout = sunxi_wdt_set_timeout,
> };
>
> +static const struct sunxi_wdt_reg sun4i_wdt_reg = {
> + .wdt_ctrl = 0x00,
> + .wdt_cfg = 0x04,
> + .wdt_mode = 0x04,
> + .wdt_timeout_shift = 3,
> + .wdt_reset_mask = 0x02,
> + .wdt_reset_val = 0x02,
> +};
> +
> +static const struct of_device_id sunxi_wdt_dt_ids[] = {
> + { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
> +
> static int sunxi_wdt_probe(struct platform_device *pdev)
> {
> struct sunxi_wdt_dev *sunxi_wdt;
> + const struct of_device_id *device;
> struct resource *res;
> int err;
>
> @@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, sunxi_wdt);
>
> + device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
> + if (!device)
> + return -ENODEV;
> +
> + sunxi_wdt->wdt_regs = device->data;
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(sunxi_wdt->wdt_base))
> @@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device *pdev)
> sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> }
>
> -static const struct of_device_id sunxi_wdt_dt_ids[] = {
> - { .compatible = "allwinner,sun4i-a10-wdt" },
> - { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
> -
> static struct platform_driver sunxi_wdt_driver = {
> .probe = sunxi_wdt_probe,
> .remove = sunxi_wdt_remove,
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
@ 2014-09-19 4:54 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2014-09-19 4:54 UTC (permalink / raw)
To: linux-arm-kernel
On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
> This patch adds support for hardware parameters tied to compatible
> strings, so similar hardware can reuse the driver.
>
> This will be used to support the newer watchdog found in A31 and
> later SoCs. Differences in the new hardware include separate
> interrupt lines for each watchdog, and corresponding interrupt
> control/status registers. Watchdog control registers were also
> slightly rearranged.
>
You might also mention that you replace some iowrite32 with writel.
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Have you considered using regmap ? Seems to be an ideal candidate.
> ---
> drivers/watchdog/sunxi_wdt.c | 101 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 76 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index 480bb55..a1f7113 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -23,6 +23,7 @@
> #include <linux/moduleparam.h>
> #include <linux/notifier.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> #include <linux/types.h>
> @@ -30,15 +31,11 @@
>
> #define WDT_MAX_TIMEOUT 16
> #define WDT_MIN_TIMEOUT 1
> -#define WDT_MODE_TIMEOUT(n) ((n) << 3)
> -#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
> +#define WDT_TIMEOUT_MASK 0x0F
>
> -#define WDT_CTRL 0x00
> #define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
>
> -#define WDT_MODE 0x04
> #define WDT_MODE_EN (1 << 0)
> -#define WDT_MODE_RST_EN (1 << 1)
>
> #define DRV_NAME "sunxi-wdt"
> #define DRV_VERSION "1.0"
> @@ -46,15 +43,29 @@
> static bool nowayout = WATCHDOG_NOWAYOUT;
> static unsigned int timeout = WDT_MAX_TIMEOUT;
>
> +/*
> + * This structure stores the register offsets for different variants
> + * of Allwinner's watchdog hardware.
> + */
> +struct sunxi_wdt_reg {
> + u8 wdt_ctrl;
> + u8 wdt_cfg;
> + u8 wdt_mode;
> + u8 wdt_timeout_shift;
> + u8 wdt_reset_mask;
> + u8 wdt_reset_val;
> +};
> +
> struct sunxi_wdt_dev {
> struct watchdog_device wdt_dev;
> void __iomem *wdt_base;
> + const struct sunxi_wdt_reg *wdt_regs;
> struct notifier_block restart_handler;
> };
>
> /*
> * wdt_timeout_map maps the watchdog timer interval value in seconds to
> - * the value of the register WDT_MODE bit 3:6
> + * the value of the register WDT_MODE at bits .wdt_timeout_shift ~ +3
> *
> * [timeout seconds] = register value
> *
> @@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block *this, unsigned long mode,
> struct sunxi_wdt_dev,
> restart_handler);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> + u32 val;
> +
> + /* Set system reset function */
> + val = readl(wdt_base + regs->wdt_cfg);
> + val &= ~(regs->wdt_reset_mask);
> + val |= regs->wdt_reset_val;
> + writel(val, wdt_base + regs->wdt_cfg);
>
> - /* Enable timer and set reset bit in the watchdog */
> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> + /* Set lowest timeout and enable watchdog */
> + val = readl(wdt_base + regs->wdt_mode);
> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> + val |= WDT_MODE_EN;
I think it would make sense to also define WDT_MODE_EN and
WDT_TIMEOUT_MASK as configurable, even if not currently needed.
It is odd to have the shift configurable but not the mask,
and to have the mode register configurable but not the mode value.
> + writel(val, wdt_base + regs->wdt_mode);
>
> /*
> * Restart the watchdog. The default (and lowest) interval
> * value for the watchdog is 0.5s.
> */
> - writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>
> while (1) {
> mdelay(5);
> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> + val = readl(wdt_base + regs->wdt_mode);
> + val |= WDT_MODE_EN;
> + writel(val, wdt_base + regs->wdt_mode);
> }
> return NOTIFY_DONE;
> }
> @@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>
> - iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>
> return 0;
> }
> @@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> u32 reg;
>
> if (wdt_timeout_map[timeout] == 0)
> @@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>
> sunxi_wdt->wdt_dev.timeout = timeout;
>
> - reg = ioread32(wdt_base + WDT_MODE);
> - reg &= ~WDT_TIMEOUT_MASK;
> - reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
> - iowrite32(reg, wdt_base + WDT_MODE);
> + reg = readl(wdt_base + regs->wdt_mode);
> + reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> + reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
> + writel(reg, wdt_base + regs->wdt_mode);
>
> sunxi_wdt_ping(wdt_dev);
>
> @@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
> {
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>
> - iowrite32(0, wdt_base + WDT_MODE);
> + writel(0, wdt_base + regs->wdt_mode);
>
> return 0;
> }
> @@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> u32 reg;
> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> void __iomem *wdt_base = sunxi_wdt->wdt_base;
> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> int ret;
>
> ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
> @@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
> if (ret < 0)
> return ret;
>
> - reg = ioread32(wdt_base + WDT_MODE);
> - reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
> - iowrite32(reg, wdt_base + WDT_MODE);
> + /* Set system reset function */
> + reg = readl(wdt_base + regs->wdt_cfg);
> + reg &= ~(regs->wdt_reset_mask);
> + reg |= ~(regs->wdt_reset_val);
> + writel(reg, wdt_base + regs->wdt_cfg);
> +
> + /* Enable watchdog */
> + reg = readl(wdt_base + regs->wdt_mode);
> + reg |= WDT_MODE_EN;
> + writel(reg, wdt_base + regs->wdt_mode);
>
> return 0;
> }
> @@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
> .set_timeout = sunxi_wdt_set_timeout,
> };
>
> +static const struct sunxi_wdt_reg sun4i_wdt_reg = {
> + .wdt_ctrl = 0x00,
> + .wdt_cfg = 0x04,
> + .wdt_mode = 0x04,
> + .wdt_timeout_shift = 3,
> + .wdt_reset_mask = 0x02,
> + .wdt_reset_val = 0x02,
> +};
> +
> +static const struct of_device_id sunxi_wdt_dt_ids[] = {
> + { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
> +
> static int sunxi_wdt_probe(struct platform_device *pdev)
> {
> struct sunxi_wdt_dev *sunxi_wdt;
> + const struct of_device_id *device;
> struct resource *res;
> int err;
>
> @@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, sunxi_wdt);
>
> + device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
> + if (!device)
> + return -ENODEV;
> +
> + sunxi_wdt->wdt_regs = device->data;
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(sunxi_wdt->wdt_base))
> @@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device *pdev)
> sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> }
>
> -static const struct of_device_id sunxi_wdt_dt_ids[] = {
> - { .compatible = "allwinner,sun4i-a10-wdt" },
> - { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
> -
> static struct platform_driver sunxi_wdt_driver = {
> .probe = sunxi_wdt_probe,
> .remove = sunxi_wdt_remove,
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
2014-09-19 4:54 ` Guenter Roeck
@ 2014-09-19 14:00 ` Chen-Yu Tsai
-1 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-19 14:00 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Maxime Ripard, linux-watchdog, linux-arm-kernel
Hi,
On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
>>
>> This patch adds support for hardware parameters tied to compatible
>> strings, so similar hardware can reuse the driver.
>>
>> This will be used to support the newer watchdog found in A31 and
>> later SoCs. Differences in the new hardware include separate
>> interrupt lines for each watchdog, and corresponding interrupt
>> control/status registers. Watchdog control registers were also
>> slightly rearranged.
>>
> You might also mention that you replace some iowrite32 with writel.
Ah.... Had that in mind, but forgot about it when finishing the series :(
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
>
> Have you considered using regmap ? Seems to be an ideal candidate.
I don't follow. Do you mean using regmap to share the interrupt registers
with the timer on sun4i? Otherwise I don't really see a difference.
Am I missing something?
>> ---
>> drivers/watchdog/sunxi_wdt.c | 101
>> ++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 76 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
>> index 480bb55..a1f7113 100644
>> --- a/drivers/watchdog/sunxi_wdt.c
>> +++ b/drivers/watchdog/sunxi_wdt.c
>> @@ -23,6 +23,7 @@
>> #include <linux/moduleparam.h>
>> #include <linux/notifier.h>
>> #include <linux/of.h>
>> +#include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/reboot.h>
>> #include <linux/types.h>
>> @@ -30,15 +31,11 @@
>>
>> #define WDT_MAX_TIMEOUT 16
>> #define WDT_MIN_TIMEOUT 1
>> -#define WDT_MODE_TIMEOUT(n) ((n) << 3)
>> -#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
>> +#define WDT_TIMEOUT_MASK 0x0F
>>
>> -#define WDT_CTRL 0x00
>> #define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
>>
>> -#define WDT_MODE 0x04
>> #define WDT_MODE_EN (1 << 0)
>> -#define WDT_MODE_RST_EN (1 << 1)
>>
>> #define DRV_NAME "sunxi-wdt"
>> #define DRV_VERSION "1.0"
>> @@ -46,15 +43,29 @@
>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> static unsigned int timeout = WDT_MAX_TIMEOUT;
>>
>> +/*
>> + * This structure stores the register offsets for different variants
>> + * of Allwinner's watchdog hardware.
>> + */
>> +struct sunxi_wdt_reg {
>> + u8 wdt_ctrl;
>> + u8 wdt_cfg;
>> + u8 wdt_mode;
>> + u8 wdt_timeout_shift;
>> + u8 wdt_reset_mask;
>> + u8 wdt_reset_val;
>> +};
>> +
>> struct sunxi_wdt_dev {
>> struct watchdog_device wdt_dev;
>> void __iomem *wdt_base;
>> + const struct sunxi_wdt_reg *wdt_regs;
>> struct notifier_block restart_handler;
>> };
>>
>> /*
>> * wdt_timeout_map maps the watchdog timer interval value in seconds to
>> - * the value of the register WDT_MODE bit 3:6
>> + * the value of the register WDT_MODE at bits .wdt_timeout_shift ~ +3
>> *
>> * [timeout seconds] = register value
>> *
>> @@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block
>> *this, unsigned long mode,
>> struct
>> sunxi_wdt_dev,
>> restart_handler);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> + u32 val;
>> +
>> + /* Set system reset function */
>> + val = readl(wdt_base + regs->wdt_cfg);
>> + val &= ~(regs->wdt_reset_mask);
>> + val |= regs->wdt_reset_val;
>> + writel(val, wdt_base + regs->wdt_cfg);
>>
>> - /* Enable timer and set reset bit in the watchdog */
>> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
>> + /* Set lowest timeout and enable watchdog */
>> + val = readl(wdt_base + regs->wdt_mode);
>> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> + val |= WDT_MODE_EN;
>
>
> I think it would make sense to also define WDT_MODE_EN and
> WDT_TIMEOUT_MASK as configurable, even if not currently needed.
> It is odd to have the shift configurable but not the mask,
> and to have the mode register configurable but not the mode value.
I think keeping these values constants clearly shows that they are
the same and shared among the hardware. Making them configurable
shouldn't impose much penalty, but I'd prefer to keep them as is
until we need to change them.
>> + writel(val, wdt_base + regs->wdt_mode);
>>
>> /*
>> * Restart the watchdog. The default (and lowest) interval
>> * value for the watchdog is 0.5s.
>> */
>> - writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
>> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>>
>> while (1) {
>> mdelay(5);
>> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base +
>> WDT_MODE);
>> + val = readl(wdt_base + regs->wdt_mode);
>> + val |= WDT_MODE_EN;
>> + writel(val, wdt_base + regs->wdt_mode);
>> }
>> return NOTIFY_DONE;
>> }
>> @@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device
>> *wdt_dev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>>
>> - iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
>> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>>
>> return 0;
>> }
>> @@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct
>> watchdog_device *wdt_dev,
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> u32 reg;
>>
>> if (wdt_timeout_map[timeout] == 0)
>> @@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct
>> watchdog_device *wdt_dev,
>>
>> sunxi_wdt->wdt_dev.timeout = timeout;
>>
>> - reg = ioread32(wdt_base + WDT_MODE);
>> - reg &= ~WDT_TIMEOUT_MASK;
>> - reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
>> - iowrite32(reg, wdt_base + WDT_MODE);
>> + reg = readl(wdt_base + regs->wdt_mode);
>> + reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> + reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
>> + writel(reg, wdt_base + regs->wdt_mode);
>>
>> sunxi_wdt_ping(wdt_dev);
>>
>> @@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device
>> *wdt_dev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>>
>> - iowrite32(0, wdt_base + WDT_MODE);
>> + writel(0, wdt_base + regs->wdt_mode);
>>
>> return 0;
>> }
>> @@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device
>> *wdt_dev)
>> u32 reg;
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> int ret;
>>
>> ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
>> @@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device
>> *wdt_dev)
>> if (ret < 0)
>> return ret;
>>
>> - reg = ioread32(wdt_base + WDT_MODE);
>> - reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
>> - iowrite32(reg, wdt_base + WDT_MODE);
>> + /* Set system reset function */
>> + reg = readl(wdt_base + regs->wdt_cfg);
>> + reg &= ~(regs->wdt_reset_mask);
>> + reg |= ~(regs->wdt_reset_val);
>> + writel(reg, wdt_base + regs->wdt_cfg);
>> +
>> + /* Enable watchdog */
>> + reg = readl(wdt_base + regs->wdt_mode);
>> + reg |= WDT_MODE_EN;
>> + writel(reg, wdt_base + regs->wdt_mode);
>>
>> return 0;
>> }
>> @@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
>> .set_timeout = sunxi_wdt_set_timeout,
>> };
>>
>> +static const struct sunxi_wdt_reg sun4i_wdt_reg = {
>> + .wdt_ctrl = 0x00,
>> + .wdt_cfg = 0x04,
>> + .wdt_mode = 0x04,
>> + .wdt_timeout_shift = 3,
>> + .wdt_reset_mask = 0x02,
>> + .wdt_reset_val = 0x02,
>> +};
>> +
>> +static const struct of_device_id sunxi_wdt_dt_ids[] = {
>> + { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg
>> },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
>> +
>> static int sunxi_wdt_probe(struct platform_device *pdev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt;
>> + const struct of_device_id *device;
>> struct resource *res;
>> int err;
>>
>> @@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, sunxi_wdt);
>>
>> + device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
>> + if (!device)
>> + return -ENODEV;
>> +
>> + sunxi_wdt->wdt_regs = device->data;
>> +
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(sunxi_wdt->wdt_base))
>> @@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device
>> *pdev)
>> sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
>> }
>>
>> -static const struct of_device_id sunxi_wdt_dt_ids[] = {
>> - { .compatible = "allwinner,sun4i-a10-wdt" },
>> - { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
>> -
>> static struct platform_driver sunxi_wdt_driver = {
>> .probe = sunxi_wdt_probe,
>> .remove = sunxi_wdt_remove,
>>
Thanks
ChenYu
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
@ 2014-09-19 14:00 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-19 14:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
>>
>> This patch adds support for hardware parameters tied to compatible
>> strings, so similar hardware can reuse the driver.
>>
>> This will be used to support the newer watchdog found in A31 and
>> later SoCs. Differences in the new hardware include separate
>> interrupt lines for each watchdog, and corresponding interrupt
>> control/status registers. Watchdog control registers were also
>> slightly rearranged.
>>
> You might also mention that you replace some iowrite32 with writel.
Ah.... Had that in mind, but forgot about it when finishing the series :(
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
>
> Have you considered using regmap ? Seems to be an ideal candidate.
I don't follow. Do you mean using regmap to share the interrupt registers
with the timer on sun4i? Otherwise I don't really see a difference.
Am I missing something?
>> ---
>> drivers/watchdog/sunxi_wdt.c | 101
>> ++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 76 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
>> index 480bb55..a1f7113 100644
>> --- a/drivers/watchdog/sunxi_wdt.c
>> +++ b/drivers/watchdog/sunxi_wdt.c
>> @@ -23,6 +23,7 @@
>> #include <linux/moduleparam.h>
>> #include <linux/notifier.h>
>> #include <linux/of.h>
>> +#include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/reboot.h>
>> #include <linux/types.h>
>> @@ -30,15 +31,11 @@
>>
>> #define WDT_MAX_TIMEOUT 16
>> #define WDT_MIN_TIMEOUT 1
>> -#define WDT_MODE_TIMEOUT(n) ((n) << 3)
>> -#define WDT_TIMEOUT_MASK WDT_MODE_TIMEOUT(0x0F)
>> +#define WDT_TIMEOUT_MASK 0x0F
>>
>> -#define WDT_CTRL 0x00
>> #define WDT_CTRL_RELOAD ((1 << 0) | (0x0a57 << 1))
>>
>> -#define WDT_MODE 0x04
>> #define WDT_MODE_EN (1 << 0)
>> -#define WDT_MODE_RST_EN (1 << 1)
>>
>> #define DRV_NAME "sunxi-wdt"
>> #define DRV_VERSION "1.0"
>> @@ -46,15 +43,29 @@
>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> static unsigned int timeout = WDT_MAX_TIMEOUT;
>>
>> +/*
>> + * This structure stores the register offsets for different variants
>> + * of Allwinner's watchdog hardware.
>> + */
>> +struct sunxi_wdt_reg {
>> + u8 wdt_ctrl;
>> + u8 wdt_cfg;
>> + u8 wdt_mode;
>> + u8 wdt_timeout_shift;
>> + u8 wdt_reset_mask;
>> + u8 wdt_reset_val;
>> +};
>> +
>> struct sunxi_wdt_dev {
>> struct watchdog_device wdt_dev;
>> void __iomem *wdt_base;
>> + const struct sunxi_wdt_reg *wdt_regs;
>> struct notifier_block restart_handler;
>> };
>>
>> /*
>> * wdt_timeout_map maps the watchdog timer interval value in seconds to
>> - * the value of the register WDT_MODE bit 3:6
>> + * the value of the register WDT_MODE at bits .wdt_timeout_shift ~ +3
>> *
>> * [timeout seconds] = register value
>> *
>> @@ -82,19 +93,32 @@ static int sunxi_restart_handle(struct notifier_block
>> *this, unsigned long mode,
>> struct
>> sunxi_wdt_dev,
>> restart_handler);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> + u32 val;
>> +
>> + /* Set system reset function */
>> + val = readl(wdt_base + regs->wdt_cfg);
>> + val &= ~(regs->wdt_reset_mask);
>> + val |= regs->wdt_reset_val;
>> + writel(val, wdt_base + regs->wdt_cfg);
>>
>> - /* Enable timer and set reset bit in the watchdog */
>> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
>> + /* Set lowest timeout and enable watchdog */
>> + val = readl(wdt_base + regs->wdt_mode);
>> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> + val |= WDT_MODE_EN;
>
>
> I think it would make sense to also define WDT_MODE_EN and
> WDT_TIMEOUT_MASK as configurable, even if not currently needed.
> It is odd to have the shift configurable but not the mask,
> and to have the mode register configurable but not the mode value.
I think keeping these values constants clearly shows that they are
the same and shared among the hardware. Making them configurable
shouldn't impose much penalty, but I'd prefer to keep them as is
until we need to change them.
>> + writel(val, wdt_base + regs->wdt_mode);
>>
>> /*
>> * Restart the watchdog. The default (and lowest) interval
>> * value for the watchdog is 0.5s.
>> */
>> - writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
>> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>>
>> while (1) {
>> mdelay(5);
>> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base +
>> WDT_MODE);
>> + val = readl(wdt_base + regs->wdt_mode);
>> + val |= WDT_MODE_EN;
>> + writel(val, wdt_base + regs->wdt_mode);
>> }
>> return NOTIFY_DONE;
>> }
>> @@ -103,8 +127,9 @@ static int sunxi_wdt_ping(struct watchdog_device
>> *wdt_dev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>>
>> - iowrite32(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
>> + writel(WDT_CTRL_RELOAD, wdt_base + regs->wdt_ctrl);
>>
>> return 0;
>> }
>> @@ -114,6 +139,7 @@ static int sunxi_wdt_set_timeout(struct
>> watchdog_device *wdt_dev,
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> u32 reg;
>>
>> if (wdt_timeout_map[timeout] == 0)
>> @@ -121,10 +147,10 @@ static int sunxi_wdt_set_timeout(struct
>> watchdog_device *wdt_dev,
>>
>> sunxi_wdt->wdt_dev.timeout = timeout;
>>
>> - reg = ioread32(wdt_base + WDT_MODE);
>> - reg &= ~WDT_TIMEOUT_MASK;
>> - reg |= WDT_MODE_TIMEOUT(wdt_timeout_map[timeout]);
>> - iowrite32(reg, wdt_base + WDT_MODE);
>> + reg = readl(wdt_base + regs->wdt_mode);
>> + reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> + reg |= wdt_timeout_map[timeout] << regs->wdt_timeout_shift;
>> + writel(reg, wdt_base + regs->wdt_mode);
>>
>> sunxi_wdt_ping(wdt_dev);
>>
>> @@ -135,8 +161,9 @@ static int sunxi_wdt_stop(struct watchdog_device
>> *wdt_dev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>>
>> - iowrite32(0, wdt_base + WDT_MODE);
>> + writel(0, wdt_base + regs->wdt_mode);
>>
>> return 0;
>> }
>> @@ -146,6 +173,7 @@ static int sunxi_wdt_start(struct watchdog_device
>> *wdt_dev)
>> u32 reg;
>> struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
>> void __iomem *wdt_base = sunxi_wdt->wdt_base;
>> + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
>> int ret;
>>
>> ret = sunxi_wdt_set_timeout(&sunxi_wdt->wdt_dev,
>> @@ -153,9 +181,16 @@ static int sunxi_wdt_start(struct watchdog_device
>> *wdt_dev)
>> if (ret < 0)
>> return ret;
>>
>> - reg = ioread32(wdt_base + WDT_MODE);
>> - reg |= (WDT_MODE_RST_EN | WDT_MODE_EN);
>> - iowrite32(reg, wdt_base + WDT_MODE);
>> + /* Set system reset function */
>> + reg = readl(wdt_base + regs->wdt_cfg);
>> + reg &= ~(regs->wdt_reset_mask);
>> + reg |= ~(regs->wdt_reset_val);
>> + writel(reg, wdt_base + regs->wdt_cfg);
>> +
>> + /* Enable watchdog */
>> + reg = readl(wdt_base + regs->wdt_mode);
>> + reg |= WDT_MODE_EN;
>> + writel(reg, wdt_base + regs->wdt_mode);
>>
>> return 0;
>> }
>> @@ -175,9 +210,25 @@ static const struct watchdog_ops sunxi_wdt_ops = {
>> .set_timeout = sunxi_wdt_set_timeout,
>> };
>>
>> +static const struct sunxi_wdt_reg sun4i_wdt_reg = {
>> + .wdt_ctrl = 0x00,
>> + .wdt_cfg = 0x04,
>> + .wdt_mode = 0x04,
>> + .wdt_timeout_shift = 3,
>> + .wdt_reset_mask = 0x02,
>> + .wdt_reset_val = 0x02,
>> +};
>> +
>> +static const struct of_device_id sunxi_wdt_dt_ids[] = {
>> + { .compatible = "allwinner,sun4i-a10-wdt", .data = &sun4i_wdt_reg
>> },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
>> +
>> static int sunxi_wdt_probe(struct platform_device *pdev)
>> {
>> struct sunxi_wdt_dev *sunxi_wdt;
>> + const struct of_device_id *device;
>> struct resource *res;
>> int err;
>>
>> @@ -187,6 +238,12 @@ static int sunxi_wdt_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, sunxi_wdt);
>>
>> + device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
>> + if (!device)
>> + return -ENODEV;
>> +
>> + sunxi_wdt->wdt_regs = device->data;
>> +
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> sunxi_wdt->wdt_base = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(sunxi_wdt->wdt_base))
>> @@ -242,12 +299,6 @@ static void sunxi_wdt_shutdown(struct platform_device
>> *pdev)
>> sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
>> }
>>
>> -static const struct of_device_id sunxi_wdt_dt_ids[] = {
>> - { .compatible = "allwinner,sun4i-a10-wdt" },
>> - { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, sunxi_wdt_dt_ids);
>> -
>> static struct platform_driver sunxi_wdt_driver = {
>> .probe = sunxi_wdt_probe,
>> .remove = sunxi_wdt_remove,
>>
Thanks
ChenYu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
2014-09-19 14:00 ` Chen-Yu Tsai
@ 2014-09-19 15:40 ` Guenter Roeck
-1 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2014-09-19 15:40 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Wim Van Sebroeck, Maxime Ripard, linux-watchdog, linux-arm-kernel
On Fri, Sep 19, 2014 at 10:00:36PM +0800, Chen-Yu Tsai wrote:
> Hi,
>
> On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
> >>
> >> This patch adds support for hardware parameters tied to compatible
> >> strings, so similar hardware can reuse the driver.
> >>
> >> This will be used to support the newer watchdog found in A31 and
> >> later SoCs. Differences in the new hardware include separate
> >> interrupt lines for each watchdog, and corresponding interrupt
> >> control/status registers. Watchdog control registers were also
> >> slightly rearranged.
> >>
> > You might also mention that you replace some iowrite32 with writel.
>
> Ah.... Had that in mind, but forgot about it when finishing the series :(
>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> >
> > Have you considered using regmap ? Seems to be an ideal candidate.
>
> I don't follow. Do you mean using regmap to share the interrupt registers
> with the timer on sun4i? Otherwise I don't really see a difference.
>
Use regmap instead of direct writel. Nice thing about it is that regmap offers
bit manipulation functions, so you could replace the read / mask / modify /write
in a single regmap function call. See imx2_wdt.c for an example how it is used.
> Am I missing something?
>
Not really. Just a suggestion.
> >>
> >> - /* Enable timer and set reset bit in the watchdog */
> >> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> >> + /* Set lowest timeout and enable watchdog */
> >> + val = readl(wdt_base + regs->wdt_mode);
> >> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> >> + val |= WDT_MODE_EN;
> >
> >
> > I think it would make sense to also define WDT_MODE_EN and
> > WDT_TIMEOUT_MASK as configurable, even if not currently needed.
> > It is odd to have the shift configurable but not the mask,
> > and to have the mode register configurable but not the mode value.
>
> I think keeping these values constants clearly shows that they are
> the same and shared among the hardware. Making them configurable
> shouldn't impose much penalty, but I'd prefer to keep them as is
> until we need to change them.
>
Ok, guess we have a different philosophy ;-). I'll leave it up to you.
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
@ 2014-09-19 15:40 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2014-09-19 15:40 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 19, 2014 at 10:00:36PM +0800, Chen-Yu Tsai wrote:
> Hi,
>
> On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
> >>
> >> This patch adds support for hardware parameters tied to compatible
> >> strings, so similar hardware can reuse the driver.
> >>
> >> This will be used to support the newer watchdog found in A31 and
> >> later SoCs. Differences in the new hardware include separate
> >> interrupt lines for each watchdog, and corresponding interrupt
> >> control/status registers. Watchdog control registers were also
> >> slightly rearranged.
> >>
> > You might also mention that you replace some iowrite32 with writel.
>
> Ah.... Had that in mind, but forgot about it when finishing the series :(
>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> >
> > Have you considered using regmap ? Seems to be an ideal candidate.
>
> I don't follow. Do you mean using regmap to share the interrupt registers
> with the timer on sun4i? Otherwise I don't really see a difference.
>
Use regmap instead of direct writel. Nice thing about it is that regmap offers
bit manipulation functions, so you could replace the read / mask / modify /write
in a single regmap function call. See imx2_wdt.c for an example how it is used.
> Am I missing something?
>
Not really. Just a suggestion.
> >>
> >> - /* Enable timer and set reset bit in the watchdog */
> >> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
> >> + /* Set lowest timeout and enable watchdog */
> >> + val = readl(wdt_base + regs->wdt_mode);
> >> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> >> + val |= WDT_MODE_EN;
> >
> >
> > I think it would make sense to also define WDT_MODE_EN and
> > WDT_TIMEOUT_MASK as configurable, even if not currently needed.
> > It is odd to have the shift configurable but not the mask,
> > and to have the mode register configurable but not the mode value.
>
> I think keeping these values constants clearly shows that they are
> the same and shared among the hardware. Making them configurable
> shouldn't impose much penalty, but I'd prefer to keep them as is
> until we need to change them.
>
Ok, guess we have a different philosophy ;-). I'll leave it up to you.
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
2014-09-19 15:40 ` Guenter Roeck
@ 2014-09-21 15:49 ` Chen-Yu Tsai
-1 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-21 15:49 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Maxime Ripard, linux-watchdog, linux-arm-kernel
On Fri, Sep 19, 2014 at 11:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Sep 19, 2014 at 10:00:36PM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
>> >>
>> >> This patch adds support for hardware parameters tied to compatible
>> >> strings, so similar hardware can reuse the driver.
>> >>
>> >> This will be used to support the newer watchdog found in A31 and
>> >> later SoCs. Differences in the new hardware include separate
>> >> interrupt lines for each watchdog, and corresponding interrupt
>> >> control/status registers. Watchdog control registers were also
>> >> slightly rearranged.
>> >>
>> > You might also mention that you replace some iowrite32 with writel.
>>
>> Ah.... Had that in mind, but forgot about it when finishing the series :(
>>
>> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >
>> >
>> > Have you considered using regmap ? Seems to be an ideal candidate.
>>
>> I don't follow. Do you mean using regmap to share the interrupt registers
>> with the timer on sun4i? Otherwise I don't really see a difference.
>>
> Use regmap instead of direct writel. Nice thing about it is that regmap offers
> bit manipulation functions, so you could replace the read / mask / modify /write
> in a single regmap function call. See imx2_wdt.c for an example how it is used.
>
>> Am I missing something?
>>
> Not really. Just a suggestion.
As the register range and offsets are configurable, using regmap might add
more code than it saves. I'll keep the it the way it is for now.
>> >>
>> >> - /* Enable timer and set reset bit in the watchdog */
>> >> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
>> >> + /* Set lowest timeout and enable watchdog */
>> >> + val = readl(wdt_base + regs->wdt_mode);
>> >> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> >> + val |= WDT_MODE_EN;
>> >
>> >
>> > I think it would make sense to also define WDT_MODE_EN and
>> > WDT_TIMEOUT_MASK as configurable, even if not currently needed.
>> > It is odd to have the shift configurable but not the mask,
>> > and to have the mode register configurable but not the mode value.
>>
>> I think keeping these values constants clearly shows that they are
>> the same and shared among the hardware. Making them configurable
>> shouldn't impose much penalty, but I'd prefer to keep them as is
>> until we need to change them.
>>
> Ok, guess we have a different philosophy ;-). I'll leave it up to you.
:)
Thanks!
ChenYu
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings
@ 2014-09-21 15:49 ` Chen-Yu Tsai
0 siblings, 0 replies; 14+ messages in thread
From: Chen-Yu Tsai @ 2014-09-21 15:49 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 19, 2014 at 11:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Sep 19, 2014 at 10:00:36PM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Fri, Sep 19, 2014 at 12:54 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 09/16/2014 07:42 AM, Chen-Yu Tsai wrote:
>> >>
>> >> This patch adds support for hardware parameters tied to compatible
>> >> strings, so similar hardware can reuse the driver.
>> >>
>> >> This will be used to support the newer watchdog found in A31 and
>> >> later SoCs. Differences in the new hardware include separate
>> >> interrupt lines for each watchdog, and corresponding interrupt
>> >> control/status registers. Watchdog control registers were also
>> >> slightly rearranged.
>> >>
>> > You might also mention that you replace some iowrite32 with writel.
>>
>> Ah.... Had that in mind, but forgot about it when finishing the series :(
>>
>> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >
>> >
>> > Have you considered using regmap ? Seems to be an ideal candidate.
>>
>> I don't follow. Do you mean using regmap to share the interrupt registers
>> with the timer on sun4i? Otherwise I don't really see a difference.
>>
> Use regmap instead of direct writel. Nice thing about it is that regmap offers
> bit manipulation functions, so you could replace the read / mask / modify /write
> in a single regmap function call. See imx2_wdt.c for an example how it is used.
>
>> Am I missing something?
>>
> Not really. Just a suggestion.
As the register range and offsets are configurable, using regmap might add
more code than it saves. I'll keep the it the way it is for now.
>> >>
>> >> - /* Enable timer and set reset bit in the watchdog */
>> >> - writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
>> >> + /* Set lowest timeout and enable watchdog */
>> >> + val = readl(wdt_base + regs->wdt_mode);
>> >> + val &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
>> >> + val |= WDT_MODE_EN;
>> >
>> >
>> > I think it would make sense to also define WDT_MODE_EN and
>> > WDT_TIMEOUT_MASK as configurable, even if not currently needed.
>> > It is odd to have the shift configurable but not the mask,
>> > and to have the mode register configurable but not the mode value.
>>
>> I think keeping these values constants clearly shows that they are
>> the same and shared among the hardware. Making them configurable
>> shouldn't impose much penalty, but I'd prefer to keep them as is
>> until we need to change them.
>>
> Ok, guess we have a different philosophy ;-). I'll leave it up to you.
:)
Thanks!
ChenYu
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-09-21 15:49 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-16 14:42 [PATCH 0/2] watchdog: sunxi: Add A31 watchdog support Chen-Yu Tsai
2014-09-16 14:42 ` Chen-Yu Tsai
2014-09-16 14:42 ` [PATCH 1/2] watchdog: sunxi: support parameterized compatible strings Chen-Yu Tsai
2014-09-16 14:42 ` Chen-Yu Tsai
2014-09-19 4:54 ` Guenter Roeck
2014-09-19 4:54 ` Guenter Roeck
2014-09-19 14:00 ` Chen-Yu Tsai
2014-09-19 14:00 ` Chen-Yu Tsai
2014-09-19 15:40 ` Guenter Roeck
2014-09-19 15:40 ` Guenter Roeck
2014-09-21 15:49 ` Chen-Yu Tsai
2014-09-21 15:49 ` Chen-Yu Tsai
2014-09-16 14:42 ` [PATCH 2/2] watchdog: sunxi: Add A31 watchdog support Chen-Yu Tsai
2014-09-16 14:42 ` Chen-Yu Tsai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.